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

[dunglas]

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.

[ianlancetaylor]

CC @neild @bradfitz

[neild]

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]

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]

/cc @neild

[rsc]

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]

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]

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]

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

[rsc]

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?