Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix force-renewing revoked on-demand certs #166

Merged
merged 3 commits into from Feb 1, 2022
Merged

Fix force-renewing revoked on-demand certs #166

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ca8d93
Fix force-renewing revoked on-demand certs
mholt Jan 31, 2022
670d8a5
One more fix for on-demand logic of revoked certs
mholt Feb 1, 2022
627a034
OCSP revocation checks at startup, too
mholt Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions certificates.go
View file
Open in desktop
Expand Up @@ -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.
Expand Down Expand Up @@ -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))
}
Expand Down Expand Up @@ -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
}
Expand Down
51 changes: 30 additions & 21 deletions config.go
View file
Open in desktop
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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.
Expand Down
19 changes: 9 additions & 10 deletions handshake.go
View file
Open in desktop
Expand Up @@ -472,7 +472,7 @@ func (cfg *Config) handshakeMaintenance(ctx context.Context, hello *tls.ClientHe

// 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.
Expand All @@ -490,14 +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.
if certShouldBeForceRenewed(cert, ocspResp) {
return cfg.renewDynamicCertificate(ctx, hello, 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, nil)
return cfg.renewDynamicCertificate(ctx, hello, cert)
}

return cert, nil
Expand All @@ -510,17 +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 is being renewed because it was revoked and not because it was
// expiring, set ocspResp to not nil, and if its status is Revoked, the renewal will be
// forced.
// 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, ocspResp *ocsp.Response) (Certificate, error) {
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 := ocspResp != nil && ocspResp.Status == ocsp.Revoked
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
Expand Down Expand Up @@ -608,7 +607,7 @@ func (cfg *Config) renewDynamicCertificate(ctx context.Context, hello *tls.Clien
var err error

if revoked {
newCert, err = cfg.forceRenew(ctx, log, currentCert, ocspResp)
newCert, err = cfg.forceRenew(ctx, log, currentCert)
} else {
mholt marked this conversation as resolved.
Show resolved Hide resolved
err = cfg.RenewCertAsync(ctx, name, false)
if err == nil {
Expand Down
50 changes: 25 additions & 25 deletions maintain.go
View file
Open in desktop
Expand Up @@ -295,8 +295,7 @@ func (certCache *Cache) updateOCSPStaples(ctx context.Context) {
lastNextUpdate time.Time
}
type renewQueueEntry struct {
oldCert Certificate
ocspResp *ocsp.Response
oldCert Certificate
}
updated := make(map[string]ocspUpdate)
var updateQueue []updateQueueEntry // certs that need a refreshed staple
Expand All @@ -313,7 +312,7 @@ func (certCache *Cache) updateOCSPStaples(ctx context.Context) {
var lastNextUpdate time.Time
if cert.ocsp != nil {
lastNextUpdate = cert.ocsp.NextUpdate
if freshOCSP(cert.ocsp) {
if cert.ocsp.Status != ocsp.Unknown && freshOCSP(cert.ocsp) {
continue // no need to update staple if ours is still fresh
}
}
Expand Down Expand Up @@ -345,8 +344,8 @@ func (certCache *Cache) updateOCSPStaples(ctx context.Context) {
continue
}

ocspResp, err := stapleOCSP(cfg.OCSP, cfg.Storage, &cert, nil)
if err != nil || ocspResp == nil {
err = stapleOCSP(cfg.OCSP, cfg.Storage, &cert, nil)
if err != nil {
if cert.ocsp != nil {
// if there was no staple before, that's fine; otherwise we should log the error
if logger != nil {
Expand All @@ -360,8 +359,9 @@ func (certCache *Cache) updateOCSPStaples(ctx context.Context) {

// By this point, we've obtained the latest OCSP response.
// If there was no staple before, or if the response is updated, make
// sure we apply the update to all names on the certificate.
if cert.ocsp != nil && (lastNextUpdate.IsZero() || lastNextUpdate != cert.ocsp.NextUpdate) {
// sure we apply the update to all names on the certificate if
// the status is still Good.
if cert.ocsp != nil && cert.ocsp.Status == ocsp.Good && (lastNextUpdate.IsZero() || lastNextUpdate != cert.ocsp.NextUpdate) {
if logger != nil {
logger.Info("advancing OCSP staple",
zap.Strings("identifiers", cert.Names),
Expand All @@ -371,11 +371,11 @@ func (certCache *Cache) updateOCSPStaples(ctx context.Context) {
updated[certHash] = ocspUpdate{rawBytes: cert.Certificate.OCSPStaple, parsed: cert.ocsp}
}

// If a managed certificate was revoked, we should attempt to replace it with a new one.
if certShouldBeForceRenewed(cert, ocspResp) {
// If a managed certificate was revoked, we should attempt to replace
// it with a new one.
if certShouldBeForceRenewed(cert) {
renewQueue = append(renewQueue, renewQueueEntry{
oldCert: cert,
ocspResp: ocspResp,
oldCert: cert,
})
configs[cert.Names[0]] = cfg
}
Expand All @@ -395,7 +395,7 @@ func (certCache *Cache) updateOCSPStaples(ctx context.Context) {
// Crucially, this happens OUTSIDE a lock on the certCache.
for _, renew := range renewQueue {
cfg := configs[renew.oldCert.Names[0]]
_, err := cfg.forceRenew(ctx, logger, renew.oldCert, renew.ocspResp)
_, err := cfg.forceRenew(ctx, logger, renew.oldCert)
if err != nil && logger != nil {
logger.Info("forcefully renewing certificate due to REVOKED status",
zap.Strings("identifiers", renew.oldCert.Names),
Expand Down Expand Up @@ -548,21 +548,18 @@ func deleteExpiredCerts(ctx context.Context, storage Storage, gracePeriod time.D
return nil
}

// forceRenew forcefully renews cert and replaces it in the cache, and returns the new certificate. It is
// intended for use primarily in the case of cert revocation. The latest OCSP response must be passed in,
// since stapleOCSP() only staples Good responses to a Certificate. This MUST NOT be called within a lock
// on cfg.certCacheMu.
func (cfg *Config) forceRenew(ctx context.Context, logger *zap.Logger, cert Certificate, ocspResp *ocsp.Response) (Certificate, error) {
// forceRenew forcefully renews cert and replaces it in the cache, and returns the new certificate. It is intended
// for use primarily in the case of cert revocation. This MUST NOT be called within a lock on cfg.certCacheMu.
func (cfg *Config) forceRenew(ctx context.Context, logger *zap.Logger, cert Certificate) (Certificate, error) {
if logger != nil {
if ocspResp.Status == ocsp.Revoked {
if cert.ocsp != nil && cert.ocsp.Status == ocsp.Revoked {
logger.Warn("OCSP status for managed certificate is REVOKED; attempting to replace with new certificate",
zap.Strings("identifiers", cert.Names),
zap.Time("expiration", cert.Leaf.NotAfter))
} else {
logger.Warn("forcefully renewing certificate",
zap.Strings("identifiers", cert.Names),
zap.Time("expiration", cert.Leaf.NotAfter),
zap.Int("ocsp_status", ocspResp.Status))
zap.Time("expiration", cert.Leaf.NotAfter))
}
}

Expand All @@ -576,7 +573,7 @@ func (cfg *Config) forceRenew(ctx context.Context, logger *zap.Logger, cert Cert
// of a prior key, we can't do a "renew" to replace the cert if we need a
// new key, so we'll have to do an obtain instead
var obtainInsteadOfRenew bool
if ocspResp.RevocationReason == acme.ReasonKeyCompromise {
if cert.ocsp != nil && cert.ocsp.RevocationReason == acme.ReasonKeyCompromise {
err := cfg.moveCompromisedPrivateKey(cert, logger)
if err != nil && logger != nil {
logger.Error("could not remove compromised private key from use",
Expand All @@ -597,7 +594,7 @@ func (cfg *Config) forceRenew(ctx context.Context, logger *zap.Logger, cert Cert
err = cfg.RenewCertAsync(ctx, renewName, true)
}
if err != nil {
if ocspResp.Status == ocsp.Revoked {
if cert.ocsp != nil && cert.ocsp.Status == ocsp.Revoked {
// probably better to not serve a revoked certificate at all
if logger != nil {
logger.Error("unable to obtain new to certificate after OCSP status of REVOKED; removing from cache",
Expand Down Expand Up @@ -646,9 +643,12 @@ func (cfg *Config) moveCompromisedPrivateKey(cert Certificate, logger *zap.Logge
}

// certShouldBeForceRenewed returns true if cert should be forcefully renewed
// according to ocspResp (i.e. if it is Revoked).
func certShouldBeForceRenewed(cert Certificate, ocspResp *ocsp.Response) bool {
return cert.managed && len(cert.Names) > 0 && ocspResp.Status == ocsp.Revoked
// (like if it is revoked according to its OCSP response).
func certShouldBeForceRenewed(cert Certificate) bool {
return cert.managed &&
len(cert.Names) > 0 &&
cert.ocsp != nil &&
cert.ocsp.Status == ocsp.Revoked
}

const (
Expand Down
46 changes: 25 additions & 21 deletions ocsp.go
View file
Open in desktop
Expand Up @@ -33,15 +33,15 @@ import (
// bundle; otherwise the DER-encoded cert will have to be PEM-encoded.
// If you don't have the PEM blocks already, just pass in nil.
//
// Errors here are not necessarily fatal, it could just be that the
// certificate doesn't have an issuer URL. This function may return
// both nil values if OCSP stapling is disabled according to ocspConfig.
// If successful, the OCSP response will be set to cert's ocsp field,
// regardless of the OCSP status. It is only stapled, however, if the
// status is Good.
//
// If a status was received, it returns that status. Note that the
// returned status is not always stapled to the certificate.
func stapleOCSP(ocspConfig OCSPConfig, storage Storage, cert *Certificate, pemBundle []byte) (*ocsp.Response, error) {
// Errors here are not necessarily fatal, it could just be that the
// certificate doesn't have an issuer URL.
func stapleOCSP(ocspConfig OCSPConfig, storage Storage, cert *Certificate, pemBundle []byte) error {
if ocspConfig.DisableStapling {
return nil, nil
return nil
}

if pemBundle == nil {
Expand Down Expand Up @@ -93,33 +93,37 @@ func stapleOCSP(ocspConfig OCSPConfig, storage Storage, cert *Certificate, pemBu
// not contain a link to an OCSP server. But we should log it anyway.
// There's nothing else we can do to get OCSP for this certificate,
// so we can return here with the error.
return nil, fmt.Errorf("no OCSP stapling for %v: %v", cert.Names, ocspErr)
return fmt.Errorf("no OCSP stapling for %v: %v", cert.Names, ocspErr)
}
gotNewOCSP = true
}

// By now, we should have a response. If good, staple it to
// the certificate. If the OCSP response was not loaded from
// storage, we persist it for next time.
if ocspResp.NextUpdate.After(cert.Leaf.NotAfter) {
// uh oh, this OCSP response expires AFTER the certificate does, that's kinda bogus.
// it was the reason a lot of Symantec-validated sites (not Caddy) went down
// in October 2017. https://twitter.com/mattiasgeniar/status/919432824708648961
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tweet got deleted it seems, unfortunately.

return fmt.Errorf("invalid: OCSP response for %v valid after certificate expiration (%s)",
cert.Names, cert.Leaf.NotAfter.Sub(ocspResp.NextUpdate))
}

// Attach the latest OCSP response to the certificate; this is NOT the same
// as stapling it, which we do below only if the status is Good, but it is
// useful to keep with the cert in order to act on it later (like if Revoked).
cert.ocsp = ocspResp

// If the response is good, staple it to the certificate. If the OCSP
// response was not loaded from storage, we persist it for next time.
if ocspResp.Status == ocsp.Good {
if ocspResp.NextUpdate.After(cert.Leaf.NotAfter) {
// uh oh, this OCSP response expires AFTER the certificate does, that's kinda bogus.
// it was the reason a lot of Symantec-validated sites (not Caddy) went down
// in October 2017. https://twitter.com/mattiasgeniar/status/919432824708648961
return ocspResp, fmt.Errorf("invalid: OCSP response for %v valid after certificate expiration (%s)",
cert.Names, cert.Leaf.NotAfter.Sub(ocspResp.NextUpdate))
}
cert.Certificate.OCSPStaple = ocspBytes
cert.ocsp = ocspResp
if gotNewOCSP {
err := storage.Store(ocspStapleKey, ocspBytes)
if err != nil {
return ocspResp, fmt.Errorf("unable to write OCSP staple file for %v: %v", cert.Names, err)
return fmt.Errorf("unable to write OCSP staple file for %v: %v", cert.Names, err)
}
}
}

return ocspResp, nil
return nil
}

// getOCSPForCert takes a PEM encoded cert or cert bundle returning the raw OCSP response,
Expand Down