Fix force-renewing revoked on-demand certs (#166)

* Fix force-renewing revoked on-demand certs

Follow-up to 9245be5

* One more fix for on-demand logic of revoked certs

* OCSP revocation checks at startup, too

Required significant refactoring, hope it works.
Yet again way too late at night for this...

certificates.go

@@ -42,6 +42,9 @@ type Certificate struct {
 	Tags []string
 
 	// OCSP contains the certificate's parsed OCSP response.
+	// It is not necessarily the response that is stapled
+	// (e.g. if the status is not Good), it is simply the
+	// most recent OCSP response we have for this certificate.
 	ocsp *ocsp.Response
 
 	// The hex-encoded hash of this cert's chain's bytes.
@@ -159,7 +162,7 @@ func (cfg *Config) CacheUnmanagedTLSCertificate(tlsCert tls.Certificate, tags []
 	if err != nil {
 		return err
 	}
-	_, err = stapleOCSP(cfg.OCSP, cfg.Storage, &cert, nil)
+	err = stapleOCSP(cfg.OCSP, cfg.Storage, &cert, nil)
 	if err != nil && cfg.Logger != nil {
 		cfg.Logger.Warn("stapling OCSP", zap.Error(err))
 	}
@@ -207,9 +210,9 @@ func (cfg Config) makeCertificateWithOCSP(certPEMBlock, keyPEMBlock []byte) (Cer
 	if err != nil {
 		return cert, err
 	}
-	_, err = stapleOCSP(cfg.OCSP, cfg.Storage, &cert, certPEMBlock)
+	err = stapleOCSP(cfg.OCSP, cfg.Storage, &cert, certPEMBlock)
 	if err != nil && cfg.Logger != nil {
-		cfg.Logger.Warn("stapling OCSP", zap.Error(err))
+		cfg.Logger.Warn("stapling OCSP", zap.Error(err), zap.Strings("identifiers", cert.Names))
 	}
 	return cert, nil
 }
@@ -310,17 +313,17 @@ func (cfg *Config) managedCertInStorageExpiresSoon(cert Certificate) (bool, erro
 // on oldCert into the cache, from storage. This also replaces the old certificate
 // with the new one, so that all configurations that used the old cert now point
 // to the new cert. It assumes that the new certificate for oldCert.Names[0] is
-// already in storage.
-func (cfg *Config) reloadManagedCertificate(oldCert Certificate) error {
+// already in storage. It returns the newly-loaded certificate if successful.
+func (cfg *Config) reloadManagedCertificate(oldCert Certificate) (Certificate, error) {
 	if cfg.Logger != nil {
 		cfg.Logger.Info("reloading managed certificate", zap.Strings("identifiers", oldCert.Names))
 	}
 	newCert, err := cfg.loadManagedCertificate(oldCert.Names[0])
 	if err != nil {
-		return fmt.Errorf("loading managed certificate for %v from storage: %v", oldCert.Names, err)
+		return Certificate{}, fmt.Errorf("loading managed certificate for %v from storage: %v", oldCert.Names, err)
 	}
 	cfg.certCache.replaceCertificate(oldCert, newCert)
-	return nil
+	return newCert, nil
 }
 
 // SubjectQualifiesForCert returns true if subj is a name which,

config.go

@@ -35,6 +35,7 @@ import (
 	"github.com/mholt/acmez"
 	"github.com/mholt/acmez/acme"
 	"go.uber.org/zap"
+	"golang.org/x/crypto/ocsp"
 	"golang.org/x/net/idna"
 )
 
@@ -358,33 +359,41 @@ func (cfg *Config) manageOne(ctx context.Context, domainName string, async bool)
 		return obtain()
 	}
 
-	// for an existing certificate, make sure it is renewed
+	// for an existing certificate, make sure it is renewed; or if it is revoked,
+	// force a renewal even if it's not expiring
 	renew := func() error {
-		var err error
-		if async {
-			err = cfg.RenewCertAsync(ctx, domainName, false)
-		} else {
-			err = cfg.RenewCertSync(ctx, domainName, false)
-		}
-		if err != nil {
-			return fmt.Errorf("%s: renewing certificate: %w", domainName, err)
+		// first, ensure status is not revoked (it was just refreshed in CacheManagedCertificate above)
+		if !cert.Expired() && cert.ocsp != nil && cert.ocsp.Status == ocsp.Revoked {
+			_, err = cfg.forceRenew(ctx, cfg.Logger, cert)
+			return err
 		}
-		// successful renewal, so update in-memory cache
-		err = cfg.reloadManagedCertificate(cert)
-		if err != nil {
-			return fmt.Errorf("%s: reloading renewed certificate into memory: %v", domainName, err)
+
+		// otherwise, simply renew the certificate if needed
+		if cert.NeedsRenewal(cfg) {
+			var err error
+			if async {
+				err = cfg.RenewCertAsync(ctx, domainName, false)
+			} else {
+				err = cfg.RenewCertSync(ctx, domainName, false)
+			}
+			if err != nil {
+				return fmt.Errorf("%s: renewing certificate: %w", domainName, err)
+			}
+			// successful renewal, so update in-memory cache
+			_, err = cfg.reloadManagedCertificate(cert)
+			if err != nil {
+				return fmt.Errorf("%s: reloading renewed certificate into memory: %v", domainName, err)
+			}
 		}
+
 		return nil
 	}
-	if cert.NeedsRenewal(cfg) {
-		if async {
-			jm.Submit(cfg.Logger, "renew_"+domainName, renew)
-			return nil
-		}
-		return renew()
-	}
 
-	return nil
+	if async {
+		jm.Submit(cfg.Logger, "renew_"+domainName, renew)
+		return nil
+	}
+	return renew()
 }
 
 // Unmanage causes the certificates for domainNames to stop being managed.

handshake.go

@@ -25,6 +25,7 @@ import (
 
 	"github.com/mholt/acmez"
 	"go.uber.org/zap"
+	"golang.org/x/crypto/ocsp"
 )
 
 // GetCertificate gets a certificate to satisfy clientHello. In getting
@@ -469,14 +470,9 @@ func (cfg *Config) obtainOnDemandCertificate(ctx context.Context, hello *tls.Cli
 func (cfg *Config) handshakeMaintenance(ctx context.Context, hello *tls.ClientHelloInfo, cert Certificate) (Certificate, error) {
 	log := loggerNamed(cfg.Logger, "on_demand")
 
-	// Check cert expiration
-	if currentlyInRenewalWindow(cert.Leaf.NotBefore, cert.Leaf.NotAfter, cfg.RenewalWindowRatio) {
-		return cfg.renewDynamicCertificate(ctx, hello, cert)
-	}
-
 	// Check OCSP staple validity
 	if cert.ocsp != nil && !freshOCSP(cert.ocsp) {
-		ocspResp, err := stapleOCSP(cfg.OCSP, cfg.Storage, &cert, nil)
+		err := stapleOCSP(cfg.OCSP, cfg.Storage, &cert, nil)
 		if err != nil {
 			// An error with OCSP stapling is not the end of the world, and in fact, is
 			// quite common considering not all certs have issuer URLs that support it.
@@ -494,7 +490,14 @@ func (cfg *Config) handshakeMaintenance(ctx context.Context, hello *tls.ClientHe
 
 		// We attempt to replace any certificates that were revoked.
 		// Crucially, this happens OUTSIDE a lock on the certCache.
-		cfg.forceRenewIfCertRevoked(ctx, log, cert, ocspResp)
+		if certShouldBeForceRenewed(cert) {
+			return cfg.renewDynamicCertificate(ctx, hello, cert)
+		}
+	}
+
+	// Check cert expiration
+	if currentlyInRenewalWindow(cert.Leaf.NotBefore, cert.Leaf.NotAfter, cfg.RenewalWindowRatio) {
+		return cfg.renewDynamicCertificate(ctx, hello, cert)
 	}
 
 	return cert, nil
@@ -507,12 +510,16 @@ func (cfg *Config) handshakeMaintenance(ctx context.Context, hello *tls.ClientHe
 // and the renewal will happen in the background; otherwise this blocks until the
 // certificate has been renewed, and returns the renewed certificate.
 //
+// If the certificate's OCSP status (currentCert.ocsp) is Revoked, it will be forcefully
+// renewed even if it is not expiring.
+//
 // This function is safe for use by multiple concurrent goroutines.
 func (cfg *Config) renewDynamicCertificate(ctx context.Context, hello *tls.ClientHelloInfo, currentCert Certificate) (Certificate, error) {
 	log := loggerNamed(cfg.Logger, "on_demand")
 
 	name := cfg.getNameFromClientHello(hello)
 	timeLeft := time.Until(currentCert.Leaf.NotAfter)
+	revoked := currentCert.ocsp != nil && currentCert.ocsp.Status == ocsp.Revoked
 
 	getCertWithoutReobtaining := func() (Certificate, error) {
 		// very important to set the obtainIfNecessary argument to false, so we don't repeat this infinitely
@@ -526,9 +533,10 @@ func (cfg *Config) renewDynamicCertificate(ctx context.Context, hello *tls.Clien
 		// lucky us -- another goroutine is already renewing the certificate
 		obtainCertWaitChansMu.Unlock()
 
-		if timeLeft > 0 {
-			// the current certificate hasn't expired, and another goroutine is already
-			// renewing it, so we might as well serve what we have without blocking
+		// the current certificate hasn't expired, and another goroutine is already
+		// renewing it, so we might as well serve what we have without blocking, UNLESS
+		// we're forcing renewal, in which case the current certificate is not usable
+		if timeLeft > 0 && !revoked {
 			if log != nil {
 				log.Debug("certificate expires soon but is already being renewed; serving current certificate",
 					zap.Strings("subjects", currentCert.Names),
@@ -538,12 +546,13 @@ func (cfg *Config) renewDynamicCertificate(ctx context.Context, hello *tls.Clien
 		}
 
 		// otherwise, we'll have to wait for the renewal to finish so we don't serve
-		// an expired certificate
+		// a revoked or expired certificate
 
 		if log != nil {
 			log.Debug("certificate has expired, but is already being renewed; waiting for renewal to complete",
 				zap.Strings("subjects", currentCert.Names),
-				zap.Time("expired", currentCert.Leaf.NotAfter))
+				zap.Time("expired", currentCert.Leaf.NotAfter),
+				zap.Bool("revoked", revoked))
 		}
 
 		// TODO: see if we can get a proper context in here, for true cancellation
@@ -575,7 +584,8 @@ func (cfg *Config) renewDynamicCertificate(ctx context.Context, hello *tls.Clien
 			zap.String("server_name", name),
 			zap.Strings("subjects", currentCert.Names),
 			zap.Time("expiration", currentCert.Leaf.NotAfter),
-			zap.Duration("remaining", timeLeft))
+			zap.Duration("remaining", timeLeft),
+			zap.Bool("revoked", revoked))
 	}
 
 	// Make sure a certificate for this name should be obtained on-demand
@@ -593,19 +603,26 @@ func (cfg *Config) renewDynamicCertificate(ctx context.Context, hello *tls.Clien
 	renewAndReload := func(ctx context.Context, cancel context.CancelFunc) (Certificate, error) {
 		defer cancel()
 
-		err = cfg.RenewCertAsync(ctx, name, false)
-		if err == nil {
-			// 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.
-			newCert, err := cfg.CacheManagedCertificate(name)
-			if err != nil {
-				if log != nil {
-					log.Error("loading renewed certificate", zap.String("server_name", name), zap.Error(err))
+		var newCert Certificate
+		var err error
+
+		if revoked {
+			newCert, err = cfg.forceRenew(ctx, log, currentCert)
+		} else {
+			err = cfg.RenewCertAsync(ctx, name, false)
+			if err == nil {
+				// 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.
+				newCert, err = cfg.CacheManagedCertificate(name)
+				if err != nil {
+					if log != nil {
+						log.Error("loading renewed certificate", zap.String("server_name", name), zap.Error(err))
+					}
+				} else {
+					// replace the old certificate with the new one
+					cfg.certCache.replaceCertificate(currentCert, newCert)
 				}
-			} else {
-				// replace the old certificate with the new one
-				cfg.certCache.replaceCertificate(currentCert, newCert)
 			}
 		}
 
@@ -615,7 +632,13 @@ func (cfg *Config) renewDynamicCertificate(ctx context.Context, hello *tls.Clien
 		unblockWaiters()
 
 		if err != nil {
-			return Certificate{}, err
+			if log != nil {
+				log.Error("renewing and reloading certificate",
+					zap.String("server_name", name),
+					zap.Error(err),
+					zap.Bool("forced", revoked))
+			}
+			return newCert, err
 		}
 
 		return getCertWithoutReobtaining()