caddyhttp: Refactor cert Managers (fix #5415) (#5533)

6 files changed, 84 insertions, 77 deletions

diff --git a/caddyconfig/httpcaddyfile/tlsapp.go b/caddyconfig/httpcaddyfile/tlsapp.go
index 2021970..c63569e 100644
--- a/caddyconfig/httpcaddyfile/tlsapp.go
+++ b/caddyconfig/httpcaddyfile/tlsapp.go
@@ -218,6 +218,10 @@ func (st ServerType) buildTLSApp(
 			if len(ap.Issuers) == 0 {
 				var internal, external []string
 				for _, s := range ap.SubjectsRaw {
+					// do not create Issuers for Tailscale domains; they will be given a Manager instead
+					if strings.HasSuffix(strings.ToLower(s), ".ts.net") {
+						continue
+					}
 					if !certmagic.SubjectQualifiesForCert(s) {
 						return nil, warnings, fmt.Errorf("subject does not qualify for certificate: '%s'", s)
 					}
diff --git a/go.mod b/go.mod
index 003da3f..0f15b86 100644
--- a/go.mod
+++ b/go.mod
@@ -7,7 +7,7 @@ require (
 	github.com/Masterminds/sprig/v3 v3.2.3
 	github.com/alecthomas/chroma/v2 v2.7.0
 	github.com/aryann/difflib v0.0.0-20210328193216-ff5ff6dc229b
-	github.com/caddyserver/certmagic v0.17.3-0.20230510193943-53140d52202c
+	github.com/caddyserver/certmagic v0.17.3-0.20230511183644-8728b186fa68
 	github.com/dustin/go-humanize v1.0.1
 	github.com/go-chi/chi v4.1.2+incompatible
 	github.com/google/cel-go v0.14.0
diff --git a/go.sum b/go.sum
index afd511b..778721e 100644
--- a/go.sum
+++ b/go.sum
@@ -97,8 +97,8 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
 github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
 github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kBD4zp0CCIs=
 github.com/boltdb/bolt v1.3.1/go.mod h1:clJnj/oiGkjum5o1McbSZDSLxVThjynRyGBgiAx27Ps=
-github.com/caddyserver/certmagic v0.17.3-0.20230510193943-53140d52202c h1:pEMS0l8kE/5xxrncv+Qq81fzr29R+zk++E7KAYiyBe4=
-github.com/caddyserver/certmagic v0.17.3-0.20230510193943-53140d52202c/go.mod h1:e0YLTnXIopZ05bBWCLzpIf1Yvk27Q90FGUmGowFRDY8=
+github.com/caddyserver/certmagic v0.17.3-0.20230511183644-8728b186fa68 h1:B3ZszrtQ3ksok0Cby1mGT/4T513vxXpOUCf5862c/NQ=
+github.com/caddyserver/certmagic v0.17.3-0.20230511183644-8728b186fa68/go.mod h1:e0YLTnXIopZ05bBWCLzpIf1Yvk27Q90FGUmGowFRDY8=
 github.com/casbin/casbin/v2 v2.1.2/go.mod h1:YcPU1XXisHhLzuxH9coDNf2FbKpjGlbCg3n9yuLkIJQ=
 github.com/cenkalti/backoff v2.2.1+incompatible/go.mod h1:90ReRw6GdpyfrHakVjL/QHaoyV4aDUVVkXQJJJ3NXXM=
 github.com/cenkalti/backoff/v4 v4.2.0 h1:HN5dHm3WBOgndBH6E8V0q2jIYIR3s9yglV8k/+MN3u4=
diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go
index 86b34d3..4ade3c5 100644
--- a/modules/caddyhttp/autohttps.go
+++ b/modules/caddyhttp/autohttps.go
@@ -83,6 +83,8 @@ func (ahc AutoHTTPSConfig) Skipped(name string, skipSlice []string) bool {
 // even servers to the app, which still need to be set up with the
 // rest of them during provisioning.
 func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) error {
+	logger := app.logger.Named("auto_https")
+
 	// this map acts as a set to store the domain names
 	// for which we will manage certificates automatically
 	uniqueDomainsForCerts := make(map[string]struct{})
@@ -114,13 +116,13 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 			srv.AutoHTTPS = new(AutoHTTPSConfig)
 		}
 		if srv.AutoHTTPS.Disabled {
-			app.logger.Warn("automatic HTTPS is completely disabled for server", zap.String("server_name", srvName))
+			logger.Warn("automatic HTTPS is completely disabled for server", zap.String("server_name", srvName))
 			continue
 		}
 
 		// skip if all listeners use the HTTP port
 		if !srv.listenersUseAnyPortOtherThan(app.httpPort()) {
-			app.logger.Warn("server is listening only on the HTTP port, so no automatic HTTPS will be applied to this server",
+			logger.Warn("server is listening only on the HTTP port, so no automatic HTTPS will be applied to this server",
 				zap.String("server_name", srvName),
 				zap.Int("http_port", app.httpPort()),
 			)
@@ -134,7 +136,7 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 		// needing to specify one empty policy to enable it
 		if srv.TLSConnPolicies == nil &&
 			!srv.listenersUseAnyPortOtherThan(app.httpsPort()) {
-			app.logger.Info("server is listening only on the HTTPS port but has no TLS connection policies; adding one to enable TLS",
+			logger.Info("server is listening only on the HTTPS port but has no TLS connection policies; adding one to enable TLS",
 				zap.String("server_name", srvName),
 				zap.Int("https_port", app.httpsPort()),
 			)
@@ -186,14 +188,9 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 		// a deduplicated list of names for which to obtain certs
 		// (only if cert management not disabled for this server)
 		if srv.AutoHTTPS.DisableCerts {
-			app.logger.Warn("skipping automated certificate management for server because it is disabled", zap.String("server_name", srvName))
+			logger.Warn("skipping automated certificate management for server because it is disabled", zap.String("server_name", srvName))
 		} else {
 			for d := range serverDomainSet {
-				// the implicit Tailscale manager module will get its own certs at run-time
-				if isTailscaleDomain(d) {
-					continue
-				}
-
 				if certmagic.SubjectQualifiesForCert(d) &&
 					!srv.AutoHTTPS.Skipped(d, srv.AutoHTTPS.SkipCerts) {
 					// if a certificate for this name is already loaded,
@@ -201,7 +198,7 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 					// supposed to ignore loaded certificates
 					if !srv.AutoHTTPS.IgnoreLoadedCerts &&
 						len(app.tlsApp.AllMatchingCertificates(d)) > 0 {
-						app.logger.Info("skipping automatic certificate management because one or more matching certificates are already loaded",
+						logger.Info("skipping automatic certificate management because one or more matching certificates are already loaded",
 							zap.String("domain", d),
 							zap.String("server_name", srvName),
 						)
@@ -212,7 +209,7 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 					// can handle that, but as a courtesy, warn the user
 					if strings.Contains(d, "*") &&
 						strings.Count(strings.Trim(d, "."), ".") == 1 {
-						app.logger.Warn("most clients do not trust second-level wildcard certificates (*.tld)",
+						logger.Warn("most clients do not trust second-level wildcard certificates (*.tld)",
 							zap.String("domain", d))
 					}
 
@@ -228,11 +225,11 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 
 		// nothing left to do if auto redirects are disabled
 		if srv.AutoHTTPS.DisableRedir {
-			app.logger.Warn("automatic HTTP->HTTPS redirects are disabled", zap.String("server_name", srvName))
+			logger.Warn("automatic HTTP->HTTPS redirects are disabled", zap.String("server_name", srvName))
 			continue
 		}
 
-		app.logger.Info("enabling automatic HTTP->HTTPS redirects", zap.String("server_name", srvName))
+		logger.Info("enabling automatic HTTP->HTTPS redirects", zap.String("server_name", srvName))
 
 		// create HTTP->HTTPS redirects
 		for _, listenAddr := range srv.Listen {
@@ -272,12 +269,15 @@ 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 []string
+	var internal, tailscale []string
 uniqueDomainsLoop:
 	for d := range uniqueDomainsForCerts {
-		// whether or not there is already an automation policy for this
-		// name, we should add it to the list to manage a cert for it
-		app.allCertDomains = append(app.allCertDomains, d)
+		if !isTailscaleDomain(d) {
+			// whether or not there is already an automation policy for this
+			// name, we should add it to the list to manage a cert for it,
+			// unless it's a Tailscale domain, because we don't manage those
+			app.allCertDomains = append(app.allCertDomains, d)
+		}
 
 		// some names we've found might already have automation policies
 		// explicitly specified for them; we should exclude those from
@@ -295,13 +295,15 @@ uniqueDomainsLoop:
 
 		// if no automation policy exists for the name yet, we
 		// will associate it with an implicit one
-		if !certmagic.SubjectQualifiesForPublicCert(d) {
+		if isTailscaleDomain(d) {
+			tailscale = append(tailscale, d)
+		} else if !certmagic.SubjectQualifiesForPublicCert(d) {
 			internal = append(internal, d)
 		}
 	}
 
 	// ensure there is an automation policy to handle these certs
-	err := app.createAutomationPolicies(ctx, internal)
+	err := app.createAutomationPolicies(ctx, internal, tailscale)
 	if err != nil {
 		return err
 	}
@@ -424,6 +426,10 @@ redirServersLoop:
 		}
 	}
 
+	logger.Debug("adjusted config",
+		zap.Reflect("tls", app.tlsApp),
+		zap.Reflect("http", app))
+
 	return nil
 }
 
@@ -466,7 +472,7 @@ func (app *App) makeRedirRoute(redirToPort uint, matcherSet MatcherSet) Route {
 // 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, internalNames []string) error {
+func (app *App) createAutomationPolicies(ctx caddy.Context, internalNames, tailscaleNames []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
@@ -480,6 +486,22 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, internalNames []stri
 		app.tlsApp.Automation = new(caddytls.AutomationConfig)
 	}
 	for _, ap := range app.tlsApp.Automation.Policies {
+		// on-demand policies can have the tailscale manager added implicitly
+		// if there's no explicit manager configured -- for convenience
+		if ap.OnDemand && len(ap.Managers) == 0 {
+			var ts caddytls.Tailscale
+			if err := ts.Provision(ctx); err != nil {
+				return err
+			}
+			ap.Managers = []certmagic.Manager{ts}
+
+			// must reprovision the automation policy so that the underlying
+			// CertMagic config knows about the updated Managers
+			if err := ap.Provision(app.tlsApp); err != nil {
+				return fmt.Errorf("re-provisioning automation policy: %v", err)
+			}
+		}
+
 		// 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
@@ -501,22 +523,6 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, internalNames []stri
 			}
 		}
 
-		// if no external managers were configured, enable
-		// implicit Tailscale support for convenience
-		if ap.Managers == nil {
-			ts, err := implicitTailscale(ctx)
-			if err != nil {
-				return err
-			}
-			ap.Managers = []certmagic.Manager{ts}
-
-			// must reprovision the automation policy so that the underlying
-			// CertMagic config knows about the updated Managers
-			if err := ap.Provision(app.tlsApp); err != nil {
-				return fmt.Errorf("re-provisioning automation policy: %v", err)
-			}
-		}
-
 		// while we're here, is this the catch-all/base policy?
 		if !foundBasePolicy && len(ap.SubjectsRaw) == 0 {
 			basePolicy = ap
@@ -529,15 +535,6 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, internalNames []stri
 		basePolicy = new(caddytls.AutomationPolicy)
 	}
 
-	if basePolicy.Managers == nil {
-		// add implicit Tailscale integration, for harmless convenience
-		ts, err := implicitTailscale(ctx)
-		if err != nil {
-			return err
-		}
-		basePolicy.Managers = []certmagic.Manager{ts}
-	}
-
 	// if the basePolicy has an existing ACMEIssuer (particularly to
 	// 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
@@ -642,6 +639,27 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, internalNames []stri
 		}
 	}
 
+	// tailscale names go in their own automation policies because
+	// they require on-demand TLS to be enabled, which we obviously
+	// can't enable for everything
+	if len(tailscaleNames) > 0 {
+		policyCopy := *basePolicy
+		newPolicy := &policyCopy
+
+		var ts caddytls.Tailscale
+		if err := ts.Provision(ctx); err != nil {
+			return err
+		}
+
+		newPolicy.SubjectsRaw = tailscaleNames
+		newPolicy.Issuers = nil
+		newPolicy.Managers = append(newPolicy.Managers, ts)
+		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 {
@@ -720,13 +738,6 @@ func (app *App) automaticHTTPSPhase2() error {
 	return nil
 }
 
-// implicitTailscale returns a new and provisioned Tailscale module configured to be optional.
-func implicitTailscale(ctx caddy.Context) (caddytls.Tailscale, error) {
-	ts := caddytls.Tailscale{Optional: true}
-	err := ts.Provision(ctx)
-	return ts, err
-}
-
 func isTailscaleDomain(name string) bool {
 	return strings.HasSuffix(strings.ToLower(name), ".ts.net")
 }
diff --git a/modules/caddytls/automation.go b/modules/caddytls/automation.go
index 58ffe4c..1664762 100644
--- a/modules/caddytls/automation.go
+++ b/modules/caddytls/automation.go
@@ -95,9 +95,11 @@ type AutomationPolicy struct {
 	// Modules that can get a custom certificate to use for any
 	// given TLS handshake at handshake-time. Custom certificates
 	// can be useful if another entity is managing certificates
-	// and Caddy need only get it and serve it.
+	// and Caddy need only get it and serve it. Specifying a Manager
+	// enables on-demand TLS, i.e. it has the side-effect of setting
+	// the on_demand parameter to `true`.
 	//
-	// TODO: This is an EXPERIMENTAL feature. It is subject to change or removal.
+	// TODO: This is an EXPERIMENTAL feature. Subject to change or removal.
 	ManagersRaw []json.RawMessage `json:"get_certificate,omitempty" caddy:"namespace=tls.get_certificate inline_key=via"`
 
 	// If true, certificates will be requested with MustStaple. Not all
@@ -233,15 +235,18 @@ func (ap *AutomationPolicy) Provision(tlsApp *TLS) error {
 
 	// on-demand TLS
 	var ond *certmagic.OnDemandConfig
-	if ap.OnDemand {
+	if ap.OnDemand || len(ap.Managers) > 0 {
 		// ask endpoint is now required after a number of negligence cases causing abuse;
 		// but is still allowed for explicit subjects (non-wildcard, non-unbounded),
-		// and for the internal issuer since it doesn't cause ACME issuer pressure
+		// for the internal issuer since it doesn't cause ACME issuer pressure
 		if ap.isWildcardOrDefault() && !ap.onlyInternalIssuer() && (tlsApp.Automation == nil || tlsApp.Automation.OnDemand == nil || tlsApp.Automation.OnDemand.Ask == "") {
 			return fmt.Errorf("on-demand TLS cannot be enabled without an 'ask' endpoint to prevent abuse; please refer to documentation for details")
 		}
 		ond = &certmagic.OnDemandConfig{
 			DecisionFunc: func(name string) error {
+				if tlsApp.Automation == nil || tlsApp.Automation.OnDemand == nil {
+					return nil
+				}
 				if err := onDemandAskRequest(tlsApp.logger, tlsApp.Automation.OnDemand.Ask, name); err != nil {
 					// distinguish true errors from denials, because it's important to elevate actual errors
 					if errors.Is(err, errAskDenied) {
@@ -264,6 +269,7 @@ func (ap *AutomationPolicy) Provision(tlsApp *TLS) error {
 				}
 				return nil
 			},
+			Managers: ap.Managers,
 		}
 	}
 
@@ -277,10 +283,9 @@ func (ap *AutomationPolicy) Provision(tlsApp *TLS) error {
 			DisableStapling:    ap.DisableOCSPStapling,
 			ResponderOverrides: ap.OCSPOverrides,
 		},
-		Storage:  storage,
-		Issuers:  issuers,
-		Managers: ap.Managers,
-		Logger:   tlsApp.logger,
+		Storage: storage,
+		Issuers: issuers,
+		Logger:  tlsApp.logger,
 	}
 	ap.magic = certmagic.New(tlsApp.certCache, template)
 
diff --git a/modules/caddytls/certmanagers.go b/modules/caddytls/certmanagers.go
index 1b701ab..23af19d 100644
--- a/modules/caddytls/certmanagers.go
+++ b/modules/caddytls/certmanagers.go
@@ -23,14 +23,6 @@ func init() {
 
 // Tailscale is a module that can get certificates from the local Tailscale process.
 type Tailscale struct {
-	// If true, this module will operate in "best-effort" mode and
-	// ignore "soft" errors; i.e. try Tailscale, and if it doesn't connect
-	// or return a certificate, oh well. Failure to connect to Tailscale
-	// results in a no-op instead of an error. Intended for the use case
-	// where this module is added implicitly for convenience, even if
-	// Tailscale isn't necessarily running.
-	Optional bool `json:"optional,omitempty"`
-
 	logger *zap.Logger
 }
 
@@ -60,16 +52,11 @@ func (ts Tailscale) GetCertificate(ctx context.Context, hello *tls.ClientHelloIn
 
 // canHazCertificate returns true if Tailscale reports it can get a certificate for the given ClientHello.
 func (ts Tailscale) canHazCertificate(ctx context.Context, hello *tls.ClientHelloInfo) (bool, error) {
-	if ts.Optional && !strings.HasSuffix(strings.ToLower(hello.ServerName), tailscaleDomainAliasEnding) {
+	if !strings.HasSuffix(strings.ToLower(hello.ServerName), tailscaleDomainAliasEnding) {
 		return false, nil
 	}
 	status, err := tscert.GetStatus(ctx)
 	if err != nil {
-		if ts.Optional {
-			// ignore error if we don't expect/require it to work anyway, but log it for debugging
-			ts.logger.Debug("error getting tailscale status", zap.Error(err), zap.String("server_name", hello.ServerName))
-			return false, nil
-		}
 		return false, err
 	}
 	for _, domain := range status.CertDomains {