x/net/quic: tests broken by d0edd9acc8 crypto/tls: implement X25519Kyber768Draft00 at go directive 1.23+

[stapelberg]

Go version

go version devel go1.23-d0edd9acc8 Wed May 22 14:56:25 2024 +0000 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/usr/local/google/home/stapelberg/.cache/go-build'
GOENV='/usr/local/google/home/stapelberg/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/usr/local/google/home/stapelberg/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/usr/local/google/home/stapelberg/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/google/home/stapelberg/upstream-go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/google/home/stapelberg/upstream-go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.23-d0edd9acc8 Wed May 22 14:56:25 2024 +0000'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/usr/local/google/home/stapelberg/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/usr/local/google/home/stapelberg/net/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1427484525=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Ran the net/quic package tests

What did you see happen?

Tests should have passed.

What did you expect to see?

Tests no longer pass.

Note that you need to run go mod edit -go 1.23 in the net repository first, otherwise the “go 1.18” line results in not triggering the bug.

Before the culprit commit:

upstream-go/src % git reset --hard d0edd9acc80a16ca1dd9c6f9045fdd34efabcd42^
HEAD is now at 7c52c064df cmd/compile: update PGO profile

upstream-go/src % ./make.bash 
Building Go cmd/dist using /usr/lib/google-golang. (go1.23-20240419-RC02 cl/626470163 +7f76c00fc5 X:fieldtrack,boringcrypto linux/amd64)
Building Go toolchain1 using /usr/lib/google-golang.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for linux/amd64.
---
Installed Go for linux/amd64 in /usr/local/google/home/stapelberg/upstream-go
Installed commands in /usr/local/google/home/stapelberg/upstream-go/bin
*** You need to add /usr/local/google/home/stapelberg/upstream-go/bin to your PATH.

net/quic % ~/upstream-go/bin/go test ./...
ok  	golang.org/x/net/quic	0.520s
ok  	golang.org/x/net/quic/qlog	0.007s

But at the culprit commit:

go/src % git reset --hard d0edd9acc80a16ca1dd9c6f9045fdd34efabcd42 
HEAD is now at d0edd9acc8 crypto/tls: implement X25519Kyber768Draft00

go/src % ./make.bash                                  
Building Go cmd/dist using /usr/lib/google-golang. (go1.23-20240419-RC02 cl/626470163 +7f76c00fc5 X:fieldtrack,boringcrypto linux/amd64)
Building Go toolchain1 using /usr/lib/google-golang.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for linux/amd64.
---
Installed Go for linux/amd64 in /usr/local/google/home/stapelberg/upstream-go
Installed commands in /usr/local/google/home/stapelberg/upstream-go/bin
*** You need to add /usr/local/google/home/stapelberg/upstream-go/bin to your PATH.

net/quic % go mod edit -go 1.23
net/quic % ~/upstream-go/bin/go test ./...
--- FAIL: TestConfigTransportParameters (0.00s)
    config_test.go:24: dgram 1:
        got datagram with 1 packets:
          Initial 0 version=1 src={c0ffee0000000000} dst={c0ffee00000000ff}
            CRYPTO Offset=0 Length=1153
        
        want datagram with 1 packets (padded to 1200 bytes):
          Initial 0 version=1 src={c0ffee0000000000} dst={c0ffee00000000ff}
            CRYPTO Offset=0 Length=1432
--- FAIL: TestConnCloseResponseBackoff (0.00s)
    conn_close_test.go:21: dgram 1:
        got datagram with 1 packets:
          Initial 0 version=1 src={c0ffee0000000000} dst={c0ffee00000000ff}
            CRYPTO Offset=0 Length=1153
        
        want datagram with 1 packets (padded to 1200 bytes):
          Initial 0 version=1 src={c0ffee0000000000} dst={c0ffee00000000ff}
            CRYPTO Offset=0 Length=1446
--- FAIL: TestConnCloseWithPeerResponse (0.00s)
    conn_close_test.go:75: dgram 1:
        got datagram with 1 packets:
          Initial 0 version=1 src={c0ffee0000000000} dst={c0ffee00000000ff}
            CRYPTO Offset=0 Length=1153
        
        want datagram with 1 packets (padded to 1200 bytes):
          Initial 0 version=1 src={c0ffee0000000000} dst={c0ffee00000000ff}
            CRYPTO Offset=0 Length=1446

[…] elided to stay under GitHub issue length limit […]

FAIL	golang.org/x/net/quic	0.202s
ok  	golang.org/x/net/quic/qlog	0.007s
FAIL

[stapelberg]

cc @neild @rolandshoemaker @dmitshur @FiloSottile

[neild]

The root cause is the tests assume that each flight in the TLS handshake using the test certificates will fit into a single QUIC datagram (1200 bytes, less overhead), and the client hello is now spilling over into a second datagram.

I can fix the assumption in the tests, but if there's some option I can set to get the handshake size down that'll be much simpler.

[FiloSottile]

I can fix the assumption in the tests, but if there's some option I can set to get the handshake size down that'll be much simpler.

You can set Config.CurvePreferences and that will disable the large Kyber share. We do that for the crypto/tls tests.

Let's make sure though we do have a test for Client Hellos that span datagrams, that's a common implementation issue (https://tldr.fail/).

Also, how can we make sure that test failures in x/ repos triggered by the go.mod version are caught in CI? /cc @golang/release

[gopherbot]

Change https://go.dev/cl/589855 mentions this issue: quic: disable X25519Kyber768Draft00 in tests

[neild]

I'm pretty sure I've already got a test exercising the case where the TLS flights span multiple datagrams, but I'm adding one that uses the default key exchange preferences as well.

Unfortunate that the client hello is no longer a single datagram. That was elegant.

[FiloSottile]

Unfortunate that the client hello is no longer a single datagram. That was elegant.

We're all pretty sad about the size of PQC keys, indeed.

[dmitshur]

Also, how can we make sure that test failures in x/ repos triggered by the go.mod version are caught in CI?

Filed #67791 for that.