From f42a2084bf5e00e4ce4e53115ce5510ff07cff4c Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Thu, 7 Nov 2024 21:25:33 -0500 Subject: [PATCH 1/4] Quiet OCSP warnings if the cert has a short lifetime --- certificates.go | 22 ++++++++++++++++++++-- handshake.go | 17 +++++++++++------ 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/certificates.go b/certificates.go index 2965712a..82395e33 100644 --- a/certificates.go +++ b/certificates.go @@ -188,6 +188,14 @@ func (cert Certificate) Expired() bool { return time.Now().After(expiresAt(cert.Leaf)) } +// Lifetime returns the duration of the certificate's validity. +func (cert Certificate) Lifetime() time.Duration { + if cert.Leaf == nil || cert.Leaf.NotAfter.IsZero() { + return 0 + } + return cert.Leaf.NotAfter.Sub(expiresAt(cert.Leaf)) +} + // currentlyInRenewalWindow returns true if the current time is within // (or after) the renewal window, according to the given start/end // dates and the ratio of the renewal window. If true is returned, @@ -330,7 +338,12 @@ func (cfg *Config) CacheUnmanagedTLSCertificate(ctx context.Context, tlsCert tls } err = stapleOCSP(ctx, cfg.OCSP, cfg.Storage, &cert, nil) if err != nil { - cfg.Logger.Warn("stapling OCSP", zap.Error(err)) + // If the certificate's lifetime is less than a week, then + // it's a short-validity cert and we can ignore not having + // OCSP. Revocation of short certs is mostly pointless. + if cert.Lifetime() > 7*24*time.Hour { + cfg.Logger.Warn("stapling OCSP", zap.Error(err)) + } } cfg.emit(ctx, "cached_unmanaged_cert", map[string]any{"sans": cert.Names}) cert.Tags = tags @@ -379,7 +392,12 @@ func (cfg Config) makeCertificateWithOCSP(ctx context.Context, certPEMBlock, key } err = stapleOCSP(ctx, cfg.OCSP, cfg.Storage, &cert, certPEMBlock) if err != nil { - cfg.Logger.Warn("stapling OCSP", zap.Error(err), zap.Strings("identifiers", cert.Names)) + // If the certificate's lifetime is less than a week, then + // it's a short-validity cert and we can ignore not having + // OCSP. Revocation of short certs is mostly pointless. + if cert.Lifetime() > 7*24*time.Hour { + cfg.Logger.Warn("stapling OCSP", zap.Error(err), zap.Strings("identifiers", cert.Names)) + } } return cert, nil } diff --git a/handshake.go b/handshake.go index 201c6c5a..18371ac1 100644 --- a/handshake.go +++ b/handshake.go @@ -568,12 +568,17 @@ func (cfg *Config) handshakeMaintenance(ctx context.Context, hello *tls.ClientHe err := stapleOCSP(ctx, 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. - logger.Warn("stapling OCSP", - zap.String("server_name", hello.ServerName), - zap.Strings("sans", cert.Names), - zap.Error(err)) + // If the certificate's lifetime is less than a week, then + // it's a short-validity cert and we can ignore not having + // OCSP. Revocation of short certs is mostly pointless. + if cert.Lifetime() > 7*24*time.Hour { + // 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. + logger.Warn("stapling OCSP", + zap.String("server_name", hello.ServerName), + zap.Strings("sans", cert.Names), + zap.Error(err)) + } } else { logger.Debug("successfully stapled new OCSP response", zap.Strings("identifiers", cert.Names), From 293668b3fd53f12dd680a0b9a3d5e09e7f0fdba3 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Fri, 8 Nov 2024 14:38:34 -0500 Subject: [PATCH 2/4] Quier differently --- certificates.go | 14 ++------------ handshake.go | 17 ++++++----------- ocsp.go | 11 ++++++++--- 3 files changed, 16 insertions(+), 26 deletions(-) diff --git a/certificates.go b/certificates.go index 82395e33..c8d0c4ba 100644 --- a/certificates.go +++ b/certificates.go @@ -338,12 +338,7 @@ func (cfg *Config) CacheUnmanagedTLSCertificate(ctx context.Context, tlsCert tls } err = stapleOCSP(ctx, cfg.OCSP, cfg.Storage, &cert, nil) if err != nil { - // If the certificate's lifetime is less than a week, then - // it's a short-validity cert and we can ignore not having - // OCSP. Revocation of short certs is mostly pointless. - if cert.Lifetime() > 7*24*time.Hour { - cfg.Logger.Warn("stapling OCSP", zap.Error(err)) - } + cfg.Logger.Warn("stapling OCSP", zap.Error(err)) } cfg.emit(ctx, "cached_unmanaged_cert", map[string]any{"sans": cert.Names}) cert.Tags = tags @@ -392,12 +387,7 @@ func (cfg Config) makeCertificateWithOCSP(ctx context.Context, certPEMBlock, key } err = stapleOCSP(ctx, cfg.OCSP, cfg.Storage, &cert, certPEMBlock) if err != nil { - // If the certificate's lifetime is less than a week, then - // it's a short-validity cert and we can ignore not having - // OCSP. Revocation of short certs is mostly pointless. - if cert.Lifetime() > 7*24*time.Hour { - cfg.Logger.Warn("stapling OCSP", zap.Error(err), zap.Strings("identifiers", cert.Names)) - } + cfg.Logger.Warn("stapling OCSP", zap.Error(err), zap.Strings("identifiers", cert.Names)) } return cert, nil } diff --git a/handshake.go b/handshake.go index 18371ac1..201c6c5a 100644 --- a/handshake.go +++ b/handshake.go @@ -568,17 +568,12 @@ func (cfg *Config) handshakeMaintenance(ctx context.Context, hello *tls.ClientHe err := stapleOCSP(ctx, cfg.OCSP, cfg.Storage, &cert, nil) if err != nil { - // If the certificate's lifetime is less than a week, then - // it's a short-validity cert and we can ignore not having - // OCSP. Revocation of short certs is mostly pointless. - if cert.Lifetime() > 7*24*time.Hour { - // 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. - logger.Warn("stapling OCSP", - zap.String("server_name", hello.ServerName), - zap.Strings("sans", cert.Names), - zap.Error(err)) - } + // 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. + logger.Warn("stapling OCSP", + zap.String("server_name", hello.ServerName), + zap.Strings("sans", cert.Names), + zap.Error(err)) } else { logger.Debug("successfully stapled new OCSP response", zap.Strings("identifiers", cert.Names), diff --git a/ocsp.go b/ocsp.go index fe6dbb8f..d164b1e7 100644 --- a/ocsp.go +++ b/ocsp.go @@ -93,11 +93,16 @@ func stapleOCSP(ctx context.Context, ocspConfig OCSPConfig, storage Storage, cer // then we need to request it from the OCSP responder if ocspResp == nil || len(ocspBytes) == 0 { ocspBytes, ocspResp, ocspErr = getOCSPForCert(ocspConfig, pemBundle) + // An error here is not a problem because a certificate + // may simply not contain a link to an OCSP server. if ocspErr != nil { - // An error here is not a problem because a certificate may simply - // not contain a link to an OCSP server. But we should log it anyway. + // For short-lived certificates, this is fine and we can ignore + // logging because OCSP doesn't make much sense for them anyway. + if cert.Lifetime() > 7*24*time.Hour { + return nil + } // There's nothing else we can do to get OCSP for this certificate, - // so we can return here with the error. + // so we can return here with the error to warn about it. return fmt.Errorf("no OCSP stapling for %v: %w", cert.Names, ocspErr) } gotNewOCSP = true From e94db3944a0ec276a50ec634db99bfd58c525344 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Fri, 8 Nov 2024 14:49:33 -0500 Subject: [PATCH 3/4] Fix condition Co-authored-by: Matt Holt --- ocsp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ocsp.go b/ocsp.go index d164b1e7..c87a560f 100644 --- a/ocsp.go +++ b/ocsp.go @@ -98,7 +98,7 @@ func stapleOCSP(ctx context.Context, ocspConfig OCSPConfig, storage Storage, cer if ocspErr != nil { // For short-lived certificates, this is fine and we can ignore // logging because OCSP doesn't make much sense for them anyway. - if cert.Lifetime() > 7*24*time.Hour { + if cert.Lifetime() < 7*24*time.Hour { return nil } // There's nothing else we can do to get OCSP for this certificate, From 828fe058e0e4d2bc113d14238d5d13f8c6f85566 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Fri, 8 Nov 2024 15:31:44 -0500 Subject: [PATCH 4/4] Oops, had the Lifetime math wrong --- certificates.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certificates.go b/certificates.go index c8d0c4ba..9ac0185c 100644 --- a/certificates.go +++ b/certificates.go @@ -193,7 +193,7 @@ func (cert Certificate) Lifetime() time.Duration { if cert.Leaf == nil || cert.Leaf.NotAfter.IsZero() { return 0 } - return cert.Leaf.NotAfter.Sub(expiresAt(cert.Leaf)) + return expiresAt(cert.Leaf).Sub(cert.Leaf.NotBefore) } // currentlyInRenewalWindow returns true if the current time is within