Fix tests, which includes a change to crypto/tls/handshake_client.go ❗
There were two test failures after initially resolving conflicts:
--- FAIL: TestBoringServerCipherSuites (0.01s)
--- FAIL: TestBoringServerCipherSuites/suite=0x544c535f4145535f3132385f47434d5f534841323536 (0.00s)
boring_test.go:179: Got error: local error: tls: missing extension; expected to succeed
--- FAIL: TestBoringServerCipherSuites/suite=0x544c535f4145535f3132385f47434d5f534841323536/fipstls (0.00s)
boring_test.go:187: Got error: local error: tls: missing extension; expected to succeed
--- FAIL: TestBoringServerCipherSuites/suite=0x544c535f43484143484132305f504f4c59313330355f534841323536 (0.00s)
boring_test.go:179: Got error: local error: tls: missing extension; expected to succeed
--- FAIL: TestBoringServerCipherSuites/suite=0x544c535f4145535f3235365f47434d5f534841333834 (0.00s)
boring_test.go:179: Got error: local error: tls: missing extension; expected to succeed
--- FAIL: TestBoringServerCipherSuites/suite=0x544c535f4145535f3235365f47434d5f534841333834/fipstls (0.00s)
boring_test.go:187: Got error: local error: tls: missing extension; expected to succeed
This "missing extension" shows up when a hello message doesn't include supportedSignatureAlgorithms.
So, I added clientHello.supportedSignatureAlgorithms = supportedSignatureAlgorithms() in the test case when simulating a hello. This fix seems straightforward and makes sense to me given the adjacent line clientHello.supportedVersions = []uint16{VersionTLS13}.
=== RUN TestBoringServerCurves/curve=29/fipstls
panic: runtime error: index out of range [0] with length 0
goroutine 92 [running]:
crypto/tls.(*Conn).makeClientHello(0xc0000cd188)
/home/dagood/git/go/go/src/crypto/tls/handshake_client.go:150 +0xfba
crypto/tls.(*Conn).clientHandshake(0xc0000cd188, {0x876640, 0xc0000991d0})
/home/dagood/git/go/go/src/crypto/tls/handshake_client.go:184 +0x7d
crypto/tls.(*Conn).handshakeContext(0xc0000cd188, {0x8764f0, 0xae1420})
/home/dagood/git/go/go/src/crypto/tls/conn.go:1554 +0x3a6
crypto/tls.(*Conn).HandshakeContext(...)
/home/dagood/git/go/go/src/crypto/tls/conn.go:1494
crypto/tls.(*Conn).Handshake(...)
/home/dagood/git/go/go/src/crypto/tls/conn.go:1478
crypto/tls.testHandshake.func1()
/home/dagood/git/go/go/src/crypto/tls/handshake_test.go:407 +0x145
created by crypto/tls.testHandshake in goroutine 122
/home/dagood/git/go/go/src/crypto/tls/handshake_test.go:405 +0x16c
FAIL crypto/tls 11.034s
FAIL
Caused by [0] in this code in crypto/tls/handshake_client.go:
curveID := config.curvePreferences()[0]
if _, ok := curveForCurveID(curveID); !ok {
return nil, nil, errors.New("tls: CurvePreferences includes unsupported curve")
}
Here, the client config specifies a list of curve preferences, in this case only X25519. The test case runs with TLS FIPS restrictions enabled, which causes curvePreferences() to filter out X25519 because it is not allowed with FIPS. Then the indexing panics on the 0-length slice.
It seems pretty clear to me from the code that detecting a 0-length slice and returning an error would match the intent, so I changed it to:
curveIDs := config.curvePreferences()
if len(curveIDs) == 0 {
return nil, nil, errors.New("tls: CurvePreferences includes no supported curves")
}
curveID := curveIDs[0]
if _, ok := curveForCurveID(curveID); !ok {
return nil, nil, errors.New("tls: CurvePreferences includes unsupported curve")
}
Perhaps it is impossible to construct a config that triggers this error in real code. If that's true, the test case is actually invalid and X25519 should be handled differently by the test code (e.g. skipped). However, I don't know for sure, and simply handling a 0-length slice here seems like an easy and safe fix.
crypto/tls/handshake_client.go❗There were two test failures after initially resolving conflicts:
This "missing extension" shows up when a hello message doesn't include
supportedSignatureAlgorithms.So, I added
clientHello.supportedSignatureAlgorithms = supportedSignatureAlgorithms()in the test case when simulating a hello. This fix seems straightforward and makes sense to me given the adjacent lineclientHello.supportedVersions = []uint16{VersionTLS13}.Caused by
[0]in this code incrypto/tls/handshake_client.go:Here, the client
configspecifies a list of curve preferences, in this case onlyX25519. The test case runs with TLS FIPS restrictions enabled, which causescurvePreferences()to filter outX25519because it is not allowed with FIPS. Then the indexing panics on the 0-length slice.It seems pretty clear to me from the code that detecting a 0-length slice and returning an error would match the intent, so I changed it to:
Perhaps it is impossible to construct a
configthat triggers this error in real code. If that's true, the test case is actually invalid andX25519should be handled differently by the test code (e.g. skipped). However, I don't know for sure, and simply handling a 0-length slice here seems like an easy and safe fix.