Issue with native-tls handshake

[mirandole]

Hello,

I got an issue using tungstenite with native-tls feature. Sometimes when my project try to connect to a websocket with tungteniste using native-tls, the connection with client_tls panic with this error : "Bug: TLS handshake not blocked".
It's annoying because I don't know how to catch that kind of panic in my connection retry mechanism.

So my question is why does this code needs to panic when TlsHandshakeError::WouldBlock ?
It would be much pleasant if the code will return an error instead of panic. So I can retry to connect with client_tls without getting my project exit with panic.

To try to find a way to avoid my rust program to panic, I now use rustls instead of native-tls. But I think it would be better to return an error instead of panic.

match mode {
                Mode::Plain => Ok(MaybeTlsStream::Plain(socket)),
                Mode::Tls => {
                    let try_connector = tls_connector.map_or_else(TlsConnector::new, Ok);
                    let connector = try_connector.map_err(TlsError::Native)?;
                    let connected = connector.connect(domain, socket);
                    match connected {
                        Err(e) => match e {
                            TlsHandshakeError::Failure(f) => Err(Error::Tls(f.into())),
                            TlsHandshakeError::WouldBlock(_) => {
                                panic!("Bug: TLS handshake not blocked")
                            }
                        },
                        Ok(s) => Ok(MaybeTlsStream::NativeTls(s)),
                    }
                }
            }

https://github.com/snapview/tungstenite-rs/blob/master/src/tls.rs

[daniel-abramov]

Tungstenite must not panic unless there is a bug in a crate. As the panic says, this error indicates a bug in a library and must not happen. I don't remember why this exact panic is there, i.e., why this error is considered a bug, as this commit has been there for more than seven years, so I'll mention @agalakhov here, he certainly knows this part better.

However, I must also say that we've never encountered this panic, and I don't remember it ever being reported. Is it reproducible?

[mirandole]

Yes, upon reflection, I was able to reproduce it some minutes ago. It happens when my internet connection drop while executing client_tls method using tcp_stream.set_read_timeout and tcp_stream.set_write_timeout. If I disconnect my internet connection during the sleep time just before client_tls(). The panic happens because of tcp_stream timeout.

Removing tcp_stream.set_read_timeout and tcp_stream.set_write_timeout resolved my issue but client_tls timeout happens without any panic after 5min which is quite long for me but ok to manage. It would be nice to be able to set a custom timeout in client_tls() or to remove this annoying panic! to be able to benefit of the tcpstream timeout feature.

use std::{net::{TcpStream, ToSocketAddrs}, time::Duration};
use tungstenite::{client::IntoClientRequest, connect, http::{self, request, Uri}, ClientRequestBuilder};
use tungstenite::client_tls;

fn main() {
    let timeout = Duration::from_secs(30);
    let hostname_port = "stream.binance.com:443";
    let socket_addr = hostname_port.to_socket_addrs().unwrap().next().unwrap();
    let wss_addr = "wss://stream.binance.com/ws/btcusdt@ticker";

    match TcpStream::connect_timeout(&socket_addr, timeout) {
        Ok(tcp_stream) => {
            println!("tcpstream connected");
            tcp_stream.set_read_timeout(Some(timeout)).expect("Can't set read timeout");
            tcp_stream.set_write_timeout(Some(timeout)).expect("Can't set write timeout");

            // DISCONNECT INTERNET CONNECTION DURING SLEEP TIME
            println!("10s sleep time to disconnect internet and reproduce panic");
            std::thread::sleep(std::time::Duration::from_secs(10));

            let uri: Uri = wss_addr.parse().unwrap();
            let request = ClientRequestBuilder::new(uri).into_client_request().unwrap();

            println!("begin client_tls connection");
            match client_tls(request, tcp_stream) {
                Ok(mut answer) => {
                    println!("client_tls connected");
                    match answer.0.get_mut() {
                        tungstenite::stream::MaybeTlsStream::Plain(stream) => {
                            println!("tungstenite::stream::MaybeTlsStream::Plain");
                            let _ = stream.set_nonblocking(true).unwrap();
                            println!("nonblocking set");
                        },
                        tungstenite::stream::MaybeTlsStream::NativeTls(stream) => {
                            println!("tungstenite::stream::MaybeTlsStream::NativeTls");
                            let _ = stream.get_mut().set_nonblocking(true).unwrap();
                            println!("nonblocking set");
                        },
                        _ => {
                            println!("unimplemented");
                            unimplemented!();
                        },
                    };

                },
                Err(e) => {
                    println!("{}", format!("Error during handshake {}", e));
                },
            }
        },
        Err(e) => {
            println!("{}", format!("error connecting to : {}\nconnection error: {}", &socket_addr, e));
        },
    }
}

PS: When I use rustls instead of native-tls, I got the same error message without panic.

[erenon]

I hit this issue as well, and it is simple to repro, without having to fiddle with the hosts connection (see below). My use-case: I'd like to use tungstenite+tls in a sans-io way, feeding it bytes, and taking bytes it wants to write, kind of like a push parser. For this end, I have a dummy stream (RWBuf), that holds bytes to be written, and bytes that tungstenite can read.
If the TLS handshake part hits EWOULDBLOCK, the BUG above gets triggered.

#[cfg(test)]
mod test {
    use anyhow::Context;

    #[derive(Default)]
    struct RWBuf {
        r: Vec<u8>,
        w: Vec<u8>,
    }

    impl std::io::Write for RWBuf {
        fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
            self.w.extend_from_slice(buf);
            Ok(buf.len())
        }

        fn flush(&mut self) -> std::io::Result<()> {
            Ok(())
        }
    }

    impl std::io::Read for RWBuf {
        fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
            // omitted: if self.r not empty, copy it to `buf`, return Ok(len)
            Err(std::io::ErrorKind::WouldBlock.into())
        }
    }

    #[test]
    fn test_wss_conn() -> anyhow::Result<()> {
        let conn = std::net::TcpStream::connect("127.0.0.1:8080").context("connect")?;
        conn.set_nodelay(true)?;
        conn.set_nonblocking(true)?;

        eprintln!("connected to {}", conn.peer_addr().unwrap());

        let stream = RWBuf::default();

        let tc = tungstenite::client_tls(format!("wss://localhost:443/hello"), stream);

        // omitted: while `tc` is not Ok
        //  - if Err is Interrupted,
        //  -- write `stream.w` to `conn`, read bytes from `conn` into `stream.r`,
        //  -- retry handshake

        Ok(())
    }
}

Instead of client_tls panicking, I would prefer it to return tungstenite::HandshakeError::Interrupted, allowing me to fill the read buffer, and continue the handshake. The logic appears to work without TLS (when paired with a tokio-tungstenite example echo server).

[daniel-abramov]

I just checked that part, and yeah, it's a bug. It must not panic at that place.

That being said, tungstenite::HandshakeError::Interrupted is probably not the right error to return in this case because it refers to the WebSocket handshake, not to the TLS handshake (TLS is conceptually a separate feature). Although the handshake error from native-tls is almost identical internally, it's still a different type.

The proper way of doing it, IMHO, is to wrap the type that native-tls contains so that handling of the TLS handshake with native-tls remains identical to the end user (symmetrical to the rustls impl).