feat(bindings): enable application owned certs #4937

jmayclin · 2024-11-26T04:11:08Z

Release Summary:

Customers can now use application owned certs from the rust bindings. This allows rust consumers of s2n-tls to load certificates for many domains on a single config, and also allows certificates to be shared across a config.

Description of changes:

This PR builds on work in #4902

There are two main design point
CertificateChain reference counting: The CertificateChain struct is now internally reference counted. This allows applications to cheaply clone it and share it across configs.

Config->Context holds references: Instead of trying to retrieve reference counts from their storage on the C side, we store a handle to each reference counted certificate in the config->context. Then when the config and context are dropped, the container is as well which ends up decrementing all of the appropriate reference counts.

Call-outs:

API Naming

This PR adds three new public APIs

config::Builder::load_chain
- s2n_config_add_cert_chain_and_key_to_store
config::Builder::set_default_chains
- s2n_config_set_cert_chain_and_key_defaults
CertificateChain::from_pem
- s2n_cert_chain_and_key_load_pem_bytes
CertificateChain::from_public_pem
- s2n_cert_chain_and_key_load_public_pem_bytes

The CertificateChain apis were named to mirror the similar config apis - load_pem and load_public_pem.

Then the config APIs are hopefully noticeably different from the "direct pem" apis.
load_chain vs load_pem.

set_default_chains is a bit of an ugly name, but didn't happen upon anything better ☹️ . Very open to feedback/suggestions.

CertificateChain builder?

There is a notable departure from the C api with CertificateChain::load_pem.

In the C API, applications must first create the certificate chain and then call load_pem with it. That felt rather silly to do in rust. The choices were

match C behavior -> not recommended, unnecessarily tedious
builder pattern -> not chosen, seemed like overkill
single API initialization -> chosen, seemed nicest for customer.

Consolidation of Cert Loading

Note that we could switch config::Builder::load_pem to use "application" owned certs, by just creating the appropriate CertificateChain for the application and storing it on the config. This would consolidate out cert loading, and prevent the edge case where a customer tried to use both load_pem and add_to_store and receives an error.

We don't need to make this decision now, which is why I didn't do this in the PR.

Testing:

Unit tests were added covering reference counting behavior, and sanity checking some end to end workflows with default/non-default certs.

I am also working on porting the test_sni_matches integration test to rust so that we could benefit from the integration test coverage.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* rustfmt * clippy

bindings/rust/s2n-tls/src/cert_chain.rs

bindings/rust/s2n-tls/src/config.rs

* better variable naming * remove useless pub on sibling struct * correct typo in doc comment

To address the memory leak, we manually track ownership using a boolean toggle. We only _actually_ free the memory when the cert is "owned".

bindings/rust/s2n-tls/src/config.rs

camshaft · 2024-12-04T03:55:37Z

bindings/rust/s2n-tls/src/config.rs

+            .application_owned_certs
+            .extend(chains.clone());
+
+        let raw_certs: Vec<*mut s2n_cert_chain_and_key> = chains


Bummer that we have to allocate here... Is there not a way to append defaults?

Unfortunately there isn't a way to append defaults. Each call to set_defaults removes the previous defaults, and there is no separate "append" API.

But we can avoid the allocation as long as we are okay with duplicating a constant/some safety checks. This lets us just use an array on the stack instead of a vector. I'll go ahead and implement that in the next revision.

camshaft · 2024-12-04T03:56:57Z

bindings/rust/s2n-tls/src/config.rs

+    /// Corresponds to [s2n_config_set_cert_chain_and_key_defaults].
+    pub fn set_default_chains(
+        &mut self,
+        chains: Vec<CertificateChain<'static>>,


Does this need to be a Vec? Wonder if it would be more flexible as an IntoIterator<Item = CertificateChain>

It works, but does make things a tiny bit messier. Will make the change in the next revision.

xonatius · 2024-12-04T20:32:29Z

bindings/rust/s2n-tls/src/cert_chain.rs

 pub struct CertificateChain<'a> {
-    ptr: NonNull<s2n_cert_chain_and_key>,
-    is_owned: bool,
+    ptr: Arc<CertificateChainHandle>,
    _lifetime: PhantomData<&'a s2n_cert_chain_and_key>,
 }

 impl CertificateChain<'_> {


What's the plan on adding mutating APIs to CertificateChain? i.e s2n_cert_chain_and_key_set_ocsp_data

Thanks for the callout! I switched to using a builder pattern. Initial mutation is done on a single thread, with the rust type system enforcing a single owner.

The returned CertificateChain is immutable, and therefore can't be modified by any threads.

That makes sense, although with fully immutable chain and config updating OCSP staple would require rebuilding the whole config, which is not ideal, but I don't see a better option without introducing thread-safety to s2n-tls methods.

If you want to maintain the flexibility to modify things whenever, we could just make the application to handle thread safety.

Instead of making CertificateChain internally reference counted, it could be externally reference counted, Arc<CertificateChain>. So if customer wanted to mutate it from multiple threads they would need to use an Arc<Mutex<CertificateChain>> or something along those lines.

But the problem is that we'd have to give non-mutex version to s2n_config internally, at which point you can't safely mutate it again even if there is mutex on rust representation. The only way this will work is if C implementation has locks and is thread-safe.

Ya, I was thinking about this further and I don't think there is any safe way to let you mutate an OCSP on an already created certificate. Because there isn't any way for us to guarantee that e.g. s2n-tls isn't currently reading that OCSP data.

* use IntoIterator trait bound for set defaults * avoid heap allocation in set defaults * avoid useless increment/decrement of ref count

jmayclin · 2024-12-05T02:06:53Z

bindings/rust/s2n-tls/src/cert_chain.rs

+    ///
+    /// This can be used with [crate::config::Builder::load_chain] to share a
+    /// single cert across multiple configs.
+    pub fn load_pem(&mut self, chain: &[u8], key: &[u8]) -> Result<&mut Self, Error> {


I'm following our convention of builders returning &mut Self, but personally I like builders returning Self by value. Thoughts on whether it would be worth breaking from convention?

Also, I switched the API naming back to load_ because that felt more appropriate for a builder 🤷

lrstewart · 2024-12-05T18:45:35Z

bindings/rust/s2n-tls/src/cert_chain.rs

+            // SAFETY: manual audit of load_public_pem_bytes shows that `chain_pem`
+            // is not modified
+            // https://github.com/aws/s2n-tls/issues/4140
+            s2n_cert_chain_and_key_load_public_pem_bytes(
+                self.cert.as_mut_ptr(),
+                chain.as_ptr() as *mut _,
+                chain.len() as u32,
+            )
+            .into_result()


What happens if I call load_public, then load_public_pem? Should you keep your "from" methods, but they return builders instead of direct chains?

The same thing that happens if you call config::Builder::load_pem then config::Builder::load_public_pem. 😅

I think the consistency is valuable, and this method preserves a bit more future flexibility (although I can't think of a scenario where you'd have a single certificate chain with both ...)

If we just want strong validation, what do you think about adding validation/errors on the builder? We could return an explicit error such as

CertificateChain can only load one certificate chain. You already called {method}, so it is invalid to call {method}`

Consistency is a good argument :( You can't by definition have a chain that both has a private key and doesn't have a private key tho.

Not blocking, but it might actually be nice as a follow-up to add that validation error. It could also catch the case where we don't add any pem at all to the chain.

bindings/rust/s2n-tls/src/config.rs

bindings/rust/s2n-tls/src/cert_chain.rs

* add intermediate to avoid "undo" of cert chain refs * add comment for as_mut_ptr

jmayclin · 2024-12-06T23:27:51Z

bindings/rust/s2n-tls/src/config.rs

+        const CHAINS_MAX_COUNT: usize = 5;
+
+        let mut chain_arrays: [Option<CertificateChain<'static>>; CHAINS_MAX_COUNT] =
+            [None, None, None, None, None];


This is admittedly not pretty, but there aren't any good solutions because Option<CertificateChain<'static>> is not Copy, and we don't want to clone placeholder certs.

https://www.joshmcguigan.com/blog/array-initialization-rust/

I'd say still better than removing chains from the list on failure, so still a win?

tests/unit/s2n_certificate_test.c

bindings/rust/s2n-tls/src/config.rs

* fix typos in comment * adjust limit to match the C code

camshaft · 2024-12-09T19:11:58Z

Merging as this doesn't interact with the failing CRT test at all

feat(bindings): enable application owned certs

222a7c3

jmayclin marked this pull request as ready for review November 26, 2024 04:11

jmayclin requested review from lrstewart and camshaft November 26, 2024 04:11

jmayclin added 3 commits November 26, 2024 23:29

address ci failures

4174080

* rustfmt * clippy

add public cert creation

ab186e6

fix api names in doc comments

bbf6aa1

lrstewart approved these changes Dec 3, 2024

View reviewed changes

jmayclin added 2 commits December 4, 2024 03:07

address pr feedback

ce3b031

* better variable naming * remove useless pub on sibling struct * correct typo in doc comment

fix: memory leak from Arc

114a08d

To address the memory leak, we manually track ownership using a boolean toggle. We only _actually_ free the memory when the cert is "owned".

jmayclin requested a review from lrstewart December 4, 2024 03:25

camshaft reviewed Dec 4, 2024

View reviewed changes

xonatius reviewed Dec 4, 2024

View reviewed changes

jmayclin added 2 commits December 4, 2024 21:44

address pr feedback

1685837

* use IntoIterator trait bound for set defaults * avoid heap allocation in set defaults * avoid useless increment/decrement of ref count

style: remove useless enumerate

3a8efae

jmayclin requested a review from camshaft December 4, 2024 21:50

add cert chain builder

226fae1

jmayclin commented Dec 5, 2024

View reviewed changes

jmayclin requested a review from xonatius December 5, 2024 02:21

lrstewart reviewed Dec 5, 2024

View reviewed changes

xonatius approved these changes Dec 5, 2024

View reviewed changes

jmayclin requested a review from lrstewart December 6, 2024 21:27

lrstewart reviewed Dec 6, 2024

View reviewed changes

bindings/rust/s2n-tls/src/config.rs Outdated Show resolved Hide resolved

bindings/rust/s2n-tls/src/config.rs Outdated Show resolved Hide resolved

lrstewart reviewed Dec 6, 2024

View reviewed changes

bindings/rust/s2n-tls/src/cert_chain.rs Outdated Show resolved Hide resolved

camshaft approved these changes Dec 6, 2024

View reviewed changes

jmayclin added 2 commits December 6, 2024 23:20

address pr feedback

2eb0aa8

* add intermediate to avoid "undo" of cert chain refs * add comment for as_mut_ptr

update C unit test

a1efc16

jmayclin commented Dec 6, 2024

View reviewed changes

jmayclin requested a review from lrstewart December 6, 2024 23:28

lrstewart reviewed Dec 6, 2024

View reviewed changes

tests/unit/s2n_certificate_test.c Outdated Show resolved Hide resolved

bindings/rust/s2n-tls/src/config.rs Outdated Show resolved Hide resolved

bindings/rust/s2n-tls/src/config.rs Outdated Show resolved Hide resolved

bindings/rust/s2n-tls/src/config.rs Outdated Show resolved Hide resolved

address pr feedback

2dc960d

* fix typos in comment * adjust limit to match the C code

lrstewart approved these changes Dec 7, 2024

View reviewed changes

jmayclin added 2 commits December 7, 2024 00:28

fix pointer cast in renegotiate test

7c22c36

Merge branch 'main' into multi-cert-config

1fbce40

camshaft merged commit aaae641 into aws:main Dec 9, 2024
38 of 39 checks passed

jmayclin mentioned this pull request Dec 10, 2024

feat(bindings): enable multiple cert chains on config #4902

Closed

BrewTestBot mentioned this pull request Dec 17, 2024

s2n 1.5.10 Homebrew/homebrew-core#201462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bindings): enable application owned certs #4937

feat(bindings): enable application owned certs #4937

jmayclin commented Nov 26, 2024 •

edited

Loading

camshaft Dec 4, 2024

jmayclin Dec 4, 2024

camshaft Dec 4, 2024

jmayclin Dec 4, 2024

xonatius Dec 4, 2024

jmayclin Dec 5, 2024

xonatius Dec 5, 2024

jmayclin Dec 5, 2024

xonatius Dec 6, 2024

jmayclin Dec 6, 2024

jmayclin Dec 5, 2024 •

edited

Loading

lrstewart Dec 5, 2024

jmayclin Dec 5, 2024 •

edited

Loading

lrstewart Dec 6, 2024

jmayclin Dec 6, 2024

lrstewart Dec 6, 2024

camshaft commented Dec 9, 2024

feat(bindings): enable application owned certs #4937

feat(bindings): enable application owned certs #4937

Conversation

jmayclin commented Nov 26, 2024 • edited Loading

Release Summary:

Description of changes:

Call-outs:

API Naming

CertificateChain builder?

Consolidation of Cert Loading

Testing:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmayclin Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmayclin Dec 5, 2024 • 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

camshaft commented Dec 9, 2024

jmayclin commented Nov 26, 2024 •

edited

Loading

jmayclin Dec 5, 2024 •

edited

Loading

jmayclin Dec 5, 2024 •

edited

Loading