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: document that ReverseProxy doesn't remove Alt-Svc header #30359

Open
evanj opened this issue Feb 22, 2019 · 7 comments
Open

net/http/httputil: document that ReverseProxy doesn't remove Alt-Svc header #30359

evanj opened this issue Feb 22, 2019 · 7 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@evanj
Copy link
Contributor

evanj commented Feb 22, 2019 •
edited

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

go version devel +27b9571de8 Fri Feb 22 19:30:43 2019 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/ej/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/ej/gopath"
GOPROXY=""
GORACE=""
GOROOT="/Users/ej/go"
GOTMPDIR=""
GOTOOLDIR="/Users/ej/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/xh/6w5vj0s12y97yt81179m2ddm0000gq/T/go-build981855942=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

When using httputil.ReverseProxy, the backend's Alt-Svc header is passed through to the original client. I do not believe it should be. Passing this header back to the original client can cause the client to connect to some OTHER server, and not the reverse proxy on a future request. This is generally not the purpose.

The httputil.ReverseProxy attempts to remove all hop-by-hop headers. I think Alt-Svc header should be treated as a hop-by-hop header. From RFC7838 Section 4: "The ALTSVC frame is processed hop-by-hop". Note this is about the HTTP/2 ALTSVC frame, and not the HTTP/1.1 Alt-Svc header. However, I believe it should be applied there as well.

Fixing this requires a one line addition to the hopHeaders slice:

go/src/net/http/httputil/reverseproxy.go

Line 145 in a14ed2a

// Hop-by-hop headers. These are removed when sent to the backend.

I'm happy to submit this change and a test if this is a good idea.

As an example, run this example proxy:

package main

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

func main() {
	addr := flag.String("addr", "[::1]:8080", "listen addr")
	target := flag.String("target", "https://www.google.com/", "target backend")
	flag.Parse()

	parsedTarget, err := url.Parse(*target)
	if err != nil {
		panic(err)
	}

	// start a proxy that replaces everything before the path
	proxy := &httputil.ReverseProxy{
		Director: func(r *http.Request) {
			r.Host = ""
			r.URL.Scheme = parsedTarget.Scheme
			r.URL.Host = parsedTarget.Host
		},
	}

	log.Printf("listening on %s; proxying to %s\n", *addr, *target)
	err = http.ListenAndServe(*addr, proxy)
	if err != nil {
		panic(err)
	}
}

Then run curl --verbose http://localhost:8080/ to make a request.

What did you expect to see?

I would expect NOT to see the Alt-Svc header returned by the response.

What did you see instead?

The original Alt-Svc header from Google's servers:

< HTTP/1.1 200 OK
< Alt-Svc: quic=":443"; ma=2592000; v="44,43,39"
...

Related things
@fraenkel
Copy link
Contributor

See https://lists.w3.org/Archives/Public/ietf-http-wg/2015JulSep/0369.html which also raised your concern but determined that its end-to-end for HTTP.

@evanj
Copy link
Contributor Author

evanj commented Feb 23, 2019

Summary: I think this is the wrong default when using ReverseProxy to write web applications (see below). However, it also seems reasonable that the Go standard library should try to match the HTTP standards. In particular, users can work around this by providing their own ReverseProxy. ModifyResponse function. Maybe a good solution is to document this in the godoc for this type, since this may not be obvious to users? Again, I'd be happy to submit a patch to do that, if it makes sense.

Details: For the context of future readers who like me may not be HTTP standards experts, that mailing list post says "Discussed in Prague; end-to-end header is useful in the OppSec
use case, and it doesn't need to be consistent with the frame." It appears that "OppSec" refers to Opportunistic Security: https://tools.ietf.org/html/rfc8164

This seems like a reasonable goal, but in the case of a reverse proxy, I'd argue this is the wrong default. A reverse proxy is intended to hide the real backend server(s) it communicates with. For example, I ran into this because part of my application's path space is implemented by some other service, hosted by a major cloud provider. I ended up noticing we were getting QUIC connections coming in, even though our service does not support QUIC, while I was debugging something. I believe the cause is this Alt-Svc advertisement, which is incorrect.

It seems to me that explicitly dropping this header is the default that most users probably expect, even if that technically violates the specification.

@bcmills
Copy link
Contributor

bcmills commented Feb 25, 2019

CC @bradfitz for net/http/httputil.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 25, 2019
@bcmills bcmills added this to the Go1.13 milestone Feb 25, 2019
@bradfitz
Copy link
Contributor

If some people want it and some people don't, removing it unconditionally seems wrong. I think it's probably best to leave it for the ModifyResponse hook to clean up.

@evanj
Copy link
Contributor Author

evanj commented Feb 26, 2019

How about I add these sentences to the ReverseProxy documentation?

ReverseProxy removes HTTP headers that apply "hop-by-hop" (see RFC7230 Section 6.1 and RFC 2616 Section 13.5.1). Depending on how ReverseProxy is used, other headers may need to be altered or removed (e.g. Alt-Svc, Cookies, Host) using the Director and ModifyRequest functions.

Direct links: RFC7230 Section 6.1 ; RFC2616 Section 13.5.1

We could also just close this. This is a very obscure issue, but after wasting so much of my time learning about it, I'd like to make it easier for the next person who is surprised by this.

@bradfitz
Copy link
Contributor

More docs are always good.

Want to send that? Except I'd omit the reference to RFC 2616.

@bradfitz bradfitz changed the title net/http/httputil: ReverseProxy should remove Alt-Svc header net/http/httputil: document that ReverseProxy doesn't remove Alt-Svc header Apr 29, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@odeke-em
Copy link
Member

Hello @evanj, might you have some bandwidth to send this change as it has been a couple of months? If unavailable, not a problem, I can send the CL. Thank you also for reporting this and for advocating the docs update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@bradfitz @rsc @andybons @evanj @fraenkel @odeke-em @bcmills