net/http/httputil: ReverseProxy can remove headers added by Director

[neild]

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:

1. Clone the incoming request to produce an outbound request, outreq.
2. Pass outreq to the Director function, which may modify it.
3. Remove hop-by-hop headers from outreq.
4. 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

  Connection: x-forwarded-proto
  X-Forwarded-Proto: https

  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.

[neild]

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:

proxy := &ReverseProxy{
  Rewrite: func(r *Rewriter) {
    r.SetXForwarded()    // sets X-Forwarded-{For,Host,Proto}
    r.AppendXForwarded() // appends to X-Forwarded-For (current behavior)
    r.SetForwarded()     // sets RFC 7239 Forwarded header
    r.SetURL(url)        // NewSingleHostReverseProxy-style routing
  },
}

[gopherbot]

Change https://go.dev/cl/407214 mentions this issue: net/http/httputil: add ReverseProxy.Rewrite

[zepatrik]

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: https://github.com/ory/oathkeeper/security/advisories/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).