From 96919acc9d583ef11ea1f9c72a9991fb3f8aab9f Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Mon, 15 May 2023 10:47:30 -0600 Subject: caddyhttp: Refactor cert Managers (fix #5415) (#5533) --- caddyconfig/httpcaddyfile/tlsapp.go | 4 ++ go.mod | 2 +- go.sum | 4 +- modules/caddyhttp/autohttps.go | 115 ++++++++++++++++++++---------------- modules/caddytls/automation.go | 21 ++++--- modules/caddytls/certmanagers.go | 15 +---- 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 { -- cgit v1.2.3