Further updates to rustls 0.20

[djc]

Note: this targets the branch for #64.

[djc]

Should we make sure to yield UnexpectedEof on future reads in this case?

[hawkw]

Yeah, I think so?

[djc]

If we get UnexpectedEof from the reader().read() call, that means the reader currently has:

• No received plaintext left to read, and
• !peer_cleanly_closed, which expands to !(has_received_close_notify && !self.message_deframer.has_pending()), and
• has_seen_eof on the input stream

has_seen_eof should be permanent, but if we have not yet received a CloseNotify it is possible that there are still frames pending in the deframer that contain a CloseNotify. So we should actually not eagerly return UnexpectedEof unless the reader gives it to us.

Do you agree?

[djc]

In testing #69, we found that the AsyncRead impl is not quite right yet. In particular, the would_block guard on the Ok(0) case for the Connection::reader().read() call seems to cause issues (potentially because !self.eof prevents would_block from being set in case of EOF). Here's what I was toying with:

diff --git a/tokio-rustls/src/common/mod.rs b/tokio-rustls/src/common/mod.rs
index 77d371d..05789b1 100644
--- a/tokio-rustls/src/common/mod.rs
+++ b/tokio-rustls/src/common/mod.rs
@@ -229,63 +229,61 @@ impl<'a, IO: AsyncRead + AsyncWrite + Unpin, S: Connection> AsyncRead for Stream
         let prev = buf.remaining();
 
         while buf.remaining() != 0 {
-            let mut would_block = false;
+            let mut io_pending = false;
 
             // read a packet
             while !self.eof && self.session.wants_read() {
-                match self.read_io(cx) {
+                match dbg!(self.read_io(cx)) {
                     Poll::Ready(Ok(0)) => {
                         self.eof = true;
                         break;
                     }
                     Poll::Ready(Ok(_)) => (),
                     Poll::Pending => {
-                        would_block = true;
+                        io_pending = true;
                         break;
                     }
                     Poll::Ready(Err(err)) => return Poll::Ready(Err(err)),
                 }
             }
 
-            return match self.session.reader().read(buf.initialize_unfilled()) {
-                Ok(0) if prev == buf.remaining() && would_block => Poll::Pending,
+            return match dbg!(self.session.reader().read(buf.initialize_unfilled())) {
+                // The TLS session has been closed cleanly, we can return since there will
+                // be no more data to read from the session.
+                Ok(0) => break,
+
+                // Read some bytes
                 Ok(n) => {
                     buf.advance(n);
-
-                    if self.eof || would_block {
+                    if self.eof || io_pending {
                         break;
                     } else {
                         continue;
                     }
                 }
 
-                // Rustls may return `WouldBlock` instead of `Ok(0)` if the
-                // stream has reached EOF without receiving a `CloseNotify` from
-                // the peer. Therefore, we must determine whether a `WouldBlock`
-                // here means EOF, or if there is more data to read.
+                // Rustls returns `WouldBlock` if it needs more data before returning.
                 Err(ref err) if err.kind() == io::ErrorKind::WouldBlock => {
-                    if prev == buf.remaining() && would_block {
+                    if prev == buf.remaining() && io_pending {
                         Poll::Pending
-                    } else if self.eof || would_block {
+                    } else if self.eof || io_pending {
                         break;
                     } else {
                         continue;
                     }
                 }
+
+                // `UnexpectedEof` means the peer closed the connection without sending
+                // `CloseNotify`. Declare EOF and propagate the error.
                 Err(err) if err.kind() == io::ErrorKind::UnexpectedEof => {
-                    self.eof = true;
-                    if prev == buf.remaining() && would_block {
+                    if prev == buf.remaining() {
                         Poll::Ready(Err(err))
                     } else {
                         break;
                     }
                 }
-                Err(ref err)
-                    if err.kind() == io::ErrorKind::ConnectionAborted
-                        && prev != buf.remaining() =>
-                {
-                    break
-                }
+
+                // This should be unreachable.
                 Err(err) => Poll::Ready(Err(err)),
             };
         }
diff --git a/tokio-rustls/src/common/test_stream.rs b/tokio-rustls/src/common/test_stream.rs
index 14766c0..a1730d1 100644
--- a/tokio-rustls/src/common/test_stream.rs
+++ b/tokio-rustls/src/common/test_stream.rs
@@ -217,8 +217,8 @@ async fn stream_eof() -> io::Result<()> {
     let mut stream = Stream::new(&mut good, &mut client).set_eof(true);
 
     let mut buf = Vec::new();
-    stream.read_to_end(&mut buf).await?;
-    assert_eq!(buf.len(), 0);
+    let result = stream.read_to_end(&mut buf).await;
+    assert_eq!(result.err().map(|e| e.kind()), Some(io::ErrorKind::UnexpectedEof));
 
     Ok(()) as io::Result<()>
 }

This still fails stream_good for reasons that I can't figure out right now. (I renamed would_block because it gets a bit confusing now that rustls can also return WouldBlock directly.)

[djc]

@hawkw would you have a chance to look at this? Would be nice to figure out if we have the reader semantics right.

[hawkw]

@hawkw would you have a chance to look at this? Would be nice to figure out if we have the reader semantics right.

Checking it out right now, sorry it took me a few days to get to this!

[djc]

@hawkw gentle ping? 🙂

[quininer]

This looks great to me, are you ready to merge?

[djc]

I think this is ready to be merged into the target branch, yes. Probably also fine to have on master.

[hawkw]

sorry it took me so long to look at this, seems good overall!

[hawkw]

Yeah, I think so?

[djc]

Reverted the assert changes and updated to current rustls main. On further investigation, I convinced myself the change relating to UnexpectedEof is not necessary after all.

[djc]

Updated to final rustls 0.20 release.

[quininer]

If there are no more comments, I plan to merge this PR in next two days.

[paolobarbolini]

If there are no more comments, I plan to merge this PR in next two days.

It's be cool to expose the tls12 rustls feature

[djc]

@paolobarbolini good suggestion. Added a commit that forwards the logging and tls12 features, enabling them by default.