Add OpenSSL TLS 1.3 PSK callbacks #1993

drbrain · 2023-07-08T00:49:29Z

For TLS 1.3 OpenSSL introduced new functions SSL_CTX_set_psk_use_session_callback and SSL_CTX_set_psk_find_session_callback to configure a pre-shared key. I chose to wrap both the SSL_CTX_psk… and SSL_psk… versions of these functions.

One thing I am not certain of is SSL_CTX_set_psk_use_session_callback says:

On successful completion the callback must store a pointer to an identifier for the PSK in *id. The identifier length in bytes should be stored in *idlen. The memory pointed to by *id remains owned by the application and should be freed by it as required at any point after the handshake is complete.

I used ssl.set_ex_data() to store the ID in the SslRef, but I don't know if this is the correct way to do that.

The notes for the new functions state:

A connection established via a TLSv1.3 PSK will appear as if session resumption has occurred so that SSL_session_reused will return true.

But this does not to be true with a minimally-configured session. Wireshark shows that the PSK is accepted and that only 1 round-trip is required to create a connection.

Output for ssl::test::psk_tls13_callbacks (4 total):

No.     Src Port Dest Port Info
      5 59642    59641     Client Hello

Frame 5: 334 bytes on wire (2672 bits), 334 bytes captured (2672 bits) on interface lo0, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59642, Dst Port: 59641, Seq: 1, Ack: 1, Len: 278
Transport Layer Security

No.     Src Port Dest Port Info
      7 59641    59642     Server Hello, Change Cipher Spec, Application Data, Application Data, Application Data, Application Data

Frame 7: 1411 bytes on wire (11288 bits), 1411 bytes captured (11288 bits) on interface lo0, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59641, Dst Port: 59642, Seq: 1, Ack: 279, Len: 1355
Transport Layer Security

No.     Src Port Dest Port Info
      9 59642    59641     Change Cipher Spec, Application Data

Frame 9: 136 bytes on wire (1088 bits), 136 bytes captured (1088 bits) on interface lo0, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59642, Dst Port: 59641, Seq: 279, Ack: 1356, Len: 80
Transport Layer Security

No.     Src Port Dest Port Info
     11 59641    59642     Application Data

Frame 11: 79 bytes on wire (632 bits), 79 bytes captured (632 bits) on interface lo0, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59641, Dst Port: 59642, Seq: 1356, Ack: 359, Len: 23
Transport Layer Security

Output for ssl::test::ssl_verify_callback (6 total, Application Data in frames 27 and 29 are the TLS 1.3 handshake):

No.     Src Port Dest Port Info
     21 59644    59643     Client Hello

Frame 21: 353 bytes on wire (2824 bits), 353 bytes captured (2824 bits) on interface lo0, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59644, Dst Port: 59643, Seq: 1, Ack: 1, Len: 297
Transport Layer Security

No.     Src Port Dest Port Info
     23 59643    59644     Server Hello, Change Cipher Spec, Application Data, Application Data, Application Data, Application Data

Frame 23: 1411 bytes on wire (11288 bits), 1411 bytes captured (11288 bits) on interface lo0, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59643, Dst Port: 59644, Seq: 1, Ack: 298, Len: 1355
Transport Layer Security

No.     Src Port Dest Port Info
     25 59644    59643     Change Cipher Spec, Application Data

Frame 25: 136 bytes on wire (1088 bits), 136 bytes captured (1088 bits) on interface lo0, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59644, Dst Port: 59643, Seq: 298, Ack: 1356, Len: 80
Transport Layer Security

No.     Src Port Dest Port Info
     27 59643    59644     Application Data

Frame 27: 311 bytes on wire (2488 bits), 311 bytes captured (2488 bits) on interface lo0, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59643, Dst Port: 59644, Seq: 1356, Ack: 378, Len: 255
Transport Layer Security

No.     Src Port Dest Port Info
     29 59643    59644     Application Data

Frame 29: 311 bytes on wire (2488 bits), 311 bytes captured (2488 bits) on interface lo0, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59643, Dst Port: 59644, Seq: 1611, Ack: 378, Len: 255
Transport Layer Security

No.     Src Port Dest Port Info
     31 59643    59644     Application Data

Frame 31: 79 bytes on wire (632 bits), 79 bytes captured (632 bits) on interface lo0, id 0
Null/Loopback
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59643, Dst Port: 59644, Seq: 1866, Ack: 378, Len: 23
Transport Layer Security

The above output can be reproduced by running Wireshark on the loopback interface then applying the tls filter.

Per the description of the above callbacks this also requires wrapping:

sfackler · 2023-07-08T22:28:32Z

openssl/src/ssl/callbacks.rs

+        + Send,
+{
+    unsafe {
+        let psk_idx = Ssl::cached_ex_index::<Vec<u8>>();


I'd prefer to use a newtype for this index to avoid potential collisions with other APIs that need to store a Vec<u8>.

From the documentation it sounds like the callback can be invoked twice for a single session. Does the PSK ID from the first call need to be preserved after the second call?

I'd prefer to use a newtype for this index to avoid potential collisions with other APIs that need to store a Vec<u8>.

OK

From the documentation it sounds like the callback can be invoked twice for a single session. Does the PSK ID from the first call need to be preserved after the second call?

I think, only until the handshake completes. My understanding is that I am keeping it for the lifetime of the SSL struct which is not the best. If the SSL_CTX_set_info_callback function were wrapped rust-openssl could see the SSL_CB_HANDSHAKE_DONE event and free the PSK ID, but that would need to be transparent to the end-user using the callback which seems like unwanted complexity

From the documentation it sounds like the callback can be invoked twice for a single session. Does the PSK ID from the first call need to be preserved after the second call?

OpenSSL says:

The PSK returned by the callback is allowed to be different between the first and second time it is called.

So I think the flow is:

First callback invocation, md is null. Returns PSK "P1", using cipher "C"

OpenSSL does stuff, finds cipher "C" is not acceptable, but "D" is acceptable

Second callback invocation, md forces cipher "D". Returns PSK "P2" using cipher "D"

Handshake completes

Which I think means the "P1" PSK ID can be dropped. I haven't dug through any OpenSSL code yet to confirm.

I did add a newtype for the PSK storage in 6a772d1

I found the callback is called in OpenSSL 1.1.1 when constructing the early data

It looks like the callback can be called twice if there is a "hello retry request".

A little further down I see the PSK id is copied into SSL * with OPENSSL_memdup() and is no longer mentioned in the function. The old PSK id is freed.

I suspect that the documentation is written as it is to prevent an FFI wrapper from freeing the memory before the OpenSSL side of the callback can complete.

Elsewhere OpenSSL notes in a comment than only one PSK can be sent, so together I think it is safe to replace the PSK id from the first invocation with the PSK id from the second invocation.

sfackler · 2023-07-08T22:35:31Z

openssl/src/ssl/mod.rs

+    pub fn find<'a>(ssl: &'a SslRef, cipher_id: &[u8]) -> Result<&'a SslCipherRef, ErrorStack> {
+        unsafe {
+            let ssl = ssl.as_ptr();
+            let ptr = ffi::SSL_CIPHER_find(ssl, cipher_id.as_ptr());


Something needs to ensure that cipher_id is (at least) two bytes.

My understanding of the documentation was that OpenSSL would set an error if the cipher ID was invalid for any reason, I'll run some tests to check that assumption.

Can you point me toward an example of the type of error I should return if the cipher ID has an invalid format?

Because SSL_CIPHER_find() doesn't seem to set internal OpenSSL errors I switched the return type from a Result<> to Option<>. I also added a length check to prevent returning a cipher for a too-short or too-long ID.

0e0f76b

sfackler · 2023-07-08T22:36:26Z

openssl/src/ssl/mod.rs

+    /// This could be used to set up a session-based PSK.  You must ensure the length of the key is
+    /// suitable for the ciphersuite associated with the session.
+    #[corresponds(SSL_SESSION_set1_master_key)]
+    pub fn set_master_key(&mut self, key: &[u8]) -> Result<(), ErrorStack> {


Sessions are reference counted so it's not sound to directly mutate them. We'll need to define a session builder like we have for SslContext instead.

In 4565f0f I added SslContextBuilder modeled on the other builders.

Previously input length was unchecked. OpenSSL still returned NULL on a too-short inuput, but returned a value on a too-long input. OpenSSL doesn't add an error internally if a SSL_CIPHER_find() fails so an Option<> seems better. SslCipherRef::find() now checks to ensure the length of the cipher ID is exactly 2 to avoid too-long cipher IDs matching.

drbrain · 2023-07-17T21:20:11Z

Can you give a clue on how to exclude SSL_CIPHER_find on OpenSSL 1.0.1 and libressl 2.5.5 to get the tests to pass? This is close, but I suspect there's something different I should be doing:

extern "C" {
    #[cfg(any(not(ossl102), libressl273, not(boringssl)))]
    pub fn SSL_CIPHER_find(ssl: *mut SSL, ptr: *const c_uchar) -> *const SSL_CIPHER;

drbrain · 2023-08-09T21:16:09Z

Can you give me an idea of how to exclude SSL_CIPHER_find on some implementations ↑ ?

Add OpenSSL TLS 1.3 PSK callbacks

4319f96

sfackler reviewed Jul 8, 2023

View reviewed changes

drbrain added 4 commits July 14, 2023 15:15

Replace SslSession::new() with SslSessionBuilder::new()

4565f0f

Use Psk newtype to hold PSK id

6a772d1

Fix clippy

8815e7a

drbrain force-pushed the drbrain/openssl-TLS1.3-PSK-callbacks branch from 4b6b7bc to eef1928 Compare July 17, 2023 20:20

drbrain added 2 commits July 17, 2023 13:47

Fix versions where SSL functions are available

36f5fe9

Find a cipher that exists on older OpenSSL

5529c79

drbrain force-pushed the drbrain/openssl-TLS1.3-PSK-callbacks branch from eef1928 to 5529c79 Compare July 17, 2023 20:47

drbrain requested a review from sfackler July 17, 2023 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenSSL TLS 1.3 PSK callbacks #1993

Add OpenSSL TLS 1.3 PSK callbacks #1993

drbrain commented Jul 8, 2023

sfackler Jul 8, 2023

sfackler Jul 8, 2023

drbrain Jul 10, 2023

drbrain Jul 14, 2023 •

edited

Loading

drbrain Jul 15, 2023

sfackler Jul 8, 2023

drbrain Jul 10, 2023

drbrain Jul 14, 2023

sfackler Jul 8, 2023

drbrain Jul 10, 2023

drbrain Jul 14, 2023

drbrain commented Jul 17, 2023

drbrain commented Aug 9, 2023

Add OpenSSL TLS 1.3 PSK callbacks #1993

Are you sure you want to change the base?

Add OpenSSL TLS 1.3 PSK callbacks #1993

Conversation

drbrain commented Jul 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drbrain Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drbrain commented Jul 17, 2023

drbrain commented Aug 9, 2023

drbrain Jul 14, 2023 •

edited

Loading