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 --- modules/caddyhttp/autohttps.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) (limited to 'modules') diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go index ad0a716..f62543b 100644 --- a/modules/caddyhttp/autohttps.go +++ b/modules/caddyhttp/autohttps.go @@ -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 { -- cgit v1.2.3