Merge pull request #1366 from mholt/tls-sni-renew-fix

tls: Fix background certificate renewals that use TLS-SNI challenge

caddytls/certificates.go

@@ -229,7 +229,7 @@ func cacheCertificate(cert Certificate) {
 	}
 	certCacheMu.Lock()
 	if _, ok := certCache[""]; !ok {
-		// use as default - must be *appended* to list, or bad things happen!
+		// use as default - must be *appended* to end of list, or bad things happen!
 		cert.Names = append(cert.Names, "")
 		certCache[""] = cert
 	}

caddytls/maintain.go

@@ -65,7 +65,7 @@ func maintainAssets(stopChan chan struct{}) {
 
 // RenewManagedCertificates renews managed certificates.
 func RenewManagedCertificates(allowPrompts bool) (err error) {
-	var renewed, deleted []Certificate
+	var renewQueue, deleteQueue []Certificate
 	visitedNames := make(map[string]struct{})
 
 	certCacheMu.RLock()
@@ -77,7 +77,7 @@ func RenewManagedCertificates(allowPrompts bool) (err error) {
 		// the list of names on this cert should never be empty...
 		if cert.Names == nil || len(cert.Names) == 0 {
 			log.Printf("[WARNING] Certificate keyed by '%s' has no names: %v - removing from cache", name, cert.Names)
-			deleted = append(deleted, cert)
+			deleteQueue = append(deleteQueue, cert)
 			continue
 		}
 
@@ -99,11 +99,21 @@ func RenewManagedCertificates(allowPrompts bool) (err error) {
 				continue
 			}
 
+			// queue for renewal when we aren't in a read lock anymore
+			// (the TLS-SNI challenge will need a write lock in order to
+			// present the certificate, so we renew outside of read lock)
+			renewQueue = append(renewQueue, cert)
+		}
+	}
+	certCacheMu.RUnlock()
+
+	// Perform renewals that are queued
+	for _, cert := range renewQueue {
 		// Get the name which we should use to renew this certificate;
 		// we only support managing certificates with one name per cert,
 		// so this should be easy. We can't rely on cert.Config.Hostname
 		// because it may be a wildcard value from the Caddyfile (e.g.
-			// *.something.com) which, as of 2016, is not supported by ACME.
+		// *.something.com) which, as of Jan. 2017, is not supported by ACME.
 		var renewName string
 		for _, name := range cert.Names {
 			if name != "" {
@@ -112,38 +122,33 @@ func RenewManagedCertificates(allowPrompts bool) (err error) {
 			}
 		}
 
+		// perform renewal
 		err := cert.Config.RenewCert(renewName, allowPrompts)
 		if err != nil {
-				if allowPrompts && timeLeft < 0 {
+			if allowPrompts && cert.NotAfter.Sub(time.Now().UTC()) < 0 {
 				// Certificate renewal failed, the operator is present, and the certificate
 				// is already expired; we should stop immediately and return the error. Note
 				// that we used to do this any time a renewal failed at startup. However,
 				// after discussion in https://github.com/mholt/caddy/issues/642 we decided to
 				// only stop startup if the certificate is expired. We still log the error
 				// otherwise. I'm not sure how permanent the change in #642 will be...
-					certCacheMu.RUnlock()
+				// TODO: Get rid of the expiration check... always break on error.
 				return err
 			}
 			log.Printf("[ERROR] %v", err)
 			if cert.Config.OnDemand {
-					deleted = append(deleted, cert)
+				deleteQueue = append(deleteQueue, cert)
 			}
 		} else {
-				renewed = append(renewed, cert)
+			// successful renewal, so update in-memory cache by loading
-			}
+			// renewed certificate so it will be used with handshakes
-		}
+			// TODO: Not until CA has valid OCSP response ready for the new cert... sigh.
-	}
-	certCacheMu.RUnlock()
-
-	// Apply changes to the cache
-	for _, cert := range renewed {
-		// TODO: Don't do these until we have valid OCSP for the new cert
 			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. When we cache
+				// renewed on every scan, which is too often. The next cert
-			// this certificate in a moment, it will be the default again.
+				// to be cached (probably this one) will become the default.
 				certCacheMu.Lock()
 				delete(certCache, "")
 				certCacheMu.Unlock()
@@ -156,7 +161,10 @@ func RenewManagedCertificates(allowPrompts bool) (err error) {
 				log.Printf("[ERROR] %v", err)
 			}
 		}
-	for _, cert := range deleted {
+	}
+
+	// Apply queued deletion changes to the cache
+	for _, cert := range deleteQueue {
 		certCacheMu.Lock()
 		for _, name := range cert.Names {
 			delete(certCache, name)