From 654f26cb910b90c63bf590c6723343730f3f30d0 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Mon, 16 Oct 2017 16:40:43 -0600 Subject: [PATCH 1/4] tls: Evict existing certificates from cache when loading ones from disk --- caddytls/certificates.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/caddytls/certificates.go b/caddytls/certificates.go index 2f24e884b..05af914fe 100644 --- a/caddytls/certificates.go +++ b/caddytls/certificates.go @@ -128,8 +128,10 @@ func (cfg *Config) CacheManagedCertificate(domain string) (Certificate, error) { // cacheUnmanagedCertificatePEMFile loads a certificate for host using certFile // and keyFile, which must be in PEM format. It stores the certificate in -// memory. The Managed and OnDemand flags of the certificate will be set to -// false. +// memory after evicting any other entries in the cache keyed by the names +// on this certificate. In other words, it replaces existing certificates keyed +// by the names on this certificate. The Managed and OnDemand flags of the +// certificate will be set to false. // // This function is safe for concurrent use. func cacheUnmanagedCertificatePEMFile(certFile, keyFile string) error { @@ -137,6 +139,16 @@ func cacheUnmanagedCertificatePEMFile(certFile, keyFile string) error { if err != nil { return err } + + // since this is manually managed, this call might be part of a reload after + // the owner renewed a certificate; so clear cache of any previous cert first, + // otherwise the renewed certificate may never be loaded + certCacheMu.Lock() + for _, name := range cert.Names { + delete(certCache, name) + } + certCacheMu.Unlock() + cacheCertificate(cert) return nil } From c6a2911725dc845985c44b9a3f1a2547b7b8c76a Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Mon, 16 Oct 2017 19:23:21 -0400 Subject: [PATCH 2/4] tls: Handle when OCSP responder cert expires before a response it issued (#1922) * Handle the case of an OCSP responder certificate expiring before an OCSP response it issued * oops * doh, gofmt --- caddytls/maintain.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/caddytls/maintain.go b/caddytls/maintain.go index a657f0c79..9e42fc87c 100644 --- a/caddytls/maintain.go +++ b/caddytls/maintain.go @@ -334,8 +334,15 @@ func DeleteOldStapleFiles() { // meaning that it is not expedient to get an // updated response from the OCSP server. func freshOCSP(resp *ocsp.Response) bool { + nextUpdate := resp.NextUpdate + // If there is an OCSP responder certificate, and it expires before the + // OCSP response, use its expiration date as the end of the OCSP + // response's validity period. + if resp.Certificate != nil && resp.Certificate.NotAfter.Before(nextUpdate) { + nextUpdate = resp.Certificate.NotAfter + } // start checking OCSP staple about halfway through validity period for good measure - refreshTime := resp.ThisUpdate.Add(resp.NextUpdate.Sub(resp.ThisUpdate) / 2) + refreshTime := resp.ThisUpdate.Add(nextUpdate.Sub(resp.ThisUpdate) / 2) return time.Now().Before(refreshTime) } From a2db340378b6a56fb3d8b885103bb07e67abc97a Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Mon, 16 Oct 2017 17:25:55 -0600 Subject: [PATCH 3/4] tls: Final check of OCSP response validity date before stapling --- caddytls/crypto.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/caddytls/crypto.go b/caddytls/crypto.go index 3ebc4be4d..3036834c4 100644 --- a/caddytls/crypto.go +++ b/caddytls/crypto.go @@ -151,6 +151,13 @@ func stapleOCSP(cert *Certificate, pemBundle []byte) error { // 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.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 fmt.Errorf("invalid: OCSP response for %v valid after certificate expiration (%s)", + cert.Names, cert.NotAfter.Sub(ocspResp.NextUpdate)) + } cert.Certificate.OCSPStaple = ocspBytes cert.OCSP = ocspResp if gotNewOCSP { From 68a495f144603e0268ca2016fa61f63654254617 Mon Sep 17 00:00:00 2001 From: Craig Peterson Date: Thu, 19 Oct 2017 10:27:10 -0400 Subject: [PATCH 4/4] actually return error on redeclaration --- caddyfile/parse.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/caddyfile/parse.go b/caddyfile/parse.go index c0ec2d4cd..3b06e8b88 100644 --- a/caddyfile/parse.go +++ b/caddyfile/parse.go @@ -100,10 +100,9 @@ func (p *parser) begin() error { if p.definedMacros == nil { p.definedMacros = map[string][]Token{} } - if p.definedMacros[name] != nil { - p.Errf("redeclaration of previously declared macro %s", name) + if _, found := p.definedMacros[name]; found { + return p.Errf("redeclaration of previously declared macro %s", name) } - // consume all tokens til matched close brace tokens, err := p.macroTokens() if err != nil {