Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected behavior of client without certificate on mTLS #83

Closed
MattesWhite opened this issue Aug 30, 2024 · 13 comments
Closed

Unexpected behavior of client without certificate on mTLS #83

MattesWhite opened this issue Aug 30, 2024 · 13 comments

Comments

@MattesWhite
Copy link

Hi there,

I'm currently working on establishing mTLS connections using this crate. My problem is that my program may connect to a TLS server that requires client auth and therefore a client certificate or not. Therefore, it is also possible to not provide a client certificate as the targeted TLS server might not require one.

All in all, there can be the case that the TLS server requires a client certificate but the client does not send one. I expected that in this case TlsConnector::connect() would fail but instead it connects successfully, immediate writes work as well and only after about 50ms I get an error on sending with: IoFailure(Os { code: 104, kind: ConnectionReset, message: "Connection reset by peer" })

I tried the same scenario with openssl s_client and it fails immediately with: 40774BE4047F0000:error:0A00045C:SSL routines:ssl3_read_bytes:tlsv13 alert certificate required:../ssl/record/rec_layer_s3.c:1586:SSL alert number 116. So it seems possible to detect the problem without waiting for a write to fail.

Is this a bug or intended behavior on your side? How should such a case be handled? And what happened to the first data of the first succeeded write? In wireshark it appears that that data was never written to the wire.

@djc
Copy link
Member

djc commented Sep 2, 2024

That does sound surprising -- I would expect that in this scenario the Future impl for Connect would yield Err which I think you're saying is not what happens in practice? Do you have a complete client reproduction? What server are you testing against?

@ctz
Copy link
Member

ctz commented Sep 2, 2024

Going to assume this is TLS1.3, going by the openssl error message.

The TLS1.3 handshake looks like:


       Client                                           Server

Key  ^ ClientHello
Exch | + key_share*
     | + signature_algorithms*
     | + psk_key_exchange_modes*
     v + pre_shared_key*       -------->
                                                  ServerHello  ^ Key
                                                 + key_share*  | Exch
                                            + pre_shared_key*  v
                                        {EncryptedExtensions}  ^  Server
                                        {CertificateRequest*}  v  Params
                                               {Certificate*}  ^
                                         {CertificateVerify*}  | Auth
                                                   {Finished}  v
                               <--------  [Application Data*]
[1]  ^ {Certificate*} 
Auth | {CertificateVerify*}
     v {Finished}              -------->
[2]    [Application Data]      <------->  [Application Data]

(ref https://datatracker.ietf.org/doc/html/rfc8446)

The line I marked [1] is the point at which the client reveals to the server whether it has a client certificate. The line I marked [2] is the point at which the handshake is complete from the point of view of the client, and it is 100% free to start writing data. Observe that there is no round-trip to the server that confirms whether the server has accepted the certificate; this is signaled only with an alert, and that is received asynchronously by the client.

However, I would expect the client to fail after one RTT with something like rustls::Error::AlertReceived(AlertDescription::CertificateRequired) (our equivalent to the openssl error), and I'm a little concerned that this isn't being surfaced in your example.

@MattesWhite
Copy link
Author

I created a fork of the repo and added a test to reproduce my scenario: https://github.com/MattesWhite/tokio-rustls/blob/matteswhite/unexpected-mtls-client/tests/mtls-without-client-cert.rs#L93
So when running against a tokio-rustls server with mTLS I don't get the error I expected. I also tried connecting with an rsyslog server with mTLS that showed the same behavior on the client side.

@ctz
Copy link
Member

ctz commented Sep 2, 2024

I can reproduce a similar but different misbehaviour with the example in this repo.

Server is:

~/src/rustls/examples$ cargo run --bin tlsserver-mio -- --certs ../test-ca/rsa-2048/end.fullchain --key ../test-ca/rsa-2048/end.key --require-auth --auth ../test-ca/rsa-2048/ca.cert -p8443 --verbose http
~/src/tokio-rustls$ cargo run --example client localhost -p 8443 --cafile ~/src/rustls/test-ca/rsa-2048/ca.cert
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/examples/client localhost -p 8443 --cafile /home/jbp/src/rustls/test-ca/rsa-2048/ca.cert`

At this point, the server has rejected the connection, but the client is still hanging around waiting on stdin. Then if I type "e\n":

e
Error: Custom { kind: InvalidData, error: AlertReceived(CertificateRequired) }

Even more damning is strace output:

recvfrom(9, "\27\3\3\0\23\20\320\331\212\211\272u\311\354\10O\255\267\27\220N\300\253M", 4096, 0, NULL, NULL) = 24

^ this is the alert coming in.

epoll_ctl(5, EPOLL_CTL_DEL, 9, NULL)    = 0
fcntl(9, F_GETFD)                       = 0x1 (flags FD_CLOEXEC)
close(9)                                = 0
write(4, "\1\0\0\0\0\0\0\0", 8)         = 8
futex(0x75bbe5000660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbe4400660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbe4800660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbe4c00660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbdfe00660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbdfa00660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbdf600660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbdf200660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbdee00660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbdea00660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbde600660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbde200660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbdde00660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbdda00660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbdd600660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbe5400660, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x75bbe59d3120, FUTEX_WAIT_PRIVATE, 1, NULL

But we go to sleep waiting for stdin!

More logging:

[src/common/mod.rs:198:19] self.read_io(cx) = Pending
[src/common/mod.rs:95:23] self.session.read_tls(&mut reader) = Ok(
    24,
)
[src/common/mod.rs:198:19] self.read_io(cx) = Ready(
    Err(
        Custom {
            kind: InvalidData,
            error: AlertReceived(
                CertificateRequired,
            ),
        },
    ),
)
<blocks on stdin anyway>

So the error does make it out, but then apparently tokio blocks main from exiting because it cannot cancel reading from stdin (tokio-rs/tokio#589, tokio-rs/tokio#2318)

@ctz
Copy link
Member

ctz commented Sep 2, 2024

I created a fork of the repo and added a test to reproduce my scenario:

Thanks!

I think there is no point in your program where it can receive a post-handshake alert. These two points are before that:

https://github.com/MattesWhite/tokio-rustls/blob/34f56d6c270df0cbc22658a9ad5115dc5bc59c0b/tests/mtls-without-client-cert.rs#L98-L101

and

https://github.com/MattesWhite/tokio-rustls/blob/34f56d6c270df0cbc22658a9ad5115dc5bc59c0b/tests/mtls-without-client-cert.rs#L103

If I add:

diff --git a/tests/mtls-without-client-cert.rs b/tests/mtls-without-client-cert.rs
index 1586543..53821ae 100644
--- a/tests/mtls-without-client-cert.rs
+++ b/tests/mtls-without-client-cert.rs
@@ -101,6 +101,7 @@ async fn connect_to_mtls_without_client_cert() {
         .unwrap(); // I'd expect to fail the test here
 
     tls.write(b"123").await.unwrap();
+    tls.read(&mut [0u8]).await.unwrap();
 
     let wait_for = store.wait_for(|store| store.len() >= 3);
     timeout(Duration::from_millis(500), wait_for)

Then things start working.

@MattesWhite
Copy link
Author

Okay so to check if I understand you correctly:

  • The client authentication has no response from the server so there is no chance to detect the error (missing certificate) on the connect() method.
  • There is an asynchronous TLS alert from the server but that can happen at any time after the handshake.
  • Therefore, I need to do some kind of operation on the stream to make it possible to receive the TLS alert from the server, e.g. tls.read()

The thing is that in my case I don't expect any data from the server I only stream data towards it. Accordingly, I expect the read() to never resolve if everything works. So I guess the best thing I can do is to use a sufficiently long timeout on the read() to check if everything worked? Seems not very ensuring considering the unknown network conditions in between but I guess it's the best effort approach.

@ctz
Copy link
Member

ctz commented Sep 3, 2024

I would recommend doing this:

  • write your data
  • call and await shutdown() -- this closes the write direction and signals to the server no more data will come
  • call and await read() -- it should return Ok(0) -- this waits for the server to close its write direction, and confirms the TLS session was successful

@MattesWhite
Copy link
Author

My solution for now is to do 500ms timeout on an initial read(&mut [0]) after the connection succeeded. This should be enough time to catch any TLS alerts the server sends on client authentication. And only after the timeout is elapsed I start writing data to the TLS stream.

The 500ms timeout is somewhat arbitrary as I don't know how long the roundtrip from client to server takes but I deemed it good enough.

BTW I also noticed that every other problem with client authentication, e.g. untrusted client certificate, is also only reported via TLS alerts.

@djc
Copy link
Member

djc commented Sep 4, 2024

Why not take @ctz's approach as outlined in the his previous comment? Would avoid the need for timeouts.

@MattesWhite
Copy link
Author

Maybe I didn't understand ctz's approach. My problem is that I want to know if client auth was successful before writing any data to give users an error before they can try to send data because all data written before client auth failed is lost somewhere in the stack.

Also as I understood ctz's approach is about closing the connection but my problem is about establishing the connection.

If I got it wrong I'm sorry maybe you can explain it to me in more detail?

@djc
Copy link
Member

djc commented Sep 4, 2024

Nope, you're correct that @ctz's approach relies on unconditionally writing the data.

@ctz
Copy link
Member

ctz commented Sep 4, 2024

My solution for now is to do 500ms timeout on an initial read(&mut [0]) after the connection succeeded. This should be enough time to catch any TLS alerts the server sends on client authentication. And only after the timeout is elapsed I start writing data to the TLS stream.

That seems reasonable, on the proviso that that server should immediately close the stream (that will make your read complete on success after 1RTT, rather than time out).

@MattesWhite
Copy link
Author

Okay, I have now a working, good-enough solution. Thank you both for your time and effort to help me 👍.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants