From 97ed9e111d04718583c8e0cd141a464c993e224a Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Fri, 24 Apr 2020 18:58:28 -0600 Subject: httpcaddyfile: Add nil check to prevent panic, fix validation logic Panic would happen if an automation policy was specified in a singular server block that had no hostnames in its address. Definitely an edge case. Fixed a bug related to checking for server blocks with a host-less key that tried to make an automation policy. Previously if you had only two server blocks like ":443" and another one at ":80", the one at ":443" could not create a TLS automation policy because it thought it would interfere with TLS automation for the block at ":80", but obviously that key doesn't enable TLS because it is on the HTTP port. So now we are a little smarter and count only non-HTTP-empty-hostname keys. Also fixed a bug so that a key like "https://:1234" is sure to have TLS enabled by giving it a TLS connection policy. (Relaxed conditions slightly; the previous conditions were too strict, requiring there to be a TLS conn policy already or a default SNI to be non-empty.) Also clarified a comment thanks to feedback from @Mohammed90 --- caddyconfig/httpcaddyfile/httptype.go | 28 +++++++++++++++++------- caddyconfig/httpcaddyfile/tlsapp.go | 41 ++++++++++++++++++++++++----------- 2 files changed, 48 insertions(+), 21 deletions(-) (limited to 'caddyconfig/httpcaddyfile') diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go index a9ee838..a22dd40 100644 --- a/caddyconfig/httpcaddyfile/httptype.go +++ b/caddyconfig/httpcaddyfile/httptype.go @@ -324,6 +324,10 @@ func (st *ServerType) serversFromPairings( 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) + } for i, p := range pairings { srv := &caddyhttp.Server{ @@ -362,7 +366,8 @@ func (st *ServerType) serversFromPairings( return specificity(iLongestHost) > specificity(jLongestHost) }) - var hasCatchAllTLSConnPolicy, usesTLS bool + var hasCatchAllTLSConnPolicy, addressQualifiesForTLS bool + autoHTTPSWillAddConnPolicy := true // create a subroute for each site in the server block for _, sblock := range p.serverBlocks { @@ -401,9 +406,9 @@ func (st *ServerType) serversFromPairings( } } - // exclude any hosts that were defined explicitly with - // "http://" in the key from automated cert management (issue #2998) for _, addr := range sblock.keys { + // exclude any hosts that were defined explicitly with "http://" + // in the key from automated cert management (issue #2998) if addr.Scheme == "http" && addr.Host != "" { if srv.AutoHTTPS == nil { srv.AutoHTTPS = new(caddyhttp.AutoHTTPSConfig) @@ -412,9 +417,16 @@ func (st *ServerType) serversFromPairings( srv.AutoHTTPS.Skip = append(srv.AutoHTTPS.Skip, addr.Host) } } - if addr.Scheme != "http" && addr.Host != "" && addr.Port != httpPort { - usesTLS = true + // we'll need to remember if the address qualifies for auto-HTTPS, so we + // can add a TLS conn policy if necessary + if addr.Scheme == "https" || + (addr.Scheme != "http" && addr.Host != "" && addr.Port != httpPort) { + addressQualifiesForTLS = true } + // predict whether auto-HTTPS will add the conn policy for us; if so, we + // may not need to add one for this server + autoHTTPSWillAddConnPolicy = autoHTTPSWillAddConnPolicy && + (addr.Port == httpsPort || (addr.Port != httpPort && addr.Host != "")) } // set up each handler directive, making sure to honor directive order @@ -477,9 +489,9 @@ func (st *ServerType) serversFromPairings( // TODO: maybe a smarter way to handle this might be to just make the // auto-HTTPS logic at provision-time detect if there is any connection // policy missing for any HTTPS-enabled hosts, if so, add it... maybe? - if usesTLS && + if addressQualifiesForTLS && !hasCatchAllTLSConnPolicy && - (len(srv.TLSConnPolicies) > 0 || defaultSNI != "") { + (len(srv.TLSConnPolicies) > 0 || !autoHTTPSWillAddConnPolicy || defaultSNI != "") { srv.TLSConnPolicies = append(srv.TLSConnPolicies, &caddytls.ConnectionPolicy{DefaultSNI: defaultSNI}) } @@ -539,7 +551,7 @@ func detectConflictingSchemes(srv *caddyhttp.Server, serverBlocks []serverBlock, if err := checkAndSetHTTP(addr); err != nil { return err } - } else if addr.Scheme == "https" || addr.Port == httpsPort { + } else if addr.Scheme == "https" || addr.Port == httpsPort || len(srv.TLSConnPolicies) > 0 { if err := checkAndSetHTTPS(addr); err != nil { return err } diff --git a/caddyconfig/httpcaddyfile/tlsapp.go b/caddyconfig/httpcaddyfile/tlsapp.go index 5c62233..f66fcae 100644 --- a/caddyconfig/httpcaddyfile/tlsapp.go +++ b/caddyconfig/httpcaddyfile/tlsapp.go @@ -20,9 +20,11 @@ import ( "fmt" "reflect" "sort" + "strconv" "github.com/caddyserver/caddy/v2" "github.com/caddyserver/caddy/v2/caddyconfig" + "github.com/caddyserver/caddy/v2/modules/caddyhttp" "github.com/caddyserver/caddy/v2/modules/caddytls" "github.com/caddyserver/certmagic" ) @@ -36,17 +38,26 @@ func (st ServerType) buildTLSApp( tlsApp := &caddytls.TLS{CertificatesRaw: make(caddy.ModuleMap)} var certLoaders []caddytls.CertificateLoader - // count how many server blocks have a 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 + httpsPort := strconv.Itoa(caddyhttp.DefaultHTTPSPort) + if hsp, ok := options["https_port"].(int); ok { + httpsPort = strconv.Itoa(hsp) + } + + // 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 serverBlocksWithHostlessKey int + var serverBlocksWithTLSHostlessKey int hostsSharedWithHostlessKey := make(map[string]struct{}) for _, pair := range pairings { for _, sb := range pair.serverBlocks { for _, addr := range sb.keys { if addr.Host == "" { - serverBlocksWithHostlessKey++ + // 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 { @@ -149,9 +160,11 @@ func (st ServerType) buildTLSApp( } if ap != nil { - // encode issuer now that it's all set up - issuerName := ap.Issuer.(caddy.Module).CaddyModule().ID.Name() - ap.IssuerRaw = caddyconfig.JSONModuleObject(ap.Issuer, "module", issuerName, &warnings) + if ap.Issuer != nil { + // encode issuer now that it's all set up + issuerName := ap.Issuer.(caddy.Module).CaddyModule().ID.Name() + ap.IssuerRaw = caddyconfig.JSONModuleObject(ap.Issuer, "module", issuerName, &warnings) + } // 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") @@ -163,12 +176,12 @@ func (st ServerType) buildTLSApp( // 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 serverBlocksWithHostlessKey > 1 { + 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 server block addresses lacking a host") + 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 @@ -293,9 +306,11 @@ func (st ServerType) buildTLSApp( // if there is a global/catch-all automation policy, ensure it goes last if catchAllAP != nil { - // first, encode its issuer - issuerName := catchAllAP.Issuer.(caddy.Module).CaddyModule().ID.Name() - catchAllAP.IssuerRaw = caddyconfig.JSONModuleObject(catchAllAP.Issuer, "module", issuerName, &warnings) + // first, encode its issuer, if there is one + if catchAllAP.Issuer != nil { + issuerName := catchAllAP.Issuer.(caddy.Module).CaddyModule().ID.Name() + catchAllAP.IssuerRaw = caddyconfig.JSONModuleObject(catchAllAP.Issuer, "module", issuerName, &warnings) + } // then append it to the end of the policies list if tlsApp.Automation == nil { -- cgit v1.2.3