Skip to content
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 does not set host header correctly #28168

Closed
broady opened this issue Oct 12, 2018 · 13 comments
Closed

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

broady opened this issue Oct 12, 2018 · 13 comments
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@broady
Copy link
Member

broady commented Oct 12, 2018

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).

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
Copy link

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

@bradfitz
Copy link
Contributor

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
Copy link
Member Author

broady commented Oct 12, 2018

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
Copy link
Contributor

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)

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 23, 2018
@bcmills bcmills added this to the Unplanned milestone Oct 23, 2018
gopherbot pushed a commit to golang/tools that referenced this issue Dec 12, 2018
ReverseProxy doesn't re-set the Request's Host field, only
Request.URL.Host.
The HTTP/2 client prefers Request.Host over Request.URL.Host, so this
results in the request being sent back to the host that originally
accepted the request.
This results in an infinite redirect (and consumption of many connections to
itself).
See Issue golang/go#28168 for details.

Replace it with a simple proxy that drops all the headers (except
Content-Type).

I tried setting the proxy.Director, but it still didn't work. Could do
with some more investigation.

Fixes golang/go#28134.

Change-Id: I5051ce72a379dcacfbe8484f58f8cf7d9385024d
Reviewed-on: https://go-review.googlesource.com/c/141718
Run-TryBot: Chris Broadfoot <cbro@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

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

gopherbot pushed a commit to golang/tools that referenced this issue Dec 12, 2018
…oxy for /share

ReverseProxy doesn't re-set the Request's Host field, only
Request.URL.Host.
The HTTP/2 client prefers Request.Host over Request.URL.Host, so this
results in the request being sent back to the host that originally
accepted the request.
This results in an infinite redirect (and consumption of many connections to
itself).
See Issue golang/go#28168 for details.

Replace it with a simple proxy that drops all the headers (except
Content-Type).

I tried setting the proxy.Director, but it still didn't work. Could do
with some more investigation.

Fixes golang/go#28134.

Change-Id: I5051ce72a379dcacfbe8484f58f8cf7d9385024d
Reviewed-on: https://go-review.googlesource.com/c/141718
Run-TryBot: Chris Broadfoot <cbro@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 837e805)
Reviewed-on: https://go-review.googlesource.com/c/153858
Run-TryBot: Andrew Bonventre <andybons@golang.org>
@bradfitz bradfitz modified the milestones: Unplanned, Go1.13 Dec 14, 2018
@ahmetb
Copy link

ahmetb commented Mar 30, 2019

I also hit this! It applies to both http/1 and http/2 based on my experimentation.

I refactored both Director and Transport (http.RoundTripper) to set the Host header, yet the local hostname of my server leaks to the backend and therefore my program doesn't function correctly.

I basically rewrite the /get request to send to https://httpbin.org/get while setting the Host: httpbin.org and Scheme = "https". The echoed response shows that the Host header remained as localhost:8080.

I also asked this at https://stackoverflow.com/questions/55426924/go-httputil-reverseproxy-not-overriding-the-host-authority-header, my minimal repro below:

package main

import (
    "log"
    "net/http"
    "net/http/httputil"
)

func main() {
    proxy := &httputil.ReverseProxy{
        Transport: roundTripper(rt),
        Director: func(req *http.Request) {
            req.URL.Scheme = "https"
            req.URL.Host = "httpbin.org"
            req.Header.Set("Host", "httpbin.org") // <--- I set it here first
        },
    }
    log.Fatal(http.ListenAndServe(":8080", proxy))
}

func rt(req *http.Request) (*http.Response, error) {
    log.Printf("request received. url=%s", req.URL)
    req.Header.Set("Host", "httpbin.org") // <--- I set it here as well
    defer log.Printf("request complete. url=%s", req.URL)

    return http.DefaultTransport.RoundTrip(req)
}


// roundTripper makes func signature a http.RoundTripper
type roundTripper func(*http.Request) (*http.Response, error)

func (f roundTripper) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) }

@uucckk
Copy link

uucckk commented Apr 25, 2019

Goang's access to nginx through proxy can not correctly orientate the website, is it solved at present?

If it's solved, you can tell me which version, or solve it by other means.thinks.

@ahmetb
Copy link

ahmetb commented Apr 25, 2019

Turns out I needed to set req.Host field instead and that solved it for me above.
https://stackoverflow.com/questions/55426924/go-httputil-reverseproxy-not-overriding-the-host-header

@thediveo
Copy link

thediveo commented Jul 9, 2019

Lost 1h searching for my downstream server returning 403 when going through the httputil.ReverseProxy with the stock Director, while curl worked as expected. Finally, I found this bug report and I can confirm that it is necessary to set req.Host to req.URL.Host for the reverse proxy to work and not cause endless recursion.

At least, the implementation of Director in NewReverseProxyForSingleHost should be fixed and properly documented.

mefellows added a commit to pact-foundation/pact-go that referenced this issue Aug 16, 2019
The local proxy currently assumes that a verification will take place
against either a) a local endpoint or b) an http endpoint. It did not
support external hosts on https.o

It also did not rewrite the Host header correctly (see golang/go#28168)

This change:

1. Rewrites the header during proxying
2. Hard codes the local proxy to run only on http - there is no reason why it should run on https
   even if the endpoint under test _is_.
3. Opens the door for client configured transports during verification,
   to enable self-signed certificates to be easily used
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@stevenh
Copy link
Contributor

stevenh commented Mar 6, 2020

Confirmed this had me digging for some time too.

@docmerlin
Copy link

Welp, we just ran into this.

@domdom82
Copy link

Ran into the same issue in almost 2021 now. Fixed it by manually adding

req.Host = targetURL.Host

in the director function.. but why do I have to do this!?

@gopherbot
Copy link

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

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 11, 2022
@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Aug 16, 2022
@golang golang locked and limited conversation to collaborators Aug 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests