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: go1.24 breaks compatibility by modifying in-place the tls.Config{NextProtos} #72100

Closed
dethi opened this issue Mar 4, 2025 · 7 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. 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.
Milestone

Comments

@dethi
Copy link

dethi commented Mar 4, 2025

Go version

go version go1.24.0 darwin/arm64

Output of go env in your module/workspace:

AR='ar'
CC='cc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='c++'
GCCGO='gccgo'
GO111MODULE=''
GOARCH='arm64'
GOARM64='v8.0'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/Users/thibault/Library/Caches/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/Users/thibault/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/c5/msv003gn2s9dqdr8twx25br80000gq/T/go-build3704636302=/tmp/go-build -gno-record-gcc-switches -fno-common'
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMOD='/Users/thibault/go/src/repo/go.mod'
GOMODCACHE='/Users/thibault/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/thibault/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.24.0/libexec'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/thibault/Library/Application Support/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.24.0/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.24.0'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

Setup:

$ openssl req -new -subj "/C=US/ST=Utah/CN=localhost" -newkey rsa:2048 -nodes -keyout localhost.key -out localhost.csr
$ openssl x509 -req -days 365 -in localhost.csr -signkey localhost.key -out localhost.crt

Run the following program locally: https://go.dev/play/p/sqxPDLLHzwX

Simulate a client connecting on the gRPC port with TLS1.3, ALPN H2:

$ openssl s_client -connect localhost:8446 -tls1_3 -alpn h2

Observe the following error:

Connecting to 127.0.0.1
CONNECTED(00000005)
40C8F9F701000000:error:0A000460:SSL routines:ssl3_read_bytes:tlsv1 alert no application protocol:ssl/record/rec_layer_s3.c:908:SSL alert number 120

Repeat the same test using Go 1.23 or older, TLS handshake will happen successfully.

What did you see happen?

net/http server added Protocols to control the allowed protocols. The server modifies the TLS Config NextProtos based on this field (func adjustNextProtos).

This is a different behaviour than before and cause issue when two servers (in my example net/http and gRPC server) utilise the same TLS Config. The TLS Config is soft-cloned in both server, but net/http use slices.DeleteFunc which delete in-place values from the slice, thus reflecting the same change on the gRPC server.

In the example I shared, h2 is disabled on the net/http server (using the old way), and so adjustNextProtos removes it from the TLS Config NextProtos. On the other side, gRPC server append h2 if it's not already present. Because the TLS Config is a shallow clone, the outcome is racy.

What did you expect to see?

The expectation is that the gRPC server has h2 set in the NextProtos and so that the ALPN negotiation works correctly. This was the case up until Go 1.24.

@dethi dethi changed the title net/http: go1.24 breaks compatibility by modifying in-plane the tls.Config{NextProtos} net/http: go1.24 breaks compatibility by modifying in-place the tls.Config{NextProtos} Mar 4, 2025
@dethi
Copy link
Author

dethi commented Mar 4, 2025

The problem can also be summarised by this tiny example: https://go.dev/play/p/eg_dXkGompq

@ianlancetaylor
Copy link
Member

CC @neild

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Mar 4, 2025
@neild
Copy link
Contributor

neild commented Mar 4, 2025

Thanks. I agree that this is a regression.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/654875 mentions this issue: net/http: don't modify caller's tls.Config.NextProtos

@neild
Copy link
Contributor

neild commented Mar 4, 2025

@gopherbot please backport to go 1.24, this is a regression

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #72103 (for 1.24).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@dmitshur dmitshur added this to the Go1.25 milestone Mar 5, 2025
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. labels Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. 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.
Projects
None yet
Development

No branches or pull requests

6 participants