You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 the Connection 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:
Clone the incoming request to produce an outbound request, outreq.
Pass outreq to the Director function, which may modify it.
Remove hop-by-hop headers from outreq.
Send 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 any Forwarded header added by the Director 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 the Connection 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 existing Director function that examines hop-by-hop headers like Proxy-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 send
and after Director runs we cannot tell whether it intends the X-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 the Connection header.
I do not believe it is possible to fix this problem within the context of the existing ReverseProxy API. The problem is that the Director function makes no distinction between the inbound request and the outbound one: We cannot remove hop-by-hop headers before passing a request to Director, since Director may need to examine those headers, but removing those headers after Director runs can strip headers that should be preserved for the next hop.
I propose that we add a new field to ReverseProxy, superseding Director, which takes both the inbound and outbound request.
type ReverseProxy struct {
// ModifyRequest is an optional function that modifies the Request
// sent using Transport. The inreq is the unmodified inbound request,
// including including hop-by-hop headers. The outreq is the request
// to be sent, with hop-by-hop headers removed and the `X-Forwarded-For`
// header added.
//
// If ModifyRequest returns an error, ErrorHandler is called
// with its error value. If ErrorHandler is nil, its default
// implementation is used.
ModifyRequest func(inreq, outreq *http.Request) error
}
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:
We would need to define how ModifyRequest and Director compose: What happens if both are set? My inclination is to call this an error.
@bradfitz proposed an interesting alternative to ModifyRequest. If we pass a type to the rewrite function, we can add additional methods to that type to make it easier for users to customize the proxy behavior. For example:
Change https://go.dev/cl/407214 mentions this issue: net/http/httputil: add ReverseProxy.Rewrite
neild
added
NeedsFix
The path to resolution is known, but the work has not been done.
and removed
NeedsInvestigation
Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
labels
Jul 22, 2022
May I ask why this has neither a CVE nor a deprecation warning on the director hook?
We had to issue a security advisory because of this: GHSA-w9mr-28mw-j8hg
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: