Clean up some significant portions of the TLS management code

caddyhttp/httpserver/https.go

@@ -23,7 +23,7 @@ func activateHTTPS(cctx caddy.Context) error {
 
 	// place certificates and keys on disk
 	for _, c := range ctx.siteConfigs {
-		err := c.TLS.ObtainCert(operatorPresent)
+		err := c.TLS.ObtainCert(c.TLS.Hostname, operatorPresent)
 		if err != nil {
 			return err
 		}

caddytls/client.go

@@ -18,11 +18,13 @@ import (
 // acmeMu ensures that only one ACME challenge occurs at a time.
 var acmeMu sync.Mutex
 
-// ACMEClient is an acme.Client with custom state attached.
+// ACMEClient is a wrapper over acme.Client with
+// some custom state attached. It is used to obtain,
+// renew, and revoke certificates with ACME.
 type ACMEClient struct {
-	*acme.Client
 	AllowPrompts bool
 	config       *Config
+	acmeClient   *acme.Client
 }
 
 // newACMEClient creates a new ACMEClient given an email and whether
@@ -100,7 +102,11 @@ var newACMEClient = func(config *Config, allowPrompts bool) (*ACMEClient, error)
 		}
 	}
 
-	c := &ACMEClient{Client: client, AllowPrompts: allowPrompts, config: config}
+	c := &ACMEClient{
+		AllowPrompts: allowPrompts,
+		config:       config,
+		acmeClient:   client,
+	}
 
 	if config.DNSProvider == "" {
 		// Use HTTP and TLS-SNI challenges by default
@@ -116,15 +122,15 @@ var newACMEClient = func(config *Config, allowPrompts bool) (*ACMEClient, error)
 
 		// See if TLS challenge needs to be handled by our own facilities
 		if caddy.HasListenerWithAddress(net.JoinHostPort(config.ListenHost, TLSSNIChallengePort)) {
-			c.SetChallengeProvider(acme.TLSSNI01, tlsSniSolver{})
+			c.acmeClient.SetChallengeProvider(acme.TLSSNI01, tlsSniSolver{})
 		}
 
 		// Always respect user's bind preferences by using config.ListenHost
-		err := c.SetHTTPAddress(net.JoinHostPort(config.ListenHost, useHTTPPort))
+		err := c.acmeClient.SetHTTPAddress(net.JoinHostPort(config.ListenHost, useHTTPPort))
 		if err != nil {
 			return nil, err
 		}
-		err = c.SetTLSAddress(net.JoinHostPort(config.ListenHost, ""))
+		err = c.acmeClient.SetTLSAddress(net.JoinHostPort(config.ListenHost, ""))
 		if err != nil {
 			return nil, err
 		}
@@ -145,23 +151,50 @@ var newACMEClient = func(config *Config, allowPrompts bool) (*ACMEClient, error)
 		}
 
 		// Use the DNS challenge exclusively
-		c.ExcludeChallenges([]acme.Challenge{acme.HTTP01, acme.TLSSNI01})
-		c.SetChallengeProvider(acme.DNS01, prov)
+		c.acmeClient.ExcludeChallenges([]acme.Challenge{acme.HTTP01, acme.TLSSNI01})
+		c.acmeClient.SetChallengeProvider(acme.DNS01, prov)
 	}
 
 	return c, nil
 }
 
-// Obtain obtains a single certificate for names. It stores the certificate
-// on the disk if successful.
-func (c *ACMEClient) Obtain(names []string) error {
+// 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.
+//
+// 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 {
+	// Get access to ACME storage
+	storage, err := c.config.StorageFor(c.config.CAUrl)
+	if err != nil {
+		return err
+	}
+
+	// We must lock the obtain with the storage engine
+	if lockObtained, err := storage.LockRegister(name); err != nil {
+		return err
+	} else if !lockObtained {
+		log.Printf("[INFO] Certificate for %v is already being obtained elsewhere", name)
+		return nil
+	}
+	defer func() {
+		if err := storage.UnlockRegister(name); err != nil {
+			log.Printf("[ERROR] Unable to unlock obtain lock for %v: %v", name, err)
+		}
+	}()
+
 Attempts:
 	for attempts := 0; attempts < 2; attempts++ {
-		namesObtaining.Add(names)
+		namesObtaining.Add([]string{name})
 		acmeMu.Lock()
-		certificate, failures := c.ObtainCertificate(names, true, nil)
+		certificate, failures := c.acmeClient.ObtainCertificate([]string{name}, true, nil)
 		acmeMu.Unlock()
-		namesObtaining.Remove(names)
+		namesObtaining.Remove([]string{name})
 		if len(failures) > 0 {
 			// Error - try to fix it or report it to the user and abort
 			var errMsg string             // we'll combine all the failures into a single error message
@@ -178,7 +211,7 @@ Attempts:
 						promptedForAgreement = true
 					}
 					if Agreed || !c.AllowPrompts {
-						err := c.AgreeToTOS()
+						err := c.acmeClient.AgreeToTOS()
 						if err != nil {
 							return errors.New("error agreeing to updated terms: " + err.Error())
 						}
@@ -193,13 +226,9 @@ Attempts:
 		}
 
 		// Success - immediately save the certificate resource
-		storage, err := c.config.StorageFor(c.config.CAUrl)
-		if err != nil {
-			return err
-		}
 		err = saveCertResource(storage, certificate)
 		if err != nil {
-			return fmt.Errorf("error saving assets for %v: %v", names, err)
+			return fmt.Errorf("error saving assets for %v: %v", name, err)
 		}
 
 		break
@@ -208,13 +237,11 @@ Attempts:
 	return nil
 }
 
-// Renew renews the managed certificate for name. Right now our storage
-// mechanism only supports one name per certificate, so this function only
-// accepts one domain as input. It can be easily modified to support SAN
-// certificates if, one day, they become desperately needed enough that our
-// storage mechanism is upgraded to be more complex to support SAN certs.
+// Renew renews the managed certificate for name. This function is
+// safe for concurrent use.
 //
-// Anyway, this function is safe for concurrent use.
+// 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 {
 	// Get access to ACME storage
 	storage, err := c.config.StorageFor(c.config.CAUrl)
@@ -251,7 +278,7 @@ func (c *ACMEClient) Renew(name string) error {
 	for attempts := 0; attempts < 2; attempts++ {
 		namesObtaining.Add([]string{name})
 		acmeMu.Lock()
-		newCertMeta, err = c.RenewCertificate(certMeta, true)
+		newCertMeta, err = c.acmeClient.RenewCertificate(certMeta, true)
 		acmeMu.Unlock()
 		namesObtaining.Remove([]string{name})
 		if err == nil {
@@ -259,10 +286,10 @@ func (c *ACMEClient) Renew(name string) error {
 			break
 		}
 
-		// If the legal terms changed and need to be agreed to again,
-		// we can handle that.
+		// If the legal terms were updated and need to be
+		// agreed to again, we can handle that.
 		if _, ok := err.(acme.TOSError); ok {
-			err := c.AgreeToTOS()
+			err := c.acmeClient.AgreeToTOS()
 			if err != nil {
 				return err
 			}
@@ -304,7 +331,7 @@ func (c *ACMEClient) Revoke(name string) error {
 		return err
 	}
 
-	err = c.Client.RevokeCertificate(siteData.Cert)
+	err = c.acmeClient.RevokeCertificate(siteData.Cert)
 	if err != nil {
 		return err
 	}

caddytls/config.go

@@ -3,13 +3,9 @@ package caddytls
 import (
 	"crypto/tls"
 	"crypto/x509"
-	"encoding/json"
-	"errors"
 	"fmt"
 	"io/ioutil"
-	"time"
 
-	"log"
 	"net/url"
 	"strings"
 
@@ -116,24 +112,21 @@ type OnDemandState struct {
 	// If it reaches MaxObtain, on-demand issuances must fail.
 	ObtainedCount int32
 
-	// Based on max_certs in tls config, it specifies the
+	// Set from max_certs in tls config, it specifies the
 	// maximum number of certificates that can be issued.
 	MaxObtain int32
 }
 
-// ObtainCert obtains a certificate for c.Hostname, as long as a certificate
-// does not already exist in storage on disk. It only obtains and stores
-// certificates (and their keys) to disk, it does not load them into memory.
-// If allowPrompts is true, the user may be shown a prompt. If proxyACME is
-// true, the relevant ACME challenges will be proxied to the alternate port.
-func (c *Config) ObtainCert(allowPrompts bool) error {
-	return c.obtainCertName(c.Hostname, allowPrompts)
-}
-
-// obtainCertName gets a certificate for name using the ACME config c
-// if c and name both qualify. It places the certificate in storage.
-// It is a no-op if the storage already has a certificate for name.
-func (c *Config) obtainCertName(name string, allowPrompts bool) error {
+// ObtainCert obtains a certificate for name using c, as long
+// as a certificate does not already exist in storage for that
+// name. The name must qualify and c must be flagged as Managed.
+// This function is a no-op if storage already has a certificate
+// for name.
+//
+// It only obtains and stores certificates (and their keys),
+// it does not load them into memory. If allowPrompts is true,
+// the user may be shown a prompt.
+func (c *Config) ObtainCert(name string, allowPrompts bool) error {
 	if !c.Managed || !HostQualifies(name) {
 		return nil
 	}
@@ -142,29 +135,13 @@ func (c *Config) obtainCertName(name string, allowPrompts bool) error {
 	if err != nil {
 		return err
 	}
-
 	siteExists, err := storage.SiteExists(name)
 	if err != nil {
 		return err
 	}
-
 	if siteExists {
 		return nil
 	}
-
-	// We must lock the obtain with the storage engine
-	if lockObtained, err := storage.LockRegister(name); err != nil {
-		return err
-	} else if !lockObtained {
-		log.Printf("[INFO] Certificate for %v is already being obtained elsewhere", name)
-		return nil
-	}
-	defer func() {
-		if err := storage.UnlockRegister(name); err != nil {
-			log.Printf("[ERROR] Unable to unlock obtain lock for %v: %v", name, err)
-		}
-	}()
-
 	if c.ACMEEmail == "" {
 		c.ACMEEmail = getEmail(storage, allowPrompts)
 	}
@@ -173,86 +150,17 @@ func (c *Config) obtainCertName(name string, allowPrompts bool) error {
 	if err != nil {
 		return err
 	}
-
-	return client.Obtain([]string{name})
+	return client.Obtain(name)
 }
 
-// RenewCert renews the certificate for c.Hostname. If there is already a lock
-// on renewal, this will not perform the renewal and no error will occur.
-func (c *Config) RenewCert(allowPrompts bool) error {
-	return c.renewCertName(c.Hostname, allowPrompts)
-}
-
-// renewCertName renews the certificate for the given name. If there is already
-// a lock on renewal, this will not perform the renewal and no error will
-// occur.
-func (c *Config) renewCertName(name string, allowPrompts bool) error {
-	storage, err := c.StorageFor(c.CAUrl)
-	if err != nil {
-		return err
-	}
-
-	// We must lock the renewal with the storage engine
-	if lockObtained, err := storage.LockRegister(name); err != nil {
-		return err
-	} else if !lockObtained {
-		log.Printf("[INFO] Certificate for %v is already being renewed elsewhere", name)
-		return nil
-	}
-	defer func() {
-		if err := storage.UnlockRegister(name); err != nil {
-			log.Printf("[ERROR] Unable to unlock renewal lock for %v: %v", name, err)
-		}
-	}()
-
-	// Prepare for renewal (load PEM cert, key, and meta)
-	siteData, err := storage.LoadSite(name)
-	if err != nil {
-		return err
-	}
-	var certMeta acme.CertificateResource
-	err = json.Unmarshal(siteData.Meta, &certMeta)
-	certMeta.Certificate = siteData.Cert
-	certMeta.PrivateKey = siteData.Key
-
+// RenewCert renews the certificate for name using c. It stows the
+// renewed certificate and its assets in storage if successful.
+func (c *Config) RenewCert(name string, allowPrompts bool) error {
 	client, err := newACMEClient(c, allowPrompts)
 	if err != nil {
 		return err
 	}
-
-	// Perform renewal and retry if necessary, but not too many times.
-	var newCertMeta acme.CertificateResource
-	var success bool
-	for attempts := 0; attempts < 2; attempts++ {
-		namesObtaining.Add([]string{name})
-		acmeMu.Lock()
-		newCertMeta, err = client.RenewCertificate(certMeta, true)
-		acmeMu.Unlock()
-		namesObtaining.Remove([]string{name})
-		if err == nil {
-			success = true
-			break
-		}
-
-		// If the legal terms were updated and need to be
-		// agreed to again, we can handle that.
-		if _, ok := err.(acme.TOSError); ok {
-			err := client.AgreeToTOS()
-			if err != nil {
-				return err
-			}
-			continue
-		}
-
-		// For any other kind of error, wait 10s and try again.
-		time.Sleep(10 * time.Second)
-	}
-
-	if !success {
-		return errors.New("too many renewal attempts; last error: " + err.Error())
-	}
-
-	return saveCertResource(storage, newCertMeta)
+	return client.Renew(name)
 }
 
 // StorageFor obtains a TLS Storage instance for the given CA URL which should

caddytls/filestorage.go

@@ -123,7 +123,6 @@ func (s FileStorage) readFile(file string) ([]byte, error) {
 		return nil, ErrNotExist(err)
 	}
 	return b, err
-
 }
 
 // SiteExists implements Storage.SiteExists by checking for the presence of

caddytls/handshake.go

@@ -172,22 +172,25 @@ func (cg configGroup) obtainOnDemandCertificate(name string, cfg *Config) (Certi
 		return cg.getCertDuringHandshake(name, true, false)
 	}
 
-	// looks like it's up to us to do all the work and obtain the cert
+	// looks like it's up to us to do all the work and obtain the cert.
+	// make a chan others can wait on if needed
 	wait = make(chan struct{})
 	obtainCertWaitChans[name] = wait
 	obtainCertWaitChansMu.Unlock()
 
-	// Unblock waiters and delete waitgroup when we return
-	defer func() {
-		obtainCertWaitChansMu.Lock()
-		close(wait)
-		delete(obtainCertWaitChans, name)
-		obtainCertWaitChansMu.Unlock()
-	}()
-
+	// do the obtain
 	log.Printf("[INFO] Obtaining new certificate for %s", name)
+	err := cfg.ObtainCert(name, false)
 
-	if err := cfg.obtainCertName(name, false); err != nil {
+	// immediately unblock anyone waiting for it; doing this in
+	// a defer would risk deadlock because of the recursive call
+	// to getCertDuringHandshake below when we return!
+	obtainCertWaitChansMu.Lock()
+	close(wait)
+	delete(obtainCertWaitChans, name)
+	obtainCertWaitChansMu.Unlock()
+
+	if err != nil {
 		// Failed to solve challenge, so don't allow another on-demand
 		// issue for this name to be attempted for a little while.
 		failedIssuanceMu.Lock()
@@ -208,7 +211,7 @@ func (cg configGroup) obtainOnDemandCertificate(name string, cfg *Config) (Certi
 	lastIssueTime = time.Now()
 	lastIssueTimeMu.Unlock()
 
-	// The certificate is already on disk; now just start over to load it and serve it
+	// certificate is already on disk; now just start over to load it and serve it
 	return cg.getCertDuringHandshake(name, true, false)
 }
 
@@ -265,17 +268,18 @@ func (cg configGroup) renewDynamicCertificate(name string, cfg *Config) (Certifi
 	obtainCertWaitChans[name] = wait
 	obtainCertWaitChansMu.Unlock()
 
-	// unblock waiters and delete waitgroup when we return
-	defer func() {
-		obtainCertWaitChansMu.Lock()
-		close(wait)
-		delete(obtainCertWaitChans, name)
-		obtainCertWaitChansMu.Unlock()
-	}()
-
+	// do the renew
 	log.Printf("[INFO] Renewing certificate for %s", name)
+	err := cfg.RenewCert(name, false)
+
+	// immediately unblock anyone waiting for it; doing this in
+	// a defer would risk deadlock because of the recursive call
+	// to getCertDuringHandshake below when we return!
+	obtainCertWaitChansMu.Lock()
+	close(wait)
+	delete(obtainCertWaitChans, name)
+	obtainCertWaitChansMu.Unlock()
 
-	err := cfg.renewCertName(name, false)
 	if err != nil {
 		return Certificate{}, err
 	}

caddytls/maintain.go

@@ -99,12 +99,20 @@ func RenewManagedCertificates(allowPrompts bool) (err error) {
 				continue
 			}
 
-			// This works well because managed certs are only associated with one name per config.
-			// Note, the renewal inside here may not actually occur and no error will be returned
-			// due to renewal lock (i.e. because a renewal is already happening). This lack of
-			// error is by intention to force cache invalidation as though it has renewed.
-			err := cert.Config.RenewCert(allowPrompts)
+			// Get the name which we should use to renew this certificate;
+			// we only support managing certificates with one name per cert,
+			// so this should be easy. We can't rely on cert.Config.Hostname
+			// because it may be a wildcard value from the Caddyfile (e.g.
+			// *.something.com) which, as of 2016, is not supported by ACME.
+			var renewName string
+			for _, name := range cert.Names {
+				if name != "" {
+					renewName = name
+					break
+				}
+			}
 
+			err := cert.Config.RenewCert(renewName, allowPrompts)
 			if err != nil {
 				if allowPrompts && timeLeft < 0 {
 					// Certificate renewal failed, the operator is present, and the certificate