From 1cfd960f3ccc3fcfaef240c93069fcc0b11c0890 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Mon, 15 Feb 2016 23:39:04 -0700 Subject: [PATCH] Bug fixes and other improvements to TLS functions Now attempt to staple OCSP even for certs that don't have an existing staple (issue #605). "tls off" short-circuits tls setup function. Now we call getEmail() when setting up an acme.Client that does renewals, rather than making a new account with empty email address. Check certificate expiry every 12 hours, and OCSP every hour. --- caddy/https/certificates.go | 8 +- caddy/https/handshake.go | 12 +-- caddy/https/https.go | 29 +++---- caddy/https/maintain.go | 84 +++++++++++++------ caddy/https/setup.go | 161 +++++++++++++++++++----------------- server/config.go | 2 +- server/server.go | 10 +-- 7 files changed, 168 insertions(+), 138 deletions(-) diff --git a/caddy/https/certificates.go b/caddy/https/certificates.go index 72a9ff1c..b123d4c3 100644 --- a/caddy/https/certificates.go +++ b/caddy/https/certificates.go @@ -24,7 +24,7 @@ var certCacheMu sync.RWMutex // we can be more efficient by extracting the metadata once so it's // just there, ready to use. type Certificate struct { - *tls.Certificate + tls.Certificate // Names is the list of names this certificate is written for. // The first is the CommonName (if any), the rest are SAN. @@ -170,7 +170,6 @@ func makeCertificate(certPEMBlock, keyPEMBlock []byte) (Certificate, error) { if len(tlsCert.Certificate) == 0 { return cert, errors.New("certificate is empty") } - cert.Certificate = &tlsCert // Parse leaf certificate and extract relevant metadata leaf, err := x509.ParseCertificate(tlsCert.Certificate[0]) @@ -198,6 +197,7 @@ func makeCertificate(certPEMBlock, keyPEMBlock []byte) (Certificate, error) { cert.OCSP = ocspResp } + cert.Certificate = tlsCert return cert, nil } @@ -213,7 +213,9 @@ func makeCertificate(certPEMBlock, keyPEMBlock []byte) (Certificate, error) { func cacheCertificate(cert Certificate) { certCacheMu.Lock() if _, ok := certCache[""]; !ok { - certCache[""] = cert // use as default + // use as default + certCache[""] = cert + cert.Names = append(cert.Names, "") } for len(certCache)+len(cert.Names) > 10000 { // for simplicity, just remove random elements diff --git a/caddy/https/handshake.go b/caddy/https/handshake.go index 3d759b31..38f9afb5 100644 --- a/caddy/https/handshake.go +++ b/caddy/https/handshake.go @@ -23,7 +23,7 @@ import ( // This function is safe for use as a tls.Config.GetCertificate callback. func GetCertificate(clientHello *tls.ClientHelloInfo) (*tls.Certificate, error) { cert, err := getCertDuringHandshake(clientHello.ServerName, false, false) - return cert.Certificate, err + return &cert.Certificate, err } // GetOrObtainCertificate will get a certificate to satisfy clientHello, even @@ -35,7 +35,7 @@ func GetCertificate(clientHello *tls.ClientHelloInfo) (*tls.Certificate, error) // This function is safe for use as a tls.Config.GetCertificate callback. func GetOrObtainCertificate(clientHello *tls.ClientHelloInfo) (*tls.Certificate, error) { cert, err := getCertDuringHandshake(clientHello.ServerName, true, true) - return cert.Certificate, err + return &cert.Certificate, err } // getCertDuringHandshake will get a certificate for name. It first tries @@ -122,8 +122,8 @@ func checkLimitsForObtainingNewCerts(name string) error { } // obtainOnDemandCertificate obtains a certificate for name for the given -// clientHello. If another goroutine has already started obtaining a cert -// for name, it will wait and use what the other goroutine obtained. +// name. If another goroutine has already started obtaining a cert for +// name, it will wait and use what the other goroutine obtained. // // This function is safe for use by multiple concurrent goroutines. func obtainOnDemandCertificate(name string) (Certificate, error) { @@ -248,7 +248,7 @@ func renewDynamicCertificate(name string) (Certificate, error) { log.Printf("[INFO] Renewing certificate for %s", name) - client, err := NewACMEClient("", false) // renewals don't use email + client, err := NewACMEClientGetEmail(server.Config{}, false) if err != nil { return Certificate{}, err } @@ -295,7 +295,7 @@ var obtainCertWaitChansMu sync.Mutex // OnDemandIssuedCount is the number of certificates that have been issued // on-demand by this process. It is only safe to modify this count atomically. -// If it reaches max_certs, on-demand issuances will fail. +// If it reaches onDemandMaxIssue, on-demand issuances will fail. var OnDemandIssuedCount = new(int32) // onDemandMaxIssue is set based on max_certs in tls config. It specifies the diff --git a/caddy/https/https.go b/caddy/https/https.go index f6cdcd46..4526a31c 100644 --- a/caddy/https/https.go +++ b/caddy/https/https.go @@ -12,7 +12,6 @@ import ( "net/http" "os" "strings" - "time" "github.com/mholt/caddy/middleware" "github.com/mholt/caddy/middleware/redirect" @@ -215,7 +214,7 @@ func hostHasOtherPort(allConfigs []server.Config, thisConfigIdx int, otherPort s // all configs. func MakePlaintextRedirects(allConfigs []server.Config) []server.Config { for i, cfg := range allConfigs { - if (cfg.TLS.Managed || cfg.TLS.OnDemand) && + if cfg.TLS.Managed && !hostHasOtherPort(allConfigs, i, "80") && (cfg.Port == "443" || !hostHasOtherPort(allConfigs, i, "443")) { allConfigs = append(allConfigs, redirPlaintextHost(cfg)) @@ -233,15 +232,16 @@ func MakePlaintextRedirects(allConfigs []server.Config) []server.Config { // setting up the config may make it look like it // doesn't qualify even though it originally did. func ConfigQualifies(cfg server.Config) bool { - return !cfg.TLS.Manual && // user can provide own cert and key + return (!cfg.TLS.Manual || cfg.TLS.OnDemand) && // user might provide own cert and key // user can force-disable automatic HTTPS for this host cfg.Scheme != "http" && cfg.Port != "80" && cfg.TLS.LetsEncryptEmail != "off" && - // we get can't certs for some kinds of hostnames - HostQualifies(cfg.Host) + // we get can't certs for some kinds of hostnames, but + // on-demand TLS allows empty hostnames at startup + (HostQualifies(cfg.Host) || cfg.TLS.OnDemand) } // HostQualifies returns true if the hostname alone @@ -387,20 +387,11 @@ var ( CAUrl string ) -// Some essential values related to the Let's Encrypt process -const ( - // AlternatePort is the port on which the acme client will open a - // listener and solve the CA's challenges. If this alternate port - // is used instead of the default port (80 or 443), then the - // default port for the challenge must be forwarded to this one. - AlternatePort = "5033" - - // RenewInterval is how often to check certificates for renewal. - RenewInterval = 6 * time.Hour - - // OCSPInterval is how often to check if OCSP stapling needs updating. - OCSPInterval = 1 * time.Hour -) +// AlternatePort is the port on which the acme client will open a +// listener and solve the CA's challenges. If this alternate port +// is used instead of the default port (80 or 443), then the +// default port for the challenge must be forwarded to this one. +const AlternatePort = "5033" // KeySize represents the length of a key in bits. type KeySize int diff --git a/caddy/https/maintain.go b/caddy/https/maintain.go index 9aa293d0..49fc1c16 100644 --- a/caddy/https/maintain.go +++ b/caddy/https/maintain.go @@ -4,9 +4,19 @@ import ( "log" "time" + "github.com/mholt/caddy/server" + "golang.org/x/crypto/ocsp" ) +const ( + // RenewInterval is how often to check certificates for renewal. + RenewInterval = 12 * time.Hour + + // OCSPInterval is how often to check if OCSP stapling needs updating. + OCSPInterval = 1 * time.Hour +) + // maintainAssets is a permanently-blocking function // that loops indefinitely and, on a regular schedule, checks // certificates for expiration and initiates a renewal of certs @@ -28,7 +38,7 @@ func maintainAssets(stopChan chan struct{}) { log.Println("[INFO] Done checking certificates") case <-ocspTicker.C: log.Println("[INFO] Scanning for stale OCSP staples") - updatePreloadedOCSPStaples() + updateOCSPStaples() log.Println("[INFO] Done checking OCSP staples") case <-stopChan: renewalTicker.Stop() @@ -70,7 +80,7 @@ func renewManagedCertificates(allowPrompts bool) (err error) { log.Printf("[INFO] Certificate for %v expires in %v; attempting renewal", cert.Names, timeLeft) if client == nil { - client, err = NewACMEClient("", allowPrompts) // renewals don't use email + client, err = NewACMEClientGetEmail(server.Config{}, allowPrompts) if err != nil { return err } @@ -116,42 +126,66 @@ func renewManagedCertificates(allowPrompts bool) (err error) { return nil } -func updatePreloadedOCSPStaples() { +func updateOCSPStaples() { // Create a temporary place to store updates - // until we release the potentially slow read - // lock so we can use a quick write lock. + // until we release the potentially long-lived + // read lock and use a short-lived write lock. type ocspUpdate struct { - rawBytes []byte - parsedResponse *ocsp.Response + rawBytes []byte + parsed *ocsp.Response } updated := make(map[string]ocspUpdate) + // A single SAN certificate maps to multiple names, so we use this + // set to make sure we don't waste cycles checking OCSP for the same + // certificate multiple times. + visited := make(map[string]struct{}) + certCacheMu.RLock() for name, cert := range certCache { - // we update OCSP for managed and un-managed certs here, but only - // if it has OCSP stapled and only for pre-loaded certificates - if cert.OnDemand || cert.OCSP == nil { + // skip this certificate if we've already visited it, + // and if not, mark all the names as visited + if _, ok := visited[name]; ok { + continue + } + for _, n := range cert.Names { + visited[n] = struct{}{} + } + + // no point in updating OCSP for expired certificates + if time.Now().After(cert.NotAfter) { continue } - // start checking OCSP staple about halfway through validity period for good measure - oldNextUpdate := cert.OCSP.NextUpdate - refreshTime := cert.OCSP.ThisUpdate.Add(oldNextUpdate.Sub(cert.OCSP.ThisUpdate) / 2) + var lastNextUpdate time.Time + if cert.OCSP != nil { + // start checking OCSP staple about halfway through validity period for good measure + lastNextUpdate = cert.OCSP.NextUpdate + refreshTime := cert.OCSP.ThisUpdate.Add(lastNextUpdate.Sub(cert.OCSP.ThisUpdate) / 2) - // only check for updated OCSP validity window if the refresh time is - // in the past and the certificate is not expired - if time.Now().After(refreshTime) && time.Now().Before(cert.NotAfter) { - err := stapleOCSP(&cert, nil) - if err != nil { - log.Printf("[ERROR] Checking OCSP for %s: %v", name, err) + // since OCSP is already stapled, we need only check if we're in that "refresh window" + if time.Now().Before(refreshTime) { continue } + } - // if the OCSP response has been updated, we use it - if oldNextUpdate != cert.OCSP.NextUpdate { - log.Printf("[INFO] Moving validity period of OCSP staple for %s from %v to %v", - name, oldNextUpdate, cert.OCSP.NextUpdate) - updated[name] = ocspUpdate{rawBytes: cert.Certificate.OCSPStaple, parsedResponse: cert.OCSP} + err := stapleOCSP(&cert, nil) + if err != nil { + if cert.OCSP != nil { + // if it was no staple before, that's fine, otherwise we should log the error + log.Printf("[ERROR] Checking OCSP for %s: %v", name, err) + } + continue + } + + // 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 lastNextUpdate.IsZero() || lastNextUpdate != cert.OCSP.NextUpdate { + log.Printf("[INFO] Advancing OCSP staple for %v from %s to %s", + cert.Names, lastNextUpdate, cert.OCSP.NextUpdate) + for _, n := range cert.Names { + updated[n] = ocspUpdate{rawBytes: cert.Certificate.OCSPStaple, parsed: cert.OCSP} } } } @@ -161,7 +195,7 @@ func updatePreloadedOCSPStaples() { certCacheMu.Lock() for name, update := range updated { cert := certCache[name] - cert.OCSP = update.parsedResponse + cert.OCSP = update.parsed cert.Certificate.OCSPStaple = update.rawBytes certCache[name] = cert } diff --git a/caddy/https/setup.go b/caddy/https/setup.go index ebf46d24..b5b53454 100644 --- a/caddy/https/setup.go +++ b/caddy/https/setup.go @@ -20,12 +20,12 @@ import ( // are specified by the user in the config file. All the automatic HTTPS // stuff comes later outside of this function. func Setup(c *setup.Controller) (middleware.Middleware, error) { - if c.Scheme == "http" { + if c.Port == "80" || c.Scheme == "http" { c.TLS.Enabled = false log.Printf("[WARNING] TLS disabled for %s://%s.", c.Scheme, c.Address()) - } else { - c.TLS.Enabled = true + return nil, nil } + c.TLS.Enabled = true for c.Next() { var certificateFile, keyFile, loadDir, maxCerts string @@ -38,6 +38,7 @@ func Setup(c *setup.Controller) (middleware.Middleware, error) { // user can force-disable managed TLS this way if c.TLS.LetsEncryptEmail == "off" { c.TLS.Enabled = false + return nil, nil } case 2: certificateFile = args[0] @@ -120,78 +121,8 @@ func Setup(c *setup.Controller) (middleware.Middleware, error) { } // load a directory of certificates, if specified - // modeled after haproxy: https://cbonte.github.io/haproxy-dconv/configuration-1.5.html#5.1-crt if loadDir != "" { - err := filepath.Walk(loadDir, func(path string, info os.FileInfo, err error) error { - if err != nil { - log.Printf("[WARNING] Unable to traverse into %s; skipping", path) - return nil - } - if info.IsDir() { - return nil - } - if strings.HasSuffix(strings.ToLower(info.Name()), ".pem") { - certBuilder, keyBuilder := new(bytes.Buffer), new(bytes.Buffer) - var foundKey bool - - bundle, err := ioutil.ReadFile(path) - if err != nil { - return err - } - - for { - // Decode next block so we can see what type it is - var derBlock *pem.Block - derBlock, bundle = pem.Decode(bundle) - if derBlock == nil { - break - } - - if derBlock.Type == "CERTIFICATE" { - // Re-encode certificate as PEM, appending to certificate chain - pem.Encode(certBuilder, derBlock) - } else if derBlock.Type == "EC PARAMETERS" { - // EC keys are composed of two blocks: parameters and key - // (parameter block should come first) - if !foundKey { - // Encode parameters - pem.Encode(keyBuilder, derBlock) - - // Key must immediately follow - derBlock, bundle = pem.Decode(bundle) - if derBlock == nil || derBlock.Type != "EC PRIVATE KEY" { - return c.Errf("%s: expected elliptic private key to immediately follow EC parameters", path) - } - pem.Encode(keyBuilder, derBlock) - foundKey = true - } - } else if derBlock.Type == "PRIVATE KEY" || strings.HasSuffix(derBlock.Type, " PRIVATE KEY") { - // RSA key - if !foundKey { - pem.Encode(keyBuilder, derBlock) - foundKey = true - } - } else { - return c.Errf("%s: unrecognized PEM block type: %s", path, derBlock.Type) - } - } - - certPEMBytes, keyPEMBytes := certBuilder.Bytes(), keyBuilder.Bytes() - if len(certPEMBytes) == 0 { - return c.Errf("%s: failed to parse PEM data", path) - } - if len(keyPEMBytes) == 0 { - return c.Errf("%s: no private key block found", path) - } - - err = cacheUnmanagedCertificatePEMBytes(certPEMBytes, keyPEMBytes) - if err != nil { - return c.Errf("%s: failed to load cert and key for %s: %v", path, c.Host, err) - } - log.Printf("[INFO] Successfully loaded TLS assets from %s", path) - } - return nil - }) + err := loadCertsInDir(c, loadDir) if err != nil { return nil, err } @@ -203,6 +134,86 @@ func Setup(c *setup.Controller) (middleware.Middleware, error) { return nil, nil } +// loadCertsInDir loads all the certificates/keys in dir, as long as +// the file ends with .pem. This method of loading certificates is +// modeled after haproxy, which expects the certificate and key to +// be bundled into the same file: +// https://cbonte.github.io/haproxy-dconv/configuration-1.5.html#5.1-crt +// +// This function may write to the log as it walks the directory tree. +func loadCertsInDir(c *setup.Controller, dir string) error { + return filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { + if err != nil { + log.Printf("[WARNING] Unable to traverse into %s; skipping", path) + return nil + } + if info.IsDir() { + return nil + } + if strings.HasSuffix(strings.ToLower(info.Name()), ".pem") { + certBuilder, keyBuilder := new(bytes.Buffer), new(bytes.Buffer) + var foundKey bool // use only the first key in the file + + bundle, err := ioutil.ReadFile(path) + if err != nil { + return err + } + + for { + // Decode next block so we can see what type it is + var derBlock *pem.Block + derBlock, bundle = pem.Decode(bundle) + if derBlock == nil { + break + } + + if derBlock.Type == "CERTIFICATE" { + // Re-encode certificate as PEM, appending to certificate chain + pem.Encode(certBuilder, derBlock) + } else if derBlock.Type == "EC PARAMETERS" { + // EC keys generated from openssl can be composed of two blocks: + // parameters and key (parameter block should come first) + if !foundKey { + // Encode parameters + pem.Encode(keyBuilder, derBlock) + + // Key must immediately follow + derBlock, bundle = pem.Decode(bundle) + if derBlock == nil || derBlock.Type != "EC PRIVATE KEY" { + return c.Errf("%s: expected elliptic private key to immediately follow EC parameters", path) + } + pem.Encode(keyBuilder, derBlock) + foundKey = true + } + } else if derBlock.Type == "PRIVATE KEY" || strings.HasSuffix(derBlock.Type, " PRIVATE KEY") { + // RSA key + if !foundKey { + pem.Encode(keyBuilder, derBlock) + foundKey = true + } + } else { + return c.Errf("%s: unrecognized PEM block type: %s", path, derBlock.Type) + } + } + + certPEMBytes, keyPEMBytes := certBuilder.Bytes(), keyBuilder.Bytes() + if len(certPEMBytes) == 0 { + return c.Errf("%s: failed to parse PEM data", path) + } + if len(keyPEMBytes) == 0 { + return c.Errf("%s: no private key block found", path) + } + + err = cacheUnmanagedCertificatePEMBytes(certPEMBytes, keyPEMBytes) + if err != nil { + return c.Errf("%s: failed to load cert and key for %s: %v", path, c.Host, err) + } + log.Printf("[INFO] Successfully loaded TLS assets from %s", path) + } + return nil + }) +} + // setDefaultTLSParams sets the default TLS cipher suites, protocol versions, // and server preferences of a server.Config if they were not previously set // (it does not overwrite; only fills in missing values). It will also set the @@ -231,7 +242,7 @@ func setDefaultTLSParams(c *server.Config) { // Default TLS port is 443; only use if port is not manually specified, // TLS is enabled, and the host is not localhost - if c.Port == "" && c.TLS.Enabled && !c.TLS.Manual && c.Host != "localhost" { + if c.Port == "" && c.TLS.Enabled && (!c.TLS.Manual || c.TLS.OnDemand) && c.Host != "localhost" { c.Port = "443" } } diff --git a/server/config.go b/server/config.go index 9acdac7f..332f4575 100644 --- a/server/config.go +++ b/server/config.go @@ -68,7 +68,7 @@ type TLSConfig struct { Enabled bool // will be set to true if TLS is enabled LetsEncryptEmail string Manual bool // will be set to true if user provides own certs and keys - Managed bool // will be set to true if config qualifies for automatic/managed HTTPS + Managed bool // will be set to true if config qualifies for implicit automatic/managed HTTPS OnDemand bool // will be set to true if user enables on-demand TLS (obtain certs during handshakes) Ciphers []uint16 ProtocolMinVersion uint16 diff --git a/server/server.go b/server/server.go index c921566c..963692fe 100644 --- a/server/server.go +++ b/server/server.go @@ -63,15 +63,7 @@ func New(addr string, configs []Config, gracefulTimeout time.Duration) (*Server, var useTLS, useOnDemandTLS bool if len(configs) > 0 { useTLS = configs[0].TLS.Enabled - if useTLS { - host, _, err := net.SplitHostPort(addr) - if err != nil { - host = addr - } - if host == "" && configs[0].TLS.OnDemand { - useOnDemandTLS = true - } - } + useOnDemandTLS = configs[0].TLS.OnDemand } s := &Server{