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

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<==>ACME and
nonpublic<==>Internal, but that is not always the case. The new logic
still assumes nonpublic<==>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.

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

modules/caddyhttp/autohttps.go

@@ -407,63 +407,151 @@ 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
+	// 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
+	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
+			}
 		}
 
-	// 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
-	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 {
+		// while we're here, is this the catch-all/base policy?
+		if !foundBasePolicy && len(ap.Subjects) == 0 {
 			basePolicy = ap
-				break
-			}
+			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
+	// 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)
+	}
+
+	// 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
+		}
+	}
+
+	// never overwrite any other issuer that might already be configured
+	if basePolicy.Issuer == nil {
+		basePolicy.Issuer = baseACMEIssuer
+	}
+
+	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)
+
+		// 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 it, since we are
-		// bypassing the JSON-unmarshaling step
-		if prov, ok := issuer.(caddy.Provisioner); ok {
-			err := prov.Provision(ctx)
+		// 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
 		}
 	}
-		newPolicy.Issuer = issuer
-		newPolicy.Subjects = hosts
 
-		return app.tlsApp.AddAutomationPolicy(newPolicy)
+	// we just changed a lot of stuff, so double-check that it's all good
+	err := app.tlsApp.Validate()
+	if err != nil {
+		return err
 	}
 
-	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)
-		}
+	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)
@@ -476,6 +564,7 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 		// don't overwrite existing explicit config
 		if acmeIssuer.Challenges.HTTP.AlternatePort == 0 {
 			acmeIssuer.Challenges.HTTP.AlternatePort = app.HTTPPort
+			anyChanges = true
 		}
 	}
 	if app.HTTPSPort > 0 {
@@ -485,25 +574,12 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 		// don't overwrite existing explicit config
 		if acmeIssuer.Challenges.TLSALPN.AlternatePort == 0 {
 			acmeIssuer.Challenges.TLSALPN.AlternatePort = app.HTTPSPort
+			anyChanges = true
 		}
 	}
-		if err := addPolicy(acmeIssuer, publicNames); err != nil {
-			return err
+	if anyChanges {
+		return acmeIssuer.Provision(app.ctx)
 	}
-	}
-
-	if len(internalNames) > 0 {
-		internalIssuer := new(caddytls.InternalIssuer)
-		if err := addPolicy(internalIssuer, internalNames); err != nil {
-			return err
-		}
-	}
-
-	err := app.tlsApp.Validate()
-	if err != nil {
-		return err
-	}
-
 	return nil
 }
 

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 {

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
 	}