Skip to content

Commit

Permalink
httpcaddyfile: Fix Auto-HTTPS edgecase
Browse files Browse the repository at this point in the history
Guh, this is complicated.

Fixes #4640

This also follows up on #4398 (reverting it) which made a change that technically worked, but was incorrect. It changed the condition in `hostsFromKeysNotHTTP` from `&&` to `||`, but then the function no longer did what its name said it would do, and it would return hosts even if they were marked with `http://`, if they used a non-HTTP port. That wasn't the intent of it. The test added in there was kept though, because it is a valid usecase.

The actual fix is to check _earlier_ whether all the addresses explicitly have `http://`, and if so we can short circuit and skip considering the rest.
  • Loading branch information
francislavoie committed Mar 25, 2022
1 parent b8dbecb commit bd50a0d
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 2 deletions.
13 changes: 12 additions & 1 deletion caddyconfig/httpcaddyfile/directives.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ func (sb serverBlock) hostsFromKeysNotHTTP(httpPort string) []string {
if addr.Host == "" {
continue
}
if addr.Scheme != "http" || addr.Port != httpPort {
if addr.Scheme != "http" && addr.Port != httpPort {
hostMap[addr.Host] = struct{}{}
}
}
Expand All @@ -519,6 +519,17 @@ func (sb serverBlock) hasHostCatchAllKey() bool {
return false
}

// isAllHTTP returns true if all sb keys explicitly specify
// the http:// scheme
func (sb serverBlock) isAllHTTP() bool {
for _, addr := range sb.keys {
if addr.Scheme != "http" {
return false
}
}
return true
}

type (
// UnmarshalFunc is a function which can unmarshal Caddyfile
// tokens into zero or more config values using a Helper type.
Expand Down
2 changes: 1 addition & 1 deletion caddyconfig/httpcaddyfile/httptype.go
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ func (st *ServerType) serversFromPairings(
}

for _, addr := range sblock.keys {
// if server only uses HTTPS port, auto-HTTPS will not apply
// if server only uses HTTP port, auto-HTTPS will not apply
if listenersUseAnyPortOtherThan(srv.Listen, httpPort) {
// exclude any hosts that were defined explicitly with "http://"
// in the key from automated cert management (issue #2998)
Expand Down
6 changes: 6 additions & 0 deletions caddyconfig/httpcaddyfile/tlsapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,12 @@ func (st ServerType) buildTLSApp(
}

for _, sblock := range p.serverBlocks {
// check the scheme of all the site addresses,
// skip building AP if they all had http://
if sblock.isAllHTTP() {
continue
}

// get values that populate an automation policy for this block
ap, err := newBaseAutomationPolicy(options, warnings, true)
if err != nil {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# example from issue #4640
http://foo:8447, http://127.0.0.1:8447 {
reverse_proxy 127.0.0.1:8080
}
----------
{
"apps": {
"http": {
"servers": {
"srv0": {
"listen": [
":8447"
],
"routes": [
{
"match": [
{
"host": [
"foo",
"127.0.0.1"
]
}
],
"handle": [
{
"handler": "subroute",
"routes": [
{
"handle": [
{
"handler": "reverse_proxy",
"upstreams": [
{
"dial": "127.0.0.1:8080"
}
]
}
]
}
]
}
],
"terminal": true
}
],
"automatic_https": {
"skip": [
"foo",
"127.0.0.1"
]
}
}
}
}
}
}

0 comments on commit bd50a0d

Please sign in to comment.