-
Notifications
You must be signed in to change notification settings - Fork 17.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net/http/httputil: ReverseProxy can remove headers added by Director #50580
Comments
We would need to define how @bradfitz proposed an interesting alternative to
|
Change https://go.dev/cl/407214 mentions this issue: |
May I ask why this has neither a CVE nor a deprecation warning on the director hook? There is also a ton of guides out there describing the director hook, so people will keep using it unless a deprecation warning is added (the best possible action IMO). |
An HTTP request may contain header fields which are intended for the immediate recipient ("hop-by-hop"), and ones intended for all recipients on the chain of a forwarded request ("end-to-end"). For example, the "Keep-Alive" header is a hop-by-hop header, because it applies to a single TCP connection. The "Cache-Control" header, in contrast, is an end-to-end header which should be forwarded by an HTTP proxy.
RFC 2616, section 13.5.1 specified a list of hop-by-hop headers which HTTP proxies should not forward.
RFC 7230, section 6.1 replaces the hardcoded list of hop-by-hop headers with the ability for the originator of a request to specify the hop-by-hop headers in the "Connection" header.
The
httputil.ReverseProxy
proxy both understands the obsolete hardcoded list of hop-by-hop headers from RFC 2616 and theConnection
header from RFC 7230.ReverseProxy
will remove all hop-by-hop headers before forwarding a request.When ReverseProxy forwards a request, it follows the following steps in order:
outreq
.outreq
to theDirector
function, which may modify it.outreq
.outreq
to the backend.This can cause headers added by the
Director
function to be removed before the request is forwarded to the backend.For example, if an inbound request contains a
Connection: forwarded
header, then anyForwarded
header added by theDirector
will not be sent to the backend. This is probably surprising; under some circumstances, it may be a security vulnerability.The user of
httputil.ReverseProxy
can prevent a header from being stripped in this fashion by removing any headers that should not be dropped from theConnection
header. This is cumbersome, and relies on the user knowing that they must do this.Some proposed solutions:
Remove hop-by-hop headers before calling the
Director
function: This breaks any existingDirector
function that examines hop-by-hop headers likeProxy-Authorization
.Detect what headers were modified by the
Director
function and avoid removing them: This does not avoid the problem, because the attacker could send the exact header they want to remove. For example, an attacker can sendand after
Director
runs we cannot tell whether it intends theX-Forwarded-Proto
header to stay.Only remove RFC 2616 hop-by-hop headers and do not remove headers listed in the
Connection
header: This is a violation of RFC 7230, which states that proxies MUST remove headers listed in theConnection
header.I do not believe it is possible to fix this problem within the context of the existing
ReverseProxy
API. The problem is that theDirector
function makes no distinction between the inbound request and the outbound one: We cannot remove hop-by-hop headers before passing a request toDirector
, sinceDirector
may need to examine those headers, but removing those headers afterDirector
runs can strip headers that should be preserved for the next hop.I propose that we add a new field to
ReverseProxy
, supersedingDirector
, which takes both the inbound and outbound request.We do not usually backport new APIs. Since in this case the existing API is inherently insecure, I propose that we backport ModifyRequest and mark Director as deprecated.
The text was updated successfully, but these errors were encountered: