From 9487a157abb4ecfc0dccd5af2adf9c5b76b47e15 Mon Sep 17 00:00:00 2001 From: Braden Ehrat Date: Sun, 16 Aug 2020 10:25:49 -0500 Subject: [PATCH] Support half-closed states (#23) After this commit, this crate will support using TLS streams in a half-closed state. Note that the TLS 1.3 spec in RFC 8446 says this should be supported: ``` Each party MUST send a "close_notify" alert before closing its write side of the connection, unless it has already sent some error alert. This does not have any effect on its read side of the connection. Note that this is a change from versions of TLS prior to TLS 1.3 in which implementations were required to react to a "close_notify" by discarding pending writes and sending an immediate "close_notify" alert of their own. That previous requirement could cause truncation in the read side. 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. ``` https://tools.ietf.org/html/rfc8446#page-87 The `rustls` crate raises such a clean closure of a [`ClientSession`](https://docs.rs/rustls/0.18.0/rustls/struct.ClientSession.html#impl-Read) or [`ServerSesson`](https://docs.rs/rustls/0.18.0/rustls/struct.ServerSession.html#impl-Read) read-side with `ErrorKind::ConnectionAborted`. This crate's `TlsState` struct already encodes support for the half-closed states `TlsState::ReadShutdown` and `TlsState::WriteShutdown`, in addition to `TlsState::FullyShutdown`. However, the current behavior of the `AsyncRead` implementation is that it unconditionally shuts-down the write-half of a connection after the read-half closes cleanly with `ErrorKind::ConnectionAborted`. This change removes the `stream.session.send_close_notify()` and `this.state.shutdown_write()` calls from `poll_read()`. Note that `stream.session.send_close_notify()` is still called in `poll_shutdown()`, which the application calls to cleanly shutdown the write-half. I highly suspect the logic of this can be simplified and cleaned up further. Minimally, the edited match statement now has two identical branches which could be combined into one. Additionally, perhaps the `Stream` implementation should simply return `Ok(0)` for this case in its implementation of [`tokio::io::AsyncRead`](https://docs.rs/tokio/0.2/tokio/io/trait.AsyncRead.html), since that's the defined way to indicate clean closure with EOF from `AsyncRead`. However, I want to make the minimal changes and have them reviewed for logical correctness first. Co-authored-by: Braden Ehrat --- tokio-rustls/src/client.rs | 4 ---- tokio-rustls/src/server.rs | 4 ---- 2 files changed, 8 deletions(-) diff --git a/tokio-rustls/src/client.rs b/tokio-rustls/src/client.rs index 2bf136d..444659e 100644 --- a/tokio-rustls/src/client.rs +++ b/tokio-rustls/src/client.rs @@ -78,10 +78,6 @@ where Poll::Ready(Ok(n)) => Poll::Ready(Ok(n)), Poll::Ready(Err(ref e)) if e.kind() == io::ErrorKind::ConnectionAborted => { this.state.shutdown_read(); - if this.state.writeable() { - stream.session.send_close_notify(); - this.state.shutdown_write(); - } Poll::Ready(Ok(0)) } output => output, diff --git a/tokio-rustls/src/server.rs b/tokio-rustls/src/server.rs index b5f8375..d9cc62c 100644 --- a/tokio-rustls/src/server.rs +++ b/tokio-rustls/src/server.rs @@ -79,10 +79,6 @@ where Poll::Ready(Ok(n)) => Poll::Ready(Ok(n)), Poll::Ready(Err(ref err)) if err.kind() == io::ErrorKind::ConnectionAborted => { this.state.shutdown_read(); - if this.state.writeable() { - stream.session.send_close_notify(); - this.state.shutdown_write(); - } Poll::Ready(Ok(0)) } Poll::Ready(Err(e)) => Poll::Ready(Err(e)),