aboutsummaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorMatt Holt <[email protected]>2017-08-23 12:26:01 -0600
committerGitHub <[email protected]>2017-08-23 12:26:01 -0600
commite49474a4f555d2b8ebfac504fb9a2d3bad08730e (patch)
tree9a432127d087eae9f0bb1c09f87977b62803d3e1
parentc026e2b734f2ecc2f23fd91cd858018473a2a6c9 (diff)
parentb699a17a1b3738bd4a68a41e39f0ec41868d83bb (diff)
downloadcaddy-e49474a4f555d2b8ebfac504fb9a2d3bad08730e.tar.gz
caddy-e49474a4f555d2b8ebfac504fb9a2d3bad08730e.zip
Merge pull request #1821 from mholt/ocspfix
tls: Fix OCSP stapling bug when certificate names overlap other certs
-rw-r--r--caddytls/certificates.go52
-rw-r--r--caddytls/maintain.go10
2 files changed, 46 insertions, 16 deletions
diff --git a/caddytls/certificates.go b/caddytls/certificates.go
index d3aee8f0d..ed26c33e6 100644
--- a/caddytls/certificates.go
+++ b/caddytls/certificates.go
@@ -14,7 +14,9 @@ import (
)
// certCache stores certificates in memory,
-// keying certificates by name.
+// keying certificates by name. Certificates
+// should not overlap in the names they serve,
+// because a name only maps to one certificate.
var certCache = make(map[string]Certificate)
var certCacheMu sync.RWMutex
@@ -27,6 +29,8 @@ type Certificate struct {
// Names is the list of names this certificate is written for.
// The first is the CommonName (if any), the rest are SAN.
+ // This should be the exact list of keys by which this cert
+ // is accessed in the cache, careful to avoid overlap.
Names []string
// NotAfter is when the certificate expires.
@@ -164,17 +168,9 @@ func makeCertificate(certPEMBlock, keyPEMBlock []byte) (Certificate, error) {
if err != nil {
return cert, err
}
- if len(tlsCert.Certificate) == 0 {
- return cert, errors.New("certificate is empty")
- }
- cert.Certificate = tlsCert
- // Parse leaf certificate, extract relevant metadata, and staple OCSP
- leaf, err := x509.ParseCertificate(tlsCert.Certificate[0])
- if err != nil {
- return cert, err
- }
- err = fillCertFromLeaf(&cert, leaf)
+ // Extract relevant metadata and staple OCSP
+ err = fillCertFromLeaf(&cert, tlsCert)
if err != nil {
return cert, err
}
@@ -186,9 +182,19 @@ func makeCertificate(certPEMBlock, keyPEMBlock []byte) (Certificate, error) {
return cert, nil
}
-// fillCertFromLeaf populates cert.Names and cert.NotAfter
-// using data in leaf.
-func fillCertFromLeaf(cert *Certificate, leaf *x509.Certificate) error {
+// fillCertFromLeaf populates metadata fields on cert from tlsCert.
+func fillCertFromLeaf(cert *Certificate, tlsCert tls.Certificate) error {
+ if len(tlsCert.Certificate) == 0 {
+ return errors.New("certificate is empty")
+ }
+ cert.Certificate = tlsCert
+
+ // the leaf cert should be the one for the site; it has what we need
+ leaf, err := x509.ParseCertificate(tlsCert.Certificate[0])
+ if err != nil {
+ return err
+ }
+
if leaf.Subject.CommonName != "" {
cert.Names = []string{strings.ToLower(leaf.Subject.CommonName)}
}
@@ -210,7 +216,9 @@ func fillCertFromLeaf(cert *Certificate, leaf *x509.Certificate) error {
if len(cert.Names) == 0 {
return errors.New("certificate has no names")
}
+
cert.NotAfter = leaf.NotAfter
+
return nil
}
@@ -231,7 +239,6 @@ func cacheCertificate(cert Certificate) {
if _, ok := certCache[""]; !ok {
// use as default - must be *appended* to end of list, or bad things happen!
cert.Names = append(cert.Names, "")
- certCache[""] = cert
}
for len(certCache)+len(cert.Names) > 10000 {
// for simplicity, just remove random elements
@@ -243,7 +250,20 @@ func cacheCertificate(cert Certificate) {
break
}
}
- for _, name := range cert.Names {
+ for i := 0; i < len(cert.Names); i++ {
+ name := cert.Names[i]
+ if _, ok := certCache[name]; ok {
+ // do not allow certificates to overlap in the names they serve;
+ // this ambiguity causes problems because it is confusing while
+ // maintaining certificates; see OCSP maintenance code and
+ // https://caddy.community/t/random-ocsp-response-errors-for-random-clients/2473?u=matt.
+ log.Printf("[NOTICE] There is already a certificate loaded for %s, "+
+ "so certificate for %v will not service that name",
+ name, cert.Names)
+ cert.Names = append(cert.Names[:i], cert.Names[i+1:]...)
+ i--
+ continue
+ }
certCache[name] = cert
}
certCacheMu.Unlock()
diff --git a/caddytls/maintain.go b/caddytls/maintain.go
index fe9b0b872..b495a752a 100644
--- a/caddytls/maintain.go
+++ b/caddytls/maintain.go
@@ -246,6 +246,16 @@ func UpdateOCSPStaples() {
log.Printf("[INFO] Advancing OCSP staple for %v from %s to %s",
cert.Names, lastNextUpdate, cert.OCSP.NextUpdate)
for _, n := range cert.Names {
+ // BUG: If this certificate has names on it that appear on another
+ // certificate in the cache, AND the other certificate is keyed by
+ // that name in the cache, then this method of 'queueing' the staple
+ // update will cause this certificate's new OCSP to be stapled to
+ // a different certificate! See:
+ // https://caddy.community/t/random-ocsp-response-errors-for-random-clients/2473?u=matt
+ // This problem should be avoided if names on certificates in the
+ // cache don't overlap with regards to the cache keys.
+ // (This is isn't a bug anymore, since we're careful when we add
+ // certificates to the cache by skipping keying when key already exists.)
updated[n] = ocspUpdate{rawBytes: cert.Certificate.OCSPStaple, parsed: cert.OCSP}
}
}