From a03eba6fbc2770ef6bbe7defa184ec4a6817f0c2 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Fri, 16 Feb 2018 12:36:28 -0700 Subject: [PATCH] tls: In HTTP->HTTPS redirects, preserve redir port in some circumstances Only strip the port from the Location URL value if the port is NOT the HTTPSPort (before, we compared against DefaultHTTPSPort instead of HTTPSPort). The HTTPSPort can be changed, but is done so for port forwarding, since in reality you can't 'change' the standard HTTPS port, you can only forward it. --- caddyhttp/httpserver/https.go | 39 ++++++++++++++++++------------ caddyhttp/httpserver/https_test.go | 2 +- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/caddyhttp/httpserver/https.go b/caddyhttp/httpserver/https.go index 3d1de7499..ae3c4e902 100644 --- a/caddyhttp/httpserver/https.go +++ b/caddyhttp/httpserver/https.go @@ -159,29 +159,38 @@ func hostHasOtherPort(allConfigs []*SiteConfig, thisConfigIdx int, otherPort str // be the HTTPS configuration. The returned configuration is set // to listen on HTTPPort. The TLS field of cfg must not be nil. func redirPlaintextHost(cfg *SiteConfig) *SiteConfig { + redirPort := cfg.Addr.Port + if redirPort == HTTPSPort { + // By default, HTTPSPort should be DefaultHTTPSPort, + // which of course doesn't need to be explicitly stated + // in the Location header. Even if HTTPSPort is changed + // so that it is no longer DefaultHTTPSPort, we shouldn't + // append it to the URL in the Location because changing + // the HTTPS port is assumed to be an internal-only change + // (in other words, we assume port forwarding is going on); + // but redirects go back to a presumably-external client. + // (If redirect clients are also internal, that is more + // advanced, and the user should configure HTTP->HTTPS + // redirects themselves.) + redirPort = "" + } + redirMiddleware := func(next Handler) Handler { return HandlerFunc(func(w http.ResponseWriter, r *http.Request) (int, error) { // Construct the URL to which to redirect. Note that the Host in a - // request might contain a port, but we just need the hostname. + // request might contain a port, but we just need the hostname from + // it; and we'll set the port if needed. toURL := "https://" requestHost, _, err := net.SplitHostPort(r.Host) if err != nil { - requestHost = r.Host // host did not contain a port; okay + requestHost = r.Host // Host did not contain a port, so use the whole value + } + if redirPort == "" { + toURL += requestHost + } else { + toURL += net.JoinHostPort(requestHost, redirPort) } - // The rest of the URL will consist of the hostname and the URI. - // We do not append a port because if the HTTPSPort is changed - // from the default value, it is probably because there is port - // forwarding going on; and we do not need to specify the default - // HTTPS port in the redirect. Serving HTTPS on a port other than - // 443 is unusual, and is considered an advanced use case. If port - // forwarding IS happening, then redirecting the external client to - // this internal port will cause the connection to fail; and it - // definitely causes ACME HTTP-01 challenges to fail, because it - // only allows redirecting to port 80 or 443 (as of Feb 2018). - // If a user wants to redirect HTTP to HTTPS on an external port - // other than 443, they can easily configure that themselves. - toURL += requestHost toURL += r.URL.RequestURI() w.Header().Set("Connection", "close") diff --git a/caddyhttp/httpserver/https_test.go b/caddyhttp/httpserver/https_test.go index 3e9fe915a..043249445 100644 --- a/caddyhttp/httpserver/https_test.go +++ b/caddyhttp/httpserver/https_test.go @@ -53,7 +53,7 @@ func TestRedirPlaintextHost(t *testing.T) { }, { Host: "foohost", - Port: "443", // since this is the default HTTPS port, should not be included in Location value + Port: HTTPSPort, // since this is the 'default' HTTPS port, should not be included in Location value }, { Host: "*.example.com",