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

@@ -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) {

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

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.
+
+			// 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()
-				delete(certCache, "")
-				certCacheMu.Unlock()
+			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 {