caddyfile: tls: Ensure there is always a catch-all conn policy (#3005)

If user provides their own certs or makes any hostname-specific TLS
connection policy, it means that no TLS connection would be served for
any other hostnames, even though you'd expect that TLS is enabled for
them, too. So now we append a catch-all conn policy if none exist, which
allows all ClientHellos to be matched and served.

We also fix the consolidation of automation policies, which previously
gobbled up automation policies without hosts in favor of automation
policies with hosts. Instead of a host-specific policy eating up an
identical catch-all policy, the catch-all policy eats up the identical
host-specific policy, ensuring that the policy is applied to all hosts
which need it.

See also:
https://caddy.community/t/v2-automatic-https-certificate-errors/6847/9?u=matt

caddyconfig/httpcaddyfile/httptype.go

@@ -372,6 +372,7 @@ func (st *ServerType) serversFromPairings(
 				srv.AutoHTTPS.Skip = append(srv.AutoHTTPS.Skip, autoHTTPSQualifiedHosts...)
 			} else if cpVals, ok := sblock.pile["tls.connection_policy"]; ok {
 				// tls connection policies
+				var hasCatchAll bool
 				for _, cpVal := range cpVals {
 					cp := cpVal.Value.(*caddytls.ConnectionPolicy)
 
@@ -381,14 +382,31 @@ func (st *ServerType) serversFromPairings(
 						return nil, err
 					}
 
-					// TODO: are matchers needed if every hostname of the config is matched?
+					// TODO: are matchers needed if every hostname of the resulting config is matched?
 					if len(hosts) > 0 {
 						cp.MatchersRaw = caddy.ModuleMap{
 							"sni": caddyconfig.JSON(hosts, warnings), // make sure to match all hosts, not just auto-HTTPS-qualified ones
 						}
+					} else {
+						hasCatchAll = true
 					}
+
 					srv.TLSConnPolicies = append(srv.TLSConnPolicies, cp)
 				}
+
+				// a catch-all is necessary to ensure TLS can be offered to
+				// all hostnames of the server; even though only one policy
+				// is needed to enable TLS for the server, that policy might
+				// apply to only certain TLS handshakes; but when using the
+				// Caddyfile, user would expect all handshakes to at least
+				// have a matching connection policy, so here we append a
+				// catch-all/default policy if there isn't one already (it's
+				// important that it goes at the end) - see issue #3004:
+				// https://github.com/caddyserver/caddy/issues/3004
+				if !hasCatchAll {
+					srv.TLSConnPolicies = append(srv.TLSConnPolicies, new(caddytls.ConnectionPolicy))
+				}
+
 				// TODO: consolidate equal conn policies
 			}
 
@@ -569,14 +587,41 @@ func consolidateAutomationPolicies(aps []caddytls.AutomationPolicy) []caddytls.A
 			if j == i {
 				continue
 			}
-			if reflect.DeepEqual(aps[i].ManagementRaw, aps[j].ManagementRaw) {
+
+			// if they're exactly equal in every way, just keep one of them
+			if reflect.DeepEqual(aps[i], aps[j]) {
+				aps = append(aps[:j], aps[j+1:]...)
+				i--
+				break
+			}
+
+			// if the policy is the same, we can keep just one, but we have
+			// to be careful which one we keep; if only one has any hostnames
+			// defined, then we need to keep the one without any hostnames,
+			// otherwise the one without any hosts (a catch-all) would be
+			// eaten up by the one with hosts; and if both have hosts, we
+			// need to combine their lists
+			if reflect.DeepEqual(aps[i].ManagementRaw, aps[j].ManagementRaw) &&
+				aps[i].ManageSync == aps[j].ManageSync {
+				if len(aps[i].Hosts) == 0 && len(aps[j].Hosts) > 0 {
+					aps = append(aps[:j], aps[j+1:]...)
+				} else if len(aps[i].Hosts) > 0 && len(aps[j].Hosts) == 0 {
+					aps = append(aps[:i], aps[i+1:]...)
+				} else {
 					aps[i].Hosts = append(aps[i].Hosts, aps[j].Hosts...)
 					aps = append(aps[:j], aps[j+1:]...)
+				}
 				i--
 				break
 			}
 		}
 	}
+
+	// ensure any catch-all policies go last
+	sort.SliceStable(aps, func(i, j int) bool {
+		return len(aps[i].Hosts) > len(aps[j].Hosts)
+	})
+
 	return aps
 }
 

go.mod

@@ -17,7 +17,7 @@ require (
 	github.com/klauspost/cpuid v1.2.2
 	github.com/kylelemons/godebug v1.1.0 // indirect
 	github.com/lucas-clemente/quic-go v0.14.1
-	github.com/mholt/certmagic v0.9.2
+	github.com/mholt/certmagic v0.9.3
 	github.com/miekg/dns v1.1.25 // indirect
 	github.com/muhammadmuzzammil1998/jsonc v0.0.0-20190906142622-1265e9b150c6
 	github.com/naoina/go-stringutil v0.1.0 // indirect

go.sum

@@ -225,8 +225,8 @@ github.com/mattn/go-runewidth v0.0.2/go.mod h1:LwmH8dsx7+W8Uxz3IHJYH5QSwggIsqBzp
 github.com/mattn/go-runewidth v0.0.4/go.mod h1:LwmH8dsx7+W8Uxz3IHJYH5QSwggIsqBzpuz5H//U1FU=
 github.com/mattn/go-tty v0.0.0-20180219170247-931426f7535a/go.mod h1:XPvLUNfbS4fJH25nqRHfWLMa1ONC8Amw+mIA639KxkE=
 github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0=
-github.com/mholt/certmagic v0.9.2 h1:m+l8jZADOwL2zBVWhaprTnJaRVmRwG4FzR1A8nHwrLw=
+github.com/mholt/certmagic v0.9.3 h1:RmzuNJ5mpFplDbyS41z+gGgE/py24IX6m0nHZ0yNTQU=
-github.com/mholt/certmagic v0.9.2/go.mod h1:nu8jbsbtwK4205EDH/ZUMTKsfYpJA1Q7MKXHfgTihNw=
+github.com/mholt/certmagic v0.9.3/go.mod h1:nu8jbsbtwK4205EDH/ZUMTKsfYpJA1Q7MKXHfgTihNw=
 github.com/miekg/dns v1.1.15 h1:CSSIDtllwGLMoA6zjdKnaE6Tx6eVUxQ29LUgGetiDCI=
 github.com/miekg/dns v1.1.15/go.mod h1:W1PPwlIAgtquWBMBEV9nkV9Cazfe8ScdGz/Lj7v3Nrg=
 github.com/miekg/dns v1.1.25 h1:dFwPR6SfLtrSwgDcIq2bcU/gVutB4sNApq2HBdqcakg=