net/http/httputil: add X-Forwarded-Proto and X-Forwarded-Host by default #50465

dunglas · 2022-01-06T13:49:36Z

Go's HTTP ReverseProxy has basic forwarded headers support, but this support is incomplete and can even be dangerous:

X-Forwarded-For is set automatically, if the request already contains an X-Forwarded-For header, the client IP is added to the existing header values (the IPs of the previous proxies in the chain). This behavior was undocumented, and already led to IP spoofing vulnerabilities. I documented this behavior in net/http/httputil: add docs about X-Forwarded-For in ReverseProxy #36672. Vulnerable projects created before the documentation was added probably exist in the wild.
While X-Forwarded-For is automatically set, X-Forwarded-Proto and X-Forwarded-Host are not, which is weird.
Disabling X-Forwarded-For support is not obvious, and looks hackish: you have to manually delete the potentially existing header before forwarding the request. Edit: you can now use nil to prevent the header to be set: net/http/httputil: do not add to empty X-Forwarded-For header in ReverseProxy #38079
Only the legacy non-standard X-Forwarded-For is supported, the standard Forwarded header (RFC 7239) isn't.

I propose to add two new public fields to the ReverseProxy struct to control the behavior regarding forwarded headers:

type ForwardedHeaderBehavior uint8
const (
	// Overwrite existing forwarded HTTP headers,
	// depending on the value of ReverseProxy.ForwardedHeaderFormat,
	// with values extracted from the current connection with the client.
	// Headers explicitly set to nil are never modified.
	ForwardedHeaderOverwrite ForwardedHeaderBehavior = iota

	// Adds values to the existing forwarded HTTP headers,
	// depending on the value of ReverseProxy.ForwardedHeaderFormat,
	// with values extracted from the current connection with the client.
	// If ForwardedHeaderLegacy is used, client IP is added to X-Forwarded-For
	// and the values of X-Forwarded-Proto and X-Forwarded-Host are preserved.
	// Headers explicitly set to nil are never modified.
	// Ensure that the previous proxies in the chain are trusted 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 before using this value, or it will lead to security issues.
	ForwardedHeaderPreserve
)

type ForwardedHeaderFormat uint8
const (
	// Set the X-Forwarded-For, X-Forwarded-Proto and X-Forwarded-Host headers (Forwarded will be left as-is)
	ForwardedHeaderLegacy ForwardedHeaderFormat = iota

	// Set the Forwarded HTTP header (X-Forwarded-For, X-Forwarded-Proto, and X-Forwarded-Host will be left as-is)
	ForwardedHeaderRFC7234
)

type ReverseProxy struct {
	ForwardedHeaderBehavior ForwardedHeaderBehavior
	ForwardedHeaderFormat ForwardedHeaderFormat
}

This is technically a BC break, as the default behavior will now be to overwrite the existing X-Forwarded-* headers, whereas currently the client IP is added (usually unexpectedly) to the existing values of X-Forwarded-For. But in my opinion, it is worth it to harden the API and prevent security issues. If this BC break is unacceptable, we can reverse the order of ForwardedHeaderOverwrite and ForwardedHeaderAdd (but the default insecure behavior will persist).

Supporting the legacy XFF headers is necessary because currently most of the ecosystem only supports these. The standardized version hasn't been widely adopted so far.
Adding support for the standardized header will require more work as it will probably require writing a custom parser. I suggest implementing support for ForwardedHeaderBehavior first to fix the current behavior, and adding support for ForwardedHeaderFormat later.

I already drafted a patch implementing the first part: #36678.

The text was updated successfully, but these errors were encountered:

ianlancetaylor · 2022-01-12T17:09:56Z

CC @neild @bradfitz

neild · 2022-01-12T20:20:19Z

We don't want to add special-purpose APIs when general-purpose ones suffice. We also don't want to add knobs to enable the right behavior; if the default behavior of ReverseProxy is wrong, then we should just fix it.

I think it makes sense to add X-Forwarded-Proto and X-Forwarded-Host automatically. These headers are a de facto standard and useful. Since we add X-Forwarded-For by default, we may as well add those too. The user should be able to disable them in the same fashion as X-Forwarded-For, by setting the appropriate entries in the Request.Header map to nil.

It might make sense to add the RFC 7239 Forwaded header automatically. I don't have a good sense for how much use this header sees in the wild vs. X-Forwarded-Proto and X-Forwaded-Host. My inclination is to leave it out for now.

It is probable that appending to an existing X-Forwarded-For header rather than replacing it is the wrong default. You (usually) want to append to the header when forwarding a request from another trusted proxy, but replace it when forwarding a request from an untrusted source. The ReverseProxy documentation recommends deleting any incoming X-Forwarded-For from an untrusted source for this reason. Perhaps we should change the default.

I just filed #50580 to address an unrelated issue, which proposes replacing the Director function with a ModifyRequest function that accepts both the inbound and outbound requests. If we do add ModifyRequest, perhaps we should change the default handling of X-Forwarded-For when using that function and leave Director unchanged.

To summarize:

I think we should add X-Forwarded-Proto and X-Forwarded-Host by default.
I don't think we should add Forwarded by default. (But I could be convinced otherwise.)
I don't think we should add ForwardedHeaderBehavior or ForwardedHeaderFormat knobs to ReverseProxy. If the user wants to change the headers sent by the proxy, they can do so within the Director function (or ModifyRequest, if added).
Perhaps we should change the default behavior of appending to X-Forwarded-For rather than overwriting it.

dunglas · 2022-01-12T21:25:40Z

I don't have a good sense for how much use this header sees in the wild vs. X-Forwarded-Proto and X-Forwaded-Host.

From my experience, almost all tools support XFF headers, but only a few support Forwarded. I agree with not supporting Forwarded at this time. However, it may be interesting to move the ecosystem forward by adopting the standard. In the future, perhaps could we provide a Director/ModifyRequest function (which would not be called by default) to switch to Forwarded instead of using XFF?

The ReverseProxy documentation recommends deleting any incoming X-Forwarded-For from an untrusted source for this reason. Perhaps we should change the default.

I'm the author of this documentation. This behavior was not documented, which was/is probably a security vulnerability.
I'm totally in favor of changing the default behavior. Doing so when using the new function is a good idea, this will prevent the potential breaking change. The only drawback I see is that old code written by authors not aware of this behavior (code written before I added the documentation) will continue to be insecure.

If this works for you, I'll update my patch to only keep the addition of X-Forwarded-Proto and X-Forwarded-Host. I'll remove everything else and let you change the default behavior of X-Forwarded-For when implementing #50580 (which is a good move, by the way).

rsc · 2022-02-09T18:48:51Z

/cc @neild

rsc · 2022-02-09T19:17:22Z

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

adam-p · 2022-03-06T15:51:19Z

Perhaps we should change the default behavior of appending to X-Forwarded-For rather than overwriting it.

Doing so when using the new function is a good idea

So if ReverseProxy is internet-facing, the new ModifyRequest should be used, but if it's used at a deeper layer, then Director must be used in order to preserve the XFF header added by the first instance? This doesn't seem elegant at first glance (and seems error-prone and hard to explain). Is there a semantic difference between Director and ModifyRequest that makes this make more sense?

You (usually) want to append to the header when forwarding a request from another trusted proxy, but replace it when forwarding a request from an untrusted source.

If the idea comes around to automatically appending or overwriting depending if a source is trusted or not, there will need to be configuration available to indicate what's "trusted". (For some setups it will just be "ip.IsPrivate() => trusted". For others using Cloudflare or the like it'll need to be a list of IP ranges.)

(I'm not a big fan of overwriting the XFF list, as there are some legitimate non-security uses for the leftmost-ish XFF IP. Completely taking that option away from users seems unfortunate. On the other hand using the leftmost for security-related purposes is a big, widespread problem. So maybe taking the option away from users is a sufficient overall improvement that the loss of flexibility is justified.)

ETA: If you're overwriting XFF, then nine out of ten times you should just be setting a single-IP header like X-Real-IP instead.

rsc · 2022-03-30T17:49:04Z

It seems clear that ReverseProxy is not right in certain contexts, but we don't know the right path forward for it.

Perhaps the right next step is to put this on hold (or decline it) and suggest that people fork ReverseProxy and experiment in their own copies?

rsc · 2022-04-06T17:48:29Z

It seems like we should decline this until we have a clearer idea of a path forward (or a new package).

rsc · 2022-04-06T17:50:34Z

Actually, on Jan 12, @neild wrote

I think we should add X-Forwarded-Proto and X-Forwarded-Host by default.

Should we make that a separate proposal? Or should we make this proposal be about that?

dunglas · 2022-04-08T20:01:25Z

If we make the proposal about that, I can easily adapt the patch to do this.

rsc · 2022-04-13T17:42:43Z

Retitled. Does anyone object to adding X-Forwarded-Proto and X-Forwarded-Host by default?

rsc · 2022-05-04T18:06:31Z

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

rsc · 2022-05-11T18:02:17Z

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

dunglas · 2022-05-11T18:18:17Z

I'll update #36678 according to the accepted proposal.

gopherbot · 2022-05-19T20:27:19Z

Change https://go.dev/cl/407414 mentions this issue: net/http/httputil: add X-Forwarded-{Host,Proto} headers in ReverseProxy

see golang/go#50465 PiperOrigin-RevId: 469184819 Change-Id: I05a5c6a59b788e769c2c85b5f27efabbdc4e2629

gopherbot · 2022-11-15T00:35:56Z

Change https://go.dev/cl/450515 mentions this issue: doc/go1.20: add release notes for net/http and net/http/httputil

For #41773 For #41773 For #50465 For #51914 For #53002 For #53896 For #53960 For #54136 For #54299 Change-Id: I729d5eafc1940d5706f980882a08ece1f69bb42c Reviewed-on: https://go-review.googlesource.com/c/go/+/450515 Auto-Submit: Damien Neil <dneil@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Damien Neil <dneil@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com>

gopherbot · 2022-12-14T18:05:56Z

Change https://go.dev/cl/457595 mentions this issue: net/http/httputil: don't add X-Forwarded-{Host,Proto} after invoking Director funcs

…Director funcs This reverts CL 407414. When forwarding an inbound request that contains an existing X-Forwarded-Host or X-Forwarded-Proto header, a proxy might want to preserve the header from the inbound request, replace it with its own header, or not include any header at all. CL 407414 replaces inbound X-Forwarded-{Host,Proto} headers by default, and allows a Director func to disable sending these headers at all. However, the Director hook API isn't sufficiently flexible to permit the previous behavior of preserving inbound values unchanged. The new Rewrite API does have this flexibility; users of Rewrite can easily pick the exact behavior they want. Revert the change to ReverseProxy when using a Director func. Users who want a convenient way to set X-Forwarded-* headers to reasonable values can migrate to Rewrite at their convenience, and users depending on the current behavior will be unaffected. For #50465. Fixes #57132. Change-Id: Ic42449c1bb525d6c9920bf721efbc519697f4f20 Reviewed-on: https://go-review.googlesource.com/c/go/+/457595 Run-TryBot: Damien Neil <dneil@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org>

dunglas added the Proposal label Jan 6, 2022

gopherbot added this to the Proposal milestone Jan 6, 2022

dunglas added a commit to dunglas/go that referenced this issue Jan 6, 2022

implement proposal golang#50465

Verified

This commit was signed with the committer’s verified signature.

dunglas Kévin Dunglas

GPG key ID: 4D04EBEF06AAF3A6

Verified
Learn about vigilant mode

aa0ff79

rsc mentioned this issue Feb 9, 2022

proposal: net/http/httputil: can ReverseProxy copy src header to dst header with Set() instead of Add() ? #50730

Closed

francislavoie mentioned this issue Mar 6, 2022

net/http: Header.Get should return the last header rather than the first #51493

Closed

adam-p mentioned this issue Mar 6, 2022

reverseproxy: Implement trusted proxies for X-Forwarded-* headers caddyserver/caddy#4507

Merged

rsc changed the title ~~proposal: net/http/httputil: better support for forwarded headers~~ proposal: net/http/httputil: add X-Forwarded-Proto and X-Forwarded-Host by default Apr 13, 2022

rsc added the Proposal-FinalCommentPeriod label May 4, 2022

rsc changed the title ~~proposal: net/http/httputil: add X-Forwarded-Proto and X-Forwarded-Host by default~~ net/http/httputil: add X-Forwarded-Proto and X-Forwarded-Host by default May 11, 2022

rsc added the Proposal-Accepted label May 11, 2022

rsc modified the milestones: Proposal, Backlog May 11, 2022

neild mentioned this issue Jun 8, 2022

net/http/httputil: replace Director with Rewrite #53002

Closed

rsc moved this to Accepted in Proposals Aug 10, 2022

rsc added this to Proposals Aug 10, 2022

gopherbot closed this as completed in 1513e57 Aug 12, 2022

copybara-service bot pushed a commit to GoogleCloudPlatform/smart-on-fhir that referenced this issue Aug 22, 2022

fix proxy x-forwarded headers to pass tests with go 1.20

6271e3b

see golang/go#50465 PiperOrigin-RevId: 469184819 Change-Id: I05a5c6a59b788e769c2c85b5f27efabbdc4e2629

rsc removed this from Proposals Aug 16, 2023

golang locked and limited conversation to collaborators Dec 14, 2023

gopherbot added the FrozenDueToAge label Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http/httputil: add X-Forwarded-Proto and X-Forwarded-Host by default #50465

net/http/httputil: add X-Forwarded-Proto and X-Forwarded-Host by default #50465

dunglas commented Jan 6, 2022 •

edited

Loading

ianlancetaylor commented Jan 12, 2022

neild commented Jan 12, 2022

dunglas commented Jan 12, 2022 •

edited

Loading

rsc commented Feb 9, 2022

rsc commented Feb 9, 2022

adam-p commented Mar 6, 2022 •

edited

Loading

rsc commented Mar 30, 2022

rsc commented Apr 6, 2022

rsc commented Apr 6, 2022

dunglas commented Apr 8, 2022

rsc commented Apr 13, 2022

rsc commented May 4, 2022

rsc commented May 11, 2022

dunglas commented May 11, 2022

gopherbot commented May 19, 2022

gopherbot commented Nov 15, 2022

gopherbot commented Dec 14, 2022

net/http/httputil: add X-Forwarded-Proto and X-Forwarded-Host by default #50465

net/http/httputil: add X-Forwarded-Proto and X-Forwarded-Host by default #50465

Comments

dunglas commented Jan 6, 2022 • edited Loading

ianlancetaylor commented Jan 12, 2022

neild commented Jan 12, 2022

dunglas commented Jan 12, 2022 • edited Loading

rsc commented Feb 9, 2022

rsc commented Feb 9, 2022

adam-p commented Mar 6, 2022 • edited Loading

rsc commented Mar 30, 2022

rsc commented Apr 6, 2022

rsc commented Apr 6, 2022

dunglas commented Apr 8, 2022

rsc commented Apr 13, 2022

rsc commented May 4, 2022

rsc commented May 11, 2022

dunglas commented May 11, 2022

gopherbot commented May 19, 2022

gopherbot commented Nov 15, 2022

gopherbot commented Dec 14, 2022

dunglas commented Jan 6, 2022 •

edited

Loading

dunglas commented Jan 12, 2022 •

edited

Loading

adam-p commented Mar 6, 2022 •

edited

Loading