feat(bindings): add external psk apis

[jmayclin]

Release Summary:

Add bindings for the External PSK functionality.

Description of changes:

This feature adds External PSK functionality to the s2n-tls bindings.

Update 2024-02-05: The refactor was a bit large, so I removed that from this PR and will be added it as a separate follow on.

Call-outs:

• I've always found the Resumption/PSK/SessionTicket stuff kinda confusing, so I named the main struct ExternalPSK instead of just PSK (Psk?).
• I made the HMAC parameter mandatory, because it seems like defaults always end up causing us trouble.
• OfferedPsk/OfferedPskList are both implemented in the callbacks module rather than the external_psk module which seems a little bit odd, but I think it's the option most consistent with the existing codebase.
• This PR doesn't add early data functionality to keep the size of the PR more reasonable. However all of the APIs are expected to neatly fit into the builder pattern.

Testing

Unit tests are added covering the new functionality. Additionally, I wrote examples showing how to use this functionality, but it was a bit large to fit into one PR so you can see that here.

New APIs

Just listing the new public APIs methods here, from cargo +stable public-api diff latest -p s2n-tls | grep "("

+pub fn s2n_tls::connection::Connection::append_psk(&mut self, psk: &s2n_tls::external_psk::Psk) -> core::result::Result<(), s2n_tls::error::Error>
+pub fn s2n_tls::connection::Connection::negotiated_psk_identity(&self, destination: &mut [u8]) -> core::result::Result<(), s2n_tls::error::Error>
+pub fn s2n_tls::connection::Connection::negotiated_psk_identity_length(&self) -> core::result::Result<usize, s2n_tls::error::Error>
+pub fn s2n_tls_sys::api::s2n_psk_hmac::Type::from(input: s2n_tls::enums::PskHmac) -> Self
+pub fn s2n_tls::enums::PskHmac::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result
+pub fn s2n_tls::enums::PskHmac::into(self) -> U
+pub fn s2n_tls::enums::PskHmac::try_from(value: U) -> core::result::Result<T, <T as core::convert::TryFrom<U>>::Error>
+pub fn s2n_tls::enums::PskHmac::try_into(self) -> core::result::Result<U, <U as core::convert::TryFrom<T>>::Error>
+pub fn s2n_tls::enums::PskHmac::type_id(&self) -> core::any::TypeId
+pub fn s2n_tls::enums::PskHmac::borrow(&self) -> &T
+pub fn s2n_tls::enums::PskHmac::borrow_mut(&mut self) -> &mut T
+pub fn s2n_tls::enums::PskHmac::from(t: T) -> T
+pub fn s2n_tls_sys::api::s2n_psk_mode::Type::from(input: s2n_tls::enums::PskMode) -> Self
+pub fn s2n_tls::enums::PskMode::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result
+pub fn s2n_tls::enums::PskMode::into(self) -> U
+pub fn s2n_tls::enums::PskMode::try_from(value: U) -> core::result::Result<T, <T as core::convert::TryFrom<U>>::Error>
+pub fn s2n_tls::enums::PskMode::try_into(self) -> core::result::Result<U, <U as core::convert::TryFrom<T>>::Error>
+pub fn s2n_tls::enums::PskMode::type_id(&self) -> core::any::TypeId
+pub fn s2n_tls::enums::PskMode::borrow(&self) -> &T
+pub fn s2n_tls::enums::PskMode::borrow_mut(&mut self) -> &mut T
+pub fn s2n_tls::enums::PskMode::from(t: T) -> T
+pub fn s2n_tls::external_psk::Builder::build(self) -> core::result::Result<s2n_tls::external_psk::Psk, s2n_tls::error::Error>
+pub fn s2n_tls::external_psk::Builder::new() -> core::result::Result<Self, s2n_tls::error::Error>
+pub fn s2n_tls::external_psk::Builder::with_hmac(&mut self, hmac: s2n_tls::enums::PskHmac) -> core::result::Result<&mut Self, s2n_tls::error::Error>
+pub fn s2n_tls::external_psk::Builder::with_identity(&mut self, identity: &[u8]) -> core::result::Result<&mut Self, s2n_tls::error::Error>
+pub fn s2n_tls::external_psk::Builder::with_secret(&mut self, secret: &[u8]) -> core::result::Result<&mut Self, s2n_tls::error::Error>
+pub fn s2n_tls::external_psk::Builder::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result
+pub fn s2n_tls::external_psk::Builder::into(self) -> U
+pub fn s2n_tls::external_psk::Builder::try_from(value: U) -> core::result::Result<T, <T as core::convert::TryFrom<U>>::Error>
+pub fn s2n_tls::external_psk::Builder::try_into(self) -> core::result::Result<U, <U as core::convert::TryFrom<T>>::Error>
+pub fn s2n_tls::external_psk::Builder::type_id(&self) -> core::any::TypeId
+pub fn s2n_tls::external_psk::Builder::borrow(&self) -> &T
+pub fn s2n_tls::external_psk::Builder::borrow_mut(&mut self) -> &mut T
+pub fn s2n_tls::external_psk::Builder::from(t: T) -> T
+pub fn s2n_tls::external_psk::Psk::builder() -> core::result::Result<s2n_tls::external_psk::Builder, s2n_tls::error::Error>
+pub fn s2n_tls::external_psk::Psk::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result
+pub fn s2n_tls::external_psk::Psk::drop(&mut self)
+pub fn s2n_tls::external_psk::Psk::into(self) -> U
+pub fn s2n_tls::external_psk::Psk::try_from(value: U) -> core::result::Result<T, <T as core::convert::TryFrom<U>>::Error>
+pub fn s2n_tls::external_psk::Psk::try_into(self) -> core::result::Result<U, <U as core::convert::TryFrom<T>>::Error>
+pub fn s2n_tls::external_psk::Psk::type_id(&self) -> core::any::TypeId
+pub fn s2n_tls::external_psk::Psk::borrow(&self) -> &T
+pub fn s2n_tls::external_psk::Psk::borrow_mut(&mut self) -> &mut T
+pub fn s2n_tls::external_psk::Psk::from(t: T) -> T

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[Mark-Simulacrum]

Some initial comments, I want to do another review but ran out of time for today.

[Mark-Simulacrum]

This type definition is pretty weird. *mut () already has !Send/!Sync impls... I guess the UnsafeCell is adding !Freeze, but that shouldn't ever matter -- after all, this is a pointer to the opaque type, right?

I guess this gets used as &Opaque/&mut Opaque? I'd sort of expect either c_void here or just struct $struct_name { _priv: () }.

[jmayclin]

I actually don't think that the UnsafeCell adds !Freeze.

Some interesting discussion here: sfackler/foreign-types#24
And also here: rust-lang/unsafe-code-guidelines#236 (comment)

I figured it was useful for documentation purposes. But I like the c_void approach. Will take that in the next iteration.

[Mark-Simulacrum]

I might call this next() since it's basically the Iterator API, just a lending iterator which borrows from &self.

[jmayclin]

I did consider that, but I worry that next is a bit leading because it doesn't imply the side effect of updating the "currently selected" PSK.

[Mark-Simulacrum]

Docs are stale (OfferedPskCursor vs OfferedPskListRef) -- but I'd avoid "Ref" in the type we expose to users, I'm not sure that's a good name.

[jmayclin]

Updated the docs.

I agree that it's a mouthful, but I expect that we will be using the pattern more going forward (like the OpenSSL crate does), and if we use this pattern for a pointer that has two ownership modes, then I think the Ref patten is the easiest way to distinguish them.

E.g. this pattern could be used for s2n_client_hello

// returns an owned struct with a drop impl that frees the `s2n_client_hello`
ClientHello::parse_client_hello -> ClientHello

// returns a "reference" created from the s2n_client_hello pointer. This can't be dropped, because the
// memory is owned by the connection. The "Ref" pattern ensures that.
connection.client_hello -> &ClientHelloRef

Although I can't imagine a scenario where we'd ever expose an owned OfferedPskList, so maybe the disambiguation isn't necessary? But the "Ref" approach seems to offer a bit more flexibility in the future, so I'm leaning towards that.

[Mark-Simulacrum]

If there were no offered PSKs, what is the behavior of calling choose_current_psk in the "-1" slot (i.e., immediately calling that on creation without calling next?).

[jmayclin]

This is covered in the choose_empty_psk test (renamed to choose_without_current_psk). A well-formed error is returned by s2n-tls, although it's unfortunately an "internal error": #5085.

[camshaft]

I think it would be better to get the mutable pointer directly rather than casting *const -> *mut.

[jmayclin]

Assumption: There is an intention that s2n-tls C API's be const correct, but we just aren't there yet.

Given that this takes a &Psk, I wanted a visible red flag that we are casting away the "logical constness" of the underlying data.

Let me know if you think the the friction is causing more noise than safety, and I'll go ahead and make as_s2n_ptr_mut take in &self and just add a comment about the safety.

[camshaft]

Yeah I worry about allowing *const -> *mut casts in general... I think it's better to just return the *mut directly from NonNull and have some safety comments around it all.