When accepting new TLS connection with Accepted::into_connection(config) no TLS alert is sent

[vartiait2]

Checklist

• I've searched the issue tracker for similar bugs.

Describe the bug
When using Accepted::into_connection(config), no TLS alert is sent when an error occurs during the handshake.
E.g. TLS protocol version mismatch, a server is configured to support only TLS 1.3 and a client supports only TLS 1.2.

let mut acceptor = Acceptor::default();
let accepted = loop {
    acceptor.read_tls(stream)?;
    if let Some(accepted) = acceptor.accept()? {
        break accepted;
    }
};
// Use a fixed config
let mut conn = accepted.into_connection(config)?;
let (_bytes_read, _bytes_written) = conn.complete_io(&mut stream)?;

When using ServerConnection::new(config), TLS alert is sent.

// Use a fixed config
let mut conn = ServerConnection::new(config)?;
let (_bytes_read, _bytes_written) = conn.complete_io(&mut stream)?;

To Reproduce
Steps to reproduce the behavior:

1. Accept new TLS connections with Accepted::into_connection(config) (e.g. with a config supporting only TLS 1.3)
2. Try to do a handshake supporting only TLS 1.2
   openssl s_client -connect 127.0.0.1:8443 -tls1_2
3. No TLS alert is received
   CONNECTED(00000003) C0C71E44F87F0000:error:0A000126:SSL routines:ssl3_read_n:unexpected eof while reading:ssl/record/rec_layer_s3.c:304:

When accepting connections with ServerConnection::new(config), TLS alert is sent
CONNECTED(00000003) C0C71E44F87F0000:error:0A00042E:SSL routines:ssl3_read_bytes:tlsv1 alert protocol version:ssl/record/rec_layer_s3.c:1586:SSL alert number 70

Applicable Version(s)
0.22.2

Expected behavior
TLS alert is sent to a client.

[djc]

I spent some time looking at this on Saturday. It's not obvious to me why this happens from looking at the Acceptor/Accepted/server handshake code, but maybe it's an interaction with how complete_io() calls things?

[vartiait2]

Is it because Accepted::into_connection(config) returns with an error result after calling ExpectClientHello::with_certified_key(self, ...)? at src/server/server_conn.rs#L796?
The error itself is propagated from src/server/hs.rs#L278

[djc]

Hmm, yes. So if an error occurs in process_new_packets(), complete_io() will still call write_tls() (exactly for the purpose of getting out the alert). However, in your acceptor-based code, you'd get an error value from Accepted::into_connection(), and you'd likely not call complete_io() after that.

So I think this is technically an error in your code, but it's also a bit of a pitfall with the Acceptor setup and likely something we should at least clearly document. Or maybe we should have a Stream-like Acceptor wrapper?

[ctz]

If an alert is queued for sending during Accepted::into_connection I think there is no way for a caller to get it? They don't have the original Accepted, and don't get a ServerConnection because they get an error instead. Maybe the Err return type of that should be struct ErrorAndAlert(Error, Vec<u8>) or something.

[djc]

Ahh, that's fair.

[djc]

I suppose the Err type could be a wrapper around the ConnectionCommon<ServerData> exposes a write_alert() method in addition to the actual Error?

[vartiait2]

Would a following change in Accepted::into_connection(config) be an adequate fix?

diff --git a/rustls/src/server/server_conn.rs b/rustls/src/server/server_conn.rs
index fe1569ca..80d4d194 100644
--- a/rustls/src/server/server_conn.rs
+++ b/rustls/src/server/server_conn.rs
@@ -798,9 +798,9 @@ impl Accepted {
             Self::client_hello_payload(&self.message),
             &self.message,
             &mut cx,
-        )?;
+        );
 
-        self.connection.replace_state(new);
+        self.connection.core.state = new;
         Ok(ServerConnection {
             inner: self.connection,
         })

At least it seems to handle the error case properly. Calling complete_io(...) on ServerConnection handles sending TLS alert and returning with an error result.

[djc]

Sketched out a fix in #1811.

[vartiait2]

@djc Thanks a lot!

Isn't returning AcceptedAlert along with an error from Acceptor::accept() a bit too soon as Acceptor does not
have ServerConfig?

In an example code snippet below, TLS alert should be sent because local ServerConfig is configured not to support TLS 1.2 (which the client is trying to use).

let mut acceptor = Acceptor::default();
let accepted = loop {
    acceptor.read_tls(stream)?;
    if let Some(accepted) = acceptor.accept()? {
        break accepted;
    }
};
// Use a fixed config
let mut conn = accepted.into_connection(config)?;
let (_bytes_read, _bytes_written) = conn.complete_io(&mut stream)?;

[djc]

Ah, yes -- I revised the PR to also change the error type for Accepted::into_connection().

[cpu]

@vartiait2 Thanks for the detailed bug report. This should be fixed with #1811 and included in the upcoming release (#1777)

[vartiait2]

@cpu @djc Thank You very much for fixing this! 👍🏻 I'm actually using tokio_rustls::LazyConfigAcceptor and tokio_rustls::StartHandshake so have to wait until the updated function signatures are used there.

[cpu]

@vartiait2 Would you be interested in writing a PR for tokio-rustls to bring those API updates over?

[vartiait2]

Sure 👍🏻

[cpu]

@vartiait2 Ah, looks like ctz beat you to it: rustls/tokio-rustls#44

[ctz]

Oops! Sorry about that.

[vartiait2]

No problem! 😄 Thanks a lot @ctz for a quick integration update! 👍🏻