From 46ae4a66524221bbf157c84ad453198c7f0facab Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Mon, 11 Sep 2017 12:37:42 -0600 Subject: [PATCH] tls: Remove expiring certificates from cache and load renewed ones Renewed certificates would not be reloaded into the cache because their names conflict with names of certificates already in the cache; this was intentional when loading new certs to avoid confusion, but is problematic when renewing, since the old certificate doesn't get evicted from the cache. (Oops.) Here, I remedy this situation by explicitly deleting the old cert from the cache before adding the renewed one back in. --- caddytls/certificates.go | 6 ++++-- caddytls/handshake.go | 34 ++++++++++++++++++++++++++++------ caddytls/maintain.go | 27 +++++++++++++++++---------- 3 files changed, 49 insertions(+), 18 deletions(-) diff --git a/caddytls/certificates.go b/caddytls/certificates.go index ed26c33e..4fe4ed88 100644 --- a/caddytls/certificates.go +++ b/caddytls/certificates.go @@ -227,8 +227,10 @@ func fillCertFromLeaf(cert *Certificate, tlsCert tls.Certificate) error { // full, random entries are deleted until there is room to map all the // names on the certificate. // -// This certificate will be keyed to the names in cert.Names. Any name -// that is already a key in the cache will be replaced with this cert. +// This certificate will be keyed to the names in cert.Names. Any names +// already used as a cache key will NOT be replaced by this cert; in +// other words, no overlap is allowed, and this certificate will not +// service those pre-existing names. // // This function is safe for concurrent use. func cacheCertificate(cert Certificate) { diff --git a/caddytls/handshake.go b/caddytls/handshake.go index 52d0a0a7..ce3c2fb3 100644 --- a/caddytls/handshake.go +++ b/caddytls/handshake.go @@ -246,7 +246,7 @@ func (cfg *Config) handshakeMaintenance(name string, cert Certificate) (Certific timeLeft := cert.NotAfter.Sub(time.Now().UTC()) if timeLeft < RenewDurationBefore { log.Printf("[INFO] Certificate for %v expires in %v; attempting renewal", cert.Names, timeLeft) - return cfg.renewDynamicCertificate(name) + return cfg.renewDynamicCertificate(name, cert) } // Check OCSP staple validity @@ -269,12 +269,12 @@ func (cfg *Config) handshakeMaintenance(name string, cert Certificate) (Certific } // renewDynamicCertificate renews the certificate for name using cfg. It returns the -// certificate to use and an error, if any. currentCert may be returned even if an -// error occurs, since we perform renewals before they expire and it may still be -// usable. name should already be lower-cased before calling this function. +// certificate to use and an error, if any. name should already be lower-cased before +// calling this function. name is the name obtained directly from the handshake's +// ClientHello. // // This function is safe for use by multiple concurrent goroutines. -func (cfg *Config) renewDynamicCertificate(name string) (Certificate, error) { +func (cfg *Config) renewDynamicCertificate(name string, currentCert Certificate) (Certificate, error) { obtainCertWaitChansMu.Lock() wait, ok := obtainCertWaitChans[name] if ok { @@ -290,9 +290,31 @@ func (cfg *Config) renewDynamicCertificate(name string) (Certificate, error) { obtainCertWaitChans[name] = wait obtainCertWaitChansMu.Unlock() - // do the renew + // do the renew and reload the certificate log.Printf("[INFO] Renewing certificate for %s", name) err := cfg.RenewCert(name, false) + if err == nil { + // immediately flush this certificate from the cache so + // the name doesn't overlap when we try to replace it, + // which would fail, because overlapping existing cert + // names isn't allowed + certCacheMu.Lock() + for _, certName := range currentCert.Names { + delete(certCache, certName) + } + certCacheMu.Unlock() + + // even though the recursive nature of the dynamic cert loading + // would just call this function anyway, we do it here to + // make the replacement as atomic as possible. (TODO: similar + // to the note in maintain.go, it'd be nice if the clearing of + // the cache entries above and this load function were truly + // atomic...) + _, err := currentCert.Config.CacheManagedCertificate(name) + if err != nil { + log.Printf("[ERROR] loading renewed certificate: %v", err) + } + } // immediately unblock anyone waiting for it; doing this in // a defer would risk deadlock because of the recursive call diff --git a/caddytls/maintain.go b/caddytls/maintain.go index b495a752..b8ba16ef 100644 --- a/caddytls/maintain.go +++ b/caddytls/maintain.go @@ -70,7 +70,8 @@ func maintainAssets(stopChan chan struct{}) { } } -// RenewManagedCertificates renews managed certificates. +// RenewManagedCertificates renews managed certificates, +// including ones loaded on-demand. func RenewManagedCertificates(allowPrompts bool) (err error) { var renewQueue, deleteQueue []Certificate visitedNames := make(map[string]struct{}) @@ -147,21 +148,27 @@ func RenewManagedCertificates(allowPrompts bool) (err error) { } log.Printf("[ERROR] %v", err) if cert.Config.OnDemand { + // loaded dynamically, removed dynamically deleteQueue = append(deleteQueue, cert) } } else { // successful renewal, so update in-memory cache by loading // renewed certificate so it will be used with handshakes - if cert.Names[len(cert.Names)-1] == "" { - // Special case: This is the default certificate. We must - // flush it out of the cache so that we no longer point to - // the old, un-renewed certificate. Otherwise it will be - // renewed on every scan, which is too often. The next cert - // to be cached (probably this one) will become the default. - certCacheMu.Lock() - delete(certCache, "") - certCacheMu.Unlock() + + // we must delete all the names this cert services from the cache + // so that we can replace the certificate, because replacing names + // already in the cache is not allowed, to avoid later conflicts + // with renewals. + // TODO: It would be nice if this whole operation were idempotent; + // i.e. a thread-safe function to replace a certificate in the cache, + // see also handshake.go for on-demand maintenance. + certCacheMu.Lock() + for _, name := range cert.Names { + delete(certCache, name) } + certCacheMu.Unlock() + + // put the certificate in the cache _, err := cert.Config.CacheManagedCertificate(cert.Names[0]) if err != nil { if allowPrompts {