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.

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

TLS connections that timeout during handshake will leak #3915

Closed
jsha opened this issue Oct 29, 2018 · 1 comment
Closed

TLS connections that timeout during handshake will leak #3915

jsha opened this issue Oct 29, 2018 · 1 comment

Comments

@jsha
Copy link
Contributor

jsha commented Oct 29, 2018

In the VA, we create our own TCP connection, then ask the crypto/tls package to do a handshake over it:

boulder/va/va.go

Lines 659 to 663 in 3bb0657

conn := tls.Client(netConn, config)
errChan := make(chan error)
go func() {
errChan <- conn.Handshake()
}()

The conn.Handshake() method doesn't take any contexts or timeouts, so if the TLS server fails to respond to any handshake messages (as we often see), this goroutine will block forever. We'll also leak a TCP connection that will never be cleaned up.

One possible workaround: after successfully dialing the TCP connection, call conn.SetReadDeadline(...) to set a read deadline that matches the context deadline.

@jsha
Copy link
Contributor Author

jsha commented Oct 30, 2018

Note: It's also appealing to put a select inside the goroutine like so:

go func() {
  select {
    case <-ctx.Done():
      return
    case errChan <- conn.Handshake():
      return
  }
}

However, this only allows us to reap this one goroutine early in the case of a timeout; there will still be goroutines in crypto/tls trying to read from the connection, and the TCP connection will still be alive.

By setting a deadline on the TCP connection, we ensure that conn.Handshake() will return in a finite amount of time, so we don't really need the select inside the goroutine.

We could even consider getting rid of the goroutine, and just putting the conn.Handshake() call in the main select.

@cpu cpu closed this as completed in #3921 Nov 5, 2018
cpu pushed a commit that referenced this issue Nov 5, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants