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

x/net/http2/h2c: H2C handler uses incorrect headers to detect upgrade #45785

Open
Gerg opened this issue Apr 26, 2021 · 7 comments
Open

x/net/http2/h2c: H2C handler uses incorrect headers to detect upgrade #45785

Gerg opened this issue Apr 26, 2021 · 7 comments
Labels
NeedsInvestigation
Milestone

Comments

@Gerg
Copy link

@Gerg Gerg commented Apr 26, 2021

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

$ go version
go version go1.16.2 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/pivotal/.cache/go-build"
GOENV="/home/pivotal/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/pivotal/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/pivotal/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.16.2"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build592242489=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Running a simple h2c server:

package main

import (
        "fmt"
        "net/http"

        "golang.org/x/net/http2"
        "golang.org/x/net/http2/h2c"
)

func main() {
        h2s := &http2.Server{}

        handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
                fmt.Fprintf(w, "Hello, %v; HTTP Version: %v", r.URL.Path, r.Proto)
        })

        server := &http.Server{
                Addr:    "0.0.0.0:8080",
                Handler: h2c.NewHandler(handler, h2s),
        }

        err := server.ListenAndServe()
        if err != nil {
                panic(err)
        }
}

What did you expect to see?

Based on my reading of RFC 7540, Section 3.2, the required headers for an h2c upgrade are Upgrade: h2c and HTTP2-Settings:

The client does so by making an HTTP/1.1 request that includes an Upgrade header field with the "h2c" token. Such an HTTP/1.1 request MUST include exactly one HTTP2-Settings (Section 3.2.1) header field.

Curling with those headers, I would expect to receive a HTTP/1.1 101 Switching Protocols response.

What did you see instead?

The h2c handler does not accept the upgrade:

$ curl localhost:8080 -H "Upgrade: h2c" -H "HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA" -vvv
*   Trying 127.0.0.1:8080...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.68.0
> Accept: */*
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Date: Mon, 26 Apr 2021 19:09:10 GMT
< Content-Length: 32
< Content-Type: text/plain; charset=utf-8
<
* Connection #0 to host localhost left intact
Hello, /; HTTP Version: HTTP/1.1

This is because the h2c package is not checking the appropriate headers:

// isH2CUpgrade returns true if the header properly request an upgrade to h2c
// as specified by Section 3.2.
func isH2CUpgrade(h http.Header) bool {
	return httpguts.HeaderValuesContainsToken(h[textproto.CanonicalMIMEHeaderKey("Upgrade")], "h2c") &&
		httpguts.HeaderValuesContainsToken(h[textproto.CanonicalMIMEHeaderKey("Connection")], "HTTP2-Settings")
}

Source: https://github.com/golang/net/blob/5f58ad60dda6b6eba34c424201d17c9fdc37953d/http2/h2c/h2c.go#L373-L378

While the Connection: HTTP2-Settings header does appear in the example in Section 3.2, I can't find anything in the spec that says it is required or that it should be used to detect h2c upgrades.

After adding the Connection: HTTP2-Settings header, the h2c upgrade succeeds:

$ curl localhost:8080 -H "Upgrade: h2c" -H "HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA" -H "Connection: HTTP2-Settings" -vvv
*   Trying 127.0.0.1:8080...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.68.0
> Accept: */*
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
> Connection: HTTP2-Settings
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
< Upgrade: h2c

This is causing problems because we are running our H2C apps behind a httputil.ReverseProxy, which removes Connection headers, thereby blocking the h2c upgrade.

@gopherbot gopherbot added this to the Unreleased milestone Apr 26, 2021
@cherrymui cherrymui added the NeedsInvestigation label Apr 26, 2021
@cherrymui
Copy link
Member

@cherrymui cherrymui commented Apr 26, 2021

cc @bradfitz @tombergan

@Gerg
Copy link
Author

@Gerg Gerg commented Apr 26, 2021

My colleague, @ameowlia, pointed out that Section 3.2.1 of the spec does say that Connection: HTTP2-Settings should be sent by the client:

Since the upgrade is only intended to apply to the immediate connection, a client sending the HTTP2-Settings header field MUST also send "HTTP2-Settings" as a connection option in the Connection header field to prevent it from being forwarded (see Section 6.1 of [RFC7230]).

Still, this feels like an odd choice for detecting h2c upgrades, especially in lieu of checking for the HTTP2-Settings header itself. Furthermore, this check isn't compatible with Golang's ReverseProxy implementation, as I described above.

For comparison, I testing the same request against a simple Python server that supports h2c:

#!/usr/bin/env python3

from quart import Quart, request

app = Quart(__name__)


@app.route("/")
async def hello():
    return f"Hello, {request.full_path}; HTTP Version: {request.http_version}."


if __name__ == "__main__":
    app.run(port=8081)

It successfully upgraded without the Connection: HTTP2-Settings header:

$ curl localhost:8081 -H "Upgrade: h2c" -H "HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA" -vvv
*   Trying 127.0.0.1:8081...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8081 (#0)
> GET / HTTP/1.1
> Host: localhost:8081
> User-Agent: curl/7.68.0
> Accept: */*
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 101
< date: Mon, 26 Apr 2021 20:54:51 GMT
< server: hypercorn-h11
< connection: upgrade
< upgrade: h2c

Gerg added a commit to Gerg/envoy_http2_tls_no_alpn that referenced this issue Apr 27, 2021
- Golang h2c package doesn't play well with reverse proxy due to
golang/go#45785
@networkimprov
Copy link

@networkimprov networkimprov commented May 16, 2021

cc @fraenkel

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented May 16, 2021

I don't believe this is an issue given that this is following the spec as identified above.
From https://datatracker.ietf.org/doc/html/rfc7540#section-3.2.1

A request that upgrades from HTTP/1.1 to HTTP/2 MUST include exactly
one "HTTP2-Settings" header field.

 a client sending the HTTP2-Settings header field MUST
   also send "HTTP2-Settings" as a connection option in the Connection
   header field

@Gerg
Copy link
Author

@Gerg Gerg commented May 16, 2021

Based on that, should httputil.ReverseProxy preserve the Connection: HTTP2-Settings header? There is already precedent for preserving headers needed for upgrades:

// After stripping all the hop-by-hop connection headers above, add back any
// necessary for protocol upgrades, such as for websockets.
if reqUpType != "" {
outreq.Header.Set("Connection", "Upgrade")
outreq.Header.Set("Upgrade", reqUpType)
}

Otherwise, it seems impossible to implement the H2C upgrade flow spanning a ReverseProxy.

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented May 16, 2021

@Gerg
Copy link
Author

@Gerg Gerg commented May 17, 2021

I see why it makes sense to strip HTTP2-Settings from the Connection header as a default.

Would you be open to making the preserved headers configurable on ReverseProxy? In our case, we have a trusted client and also don't enforce any access controls via our reverse proxy, so I don't think h2c smuggling is a concern. If not, I suppose we can always shim the ReverseProxy's Transport to add the headers back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

5 participants