search

rustls / tokio-rustls

Async TLS for the Tokio runtime
Apache License 2.0
125 stars 70 forks source link

Read any pending data before shutting down stream #63

Closed levkk closed 7 months ago

levkk commented 7 months ago

I ran into the issue where another Rustls client (launchbadge/sqlx) is sending CloseNotify (correctly I believe) before shutting down the stream. If the server doesn't wait for it when shutting down the socket, the client throws:

Transport endpoint is not connected (os error 107)

This PR ensures if there are any data still in the stream, it's read in and only then the stream is closed. All incoming data at this point is discarded, so this shouldn't be security issue I think, but I'm not an expert.

quininer commented 7 months ago

Both parties need not wait to receive a "close_notify" alert before closing their read side of the connection, though doing so would introduce the possibility of truncation.

According to rfc https://www.rfc-editor.org/rfc/rfc8446#page-87 I don't think this is necessary. And waiting for packet before shutdown is rather a DoS risk, as the other party may send TLS packets slowly enough to prevent the TCP connection from being closed.

quininer commented 7 months ago

I'm considering having the TlsStream shutdown not actually do a TCP shutdown, which has two benefits

  1. Users can decide for themselves when to do TCP shutdown.
  2. Users can reuse the TCP connection after the TLS connection has ended.
levkk commented 7 months ago

I think I agree. The bug seems to be more on the client side here. I've submitted a fix to SQLx which was my specific use case and that worked without this patch. Even if this patch is merged, I think there still will be a race condition between clients sending CloseNotify and us polling the socket for read data.

quininer commented 7 months ago

I will close this PR and I will consider remove TCP shutdown in next major version.