caddytls: Support multiple issuers (#3862)

* caddytls: Support multiple issuers

Defaults are Let's Encrypt and ZeroSSL.

There are probably bugs.

* Commit updated integration tests, d'oh

* Update go.mod

5 files changed, 125 insertions, 89 deletions

diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go
index 0780981..805a37c 100644
--- a/modules/caddyhttp/autohttps.go
+++ b/modules/caddyhttp/autohttps.go
@@ -241,7 +241,7 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 	// we now have a list of all the unique names for which we need certs;
 	// turn the set into a slice so that phase 2 can use it
 	app.allCertDomains = make([]string, 0, len(uniqueDomainsForCerts))
-	var internal, external []string
+	var internal []string
 uniqueDomainsLoop:
 	for d := range uniqueDomainsForCerts {
 		// whether or not there is already an automation policy for this
@@ -264,15 +264,13 @@ uniqueDomainsLoop:
 
 		// if no automation policy exists for the name yet, we
 		// will associate it with an implicit one
-		if certmagic.SubjectQualifiesForPublicCert(d) {
-			external = append(external, d)
-		} else {
+		if !certmagic.SubjectQualifiesForPublicCert(d) {
 			internal = append(internal, d)
 		}
 	}
 
 	// ensure there is an automation policy to handle these certs
-	err := app.createAutomationPolicies(ctx, external, internal)
+	err := app.createAutomationPolicies(ctx, internal)
 	if err != nil {
 		return err
 	}
@@ -430,7 +428,7 @@ redirServersLoop:
 // automation policy exists, it will be shallow-copied and used as the
 // base for the new ones (this is important for preserving behavior the
 // user intends to be "defaults").
-func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, internalNames []string) error {
+func (app *App) createAutomationPolicies(ctx caddy.Context, internalNames []string) error {
 	// before we begin, loop through the existing automation policies
 	// and, for any ACMEIssuers we find, make sure they're filled in
 	// with default values that might be specified in our HTTP app; also
@@ -447,16 +445,23 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 		// set up default issuer -- honestly, this is only
 		// really necessary because the HTTP app is opinionated
 		// and has settings which could be inferred as new
-		// defaults for the ACMEIssuer in the TLS app
-		if ap.Issuer == nil {
-			ap.Issuer = new(caddytls.ACMEIssuer)
-		}
-		if acmeIssuer, ok := ap.Issuer.(acmeCapable); ok {
-			err := app.fillInACMEIssuer(acmeIssuer.GetACMEIssuer())
+		// defaults for the ACMEIssuer in the TLS app (such as
+		// what the HTTP and HTTPS ports are)
+		if ap.Issuers == nil {
+			var err error
+			ap.Issuers, err = caddytls.DefaultIssuers(ctx)
 			if err != nil {
 				return err
 			}
 		}
+		for _, iss := range ap.Issuers {
+			if acmeIssuer, ok := iss.(acmeCapable); ok {
+				err := app.fillInACMEIssuer(acmeIssuer.GetACMEIssuer())
+				if err != nil {
+					return err
+				}
+			}
+		}
 
 		// while we're here, is this the catch-all/base policy?
 		if !foundBasePolicy && len(ap.Subjects) == 0 {
@@ -471,11 +476,14 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 	}
 
 	// if the basePolicy has an existing ACMEIssuer (particularly to
-	// include any type that embeds/wraps an ACMEIssuer), let's use it,
-	// otherwise we'll make one
+	// include any type that embeds/wraps an ACMEIssuer), let's use it
+	// (I guess we just use the first one?), otherwise we'll make one
 	var baseACMEIssuer *caddytls.ACMEIssuer
-	if acmeWrapper, ok := basePolicy.Issuer.(acmeCapable); ok {
-		baseACMEIssuer = acmeWrapper.GetACMEIssuer()
+	for _, iss := range basePolicy.Issuers {
+		if acmeWrapper, ok := iss.(acmeCapable); ok {
+			baseACMEIssuer = acmeWrapper.GetACMEIssuer()
+			break
+		}
 	}
 	if baseACMEIssuer == nil {
 		// note that this happens if basePolicy.Issuer is nil
@@ -485,7 +493,7 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 
 	// if there was a base policy to begin with, we already
 	// filled in its issuer's defaults; if there wasn't, we
-	// stil need to do that
+	// still need to do that
 	if !foundBasePolicy {
 		err := app.fillInACMEIssuer(baseACMEIssuer)
 		if err != nil {
@@ -494,8 +502,20 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 	}
 
 	// never overwrite any other issuer that might already be configured
-	if basePolicy.Issuer == nil {
-		basePolicy.Issuer = baseACMEIssuer
+	if basePolicy.Issuers == nil {
+		var err error
+		basePolicy.Issuers, err = caddytls.DefaultIssuers(ctx)
+		if err != nil {
+			return err
+		}
+		for _, iss := range basePolicy.Issuers {
+			if acmeIssuer, ok := iss.(acmeCapable); ok {
+				err := app.fillInACMEIssuer(acmeIssuer.GetACMEIssuer())
+				if err != nil {
+					return err
+				}
+			}
+		}
 	}
 
 	if !foundBasePolicy {
@@ -549,8 +569,7 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 		// of names that would normally use the production API;
 		// anyway, that gets into the weeds a bit...
 		newPolicy.Subjects = internalNames
-		newPolicy.Issuer = internalIssuer
-
+		newPolicy.Issuers = []certmagic.Issuer{internalIssuer}
 		err := app.tlsApp.AddAutomationPolicy(newPolicy)
 		if err != nil {
 			return err
diff --git a/modules/caddytls/acmeissuer.go b/modules/caddytls/acmeissuer.go
index 6466229..7c79c7e 100644
--- a/modules/caddytls/acmeissuer.go
+++ b/modules/caddytls/acmeissuer.go
@@ -97,6 +97,15 @@ func (ACMEIssuer) CaddyModule() caddy.ModuleInfo {
 func (iss *ACMEIssuer) Provision(ctx caddy.Context) error {
 	iss.logger = ctx.Logger(iss)
 
+	// expand email address, if non-empty
+	if iss.Email != "" {
+		email, err := caddy.NewReplacer().ReplaceOrErr(iss.Email, true, true)
+		if err != nil {
+			return fmt.Errorf("expanding email address '%s': %v", iss.Email, err)
+		}
+		iss.Email = email
+	}
+
 	// DNS providers
 	if iss.Challenges != nil && iss.Challenges.DNS != nil && iss.Challenges.DNS.ProviderRaw != nil {
 		val, err := ctx.LoadModule(iss.Challenges.DNS, "ProviderRaw")
diff --git a/modules/caddytls/automation.go b/modules/caddytls/automation.go
index 1612391..509ad6e 100644
--- a/modules/caddytls/automation.go
+++ b/modules/caddytls/automation.go
@@ -23,7 +23,6 @@ import (
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/certmagic"
 	"github.com/mholt/acmez"
-	"go.uber.org/zap"
 )
 
 // AutomationConfig governs the automated management of TLS certificates.
@@ -72,8 +71,13 @@ type AutomationPolicy struct {
 	// Which subjects (hostnames or IP addresses) this policy applies to.
 	Subjects []string `json:"subjects,omitempty"`
 
-	// The module that will issue certificates. Default: internal if all
-	// subjects do not qualify for public certificates; othewise acme.
+	// The modules that may issue certificates. Default: internal if all
+	// subjects do not qualify for public certificates; othewise acme and
+	// zerossl.
+	IssuersRaw []json.RawMessage `json:"issuers,omitempty" caddy:"namespace=tls.issuance inline_key=module"`
+
+	// DEPRECATED: Use `issuers` instead (November 2020). This field will
+	// be removed in the future.
 	IssuerRaw json.RawMessage `json:"issuer,omitempty" caddy:"namespace=tls.issuance inline_key=module"`
 
 	// If true, certificates will be requested with MustStaple. Not all
@@ -103,10 +107,10 @@ type AutomationPolicy struct {
 	// load.
 	OnDemand bool `json:"on_demand,omitempty"`
 
-	// Issuer stores the decoded issuer parameters. This is only
-	// used to populate an underlying certmagic.Config's Issuer
+	// Issuers stores the decoded issuer parameters. This is only
+	// used to populate an underlying certmagic.Config's Issuers
 	// field; it is not referenced thereafter.
-	Issuer certmagic.Issuer `json:"-"`
+	Issuers []certmagic.Issuer `json:"-"`
 
 	magic   *certmagic.Config
 	storage certmagic.Storage
@@ -150,34 +154,30 @@ func (ap *AutomationPolicy) Provision(tlsApp *TLS) error {
 		}
 	}
 
-	// if this automation policy has no Issuer defined, and
-	// none of the subjects qualify for a public certificate,
-	// set the issuer to internal so that these names can all
-	// get certificates; critically, we can only do this if an
-	// issuer is not explicitly configured (IssuerRaw, vs. just
-	// Issuer) AND if the list of subjects is non-empty
-	if ap.IssuerRaw == nil && len(ap.Subjects) > 0 {
-		var anyPublic bool
-		for _, s := range ap.Subjects {
-			if certmagic.SubjectQualifiesForPublicCert(s) {
-				anyPublic = true
-				break
-			}
+	// TODO: IssuerRaw field deprecated as of November 2020 - remove this shim after deprecation is complete
+	if ap.IssuerRaw != nil {
+		tlsApp.logger.Warn("the 'issuer' field is deprecated and will be removed in the future; use 'issuers' instead; your issuer has been appended automatically for now")
+		ap.IssuersRaw = append(ap.IssuersRaw, ap.IssuerRaw)
+	}
+
+	// load and provision any explicitly-configured issuer modules
+	if ap.IssuersRaw != nil {
+		val, err := tlsApp.ctx.LoadModule(ap, "IssuersRaw")
+		if err != nil {
+			return fmt.Errorf("loading TLS automation management module: %s", err)
 		}
-		if !anyPublic {
-			tlsApp.logger.Info("setting internal issuer for automation policy that has only internal subjects but no issuer configured",
-				zap.Strings("subjects", ap.Subjects))
-			ap.IssuerRaw = json.RawMessage(`{"module":"internal"}`)
+		for _, issVal := range val.([]interface{}) {
+			ap.Issuers = append(ap.Issuers, issVal.(certmagic.Issuer))
 		}
 	}
 
-	// load and provision any explicitly-configured issuer module
-	if ap.IssuerRaw != nil {
-		val, err := tlsApp.ctx.LoadModule(ap, "IssuerRaw")
+	issuers := ap.Issuers
+	if len(issuers) == 0 {
+		var err error
+		issuers, err = DefaultIssuers(tlsApp.ctx)
 		if err != nil {
-			return fmt.Errorf("loading TLS automation management module: %s", err)
+			return err
 		}
-		ap.Issuer = val.(certmagic.Issuer)
 	}
 
 	keyType := ap.KeyType
@@ -206,12 +206,9 @@ func (ap *AutomationPolicy) Provision(tlsApp *TLS) error {
 		KeySource:          keySource,
 		OnDemand:           ond,
 		Storage:            storage,
-		Issuer:             ap.Issuer, // if nil, certmagic.New() will create one
+		Issuers:            issuers,
 		Logger:             tlsApp.logger,
 	}
-	if rev, ok := ap.Issuer.(certmagic.Revoker); ok {
-		template.Revoker = rev
-	}
 	ap.magic = certmagic.New(tlsApp.certCache, template)
 
 	// sometimes issuers may need the parent certmagic.Config in
@@ -219,13 +216,32 @@ func (ap *AutomationPolicy) Provision(tlsApp *TLS) error {
 	// access to the correct storage and cache so it can solve
 	// ACME challenges -- it's an annoying, inelegant circular
 	// dependency that I don't know how to resolve nicely!)
-	if annoying, ok := ap.Issuer.(ConfigSetter); ok {
-		annoying.SetConfig(ap.magic)
+	for _, issuer := range ap.magic.Issuers {
+		if annoying, ok := issuer.(ConfigSetter); ok {
+			annoying.SetConfig(ap.magic)
+		}
 	}
 
 	return nil
 }
 
+// DefaultIssuers returns empty but provisioned default Issuers.
+// This function is experimental and has no compatibility promises.
+func DefaultIssuers(ctx caddy.Context) ([]certmagic.Issuer, error) {
+	acme := new(ACMEIssuer)
+	err := acme.Provision(ctx)
+	if err != nil {
+		return nil, err
+	}
+	zerossl := new(ZeroSSLIssuer)
+	err = zerossl.Provision(ctx)
+	if err != nil {
+		return nil, err
+	}
+	// TODO: eventually, insert ZeroSSL into first position in the slice -- see also httpcaddyfile/tlsapp.go for where similar defaults are configured
+	return []certmagic.Issuer{acme, zerossl}, nil
+}
+
 // ChallengesConfig configures the ACME challenges.
 type ChallengesConfig struct {
 	// HTTP configures the ACME HTTP challenge. This
diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go
index 12d25ad..146eed4 100644
--- a/modules/caddytls/tls.go
+++ b/modules/caddytls/tls.go
@@ -137,7 +137,7 @@ func (t *TLS) Provision(ctx caddy.Context) error {
 			continue
 		}
 		t.Automation.defaultInternalAutomationPolicy = &AutomationPolicy{
-			IssuerRaw: json.RawMessage(`{"module":"internal"}`),
+			IssuersRaw: []json.RawMessage{json.RawMessage(`{"module":"internal"}`)},
 		}
 		err = t.Automation.defaultInternalAutomationPolicy.Provision(t)
 		if err != nil {
@@ -303,20 +303,22 @@ func (t *TLS) Manage(names []string) error {
 
 // HandleHTTPChallenge ensures that the HTTP challenge is handled for the
 // certificate named by r.Host, if it is an HTTP challenge request. It
-// requires that the automation policy for r.Host has an issue of type
-// *certmagic.ACMEManager.
+// requires that the automation policy for r.Host has an issuer of type
+// *certmagic.ACMEManager, or one that is ACME-enabled (GetACMEIssuer()).
 func (t *TLS) HandleHTTPChallenge(w http.ResponseWriter, r *http.Request) bool {
 	if !certmagic.LooksLikeHTTPChallenge(r) {
 		return false
 	}
+	// try all the issuers until we find the one that initiated the challenge
 	ap := t.getAutomationPolicyForName(r.Host)
-	if ap.magic.Issuer == nil {
-		return false
-	}
 	type acmeCapable interface{ GetACMEIssuer() *ACMEIssuer }
-	if am, ok := ap.magic.Issuer.(acmeCapable); ok {
-		iss := am.GetACMEIssuer()
-		return certmagic.NewACMEManager(iss.magic, iss.template).HandleHTTPChallenge(w, r)
+	for _, iss := range ap.magic.Issuers {
+		if am, ok := iss.(acmeCapable); ok {
+			iss := am.GetACMEIssuer()
+			if certmagic.NewACMEManager(iss.magic, iss.template).HandleHTTPChallenge(w, r) {
+				return true
+			}
+		}
 	}
 	return false
 }
diff --git a/modules/caddytls/zerosslissuer.go b/modules/caddytls/zerosslissuer.go
index d0f4950..4680d1b 100644
--- a/modules/caddytls/zerosslissuer.go
+++ b/modules/caddytls/zerosslissuer.go
@@ -59,16 +59,13 @@ func (*ZeroSSLIssuer) CaddyModule() caddy.ModuleInfo {
 // Provision sets up iss.
 func (iss *ZeroSSLIssuer) Provision(ctx caddy.Context) error {
 	iss.logger = ctx.Logger(iss)
-
 	if iss.ACMEIssuer == nil {
 		iss.ACMEIssuer = new(ACMEIssuer)
 	}
-	err := iss.ACMEIssuer.Provision(ctx)
-	if err != nil {
-		return err
+	if iss.ACMEIssuer.CA == "" {
+		iss.ACMEIssuer.CA = certmagic.ZeroSSLProductionCA
 	}
-
-	return nil
+	return iss.ACMEIssuer.Provision(ctx)
 }
 
 func (iss *ZeroSSLIssuer) newAccountCallback(ctx context.Context, am *certmagic.ACMEManager, _ acme.Account) error {
@@ -86,26 +83,22 @@ func (iss *ZeroSSLIssuer) generateEABCredentials(ctx context.Context) (*acme.EAB
 
 	// there are two ways to generate EAB credentials: authenticated with
 	// their API key, or unauthenticated with their email address
-	switch {
-	case iss.APIKey != "":
+	if iss.APIKey != "" {
 		apiKey := caddy.NewReplacer().ReplaceAll(iss.APIKey, "")
 		if apiKey == "" {
 			return nil, fmt.Errorf("missing API key: '%v'", iss.APIKey)
 		}
 		qs := url.Values{"access_key": []string{apiKey}}
 		endpoint = fmt.Sprintf("%s/eab-credentials?%s", zerosslAPIBase, qs.Encode())
-
-	case iss.Email != "":
-		email := caddy.NewReplacer().ReplaceAll(iss.Email, "")
+	} else {
+		email := iss.Email
 		if email == "" {
-			return nil, fmt.Errorf("missing email: '%v'", iss.Email)
+			iss.logger.Warn("missing email address for ZeroSSL; it is strongly recommended to set one for next time")
+			email = "caddy@zerossl.com" // special email address that preserves backwards-compat, but which black-holes dashboard features, oh well
 		}
 		endpoint = zerosslAPIBase + "/eab-credentials-email"
 		form := url.Values{"email": []string{email}}
 		body = strings.NewReader(form.Encode())
-
-	default:
-		return nil, fmt.Errorf("must configure either an API key or email address to use ZeroSSL without explicit EAB")
 	}
 
 	req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, body)
@@ -161,9 +154,6 @@ func (iss *ZeroSSLIssuer) generateEABCredentials(ctx context.Context) (*acme.EAB
 func (iss *ZeroSSLIssuer) initialize() {
 	iss.mu.Lock()
 	defer iss.mu.Unlock()
-	if iss.template.CA == "" {
-		iss.template.CA = zerosslACMEDirectory
-	}
 	if iss.template.NewAccountFunc == nil {
 		iss.template.NewAccountFunc = iss.newAccountCallback
 	}
@@ -195,15 +185,18 @@ func (iss *ZeroSSLIssuer) Revoke(ctx context.Context, cert certmagic.Certificate
 
 // UnmarshalCaddyfile deserializes Caddyfile tokens into iss.
 //
-//     ... zerossl <api_key> {
+//     ... zerossl [<api_key>] {
 //         ...
 //     }
 //
 // Any of the subdirectives for the ACME issuer can be used in the block.
 func (iss *ZeroSSLIssuer) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 	for d.Next() {
-		if !d.AllArgs(&iss.APIKey) {
-			return d.ArgErr()
+		if d.NextArg() {
+			iss.APIKey = d.Val()
+			if d.NextArg() {
+				return d.ArgErr()
+			}
 		}
 
 		if iss.ACMEIssuer == nil {
@@ -217,10 +210,7 @@ func (iss *ZeroSSLIssuer) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 	return nil
 }
 
-const (
-	zerosslACMEDirectory = "https://acme.zerossl.com/v2/DV90"
-	zerosslAPIBase       = "https://api.zerossl.com/acme"
-)
+const zerosslAPIBase = "https://api.zerossl.com/acme"
 
 // Interface guards
 var (