net/http/httputil: ReverseProxy does not set host header correctly

[broady]

The HTTP/2 client prefers Request.Host over Request.URL.Host:

go/src/net/http/h2_bundle.go

Lines 7947 to 7954 in a0d6420

	func (cc *http2ClientConn) encodeHeaders(req *Request, addGzipHeader bool, trailers string, contentLength int64) ([]byte, error) {
	cc.hbuf.Reset()
	host := req.Host
	if host == "" {
	host = req.URL.Host
	}
	host, err := httpguts.PunycodeHostPort(host)

ReverseProxy's default Director only sets Request.URL.Host, and not Request.Host, so the request would get proxied to the same host, rather than the proxy target. This resulted in an infinite loop on golang.org (see Issue #28134).

go/src/net/http/httputil/reverseproxy.go

Lines 105 to 121 in a0d6420

	func NewSingleHostReverseProxy(target *url.URL) *ReverseProxy {
	targetQuery := target.RawQuery
	director := func(req *http.Request) {
	req.URL.Scheme = target.Scheme
	req.URL.Host = target.Host
	req.URL.Path = singleJoiningSlash(target.Path, req.URL.Path)
	if targetQuery == "" || req.URL.RawQuery == "" {
	req.URL.RawQuery = targetQuery + req.URL.RawQuery
	} else {
	req.URL.RawQuery = targetQuery + "&" + req.URL.RawQuery
	}
	if _, ok := req.Header["User-Agent"]; !ok {
	// explicitly disable User-Agent so it's not set to default value
	req.Header.Set("User-Agent", "")
	}
	}
	return &ReverseProxy{Director: director}

[gopherbot]

Change https://golang.org/cl/141718 mentions this issue: godoc/proxy: workaround for infinite redirect on ReverseProxy

[bradfitz]

It's preferring the req.Host first per the docs on net/http.Request.Host:

        // For client requests Host optionally overrides the Host
        // header to send. If empty, the Request.Write method uses
        // the value of URL.Host. Host may contain an international
        // domain name.
        Host string

I worry it might not be safe to change this without breaking other people. (There's the usual forward-proxy vs reverse-proxy distinction that people often bring up, too, which is kinda related here.) But if we have different behavior already whether we use HTTP/1 vs HTTP/2 to the backend, then I could see changing it.

[broady]

Hm, would it be safe to set (or unset) Request.Host in the default director?

To note....

The workaround I tried for godoc actually didn't work in production (but worked locally). It 502s and logs this:

httputil: ReverseProxy read error during body copy: stream error: stream ID 5; PROTOCOL_ERROR

[bradfitz]

Well a PROTOCOL_ERROR is very interesting too.

If we have a nice way to capture stderr on one of those, GODEBUG=http2debug=2 would be very interesting. (but send it to me privately in case there are secrets: likely)

[gopherbot]

Change https://golang.org/cl/153858 mentions this issue: [release-branch.go1.11] godoc/proxy: remove use of httputil.ReverseProxy for /share