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

Add Rust bindings method to create certs without private keys #3860

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

lrstewart
Copy link
Contributor

Description of changes:

There are two mutually exclusive approaches to configuring certs in s2n-tls. You can either set a single "s2n-owned" certificate of each type on the config via s2n_config_add_cert_chain_and_key, or you can set a variable number of "application-owned" certificates of each type via s2n_cert_chain_and_key_new and s2n_config_add_cert_chain_and_key_to_store. s2n-tls will manage the lifecycle of "s2n-owned" certs, but the application is responsible for managing "application-owned" certificates. s2n-tls currently prefers "application-owned", and the "s2n-owned" methods are marked deprecated.

The current method to load a certificate without a private key is only available from the "application-owned" approach: s2n_cert_chain_and_key_load_public_pem_bytes. However, the s2n-tls Rust bindings currently only use "s2n-owned" certificates, and don't offer the CertChainAndKey structure that would be required to support "application-owned" certificates.

Adding "application-owned" certificates to the bindings wouldn't be particularly easy. We'd need to add some C methods to retrieve the certificates associated with a config in order to properly implement Drop for CertChainAndKey. We'd also need to replace the current "s2n-owned" implementation of the existing methods, because you can't mix "s2n-owned" and "application-owned" so supporting both would be a bit of a foot gun. We'd either need to remove set_ocsp_data, accept that it's a sharp edge and will fail when combined with setting more than one cert, or find a way to make it inaccessible if the Config builder has a second cert set.

So as a faster, simpler solution, I've chosen to add an API to allow "s2n-owned" certificates to be created without public keys. I made it internal, so we should be able to remove it when/if we switch the bindings to "application-owned" certificates. However, there wouldn't really be any harm in adding it to the public API, so I could do that instead.

Testing:

Basic unit test for the new C method.
Switched the async pkey tests to use the new Rust method.

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 Mar 1, 2023
@lrstewart lrstewart marked this pull request as ready for review March 1, 2023 08:28
@lrstewart lrstewart requested review from camshaft and maddeleine March 1, 2023 08:28
Copy link
Contributor

@maddeleine maddeleine left a comment

Choose a reason for hiding this comment

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

suave

Copy link
Contributor

@camshaft camshaft left a comment

Choose a reason for hiding this comment

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

looks great!

@lrstewart lrstewart enabled auto-merge (squash) March 2, 2023 04:05
@lrstewart lrstewart merged commit e885b1b into aws:main Mar 2, 2023
@lrstewart lrstewart deleted the binding_cert_min branch March 2, 2023 06:07
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