net/http/httputil: deprecate ReverseProxy.Director

The Director function has been superseded by Rewrite.
Rewrite avoids fundamental security issues with hop-by-hop header
handling in the Director API and has better default handling
of X-Forwarded-* headers.

Fixes #73161

Change-Id: Iadaf3070e0082458f79fb892ade51cb7ce832802
Reviewed-on: https://go-review.googlesource.com/c/go/+/708615
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>

diff --git a/api/next/73161.txt b/api/next/73161.txt
new file mode 100644 (file)
index 0000000..86526b5
--- /dev/null
+++ b/api/next/73161.txt
@@ -0,0 +1 @@
+pkg net/http/httputil, type ReverseProxy struct, Director //deprecated #73161

diff --git a/doc/next/6-stdlib/99-minor/net/http/httputil/73161.md b/doc/next/6-stdlib/99-minor/net/http/httputil/73161.md
new file mode 100644 (file)
index 0000000..f6318f8
--- /dev/null
+++ b/doc/next/6-stdlib/99-minor/net/http/httputil/73161.md
@@ -0,0 +1,11 @@
+The [ReverseProxy.Director] configuration field is deprecated
+in favor of [ReverseProxy.Rewrite].
+
+A malicious client can remove headers added by a `Director` function
+by designating those headers as hop-by-hop. Since there is no way to address
+this problem within the scope of the `Director` API, we added a new
+`Rewrite` hook in Go 1.20. `Rewrite` hooks are provided with both the
+unmodified inbound request received by the proxy and the outbound request
+which will be sent by the proxy.
+
+Since the `Director` hook is fundamentally unsafe, we are now deprecating it.

diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 6ed4930727404b0fcbce93e5625502e1f13e6183..9d8784cd2bc7a9c879c380e854723a05df461ed3 100644 (file)
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -133,36 +133,6 @@ type ReverseProxy struct {
        // At most one of Rewrite or Director may be set.
        Rewrite func(*ProxyRequest)
 
-       // Director is a function which modifies
-       // the request into a new request to be sent
-       // using Transport. Its response is then copied
-       // back to the original client unmodified.
-       // Director must not access the provided Request
-       // after returning.
-       //
-       // By default, the X-Forwarded-For header is set to the
-       // value of the client IP address. If an X-Forwarded-For
-       // header already exists, the client IP is appended to the
-       // existing values. As a special case, if the header
-       // exists in the Request.Header map but has a nil value
-       // (such as when set by the Director func), the X-Forwarded-For
-       // header is not modified.
-       //
-       // To prevent IP spoofing, be sure to delete any pre-existing
-       // X-Forwarded-For header coming from the client or
-       // an untrusted proxy.
-       //
-       // Hop-by-hop headers are removed from the request after
-       // Director returns, which can remove headers added by
-       // Director. Use a Rewrite function instead to ensure
-       // modifications to the request are preserved.
-       //
-       // Unparsable query parameters are removed from the outbound
-       // request if Request.Form is set after Director returns.
-       //
-       // At most one of Rewrite or Director may be set.
-       Director func(*http.Request)
-
        // The transport used to perform proxy requests.
        // If nil, http.DefaultTransport is used.
        Transport http.RoundTripper
@@ -210,6 +180,88 @@ type ReverseProxy struct {
        // If nil, the default is to log the provided error and return
        // a 502 Status Bad Gateway response.
        ErrorHandler func(http.ResponseWriter, *http.Request, error)
+
+       // Director is deprecated. Use Rewrite instead.
+       //
+       // This function is insecure:
+       //
+       //   - Hop-by-hop headers are removed from the request after Director
+       //     returns, which can remove headers added by Director.
+       //     A client can designate headers as hop-by-hop by listing them
+       //     in the Connection header, so this permits a malicious client
+       //     to remove any headers that may be added by Director.
+       //
+       //   - X-Forwarded-For, X-Forwarded-Host, and X-Forwarded-Proto
+       //     headers in inbound requests are preserved by default,
+       //     which can permit IP spoofing if the Director function is
+       //     not careful to remove these headers.
+       //
+       // Rewrite addresses these issues.
+       //
+       // As an example of converting a Director function to Rewrite:
+       //
+       //      // ReverseProxy with a Director function.
+       //      proxy := &httputil.ReverseProxy{
+       //              Director: func(req *http.Request) {
+       //                      req.URL.Scheme = "https"
+       //                      req.URL.Host = proxyHost
+       //
+       //                      // A malicious client can remove this header.
+       //                      req.Header.Set("Some-Header", "some-header-value")
+       //
+       //                      // X-Forwarded-* headers sent by the client are preserved,
+       //                      // since Director did not remove them.
+       //              },
+       //      }
+       //
+       //      // ReverseProxy with a Rewrite function.
+       //      proxy := &httputil.ReverseProxy{
+       //              Rewrite: func(preq *httputil.ProxyRequest) {
+       //                      // See also ProxyRequest.SetURL.
+       //                      preq.Out.URL.Scheme = "https"
+       //                      preq.Out.URL.Host = proxyHost
+       //
+       //                      // This header cannot be affected by a malicious client.
+       //                      preq.Out.Header.Set("Some-Header", "some-header-value")
+       //
+       //                      // X-Forwarded- headers sent by the client have been
+       //                      // removed from preq.Out.
+       //                      // ProxyRequest.SetXForwarded optionally adds new ones.
+       //                      preq.SetXForwarded()
+       //              },
+       //      }
+       //
+       // Director is a function which modifies
+       // the request into a new request to be sent
+       // using Transport. Its response is then copied
+       // back to the original client unmodified.
+       // Director must not access the provided Request
+       // after returning.
+       //
+       // By default, the X-Forwarded-For header is set to the
+       // value of the client IP address. If an X-Forwarded-For
+       // header already exists, the client IP is appended to the
+       // existing values. As a special case, if the header
+       // exists in the Request.Header map but has a nil value
+       // (such as when set by the Director func), the X-Forwarded-For
+       // header is not modified.
+       //
+       // To prevent IP spoofing, be sure to delete any pre-existing
+       // X-Forwarded-For header coming from the client or
+       // an untrusted proxy.
+       //
+       // Hop-by-hop headers are removed from the request after
+       // Director returns, which can remove headers added by
+       // Director. Use a Rewrite function instead to ensure
+       // modifications to the request are preserved.
+       //
+       // Unparsable query parameters are removed from the outbound
+       // request if Request.Form is set after Director returns.
+       //
+       // At most one of Rewrite or Director may be set.
+       //
+       // Deprecated: Use Rewrite instead.
+       Director func(*http.Request)
 }
 
 // A BufferPool is an interface for getting and returning temporary
@@ -259,6 +311,10 @@ func joinURLPath(a, b *url.URL) (path, rawpath string) {
 //
 // NewSingleHostReverseProxy does not rewrite the Host header.
 //
+// For backwards compatibility reasons, NewSingleHostReverseProxy
+// returns a ReverseProxy using the deprecated Director function.
+// This proxy preserves X-Forwarded-* headers sent by the client.
+//
 // To customize the ReverseProxy behavior beyond what
 // NewSingleHostReverseProxy provides, use ReverseProxy directly
 // with a Rewrite function. The ProxyRequest SetURL method