-
Notifications
You must be signed in to change notification settings - Fork 30
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
0.14.0-rc1: Adopt crypto provider API, use aws-lc-rs as default provider #441
0.14.0-rc1: Adopt crypto provider API, use aws-lc-rs as default provider #441
Conversation
d385354
to
d29400b
Compare
I recommend reviewing this commit-by-commit. I've taken extra care to make sure each commit builds/tests cleanly along the way. |
I put together a rough proof of concept for this. I don't plan to "productionize" it at this time, but it was sufficient for proving the concept. rustls-post-quantum-ffi is a new crate that I made that wraps up the Rust I used that to quickly extend the rustls-ffi With that in place we can build
|
Here's a WIP branch. For the time being I've left all of the |
d29400b
to
b07551e
Compare
This one needs a bit more work, but here's the start of a HTTPD branch that builds mod-tls w/ 0.14.0. Like the The experience made me think that it would be nice to offer a simplified certified key construction and supported ciphersuite list construction that assumes the default provider. These were two places where the mod-tls diff required getting a handle on the default explicitly and passing it in. It'd be more convenient for rustls-ffi to do that work for you. I'll push a revision for this when I'm back in office.
I noticed a |
b07551e
to
945aa3d
Compare
A full run is ~9m. That feels reasonable-ish to me, but it's also probably not necessary to build/test with both clang/gcc for each provider/platform choice. I'm going to leave this as-is until there's additional feedback to consider. |
945aa3d
to
71afa4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed up through 31aed7a. Looking great so far.
Finished reviewing. Two last thoughts:
|
71afa4d
to
8e7b45a
Compare
I should make this more clear in the PR description, but I've tried to not support this use-case. It's technically true that you can build the I did that on purpose because I couldn't come up with a use-case where you would want both crypto providers built in, and it makes life harder for consuming applications. When both features are enabled the various builder fns that rely on a default will error unless a process-wide default has been explicitly set. If only one of the two features are enabled, the process is implicit and doesn't require any updates in the consumer code. As a concrete example of this the WIP WDYT? Do you think I should revisit this aspect of the design? |
Great idea. I included an additional commit for this near the end of the series. |
Maybe this is an oversimplification w.rt. |
That's great, I support that. This makes me realize we need an update to the README discussing building with ring. That could be where we specifically document that building with both is unsupported. |
@ctz Can I bug you for a second review? I'm out today but can finish up the changelog, README update, and other misc bits on ~Thurs if this looks good to everyone. |
8e7b45a
to
174725b
Compare
Added a README update to discuss the crypto provider support, choosing one, and to mention both at once is unsupported.
Added extensive coverage of the added/breaking changes/removed updates to the
I reworked the
I also added a new I think these two changes will make life easier for the downstream users that just want to work with the defaults and not handle a
Lastly, I remembered this issue I had filed as a breaking change and rolled it into this branch. The crypto provider work already made @jsha Did you want to give any of these updates another pass or is this safe to merge/release? @jsha @ctz Thank you both for the reviews! Hopefully this is the last big API update for a while now that we have more parity between this repo and the Rustls 0.21+ crypto provider API. |
Now that the `rustls_client_config_builder_build()` fn is fallible it makes more sense to return an error (`RUSTLS_RESULT_NO_SERVER_CERT_VERIFIER`) when the required server certificate verifier hasn't been set instead of using `NoneVerifier` and failing all certificate validations. This commit removes the `NoneVerifier` and updates the tests that were building a client config without specifying a verifier to use the platform verifier instead. A new unit test is added that ensures the correct error is returned when a config is built without a verifier.
* Adds a `rustls_crypto_provider` type for representing a `rustls::crypto::CryptoProvider`. * The `*ring*` specific provider can be retrieved with `rustls_ring_crypto_provider()`. * The process-wide default crypto provider (if any) can be retrieved with `rustls_crypto_provider_default()`. * `rustls_crypto_provider_ciphersuites_len()` and `rustls_crypto_provider_ciphersuites_get()` can be used to fetch `rustls_supported_ciphersuite` instances the provider supports. * `rustls_default_crypto_provider_ciphersuites_len()` and `rustls_default_crypto_provider_ciphersuites_get()` can be used to fetch `rustls_supported_ciphersuite` instances the _default_ provider supports. * Adds a `rustls_crypto_provider_builder` that can be constructed based on the process default (`rustls_crypto_provider_builder_new()`) or a specific `rustls_crypto_provider` (`rustls_crypto_provider_builder_new_with_base()`). * The builder's supported ciphersuites can be customized with `rustls_crypto_provider_builder_set_cipher_suites()` * The builder can be turned into a `rustls_crypto_provider` with `rustls_crypto_provider_builder_build()`, or it can be built and installed as the process-wide default using `rustls_crypto_provider_builder_build_as_default()`. For the functions that assume a default (e.g. `rustls_default_supported_ciphersuites_len/get()`, and `rustls_crypto_provider_builder_new()`) we make an attempt to install a default based on unambiguous feature state if none has been explicitly set at the time of use. This matches the upstream Rustls behaviour using a function like `ClientConfig::builder()` and makes life easier for existing applications. The existing rustls-ffi code is not yet updated to use these abstractions. Similarly, the `*ring*` backend is unconditionally offered, but will become optional in subsequent commits.
* `rustls_server_config_builder_new()` now uses the process default crypto provider instead of being hardcoded to `*ring*`. We defer constructing the `ServerConfig` to avoid a panic in the event the process default isn't constructed. This will be surfaced as an error at build time instead. Like the upstream `ServerConfig::builder()` we make an attempt to install a process default provider from `rustls_server_config_builder_new()` if one has not been set and a clear choice is available based on crate features. * `rustls_server_config_builder_new_custom()` now takes a `rustls_crypto_provider` as an argument in place of the list of custom ciphersuites. The ciphersuites can be customized when the provider is constructed. * `rustls_server_config_builder_build()` now uses an out param for the `ServerConfig` so we can return a suitable error if there is no crypto provider (e.g. because `rustls_server_config_builder_new()` was used but the process default wasn't set and couldn't be guessed by crate features). * The `server.c` test code is updated to account for the breaking change in the builder out param.
* `rustls_client_config_builder_new()` now uses the process default crypto provider instead of being hardcoded to `*ring*`. We defer constructing the `ClientConfig` to avoid a panic in the event the process default isn't constructed. This will be surfaced as an error at build time instead. Like the upstream `ClientConfig::builder()` if no process default provider has been set when `rustls_client_config_builder_new()` is called we try to set one based on an unambiguous default implied by crate features. * `rustls_client_config_builder_new_custom()` now takes a `rustls_crypto_provider` as an argument in place of the list of custom ciphersuites. The ciphersuites can be customized when the provider is constructed. * `rustls_client_config_builder_build()` now uses an out param for the `ClientConfig` so we can return a suitable error if there is no crypto provider (e.g. because `rustls_client_config_builder_new()` was used but the process default wasn't set and couldn't be inferred from crate features). * The `client.c` test binary is updated to account for the breaking change in the client config builder out-param.
The provider model replaces these.
The `Verifier` type previously had an unconditional dependency on the `*ring*` crypto provider. This commit converts it to use the crypto provider set up by the client config builder as appropriate.
This commit adds a new type, `rustls_signing_key`, that represents a `&dyn SigningKey` loaded by a `rustls_crypto_provider`. A new `rustls_crypto_provider_load_key` fn is added to create a `rustls_signing_key` from a pointer to a `rustls_crypto_provider`, and PEM content in-memory. Wiring this up will be done in a subsequent commit.
This breaks an unconditional dependency on `*ring*` for loading certified key private keys. The existing `rustls_certified_key_build()` fn is converted to use the process-default crypto provider for this purpose. Like other functions that use the implied default if we find no default has been set yet and a clear default is available based on crate features this function will install & use it. For more control over which crypto provider is used to load a private key a new `rustls_certified_key_build_with_signing_key()` fn is added that allows specifying a `rustls_crypto_provider` to use.
This breaks an unconditional dep on `*ring*` for both verifiers. The client/server test binaries do not require any update in this case since they are using the APIs that assume a process-wide default crypto provider has been set.
This breaks an unconditional dep on `*ring*` for the `rustls_platform_verifier` verifier. The `client.c` test binary is updated to use the fallible form of the verifier constructor that uses the default crypto provider.
My IDE (clion) wants to do this automatically and I agree with its choices w.r.t removing hard tabs and adding some more consistent whitespace.
This commit: * Makes the `*ring*` dep optional, behind a `ring` feature flag * Adds an optional (but default) dep on `aws-lc-rs` behind a `aws-lc-rs` feature flag. * Adds `nasm` to the Windows build runners for the `aws-lc-rs` default crypto provider. This build requirement may be relaxed in the future depending on whether the upstream project chooses to take a ring-like strategy of distributing pre-built content. * Updates the cbindgen config to respect these new features. * Updates Makefile/Makefile.pkg-config and CMake build systems to support specifying which crypto provider to use, piping through the correct Rust features and C defines to make it all work. * One acceptor unit test is updated: the list of expected supported ciphersuites differs between `ring` and `aws-lc-rs`, with the latter also offering a P-521 suite that isn't present in `*ring*`. * The client/server examples use the implied default and so require no adjustments.
Rather than using decimal constants, rely on the rustls `SignatureScheme` enum.
This commit updates the `test` and `pkg-config` CI workflows to take into account the variable `CRYPTO_PROVIDER` support.
This commit updates both `client.c` and `server.c` to respect a new `RUSTLS_CIPHERSUITE` env var. When set, the process-default cryptography provider's supported ciphersuites will be reduced to _just_ the one specified by name in the env var. The `client_server.rs` integration test is then updated to start a server that only supports one ciphersuite. Two clients are created, one with a matching ciphersuite and one without. We use each client to connect to the server and assert only the expected one with matching ciphersuite support works.
* Mentions which providers we support, and explicitly that we do not encourage/support building with both providers enabled. * Mentions how to select a provider with the supported build systems (Make, cmake, cargo-c). * Mentions the build requirements/supported platforms of the upstream providers. For e.g. on Windows aws-lc-rs presently requires nasm because at present it (sensibly) does not ship pre-generated binaries.
There are no breaking changes to account for.
2fba428
to
d5b6b6f
Compare
Keeping as a release candidate while we debug one remaining issue with a downstream HTTPD mod_tls update.
d5b6b6f
to
a582386
Compare
I switched the version number from 0.14.0 to 0.14.0-rc1. Despite a bunch of debugging time I still haven't been able to get to the bottom of the HTTPD
Admin merging so I can fixup the required check names afterwards. |
Done. |
Ctz to the rescue 🦸 🏆 It turns out to be the combination of a pre-existing |
This branch updates
rustls-ffi
to support the cryptography provider model established upsteam inrustls
since version 0.22, and later refined in 0.23. By the end of the changesetrustls-ffi
, the unit tests, the integration tests, and CI will supportaws-lc-rs
or*ring*
as cryptography providers. We follow the precedent set upstream in Rustls 0.23 and makeaws-lc-rs
the default choice. We do not support a model whererustls-ffi
is built with both crypto providers enabled. TheMakefile
/CMake
build tooling only allows selecting one or the other.Notably as this branch introduces a number of breaking API changes the version number is updated from 0.13.0 to 0.14.0-rc1. I've labelled this "rc1" while we figure out one remaining downstream issue in HTTPD mod_tls.
Resolves #366
TODO
Changelog.md
to describe breaking changes (note: waiting on review feedback in case there are changes).curl
mod-tls