Some hotfixes for beta 16

5 files changed, 59 insertions, 48 deletions

diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go
index 6a23ca0..39ca0f3 100644
--- a/modules/caddyhttp/autohttps.go
+++ b/modules/caddyhttp/autohttps.go
@@ -101,10 +101,6 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 			continue
 		}
 
-		defaultConnPolicies := caddytls.ConnectionPolicies{
-			&caddytls.ConnectionPolicy{ALPN: defaultALPN},
-		}
-
 		// if all listeners are on the HTTPS port, make sure
 		// there is at least one TLS connection policy; it
 		// should be obvious that they want to use TLS without
@@ -115,7 +111,7 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 				zap.String("server_name", srvName),
 				zap.Int("https_port", app.httpsPort()),
 			)
-			srv.TLSConnPolicies = defaultConnPolicies
+			srv.TLSConnPolicies = caddytls.ConnectionPolicies{new(caddytls.ConnectionPolicy)}
 		}
 
 		// find all qualifying domain names (deduplicated) in this server
@@ -149,7 +145,8 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 		// 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 certmagic.SubjectQualifiesForCert(d) &&
+				!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
@@ -176,7 +173,7 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 
 		// tell the server to use TLS if it is not already doing so
 		if srv.TLSConnPolicies == nil {
-			srv.TLSConnPolicies = defaultConnPolicies
+			srv.TLSConnPolicies = caddytls.ConnectionPolicies{new(caddytls.ConnectionPolicy)}
 		}
 
 		// nothing left to do if auto redirects are disabled
@@ -212,13 +209,33 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 	// turn the set into a slice so that phase 2 can use it
 	app.allCertDomains = make([]string, 0, len(uniqueDomainsForCerts))
 	var internal, external []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)
+
+		// some names we've found might already have automation policies
+		// explicitly specified for them; we should exclude those from
+		// our hidden/implicit policy, since applying a name to more than
+		// one automation policy  would be confusing and an error
+		if app.tlsApp.Automation != nil {
+			for _, ap := range app.tlsApp.Automation.Policies {
+				for _, apHost := range ap.Hosts {
+					if apHost == d {
+						continue uniqueDomainsLoop
+					}
+				}
+			}
+		}
+
+		// if no automation policy exists for the name yet, we
+		// will associate it with an implicit one
 		if certmagic.SubjectQualifiesForPublicCert(d) {
 			external = append(external, d)
 		} else {
 			internal = append(internal, d)
 		}
-		app.allCertDomains = append(app.allCertDomains, d)
 	}
 
 	// ensure there is an automation policy to handle these certs
@@ -263,7 +280,7 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 			return err
 		}
 		redirTo := "https://{http.request.host}"
-		if addr.StartPort != DefaultHTTPSPort {
+		if addr.StartPort != uint(app.httpsPort()) {
 			redirTo += ":" + strconv.Itoa(int(addr.StartPort))
 		}
 		redirTo += "{http.request.uri}"
@@ -384,23 +401,23 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 	// 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 matchingPolicy *caddytls.AutomationPolicy
+	var basePolicy *caddytls.AutomationPolicy
 	if app.tlsApp.Automation != nil {
-		// 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
 		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.Hosts) == 0 {
-				matchingPolicy = ap
+				basePolicy = ap
 				break
 			}
 		}
 	}
-	if matchingPolicy == nil {
-		matchingPolicy = new(caddytls.AutomationPolicy)
+	if basePolicy == nil {
+		basePolicy = new(caddytls.AutomationPolicy)
 	}
 
 	// addPolicy adds an automation policy that uses issuer for hosts.
@@ -408,7 +425,7 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 		// shallow-copy the matching policy; we want to inherit
 		// from it, not replace it... this takes two lines to
 		// overrule compiler optimizations
-		policyCopy := *matchingPolicy
+		policyCopy := *basePolicy
 		newPolicy := &policyCopy
 
 		// very important to provision it, since we are
@@ -429,7 +446,7 @@ func (app *App) createAutomationPolicies(ctx caddy.Context, publicNames, interna
 		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, _ = matchingPolicy.Issuer.(*caddytls.ACMEIssuer)
+		acmeIssuer, _ = basePolicy.Issuer.(*caddytls.ACMEIssuer)
 		if acmeIssuer == nil {
 			acmeIssuer = new(caddytls.ACMEIssuer)
 		}
diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index 06719b5..99f215f 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -29,7 +29,6 @@ import (
 
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/modules/caddytls"
-	"github.com/caddyserver/certmagic"
 	"github.com/lucas-clemente/quic-go/http3"
 	"go.uber.org/zap"
 )
@@ -113,10 +112,6 @@ type App struct {
 	// affect functionality.
 	Servers map[string]*Server `json:"servers,omitempty"`
 
-	// DefaultSNI if set configures all certificate lookups to fallback to use
-	// this SNI name if a more specific certificate could not be found
-	DefaultSNI string `json:"default_sni,omitempty"`
-
 	servers     []*http.Server
 	h3servers   []*http3.Server
 	h3listeners []net.PacketConn
@@ -150,8 +145,6 @@ func (app *App) Provision(ctx caddy.Context) error {
 
 	repl := caddy.NewReplacer()
 
-	certmagic.Default.DefaultServerName = app.DefaultSNI
-
 	// this provisions the matchers for each route,
 	// and prepares auto HTTP->HTTPS redirects, and
 	// is required before we provision each server
@@ -278,13 +271,6 @@ func (app *App) Start() error {
 					return fmt.Errorf("%s: listening on %s: %v", listenAddr.Network, hostport, err)
 				}
 
-				// enable HTTP/2 by default
-				for _, pol := range srv.TLSConnPolicies {
-					if len(pol.ALPN) == 0 {
-						pol.ALPN = append(pol.ALPN, defaultALPN...)
-					}
-				}
-
 				// enable TLS if there is a policy and if this is not the HTTP port
 				useTLS := len(srv.TLSConnPolicies) > 0 && int(listenAddr.StartPort+portOffset) != app.httpPort()
 				if useTLS {
@@ -397,8 +383,6 @@ func (app *App) httpsPort() int {
 	return app.HTTPSPort
 }
 
-var defaultALPN = []string{"h2", "http/1.1"}
-
 // RequestMatcher is a type that can match to a request.
 // A route matcher MUST NOT modify the request, with the
 // only exception being its context.
diff --git a/modules/caddytls/acmeissuer.go b/modules/caddytls/acmeissuer.go
index f108d72..53638fe 100644
--- a/modules/caddytls/acmeissuer.go
+++ b/modules/caddytls/acmeissuer.go
@@ -144,6 +144,10 @@ func (m *ACMEIssuer) SetConfig(cfg *certmagic.Config) {
 	m.magic = cfg
 }
 
+// TODO: I kind of hate how each call to these methods needs to
+// make a new ACME manager to fill in defaults before using; can
+// we find the right place to do that just once and then re-use?
+
 // PreCheck implements the certmagic.PreChecker interface.
 func (m *ACMEIssuer) PreCheck(names []string, interactive bool) error {
 	return certmagic.NewACMEManager(m.magic, m.template).PreCheck(names, interactive)
@@ -156,7 +160,7 @@ func (m *ACMEIssuer) Issue(ctx context.Context, csr *x509.CertificateRequest) (*
 
 // IssuerKey returns the unique issuer key for the configured CA endpoint.
 func (m *ACMEIssuer) IssuerKey() string {
-	return m.template.IssuerKey() // does not need storage and cache
+	return certmagic.NewACMEManager(m.magic, m.template).IssuerKey()
 }
 
 // Revoke revokes the given certificate.
diff --git a/modules/caddytls/connpolicy.go b/modules/caddytls/connpolicy.go
index 5b830f9..7618db4 100644
--- a/modules/caddytls/connpolicy.go
+++ b/modules/caddytls/connpolicy.go
@@ -55,6 +55,11 @@ func (cp ConnectionPolicies) Provision(ctx caddy.Context) error {
 			cp[i].certSelector = val.(certmagic.CertificateSelector)
 		}
 
+		// enable HTTP/2 by default
+		if len(pol.ALPN) == 0 {
+			pol.ALPN = append(pol.ALPN, defaultALPN...)
+		}
+
 		// pre-build standard TLS config so we don't have to at handshake-time
 		err = pol.buildStandardTLSConfig(ctx)
 		if err != nil {
@@ -452,3 +457,5 @@ func (a *PublicKeyAlgorithm) UnmarshalJSON(b []byte) error {
 type ConnectionMatcher interface {
 	Match(*tls.ClientHelloInfo) bool
 }
+
+var defaultALPN = []string{"h2", "http/1.1"}
diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go
index f91229f..0b39c71 100644
--- a/modules/caddytls/tls.go
+++ b/modules/caddytls/tls.go
@@ -181,7 +181,6 @@ func (t *TLS) Validate() error {
 		// ensure that host aren't repeated; since only the first
 		// automation policy is used, repeating a host in the lists
 		// isn't useful and is probably a mistake
-		// TODO: test this
 		hostSet := make(map[string]int)
 		for i, ap := range t.Automation.Policies {
 			for _, h := range ap.Hosts {
@@ -279,8 +278,8 @@ func (t *TLS) HandleHTTPChallenge(w http.ResponseWriter, r *http.Request) bool {
 	if ap.magic.Issuer == nil {
 		return false
 	}
-	if am, ok := ap.magic.Issuer.(*certmagic.ACMEManager); ok {
-		return am.HandleHTTPChallenge(w, r)
+	if am, ok := ap.magic.Issuer.(*ACMEIssuer); ok {
+		return certmagic.NewACMEManager(am.magic, am.template).HandleHTTPChallenge(w, r)
 	}
 	return false
 }
@@ -709,7 +708,7 @@ const automateKey = "automate"
 // (beta 16 changed the storage path for certificates),
 // after which this function can be deleted
 func (t *TLS) moveCertificates() error {
-	log := t.logger.Named("automigrate")
+	logger := t.logger.Named("automigrate")
 
 	baseDir := caddy.AppDataDir()
 
@@ -760,7 +759,7 @@ func (t *TLS) moveCertificates() error {
 		}
 
 		if len(oldAcmeSites) > 0 {
-			log.Warn("certificate storage path has changed; attempting one-time auto-migration",
+			logger.Warn("certificate storage path has changed; attempting one-time auto-migration",
 				zap.String("old_folder", oldAcmeSitesDir),
 				zap.String("new_folder", newBaseDir),
 				zap.String("details", "https://github.com/caddyserver/caddy/issues/2955"))
@@ -775,13 +774,13 @@ func (t *TLS) moveCertificates() error {
 			// move the folder
 			oldPath := filepath.Join(oldAcmeSitesDir, siteInfo.Name())
 			newPath := filepath.Join(newBaseDir, siteInfo.Name())
-			log.Info("moving certificate assets",
+			logger.Info("moving certificate assets",
 				zap.String("ca", oldCA),
 				zap.String("site", siteInfo.Name()),
 				zap.String("destination", newPath))
 			err = os.Rename(oldPath, newPath)
 			if err != nil {
-				log.Error("failed moving site to new path; skipping",
+				logger.Error("failed moving site to new path; skipping",
 					zap.String("old_path", oldPath),
 					zap.String("new_path", newPath),
 					zap.Error(err))
@@ -792,7 +791,7 @@ func (t *TLS) moveCertificates() error {
 			metaFilePath := filepath.Join(newPath, siteInfo.Name()+".json")
 			metaContents, err := ioutil.ReadFile(metaFilePath)
 			if err != nil {
-				log.Error("could not read metadata file",
+				logger.Error("could not read metadata file",
 					zap.String("filename", metaFilePath),
 					zap.Error(err))
 				continue
@@ -806,12 +805,12 @@ func (t *TLS) moveCertificates() error {
 			}
 			newMeta, err := json.MarshalIndent(cr, "", "\t")
 			if err != nil {
-				log.Error("encoding new metadata file", zap.Error(err))
+				logger.Error("encoding new metadata file", zap.Error(err))
 				continue
 			}
 			err = ioutil.WriteFile(metaFilePath, newMeta, 0600)
 			if err != nil {
-				log.Error("writing new metadata file", zap.Error(err))
+				logger.Error("writing new metadata file", zap.Error(err))
 				continue
 			}
 		}