From 3e1fd2a8d4d1463574033fbbdf5c27a693f9a86c Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Wed, 12 Oct 2022 09:27:08 -0600 Subject: [PATCH] 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 | 32 +++- .../global_server_options_multi.txt | 8 +- .../caddyfile_adapt/php_fastcgi_matcher.txt | 157 ++++++++++-------- modules/caddyhttp/app.go | 2 +- 4 files changed, 117 insertions(+), 82 deletions(-) diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go index c220c066..77f990b9 100644 --- a/caddyconfig/httpcaddyfile/httptype.go +++ b/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 - // the only server block and there is no matcher for it - routeList = append(routeList, subroute.Routes...) - } else { + var hasHostMatcher bool + outer: + for _, route := range subroute.Routes { + 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 } diff --git a/caddytest/integration/caddyfile_adapt/global_server_options_multi.txt b/caddytest/integration/caddyfile_adapt/global_server_options_multi.txt index c01173b4..ca5306fd 100644 --- a/caddytest/integration/caddyfile_adapt/global_server_options_multi.txt +++ b/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 } } diff --git a/caddytest/integration/caddyfile_adapt/php_fastcgi_matcher.txt b/caddytest/integration/caddyfile_adapt/php_fastcgi_matcher.txt index d90b2cb4..e5b331e3 100644 --- a/caddytest/integration/caddyfile_adapt/php_fastcgi_matcher.txt +++ b/caddytest/integration/caddyfile_adapt/php_fastcgi_matcher.txt @@ -1,7 +1,12 @@ :8884 -@api host example.com -php_fastcgi @api localhost:9000 +# the use of a host matcher here should cause this +# 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", - "headers": { - "Location": [ - "{http.request.orig_uri.path}/" - ] - }, - "status_code": 308 - } - ], - "match": [ - { - "file": { - "try_files": [ - "{http.request.uri.path}/index.php" - ] - }, - "not": [ + "handler": "subroute", + "routes": [ { - "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": { - "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" + "host": [ + "example.com" ] } ] } ] } - ] + ], + "terminal": true } ] } diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go index 33d96d86..0943b32d 100644 --- a/modules/caddyhttp/app.go +++ b/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