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.

Sign up for GitHub

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

Jump to bottom

crypto/tls: TestVersion incorrectly tests "tls10default" #51709

Closed
stevenjohnstone opened this issue Mar 16, 2022 · 3 comments
Closed

crypto/tls: TestVersion incorrectly tests "tls10default" #51709

stevenjohnstone opened this issue Mar 16, 2022 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@stevenjohnstone
Copy link
Contributor

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

$ go version
go version devel go1.19-5fd0ed7aaf Wed Mar 16 07:03:20 2022 +0000 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/stevie/.cache/go-build"
GOENV="/home/stevie/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/stevie/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/stevie/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/stevie/golang"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/stevie/golang/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.19-5fd0ed7aaf Wed Mar 16 07:03:20 2022 +0000"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/stevie/golang/src/go.mod"
GOWORK=""
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-build478804125=/tmp/go-build -gno-record-gcc-switches"

What did you do?

go/src/crypto/tls/handshake_server_test.go

Line 410 in 5fd0ed7

if state.Version != VersionTLS11 {
"state.Version" is checked but "state" captures the state of the TLS connection immediately after the first handshake in the test, not the last. This means the final check of the TLS version used will always pass even if we break "tls10default" environment variable support.

What did you expect to see?

Something like this:

diff --git a/src/crypto/tls/handshake_server_test.go b/src/crypto/tls/handshake_server_test.go
index 6d2c405626..df7094cef9 100644
--- a/src/crypto/tls/handshake_server_test.go
+++ b/src/crypto/tls/handshake_server_test.go
@@ -403,7 +403,7 @@ func TestVersion(t *testing.T) {

        defer func(old bool) { debugEnableTLS10 = old }(debugEnableTLS10)
        debugEnableTLS10 = true
-       _, _, err = testHandshake(t, clientConfig, serverConfig)
+       state, _, err = testHandshake(t, clientConfig, serverConfig)
        if err != nil {
                t.Fatalf("handshake failed: %s", err)
        }

What did you see instead?

Stale version of a variable is used.
@gopherbot
Copy link

Change https://go.dev/cl/393334 mentions this issue: crypto/tls: fix TestVersion

@heschi
Copy link
Contributor

heschi commented Mar 16, 2022

cc @FiloSottile @agl @rolandshoemaker

@heschi heschi added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 16, 2022
@heschi heschi added this to the Backlog milestone Mar 16, 2022
@stevenjohnstone
Copy link
Contributor Author

f0ee7fd removes the code in question.

@golang golang locked and limited conversation to collaborators May 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@stevenjohnstone @gopherbot @heschi