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: do not enable HTTP/2 extended connect by default #71128

Open
rthellend opened this issue Jan 4, 2025 · 13 comments
Open

net/http: do not enable HTTP/2 extended connect by default #71128

rthellend opened this issue Jan 4, 2025 · 13 comments
Assignees
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@rthellend
Copy link

Go version

go version 1.24rc1

Output of go env in your module/workspace:

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/robin/.cache/go-build'
GODEBUG=''
GOENV='/home/robin/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2734898122=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/robin/src/tlsproxy/go.mod'
GOMODCACHE='/home/robin/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/robin/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/robin/src/goroot'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/robin/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/robin/src/goroot/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.24-705b5a569a Fri Jan 3 14:40:11 2025 -0800'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

This change, which should be autogenerated appears to have picked up an unintended change to src/net/http/h2_bundle.go

https://go-review.googlesource.com/c/go/+/631336

https://go-review.googlesource.com/c/go/+/631336/2/src/net/http/h2_bundle.go

all: update vendored dependencies [generated]

The Go 1.24 code freeze has recently started. This is a time to update
all golang.org/x/... module versions that contribute packages to the
std and cmd modules in the standard library to latest master versions.

For [#36905](https://go.dev/issue/36905).

[git-generate]
go install golang.org/x/build/cmd/updatestd@latest
go install golang.org/x/tools/cmd/bundle@latest
updatestd -goroot=$(pwd) -branch=master

What did you see happen?

The change to h2_bundle.go that enables the Extended Connect Protocol is undocumented.

What did you expect to see?

The change should either be documented or removed.

@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Jan 4, 2025
@seankhliao seankhliao changed the title net/http: undocumented (unintended?) change that enables extended connect protocol net/http: document http2 extended connect support Jan 4, 2025
@seankhliao seankhliao added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Jan 4, 2025
@seankhliao seankhliao added this to the Go1.24 milestone Jan 4, 2025
@seankhliao
Copy link
Member

The change looks intentional (part of #49918).
I think this just needs a release note?

cc @WeidiDeng @neild

@rthellend
Copy link
Author

I doubt this is actually intentional. It will break existing websocket implementations. If anything, the feature should be optional (off by default).

Browsers will start trying to open websockets using h2, and it will fail unless there is code in the server to handle those requests.

@rthellend
Copy link
Author

Here's a boiled down example that demonstrates the problem: https://gist.github.com/rthellend/e77264034eb761ccf32abd274792775a

2025/01/04 16:19:27 HTTP/2.0 GET /
2025/01/04 16:19:27 HTTP/2.0 CONNECT /websocket
2025/01/04 16:19:27 ERR websocket: the client is not using the websocket protocol: 'upgrade' token not found in 'Connection' header

@prattmic
Copy link
Member

prattmic commented Jan 6, 2025

This was added in https://go.dev/cl/610977. I'll leave it to @neild to say whether it is intentional/correct, but I also want to point out that #70728 was filed as a follow-up issue, which I've tentatively made a release blocker as well, and the CL has another comment about a potential deadlock with no issue.

@WeidiDeng
Copy link

WeidiDeng commented Jan 7, 2025

The exact proposal is here. And according to rsc, this should be on by default. To turn this flag off, GODEBUG=http2xconnect=0 is used. As said by @rthellend, it will break existing codes if that env isn't specified because the server advertised it can handle websockets over h2 but the handler still uses h1 upgrade mechanics which isn't compatible with h2 websockets. And some of the most used browsers will try to use websockets over h2 if the server advertises support.

#70728 is when the golang http2 client tries to use extended connect to establish a websocket connection over h2, which can only happen if user tries to use the new websocket upgrade mechanics.

@rthellend
Copy link
Author

It's not clear to me that @rsc understood that this will break all websocket implementations when he said this didn't have to be opt-in.

#53208 (comment)

I don't think it has to be opt-in but it probably needs a GODEBUG to provide a fallback when hitting endpoints or middleware that is buggy when extended CONNECTs are involved. http2xconnect=0 or something like that.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Jan 7, 2025
rthellend added a commit to c2FmZQ/tlsproxy that referenced this issue Jan 8, 2025
### Description

Disable http2 extended connect.

http2 extended connect was added in x/net 0.33, and go1.24rc1. It breaks
websockets.

### Type of change

* [ ] New feature
* [ ] Feature improvement
* [ ] Bug fix
* [ ] Documentation
* [ ] Cleanup / refactoring
* [x] Other (please explain)

### How is this change tested ?

* [ ] Unit tests
* [x] Manual tests (explain)
* [ ] Tests are not needed

### Links to related issues

golang/go#71128
rthellend added a commit to c2FmZQ/tlsproxy that referenced this issue Jan 8, 2025
### Description

Disable http2 extended connect.

http2 extended connect was added in x/net 0.33, and go1.24rc1. It breaks
websockets.

### Type of change

* [ ] New feature
* [ ] Feature improvement
* [ ] Bug fix
* [ ] Documentation
* [ ] Cleanup / refactoring
* [x] Other (please explain)

### How is this change tested ?

* [ ] Unit tests
* [x] Manual tests (explain)
* [ ] Tests are not needed

### Links to related issues

golang/go#71128
@neild
Copy link
Contributor

neild commented Jan 8, 2025

To summarize:

https://go-review.googlesource.com/c/net/+/610977 implements proposal #49918, adding support for HTTP/2 Extended CONNECT (RFC 8441) to x/net/http2.

Extended CONNECT is a protocol extension permitting an HTTP/2 client to negotiate a protocol switch on an HTTP/2 stream. The primary (only?) use of extended CONNECT is to bootstrap the WebSocket protocol.

Extended CONNECT support is negotiated while establishing an HTTP/2 connection. With this change, x/net/http2 servers now advertise support for extended CONNECT. Browsers interpret this as the server indicating support for WebSocket-over-HTTP/2. This causes problems, because WebSocket-over-HTTP/2 requires support from the server's WebSocket layer as well as the HTTP/2 layer.

This indicates that, contrary to our belief, advertising extended CONNECT support is not a backwards-compatible change. We should revert the change to x/net to advertise extended CONNECT support by default, and add a mechanism by which servers can explicitly enable it.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/641475 mentions this issue: http2: disable extended CONNECT by default

gopherbot pushed a commit to golang/net that referenced this issue Jan 8, 2025
Browsers interpret a server advertising extended CONNECT support as
indicating the server supports WebSockets-over-HTTP/2.
However, WebSocket-over-HTTP/2 requires support from both the HTTP
implementation and the WebSocket implementation, and existing
Go WebSocket packages don't support HTTP/2.

Disable extended CONNECT support by default, since advertising it
is a non-backwards-compatible change.

For golang/go#71128
For golang/go#49918

Change-Id: Ie7d3ee2cd48124836a00bad320752e78719ffc46
Reviewed-on: https://go-review.googlesource.com/c/net/+/641475
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@neild

This comment was marked as resolved.

@gopherbot

This comment was marked as resolved.

@neild neild changed the title net/http: document http2 extended connect support net/http: do not enable HTTP/2 extended connect by default Jan 8, 2025
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation Issues describing a change to documentation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 9, 2025
@cdujeu
Copy link

cdujeu commented Jan 13, 2025

Sorry i i'm wrong, I do not fully get the link between go versions and golang.org/x packages versioning.

For those having the issue, this has been introduced in golang.org/x/net v0.32.0 , so for me the http2xconnect flag is required although I'm building with go 22 or 23... Reverting to v0.31.0 fixes that (but brings back a known vulnerability 😭).

Will the backport you are referring to also be part of a golang.org/x/net next version ?

Thanks!

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/642606 mentions this issue: [internal-branch.go1.24-vendor] http2: disable extended CONNECT by default

@dmitshur dmitshur added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

9 participants