diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go index 319e2a3f3f0430..4d54390baf713a 100644 --- a/src/net/http/httputil/reverseproxy.go +++ b/src/net/http/httputil/reverseproxy.go @@ -24,22 +24,37 @@ import ( "golang.org/x/net/http/httpguts" ) +type ForwardedHeaderBehavior uint8 + +const ( + // Overwrite existing X-Forwarded-For, X-Forwarded-Proto + // and X-Forwarded-Host HTTP headers with values extracted + // from the current connection with the client. + // As a special case, headers explicitly set to nil + // are never modified. + ForwardedHeaderOverwrite ForwardedHeaderBehavior = iota + + // Add client IP to the existing IPs in X-Forwarded-For. + // Preserve X-Forwarded-Proto and X-Forwarded-Host if set. + // Ensure that the previous proxies in the chain are trusted + // As a special case, headers explicitly set to nil + // are never modified. + // when using this value, or it will lead to security issues. + ForwardedHeaderAdd + + // Preserve all existing forwarded HTTP headers. + // Ensure that the previous proxies in the chain are trusted + // when using this value, or it will lead to security issues. + ForwardedHeaderPreserve +) + // ReverseProxy is an HTTP Handler that takes an incoming request and // 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. -// -// 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. -// -// To prevent IP spoofing, be sure to delete any pre-existing -// X-Forwarded-For header coming from the client or -// an untrusted proxy. +// X-Forwarded-For header. To control this behavior, +// set the ForwardedHeaderBehavior field. type ReverseProxy struct { // Director must be a function which modifies // the request into a new request to be sent @@ -92,6 +107,11 @@ type ReverseProxy struct { // If nil, the default is to log the provided error and return // a 502 Status Bad Gateway response. ErrorHandler func(http.ResponseWriter, *http.Request, error) + + // OverwriteForwardedHeaders specifies how to deal with the + // X-Forwarded-For, X-Forwarded-Proto, and X-Forwarded-Host + // headers. + ForwardedHeaderBehavior ForwardedHeaderBehavior } // A BufferPool is an interface for getting and returning temporary @@ -282,17 +302,38 @@ 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.ForwardedHeaderBehavior != ForwardedHeaderPreserve { + 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 + log.Printf("%#v %#v", prior, omit) + if p.ForwardedHeaderBehavior == ForwardedHeaderAdd && len(prior) > 0 { + clientIP = strings.Join(prior, ", ") + ", " + clientIP + } + if !omit { + outreq.Header.Set("X-Forwarded-For", clientIP) + } } - if !omit { - outreq.Header.Set("X-Forwarded-For", clientIP) + + if p.ForwardedHeaderBehavior == ForwardedHeaderOverwrite { + prior, ok := outreq.Header["X-Forwarded-Host"] + omit := ok && prior == nil // nil means don't populate the header + if !omit { + outreq.Header.Set("X-Forwarded-Host", req.Host) + } + + prior, ok = outreq.Header["X-Forwarded-Proto"] + omit = ok && prior == nil // nil means don't populate the header + if !omit { + if req.TLS == nil { + outreq.Header.Set("X-Forwarded-Proto", "http") + } else { + outreq.Header.Set("X-Forwarded-Proto", "https") + } + } } } diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go index 90e8903e9cfd5b..d2e7b27db2d7a2 100644 --- a/src/net/http/httputil/reverseproxy_test.go +++ b/src/net/http/httputil/reverseproxy_test.go @@ -13,6 +13,7 @@ import ( "errors" "fmt" "io" + "io/ioutil" "log" "net/http" "net/http/httptest" @@ -297,6 +298,8 @@ func TestReverseProxyStripEmptyConnection(t *testing.T) { func TestXForwardedFor(t *testing.T) { const prevForwardedFor = "client ip" + const prevForwardedProto = "https" + const prevForwardedHost = "example.com" const backendResponse = "I am the backend" const backendStatus = 404 backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -306,6 +309,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 !strings.Contains(r.Header.Get("X-Forwarded-Proto"), prevForwardedProto) { + t.Errorf("X-Forwarded-Proto didn't contain prior data") + } + if !strings.Contains(r.Header.Get("X-Forwarded-Host"), prevForwardedHost) { + t.Errorf("X-Forwarded-Host didn't contain prior data") + } w.WriteHeader(backendStatus) w.Write([]byte(backendResponse)) })) @@ -322,6 +331,8 @@ func TestXForwardedFor(t *testing.T) { getReq.Host = "some-name" getReq.Header.Set("Connection", "close") getReq.Header.Set("X-Forwarded-For", prevForwardedFor) + getReq.Header.Set("X-Forwarded-Proto", prevForwardedProto) + getReq.Header.Set("X-Forwarded-Host", prevForwardedHost) getReq.Close = true res, err := frontend.Client().Do(getReq) if err != nil { @@ -336,6 +347,274 @@ func TestXForwardedFor(t *testing.T) { } } +func TestForwardedHeaderOverwrite(t *testing.T) { + const prevForwardedFor = "client ip" + const prevForwardedProto = "https" + const prevForwardedHost = "example.com" + const backendResponse = "I am the backend" + const backendStatus = 404 + 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") + } + if strings.Contains(r.Header.Get("X-Forwarded-For"), prevForwardedFor) { + t.Errorf("X-Forwarded-For contains prior data") + } + if strings.Contains(r.Header.Get("X-Forwarded-Proto"), prevForwardedProto) { + t.Errorf("X-Forwarded-Proto contains prior data") + } + if strings.Contains(r.Header.Get("X-Forwarded-Host"), prevForwardedHost) { + t.Errorf("X-Forwarded-Host contains prior data") + } + w.WriteHeader(backendStatus) + w.Write([]byte(backendResponse)) + })) + defer backend.Close() + backendURL, err := url.Parse(backend.URL) + if err != nil { + t.Fatal(err) + } + proxyHandler := NewSingleHostReverseProxy(backendURL) + frontend := httptest.NewServer(proxyHandler) + defer frontend.Close() + + getReq, _ := http.NewRequest("GET", frontend.URL, nil) + getReq.Host = "some-name" + getReq.Header.Set("Connection", "close") + getReq.Header.Set("X-Forwarded-For", prevForwardedFor) + getReq.Header.Set("X-Forwarded-Proto", prevForwardedProto) + getReq.Header.Set("X-Forwarded-Host", prevForwardedHost) + getReq.Close = true + res, err := frontend.Client().Do(getReq) + if err != nil { + t.Fatalf("Get: %v", err) + } + if g, e := res.StatusCode, backendStatus; g != e { + t.Errorf("got res.StatusCode %d; expected %d", g, e) + } + bodyBytes, _ := ioutil.ReadAll(res.Body) + if g, e := string(bodyBytes), backendResponse; g != e { + t.Errorf("got body %q; expected %q", g, e) + } +} + +func TestForwardedHeaderOverwrite_Omit(t *testing.T) { + const prevForwardedFor = "client ip" + const prevForwardedProto = "https" + const prevForwardedHost = "example.com" + const backendResponse = "I am the backend" + const backendStatus = 404 + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header["X-Forwarded-For"] != nil { + t.Errorf("X-Forwarded-For header is not nil") + } + if r.Header["X-Forwarded-Proto"] != nil { + t.Errorf("X-Forwarded-Proto header is not nil") + } + if r.Header["X-Forwarded-For"] != nil { + t.Errorf("X-Forwarded-For header is not nil") + } + w.WriteHeader(backendStatus) + w.Write([]byte(backendResponse)) + })) + defer backend.Close() + backendURL, err := url.Parse(backend.URL) + if err != nil { + t.Fatal(err) + } + proxyHandler := NewSingleHostReverseProxy(backendURL) + frontend := httptest.NewServer(proxyHandler) + defer frontend.Close() + + oldDirector := proxyHandler.Director + proxyHandler.Director = func(r *http.Request) { + r.Header["X-Forwarded-For"] = nil + r.Header["X-Forwarded-Proto"] = nil + r.Header["X-Forwarded-Host"] = nil + oldDirector(r) + } + + getReq, _ := http.NewRequest("GET", frontend.URL, nil) + getReq.Host = "some-name" + getReq.Header.Set("Connection", "close") + getReq.Header.Set("X-Forwarded-For", prevForwardedFor) + getReq.Header.Set("X-Forwarded-Proto", prevForwardedProto) + getReq.Header.Set("X-Forwarded-Host", prevForwardedHost) + getReq.Close = true + res, err := frontend.Client().Do(getReq) + if err != nil { + t.Fatalf("Get: %v", err) + } + if g, e := res.StatusCode, backendStatus; g != e { + t.Errorf("got res.StatusCode %d; expected %d", g, e) + } + bodyBytes, _ := ioutil.ReadAll(res.Body) + if g, e := string(bodyBytes), backendResponse; g != e { + t.Errorf("got body %q; expected %q", g, e) + } +} + +func TestForwardedHeaderAdd(t *testing.T) { + const prevForwardedFor = "127.0.0.42" + const prevForwardedProto = "https" + const prevForwardedHost = "example.com" + const backendResponse = "I am the backend" + const backendStatus = 404 + 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") + } + if r.Header.Get("X-Forwarded-For") == "127.0.0.42,127.0.0.1" { + t.Errorf("X-Forwarded-For does not contain prior and current data") + } + if r.Header.Get("X-Forwarded-Proto") != prevForwardedProto { + t.Errorf("X-Forwarded-Proto does not contain prior data") + } + if r.Header.Get("X-Forwarded-Host") != prevForwardedHost { + t.Errorf("X-Forwarded-Host does not contain prior data") + } + w.WriteHeader(backendStatus) + w.Write([]byte(backendResponse)) + })) + defer backend.Close() + backendURL, err := url.Parse(backend.URL) + if err != nil { + t.Fatal(err) + } + proxyHandler := NewSingleHostReverseProxy(backendURL) + proxyHandler.ForwardedHeaderBehavior = ForwardedHeaderAdd + frontend := httptest.NewServer(proxyHandler) + defer frontend.Close() + + getReq, _ := http.NewRequest("GET", frontend.URL, nil) + getReq.Host = "some-name" + getReq.Header.Set("Connection", "close") + getReq.Header.Set("X-Forwarded-For", prevForwardedFor) + getReq.Header.Set("X-Forwarded-Proto", prevForwardedProto) + getReq.Header.Set("X-Forwarded-Host", prevForwardedHost) + getReq.Close = true + res, err := frontend.Client().Do(getReq) + if err != nil { + t.Fatalf("Get: %v", err) + } + if g, e := res.StatusCode, backendStatus; g != e { + t.Errorf("got res.StatusCode %d; expected %d", g, e) + } + bodyBytes, _ := ioutil.ReadAll(res.Body) + if g, e := string(bodyBytes), backendResponse; g != e { + t.Errorf("got body %q; expected %q", g, e) + } +} + +func TestForwardedHeaderAdd_Omit(t *testing.T) { + const prevForwardedFor = "127.0.0.42" + const prevForwardedProto = "https" + const prevForwardedHost = "example.com" + const backendResponse = "I am the backend" + const backendStatus = 404 + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header["X-Forwarded-For"] != nil { + t.Errorf("X-Forwarded-For header is not nil") + } + if r.Header["X-Forwarded-Proto"] != nil { + t.Errorf("X-Forwarded-Proto header is not nil") + } + if r.Header["X-Forwarded-For"] != nil { + t.Errorf("X-Forwarded-For header is not nil") + } + w.WriteHeader(backendStatus) + w.Write([]byte(backendResponse)) + })) + defer backend.Close() + backendURL, err := url.Parse(backend.URL) + if err != nil { + t.Fatal(err) + } + proxyHandler := NewSingleHostReverseProxy(backendURL) + proxyHandler.ForwardedHeaderBehavior = ForwardedHeaderAdd + frontend := httptest.NewServer(proxyHandler) + defer frontend.Close() + + oldDirector := proxyHandler.Director + proxyHandler.Director = func(r *http.Request) { + r.Header["X-Forwarded-For"] = nil + r.Header["X-Forwarded-Proto"] = nil + r.Header["X-Forwarded-Host"] = nil + oldDirector(r) + } + + getReq, _ := http.NewRequest("GET", frontend.URL, nil) + getReq.Host = "some-name" + getReq.Header.Set("Connection", "close") + getReq.Header.Set("X-Forwarded-For", prevForwardedFor) + getReq.Header.Set("X-Forwarded-Proto", prevForwardedProto) + getReq.Header.Set("X-Forwarded-Host", prevForwardedHost) + getReq.Close = true + res, err := frontend.Client().Do(getReq) + if err != nil { + t.Fatalf("Get: %v", err) + } + if g, e := res.StatusCode, backendStatus; g != e { + t.Errorf("got res.StatusCode %d; expected %d", g, e) + } + bodyBytes, _ := ioutil.ReadAll(res.Body) + if g, e := string(bodyBytes), backendResponse; g != e { + t.Errorf("got body %q; expected %q", g, e) + } +} + +func TestForwardedHeaderPreserve(t *testing.T) { + const prevForwardedFor = "127.0.0.42" + const prevForwardedProto = "https" + const prevForwardedHost = "example.com" + const backendResponse = "I am the backend" + const backendStatus = 404 + 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") + } + if r.Header.Get("X-Forwarded-For") != "127.0.0.42" { + t.Errorf("X-Forwarded-For does not contain prior data") + } + if r.Header.Get("X-Forwarded-Proto") != prevForwardedProto { + t.Errorf("X-Forwarded-Proto does not contain prior data") + } + if r.Header.Get("X-Forwarded-Host") != prevForwardedHost { + t.Errorf("X-Forwarded-Host does not contain prior data") + } + w.WriteHeader(backendStatus) + w.Write([]byte(backendResponse)) + })) + defer backend.Close() + backendURL, err := url.Parse(backend.URL) + if err != nil { + t.Fatal(err) + } + proxyHandler := NewSingleHostReverseProxy(backendURL) + proxyHandler.ForwardedHeaderBehavior = ForwardedHeaderPreserve + frontend := httptest.NewServer(proxyHandler) + defer frontend.Close() + + getReq, _ := http.NewRequest("GET", frontend.URL, nil) + getReq.Host = "some-name" + getReq.Header.Set("Connection", "close") + getReq.Header.Set("X-Forwarded-For", prevForwardedFor) + getReq.Header.Set("X-Forwarded-Proto", prevForwardedProto) + getReq.Header.Set("X-Forwarded-Host", prevForwardedHost) + getReq.Close = true + res, err := frontend.Client().Do(getReq) + if err != nil { + t.Fatalf("Get: %v", err) + } + if g, e := res.StatusCode, backendStatus; g != e { + t.Errorf("got res.StatusCode %d; expected %d", g, e) + } + bodyBytes, _ := ioutil.ReadAll(res.Body) + if g, e := string(bodyBytes), backendResponse; g != e { + t.Errorf("got body %q; expected %q", g, e) + } +} + // 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) {