net/http/httputil: add ReverseProxy.Rewrite

Add a new Rewrite hook to ReverseProxy, superseding the Director hook.

Director does not distinguish between the inbound and outbound request,
which makes it possible for headers added by Director to be inadvertently
removed before forwarding if they are listed in the inbound request's
Connection header. Rewrite accepts a value containing the inbound
and outbound requests, with hop-by-hop headers already removed from
the outbound request, avoiding this problem.

ReverseProxy's appends the client IP to the inbound X-Forwarded-For
header by default. Users must manually delete untrusted X-Forwarded-For
values. When used with a Rewrite hook, ReverseProxy now strips
X-Forwarded-* headers by default.

NewSingleHostReverseProxy creates a proxy that does not rewrite the
Host header of inbound requests. Changing this behavior is
cumbersome, as it requires wrapping the Director function created
by NewSingleHostReverseProxy. The Rewrite hook's ProxyRequest
parameter provides a SetURL method that provides equivalent
functionality to NewSingleHostReverseProxy, rewrites the Host
header by default, and can be more easily extended with additional
customizations.

Fixes #28168.
Fixes #50580.
Fixes #53002.

Change-Id: Ib84e2fdd1d52c610e3887af66f517d4a74e594d0
Reviewed-on: https://go-review.googlesource.com/c/go/+/407214
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>

4 files changed, 304 insertions, 66 deletions

diff --git a/api/next/53002.txt b/api/next/53002.txt
new file mode 100644
index 0000000000..b078fee55c
--- /dev/null
+++ b/api/next/53002.txt
@@ -0,0 +1,6 @@
+pkg net/http/httputil, method (*ProxyRequest) SetURL(*url.URL) #53002
+pkg net/http/httputil, method (*ProxyRequest) SetXForwarded() #53002
+pkg net/http/httputil, type ProxyRequest struct #53002
+pkg net/http/httputil, type ProxyRequest struct, In *http.Request #53002
+pkg net/http/httputil, type ProxyRequest struct, Out *http.Request #53002
+pkg net/http/httputil, type ReverseProxy struct, Rewrite func(*ProxyRequest) #53002
diff --git a/src/net/http/httputil/example_test.go b/src/net/http/httputil/example_test.go
index b77a243ca3..6c107f8390 100644
--- a/src/net/http/httputil/example_test.go
+++ b/src/net/http/httputil/example_test.go
@@ -103,7 +103,12 @@ func ExampleReverseProxy() {
 	if err != nil {
 		log.Fatal(err)
 	}
-	frontendProxy := httptest.NewServer(httputil.NewSingleHostReverseProxy(rpURL))
+	frontendProxy := httptest.NewServer(&httputil.ReverseProxy{
+		Rewrite: func(r *httputil.ProxyRequest) {
+			r.SetXForwarded()
+			r.SetURL(rpURL)
+		},
+	})
 	defer frontendProxy.Close()
 
 	resp, err := http.Get(frontendProxy.URL)
diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 0c52497177..11711e6f97 100644
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -8,6 +8,7 @@ package httputil
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"log"
@@ -24,33 +25,118 @@ import (
 	"golang.org/x/net/http/httpguts"
 )
 
-// ReverseProxy is an HTTP Handler that takes an incoming request and
-// sends it to another server, proxying the response back to the
-// client.
+// A ProxyRequest contains a request to be rewritten by a ReverseProxy.
+type ProxyRequest struct {
+	// In is the request received by the proxy.
+	// The Rewrite function must not modify In.
+	In *http.Request
+
+	// Out is the request which will be sent by the proxy.
+	// The Rewrite function may modify or replace this request.
+	// Hop-by-hop headers are removed from this request
+	// before Rewrite is called.
+	Out *http.Request
+}
+
+// SetURL routes the outbound request to the scheme, host, and base path
+// provided in target. If the target's path is "/base" and the incoming
+// request was for "/dir", the target request will be for "/base/dir".
 //
-// ReverseProxy by default sets
-//   - the X-Forwarded-For header to the client IP address;
-//   - the X-Forwarded-Host header to the host of the original client
-//     request; and
-//   - the X-Forwarded-Proto header to "https" if the client request
-//     was made on a TLS-enabled connection or "http" otherwise.
+// SetURL rewrites the outbound Host header to match the target's host.
+// To preserve the inbound request's Host header (the default behavior
+// of NewSingleHostReverseProxy):
 //
-// If an X-Forwarded-For header already exists, the client IP is
-// appended to the existing values.
+//	rewriteFunc := func(r *httputil.ProxyRequest) {
+//		r.SetURL(url)
+//		r.Out.Host = r.In.Host
+//	}
+func (r *ProxyRequest) SetURL(target *url.URL) {
+	rewriteRequestURL(r.Out, target)
+	r.Out.Host = ""
+}
+
+// SetXForwarded sets the X-Forwarded-For, X-Forwarded-Host, and
+// X-Forwarded-Proto headers of the outbound request.
 //
-// If a header exists in the Request.Header map but has a nil value
-// (such as when set by the Director func), it is not modified.
+//   - The X-Forwarded-For header is set to the client IP address.
+//   - The X-Forwarded-Host header is set to the host name requested
+//     by the client.
+//   - The X-Forwarded-Proto header is set to "http" or "https", depending
+//     on whether the inbound request was made on a TLS-enabled connection.
 //
-// To prevent IP spoofing, be sure to delete any pre-existing
-// X-Forwarded-For header coming from the client or
-// an untrusted proxy.
+// If the outbound request contains an existing X-Forwarded-For header,
+// SetXForwarded appends the client IP address to it. To append to the
+// inbound request's X-Forwarded-For header (the default behavior of
+// ReverseProxy when using a Director function), copy the header
+// from the inbound request before calling SetXForwarded:
+//
+//	rewriteFunc := func(r *httputil.ProxyRequest) {
+//		r.Out.Header["X-Forwarded-For"] = r.In.Header["X-Forwarded-For"]
+//		r.SetXForwarded()
+//	}
+func (r *ProxyRequest) SetXForwarded() {
+	clientIP, _, err := net.SplitHostPort(r.In.RemoteAddr)
+	if err == nil {
+		prior := r.Out.Header["X-Forwarded-For"]
+		if len(prior) > 0 {
+			clientIP = strings.Join(prior, ", ") + ", " + clientIP
+		}
+		r.Out.Header.Set("X-Forwarded-For", clientIP)
+	} else {
+		r.Out.Header.Del("X-Forwarded-For")
+	}
+	r.Out.Header.Set("X-Forwarded-Host", r.In.Host)
+	if r.In.TLS == nil {
+		r.Out.Header.Set("X-Forwarded-Proto", "http")
+	} else {
+		r.Out.Header.Set("X-Forwarded-Proto", "https")
+	}
+}
+
+// ReverseProxy is an HTTP Handler that takes an incoming request and
+// sends it to another server, proxying the response back to the
+// client.
 type ReverseProxy struct {
-	// Director must be a function which modifies
+	// Rewrite must be a function which modifies
+	// the request into a new request to be sent
+	// using Transport. Its response is then copied
+	// back to the original client unmodified.
+	// Rewrite must not access the provided ProxyRequest
+	// or its contents after returning.
+	//
+	// The Forwarded, X-Forwarded, X-Forwarded-Host,
+	// and X-Forwarded-Proto headers are removed from the
+	// outbound request before Rewrite is called. See also
+	// the ProxyRequest.SetXForwarded method.
+	//
+	// At most one of Rewrite or Director may be set.
+	Rewrite func(*ProxyRequest)
+
+	// Director is a function which modifies the
 	// the request into a new request to be sent
 	// using Transport. Its response is then copied
 	// back to the original client unmodified.
 	// Director must not access the provided Request
 	// after returning.
+	//
+	// By default, the X-Forwarded-For, X-Forwarded-Host, and
+	// X-Forwarded-Proto headers of the ourgoing request are
+	// set as by the ProxyRequest.SetXForwarded function.
+	//
+	// If an X-Forwarded-For header already exists, the client IP is
+	// appended to the existing values. To prevent IP spoofing, be
+	// sure to delete any pre-existing X-Forwarded-For header
+	// coming from the client or an untrusted proxy.
+	//
+	// If a header exists in the Request.Header map but has a nil value
+	// (such as when set by the Director func), it is not modified.
+	//
+	// Hop-by-hop headers are removed from the request after
+	// Director returns, which can remove headers added by
+	// Director. Use a Rewrite function instead to ensure
+	// modifications to the request are preserved.
+	//
+	// At most one of Rewrite or Director may be set.
 	Director func(*http.Request)
 
 	// The transport used to perform proxy requests.
@@ -142,24 +228,41 @@ func joinURLPath(a, b *url.URL) (path, rawpath string) {
 // URLs to the scheme, host, and base path provided in target. If the
 // target's path is "/base" and the incoming request was for "/dir",
 // the target request will be for /base/dir.
+//
 // NewSingleHostReverseProxy does not rewrite the Host header.
-// To rewrite Host headers, use ReverseProxy directly with a custom
-// Director policy.
+//
+// To customize the ReverseProxy behavior beyond what
+// NewSingleHostReverseProxy provides, use ReverseProxy directly
+// with a Rewrite function. The ProxyRequest SetURL method
+// may be used to route the outbound request. (Note that SetURL,
+// unlike NewSingleHostReverseProxy, rewrites the Host header
+// of the outbound request by default.)
+//
+//	proxy := &ReverseProxy{
+//		Rewrite: func(r *ProxyRequest) {
+//			r.SetURL(target)
+//			r.Out.Host = r.In.Host // if desired
+//		}
+//	}
 func NewSingleHostReverseProxy(target *url.URL) *ReverseProxy {
-	targetQuery := target.RawQuery
 	director := func(req *http.Request) {
-		req.URL.Scheme = target.Scheme
-		req.URL.Host = target.Host
-		req.URL.Path, req.URL.RawPath = joinURLPath(target, req.URL)
-		if targetQuery == "" || req.URL.RawQuery == "" {
-			req.URL.RawQuery = targetQuery + req.URL.RawQuery
-		} else {
-			req.URL.RawQuery = targetQuery + "&" + req.URL.RawQuery
-		}
+		rewriteRequestURL(req, target)
 	}
 	return &ReverseProxy{Director: director}
 }
 
+func rewriteRequestURL(req *http.Request, target *url.URL) {
+	targetQuery := target.RawQuery
+	req.URL.Scheme = target.Scheme
+	req.URL.Host = target.Host
+	req.URL.Path, req.URL.RawPath = joinURLPath(target, req.URL)
+	if targetQuery == "" || req.URL.RawQuery == "" {
+		req.URL.RawQuery = targetQuery + req.URL.RawQuery
+	} else {
+		req.URL.RawQuery = targetQuery + "&" + req.URL.RawQuery
+	}
+}
+
 func copyHeader(dst, src http.Header) {
 	for k, vv := range src {
 		for _, v := range vv {
@@ -260,7 +363,14 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		outreq.Header = make(http.Header) // Issue 33142: historical behavior was to always allocate
 	}
 
-	p.Director(outreq)
+	if (p.Director != nil) == (p.Rewrite != nil) {
+		p.getErrorHandler()(rw, req, errors.New("ReverseProxy must have exactly one of Director or Rewrite set"))
+		return
+	}
+
+	if p.Director != nil {
+		p.Director(outreq)
+	}
 	outreq.Close = false
 
 	reqUpType := upgradeType(outreq.Header)
@@ -268,20 +378,13 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		p.getErrorHandler()(rw, req, fmt.Errorf("client tried to switch to invalid protocol %q", reqUpType))
 		return
 	}
-	removeConnectionHeaders(outreq.Header)
-
-	// Remove hop-by-hop headers to the backend. Especially
-	// important is "Connection" because we want a persistent
-	// connection, regardless of what the client sent to us.
-	for _, h := range hopHeaders {
-		outreq.Header.Del(h)
-	}
+	removeHopByHopHeaders(outreq.Header)
 
 	// Issue 21096: tell backend applications that care about trailer support
 	// that we support trailers. (We do, but we don't go out of our way to
 	// advertise that unless the incoming client request thought it was worth
 	// mentioning.) Note that we look at req.Header, not outreq.Header, since
-	// the latter has passed through removeConnectionHeaders.
+	// the latter has passed through removeHopByHopHeaders.
 	if httpguts.HeaderValuesContainsToken(req.Header["Te"], "trailers") {
 		outreq.Header.Set("Te", "trailers")
 	}
@@ -293,27 +396,44 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		outreq.Header.Set("Upgrade", reqUpType)
 	}
 
-	if clientIP, _, err := net.SplitHostPort(req.RemoteAddr); err == nil {
-		// If we aren't the first proxy retain prior
-		// X-Forwarded-For information as a comma+space
-		// separated list and fold multiple headers into one.
-		prior, ok := outreq.Header["X-Forwarded-For"]
-		omit := ok && prior == nil // Issue 38079: nil now means don't populate the header
-		if len(prior) > 0 {
-			clientIP = strings.Join(prior, ", ") + ", " + clientIP
+	if p.Rewrite != nil {
+		// Strip client-provided forwarding headers.
+		// The Rewrite func may use SetXForwarded to set new values
+		// for these or copy the previous values from the inbound request.
+		outreq.Header.Del("Forwarded")
+		outreq.Header.Del("X-Forwarded-For")
+		outreq.Header.Del("X-Forwarded-Host")
+		outreq.Header.Del("X-Forwarded-Proto")
+
+		pr := &ProxyRequest{
+			In:  req,
+			Out: outreq,
 		}
-		if !omit {
-			outreq.Header.Set("X-Forwarded-For", clientIP)
+		p.Rewrite(pr)
+		outreq = pr.Out
+	} else {
+		if clientIP, _, err := net.SplitHostPort(req.RemoteAddr); err == nil {
+			// If we aren't the first proxy retain prior
+			// X-Forwarded-For information as a comma+space
+			// separated list and fold multiple headers into one.
+			prior, ok := outreq.Header["X-Forwarded-For"]
+			omit := ok && prior == nil // Issue 38079: nil now means don't populate the header
+			if len(prior) > 0 {
+				clientIP = strings.Join(prior, ", ") + ", " + clientIP
+			}
+			if !omit {
+				outreq.Header.Set("X-Forwarded-For", clientIP)
+			}
 		}
-	}
-	if prior, ok := outreq.Header["X-Forwarded-Host"]; !(ok && prior == nil) {
-		outreq.Header.Set("X-Forwarded-Host", req.Host)
-	}
-	if prior, ok := outreq.Header["X-Forwarded-Proto"]; !(ok && prior == nil) {
-		if req.TLS == nil {
-			outreq.Header.Set("X-Forwarded-Proto", "http")
-		} else {
-			outreq.Header.Set("X-Forwarded-Proto", "https")
+		if prior, ok := outreq.Header["X-Forwarded-Host"]; !(ok && prior == nil) {
+			outreq.Header.Set("X-Forwarded-Host", req.Host)
+		}
+		if prior, ok := outreq.Header["X-Forwarded-Proto"]; !(ok && prior == nil) {
+			if req.TLS == nil {
+				outreq.Header.Set("X-Forwarded-Proto", "http")
+			} else {
+				outreq.Header.Set("X-Forwarded-Proto", "https")
+			}
 		}
 	}
 
@@ -323,6 +443,12 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		outreq.Header.Set("User-Agent", "")
 	}
 
+	if _, ok := outreq.Header["User-Agent"]; !ok {
+		// If the outbound request doesn't have a User-Agent header set,
+		// don't send the default Go HTTP client User-Agent.
+		outreq.Header.Set("User-Agent", "")
+	}
+
 	res, err := transport.RoundTrip(outreq)
 	if err != nil {
 		p.getErrorHandler()(rw, outreq, err)
@@ -338,11 +464,7 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	removeConnectionHeaders(res.Header)
-
-	for _, h := range hopHeaders {
-		res.Header.Del(h)
-	}
+	removeHopByHopHeaders(res.Header)
 
 	if !p.modifyResponse(rw, res, outreq) {
 		return
@@ -421,9 +543,9 @@ func shouldPanicOnCopyError(req *http.Request) bool {
 	return false
 }
 
-// removeConnectionHeaders removes hop-by-hop headers listed in the "Connection" header of h.
-// See RFC 7230, section 6.1
-func removeConnectionHeaders(h http.Header) {
+// removeHopByHopHeaders removes hop-by-hop headers.
+func removeHopByHopHeaders(h http.Header) {
+	// RFC 7230, section 6.1: Remove headers listed in the "Connection" header.
 	for _, f := range h["Connection"] {
 		for _, sf := range strings.Split(f, ",") {
 			if sf = textproto.TrimString(sf); sf != "" {
@@ -431,6 +553,12 @@ func removeConnectionHeaders(h http.Header) {
 			}
 		}
 	}
+	// RFC 2616, section 13.5.1: Remove a set of known hop-by-hop headers.
+	// This behavior is superseded by the RFC 7230 Connection header, but
+	// preserve it for backwards compatibility.
+	for _, f := range hopHeaders {
+		h.Del(f)
+	}
 }
 
 // flushInterval returns the p.FlushInterval value, conditionally
diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index 3090e37582..f8157e9435 100644
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -409,6 +409,46 @@ func TestXForwardedFor_Omit(t *testing.T) {
 	res.Body.Close()
 }
 
+func TestReverseProxyRewriteStripsForwarded(t *testing.T) {
+	headers := []string{
+		"Forwarded",
+		"X-Forwarded-For",
+		"X-Forwarded-Host",
+		"X-Forwarded-Proto",
+	}
+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		for _, h := range headers {
+			if v := r.Header.Get(h); v != "" {
+				t.Errorf("got %v header: %q", h, v)
+			}
+		}
+	}))
+	defer backend.Close()
+	backendURL, err := url.Parse(backend.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+	proxyHandler := &ReverseProxy{
+		Rewrite: func(r *ProxyRequest) {
+			r.SetURL(backendURL)
+		},
+	}
+	frontend := httptest.NewServer(proxyHandler)
+	defer frontend.Close()
+
+	getReq, _ := http.NewRequest("GET", frontend.URL, nil)
+	getReq.Host = "some-name"
+	getReq.Close = true
+	for _, h := range headers {
+		getReq.Header.Set(h, "x")
+	}
+	res, err := frontend.Client().Do(getReq)
+	if err != nil {
+		t.Fatalf("Get: %v", err)
+	}
+	res.Body.Close()
+}
+
 var proxyQueryTests = []struct {
 	baseSuffix string // suffix to add to backend URL
 	reqSuffix  string // suffix to add to frontend's request URL
@@ -1523,6 +1563,40 @@ func TestUnannouncedTrailer(t *testing.T) {
 
 }
 
+func TestSetURL(t *testing.T) {
+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte(r.Host))
+	}))
+	defer backend.Close()
+	backendURL, err := url.Parse(backend.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+	proxyHandler := &ReverseProxy{
+		Rewrite: func(r *ProxyRequest) {
+			r.SetURL(backendURL)
+		},
+	}
+	frontend := httptest.NewServer(proxyHandler)
+	defer frontend.Close()
+	frontendClient := frontend.Client()
+
+	res, err := frontendClient.Get(frontend.URL)
+	if err != nil {
+		t.Fatalf("Get: %v", err)
+	}
+	defer res.Body.Close()
+
+	body, err := io.ReadAll(res.Body)
+	if err != nil {
+		t.Fatalf("Reading body: %v", err)
+	}
+
+	if got, want := string(body), backendURL.Host; got != want {
+		t.Errorf("backend got Host %q, want %q", got, want)
+	}
+}
+
 func TestSingleJoinSlash(t *testing.T) {
 	tests := []struct {
 		slasha   string
@@ -1572,3 +1646,28 @@ func TestJoinURLPath(t *testing.T) {
 		}
 	}
 }
+
+func TestReverseProxyRewriteReplacesOut(t *testing.T) {
+	const content = "response_content"
+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte(content))
+	}))
+	defer backend.Close()
+	proxyHandler := &ReverseProxy{
+		Rewrite: func(r *ProxyRequest) {
+			r.Out, _ = http.NewRequest("GET", backend.URL, nil)
+		},
+	}
+	frontend := httptest.NewServer(proxyHandler)
+	defer frontend.Close()
+
+	res, err := frontend.Client().Get(frontend.URL)
+	if err != nil {
+		t.Fatalf("Get: %v", err)
+	}
+	defer res.Body.Close()
+	body, _ := io.ReadAll(res.Body)
+	if got, want := string(body), content; got != want {
+		t.Errorf("got response %q, want %q", got, want)
+	}
+}