tls: Fix HTTP->HTTPS redirects and HTTP challenge when using custom port

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")

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

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

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)

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")