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.ServeHTTP should empty the outgoing request's Host once the outgoing request is cloned #33861

Open
aofei opened this issue Aug 27, 2019 · 6 comments

Comments

@aofei
Copy link
Contributor

@aofei aofei commented Aug 27, 2019

So I ran the following program today:

package main

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

func main() {
	u, err := url.Parse("https://storage.googleapis.com")
	if err != nil {
		log.Fatal(err)
	}

	log.Fatal(http.ListenAndServe("localhost:8080", httputil.NewSingleHostReverseProxy(u)))
}

Its purpose is to reverse proxy an endpoint of Google Cloud Storage. I think it's supposed to be the simplest way for doing this in Go. But, it failed.

GCP's response
aofei@as-macbook-pro:~$ curl -i http://localhost:8080                                                                                                                                                                                        
HTTP/1.1 500 Internal Server Error
Alt-Svc: quic=":443"; ma=2592000; v="46,43,39"
Cache-Control: private, max-age=0
Content-Length: 308
Content-Type: application/xml; charset=UTF-8
Date: Tue, 27 Aug 2019 11:40:22 GMT
Expires: Tue, 27 Aug 2019 11:40:22 GMT
Server: UploadServer
X-Guploader-Uploadid: AEnB2UpBoX19Ndn4aoWbwiBv2AO6w3MlSktmRcWxWjbJ_aOBJUGUXV8-D16s1mQXm9EJ0OOFoI8sAaqa49x7caNVQmkjdiHw7PGaoxnVjr9et6IXAbJ6rk4
InternalErrorWe encountered an internal error. Please try again.
AE9s14G5pU5PAANPoBlZiES6sh4FZza50l6+RxIgxVQZsDkcNhSA7imgOikw/LgAvi768P+dCA1bd3NTOeLUngmo2GcR5gp/7P2vf3qUUUyqIVavJImCYvdYjnfIRDMEBHtT5iscEd/P

And I found the reason why the above program failed. Currently, httputil.ReverseProxy.ServeHTTP clones a request before it is forwarded. This causes the Host field of the outgoing request to be equivalent to the client request instead of the one in the target URL.

According to the documentation of http.Request.Host:

// For server requests, Host specifies the host on which the URL
// is sought. Per RFC 7230, section 5.4, this is either the value
// of the "Host" header or the host name given in the URL itself.
// It may be of the form "host:port". For international domain
// names, Host may be in Punycode or Unicode form. Use
// golang.org/x/net/idna to convert it to either format if
// needed.
// To prevent DNS rebinding attacks, server Handlers should
// validate that the Host header has a value for which the
// Handler considers itself authoritative. The included
// ServeMux supports patterns registered to particular host
// names and thus protects its registered Handlers.
//
// 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.

the Host header in the outgoing request we actually send will be the same as the incoming request sent from the client.

Usually, this doesn't seem to have any serious impact. In fact, I have never seen one before I wrote the above program. For the above example, the Host header is "127.0.0.1:8080" instead of "storage.googleapis.com".

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 27, 2019

Change https://golang.org/cl/191937 mentions this issue: net/http/httputil: empty outgoing request's Host once it is cloned by ReverseProxy.ServeHTTP

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 27, 2019

I replied on the CL:

I'm afraid this would break more people than it'd fix.

I'm not sure we can make a change like this at this point for compatibility reasons.

You can also do this in your Director func.

Perhaps we can have some new Director/ReverseProxy constructors instead. Everybody keeps trying to shove their preferred behavior into NewSingleHostReverseProxy but that probably can't accommodate everybody.

@aofei
Copy link
Contributor Author

@aofei aofei commented Aug 27, 2019

My current workaround is to empty it in Director func. But I think the current way is a wrong implementation of the reverse proxy. Don't we need to correct it?

If I understand the reverse proxy technique correctly, the incoming request's Host header should be placed in the X-Forwarded-Host header or the Forwarded header of the outgoing request. And the outgoing request's Host header should be the hostname of its destination. For now, in the example above, GCS will receive a request from me with a Host: 127.0.0.1:8080 header. That looks weird.

However, I can't estimate how many people's code will be broken by golang.org/cl/191937. I just think this is a mistake that needs to be corrected.

@aofei
Copy link
Contributor Author

@aofei aofei commented Aug 27, 2019

Also found #14413.

@DisposaBoy
Copy link

@DisposaBoy DisposaBoy commented Aug 27, 2019

However, I can't estimate how many people's code will be broken by golang.org/cl/191937. I just think this is a mistake that needs to be corrected.

I remember coming across this issue probably more than 6 years ago, and I fixing it with my own Director, I quickly found out that not everyone has the same idea about what a reverse proxy is supposed to be. IIRC the broken apps were written in other languages, but it doesn't matter; https://golang.org/doc/go1compat exists for a reason.

@aofei
Copy link
Contributor Author

@aofei aofei commented Aug 27, 2019

I haven't thought of any scenarios that might cause the code to be broken by that change. As @DisposaBoy said, if most people don't know what the outgoing request's Host field should be, what do they do by relying on it?

In fact, this reminds me of the same errors I encountered in my previous programs, but I didn't investigate the reasons at the time. My usual solution is to use alternatives like Nginx or Envoy, both of which work as expected.

https://golang.org/doc/go1compat#expectations

  • Bugs. If a compiler or library has a bug that violates the specification, a program that depends on the buggy behavior may break if the bug is fixed. We reserve the right to fix such bugs.

So what we should do now is to do nothing, is it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.