httpcaddyfile: Fixes for prefer_wildcard mode (#6636)

* httpcaddyfile: Fixes for prefer_wildcard mode

The wildcard hosts need to be collected first, then considered after, because there's no guarantee that all non-wildcards will appear after all wildcards when looping. Also we should not add a domain to Skip if it doesn't qualify for TLS anyway.

* Alternate solution by avoiding adding APs altogether if covered by wildcard

Author: francislavoie

caddyconfig/httpcaddyfile/httptype.go

@@ -706,6 +706,16 @@ func (st *ServerType) serversFromPairings(
 			return specificity(iLongestHost) > specificity(jLongestHost)
 		})
 
+		// collect all hosts that have a wildcard in them
+		wildcardHosts := []string{}
+		for _, sblock := range p.serverBlocks {
+			for _, addr := range sblock.parsedKeys {
+				if strings.HasPrefix(addr.Host, "*.") {
+					wildcardHosts = append(wildcardHosts, addr.Host[2:])
+				}
+			}
+		}
+
 		var hasCatchAllTLSConnPolicy, addressQualifiesForTLS bool
 		autoHTTPSWillAddConnPolicy := srv.AutoHTTPS == nil || !srv.AutoHTTPS.Disabled
 
@@ -791,13 +801,6 @@ func (st *ServerType) serversFromPairings(
 				}
 			}
 
-			wildcardHosts := []string{}
-			for _, addr := range sblock.parsedKeys {
-				if strings.HasPrefix(addr.Host, "*.") {
-					wildcardHosts = append(wildcardHosts, addr.Host[2:])
-				}
-			}
-
 			for _, addr := range sblock.parsedKeys {
 				// if server only uses HTTP port, auto-HTTPS will not apply
 				if listenersUseAnyPortOtherThan(srv.Listen, httpPort) {
@@ -813,18 +816,6 @@ func (st *ServerType) serversFromPairings(
 					}
 				}
 
-				// If prefer wildcard is enabled, then we add hosts that are
-				// already covered by the wildcard to the skip list
-				if srv.AutoHTTPS != nil && srv.AutoHTTPS.PreferWildcard && addr.Scheme == "https" {
-					baseDomain := addr.Host
-					if idx := strings.Index(baseDomain, "."); idx != -1 {
-						baseDomain = baseDomain[idx+1:]
-					}
-					if !strings.HasPrefix(addr.Host, "*.") && slices.Contains(wildcardHosts, baseDomain) {
-						srv.AutoHTTPS.Skip = append(srv.AutoHTTPS.Skip, addr.Host)
-					}
-				}
-
 				// If TLS is specified as directive, it will also result in 1 or more connection policy being created
 				// Thus, catch-all address with non-standard port, e.g. :8443, can have TLS enabled without
 				// specifying prefix "https://"
@@ -841,6 +832,19 @@ func (st *ServerType) serversFromPairings(
 					(addr.Scheme != "http" && addr.Port != httpPort && hasTLSEnabled) {
 					addressQualifiesForTLS = true
 				}
+
+				// If prefer wildcard is enabled, then we add hosts that are
+				// already covered by the wildcard to the skip list
+				if addressQualifiesForTLS && srv.AutoHTTPS != nil && srv.AutoHTTPS.PreferWildcard {
+					baseDomain := addr.Host
+					if idx := strings.Index(baseDomain, "."); idx != -1 {
+						baseDomain = baseDomain[idx+1:]
+					}
+					if !strings.HasPrefix(addr.Host, "*.") && slices.Contains(wildcardHosts, baseDomain) {
+						srv.AutoHTTPS.SkipCerts = append(srv.AutoHTTPS.SkipCerts, addr.Host)
+					}
+				}
+
 				// predict whether auto-HTTPS will add the conn policy for us; if so, we
 				// may not need to add one for this server
 				autoHTTPSWillAddConnPolicy = autoHTTPSWillAddConnPolicy &&

caddyconfig/httpcaddyfile/tlsapp.go

@@ -92,6 +92,25 @@ func (st ServerType) buildTLSApp(
 		tlsApp.Automation.Policies = append(tlsApp.Automation.Policies, catchAllAP)
 	}
 
+	// collect all hosts that have a wildcard in them, and arent HTTP
+	wildcardHosts := []string{}
+	for _, p := range pairings {
+		var addresses []string
+		for _, addressWithProtocols := range p.addressesWithProtocols {
+			addresses = append(addresses, addressWithProtocols.address)
+		}
+		if !listenersUseAnyPortOtherThan(addresses, httpPort) {
+			continue
+		}
+		for _, sblock := range p.serverBlocks {
+			for _, addr := range sblock.parsedKeys {
+				if strings.HasPrefix(addr.Host, "*.") {
+					wildcardHosts = append(wildcardHosts, addr.Host[2:])
+				}
+			}
+		}
+	}
+
 	for _, p := range pairings {
 		// avoid setting up TLS automation policies for a server that is HTTP-only
 		var addresses []string
@@ -115,6 +134,12 @@ func (st ServerType) buildTLSApp(
 				return nil, warnings, err
 			}
 
+			// make a plain copy so we can compare whether we made any changes
+			apCopy, err := newBaseAutomationPolicy(options, warnings, true)
+			if err != nil {
+				return nil, warnings, err
+			}
+
 			sblockHosts := sblock.hostsFromKeys(false)
 			if len(sblockHosts) == 0 && catchAllAP != nil {
 				ap = catchAllAP
@@ -217,9 +242,21 @@ func (st ServerType) buildTLSApp(
 				catchAllAP = ap
 			}
 
+			hostsNotHTTP := sblock.hostsFromKeysNotHTTP(httpPort)
+			sort.Strings(hostsNotHTTP) // solely for deterministic test results
+
+			// if the we prefer wildcards and the AP is unchanged,
+			// then we can skip this AP because it should be covered
+			// by an AP with a wildcard
+			if slices.Contains(autoHTTPS, "prefer_wildcard") {
+				if hostsCoveredByWildcard(hostsNotHTTP, wildcardHosts) &&
+					reflect.DeepEqual(ap, apCopy) {
+					continue
+				}
+			}
+
 			// associate our new automation policy with this server block's hosts
-			ap.SubjectsRaw = sblock.hostsFromKeysNotHTTP(httpPort)
-			sort.Strings(ap.SubjectsRaw) // solely for deterministic test results
+			ap.SubjectsRaw = hostsNotHTTP
 
 			// if a combination of public and internal names were given
 			// for this same server block and no issuer was specified, we
@@ -258,6 +295,7 @@ func (st ServerType) buildTLSApp(
 					ap2.IssuersRaw = []json.RawMessage{caddyconfig.JSONModuleObject(caddytls.InternalIssuer{}, "module", "internal", &warnings)}
 				}
 			}
+
 			if tlsApp.Automation == nil {
 				tlsApp.Automation = new(caddytls.AutomationConfig)
 			}
@@ -418,10 +456,7 @@ func (st ServerType) buildTLSApp(
 		}
 
 		// consolidate automation policies that are the exact same
-		tlsApp.Automation.Policies = consolidateAutomationPolicies(
-			tlsApp.Automation.Policies,
-			slices.Contains(autoHTTPS, "prefer_wildcard"),
-		)
+		tlsApp.Automation.Policies = consolidateAutomationPolicies(tlsApp.Automation.Policies)
 
 		// ensure automation policies don't overlap subjects (this should be
 		// an error at provision-time as well, but catch it in the adapt phase
@@ -567,7 +602,7 @@ func newBaseAutomationPolicy(
 
 // consolidateAutomationPolicies combines automation policies that are the same,
 // for a cleaner overall output.
-func consolidateAutomationPolicies(aps []*caddytls.AutomationPolicy, preferWildcard bool) []*caddytls.AutomationPolicy {
+func consolidateAutomationPolicies(aps []*caddytls.AutomationPolicy) []*caddytls.AutomationPolicy {
 	// sort from most specific to least specific; we depend on this ordering
 	sort.SliceStable(aps, func(i, j int) bool {
 		if automationPolicyIsSubset(aps[i], aps[j]) {
@@ -652,31 +687,6 @@ outer:
 					j--
 				}
 			}
-
-			if preferWildcard {
-				// remove subjects from i if they're covered by a wildcard in j
-				iSubjs := aps[i].SubjectsRaw
-				for iSubj := 0; iSubj < len(iSubjs); iSubj++ {
-					for jSubj := range aps[j].SubjectsRaw {
-						if !strings.HasPrefix(aps[j].SubjectsRaw[jSubj], "*.") {
-							continue
-						}
-						if certmagic.MatchWildcard(aps[i].SubjectsRaw[iSubj], aps[j].SubjectsRaw[jSubj]) {
-							iSubjs = slices.Delete(iSubjs, iSubj, iSubj+1)
-							iSubj--
-							break
-						}
-					}
-				}
-				aps[i].SubjectsRaw = iSubjs
-
-				// remove i if it has no subjects left
-				if len(aps[i].SubjectsRaw) == 0 {
-					aps = slices.Delete(aps, i, i+1)
-					i--
-					continue outer
-				}
-			}
 		}
 	}
 
@@ -748,3 +758,20 @@ func automationPolicyHasAllPublicNames(ap *caddytls.AutomationPolicy) bool {
 func isTailscaleDomain(name string) bool {
 	return strings.HasSuffix(strings.ToLower(name), ".ts.net")
 }
+
+func hostsCoveredByWildcard(hosts []string, wildcards []string) bool {
+	if len(hosts) == 0 || len(wildcards) == 0 {
+		return false
+	}
+	for _, host := range hosts {
+		for _, wildcard := range wildcards {
+			if strings.HasPrefix(host, "*.") {
+				continue
+			}
+			if certmagic.MatchWildcard(host, "*."+wildcard) {
+				return true
+			}
+		}
+	}
+	return false
+}

caddytest/integration/caddyfile_adapt/auto_https_prefer_wildcard.caddyfiletest

@@ -74,6 +74,9 @@ foo.example.com {
 						}
 					],
 					"automatic_https": {
+						"skip_certificates": [
+							"foo.example.com"
+						],
 						"prefer_wildcard": true
 					}
 				}