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

Jump to bottom

net/http/httputil: ReverseProxy.ServeHTTP should empty the outgoing request's Host once the outgoing request is cloned #33861

Closed
aofei opened this issue Aug 27, 2019 · 8 comments
Closed

net/http/httputil: ReverseProxy.ServeHTTP should empty the outgoing request's Host once the outgoing request is cloned #33861

aofei opened this issue Aug 27, 2019 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@aofei
Copy link
Contributor

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

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

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 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 commented Aug 27, 2019

Also found #14413.

@DisposaBoy
Copy link

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 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?

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 29, 2019
@neild neild mentioned this issue Jun 8, 2022
Closed
@neild
Copy link
Contributor

neild commented Jun 10, 2022

Duplicate of #28168.

@neild neild closed this as completed Jun 10, 2022
@ferdypruis
Copy link

ferdypruis commented Oct 20, 2022 •
edited

Just lost a couple of hours on this "bug" while trying to use it as a forwarding proxy.

@golang golang locked and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@bradfitz @neild @DisposaBoy @ferdypruis @aofei @julieqiu @gopherbot