Add support for TLS 1.3 - #564
Conversation
This PR allows TLS 1.3, by removing the MaxVersion in the client config. This would silently swallow errors, so e.g. a client without cert dialing a server that requires client certs would lead to an error which gets ignored, leading to retries until timeout. In this PR, we wrap the connection and if an error occurs we send it to the existing `result` channel. I think this matches @jhump's comment in fullstorydev#387 (comment) **Testing** ```console # Start the test server (in another tab) go run ./internal/testing/cmd/testserver \ -cert internal/testing/tls/server.crt \ -key internal/testing/tls/server.key \ -cacert internal/testing/tls/ca.crt \ -requirecert -p 9999 # Old behavior $ grpcurl -cacert internal/testing/tls/ca.crt \ localhost:9999 list Failed to dial target host "localhost:9999": context deadline exceeded # New behavior $ go run ./cmd/grpcurl -cacert internal/testing/tls/ca.crt \ localhost:9999 list Failed to dial target host "localhost:9999": remote error: tls: certificate required exit status 1 ``` The old behavior is to hang until we hit the deadline. The new behavior is to return immediately with an error. Fixes fullstorydev#563
| type errSignalingConn struct { | ||
| net.Conn | ||
| writeResult func(res interface{}) | ||
| once sync.Once |
There was a problem hiding this comment.
I don't think we need the sync.Once, writeResult() has to be idempotent. If we're worried about spamming that select statement too many times, we should add an atomic CompareAndSwap guard inside writeResult(), and then unconditionally push and close the channel
Removing unnecessary sync.Once, as the channel will already drop repeated calls.
|
Thanks for the review! I've implemented your suggestion. I'm also happy to apply the same connection-error handling logic to apply to plaintext connections, if you'd like (by getting rid of this if statement). If that sounds good, I can do it as a follow-up PR, as some tests need updating if I make that change. |
|
I think it's fine to just support TLS here |
|
But feel free to follow up if you can make better error reporting on plain text happen! |
Avoid hanging on connection errors; fail fast by propagating connection errors. This also speeds up tests a lot (from >20s to about 3s), since the tests included some connection timeouts. This expands on fullstorydev#564 ; it does the same for plaintext connections. Fixes fullstorydev#387
|
FYI: we've been getting test flakes recently-- It's either this PR, or #567 - unsure. Would you mind taking a look @bcleenders ? |
|
hi! Sorry, I hadn't noticed your message here. Yes, looking into it! |
PRs fullstorydev#564 and fullstorydev#567 introduced errSignalingConn to surface TLS errors (especially TLS 1.3 post-handshake alerts) to BlockingDial callers. This caused a flaky test failure (reported in: fullstorydev#564 (comment)): ``` TestBrokenTLS_RequireClientCertButNonePresented: expecting a TLS certificate error, got: read tcp ...: use of closed network connection ``` The race works as follows: 1. With TLS 1.3, the server rejects the connection *after* the handshake completes (e.g. missing client cert) by sending a TLS alert, then closing the connection. 2. Simultaneously, the client's Write fails (broken pipe) because the server already closed its side. 3. grpc-go's `NewHTTP2Client` returns the Write error and calls `t.Close()` to tear down the transport, which calls `errSignalingConn.Close()` -> `c.Conn.Close()`, closing the connection. 4. The reader goroutine's pending Read (which should have returned the TLS alert (e.g. "certificate required")) instead returns `use of closed network connection` because the connection was closed underneath it. 5. `writeResult` surfaces the "closed network connection" error to the caller instead of the meaningful TLS alert. The fix: delay Close on the errSignalingConn until the outcome of the dial is known, with a 50ms timeout. This gives the reader a chance to surface the TLS alert. Demonstrating this by stressing the test: **Without the fix** ``` alasia :: github/fullstorydev/grpcurl ‹master› » go test -c -o /tmp/grpcurl.test . && GOMAXPROCS=2 $HOME/go/bin/stress -p 16 -count 5000 -ignore 'assign requested address|context deadline exceeded' /tmp/grpcurl.test -test.run 'TestBrokenTLS' /var/folders/7l/0tdjxwg912j9gqh446yk3ll40000gn/T/go-stress-20260724T113423-1710148011 --- FAIL: TestBrokenTLS_RequireClientCertButNonePresented (0.00s) tls_settings_test.go:332: expecting a TLS certificate error, got: read tcp 127.0.0.1:52850->127.0.0.1:52848: use of closed network connection FAIL ERROR: exit status 1 [...] 5s: 1580 runs so far, 5 failures (0.32%), 13 active ``` **With the fix** ``` alasia :: github/fullstorydev/grpcurl ‹surface-post-dial-conn-errors› » go test -c -o /tmp/grpcurl.test . && GOMAXPROCS=2 $HOME/go/bin/stress -p 16 -count 5000 -ignore 'assign requested address|context deadline exceeded' /tmp/grpcurl.test -test.run 'TestBrokenTLS' 5s: 1594 runs so far, 0 failures, 16 active 10s: 3386 runs so far, 0 failures, 16 active 15s: 4999 runs so far, 0 failures, 1 active 15s: 5000 runs total, 0 failures ```
…ion (#572) * Fix flaky TLS test by waiting for dial outcome before closing connection PRs #564 and #567 introduced errSignalingConn to surface TLS errors (especially TLS 1.3 post-handshake alerts) to BlockingDial callers. This caused a flaky test failure (reported in: #564 (comment)): ``` TestBrokenTLS_RequireClientCertButNonePresented: expecting a TLS certificate error, got: read tcp ...: use of closed network connection ``` The race works as follows: 1. With TLS 1.3, the server rejects the connection *after* the handshake completes (e.g. missing client cert) by sending a TLS alert, then closing the connection. 2. Simultaneously, the client's Write fails (broken pipe) because the server already closed its side. 3. grpc-go's `NewHTTP2Client` returns the Write error and calls `t.Close()` to tear down the transport, which calls `errSignalingConn.Close()` -> `c.Conn.Close()`, closing the connection. 4. The reader goroutine's pending Read (which should have returned the TLS alert (e.g. "certificate required")) instead returns `use of closed network connection` because the connection was closed underneath it. 5. `writeResult` surfaces the "closed network connection" error to the caller instead of the meaningful TLS alert. The fix: delay Close on the errSignalingConn until the outcome of the dial is known, with a 50ms timeout. This gives the reader a chance to surface the TLS alert. Demonstrating this by stressing the test: **Without the fix** ``` alasia :: github/fullstorydev/grpcurl ‹master› » go test -c -o /tmp/grpcurl.test . && GOMAXPROCS=2 $HOME/go/bin/stress -p 16 -count 5000 -ignore 'assign requested address|context deadline exceeded' /tmp/grpcurl.test -test.run 'TestBrokenTLS' /var/folders/7l/0tdjxwg912j9gqh446yk3ll40000gn/T/go-stress-20260724T113423-1710148011 --- FAIL: TestBrokenTLS_RequireClientCertButNonePresented (0.00s) tls_settings_test.go:332: expecting a TLS certificate error, got: read tcp 127.0.0.1:52850->127.0.0.1:52848: use of closed network connection FAIL ERROR: exit status 1 [...] 5s: 1580 runs so far, 5 failures (0.32%), 13 active ``` **With the fix** ``` alasia :: github/fullstorydev/grpcurl ‹surface-post-dial-conn-errors› » go test -c -o /tmp/grpcurl.test . && GOMAXPROCS=2 $HOME/go/bin/stress -p 16 -count 5000 -ignore 'assign requested address|context deadline exceeded' /tmp/grpcurl.test -test.run 'TestBrokenTLS' 5s: 1594 runs so far, 0 failures, 16 active 10s: 3386 runs so far, 0 failures, 16 active 15s: 4999 runs so far, 0 failures, 1 active 15s: 5000 runs total, 0 failures ``` * Implement review comment
This PR allows TLS 1.3, by removing the MaxVersion in the client config.
This would silently swallow errors, so e.g. a client without cert dialing a server that requires client certs would lead to an error which gets ignored, leading to retries until timeout.
In this PR, we wrap the connection and if an error occurs we send it to the existing
resultchannel.I think this matches @jhump's comment in #387 (comment) (but for this PR, I scoped the change to TLS only to avoid dropping a huge patch)
Testing
Start a test server that requires client certs
Old behavior
New behavior
The old behavior is to hang until we hit the deadline. The new behavior is to return immediately with an error.
Fixes #563