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 should remove Alt-Svc header #30359

Open
evanj opened this Issue Feb 22, 2019 · 6 comments

Comments

Projects
None yet
4 participants
@evanj
Copy link
Contributor

commented Feb 22, 2019

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:

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

  • Issue #18341 is discussing supporting HTTP Status 421 Misdirected Request. Issue #25804 is a timed out issue asking for http2 to support handling Alt-Svc. Both of these would allow ReverseProxy to handle this header internally, and are not related to removing it here.

  • Caddy had an issue about this, and decided to strip it: mholt/caddy#1051

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

CC @bradfitz for net/http/httputil.

@bcmills bcmills added this to the Go1.13 milestone Feb 25, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

More docs are always good.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.