Add a deadline to TLS handshake. #3921
Conversation
Previously, if a TLS handshake timed out, we would block forever in `conn.Handshake()`, leaking both a TCP connection and a goroutine. This sets a deadline on the underlying TCP connection, ensuring that `conn.Handshake()` eventually times out.
va/va.go
Outdated
| @@ -665,18 +665,13 @@ func tlsDial(ctx context.Context, hostPort string, config *tls.Config) (*tls.Con | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| if deadline, ok := ctx.Deadline(); ok { | |||
| netConn.SetDeadline(deadline) | |||
There was a problem hiding this comment.
errcheck wants you to check the error returned here.
va/va.go
Outdated
| @@ -665,18 +665,13 @@ func tlsDial(ctx context.Context, hostPort string, config *tls.Config) (*tls.Con | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| if deadline, ok := ctx.Deadline(); ok { | |||
There was a problem hiding this comment.
Is there ever a normal case in which !ok? It seems like we should probably have an audit err log line for the case where we get this far without a Deadline on the context to catch future bugs.
| @@ -671,7 +671,7 @@ func TestTLSSNI01TimeoutAfterConnect(t *testing.T) { | |||
| t.Fatalf("Connection should've timed out") | |||
| } | |||
| test.AssertEquals(t, prob.Type, probs.ConnectionProblem) | |||
| expected := "Timeout after connect (your server may be slow or overloaded)" | |||
| expected := "Timeout during read (your server may be slow or overloaded)" | |||
There was a problem hiding this comment.
Can you update the va tests to include a server that sleeps during the Handshake? I remember you suggesting this when this was in my sprint. Something like overloading a callback for providing a cert and adding a long blocking sleep.
There was a problem hiding this comment.
I had forgotten that we already have a server like that in these unittests, and that's what's used in this particular test:
Lines 632 to 642 in 8167d87
In other words, we were already testing that we do the right thing when we time out during a handshake, in terms of error messages to the user. An ideal test for this fix would check that we haven't leaked goroutines or TCP connections, but I'm not sure how to achieve that reliably. I'll take a quick look at what's available to us.
There was a problem hiding this comment.
In other words, we were already testing that we do the right thing when we time out during a handshake, in terms of error messages to the user.
Ahhh! Ok that makes sense 👍
An ideal test for this fix would check that we haven't leaked goroutines or TCP connections, but I'm not sure how to achieve that reliably. I'll take a quick look at what's available to us.
Knowing that the existing slowTLSSrv is there makes me think its fine to +1 this as-is if it turns out to be fiddly testing the leak.
|
Unit test is broken. |
|
Yeah, I added a potentially-flaky test to check for the leak, and it appears to be solidly flaky. :-) I'll back it out. |
Previously, if a TLS handshake timed out, we would block forever in
conn.Handshake(), leaking both a TCP connection and a goroutine.This sets a deadline on the underlying TCP connection, ensuring that
conn.Handshake()eventually times out.Fixes #3915