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

2 files changed, 14 insertions, 22 deletions

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 {