[go] net/http/httputil: add support for X-Forwarded-Proto, X-Forwarded-Host and an option to not trust forwarded headers in ReverseProxy
Gerrit Bot (Gerrit)
Gerrit Bot would like Kévin Dunglas to review this change.
net/http/httputil: add support for X-Forwarded-Proto, X-Forwarded-Host and an option to not trust forwarded headers in ReverseProxy
In addition to X-Forwarded-For, which is already supported but undocumented
(#36672), this patch adds support for the X-Forwarded-Proto (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Proto)
and X-Forwarded-Host (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host)
headers in ReverseProxy.
It also introduces a new option, ReverseProxy.TrustForwardedHeaders
that must be explicitly set to true to trust forwarded headers coming from
the client or previous proxies.
Setting these headers is a standard behavior expected from reverse
proxies, especially when X-Forwarded-For is set (which is already the
case).
The new ReverseProxy.TrustForwardedHeaders option fixes a potential
common security issue in user land code. It forces the user to explicitly
opt-in to trust reverse proxies in front of the Go application using
ReverseProxy.
The current behavior of blindly trusting the X-Forwarded-For header
provided by the client is very dangerous.
For instance, because of the current (undocumented) behavior, the Vulcain
gateway server (https://vulcain.rocks) was sensible to a potential security
issue.
If this flag is not set to true, previous values set by the client or
untrusted proxies will be discarded (replaced by the values computed
by ReverseProxy).
Technically, this is a BC break, but, in my opinion, it is a necessary one
to improve the security of the whole Go ecosystem using this module.
Change-Id: I5f9dc73afdfbc33ac87f27592ec0103fe6f4df42
GitHub-Last-Rev: 8648d028e1703ee0a55cf7bb2ef37cdc671a1d47
GitHub-Pull-Request: golang/go#36678
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 92 insertions(+), 3 deletions(-)
diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index e8f7df2..a59a0f8 100644
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -76,6 +76,16 @@
// 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)
+
+ // TrustForwardedHeaders specifies if X-Forwarded-For,
+ // X-Forwarded-Proto and X-Forwarded-Host headers comming from
+ // the previous proxy must be trusted or not.
+ // If true, existing values of X-Forwarded-Proto and
+ // X-Forwarded-Host will be preserved, and the current client IP
+ // will be appended to the list in X-Forwarded-For.
+ // If false, values of these headers will be set regardless of
+ // any existing value.
+ TrustForwardedHeaders bool
}
// A BufferPool is an interface for getting and returning temporary
@@ -236,6 +246,23 @@
outreq.Header.Set("Upgrade", reqUpType)
}
+ if _, ok := outreq.Header["X-Forwarded-Proto"]; !ok || !p.TrustForwardedHeaders {
+ proto := "https"
+ if req.TLS == nil {
+ proto = "http"
+ }
+
+ outreq.Header.Set("X-Forwarded-Proto", proto)
+ }
+
+ if _, ok := outreq.Header["X-Forwarded-Host"]; !ok || !p.TrustForwardedHeaders {
+ outreq.Header.Set("X-Forwarded-Host", outreq.Host)
+ }
+
+ if !p.TrustForwardedHeaders {
+ outreq.Header.Del("X-Forwarded-For")
+ }
+
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
diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index f58e088..fdc5b0e 100644
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -236,16 +236,24 @@
}
}
-func TestXForwardedFor(t *testing.T) {
+func TestDontTrustForwardedHeaders(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 didn't contain prior data")
+ 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))
@@ -263,6 +271,60 @@
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 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) {
+ 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 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))
+ }))
+ defer backend.Close()
+ backendURL, err := url.Parse(backend.URL)
+ if err != nil {
+ t.Fatal(err)
+ }
+ proxyHandler := NewSingleHostReverseProxy(backendURL)
+ proxyHandler.TrustForwardedHeaders = true
+ 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 {
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #2 to this change.
GitHub-Last-Rev: c0b06fb704dc4adaf9ed1ebd8d0a6fdcde69ddb7
GitHub-Pull-Request: golang/go#36678
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 100 insertions(+), 10 deletions(-)
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #3 to this change.
GitHub-Last-Rev: a4f0f3e6569525c40c1ab27babf97b8df81d7feb
GitHub-Pull-Request: golang/go#36678
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 79 insertions(+), 1 deletion(-)
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #4 to this change.
Edit: added a backward compatible implementation, see https://github.com/golang/go/pull/36678#issuecomment-577163525
Change-Id: I5f9dc73afdfbc33ac87f27592ec0103fe6f4df42
GitHub-Last-Rev: a4f0f3e6569525c40c1ab27babf97b8df81d7feb
GitHub-Pull-Request: golang/go#36678
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 79 insertions(+), 1 deletion(-)
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #5 to this change.
Edit: added a backward compatible implementation, see https://github.com/golang/go/pull/36678#issuecomment-577163525
Change-Id: I5f9dc73afdfbc33ac87f27592ec0103fe6f4df42
GitHub-Last-Rev: 64fd62d03ac30c73e06fc59ff599d761302cec44
GitHub-Pull-Request: golang/go#36678
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 79 insertions(+), 1 deletion(-)
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #6 to this change.
Edit: added a backward compatible implementation, see https://github.com/golang/go/pull/36678#issuecomment-577163525
Change-Id: I5f9dc73afdfbc33ac87f27592ec0103fe6f4df42
GitHub-Last-Rev: 44b8b3d9e317ca225a585faaebde9518a0741551
GitHub-Pull-Request: golang/go#36678
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 79 insertions(+), 1 deletion(-)
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Gopher Robot abandoned this change.
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Kévin Dunglas (Gerrit)
1 comment:
Patchset:
This PR isn't abandoned but waiting for feedback. In my opinion it is ready to be merged.
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Ian Lance Taylor restored this change.
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Kévin Dunglas.
1 comment:
Patchset:
This PR isn't abandoned but waiting for feedback. In my opinion it is ready to be merged.
Restored.
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Kévin Dunglas (Gerrit)
Attention is currently required from: Ian Lance Taylor.
1 comment:
Patchset:
Restored.
Thanks! Is there something else I can do to move this PR forward?
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
5 comments:
Commit Message:
Patch Set #6, Line 13:
It also introduces a new option, ReverseProxy.TrustForwardedHeadersThis CL description appears to be out of date. I don't see a TrustForwardedHeaders option.
Patchset:
API changes should start with a proposal.
So far as I can tell, users can get the behavior provided by this CL with a custom Director function that sets the X-Forwarded-Proto and X-Forwarded-Host headers and deletes the X-Forwarded-For header. As such, this API extension is a convenience only. Is the expansion of API surface worth that convenience?
Does this API need to consider the standards track Forwarded header from RFC 7239?
But please start with a proposal:
https://github.com/golang/proposal
File src/net/http/httputil/reverseproxy.go:
Patch Set #6, Line 107:
OverwriteForwardedHeaders boolThis option does too many things:
- It enables sending X-Forwarded-Proto and X-Forwarded-Host.
- It removes prior values of X-F-Proto, X-F-Host.
- It overwrites prior values of X-F-For rather than appending to them.
Patch Set #6, Line 296:
outreq.Header.Set("X-Forwarded-Host", outreq.Host)outreq.Host doesn't seem to be right. X-Forwarded-Host contains the Host sent to the proxy, not a copy of the Host sent by the proxy.
Patch Set #6, Line 297:
outreq.Header.Del("X-Forwarded-For")The user can set X-Forwarded-For to nil to prevent adding a new value. Deleting X-Forwarded-For defeats this behavior.
Should we omit sending X-F-Proto and X-F-Host if they are set to nil? Should we set X-F-Proto if we don't set X-F-For?
There needs to be consideration of how these various configuration knobs interact.
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Kévin Dunglas (Gerrit)
Attention is currently required from: Damien Neil.
2 comments:
Patchset:
API changes should start with a proposal. […]
I opened a proposal detailing while I think it is worth it: https://github.com/golang/go/issues/50465
File src/net/http/httputil/reverseproxy.go:
Patch Set #6, Line 297:
outreq.Header.Del("X-Forwarded-For")The user can set X-Forwarded-For to nil to prevent adding a new value. […]
This behavior has been added after I crafted this patch. It's why it's not supported yet: https://github.com/golang/go/issues/38079
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Dou (Gerrit)
Attention is currently required from: Damien Neil.
Gerrit Dou uploaded patch set #7 to this change.
Edit: added a backward compatible implementation, see https://github.com/golang/go/pull/36678#issuecomment-577163525
Change-Id: I5f9dc73afdfbc33ac87f27592ec0103fe6f4df42
GitHub-Last-Rev: aa0ff79abfbd7a6fafa6dca27614051ebca4c2c1
GitHub-Pull-Request: golang/go#36678
---
M src/net/http/httputil/reverseproxy_test.go
M src/net/http/httputil/reverseproxy.go
2 files changed, 383 insertions(+), 21 deletions(-)
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Kévin Dunglas (Gerrit)
Attention is currently required from: Damien Neil.
3 comments:
File src/net/http/httputil/reverseproxy.go:
Patch Set #6, Line 107:
OverwriteForwardedHeaders boolThis option does too many things: […]
Changed in the proposal and fixed in patchset 7.
Patch Set #6, Line 296:
outreq.Header.Set("X-Forwarded-Host", outreq.Host)
outreq.Host doesn't seem to be right. […]
Thanks, fixed.
Patch Set #6, Line 297:
outreq.Header.Del("X-Forwarded-For")
This behavior has been added after I crafted this patch. […]
Changed in the proposal and fixed in patchset 7.
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
1 comment:
Patchset:
https://go.dev/cl/407414 changed ReverseProxy to add X-Forwarded-{Host,Proto} headers by default.
https://go.dev/issue/57132 objected to this change, and the change was reverted in https://go.dev/cl/457595. (See the issue for some discussion and rationale on reverting.)
Go 1.20 adds a new Rewrite hook to ReverseProxy, intended to address various limitations of Director hooks and superseding that hook. Programs using Rewrite can easily select their desired behavior for X-Forwarded headers:
```
// Overwrite existing X-Forwarded-* HTTP headers.
Rewrite: func(r *httputil.ProxyRequest) {
r.SetXForwarded()
}
``````
// Add to existing X-Forwarded-* HTTP headers.
Rewrite: func(r *httputil.ProxyRequest) {
r.Out.Header["X-Forwarded-For"] = r.In.Header["X-Forwarded-For"]
r.SetXForwarded()
}
``````
// Preserve existing X-Forwarded-* HTTP headers.
Rewrite: func(r *httputil.ProxyRequest) {
r.Out.Header["X-Forwarded-For"] = r.In.Header["X-Forwarded-For"]
r.Out.Header["X-Forwarded-Host"] = r.In.Header["X-Forwarded-Host"]
r.Out.Header["X-Forwarded-Proto"] = r.In.Header["X-Forwarded-Proto"]
}
```Thanks for pushing support for X-Forwarded-Proto and X-Forwarded-Host forward.
To view, visit change 215637. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Gopher Robot abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |