Gorouter no longer preserves X-Forwarded-Host for Expect: 100-continue requests

[maxmoehl]

Current behavior

When disabling the keep-alive for Expect: 100-continue requests, the X-Forwarded-Host present in the request is overwritten with the value of the Host header.

Desired behavior

Gorouter does not remove / modify the X-Forwarded-Host header.

Affected Version

0.298.0

[maxmoehl]

This is caused by the switch from httputil.ReverseProxy.Director to httputil.ReverseProxy.Rewrite. Rewrite deletes the X-Forwarded-* headers and they are re-set using the SetXForwarded method, however, that does set its own values instead of the ones from the incoming request. We can probably manually copy those headers if they are present.

ping: @geofffranks @mariash

[maxmoehl]

We can fix this rather easily as we already copy most of the forwarded headers:

diff --git a/proxy/proxy.go b/proxy/proxy.go
index e17444df..9f7492f6 100644
--- a/proxy/proxy.go
+++ b/proxy/proxy.go
@@ -314,6 +314,7 @@ func (p *proxy) setupProxyRequestClose100Continue(target *httputil.ProxyRequest)
 	target.Out.Header["Forwarded"] = target.In.Header["Forwarded"]
 	target.SetXForwarded()
 	target.Out.Header["X-Forwarded-Proto"] = target.In.Header["X-Forwarded-Proto"]
+	target.Out.Header["X-Forwarded-Host"] = target.In.Header["X-Forwarded-Host"]
 }
 
 func setRequestXRequestStart(request *http.Request) {

Why do we even bother to call target.SetXForwarded() if we set almost all headers on our own? Also copying the headers without checking whether they have a value causes some of the headers set by that function to be removed again.

I'll go ahead and create a PR for my change.

[geofffranks]

Why do we even bother to call target.SetXForwarded() if we set almost all headers on our own?

I believe we did this because of the example in the go docs for Rewrite, but then @mariash needed to fix a number of things to make the tests pass. Seems like this was a case that we just didn't have testing on initially. :(

If we don't need the SetXForwarded() call and doing everthing ourselves explicitly makes more sense, I'm cool with that.

[maxmoehl]

Okay, I'll take another look at what that function does and whether we need it.

[maxmoehl]

The function sets the X-Forwarded-Header, we want that so we should keep the call in.