From 8002d283e8dc3d02f087a3885894c2c29fac93fa Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 23 Jun 2025 15:48:57 -0400 Subject: [PATCH] crypto/tls: update bogo version This commit updates the pinned revision of BoringSSL that's used for the BoGo integration test. Doing this requires a few categories of config changes: * ignoring a few new tests for features crypto/tls doesn't implement * ignoring a few new tests that require further investigation/classification, or that already have an associated tracking issue * updating the error map syntax to accommodate the upstream change that allows a one-to-many mapping One code change is required in the shim test process to adjust how we tear down a connection after an error to account for an upstream change in the test runner. Previously, for error conditions we would immediately close the connection when exiting the shim process. We instead need to do this in a multi-step process: 1. Flush any pending TLS writes to surface any alerts the error condition may have generated. 2. Close the write side of the TCP connection to signal we're not writing anymore. 3. Read and discard any pending data from the peer. 4. Close the read side of the TCP connection to fully close the socket. Without doing this unpredictable timing factors may result in spurious test failures where: 1. The runner sends us data that produces an error. 2. We send an alert, and immediately tear down the connection. 3. The runner tries to perform a write, and hits an error because the pipe is closed. 4. The runner fails the test with the pipe write error, before it reads from the connection to see the expected alert. With the new code we instead swallow the unrelated writes and the runner sees our alert after its ignored write when it tries to read from the conn. The alert is the expected test outcome, and so the test passes. This was previously not an issue because the runner was discarding the write errors. Updates #72006 Change-Id: Ib72a1c5e693aac92144696c8bae888d5f3f6c32f Reviewed-on: https://go-review.googlesource.com/c/go/+/683456 Auto-Submit: Daniel McCarney Reviewed-by: David Chase LUCI-TryBot-Result: Go LUCI Reviewed-by: Roland Shoemaker --- src/crypto/tls/bogo_config.json | 14 ++++++++++++-- src/crypto/tls/bogo_shim_test.go | 33 +++++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/crypto/tls/bogo_config.json b/src/crypto/tls/bogo_config.json index 9e3990ecb5..b269d4b670 100644 --- a/src/crypto/tls/bogo_config.json +++ b/src/crypto/tls/bogo_config.json @@ -74,6 +74,9 @@ "BadRSAClientKeyExchange-5": "crypto/tls doesn't check the version number in the premaster secret - see processClientKeyExchange comment", "SupportTicketsWithSessionID": "We don't support session ID resumption", "ResumeTLS12SessionID-TLS13": "We don't support session ID resumption", + "TrustAnchors-*": "We don't support draft-beck-tls-trust-anchor-ids", + "PAKE-Extension-*": "We don't support PAKE", + "*TicketFlags": "We don't support draft-ietf-tls-tlsflags", "CheckLeafCurve": "TODO: first pass, this should be fixed", "KeyUpdate-RequestACK": "TODO: first pass, this should be fixed", @@ -206,7 +209,14 @@ "EarlyData-Server-BadFinished-TLS13": "TODO: first pass, this should be fixed", "EarlyData-UnexpectedHandshake-Server-TLS13": "TODO: first pass, this should be fixed", "EarlyData-CipherMismatch-Client-TLS13": "TODO: first pass, this should be fixed", - "Resume-Server-UnofferedCipher-TLS13": "TODO: first pass, this should be fixed" + + "ServerNameExtensionServer-TLS-*": "https://github.com/golang/go/issues/74282", + + "Resume-Server-UnofferedCipher-TLS13": "TODO: first pass, this should be fixed", + "GarbageCertificate-Server-TLS13": "TODO: 2025/06 BoGo update, should be fixed", + "WrongMessageType-TLS13-ClientCertificate-TLS": "TODO: 2025/06 BoGo update, should be fixed", + "KeyUpdate-Requested": "TODO: 2025/06 BoGo update, should be fixed", + "AppDataBeforeTLS13KeyChange-*": "TODO: 2025/06 BoGo update, should be fixed" }, "AllCurves": [ 23, @@ -216,6 +226,6 @@ 4588 ], "ErrorMap": { - ":ECH_REJECTED:": "tls: server rejected ECH" + ":ECH_REJECTED:": ["tls: server rejected ECH"] } } diff --git a/src/crypto/tls/bogo_shim_test.go b/src/crypto/tls/bogo_shim_test.go index 2e88d539c4..7cab568db8 100644 --- a/src/crypto/tls/bogo_shim_test.go +++ b/src/crypto/tls/bogo_shim_test.go @@ -420,6 +420,12 @@ func bogoShim() { } } if err != io.EOF { + // Flush the TLS conn and then perform a graceful shutdown of the + // TCP connection to avoid the runner side hitting an unexpected + // write error before it has processed the alert we may have + // generated for the error condition. + orderlyShutdown(tlsConn) + retryErr, ok := err.(*ECHRejectionError) if !ok { log.Fatal(err) @@ -505,6 +511,31 @@ func bogoShim() { } } +// If the test case produces an error, we don't want to immediately close the +// TCP connection after generating an alert. The runner side may try to write +// additional data to the connection before it reads the alert. If the conn +// has already been torn down, then these writes will produce an unexpected +// broken pipe err and fail the test. +func orderlyShutdown(tlsConn *Conn) { + // Flush any pending alert data + tlsConn.flush() + + netConn := tlsConn.NetConn() + tcpConn := netConn.(*net.TCPConn) + tcpConn.CloseWrite() + + // Read and discard any data that was sent by the peer. + buf := make([]byte, maxPlaintext) + for { + n, err := tcpConn.Read(buf) + if n == 0 || err != nil { + break + } + } + + tcpConn.CloseRead() +} + func TestBogoSuite(t *testing.T) { if testing.Short() { t.Skip("skipping in short mode") @@ -526,7 +557,7 @@ func TestBogoSuite(t *testing.T) { if *bogoLocalDir != "" { bogoDir = *bogoLocalDir } else { - const boringsslModVer = "v0.0.0-20241120195446-5cce3fbd23e1" + const boringsslModVer = "v0.0.0-20250620172916-f51d8b099832" bogoDir = cryptotest.FetchModule(t, "boringssl.googlesource.com/boringssl.git", boringsslModVer) } -- 2.50.0