From 1513e57b704056b794f0706362fa3c949f2972a4 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 19 May 2022 13:20:21 -0700 Subject: [PATCH] net/http/httputil: add X-Forwarded-{Host,Proto} headers in ReverseProxy X-Forwarded-Host contains the original request's host. X-Forwarded-Proto contains "http" or "https", depending on whether the original request was made on a TLS-secured connection. Setting either header to nil in Director disables adding the header, same as for X-Forwarded-For. Fixes #50465. Change-Id: If8ed1f48d83f8ea0389c53519bc7994cb53891db Reviewed-on: https://go-review.googlesource.com/c/go/+/407414 Reviewed-by: Michael Knyszek TryBot-Result: Gopher Robot Run-TryBot: Damien Neil Reviewed-by: Brad Fitzpatrick --- src/net/http/httputil/reverseproxy.go | 26 ++++++--- src/net/http/httputil/reverseproxy_test.go | 61 ++++++++++++++++++---- 2 files changed, 72 insertions(+), 15 deletions(-) diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go index b5d3ce7110..a5a3900fb3 100644 --- a/src/net/http/httputil/reverseproxy.go +++ b/src/net/http/httputil/reverseproxy.go @@ -28,14 +28,18 @@ import ( // sends it to another server, proxying the response back to the // client. // -// ReverseProxy by default sets the client IP as the value of the -// X-Forwarded-For header. +// 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. // // If an X-Forwarded-For header already exists, the client IP is -// appended to the existing values. As a special case, if the header -// exists in the Request.Header map but has a nil value (such as when -// set by the Director func), the X-Forwarded-For header is -// not modified. +// appended to the existing values. +// +// 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. // // To prevent IP spoofing, be sure to delete any pre-existing // X-Forwarded-For header coming from the client or @@ -306,6 +310,16 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { 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") + } + } res, err := transport.RoundTrip(outreq) if err != nil { diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go index 90e8903e9c..23453c8bdc 100644 --- a/src/net/http/httputil/reverseproxy_test.go +++ b/src/net/http/httputil/reverseproxy_test.go @@ -50,6 +50,12 @@ func TestReverseProxy(t *testing.T) { if r.Header.Get("X-Forwarded-For") == "" { t.Errorf("didn't get X-Forwarded-For header") } + if r.Header.Get("X-Forwarded-Host") == "" { + t.Errorf("didn't get X-Forwarded-Host header") + } + if r.Header.Get("X-Forwarded-Proto") == "" { + t.Errorf("didn't get X-Forwarded-Proto header") + } if c := r.Header.Get("Connection"); c != "" { t.Errorf("handler got Connection header value %q", c) } @@ -299,6 +305,7 @@ func TestXForwardedFor(t *testing.T) { const prevForwardedFor = "client ip" const backendResponse = "I am the backend" const backendStatus = 404 + const host = "some-name" backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Header.Get("X-Forwarded-For") == "" { t.Errorf("didn't get X-Forwarded-For header") @@ -306,6 +313,12 @@ func TestXForwardedFor(t *testing.T) { if !strings.Contains(r.Header.Get("X-Forwarded-For"), prevForwardedFor) { t.Errorf("X-Forwarded-For didn't contain prior data") } + if got, want := r.Header.Get("X-Forwarded-Host"), host; got != want { + t.Errorf("X-Forwarded-Host = %q, want %q", got, want) + } + if got, want := r.Header.Get("X-Forwarded-Proto"), "http"; got != want { + t.Errorf("X-Forwarded-Proto = %q, want %q", got, want) + } w.WriteHeader(backendStatus) w.Write([]byte(backendResponse)) })) @@ -319,7 +332,7 @@ func TestXForwardedFor(t *testing.T) { defer frontend.Close() getReq, _ := http.NewRequest("GET", frontend.URL, nil) - getReq.Host = "some-name" + getReq.Host = host getReq.Header.Set("Connection", "close") getReq.Header.Set("X-Forwarded-For", prevForwardedFor) getReq.Close = true @@ -336,11 +349,36 @@ func TestXForwardedFor(t *testing.T) { } } +func TestXForwardedProtoTLS(t *testing.T) { + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got, want := r.Header.Get("X-Forwarded-Proto"), "https"; got != want { + t.Errorf("X-Forwarded-Proto = %q, want %q", got, want) + } + })) + defer backend.Close() + backendURL, err := url.Parse(backend.URL) + if err != nil { + t.Fatal(err) + } + proxyHandler := NewSingleHostReverseProxy(backendURL) + frontend := httptest.NewTLSServer(proxyHandler) + defer frontend.Close() + + getReq, _ := http.NewRequest("GET", frontend.URL, nil) + getReq.Host = "some-host" + _, err = frontend.Client().Do(getReq) + if err != nil { + t.Fatalf("Get: %v", err) + } +} + // Issue 38079: don't append to X-Forwarded-For if it's present but nil func TestXForwardedFor_Omit(t *testing.T) { backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if v := r.Header.Get("X-Forwarded-For"); v != "" { - t.Errorf("got X-Forwarded-For header: %q", v) + for _, h := range []string{"X-Forwarded-For", "X-Forwarded-Host", "X-Forwarded-Proto"} { + if v := r.Header.Get(h); v != "" { + t.Errorf("got %v header: %q", h, v) + } } w.Write([]byte("hi")) })) @@ -356,6 +394,8 @@ func TestXForwardedFor_Omit(t *testing.T) { oldDirector := proxyHandler.Director proxyHandler.Director = func(r *http.Request) { r.Header["X-Forwarded-For"] = nil + r.Header["X-Forwarded-Host"] = nil + r.Header["X-Forwarded-Proto"] = nil oldDirector(r) } @@ -1029,13 +1069,16 @@ func TestClonesRequestHeaders(t *testing.T) { } rp.ServeHTTP(httptest.NewRecorder(), req) - if req.Header.Get("From-Director") == "1" { - t.Error("Director header mutation modified caller's request") + for _, h := range []string{ + "From-Director", + "X-Forwarded-For", + "X-Forwarded-Host", + "X-Forwarded-Proto", + } { + if req.Header.Get(h) != "" { + t.Errorf("%v header mutation modified caller's request", h) + } } - if req.Header.Get("X-Forwarded-For") != "" { - t.Error("X-Forward-For header mutation modified caller's request") - } - } type roundTripperFunc func(req *http.Request) (*http.Response, error)