Skip to content

Commit

Permalink
httpcaddyfile: Wrap site block in subroute if host matcher used (#5130)
Browse files Browse the repository at this point in the history 
* httpcaddyfile: Wrap site block in subroute if host matcher used (fix #5124)

* Correct boolean logic (oops)
  • Loading branch information
@mholt
mholt committed Oct 12, 2022
1 parent 33f60da commit 3e1fd2a
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 81 deletions.
32 changes: 28 additions & 4 deletions caddyconfig/httpcaddyfile/httptype.go
View file
Expand Up @@ -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
Expand All @@ -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
}

Expand Down
8 changes: 2 additions & 6 deletions caddytest/integration/caddyfile_adapt/global_server_options_multi.txt
View file
Expand Up @@ -3,9 +3,7 @@
timeouts {
idle 90s
}
protocol {
strict_sni_host insecure_off
}
strict_sni_host insecure_off
}
servers :80 {
timeouts {
Expand All @@ -16,9 +14,7 @@
timeouts {
idle 30s
}
protocol {
strict_sni_host
}
strict_sni_host
}
}

Expand Down
155 changes: 85 additions & 70 deletions caddytest/integration/caddyfile_adapt/php_fastcgi_matcher.txt
View file
@@ -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": {
Expand All @@ -13,96 +18,106 @@ php_fastcgi @api localhost:9000
],
"routes": [
{
"match": [
{
"host": [
"example.com"
]
}
],
"handle": [
{
"handler": "subroute",
"routes": [
{
"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"
"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"
]
}
]
}
]
}
],
"match": [
{
"path": [
"*.php"
"host": [
"example.com"
]
}
]
}
]
}
]
],
"terminal": true
}
]
}
Expand Down
2 changes: 1 addition & 1 deletion modules/caddyhttp/app.go
View file
Expand Up @@ -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
Expand Down

0 comments on commit 3e1fd2a

Please sign in to comment.