Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(bindings): expose context on cert chain #5132

Merged
merged 5 commits into from
Feb 26, 2025

Conversation

jmayclin
Copy link
Contributor

Description of changes:

This PR allows customer to set an application context on the CertificateChain object. This PR largely follows the approach that is used for Connection and it's associated context object.

Callouts

  • for the builder, we could store an Option<Box<dyn Any>> on the actual builder struct and then only associate it with the connection during the build step. This might make the "override" logic a bit simpler, at the cost of making the builder struct more complicated.

Testing:

I added unit tests which cover various context operations.

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

@github-actions github-actions bot added the s2n-core team label Feb 20, 2025
/// This API will override an existing application context set on the Builder.
///
/// Corresponds to [s2n_cert_chain_and_key_set_ctx].
pub fn set_application_context<T: Send + Sync + 'static>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only work when setting an application context on your own cert chain, right? Is there any reason you might want to set a context on a cert chain owned by s2n-tls? I wonder if this should be an API on the cert chain rather than the builder, similar to Connection::set_application_context()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, CertificateChain can be shared across threads, so we can't safely vend mutable access to the context unless we synchronize it/wrap it in a mutex. And even then, the mutex wouldn't be visible from the C side so it's possible that shenanigans in the C library might cause something to break.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good because 1) a cert chain owned by s2n-tls is owned by the config, so you could just set config context (if the Rust bindings support that yet?) 2) application-owned certs are kind of soft deprecated, and we recommend they not be used.

jmayclin and others added 2 commits February 20, 2025 15:14
* rename InternalContext to Context
* return err if context is set multiple times
* null cert context
@jmayclin jmayclin enabled auto-merge February 26, 2025 20:36
@jmayclin jmayclin added this pull request to the merge queue Feb 26, 2025
Merged via the queue into aws:main with commit 711ee0d Feb 26, 2025
46 checks passed
@jmayclin jmayclin deleted the feat-cert-chain-context branch February 26, 2025 23:21
dougch pushed a commit to dougch/s2n-tls that referenced this pull request Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants