Merge branch 'certmagic-refactor' into v2

5 files changed, 150 insertions, 110 deletions

diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go
index 8b6fa4d..6b53d39 100644
--- a/modules/caddyhttp/autohttps.go
+++ b/modules/caddyhttp/autohttps.go
@@ -8,7 +8,7 @@ import (
 
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/modules/caddytls"
-	"github.com/mholt/certmagic"
+	"github.com/caddyserver/certmagic"
 	"go.uber.org/zap"
 )
 
@@ -42,12 +42,10 @@ type AutoHTTPSConfig struct {
 	// enabled. To force automated certificate management
 	// regardless of loaded certificates, set this to true.
 	IgnoreLoadedCerts bool `json:"ignore_loaded_certificates,omitempty"`
-
-	domainSet map[string]struct{}
 }
 
 // Skipped returns true if name is in skipSlice, which
-// should be one of the Skip* fields on ahc.
+// should be either the Skip or SkipCerts field on ahc.
 func (ahc AutoHTTPSConfig) Skipped(name string, skipSlice []string) bool {
 	for _, n := range skipSlice {
 		if name == n {
@@ -68,6 +66,8 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 	// addresses to the routes that do HTTP->HTTPS redirects
 	lnAddrRedirRoutes := make(map[string]Route)
 
+	uniqueDomainsForCerts := make(map[string]struct{})
+
 	for srvName, srv := range app.Servers {
 		// as a prerequisite, provision route matchers; this is
 		// required for all routes on all servers, and must be
@@ -116,8 +116,8 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 			srv.TLSConnPolicies = defaultConnPolicies
 		}
 
-		// find all qualifying domain names in this server
-		srv.AutoHTTPS.domainSet = make(map[string]struct{})
+		// find all qualifying domain names (deduplicated) in this server
+		serverDomainSet := make(map[string]struct{})
 		for routeIdx, route := range srv.Routes {
 			for matcherSetIdx, matcherSet := range route.MatcherSets {
 				for matcherIdx, m := range matcherSet {
@@ -131,7 +131,7 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 							}
 							if certmagic.HostQualifies(d) &&
 								!srv.AutoHTTPS.Skipped(d, srv.AutoHTTPS.Skip) {
-								srv.AutoHTTPS.domainSet[d] = struct{}{}
+								serverDomainSet[d] = struct{}{}
 							}
 						}
 					}
@@ -141,10 +141,29 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 
 		// nothing more to do here if there are no
 		// domains that qualify for automatic HTTPS
-		if len(srv.AutoHTTPS.domainSet) == 0 {
+		if len(serverDomainSet) == 0 {
 			continue
 		}
 
+		// for all the hostnames we found, filter them so we have
+		// a deduplicated list of names for which to obtain certs
+		for d := range serverDomainSet {
+			if !srv.AutoHTTPS.Skipped(d, srv.AutoHTTPS.SkipCerts) {
+				// if a certificate for this name is already loaded,
+				// don't obtain another one for it, unless we are
+				// 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",
+						zap.String("domain", d),
+						zap.String("server_name", srvName),
+					)
+					continue
+				}
+				uniqueDomainsForCerts[d] = struct{}{}
+			}
+		}
+
 		// tell the server to use TLS if it is not already doing so
 		if srv.TLSConnPolicies == nil {
 			srv.TLSConnPolicies = defaultConnPolicies
@@ -209,6 +228,19 @@ 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))
+	for d := range uniqueDomainsForCerts {
+		app.allCertDomains = append(app.allCertDomains, d)
+	}
+
+	// ensure there is an automation policy to handle these certs
+	err := app.createAutomationPolicy(ctx)
+	if err != nil {
+		return err
+	}
+
 	// if there are HTTP->HTTPS redirects to add, do so now
 	if len(lnAddrRedirRoutes) == 0 {
 		return nil
@@ -258,28 +290,78 @@ redirRoutesLoop:
 	return nil
 }
 
-// automaticHTTPSPhase2 attaches a TLS app pointer to each
-// server. This phase must occur after provisioning, and
-// at the beginning of the app start, before starting each
-// of the servers.
-func (app *App) automaticHTTPSPhase2() error {
-	tlsAppIface, err := app.ctx.App("tls")
-	if err != nil {
-		return fmt.Errorf("getting tls app: %v", err)
+// createAutomationPolicy ensures that certificates for this app are
+// managed properly; for example, it's implied that the HTTPPort
+// should also be the port the HTTP challenge is solved on; the same
+// for HTTPS port and TLS-ALPN challenge also. We need to tell the
+// TLS app to manage these certs by honoring those port configurations,
+// so we either find an existing matching automation policy with an
+// ACME issuer, or make a new one and append it.
+func (app *App) createAutomationPolicy(ctx caddy.Context) error {
+	var matchingPolicy *caddytls.AutomationPolicy
+	var acmeIssuer *caddytls.ACMEIssuer
+	if app.tlsApp.Automation != nil {
+		// maybe we can find an exisitng one that matches; this is
+		// useful if the user made a single automation policy to
+		// set the CA endpoint to a test/staging endpoint (very
+		// common), but forgot to customize the ports here, while
+		// setting them in the HTTP app instead (I did this too
+		// many times)
+		for _, ap := range app.tlsApp.Automation.Policies {
+			if len(ap.Hosts) == 0 {
+				matchingPolicy = ap
+				break
+			}
+		}
+	}
+	if matchingPolicy != nil {
+		// if it has an ACME issuer, maybe we can just use that
+		acmeIssuer, _ = matchingPolicy.Issuer.(*caddytls.ACMEIssuer)
+	}
+	if acmeIssuer.Challenges == nil {
+		acmeIssuer.Challenges = new(caddytls.ChallengesConfig)
+	}
+	if acmeIssuer.Challenges.HTTP == nil {
+		acmeIssuer.Challenges.HTTP = new(caddytls.HTTPChallengeConfig)
+	}
+	if acmeIssuer.Challenges.HTTP.AlternatePort == 0 {
+		// don't overwrite existing explicit config
+		acmeIssuer.Challenges.HTTP.AlternatePort = app.HTTPPort
+	}
+	if acmeIssuer.Challenges.TLSALPN == nil {
+		acmeIssuer.Challenges.TLSALPN = new(caddytls.TLSALPNChallengeConfig)
+	}
+	if acmeIssuer.Challenges.TLSALPN.AlternatePort == 0 {
+		// don't overwrite existing explicit config
+		acmeIssuer.Challenges.TLSALPN.AlternatePort = app.HTTPSPort
 	}
-	tlsApp := tlsAppIface.(*caddytls.TLS)
 
-	// set the tlsApp pointer before starting any
-	// challenges, since it is required to solve
-	// the ACME HTTP challenge
-	for _, srv := range app.Servers {
-		srv.tlsApp = tlsApp
+	if matchingPolicy == nil {
+		// if there was no matching policy, we'll have to append our own
+		err := app.tlsApp.AddAutomationPolicy(&caddytls.AutomationPolicy{
+			Hosts:  app.allCertDomains,
+			Issuer: acmeIssuer,
+		})
+		if err != nil {
+			return err
+		}
+	} else {
+		// if there was an existing matching policy, we need to reprovision
+		// its issuer (because we just changed its port settings and it has
+		// to re-build its stored certmagic config template with the new
+		// values), then re-assign the Issuer pointer on the policy struct
+		// because our type assertion changed the address
+		err := acmeIssuer.Provision(ctx)
+		if err != nil {
+			return err
+		}
+		matchingPolicy.Issuer = acmeIssuer
 	}
 
 	return nil
 }
 
-// automaticHTTPSPhase3 begins certificate management for
+// 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
 // of app start, after all the servers have been started.
@@ -289,72 +371,17 @@ func (app *App) automaticHTTPSPhase2() error {
 // first, then our servers would fail to bind to them,
 // which would be bad, since CertMagic's bindings are
 // temporary and don't serve the user's sites!).
-func (app *App) automaticHTTPSPhase3() error {
-	// begin managing certificates for enabled servers
-	for srvName, srv := range app.Servers {
-		if srv.AutoHTTPS == nil ||
-			srv.AutoHTTPS.Disabled ||
-			len(srv.AutoHTTPS.domainSet) == 0 {
-			continue
-		}
-
-		// marshal the domains into a slice
-		var domains, domainsForCerts []string
-		for d := range srv.AutoHTTPS.domainSet {
-			domains = append(domains, d)
-			if !srv.AutoHTTPS.Skipped(d, srv.AutoHTTPS.SkipCerts) {
-				// if a certificate for this name is already loaded,
-				// don't obtain another one for it, unless we are
-				// supposed to ignore loaded certificates
-				if !srv.AutoHTTPS.IgnoreLoadedCerts &&
-					len(srv.tlsApp.AllMatchingCertificates(d)) > 0 {
-					app.logger.Info("skipping automatic certificate management because one or more matching certificates are already loaded",
-						zap.String("domain", d),
-						zap.String("server_name", srvName),
-					)
-					continue
-				}
-				domainsForCerts = append(domainsForCerts, d)
-			}
-		}
-
-		// ensure that these certificates are managed properly;
-		// for example, it's implied that the HTTPPort should also
-		// be the port the HTTP challenge is solved on, and so
-		// for HTTPS port and TLS-ALPN challenge also - we need
-		// to tell the TLS app to manage these certs by honoring
-		// those port configurations
-		acmeManager := &caddytls.ACMEManagerMaker{
-			Challenges: &caddytls.ChallengesConfig{
-				HTTP: &caddytls.HTTPChallengeConfig{
-					AlternatePort: app.HTTPPort, // we specifically want the user-configured port, if any
-				},
-				TLSALPN: &caddytls.TLSALPNChallengeConfig{
-					AlternatePort: app.HTTPSPort, // we specifically want the user-configured port, if any
-				},
-			},
-		}
-		if srv.tlsApp.Automation == nil {
-			srv.tlsApp.Automation = new(caddytls.AutomationConfig)
-		}
-		srv.tlsApp.Automation.Policies = append(srv.tlsApp.Automation.Policies,
-			&caddytls.AutomationPolicy{
-				Hosts:      domainsForCerts,
-				Management: acmeManager,
-			})
-
-		// manage their certificates
-		app.logger.Info("enabling automatic TLS certificate management",
-			zap.Strings("domains", domainsForCerts),
-		)
-		err := srv.tlsApp.Manage(domainsForCerts)
-		if err != nil {
-			return fmt.Errorf("%s: managing certificate for %s: %s", srvName, domains, err)
-		}
-
-		// no longer needed; allow GC to deallocate
-		srv.AutoHTTPS.domainSet = nil
+func (app *App) automaticHTTPSPhase2() error {
+	if len(app.allCertDomains) == 0 {
+		return nil
 	}
-
+	app.logger.Info("enabling automatic TLS certificate management",
+		zap.Strings("domains", app.allCertDomains),
+	)
+	err := app.tlsApp.Manage(app.allCertDomains)
+	if err != nil {
+		return fmt.Errorf("managing certificates for %v: %s", app.allCertDomains, err)
+	}
+	app.allCertDomains = nil // no longer needed; allow GC to deallocate
 	return nil
 }
diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index 02e4468..6ad70f5 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -28,8 +28,9 @@ import (
 	"time"
 
 	"github.com/caddyserver/caddy/v2"
+	"github.com/caddyserver/caddy/v2/modules/caddytls"
+	"github.com/caddyserver/certmagic"
 	"github.com/lucas-clemente/quic-go/http3"
-	"github.com/mholt/certmagic"
 	"go.uber.org/zap"
 )
 
@@ -122,6 +123,10 @@ type App struct {
 
 	ctx    caddy.Context
 	logger *zap.Logger
+	tlsApp *caddytls.TLS
+
+	// used temporarily between phases 1 and 2 of auto HTTPS
+	allCertDomains []string
 }
 
 // CaddyModule returns the Caddy module information.
@@ -134,6 +139,12 @@ func (App) CaddyModule() caddy.ModuleInfo {
 
 // Provision sets up the app.
 func (app *App) Provision(ctx caddy.Context) error {
+	// store some references
+	tlsAppIface, err := ctx.App("tls")
+	if err != nil {
+		return fmt.Errorf("getting tls app: %v", err)
+	}
+	app.tlsApp = tlsAppIface.(*caddytls.TLS)
 	app.ctx = ctx
 	app.logger = ctx.Logger(app)
 
@@ -144,12 +155,14 @@ func (app *App) Provision(ctx caddy.Context) error {
 	// this provisions the matchers for each route,
 	// and prepares auto HTTP->HTTPS redirects, and
 	// is required before we provision each server
-	err := app.automaticHTTPSPhase1(ctx, repl)
+	err = app.automaticHTTPSPhase1(ctx, repl)
 	if err != nil {
 		return err
 	}
 
+	// prepare each server
 	for srvName, srv := range app.Servers {
+		srv.tlsApp = app.tlsApp
 		srv.logger = app.logger.Named("log")
 		srv.errorLogger = app.logger.Named("log.error")
 
@@ -202,9 +215,14 @@ func (app *App) Provision(ctx caddy.Context) error {
 			if err != nil {
 				return fmt.Errorf("server %s: setting up server error handling routes: %v", srvName, err)
 			}
-
 			srv.errorHandlerChain = srv.Errors.Routes.Compile(errorEmptyHandler)
 		}
+
+		// prepare the TLS connection policies
+		err = srv.TLSConnPolicies.Provision(ctx)
+		if err != nil {
+			return fmt.Errorf("server %s: setting up TLS connection policies: %v", srvName, err)
+		}
 	}
 
 	return nil
@@ -238,14 +256,6 @@ func (app *App) Validate() error {
 // Start runs the app. It finishes automatic HTTPS if enabled,
 // including management of certificates.
 func (app *App) Start() error {
-	// give each server a pointer to the TLS app;
-	// this is required before they are started so
-	// they can solve ACME challenges
-	err := app.automaticHTTPSPhase2()
-	if err != nil {
-		return fmt.Errorf("enabling automatic HTTPS, phase 2: %v", err)
-	}
-
 	for srvName, srv := range app.Servers {
 		s := &http.Server{
 			ReadTimeout:       time.Duration(srv.ReadTimeout),
@@ -279,10 +289,7 @@ func (app *App) Start() error {
 				if len(srv.TLSConnPolicies) > 0 &&
 					int(listenAddr.StartPort+portOffset) != app.httpPort() {
 					// create TLS listener
-					tlsCfg, err := srv.TLSConnPolicies.TLSConfig(app.ctx)
-					if err != nil {
-						return fmt.Errorf("%s/%s: making TLS configuration: %v", listenAddr.Network, hostport, err)
-					}
+					tlsCfg := srv.TLSConnPolicies.TLSConfig(app.ctx)
 					ln = tls.NewListener(ln, tlsCfg)
 
 					/////////
@@ -318,7 +325,7 @@ func (app *App) Start() error {
 
 	// finish automatic HTTPS by finally beginning
 	// certificate management
-	err = app.automaticHTTPSPhase3()
+	err := app.automaticHTTPSPhase2()
 	if err != nil {
 		return fmt.Errorf("finalizing automatic HTTPS: %v", err)
 	}
diff --git a/modules/caddyhttp/fileserver/command.go b/modules/caddyhttp/fileserver/command.go
index e553182..fa6560b 100644
--- a/modules/caddyhttp/fileserver/command.go
+++ b/modules/caddyhttp/fileserver/command.go
@@ -26,7 +26,7 @@ import (
 	"github.com/caddyserver/caddy/v2/caddyconfig/httpcaddyfile"
 	caddycmd "github.com/caddyserver/caddy/v2/cmd"
 	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
-	"github.com/mholt/certmagic"
+	"github.com/caddyserver/certmagic"
 )
 
 func init() {
diff --git a/modules/caddyhttp/httpcache/httpcache.go b/modules/caddyhttp/httpcache/httpcache.go
index f8bdde8..605a183 100644
--- a/modules/caddyhttp/httpcache/httpcache.go
+++ b/modules/caddyhttp/httpcache/httpcache.go
@@ -16,6 +16,7 @@ package httpcache
 
 import (
 	"bytes"
+	"context"
 	"encoding/gob"
 	"fmt"
 	"io"
@@ -108,7 +109,8 @@ func (c *Cache) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp
 		return next.ServeHTTP(w, r)
 	}
 
-	ctx := getterContext{w, r, next}
+	getterCtx := getterContext{w, r, next}
+	ctx := context.WithValue(r.Context(), getterContextCtxKey, getterCtx)
 
 	// TODO: rigorous performance testing
 
@@ -152,8 +154,8 @@ func (c *Cache) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp
 	return nil
 }
 
-func (c *Cache) getter(ctx groupcache.Context, key string, dest groupcache.Sink) error {
-	combo := ctx.(getterContext)
+func (c *Cache) getter(ctx context.Context, key string, dest groupcache.Sink) error {
+	combo := ctx.Value(getterContextCtxKey).(getterContext)
 
 	// the buffer will store the gob-encoded header, then the body
 	buf := bufPool.Get().(*bytes.Buffer)
@@ -228,6 +230,10 @@ var errUncacheable = fmt.Errorf("uncacheable")
 
 const groupName = "http_requests"
 
+type ctxKey string
+
+const getterContextCtxKey ctxKey = "getter_context"
+
 // Interface guards
 var (
 	_ caddy.Provisioner           = (*Cache)(nil)
diff --git a/modules/caddyhttp/reverseproxy/command.go b/modules/caddyhttp/reverseproxy/command.go
index c47447f..6f70d14 100644
--- a/modules/caddyhttp/reverseproxy/command.go
+++ b/modules/caddyhttp/reverseproxy/command.go
@@ -29,7 +29,7 @@ import (
 	caddycmd "github.com/caddyserver/caddy/v2/cmd"
 	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
 	"github.com/caddyserver/caddy/v2/modules/caddyhttp/headers"
-	"github.com/mholt/certmagic"
+	"github.com/caddyserver/certmagic"
 )
 
 func init() {