From dcbd14dd3c94236fc16798944c40bec0f9ceab86 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Fri, 22 Mar 2024 01:18:51 +0000 Subject: [PATCH 1/6] feat(binding): add key update request --- bindings/rust/s2n-tls/Cargo.toml | 1 + bindings/rust/s2n-tls/src/connection.rs | 42 ++++++++++++++++ bindings/rust/s2n-tls/src/enums.rs | 16 ++++++ bindings/rust/s2n-tls/src/testing/s2n_tls.rs | 53 +++++++++++++++++++- 4 files changed, 111 insertions(+), 1 deletion(-) diff --git a/bindings/rust/s2n-tls/Cargo.toml b/bindings/rust/s2n-tls/Cargo.toml index c3ec4326f82..d949d6ead87 100644 --- a/bindings/rust/s2n-tls/Cargo.toml +++ b/bindings/rust/s2n-tls/Cargo.toml @@ -11,6 +11,7 @@ license = "Apache-2.0" [features] default = [] unstable-fingerprint = ["s2n-tls-sys/unstable-fingerprint"] +unstable-ktls = ["s2n-tls-sys/unstable-ktls"] quic = ["s2n-tls-sys/quic"] pq = ["s2n-tls-sys/pq"] testing = ["bytes"] diff --git a/bindings/rust/s2n-tls/src/connection.rs b/bindings/rust/s2n-tls/src/connection.rs index efbf2757afb..aef905fcaf3 100644 --- a/bindings/rust/s2n-tls/src/connection.rs +++ b/bindings/rust/s2n-tls/src/connection.rs @@ -252,6 +252,48 @@ impl Connection { Ok(self) } + /// Signals the connection to do a key_update at the next possible opportunity. + /// Note that the resulting key update message will not be sent until `send` is + /// called on the connection. + /// + /// `peer_request` indicates if a key update should also be requested + /// of the peer. When set to `KeyUpdateNotRequested`, then only the sending + /// key of the connection will be updated. If set to `KeyUpdateRequested`, then + /// the sending key of conn will be updated AND the peer will be requested to + /// update their sending key. Note that s2n-tls currently only supports + /// `peer_request` being set to `KeyUpdateNotRequested` and will return an error + /// if any other value is used. + pub fn request_key_update(&mut self, peer_request: PeerKeyUpdate) -> Result<&mut Self, Error> { + unsafe { + s2n_connection_request_key_update(self.connection.as_ptr(), peer_request.into()) + .into_result() + }?; + Ok(self) + } + + /// Reports the number of times sending and receiving keys have been updated. + /// + /// This only applies to TLS1.3. Earlier versions do not support key updates. + /// + /// s2n-tls only tracks up to u8::MAX (255) key updates. If this method + /// reports 255 updates, then more than 255 updates may have occurred. + /// + /// The return value is a tuple of `(send_key_updates, recv_key_updates)` + #[cfg(feature = "unstable-ktls")] + pub fn key_update_counts(&self) -> Result<(u8, u8), Error> { + let mut send_key_updates = 0; + let mut recv_key_updates = 0; + unsafe { + s2n_connection_get_key_update_counts( + self.connection.as_ptr(), + &mut send_key_updates, + &mut recv_key_updates, + ) + .into_result()?; + } + Ok((send_key_updates, recv_key_updates)) + } + /// sets the application protocol preferences on an s2n_connection object. /// /// protocols is a list in order of preference, with most preferred protocol first, and of diff --git a/bindings/rust/s2n-tls/src/enums.rs b/bindings/rust/s2n-tls/src/enums.rs index 5bc32d47b14..3116dfb2323 100644 --- a/bindings/rust/s2n-tls/src/enums.rs +++ b/bindings/rust/s2n-tls/src/enums.rs @@ -177,3 +177,19 @@ impl TryFrom for HashAlgorithm { Ok(version) } } + +#[non_exhaustive] +#[derive(Debug, PartialEq, Copy, Clone)] +pub enum PeerKeyUpdate { + KeyUpdateNotRequested, + KeyUpdatedRequested, +} + +impl From for s2n_peer_key_update::Type { + fn from(input: PeerKeyUpdate) -> s2n_peer_key_update::Type { + match input { + PeerKeyUpdate::KeyUpdateNotRequested => s2n_peer_key_update::KEY_UPDATE_NOT_REQUESTED, + PeerKeyUpdate::KeyUpdatedRequested => s2n_peer_key_update::KEY_UPDATE_REQUESTED, + } + } +} diff --git a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs index 8fcf7c86db6..c1f491c5057 100644 --- a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs +++ b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs @@ -234,7 +234,7 @@ impl<'a, T: 'a + Context> Callback<'a, T> { mod tests { use crate::{ callbacks::{ClientHelloCallback, ConnectionFuture}, - enums::ClientAuthType, + enums::{ClientAuthType, PeerKeyUpdate}, testing::{client_hello::*, s2n_tls::*, *}, }; use alloc::sync::Arc; @@ -892,4 +892,55 @@ mod tests { Ok(()) } + + #[test] + fn key_updates() -> Result<(), Error> { + let config = { + let config = config_builder(&security::DEFAULT_TLS13)?; + config.build()? + }; + + let server = { + let mut server = crate::connection::Connection::new_server(); + server.set_config(config.clone())?; + Harness::new(server) + }; + + let client = { + let mut client = crate::connection::Connection::new_client(); + client.set_config(config)?; + Harness::new(client) + }; + + let pair = Pair::new(server, client); + let mut pair = poll_tls_pair(pair); + + // there haven't been any key updates at the start of the connection + #[cfg(feature = "unstable-ktls")] + { + let client_updates = pair.client.0.connection.as_ref().key_update_counts()?; + assert_eq!(client_updates, (0, 0)); + let server_updates = pair.server.0.connection.as_ref().key_update_counts()?; + assert_eq!(server_updates, (0, 0)); + } + + pair.server + .0 + .connection + .as_mut() + .request_key_update(PeerKeyUpdate::KeyUpdateNotRequested)?; + assert!(pair.poll_send(Mode::Server, &[0]).is_ready()); + + // the server send key has been updated + #[cfg(feature = "unstable-ktls")] + { + println!("testing the key updates"); + let client_updates = pair.client.0.connection.as_ref().key_update_counts()?; + assert_eq!(client_updates, (0, 0)); + let server_updates = pair.server.0.connection.as_ref().key_update_counts()?; + assert_eq!(server_updates, (1, 0)); + } + + Ok(()) + } } From eaeabe21f6ec7a24de034c92e7ba8b9ee26cc439 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Mon, 25 Mar 2024 18:58:34 +0000 Subject: [PATCH 2/6] address pr feedback - return empty tuple for request key update - return non-exhaustive struct for key update count - simplify config builder - gate entire test on ktls feature --- bindings/rust/s2n-tls/src/connection.rs | 18 ++++++-- bindings/rust/s2n-tls/src/testing/s2n_tls.rs | 46 +++++++------------- 2 files changed, 29 insertions(+), 35 deletions(-) diff --git a/bindings/rust/s2n-tls/src/connection.rs b/bindings/rust/s2n-tls/src/connection.rs index aef905fcaf3..8f66e35333a 100644 --- a/bindings/rust/s2n-tls/src/connection.rs +++ b/bindings/rust/s2n-tls/src/connection.rs @@ -36,6 +36,13 @@ macro_rules! static_const_str { }; } +#[non_exhaustive] +#[derive(Debug, PartialEq, Default)] +pub struct KeyUpdateCount { + pub send_key_updates: u8, + pub recv_key_updates: u8, +} + pub struct Connection { connection: NonNull, } @@ -263,12 +270,12 @@ impl Connection { /// update their sending key. Note that s2n-tls currently only supports /// `peer_request` being set to `KeyUpdateNotRequested` and will return an error /// if any other value is used. - pub fn request_key_update(&mut self, peer_request: PeerKeyUpdate) -> Result<&mut Self, Error> { + pub fn request_key_update(&mut self, peer_request: PeerKeyUpdate) -> Result<(), Error> { unsafe { s2n_connection_request_key_update(self.connection.as_ptr(), peer_request.into()) .into_result() }?; - Ok(self) + Ok(()) } /// Reports the number of times sending and receiving keys have been updated. @@ -280,7 +287,7 @@ impl Connection { /// /// The return value is a tuple of `(send_key_updates, recv_key_updates)` #[cfg(feature = "unstable-ktls")] - pub fn key_update_counts(&self) -> Result<(u8, u8), Error> { + pub fn key_update_counts(&self) -> Result { let mut send_key_updates = 0; let mut recv_key_updates = 0; unsafe { @@ -291,7 +298,10 @@ impl Connection { ) .into_result()?; } - Ok((send_key_updates, recv_key_updates)) + Ok(KeyUpdateCount { + send_key_updates, + recv_key_updates, + }) } /// sets the application protocol preferences on an s2n_connection object. diff --git a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs index c1f491c5057..6a686c429a2 100644 --- a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs +++ b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs @@ -234,6 +234,7 @@ impl<'a, T: 'a + Context> Callback<'a, T> { mod tests { use crate::{ callbacks::{ClientHelloCallback, ConnectionFuture}, + connection::KeyUpdateCount, enums::{ClientAuthType, PeerKeyUpdate}, testing::{client_hello::*, s2n_tls::*, *}, }; @@ -893,36 +894,22 @@ mod tests { Ok(()) } + #[cfg(feature = "unstable-ktls")] #[test] fn key_updates() -> Result<(), Error> { - let config = { - let config = config_builder(&security::DEFAULT_TLS13)?; - config.build()? - }; - - let server = { - let mut server = crate::connection::Connection::new_server(); - server.set_config(config.clone())?; - Harness::new(server) + let empty_key_updates = KeyUpdateCount { + recv_key_updates: 0, + send_key_updates: 0, }; - let client = { - let mut client = crate::connection::Connection::new_client(); - client.set_config(config)?; - Harness::new(client) - }; - - let pair = Pair::new(server, client); + let pair = tls_pair(build_config(&security::DEFAULT_TLS13)?); let mut pair = poll_tls_pair(pair); // there haven't been any key updates at the start of the connection - #[cfg(feature = "unstable-ktls")] - { - let client_updates = pair.client.0.connection.as_ref().key_update_counts()?; - assert_eq!(client_updates, (0, 0)); - let server_updates = pair.server.0.connection.as_ref().key_update_counts()?; - assert_eq!(server_updates, (0, 0)); - } + let client_updates = pair.client.0.connection.as_ref().key_update_counts()?; + assert_eq!(client_updates, empty_key_updates); + let server_updates = pair.server.0.connection.as_ref().key_update_counts()?; + assert_eq!(server_updates, empty_key_updates); pair.server .0 @@ -932,14 +919,11 @@ mod tests { assert!(pair.poll_send(Mode::Server, &[0]).is_ready()); // the server send key has been updated - #[cfg(feature = "unstable-ktls")] - { - println!("testing the key updates"); - let client_updates = pair.client.0.connection.as_ref().key_update_counts()?; - assert_eq!(client_updates, (0, 0)); - let server_updates = pair.server.0.connection.as_ref().key_update_counts()?; - assert_eq!(server_updates, (1, 0)); - } + let client_updates = pair.client.0.connection.as_ref().key_update_counts()?; + assert_eq!(client_updates, empty_key_updates); + let server_updates = pair.server.0.connection.as_ref().key_update_counts()?; + assert_eq!(server_updates.recv_key_updates, 0); + assert_eq!(server_updates.send_key_updates, 1); Ok(()) } From b4610b23fa03bb176faa52f2bb85b49b07998264 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Mon, 25 Mar 2024 23:42:32 +0000 Subject: [PATCH 3/6] address ci failure - remove conditionally unused import --- bindings/rust/s2n-tls/src/connection.rs | 2 +- bindings/rust/s2n-tls/src/testing/s2n_tls.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/bindings/rust/s2n-tls/src/connection.rs b/bindings/rust/s2n-tls/src/connection.rs index 8f66e35333a..900289346b9 100644 --- a/bindings/rust/s2n-tls/src/connection.rs +++ b/bindings/rust/s2n-tls/src/connection.rs @@ -37,7 +37,7 @@ macro_rules! static_const_str { } #[non_exhaustive] -#[derive(Debug, PartialEq, Default)] +#[derive(Debug, PartialEq)] pub struct KeyUpdateCount { pub send_key_updates: u8, pub recv_key_updates: u8, diff --git a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs index 6a686c429a2..24e8878680d 100644 --- a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs +++ b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs @@ -235,7 +235,7 @@ mod tests { use crate::{ callbacks::{ClientHelloCallback, ConnectionFuture}, connection::KeyUpdateCount, - enums::{ClientAuthType, PeerKeyUpdate}, + enums::ClientAuthType, testing::{client_hello::*, s2n_tls::*, *}, }; use alloc::sync::Arc; @@ -897,6 +897,8 @@ mod tests { #[cfg(feature = "unstable-ktls")] #[test] fn key_updates() -> Result<(), Error> { + use crate::enums::PeerKeyUpdate; + let empty_key_updates = KeyUpdateCount { recv_key_updates: 0, send_key_updates: 0, From 871a913494d05cd46db28016b93f6afed6ca1ef8 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Mon, 25 Mar 2024 23:58:09 +0000 Subject: [PATCH 4/6] address ci failure - another unused import. --- bindings/rust/s2n-tls/src/testing/s2n_tls.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs index 24e8878680d..b5235d26684 100644 --- a/bindings/rust/s2n-tls/src/testing/s2n_tls.rs +++ b/bindings/rust/s2n-tls/src/testing/s2n_tls.rs @@ -234,7 +234,6 @@ impl<'a, T: 'a + Context> Callback<'a, T> { mod tests { use crate::{ callbacks::{ClientHelloCallback, ConnectionFuture}, - connection::KeyUpdateCount, enums::ClientAuthType, testing::{client_hello::*, s2n_tls::*, *}, }; @@ -897,7 +896,7 @@ mod tests { #[cfg(feature = "unstable-ktls")] #[test] fn key_updates() -> Result<(), Error> { - use crate::enums::PeerKeyUpdate; + use crate::{connection::KeyUpdateCount, enums::PeerKeyUpdate}; let empty_key_updates = KeyUpdateCount { recv_key_updates: 0, From e8f3551bf0ebe20bde2e139698c3052ec2b9d42d Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 26 Mar 2024 00:01:41 +0000 Subject: [PATCH 5/6] fix documentation --- bindings/rust/s2n-tls/src/connection.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/bindings/rust/s2n-tls/src/connection.rs b/bindings/rust/s2n-tls/src/connection.rs index 900289346b9..3c3bdb6efb5 100644 --- a/bindings/rust/s2n-tls/src/connection.rs +++ b/bindings/rust/s2n-tls/src/connection.rs @@ -38,6 +38,8 @@ macro_rules! static_const_str { #[non_exhaustive] #[derive(Debug, PartialEq)] +/// s2n-tls only tracks up to u8::MAX (255) key updates. If any of the fields show +/// 255 updates, then more than 255 updates may have occurred. pub struct KeyUpdateCount { pub send_key_updates: u8, pub recv_key_updates: u8, @@ -281,11 +283,6 @@ impl Connection { /// Reports the number of times sending and receiving keys have been updated. /// /// This only applies to TLS1.3. Earlier versions do not support key updates. - /// - /// s2n-tls only tracks up to u8::MAX (255) key updates. If this method - /// reports 255 updates, then more than 255 updates may have occurred. - /// - /// The return value is a tuple of `(send_key_updates, recv_key_updates)` #[cfg(feature = "unstable-ktls")] pub fn key_update_counts(&self) -> Result { let mut send_key_updates = 0; From 271f881aa4a7b8c27a01743b11b5fcb9ccd45c44 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 9 Apr 2024 22:29:05 +0000 Subject: [PATCH 6/6] address pr feedback - return &mut Self to stay consistent --- bindings/rust/s2n-tls/src/connection.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bindings/rust/s2n-tls/src/connection.rs b/bindings/rust/s2n-tls/src/connection.rs index e581e497290..7540b73fdbe 100644 --- a/bindings/rust/s2n-tls/src/connection.rs +++ b/bindings/rust/s2n-tls/src/connection.rs @@ -287,12 +287,12 @@ impl Connection { /// update their sending key. Note that s2n-tls currently only supports /// `peer_request` being set to `KeyUpdateNotRequested` and will return an error /// if any other value is used. - pub fn request_key_update(&mut self, peer_request: PeerKeyUpdate) -> Result<(), Error> { + pub fn request_key_update(&mut self, peer_request: PeerKeyUpdate) -> Result<&mut Self, Error> { unsafe { s2n_connection_request_key_update(self.connection.as_ptr(), peer_request.into()) .into_result() }?; - Ok(()) + Ok(self) } /// Reports the number of times sending and receiving keys have been updated.