tls/http: Fix auto-HTTPS logic w/rt default issuers (fixes #3164)

mholt · mholt · commit d692d503a3d3 · 2020-03-20T20:25:46.000-06:00
The comments in the code should explain the new logic thoroughly.
The basic problem for the issue was that we were overriding a catch-all
automation policy's explicitly-configured issuer with our own, for names
that we thought looked like public names. In other words, one could
configure an internal issuer for all names, but then our auto HTTPS
would create a new policy for public-looking names that uses the
default ACME issuer, because we assume public&lt;==&gt;ACME and
nonpublic&lt;==&gt;Internal, but that is not always the case. The new logic
still assumes nonpublic&lt;==&gt;Internal (on catch-all policies only), but
no longer assumes that public-looking names always use an ACME issuer.

Also fix a bug where HTTPPort and HTTPSPort from the HTTP app weren't
being carried through to ACME issuers properly. It required a bit of
refactoring.
diff --git a/context.go b/context.go
@@ -377,9 +377,14 @@ func (ctx Context) loadModuleInline(moduleNameKey, moduleScope string, raw json.
 	return val, nil
 }
 
-// App returns the configured app named name. If no app with
-// that name is currently configured, a new empty one will be
-// instantiated. (The app module must still be registered.)
+// App returns the configured app named name. If that app has
+// not yet been loaded and provisioned, it will be immediately
+// loaded and provisioned. If no app with that name is
+// configured, a new empty one will be instantiated instead.
+// (The app module must still be registered.) This must not be
+// called during the Provision/Validate phase to reference a
+// module's own host app (since the parent app module is still
+// in the process of being provisioned, it is not yet ready).
 func (ctx Context) App(name string) (interface{}, error) {
 	if app, ok := ctx.cfg.apps[name]; ok {
 		return app, nil
diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go
@@ -407,98 +407,129 @@ redirServersLoop:
 // 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 {
-	// nothing to do if no names to manage certs for
-	if len(publicNames) == 0 && len(internalNames) == 0 {
-		return nil
-	}
-
-	// start by finding a base policy that the user may have defined
-	// which should, in theory, apply to any policies derived from it;
-	// typically this would be a "catch-all" policy with no host filter
+	// 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
+	// look for a base (or "catch-all" / default) automation policy,
+	// which we're going to essentially require, to make sure it has
+	// those defaults, too
 	var basePolicy *caddytls.AutomationPolicy
-	if app.tlsApp.Automation != nil {
-		for _, ap := range app.tlsApp.Automation.Policies {
-			// if an existing policy matches (specifically, a catch-all policy),
-			// we should inherit from it, because that is what the user expects;
-			// this is very common for user setting a default issuer, with a
-			// custom CA endpoint, for example - whichever one we choose must
-			// have a host list that is a superset of the policy we make...
-			// the policy with no host filter is guaranteed to qualify
-			if len(ap.Subjects) == 0 {
-				basePolicy = ap
-				break
+	var foundBasePolicy bool
+	if app.tlsApp.Automation == nil {
+		// we will expect this to not be nil from now on
+		app.tlsApp.Automation = new(caddytls.AutomationConfig)
+	}
+	for _, ap := range app.tlsApp.Automation.Policies {
+		// 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.(*caddytls.ACMEIssuer); ok {
+			err := app.fillInACMEIssuer(acmeIssuer)
+			if err != nil {
+				return err
 			}
 		}
+
+		// while we're here, is this the catch-all/base policy?
+		if !foundBasePolicy && len(ap.Subjects) == 0 {
+			basePolicy = ap
+			foundBasePolicy = true
+		}
 	}
+
 	if basePolicy == nil {
+		// no base policy found, we will make one!
 		basePolicy = new(caddytls.AutomationPolicy)
 	}
 
-	// addPolicy adds an automation policy that uses issuer for hosts.
-	addPolicy := func(issuer certmagic.Issuer, hosts []string) error {
-		// shallow-copy the matching policy; we want to inherit
-		// from it, not replace it... this takes two lines to
-		// overrule compiler optimizations
-		policyCopy := *basePolicy
-		newPolicy := &policyCopy
+	// if the basePolicy has an existing ACMEIssuer, let's
+	// use it, otherwise we'll make one
+	baseACMEIssuer, _ := basePolicy.Issuer.(*caddytls.ACMEIssuer)
+	if baseACMEIssuer == nil {
+		// note that this happens if basePolicy.Issuer is nil
+		// OR if it is not nil but is not an ACMEIssuer
+		baseACMEIssuer = new(caddytls.ACMEIssuer)
+	}
 
-		// very important to provision it, since we are
-		// bypassing the JSON-unmarshaling step
-		if prov, ok := issuer.(caddy.Provisioner); ok {
-			err := prov.Provision(ctx)
-			if err != nil {
-				return err
-			}
+	// 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
+	if !foundBasePolicy {
+		err := app.fillInACMEIssuer(baseACMEIssuer)
+		if err != nil {
+			return err
 		}
-		newPolicy.Issuer = issuer
-		newPolicy.Subjects = hosts
+	}
 
-		return app.tlsApp.AddAutomationPolicy(newPolicy)
+	// never overwrite any other issuer that might already be configured
+	if basePolicy.Issuer == nil {
+		basePolicy.Issuer = baseACMEIssuer
 	}
 
-	if len(publicNames) > 0 {
-		var acmeIssuer *caddytls.ACMEIssuer
-		// if it has an ACME issuer, maybe we can just use that
-		// TODO: we might need a deep copy here, like a Clone() method on ACMEIssuer...
-		acmeIssuer, _ = basePolicy.Issuer.(*caddytls.ACMEIssuer)
-		if acmeIssuer == nil {
-			acmeIssuer = new(caddytls.ACMEIssuer)
-		}
-		if app.HTTPPort > 0 || app.HTTPSPort > 0 {
-			if acmeIssuer.Challenges == nil {
-				acmeIssuer.Challenges = new(caddytls.ChallengesConfig)
-			}
-		}
-		if app.HTTPPort > 0 {
-			if acmeIssuer.Challenges.HTTP == nil {
-				acmeIssuer.Challenges.HTTP = new(caddytls.HTTPChallengeConfig)
-			}
-			// don't overwrite existing explicit config
-			if acmeIssuer.Challenges.HTTP.AlternatePort == 0 {
-				acmeIssuer.Challenges.HTTP.AlternatePort = app.HTTPPort
-			}
-		}
-		if app.HTTPSPort > 0 {
-			if acmeIssuer.Challenges.TLSALPN == nil {
-				acmeIssuer.Challenges.TLSALPN = new(caddytls.TLSALPNChallengeConfig)
-			}
-			// don't overwrite existing explicit config
-			if acmeIssuer.Challenges.TLSALPN.AlternatePort == 0 {
-				acmeIssuer.Challenges.TLSALPN.AlternatePort = app.HTTPSPort
-			}
-		}
-		if err := addPolicy(acmeIssuer, publicNames); err != nil {
+	if !foundBasePolicy {
+		// there was no base policy to begin with, so add
+		// our base/catch-all policy - this will serve the
+		// public-looking names as well as any other names
+		// that don't match any other policy
+		app.tlsApp.AddAutomationPolicy(basePolicy)
+	} else {
+		// a base policy already existed; we might have
+		// changed it, so re-provision it
+		err := basePolicy.Provision(app.tlsApp)
+		if err != nil {
 			return err
 		}
 	}
 
+	// public names will be taken care of by the base (catch-all)
+	// policy, which we've ensured exists if not already specified;
+	// internal names, however, need to be handled by an internal
+	// issuer, which we need to make a new policy for, scoped to
+	// just those names (yes, this logic is a bit asymmetric, but
+	// it works, because our assumed/natural default issuer is an
+	// ACME issuer)
 	if len(internalNames) > 0 {
 		internalIssuer := new(caddytls.InternalIssuer)
-		if err := addPolicy(internalIssuer, internalNames); err != nil {
+
+		// shallow-copy the base policy; we want to inherit
+		// from it, not replace it... this takes two lines to
+		// overrule compiler optimizations
+		policyCopy := *basePolicy
+		newPolicy := &policyCopy
+
+		// very important to provision the issuer, since we
+		// are bypassing the JSON-unmarshaling step
+		if err := internalIssuer.Provision(ctx); err != nil {
+			return err
+		}
+
+		// this policy should apply only to the given names
+		// and should use our issuer -- yes, this overrides
+		// any issuer that may have been set in the base
+		// policy, but we do this because these names do not
+		// already have a policy associated with them, which
+		// is easy to do; consider the case of a Caddyfile
+		// that has only "localhost" as a name, but sets the
+		// default/global ACME CA to the Let's Encrypt staging
+		// endpoint... they probably don't intend to change the
+		// fundamental set of names that setting applies to,
+		// rather they just want to change the CA for the set
+		// of names that would normally use the production API;
+		// anyway, that gets into the weeds a bit...
+		newPolicy.Subjects = internalNames
+		newPolicy.Issuer = internalIssuer
+
+		err := app.tlsApp.AddAutomationPolicy(newPolicy)
+		if err != nil {
 			return err
 		}
 	}
 
+	// we just changed a lot of stuff, so double-check that it's all good
 	err := app.tlsApp.Validate()
 	if err != nil {
 		return err
@@ -507,6 +538,51 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 	return nil
 }
 
+// fillInACMEIssuer fills in default values into acmeIssuer that
+// are defined in app; these values at time of writing are just
+// app.HTTPPort and app.HTTPSPort, which are used by ACMEIssuer.
+// Sure, we could just use the global/CertMagic defaults, but if
+// a user has configured those ports in the HTTP app, it makes
+// sense to use them in the TLS app too, even if they forgot (or
+// were too lazy, like me) to set it in each automation policy
+// that uses it -- this just makes things a little less tedious
+// for the user, so they don't have to repeat those ports in
+// potentially many places. This function never steps on existing
+// config values. If any changes are made, acmeIssuer is
+// reprovisioned. acmeIssuer must not be nil.
+func (app *App) fillInACMEIssuer(acmeIssuer *caddytls.ACMEIssuer) error {
+	var anyChanges bool
+	if app.HTTPPort > 0 || app.HTTPSPort > 0 {
+		if acmeIssuer.Challenges == nil {
+			acmeIssuer.Challenges = new(caddytls.ChallengesConfig)
+		}
+	}
+	if app.HTTPPort > 0 {
+		if acmeIssuer.Challenges.HTTP == nil {
+			acmeIssuer.Challenges.HTTP = new(caddytls.HTTPChallengeConfig)
+		}
+		// don't overwrite existing explicit config
+		if acmeIssuer.Challenges.HTTP.AlternatePort == 0 {
+			acmeIssuer.Challenges.HTTP.AlternatePort = app.HTTPPort
+			anyChanges = true
+		}
+	}
+	if app.HTTPSPort > 0 {
+		if acmeIssuer.Challenges.TLSALPN == nil {
+			acmeIssuer.Challenges.TLSALPN = new(caddytls.TLSALPNChallengeConfig)
+		}
+		// don't overwrite existing explicit config
+		if acmeIssuer.Challenges.TLSALPN.AlternatePort == 0 {
+			acmeIssuer.Challenges.TLSALPN.AlternatePort = app.HTTPSPort
+			anyChanges = true
+		}
+	}
+	if anyChanges {
+		return acmeIssuer.Provision(app.ctx)
+	}
+	return nil
+}
+
 // automaticHTTPSPhase2 begins certificate management for
 // all names in the qualifying domain set for each server.
 // This phase must occur after provisioning and at the end
diff --git a/modules/caddytls/automation.go b/modules/caddytls/automation.go
@@ -115,8 +115,8 @@ type AutomationPolicy struct {
 	storage certmagic.Storage
 }
 
-// provision converts ap into a CertMagic config.
-func (ap *AutomationPolicy) provision(tlsApp *TLS) error {
+// Provision sets up ap and builds its underlying CertMagic config.
+func (ap *AutomationPolicy) Provision(tlsApp *TLS) error {
 	// policy-specific storage implementation
 	if ap.StorageRaw != nil {
 		val, err := tlsApp.ctx.LoadModule(ap, "StorageRaw")
@@ -157,8 +157,8 @@ func (ap *AutomationPolicy) provision(tlsApp *TLS) error {
 	// none the subjects do not 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 explictly configured AND if the list of
-	// subjects is non-empty
+	// issuer is not explictly 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 {
@@ -174,7 +174,7 @@ func (ap *AutomationPolicy) provision(tlsApp *TLS) error {
 		}
 	}
 
-	// load and provision the issuer module
+	// load and provision any explicitly-configured issuer module
 	if ap.IssuerRaw != nil {
 		val, err := tlsApp.ctx.LoadModule(ap, "IssuerRaw")
 		if err != nil {
diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go
@@ -94,12 +94,12 @@ func (t *TLS) Provision(ctx caddy.Context) error {
 		t.Automation = new(AutomationConfig)
 	}
 	t.Automation.defaultAutomationPolicy = new(AutomationPolicy)
-	err := t.Automation.defaultAutomationPolicy.provision(t)
+	err := t.Automation.defaultAutomationPolicy.Provision(t)
 	if err != nil {
 		return fmt.Errorf("provisioning default automation policy: %v", err)
 	}
 	for i, ap := range t.Automation.Policies {
-		err := ap.provision(t)
+		err := ap.Provision(t)
 		if err != nil {
 			return fmt.Errorf("provisioning automation policy %d: %v", i, err)
 		}
@@ -300,7 +300,7 @@ func (t *TLS) AddAutomationPolicy(ap *AutomationPolicy) error {
 	if t.Automation == nil {
 		t.Automation = new(AutomationConfig)
 	}
-	err := ap.provision(t)
+	err := ap.Provision(t)
 	if err != nil {
 		return err
 	}

Original file line number	Diff line number	Diff line change
`@@ -94,12 +94,12 @@ func (t *TLS) Provision(ctx caddy.Context) error {`
`94`	`94`	`t.Automation = new(AutomationConfig)`
`95`	`95`	`}`
`96`	`96`	`t.Automation.defaultAutomationPolicy = new(AutomationPolicy)`
`97`		`- err := t.Automation.defaultAutomationPolicy.provision(t)`
	`97`	`+ err := t.Automation.defaultAutomationPolicy.Provision(t)`
`98`	`98`	`if err != nil {`
`99`	`99`	`return fmt.Errorf("provisioning default automation policy: %v", err)`
`100`	`100`	`}`
`101`	`101`	`for i, ap := range t.Automation.Policies {`
`102`		`- err := ap.provision(t)`
	`102`	`+ err := ap.Provision(t)`
`103`	`103`	`if err != nil {`
`104`	`104`	`return fmt.Errorf("provisioning automation policy %d: %v", i, err)`
`105`	`105`	`}`
`@@ -300,7 +300,7 @@ func (t TLS) AddAutomationPolicy(ap AutomationPolicy) error {`
`300`	`300`	`if t.Automation == nil {`
`301`	`301`	`t.Automation = new(AutomationConfig)`
`302`	`302`	`}`
`303`		`- err := ap.provision(t)`
	`303`	`+ err := ap.Provision(t)`
`304`	`304`	`if err != nil {`
`305`	`305`	`return err`
`306`	`306`	`}`