From 8db80c4a88e53c9b710dc7753fd2561fa974ffb2 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Fri, 16 Feb 2018 12:05:34 -0700 Subject: [PATCH] tls: Fix HTTP->HTTPS redirects and HTTP challenge when using custom port --- caddyhttp/httpserver/https.go | 30 +++++++++++++++++------------- caddyhttp/httpserver/server.go | 22 ++-------------------- caddytls/config.go | 13 +++++++------ caddytls/httphandler.go | 15 ++++++++++----- caddytls/httphandler_test.go | 4 ++-- 5 files changed, 38 insertions(+), 46 deletions(-) diff --git a/caddyhttp/httpserver/https.go b/caddyhttp/httpserver/https.go index a12d9982..3d1de749 100644 --- a/caddyhttp/httpserver/https.go +++ b/caddyhttp/httpserver/https.go @@ -159,25 +159,29 @@ 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 == DefaultHTTPSPort { - redirPort = "" // default port is redundant - } - 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; we'll set the port if needed. + // Construct the URL to which to redirect. Note that the Host in a + // request might contain a port, but we just need the hostname. toURL := "https://" requestHost, _, err := net.SplitHostPort(r.Host) if err != nil { - requestHost = r.Host // Host did not contain a port; great - } - if redirPort == "" { - toURL += requestHost - } else { - toURL += net.JoinHostPort(requestHost, redirPort) + requestHost = r.Host // host did not contain a port; okay } + + // 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/server.go b/caddyhttp/httpserver/server.go index 92f2b6fd..9d3ae038 100644 --- a/caddyhttp/httpserver/server.go +++ b/caddyhttp/httpserver/server.go @@ -389,7 +389,7 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) (int, error) if vhost == nil { // check for ACME challenge even if vhost is nil; // could be a new host coming online soon - if caddytls.HTTPChallengeHandler(w, r, "localhost", caddytls.DefaultHTTPAlternatePort) { + if caddytls.HTTPChallengeHandler(w, r, "localhost") { return 0, nil } // otherwise, log the error and write a message to the client @@ -405,7 +405,7 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) (int, error) // we still check for ACME challenge if the vhost exists, // because we must apply its HTTP challenge config settings - if s.proxyHTTPChallenge(vhost, w, r) { + if caddytls.HTTPChallengeHandler(w, r, vhost.ListenHost) { return 0, nil } @@ -422,24 +422,6 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) (int, error) return vhost.middlewareChain.ServeHTTP(w, r) } -// proxyHTTPChallenge solves the ACME HTTP challenge if r is the HTTP -// request for the challenge. If it is, and if the request has been -// fulfilled (response written), true is returned; false otherwise. -// If you don't have a vhost, just call the challenge handler directly. -func (s *Server) proxyHTTPChallenge(vhost *SiteConfig, w http.ResponseWriter, r *http.Request) bool { - if vhost.Addr.Port != caddytls.HTTPChallengePort { - return false - } - if vhost.TLS != nil && vhost.TLS.Manual { - return false - } - altPort := caddytls.DefaultHTTPAlternatePort - if vhost.TLS != nil && vhost.TLS.AltHTTPPort != "" { - altPort = vhost.TLS.AltHTTPPort - } - return caddytls.HTTPChallengeHandler(w, r, vhost.ListenHost, altPort) -} - // Address returns the address s was assigned to listen on. func (s *Server) Address() string { return s.Server.Addr diff --git a/caddytls/config.go b/caddytls/config.go index 0b64f357..938cb08c 100644 --- a/caddytls/config.go +++ b/caddytls/config.go @@ -93,16 +93,17 @@ type Config struct { // an ACME challenge ListenHost string - // The alternate port (ONLY port, not host) - // to use for the ACME HTTP challenge; this - // port will be used if we proxy challenges - // coming in on port 80 to this alternate port + // The alternate port (ONLY port, not host) to + // use for the ACME HTTP challenge; if non-empty, + // this port will be used instead of + // HTTPChallengePort to spin up a listener for + // the HTTP challenge AltHTTPPort string // The alternate port (ONLY port, not host) // to use for the ACME TLS-SNI challenge. - // The system must forward the standard port - // for the TLS-SNI challenge to this port. + // The system must forward TLSSNIChallengePort + // to this port for challenge to succeed AltTLSSNIPort string // The string identifier of the DNS provider diff --git a/caddytls/httphandler.go b/caddytls/httphandler.go index ca356cd4..663e2eb0 100644 --- a/caddytls/httphandler.go +++ b/caddytls/httphandler.go @@ -27,10 +27,11 @@ import ( const challengeBasePath = "/.well-known/acme-challenge" // HTTPChallengeHandler proxies challenge requests to ACME client if the -// request path starts with challengeBasePath. It returns true if it -// handled the request and no more needs to be done; it returns false -// if this call was a no-op and the request still needs handling. -func HTTPChallengeHandler(w http.ResponseWriter, r *http.Request, listenHost, altPort string) bool { +// request path starts with challengeBasePath, if the HTTP challenge is not +// disabled, and if we are known to be obtaining a certificate for the name. +// It returns true if it handled the request and no more needs to be done; +// it returns false if this call was a no-op and the request still needs handling. +func HTTPChallengeHandler(w http.ResponseWriter, r *http.Request, listenHost string) bool { if !strings.HasPrefix(r.URL.Path, challengeBasePath) { return false } @@ -50,7 +51,11 @@ func HTTPChallengeHandler(w http.ResponseWriter, r *http.Request, listenHost, al listenHost = "localhost" } - upstream, err := url.Parse(fmt.Sprintf("%s://%s:%s", scheme, listenHost, altPort)) + // always proxy to the DefaultHTTPAlternatePort because obviously the + // ACME challenge request already got into one of our HTTP handlers, so + // it means we must have started a HTTP listener on the alternate + // port instead; which is only accessible via listenHost + upstream, err := url.Parse(fmt.Sprintf("%s://%s:%s", scheme, listenHost, DefaultHTTPAlternatePort)) if err != nil { w.WriteHeader(http.StatusInternalServerError) log.Printf("[ERROR] ACME proxy handler: %v", err) diff --git a/caddytls/httphandler_test.go b/caddytls/httphandler_test.go index 451c0cf4..cae65ac8 100644 --- a/caddytls/httphandler_test.go +++ b/caddytls/httphandler_test.go @@ -39,7 +39,7 @@ func TestHTTPChallengeHandlerNoOp(t *testing.T) { t.Fatalf("Could not craft request, got error: %v", err) } rw := httptest.NewRecorder() - if HTTPChallengeHandler(rw, req, "", DefaultHTTPAlternatePort) { + if HTTPChallengeHandler(rw, req, "") { t.Errorf("Got true with this URL, but shouldn't have: %s", url) } } @@ -76,7 +76,7 @@ func TestHTTPChallengeHandlerSuccess(t *testing.T) { } rw := httptest.NewRecorder() - HTTPChallengeHandler(rw, req, "", DefaultHTTPAlternatePort) + HTTPChallengeHandler(rw, req, "") if !proxySuccess { t.Fatal("Expected request to be proxied, but it wasn't")