From 6bad878a22e048762262d6fabe2144cefaf4ca81 Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Thu, 13 Oct 2022 11:30:57 -0600 Subject: httpcaddyfile: Improve detection of indistinguishable TLS automation policies (#5120) * httpcaddyfile: Skip some logic if auto_https off * Try removing this check altogether... * Refine test timeouts slightly, sigh * caddyhttp: Assume udp for unrecognized network type Seems like the reasonable thing to do if a plugin registers its own network type. * Add comment to document my lack of knowledge * Clean up and prepare to merge Add comments to try to explain what happened --- caddyconfig/httpcaddyfile/tlsapp.go | 88 ++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 44 deletions(-) (limited to 'caddyconfig') diff --git a/caddyconfig/httpcaddyfile/tlsapp.go b/caddyconfig/httpcaddyfile/tlsapp.go index 240cb02..3d32b4f 100644 --- a/caddyconfig/httpcaddyfile/tlsapp.go +++ b/caddyconfig/httpcaddyfile/tlsapp.go @@ -44,41 +44,32 @@ func (st ServerType) buildTLSApp( if hp, ok := options["http_port"].(int); ok { httpPort = strconv.Itoa(hp) } - httpsPort := strconv.Itoa(caddyhttp.DefaultHTTPSPort) - if hsp, ok := options["https_port"].(int); ok { - httpsPort = strconv.Itoa(hsp) - } autoHTTPS := "on" if ah, ok := options["auto_https"].(string); ok { autoHTTPS = ah } - // count how many server blocks have a TLS-enabled key with - // no host, and find all hosts that share a server block with - // a hostless key, so that they don't get forgotten/omitted - // by auto-HTTPS (since they won't appear in route matchers) - var serverBlocksWithTLSHostlessKey int + // find all hosts that share a server block with a hostless + // key, so that they don't get forgotten/omitted by auto-HTTPS + // (since they won't appear in route matchers) httpsHostsSharedWithHostlessKey := make(map[string]struct{}) - for _, pair := range pairings { - for _, sb := range pair.serverBlocks { - for _, addr := range sb.keys { - if addr.Host == "" { - // this address has no hostname, but if it's explicitly set - // to HTTPS, then we need to count it as being TLS-enabled - if addr.Scheme == "https" || addr.Port == httpsPort { - serverBlocksWithTLSHostlessKey++ - } - // this server block has a hostless key, now - // go through and add all the hosts to the set - for _, otherAddr := range sb.keys { - if otherAddr.Original == addr.Original { - continue - } - if otherAddr.Host != "" && otherAddr.Scheme != "http" && otherAddr.Port != httpPort { - httpsHostsSharedWithHostlessKey[otherAddr.Host] = struct{}{} + if autoHTTPS != "off" { + for _, pair := range pairings { + for _, sb := range pair.serverBlocks { + for _, addr := range sb.keys { + if addr.Host == "" { + // this server block has a hostless key, now + // go through and add all the hosts to the set + for _, otherAddr := range sb.keys { + if otherAddr.Original == addr.Original { + continue + } + if otherAddr.Host != "" && otherAddr.Scheme != "http" && otherAddr.Port != httpPort { + httpsHostsSharedWithHostlessKey[otherAddr.Host] = struct{}{} + } } + break } - break } } } @@ -138,6 +129,19 @@ func (st ServerType) buildTLSApp( issuers = append(issuers, issuerVal.Value.(certmagic.Issuer)) } if ap == catchAllAP && !reflect.DeepEqual(ap.Issuers, issuers) { + // this more correctly implements an error check that was removed + // below; try it with this config: + // + // :443 { + // bind 127.0.0.1 + // } + // + // :443 { + // bind ::1 + // tls { + // issuer acme + // } + // } return nil, warnings, fmt.Errorf("automation policy from site block is also default/catch-all policy because of key without hostname, and the two are in conflict: %#v != %#v", ap.Issuers, issuers) } ap.Issuers = issuers @@ -180,29 +184,25 @@ func (st ServerType) buildTLSApp( } } - // first make sure this block is allowed to create an automation policy; - // doing so is forbidden if it has a key with no host (i.e. ":443") + // we used to ensure this block is allowed to create an automation policy; + // doing so was forbidden if it has a key with no host (i.e. ":443") // and if there is a different server block that also has a key with no // host -- since a key with no host matches any host, we need its // associated automation policy to have an empty Subjects list, i.e. no // host filter, which is indistinguishable between the two server blocks // because automation is not done in the context of a particular server... // this is an example of a poor mapping from Caddyfile to JSON but that's - // the least-leaky abstraction I could figure out - if len(sblockHosts) == 0 { - if serverBlocksWithTLSHostlessKey > 1 { - // this server block and at least one other has a key with no host, - // making the two indistinguishable; it is misleading to define such - // a policy within one server block since it actually will apply to - // others as well - return nil, warnings, fmt.Errorf("cannot make a TLS automation policy from a server block that has a host-less address when there are other TLS-enabled server block addresses lacking a host") - } - if catchAllAP == nil { - // this server block has a key with no hosts, but there is not yet - // a catch-all automation policy (probably because no global options - // were set), so this one becomes it - catchAllAP = ap - } + // the least-leaky abstraction I could figure out -- however, this check + // was preventing certain listeners, like those provided by plugins, from + // being used as desired (see the Tailscale listener plugin), so I removed + // the check: and I think since I originally wrote the check I added a new + // check above which *properly* detects this ambiguity without breaking the + // listener plugin; see the check above with a commented example config + if len(sblockHosts) == 0 && catchAllAP == nil { + // this server block has a key with no hosts, but there is not yet + // a catch-all automation policy (probably because no global options + // were set), so this one becomes it + catchAllAP = ap } // associate our new automation policy with this server block's hosts -- cgit v1.2.3