httpcaddyfile: Wrap site block in subroute if host matcher used (#5130)

* httpcaddyfile: Wrap site block in subroute if host matcher used (fix #5124)

* Correct boolean logic (oops)

caddyconfig/httpcaddyfile/httptype.go

@@ -907,11 +907,32 @@ func appendSubrouteToRouteList(routeList caddyhttp.RouteList,
 		return routeList
 	}
 
+	// No need to wrap the handlers in a subroute if this is the only server block
+	// and there is no matcher for it (doing so would produce unnecessarily nested
+	// JSON), *unless* there is a host matcher within this site block; if so, then
+	// we still need to wrap in a subroute because otherwise the host matcher from
+	// the inside of the site block would be a top-level host matcher, which is
+	// subject to auto-HTTPS (cert management), and using a host matcher within
+	// a site block is a valid, common pattern for excluding domains from cert
+	// management, leading to unexpected behavior; see issue #5124.
+	wrapInSubroute := true
 	if len(matcherSetsEnc) == 0 && len(p.serverBlocks) == 1 {
-		// no need to wrap the handlers in a subroute if this is
+		var hasHostMatcher bool
-		// the only server block and there is no matcher for it
+	outer:
-		routeList = append(routeList, subroute.Routes...)
+		for _, route := range subroute.Routes {
-	} else {
+			for _, ms := range route.MatcherSetsRaw {
+				for matcherName := range ms {
+					if matcherName == "host" {
+						hasHostMatcher = true
+						break outer
+					}
+				}
+			}
+		}
+		wrapInSubroute = hasHostMatcher
+	}
+
+	if wrapInSubroute {
 		route := caddyhttp.Route{
 			// the semantics of a site block in the Caddyfile dictate
 			// that only the first matching one is evaluated, since
@@ -929,7 +950,10 @@ func appendSubrouteToRouteList(routeList caddyhttp.RouteList,
 		if len(route.MatcherSetsRaw) > 0 || len(route.HandlersRaw) > 0 {
 			routeList = append(routeList, route)
 		}
+	} else {
+		routeList = append(routeList, subroute.Routes...)
 	}
+
 	return routeList
 }
 

caddytest/integration/caddyfile_adapt/global_server_options_multi.txt

@@ -3,9 +3,7 @@
 		timeouts {
 			idle 90s
 		}
-		protocol {
+		strict_sni_host insecure_off
-			strict_sni_host insecure_off
-		}
 	}
 	servers :80 {
 		timeouts {
@@ -16,9 +14,7 @@
 		timeouts {
 			idle 30s
 		}
-		protocol {
+		strict_sni_host
-			strict_sni_host
-		}
 	}
 }
 

caddytest/integration/caddyfile_adapt/php_fastcgi_matcher.txt

@@ -1,7 +1,12 @@
 :8884
 
-@api host example.com
+# the use of a host matcher here should cause this
-php_fastcgi @api localhost:9000
+# site block to be wrapped in a subroute, even though
+# the site block does not have a hostname; this is
+# to prevent auto-HTTPS from picking up on this host
+# matcher because it is not a key on the site block
+@test host example.com
+php_fastcgi @test localhost:9000
 ----------
 {
 	"apps": {
@@ -13,13 +18,6 @@ php_fastcgi @api localhost:9000
 					],
 					"routes": [
 						{
-							"match": [
-								{
-									"host": [
-										"example.com"
-									]
-								}
-							],
 							"handle": [
 								{
 									"handler": "subroute",
@@ -27,82 +25,99 @@ php_fastcgi @api localhost:9000
 										{
 											"handle": [
 												{
-													"handler": "static_response",
+													"handler": "subroute",
-													"headers": {
+													"routes": [
-														"Location": [
-															"{http.request.orig_uri.path}/"
-														]
-													},
-													"status_code": 308
-												}
-											],
-											"match": [
-												{
-													"file": {
-														"try_files": [
-															"{http.request.uri.path}/index.php"
-														]
-													},
-													"not": [
 														{
-															"path": [
+															"handle": [
-																"*/"
+																{
+																	"handler": "static_response",
+																	"headers": {
+																		"Location": [
+																			"{http.request.orig_uri.path}/"
+																		]
+																	},
+																	"status_code": 308
+																}
+															],
+															"match": [
+																{
+																	"file": {
+																		"try_files": [
+																			"{http.request.uri.path}/index.php"
+																		]
+																	},
+																	"not": [
+																		{
+																			"path": [
+																				"*/"
+																			]
+																		}
+																	]
+																}
+															]
+														},
+														{
+															"handle": [
+																{
+																	"handler": "rewrite",
+																	"uri": "{http.matchers.file.relative}"
+																}
+															],
+															"match": [
+																{
+																	"file": {
+																		"split_path": [
+																			".php"
+																		],
+																		"try_files": [
+																			"{http.request.uri.path}",
+																			"{http.request.uri.path}/index.php",
+																			"index.php"
+																		]
+																	}
+																}
+															]
+														},
+														{
+															"handle": [
+																{
+																	"handler": "reverse_proxy",
+																	"transport": {
+																		"protocol": "fastcgi",
+																		"split_path": [
+																			".php"
+																		]
+																	},
+																	"upstreams": [
+																		{
+																			"dial": "localhost:9000"
+																		}
+																	]
+																}
+															],
+															"match": [
+																{
+																	"path": [
+																		"*.php"
+																	]
+																}
 															]
 														}
 													]
 												}
-											]
-										},
-										{
-											"handle": [
-												{
-													"handler": "rewrite",
-													"uri": "{http.matchers.file.relative}"
-												}
 											],
 											"match": [
 												{
-													"file": {
+													"host": [
-														"split_path": [
+														"example.com"
-															".php"
-														],
-														"try_files": [
-															"{http.request.uri.path}",
-															"{http.request.uri.path}/index.php",
-															"index.php"
-														]
-													}
-												}
-											]
-										},
-										{
-											"handle": [
-												{
-													"handler": "reverse_proxy",
-													"transport": {
-														"protocol": "fastcgi",
-														"split_path": [
-															".php"
-														]
-													},
-													"upstreams": [
-														{
-															"dial": "localhost:9000"
-														}
-													]
-												}
-											],
-											"match": [
-												{
-													"path": [
-														"*.php"
 													]
 												}
 											]
 										}
 									]
 								}
-							]
+							],
+							"terminal": true
 						}
 					]
 				}

modules/caddyhttp/app.go

@@ -66,7 +66,7 @@ func init() {
 // `{http.request.orig_uri}` | The request's original URI
 // `{http.request.port}` | The port part of the request's Host header
 // `{http.request.proto}` | The protocol of the request
-// `{http.request.remote.host}` | The host part of the remote client's address
+// `{http.request.remote.host}` | The host (IP) part of the remote client's address
 // `{http.request.remote.port}` | The port part of the remote client's address
 // `{http.request.remote}` | The address of the remote client
 // `{http.request.scheme}` | The request scheme