Error NotConnected when serving a TlsStream<TcpStream> when using hyper

[Revanee]

When serving a TlsStream<TcpStream> using a hyper server, the stream's poll_shutdown method sometimes returns a io::ErrorKind::NotConnected. This happens when calling the server using curl with HTTP/1.1, but not with HTTP/1.0. Also, it happens when calling the server using openssl s_client and pressing ctrl+c instead of ctrl+d.

It seems that when a client doesn't explicitly close the connection, but simply hangs up, the stream ends with an error.

This only happens with tokio_rustls and not with tokio_native_tls.

Here is a somewhat minimal reproduction of the error:

use std::{
	fs::File,
	io::BufReader,
	net::{Ipv4Addr, SocketAddrV4},
	sync::Arc,
};

use hyper::{Request, Response};
use hyper_util::rt::TokioIo;
use tokio::net::TcpListener;
use tokio_native_tls::native_tls::Identity;
use tokio_rustls::{rustls::pki_types::CertificateDer, TlsAcceptor};

#[tokio::main]
async fn main() {
	let bind_addr = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080);
	let listener = TcpListener::bind(bind_addr).await.unwrap();
	let service = hyper::service::service_fn(move |_request: Request<_>| async {
		Result::<_, std::io::Error>::Ok(Response::new("".to_string()))
	});

	println!("Listening on {bind_addr}");
	loop {
		let (stream, _addr) = listener.accept().await.unwrap();
		tokio::spawn(async move {
			let tls_acceptor = get_tls_acceptor();
			// Uncomment to use native_tls
			// let tls_acceptor = get_tls_acceptor_native();

			let tls_stream = tls_acceptor.accept(stream).await.unwrap();
			hyper::server::conn::http1::Builder::new()
				.preserve_header_case(true)
				.title_case_headers(true)
				.serve_connection(TokioIo::new(tls_stream), service)
				.with_upgrades()
				.await
				.unwrap()
		});
	}
}

fn get_tls_acceptor_native() -> tokio_native_tls::TlsAcceptor {
	let key = std::fs::read("certs/key.pem").unwrap();
	let cert = std::fs::read("certs/certs.pem").unwrap();
	let identity = Identity::from_pkcs8(&cert, &key).unwrap();
	let acceptor = tokio_native_tls::native_tls::TlsAcceptor::builder(identity).build().unwrap();
	tokio_native_tls::TlsAcceptor::from(acceptor)
}

fn get_tls_acceptor() -> tokio_rustls::TlsAcceptor {
	let mut key_reader = BufReader::new(File::open("certs/key.pem").unwrap());
	let mut cert_reader = BufReader::new(File::open("certs/certs.pem").unwrap());

	let key = rustls_pemfile::private_key(&mut key_reader).unwrap().unwrap();
	let certs = rustls_pemfile::certs(&mut cert_reader)
		.map(|cert| cert.map(CertificateDer::from))
		.collect::<std::result::Result<_, std::io::Error>>()
		.unwrap();

	let config = tokio_rustls::rustls::ServerConfig::builder()
		.with_no_client_auth()
		.with_single_cert(certs, key)
		.unwrap();

	TlsAcceptor::from(Arc::new(config))
}

To reproduce using curl:

curl -k -v https://localhost:8080 --http1.0 gives no error.
curl -k -v https://localhost:8080 --http1.1 gives a NotConnected error.

To reproduce using openssl s_client:

openssl s_client -connect localhost:8080 then Ctrl+D gives no error.
openssl s_client -connect localhost:8080 then Ctrl+C gives a NotConnected error.

[djc]

I'm not sure why this is yielding NotConnected, but it sounds pretty similar to hyperium/hyper#3427 and connected issues (rustls/rustls#1635, denoland/deno#13058). Those were mainly for HTTP/2, but I wouldn't surprised if this same issue is happening for HTTP/1.1. I don't see tokio-rustls building NotConnected specifically, so maybe that's coming out of hyper?

[Revanee]

I think the underlying error comes from the tokio::net::TcpStream, which then gets propagated by the tokio_rustls::server::TlsStream and causes the hyper server return the wrapped error. A similar issue seems to have been addressed in h2, but the NotConnected error happens with HTTP/2 as well. I'm not sure if the issue should be addressed in rustls, tokio_rustls, or hyper. Since this doesn't happen with tokio_native_tls, maybe it should be addressed here?

[djc]

Okay, so I think this happens because tokio-rustls tries to write outgoing data to the TcpStream before it actually shuts it down:

https://github.com/rustls/tokio-rustls/blob/main/src/common/mod.rs#L335

tokio-native-tls doesn't do this:

https://github.com/tokio-rs/tls/blob/master/tokio-native-tls/src/lib.rs#L218
https://github.com/sfackler/rust-native-tls/blob/master/src/imp/openssl.rs#L455

Looks like this behavior was first added this behavior in 7949f43 about 5 years ago. @quininer do you remember why this is necessary?

Relevant context from tokio's AsyncWrite docs:

This shutdown method is required by implementers of the AsyncWrite trait. Wrappers typically just want to proxy this call through to the wrapped type, and base types will typically implement shutdown logic here or just return Ok(().into()). Note that if you’re wrapping an underlying AsyncWrite a call to shutdown implies that transitively the entire stream has been shut down. After your wrapper’s shutdown logic has been executed you should shut down the underlying stream.

Invocation of a shutdown implies an invocation of flush. Once this method returns Ready it implies that a flush successfully happened before the shutdown happened. That is, callers don’t need to call flush before calling shutdown. They can rely that by calling shutdown any pending buffered data will be written out.

[djc]

#42 proposes just swallowing NotConnected errors from write_io() in poll_shutdown().