Infrastructure/caddy
1
Fork 0
mirror of https://github.com/caddyserver/caddy.git synced 2024-12-16 21:56:40 -05:00
Code Issues Activity

httpcaddyfile: Add nil check to prevent panic, fix validation logic

Browse source 
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
This commit is contained in:
Matthew Holt 2020-04-24 18:58:28 -06:00
parent 100d19e3af
commit 97ed9e111d
No known key found for this signature in database
GPG key ID: 2A349DD577D586A5
3 changed files with 61 additions and 27 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
caddyconfig/httpcaddyfile/httptype.go
View file

@ -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
}

41
caddyconfig/httpcaddyfile/tlsapp.go
View file

@ -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 {

19
modules/caddyhttp/autohttps.go
View file

@ -152,12 +152,12 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
}
// nothing more to do here if there are no domains that qualify for
// automatic HTTPS or there are no explicit TLS connection policies;
// if there is at least one domain but no TLS conn policy, we'll add
// one below; if there is a TLS conn policy (meaning TLS is enabled)
// and no domains, it could be a catch-all with on-demand TLS, and
// in that case we would still need HTTP->HTTPS redirects, which we
// do below
// automatic HTTPS and there are no explicit TLS connection policies:
// if there is at least one domain but no TLS conn policy (F&&T), we'll
// add one below; if there are no domains but at least one TLS conn
// policy (meaning TLS is enabled) (T&&F), it could be a catch-all with
// on-demand TLS -- and in that case we would still need HTTP->HTTPS
// redirects, which we set up below; hence these two conditions
if len(serverDomainSet) == 0 && len(srv.TLSConnPolicies) == 0 {
continue
}
@ -345,6 +345,13 @@ uniqueDomainsLoop:
// not entirely clear what the redirect destination should be,
// so I'm going to just hard-code the app's HTTPS port and call
// it good for now...
// TODO: This implies that all plaintext requests will be blindly
// redirected to their HTTPS equivalent, even if this server
// doesn't handle that hostname at all; I don't think this is a
// bad thing, and it also obscures the actual hostnames that this
// server is configured to match on, which may be desirable, but
// it's not something that should be relied on. We can change this
// if we want to.
appendCatchAll := func(routes []Route) []Route {
redirTo := "https://{http.request.host}"
if app.httpsPort() != DefaultHTTPSPort {