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: NewSingleHostReverseProxy - omit X-Forwarded-For not working #53423

Open
firefart opened this issue Jun 17, 2022 · 7 comments
Open
Labels
NeedsFix
Milestone

Comments

@firefart
Copy link

@firefart firefart commented Jun 17, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/firefart/.cache/go-build"
GOENV="/home/firefart/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/firefart/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/firefart/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/firefart/code/zwiebelproxy/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build534953705=/tmp/go-build -gno-record-gcc-switches"

What did you do?

In #38079 a check was added to not leak the IP if the X-Forwarded-For header is nil.

Doing this does not seem to work as it always return an empty string slice

r.Header["X-Forwarded-For"] = nil
fmt.Printf("%T", r.Header["X-Forwarded-For"])

Output:

[]string

What did you expect to see?

The header is not added

What did you see instead?

The IP header is always added as it seems impossible to set the value to nil

omit := ok && prior == nil // Issue 38079: nil now means don't populate the header

@firefart firefart changed the title net/http/httputil: omit X-Forwarded-For not working net/http/httputil: NewSingleHostReverseProxy - omit X-Forwarded-For not working Jun 17, 2022
@firefart
Copy link
Author

@firefart firefart commented Jun 17, 2022

The actual issue lies in the Header.Clone method.

ServeHTTP calls req.Clone(ctx) over here:

outreq := req.Clone(ctx)

Clone then calls the Clone method on the Headers map:

r2.Header = r.Header.Clone()

I added a debug output before and after the r.Header.Clone() call and this is the output:

http.Header{"X-Forwarded-For":[]string(nil)}
-- Clone()
http.Header{"X-Forwarded-For":[]string{}}

So it looks like the Header.Clone() method does not preserve nil values in headers which makes the check for nil invalid and thus the client ip is always added.

This can be a privacy issue because currently golang is leaking the original client ip to the end targets even if the header is set to nil to prevent that

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Jun 17, 2022

Seems to be working? https://go.dev/play/p/ME0dFW6gT1g

@seankhliao seankhliao added the WaitingForInfo label Jun 17, 2022
@firefart
Copy link
Author

@firefart firefart commented Jun 17, 2022

Nope. Here is a short play to demonstrate the underlying issue:
https://go.dev/play/p/c8Oc19Te76u

http.Header{"X-Forwarded-For":[]string(nil)}
http.Header{"X-Forwarded-For":[]string{}}

The Clone converts the nil value to an empty slice

Here is also some pseudocode for an http handler that forwards the request using httputil.NewSingleHostReverseProxy.

func (app *application) proxyHandler(w http.ResponseWriter, r *http.Request) {
	host, port, err := net.SplitHostPort(r.Host)
	if err != nil {
		// no port present
		host = r.Host
		port = r.URL.Port()
	}

	ctx, cancel := context.WithTimeout(r.Context(), app.timeout)
	defer cancel()
	r = r.WithContext(ctx)

	if port != "" && port != "80" && port != "443" {
		host = net.JoinHostPort(host, port)
	}
	r.URL.Host = host
	r.Host = host

	if r.URL.Scheme == "" {
		switch port {
		case "":
			r.URL.Scheme = "http"
		case "80":
			r.URL.Scheme = "http"
		case "443":
			r.URL.Scheme = "https"
		default:
			r.URL.Scheme = "http"
		}
	}

	// needed so the ip will not be leaked
	r.Header["X-Forwarded-For"] = nil

	app.logger.Debugf("port: %+v", port)
	app.logger.Debugf("r.URL: %+v", r.URL)
	app.logger.Debugf("r.RequestURI: %+v", r.RequestURI)
	app.logger.Debugf("r.Host: %+v", r.Host)
	app.logger.Debugf("r.Header: %+v", r.Header)

	proxy := httputil.NewSingleHostReverseProxy(r.URL)
	proxy.FlushInterval = -1
	proxy.ModifyResponse = app.modifyResponse
	proxy.Transport = app.httpClient.tr.Clone()

	app.logger.Debugf("sending request %+v", r)

	proxy.ServeHTTP(w, r)
}

If you add the debug statements on the clone method mentioned in the comment, you can see that the header value is not nil after cloning

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Jun 17, 2022

I understood #38079 to mean allowing control over X-Forwarded-For from within the proxy, ie using your own Director, not from the outside.

cc @neild

@firefart
Copy link
Author

@firefart firefart commented Jun 17, 2022

Maybe, but the comment does not say this exactly, it only mentions the header value needs to be nil

// If an X-Forwarded-For header already exists, the client IP is
// appended to the existing values. As a special case, if the header
// exists in the Request.Header map but has a nil value (such as when
// set by the Director func), the X-Forwarded-For header is
// not modified.

I also think the Clone() function not handling nil values is a bit inconsistent as it modifies the original request.

@neild
Copy link
Contributor

@neild neild commented Jun 17, 2022

Given that ReverseProxy assigns meaning to nil header values, Header.Clone should probably preserve nil-ness.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 17, 2022

Change https://go.dev/cl/412857 mentions this issue: net/http: preserve nil values in Header.Clone

@seankhliao seankhliao added NeedsFix and removed WaitingForInfo labels Jun 18, 2022
@seankhliao seankhliao added this to the Go1.20 milestone Jun 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

4 participants