Example simpleclient is broken

[xuganyu96]

The example simple client is broken. When running cargo run --bin simpleclient the program output is as follows:

Current ciphersuite: TLS13_AES_256_GCM_SHA384
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Kind(UnexpectedEof)', examples/src/bin/simpleclient.rs:62:37
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

git bisect identified ab685b5e89f15725436effadd31ae4411b95738d to be the first bad commit.

Further investigation is needed on what changes are needed to restore simple client back to a working state. I am willing to submit PR if appropriate and needed.

[djc]

Have you tried this with different hosts? The change you identified was an intentional strictening of the code that produces an UnexpectedEof error if the remote host closes the connection without sending a CloseNotify alert, because there is a risk of truncation attacks. We've seen that some hosts don't send a CloseNotify so there are some situations where ignoring the error makes sense, if the protocol you're using has another kind of framing so that you know the connection is ended on a reasonable boundary.

[cpu]

(Edit: oops, comment clash with Djc!)

Hi there,

Good catch 👍 We should think about adding something to the .github/workflows/connect-tests.yml CI task that uses this example to flag failures like this sooner.

thread 'main' panicked at 'called Result::unwrap() on an Err value: Kind(UnexpectedEof)', examples/src/bin/simpleclient.rs:62:37

git bisect identified ab685b5 to be the first bad commit.

That makes sense - that commit was meant to surface this error condition, it just interacts badly with the demo's liberal use of unwrap for network errors 😅

It might be worth trying to change the demo request to see if we can get the peer server to properly send a TLS close notify alert before the TCP EOF (maybe also worth a quick pcap to verify there isn't one being sent already we're miss-handling?)

If getting a clean TLS close isn't feasible, the expect() could be replaced by something that squelches the unexpected EOF error.

[xuganyu96]

I did some quick search and I think that the root cause (not that it is a problem) lies with the higher level application protocol (HTTP in this case). The short of it is that modern versions of HTTP is a self-terminating protocol, and common implementation will not send close_notify at TLS level because HTTP already knows when all data has been received (source).

So in other words, having a connection that is dropped without close_notify is technically expected behavior.

I am honestly not sure where to go from here. I don't think that the simple client should be responsible for being aware of higher level protocol behaviors, but I also have no idea where to find publicly available HTTP server that properly sends close_notify

[xuganyu96]

@djc Changing the host to www.rust-lang.org works. Would that be a viable alternative for the simpleclient example?

[quininer]

If the HTTP client knows when to terminate, it can terminate the read before it reaches eof, and UnexpectedEof will not be triggered at this time.

For example, if the server returns Content-Length: 123, then we can use tls_stream.take(123).read_to_end(&mut buf)?; to read the body without eof.

[djc]

In this example code, we're not even parsing the HTTP response, so using the value from the content-length header is a substantial increase in complexity that would distract from the core TLS functionality. Changing to www.rust-lang.org seems more attractive to me.

Does make me wonder why we haven't had more complaints about this... as far as I'm aware tokio-rustls and hyper-rustls will still propagate this error. (Some people have clearly run into it in denoland/deno#13058, but it seems to be fairly rare.)

[cpu]

Changing to www.rust-lang.org seems more attractive to me.

SGTM. Maybe with a comment-in-file that has text similar to your response earlier in this thread? Something that alludes to the truncation attack and that some servers don't cleanly close and applications need to decide how to handle that.

[ctz]

Note that limitedclient also is affected in the same way. I've also just noticed that tlsclient-mio also mistakenly reports clean closure on TCP EOF. That could be fixed by:

diff --git a/examples/src/bin/tlsclient-mio.rs b/examples/src/bin/tlsclient-mio.rs
index 49dc85b9..0dc8f4ef 100644
--- a/examples/src/bin/tlsclient-mio.rs
+++ b/examples/src/bin/tlsclient-mio.rs
@@ -89,7 +89,6 @@ impl TlsClient {
             Ok(0) => {
                 println!("EOF");
                 self.closing = true;
-                self.clean_closure = true;
                 return;
             }

(after that fix, tlsclient-mio --http www.rust-lang.org exits zero, but tlsclient-mio --http google.com exits non-zero)

[ctz]

Out of interest @dadrian / @davidben is this expected? For me google.com is negotiating TLS1.3, but then neglects to send a close_notify. RFC8446 says:

Each party MUST send a "close_notify" alert before closing its write
side of the connection, unless it has already sent some error alert.

[djc]

(Google folks: we've seen similar issues in curl/curl#10457 (comment), where the GMail IMAP TLS termination doesn't seem to send close_notify either. While the issue at a glance doesn't talk about the TLS version, it seems likely if using rustls that it negotiated TLS 1.3.)