feat(bindings): enable multiple cert chains on config #4902
Conversation
Adds ref counting and load_pem to cert chain. Makes use of newer C functions that default to app owned instead of lib owned. Config builder extended to make use of cert_chain instead of only creating certs. This will enable multiple cert_chains. Still work in progress. Need to implement C function to get all cert chain and keys on the config. Need to add unit tests.
| // original reference prevents other threads from erroneously deleting | ||
| // the object. | ||
| // https://github.com/rust-lang/rust/blob/e012a191d768adeda1ee36a99ef8b92d51920154/library/alloc/src/sync.rs#L1329 | ||
| let _count = context.refcount.fetch_add(1, Ordering::Relaxed); |
There was a problem hiding this comment.
Clone and drop are nearly the same as Config
| // count, which happens before this fence, which happens before the | ||
| // deletion of the data. | ||
| // https://github.com/rust-lang/rust/blob/e012a191d768adeda1ee36a99ef8b92d51920154/library/alloc/src/sync.rs#L1637 | ||
| std::sync::atomic::fence(Ordering::Acquire); |
There was a problem hiding this comment.
I'm not sure if this fence is actually required. I followed drop from Config here but Config is a bit more complicated than cert_chain so it's possible this isn't needed.
There was a problem hiding this comment.
Hey Nathan! Thanks so much for the PR! Especially since this is a pretty ticky one.
At a high level, I'd prefer to see an approach that focused on having the main add_cert_chain api accept an Arc<CertificateChain>, as I worry that other approaches are going to be very tricky for library consumers to use correctly.
I'd actually advise that you hold off on addressing any comments for now, because I'd like to chat with the team about whether we can flex (break 😬)the existing apis a little bit, because I think a different CertificateChain struct would make this feature much easier to deliver.
I'll let you know once I've talked with my team and we'll go from there!
Release Summary:
add_cert_chainto allow more than one CertificateChain (s2n_cert_chain_and_key in C)cloneto CertificateChain to allow multiple configs to share CertificateChainsResolved issues:
N/A did not see any issues for this
Description of changes:
Prior to this change the only way of using the bindings was to use the older
s2n_config_add_cert_chain_and_keyC code that sets S2N ownership. This change is to allow use of the newers2n_config_add_cert_chainwhich does not set S2N ownership of the cert. This PR does not change any existing C functionality and the older rust functions are still usable. The C code added is to implement drop on config and CertificateChain.This change is needed to support more than one cert chain while using Rust.
Call-outs:
load_public_pemwas not added on CertificateChain but would likely need to be implemented in the future. I left it out as it would need more C code and can be added independently of this PR.I went with documentation about which Config Builder functions can be used together. The setters will fail due to the C code checking the condition. LMK if you think there is a better way to communicate when to pick one function over the other.
from_owned_ptr_referencevsfrom_ptr_referencerequires knowing the implementation details of the ownership. Not sure how to get around that and still keep backwards compatibility.I did not create a builder for CertificateChain as it seemed a bit superfluous since the build would not be able to validate anything.
Testing:
Used the following to execute the tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.