Handle EOF like rustls::Stream

[vilgotf]

This aims to fix twilight-rs/twilight#1428, which is two layers of abstraction removed from tokio-rustls (this fix might therefore not be optimal). That bug occurs on closing a TLS encrypted websocket connetion with Discord's gateway. The following blocking RusTLS + tungstenite MRE shows no error:

use tracing_subscriber::filter::LevelFilter;
use tungstenite::{connect, Error};

fn main() {
    tracing_subscriber::fmt()
        .with_max_level(LevelFilter::TRACE)
        .init();

    let (mut socket, _) = connect("wss://gateway.discord.gg").expect("Can't connect");

    let hello = socket.read_message().unwrap();
    println!("{hello}");
    socket.close(None).unwrap();
    loop {
        match socket.read_message() {
            Ok(msg) => println!("{msg}"),
            Err(Error::ConnectionClosed) => break,
            Err(e) => panic!("{e}"),
        };
    }
}

Whereas the same exact code with tokio-tungstenite does error:

use tokio_stream::StreamExt;
use tokio_tungstenite::connect_async;
use tracing_subscriber::filter::LevelFilter;

#[tokio::main(flavor = "current_thread")]
async fn main() {
    tracing_subscriber::fmt()
        .with_max_level(LevelFilter::TRACE)
        .init();

    let (mut socket, _) = connect_async("wss://gateway.discord.gg")
        .await
        .expect("Can't connect");

    let hello = socket.next().await.unwrap().unwrap();
    println!("{hello}");
    socket.close(None).await.unwrap();
    loop {
        match socket.next().await {
            Some(Ok(msg)) => println!("{msg}"),
            Some(Err(e)) => panic!("{e}"),
            None => break,
        };
    }
}

In the first example, tungstenite uses the rustls::Stream abstraction to read from the websocket, and in the second example tokio-tungstenite uses tokio_rustls::client::TlsStream (which exposes a Read compatible interface to tungstenite). This leads me to believe the bug is in tokio_rustls.

rustls::Stream's Read implemention is very similar to tokio_rustls::client::TlsStream's AsyncRead (the logic I'm interested in for tokio_tungstenite is in the private tokio_rustls::common::Stream type), both calling read_tls while wants_read returns true and then calling Reader::read afterwards, but differs in two ways:

1. It calls rustls::ConnectionCommon::complete_io(), potentially writing data in addition to reading data
2. It returns early (before Reader::read) if read_tls returns Ok(0) and process_new_packets returns Ok with IoState's plaintext_bytes_to_read returning 0.

This PR then adds the logic from 2. to tokio_rustls::common::Stream which fixes the issue I'm running into.

[quininer]

This seems reasonable, would you be able to add a regression test?

[vilgotf]

This seems reasonable, would you be able to add a regression test?

There existed an EOF test already so I just updated that

[quininer]

It is not reasonable to break this test, we can only return Ok(0) when we receive a close_notify, otherwise this would result in a truncation attack.

see https://docs.rs/rustls/latest/rustls/struct.Reader.html#method.read

[quininer]

I have re-read the notes on rustls stream and I suspect that current behavior is not expected. maybe rustls contributors can give some suggestion.

cc @ctz @djc

[djc]

@vilgotf how does the example code error, and why do you believe it is not reasonable for it to do so? I'm honestly not sure the rustls::Stream code is the best comparison, but your MRE is not actually complete (that is, it's not enough for me to reproduce the issue).

@quininer which current behavior is not expected?

[quininer]

rustls Stream cannot distinguish between a normal close notify and an unexpected tcp eof, which allows truncation attack, which I suspect is not expected.

[djc]

That's fair, do you want to file an issue about this against the rustls repo? We're in the process of preparing a 0.21 release so now would be a good time to adjust the API accordingly.

[vilgotf]

@vilgotf how does the example code error, and why do you believe it is not reasonable for it to do so? I'm honestly not sure the rustls::Stream code is the best comparison, but your MRE is not actually complete (that is, it's not enough for me to reproduce the issue).

Only the second example panics, which I thought was an error in tokio-rustls as rustls and OpenSSL (tokio-tls-native & tls-native) does not error with UnexpectedEof. However, from what I can gather from @quininer, this should actually be the other way around, i.e. all TLS implementations should error but only tokio-rustls does successfully. No close_notify is seemingly received.

As for the examples not working, I have no idea why that would be. I'm running both example with the rustls-tls-native-roots (tokio-)tungstenite feature.

Also, thank you both for your time with helping me out :)

[quininer]

@djc I opened an issue on rustls repo rustls/rustls#1188

[cpu]

This should be fixed in the main branch of rustls as of rustls/rustls#1299