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 <braden@cloudflare.com>
This commit is contained in:
Braden Ehrat 2020-08-16 10:25:49 -05:00 committed by GitHub
parent c2dd82e323
commit 9487a157ab
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 0 additions and 8 deletions

View File

@ -78,10 +78,6 @@ where
Poll::Ready(Ok(n)) => Poll::Ready(Ok(n)), Poll::Ready(Ok(n)) => Poll::Ready(Ok(n)),
Poll::Ready(Err(ref e)) if e.kind() == io::ErrorKind::ConnectionAborted => { Poll::Ready(Err(ref e)) if e.kind() == io::ErrorKind::ConnectionAborted => {
this.state.shutdown_read(); this.state.shutdown_read();
if this.state.writeable() {
stream.session.send_close_notify();
this.state.shutdown_write();
}
Poll::Ready(Ok(0)) Poll::Ready(Ok(0))
} }
output => output, output => output,

View File

@ -79,10 +79,6 @@ where
Poll::Ready(Ok(n)) => Poll::Ready(Ok(n)), Poll::Ready(Ok(n)) => Poll::Ready(Ok(n)),
Poll::Ready(Err(ref err)) if err.kind() == io::ErrorKind::ConnectionAborted => { Poll::Ready(Err(ref err)) if err.kind() == io::ErrorKind::ConnectionAborted => {
this.state.shutdown_read(); this.state.shutdown_read();
if this.state.writeable() {
stream.session.send_close_notify();
this.state.shutdown_write();
}
Poll::Ready(Ok(0)) Poll::Ready(Ok(0))
} }
Poll::Ready(Err(e)) => Poll::Ready(Err(e)), Poll::Ready(Err(e)) => Poll::Ready(Err(e)),