caddytls: Use latest certmagic package, with updated Storage interface

caddy.go

@@ -487,7 +487,6 @@ func Start(cdyfile Input) (*Instance, error) {
 			return nil, fmt.Errorf("constructing cluster plugin %s: %v", clusterPluginName, err)
 		}
 		certmagic.DefaultStorage = storage
-		OnProcessExit = append(OnProcessExit, certmagic.DefaultStorage.UnlockAllObtained)
 	}
 
 	inst := &Instance{serverType: cdyfile.ServerType(), wg: new(sync.WaitGroup), Storage: make(map[interface{}]interface{})}

caddytls/setup.go

@@ -432,5 +432,5 @@ func loadCertsInDir(cfg *Config, c *caddy.Controller, dir string) error {
 }
 
 func constructDefaultClusterPlugin() (certmagic.Storage, error) {
-	return certmagic.FileStorage{Path: caddy.AssetsPath()}, nil
+	return &certmagic.FileStorage{Path: caddy.AssetsPath()}, nil
 }

vendor/github.com/mholt/certmagic/certificates.go

@@ -64,12 +64,11 @@ func (c Certificate) NeedsRenewal() bool {
 	if c.NotAfter.IsZero() {
 		return false
 	}
-	timeLeft := c.NotAfter.UTC().Sub(time.Now().UTC())
 	renewDurationBefore := DefaultRenewDurationBefore
 	if len(c.configs) > 0 && c.configs[0].RenewDurationBefore > 0 {
 		renewDurationBefore = c.configs[0].RenewDurationBefore
 	}
-	return timeLeft < renewDurationBefore
+	return time.Until(c.NotAfter) < renewDurationBefore
 }
 
 // CacheManagedCertificate loads the certificate for domain into the

vendor/github.com/mholt/certmagic/certmagic.go

@@ -52,6 +52,16 @@ import (
 // HTTPS serves mux for all domainNames using the HTTP
 // and HTTPS ports, redirecting all HTTP requests to HTTPS.
 //
+// This high-level convenience function is opinionated and
+// applies sane defaults for production use, including
+// timeouts for HTTP requests and responses. To allow very
+// long-lived requests or connections, you should make your
+// own http.Server values and use this package's Listen(),
+// TLS(), or Config.TLSConfig() functions to customize to
+// your needs. For example, servers which need to support
+// large uploads or downloads with slow clients may need to
+// use longer timeouts, thus this function is not suitable.
+//
 // Calling this function signifies your acceptance to
 // the CA's Subscriber Agreement and/or Terms of Service.
 func HTTPS(domainNames []string, mux http.Handler) error {
@@ -96,13 +106,32 @@ func HTTPS(domainNames []string, mux http.Handler) error {
 	hln, hsln := httpLn, httpsLn
 	lnMu.Unlock()
 
-	httpHandler := cfg.HTTPChallengeHandler(http.HandlerFunc(httpRedirectHandler))
+	// create HTTP/S servers that are configured
+	// with sane default timeouts and appropriate
+	// handlers (the HTTP server solves the HTTP
+	// challenge and issues redirects to HTTPS,
+	// while the HTTPS server simply serves the
+	// user's handler)
+	httpServer := &http.Server{
+		ReadHeaderTimeout: 5 * time.Second,
+		ReadTimeout:       5 * time.Second,
+		WriteTimeout:      5 * time.Second,
+		IdleTimeout:       5 * time.Second,
+		Handler:           cfg.HTTPChallengeHandler(http.HandlerFunc(httpRedirectHandler)),
+	}
+	httpsServer := &http.Server{
+		ReadHeaderTimeout: 10 * time.Second,
+		ReadTimeout:       30 * time.Second,
+		WriteTimeout:      2 * time.Minute,
+		IdleTimeout:       5 * time.Minute,
+		Handler:           mux,
+	}
 
 	log.Printf("%v Serving HTTP->HTTPS on %s and %s",
 		domainNames, hln.Addr(), hsln.Addr())
 
-	go http.Serve(hln, httpHandler)
-	return http.Serve(hsln, mux)
+	go httpServer.Serve(hln)
+	return httpsServer.Serve(hsln)
 }
 
 func httpRedirectHandler(w http.ResponseWriter, r *http.Request) {

vendor/github.com/mholt/certmagic/client.go

@@ -208,6 +208,8 @@ func (cfg *Config) newACMEClient(interactive bool) (*acmeClient, error) {
 	return c, nil
 }
 
+// lockKey returns a key for a lock that is specific to the operation
+// named op being performed related to domainName and this config's CA.
 func (cfg *Config) lockKey(op, domainName string) string {
 	return fmt.Sprintf("%s:%s:%s", op, domainName, cfg.CA)
 }
@@ -215,30 +217,34 @@ func (cfg *Config) lockKey(op, domainName string) string {
 // Obtain obtains a single certificate for name. It stores the certificate
 // on the disk if successful. This function is safe for concurrent use.
 //
-// Right now our storage mechanism only supports one name per certificate,
-// so this function (along with Renew and Revoke) only accepts one domain
-// as input. It can be easily modified to support SAN certificates if our
-// storage mechanism is upgraded later.
+// Our storage mechanism only supports one name per certificate, so this
+// function (along with Renew and Revoke) only accepts one domain as input.
+// It could be easily modified to support SAN certificates if our storage
+// mechanism is upgraded later, but that will increase logical complexity
+// in other areas.
 //
 // Callers who have access to a Config value should use the ObtainCert
 // method on that instead of this lower-level method.
 func (c *acmeClient) Obtain(name string) error {
+	// ensure idempotency of the obtain operation for this name
 	lockKey := c.config.lockKey("cert_acme", name)
-	waiter, err := c.config.certCache.storage.TryLock(lockKey)
+	err := c.config.certCache.storage.Lock(lockKey)
 	if err != nil {
 		return err
 	}
-	if waiter != nil {
-		log.Printf("[INFO] Certificate for %s is already being obtained elsewhere and stored; waiting", name)
-		waiter.Wait()
-		return nil // we assume the process with the lock succeeded, rather than hammering this execution path again
-	}
 	defer func() {
 		if err := c.config.certCache.storage.Unlock(lockKey); err != nil {
-			log.Printf("[ERROR] Unable to unlock obtain call for %s: %v", name, err)
+			log.Printf("[ERROR][%s] Obtain: Unable to unlock '%s': %v", name, lockKey, err)
 		}
 	}()
 
+	// check if obtain is still needed -- might have
+	// been obtained during lock
+	if c.config.storageHasCertResources(name) {
+		log.Printf("[INFO][%s] Obtain: Certificate already exists in storage", name)
+		return nil
+	}
+
 	for attempts := 0; attempts < 2; attempts++ {
 		request := certificate.ObtainRequest{
 			Domains:    []string{name},
@@ -280,19 +286,15 @@ func (c *acmeClient) Obtain(name string) error {
 // Callers who have access to a Config value should use the RenewCert
 // method on that instead of this lower-level method.
 func (c *acmeClient) Renew(name string) error {
+	// ensure idempotency of the renew operation for this name
 	lockKey := c.config.lockKey("cert_acme", name)
-	waiter, err := c.config.certCache.storage.TryLock(lockKey)
+	err := c.config.certCache.storage.Lock(lockKey)
 	if err != nil {
 		return err
 	}
-	if waiter != nil {
-		log.Printf("[INFO] Certificate for %s is already being renewed elsewhere and stored; waiting", name)
-		waiter.Wait()
-		return nil // assume that the worker that renewed the cert succeeded to avoid hammering this path over and over
-	}
 	defer func() {
 		if err := c.config.certCache.storage.Unlock(lockKey); err != nil {
-			log.Printf("[ERROR] Unable to unlock renew call for %s: %v", name, err)
+			log.Printf("[ERROR][%s] Renew: Unable to unlock '%s': %v", name, lockKey, err)
 		}
 	}()
 
@@ -302,6 +304,12 @@ func (c *acmeClient) Renew(name string) error {
 		return err
 	}
 
+	// Check if renew is still needed - might have been renewed while waiting for lock
+	if !c.config.managedCertNeedsRenewal(certRes) {
+		log.Printf("[INFO][%s] Renew: Certificate appears to have been renewed already", name)
+		return nil
+	}
+
 	// Perform renewal and retry if necessary, but not too many times.
 	var newCertMeta *certificate.Resource
 	var success bool

vendor/github.com/mholt/certmagic/config.go

@@ -21,6 +21,7 @@ import (
 	"time"
 
 	"github.com/xenolf/lego/certcrypto"
+	"github.com/xenolf/lego/certificate"
 	"github.com/xenolf/lego/challenge"
 	"github.com/xenolf/lego/challenge/tlsalpn01"
 	"github.com/xenolf/lego/lego"
@@ -277,12 +278,6 @@ func (cfg *Config) ObtainCert(name string, interactive bool) error {
 		return nil
 	}
 
-	// we expect this to be a new site; if the
-	// cert already exists, then no-op
-	if cfg.certCache.storage.Exists(StorageKeys.SiteCert(cfg.CA, name)) {
-		return nil
-	}
-
 	client, err := cfg.newACMEClient(interactive)
 	if err != nil {
 		return err
@@ -317,24 +312,37 @@ func (cfg *Config) RevokeCert(domain string, interactive bool) error {
 	return client.Revoke(domain)
 }
 
-// TLSConfig returns a TLS configuration that
-// can be used to configure TLS listeners. It
-// supports the TLS-ALPN challenge and serves
-// up certificates managed by cfg.
+// TLSConfig is an opinionated method that returns a
+// recommended, modern TLS configuration that can be
+// used to configure TLS listeners, which also supports
+// the TLS-ALPN challenge and serves up certificates
+// managed by cfg.
+//
+// Unlike the package TLS() function, this method does
+// not, by itself, enable certificate management for
+// any domain names.
+//
+// Feel free to further customize the returned tls.Config,
+// but do not mess with the GetCertificate or NextProtos
+// fields unless you know what you're doing, as they're
+// necessary to solve the TLS-ALPN challenge.
 func (cfg *Config) TLSConfig() *tls.Config {
 	return &tls.Config{
+		// these two fields necessary for TLS-ALPN challenge
 		GetCertificate: cfg.GetCertificate,
 		NextProtos:     []string{"h2", "http/1.1", tlsalpn01.ACMETLS1Protocol},
+
+		// the rest recommended for modern TLS servers
+		MinVersion: tls.VersionTLS12,
+		CurvePreferences: []tls.CurveID{
+			tls.X25519,
+			tls.CurveP256,
+		},
+		CipherSuites:             preferredDefaultCipherSuites(),
+		PreferServerCipherSuites: true,
 	}
 }
 
-// RenewAllCerts triggers a renewal check of all
-// certificates in the cache. It only renews
-// certificates if they need to be renewed.
-// func (cfg *Config) RenewAllCerts(interactive bool) error {
-// 	return cfg.certCache.RenewManagedCertificates(interactive)
-// }
-
 // preObtainOrRenewChecks perform a few simple checks before
 // obtaining or renewing a certificate with ACME, and returns
 // whether this name should be skipped (like if it's not
@@ -356,3 +364,27 @@ func (cfg *Config) preObtainOrRenewChecks(name string, allowPrompts bool) (bool,
 
 	return false, nil
 }
+
+// storageHasCertResources returns true if the storage
+// associated with cfg's certificate cache has all the
+// resources related to the certificate for domain: the
+// certificate, the private key, and the metadata.
+func (cfg *Config) storageHasCertResources(domain string) bool {
+	certKey := StorageKeys.SiteCert(cfg.CA, domain)
+	keyKey := StorageKeys.SitePrivateKey(cfg.CA, domain)
+	metaKey := StorageKeys.SiteMeta(cfg.CA, domain)
+	return cfg.certCache.storage.Exists(certKey) &&
+		cfg.certCache.storage.Exists(keyKey) &&
+		cfg.certCache.storage.Exists(metaKey)
+}
+
+// managedCertNeedsRenewal returns true if certRes is
+// expiring soon or already expired, or if the process
+// of checking the expiration returned an error.
+func (cfg *Config) managedCertNeedsRenewal(certRes certificate.Resource) bool {
+	cert, err := cfg.makeCertificate(certRes.Certificate, certRes.PrivateKey)
+	if err != nil {
+		return true
+	}
+	return cert.NeedsRenewal()
+}

vendor/github.com/mholt/certmagic/crypto.go

@@ -19,12 +19,14 @@ import (
 	"crypto/ecdsa"
 	"crypto/rsa"
 	"crypto/sha256"
+	"crypto/tls"
 	"crypto/x509"
 	"encoding/json"
 	"encoding/pem"
 	"fmt"
 	"hash/fnv"
 
+	"github.com/klauspost/cpuid"
 	"github.com/xenolf/lego/certificate"
 )
 
@@ -153,3 +155,34 @@ func hashCertificateChain(certChain [][]byte) string {
 	}
 	return fmt.Sprintf("%x", h.Sum(nil))
 }
+
+// preferredDefaultCipherSuites returns an appropriate
+// cipher suite to use depending on hardware support
+// for AES-NI.
+//
+// See https://github.com/mholt/caddy/issues/1674
+func preferredDefaultCipherSuites() []uint16 {
+	if cpuid.CPU.AesNi() {
+		return defaultCiphersPreferAES
+	}
+	return defaultCiphersPreferChaCha
+}
+
+var (
+	defaultCiphersPreferAES = []uint16{
+		tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
+		tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
+		tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+		tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
+		tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
+	}
+	defaultCiphersPreferChaCha = []uint16{
+		tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
+		tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
+		tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
+		tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+	}
+)

vendor/github.com/mholt/certmagic/storage.go

@@ -64,22 +64,24 @@ type Storage interface {
 // Locker facilitates synchronization of certificate tasks across
 // machines and networks.
 type Locker interface {
-	// TryLock will attempt to acquire the lock for key. If a
-	// lock could be obtained, nil values are returned as no
-	// waiting is required. If not (meaning another process is
-	// already working on key), a Waiter value will be returned,
-	// upon which you should Wait() until it is finished.
+	// Lock acquires the lock for key, blocking until the lock
+	// can be obtained or an error is returned. Note that, even
+	// after acquiring a lock, an idempotent operation may have
+	// already been performed by another process that acquired
+	// the lock before - so always check to make sure idempotent
+	// operations still need to be performed after acquiring the
+	// lock.
 	//
 	// The actual implementation of obtaining of a lock must be
-	// an atomic operation so that multiple TryLock calls at the
+	// an atomic operation so that multiple Lock calls at the
 	// same time always results in only one caller receiving the
-	// lock. TryLock always returns without waiting.
+	// lock at any given time.
 	//
 	// To prevent deadlocks, all implementations (where this concern
 	// is relevant) should put a reasonable expiration on the lock in
 	// case Unlock is unable to be called due to some sort of network
 	// or system failure or crash.
-	TryLock(key string) (Waiter, error)
+	Lock(key string) error
 
 	// Unlock releases the lock for key. This method must ONLY be
 	// called after a successful call to TryLock where no Waiter was
@@ -89,20 +91,6 @@ type Locker interface {
 	// TryLock or if Unlock was not called at all. Unlock should also
 	// clean up any unused resources allocated during TryLock.
 	Unlock(key string) error
-
-	// UnlockAllObtained removes all locks obtained by this process,
-	// upon which others may be waiting. The importer should call
-	// this on shutdowns (and crashes, ideally) to avoid leaving stale
-	// locks, but Locker implementations must NOT rely on this being
-	// the case and should anticipate and handle stale locks. Errors
-	// should be printed or logged, since there could be multiple,
-	// with no good way to handle them anyway.
-	UnlockAllObtained()
-}
-
-// Waiter is a type that can block until a lock is released.
-type Waiter interface {
-	Wait()
 }
 
 // KeyInfo holds information about a key in storage.
@@ -281,7 +269,7 @@ type ErrNotExist interface {
 
 // defaultFileStorage is a convenient, default storage
 // implementation using the local file system.
-var defaultFileStorage = FileStorage{Path: dataDir()}
+var defaultFileStorage = &FileStorage{Path: dataDir()}
 
 // DefaultStorage is the default Storage implementation.
 var DefaultStorage Storage = defaultFileStorage

vendor/manifest

@@ -138,7 +138,7 @@
 			"importpath": "github.com/mholt/certmagic",
 			"repository": "https://github.com/mholt/certmagic",
 			"vcs": "git",
-			"revision": "fe722057f2654b33cd528b8fd8b90e53fa495564",
+			"revision": "a3b276a1b44e1c2c3dcab752729976ea04f4839b",
 			"branch": "master",
 			"notests": true
 		},