0.14.0: Adopt crypto provider API, use aws-lc-rs as default provider

[cpu]

This branch updates rustls-ffi to support the cryptography provider model established upsteam in rustls since version 0.22, and later refined in 0.23. By the end of the changeset rustls-ffi, the unit tests, the integration tests, and CI will support aws-lc-rs or *ring* as cryptography providers. We follow the precedent set upstream in Rustls 0.23 and make aws-lc-rs the default choice. We do not support a model where rustls-ffi is built with both crypto providers enabled. The Makefile/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.

Resolves https://github.com/rustls/rustls-ffi/issues/366

TODO

• [x] update Rustls 0.23.4 -> 0.23.11
• [x] update Changelog.md to describe breaking changes (note: waiting on review feedback in case there are changes).
• [x] consider trimming CI matrix down?
• [x] attempt a demo using a 3rd party crypto provider
• [ ] stage a downstream update in curl
• [ ] stage a downstream update in mod-tls

[cpu]

I recommend reviewing this commit-by-commit. I've taken extra care to make sure each commit builds/tests cleanly along the way.

[cpu]

attempt a demo using a 3rd party crypto provider

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 rustls-post-quantum provider. It exposes one fn, const void *rustls_post_quantum_ffi_crypto_provider(void), returning a void* to what is an Arc<CryptoProvider> under the hood.

I used that to quickly extend the rustls-ffi client.c demo so that it would use rustls_post_quantum_ffi_crypto_provider() in place of the AWS LC RS backend.

With that in place we can build client and observe it doing post-quantum KEX with Cloudflare's test server:

[daniel@blanc:~/Code/Rust/rustls-post-quantum-ffi]$ cargo capi install --prefix=/tmp/rustls-ffi
<snipped>
    Finished `release` profile [optimized] target(s) in 0.08s
  Installing pkg-config file
  Installing header file
  Installing static library
  Installing shared library

[daniel@blanc:~/Code/Rust/rustls-ffi]$ cargo capi install --prefix=/tmp/rustls-ffi
<snipped>
    Finished `release` profile [optimized] target(s) in 18.19s
    Building pkg-config files
  Populating uninstalled header directory
  Installing pkg-config file
  Installing header file
  Installing static library
  Installing shared library

[daniel@blanc:~/Code/Rust/rustls-ffi]$ tree /tmp/rustls-ffi/
/tmp/rustls-ffi/
├── include
│   ├── rustls.h
│   └── rustls-post-quantum-ffi.h
└── lib
    ├── librustls.a
    ├── librustls-post-quantum-ffi.a
    ├── librustls-post-quantum-ffi.so -> librustls-post-quantum-ffi.so.0.1.0
    ├── librustls-post-quantum-ffi.so.0.1.0
    ├── librustls.so -> librustls.so.0.14.0
    ├── librustls.so.0.14.0
    └── pkgconfig
        ├── rustls.pc
        └── rustls-post-quantum-ffi.pc

[daniel@blanc:~/Code/Rust/rustls-ffi]$ PKG_CONFIG_PATH=/tmp/rustls-ffi/lib/pkgconfig make --file=Makefile.pkg-config PROFILE=debug CRYPTO_PROVIDER=aws_lc_rs
<snipped>

[daniel@blanc:~/Code/Rust/rustls-ffi]$ LD_LIBRARY_PATH=/tmp/rustls-ffi/lib ldd target/client
    <snipped>
    librustls.so.0.14.0 => /tmp/rustls-ffi/lib/librustls.so.0.14.0 (0x00007ffff7085000)
    librustls-post-quantum-ffi.so.0.1.0 => /tmp/rustls-ffi/lib/librustls-post-quantum-ffi.so.0.1.0 (0x00007ffff6dee000)
    <snipped>

[daniel@blanc:~/Code/Rust/rustls-ffi]$ RUSTLS_PLATFORM_VERIFIER=1 LD_LIBRARY_PATH=/tmp/rustls-ffi/lib ./target/client pq.cloudflareresearch.com 443 /cdn-cgi/trace 2>/dev/null | grep "kex"
kex=X25519Kyber768Draft00
kex=X25519Kyber768Draft00
kex=X25519Kyber768Draft00

[cpu]

@ctz @jsha I'll be out of office next week. Is it possible you folks could take a look at this branch during that time so I can address feedback when I'm back? I think this is one where we want both reviews before merging. Thanks!

[cpu]

stage a downstream update in curl

Here's a WIP branch. For the time being I've left all of the vtls/rustls.c code using the interfaces that assume a clear default provider is available. That means whoever builds/installs the librustls.so/librustls.a that curl links choses the crypto provider. There are potentially more complicated options we could pursue where curl sets the default provider based on its own configuration, but its not clear if that's the direction the downstream project will want to go or not. Probably merits discussion.

[cpu]

stage a downstream update in mod-tls

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 curl branch for now I've left everything using the default crypto provider.

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.

cpu force-pushed the cpu-366-choose-your-own-crypto-adventure branch from d29400b to b07551e

I noticed a Makefile omission where building with CRYPTO_PROVIDER=ring was causing aws-lc-rs to be built as well. The force push was for that fix.

[cpu]

consider trimming CI matrix down?

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.

[jsha]

Finished reviewing. Two last thoughts:

• I think the test matrix should include a case that builds both ring and aws-lc-rs into the same binary, since that's supported.
• Ideally the version string should say which crypto providers the package was built against, though that can be a followup.

[cpu]

I think the test matrix should include a case that builds both ring and aws-lc-rs into the same binary, since that's supported.

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 rustls-ffi Rust components manually activating both --features=ring and --features=aws_lc_rs, but the Makefile, Makefile.pkg-config, and CMake build configurations only support choosing one or the other (defaulting to aws_lc_rs if not specified) and will never activate both features at once. IMO those are the only supported ways to build rustls-ffi and so we don't support builds w/ both providers.

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 curl diff I staged (diff) requires minimal changes under this model. Notably it doesn't need new code to install either rustls_aws_lc_rs_crypto_provider() or rustls_ring_crypto_provider() as the default.

WDYT? Do you think I should revisit this aspect of the design?

[cpu]

Ideally the version string should say which crypto providers the package was built against, though that can be a followup.

Great idea. I included an additional commit for this near the end of the series.

[cpu]

IMO those are the only supported ways to build rustls-ffi and so we don't support builds w/ both providers.

Maybe this is an oversimplification w.rt. cargo-c builds and it is worth having coverage? :thinking:

[jsha]

we don't support builds w/ both providers.

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.

[cpu]

@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.

[cpu]

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.

Added a README update to discuss the crypto provider support, choosing one, and to mention both at once is unsupported.

update Changelog.md to describe breaking changes (note: waiting on review feedback in case there are changes).

Added extensive coverage of the added/breaking changes/removed updates to the CHANGELOG.md.

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.

I reworked the rustls_signing_key update so that:

• rustls_certified_key_build() retains the private_key and private_key_len arguments and uses the default crypto provider to create a rustls_signing_key to use for the certified key (or will error if not set).
• a new rustls_certified_key_build_with_signing_key() fn is added that allows providing a rustls_signing_key constructed with a rustls_crypto_provider in place of the private_key and private_key_len arguments of rustls_certified_key_build(). The latter uses this function internally.

I also added a new rustls_default_supported_ciphersuites() fn to return the rustls_supported_ciphersuites of the default crypto provider (or an error if not set).

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 rustls_crypto_provider explicitly.

https://github.com/rustls/rustls-ffi/issues/422 - "Make rustls_client_config_builder_build fallible, rm NoneVerifier"

Lastly, I remembered this issue I had filed as a breaking change and rolled it into this branch. The crypto provider work already made rustls_client_config_builder_build fallible so it was straight forward to remove the NoneVerifier. A handful of tests needed to be updated to explicitly set a provider to avoid the new RUSTLS_RESULT_NO_SERVER_CERT_VERIFIER error returned by the builder operation when no verifier is set.

@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.

[cpu]

cpu marked this pull request as draft 6 minutes ago

I think I need a couple more tweaks here (and test coverage) for consumers that want to use the implicit default provider. I'm seeing a failure mode downstream in curl where building a server cert verifier fails unexpectedly with this branch. I think the issue is that the verifier is built before a client config. When setting up the rustls-ffi verifier builder we query the process default provider, but it's None at this point because no configs have been built to have the Rustls-internal get_default_or_install_from_crate_features() do the required magic to install the default implied by unambiguous feature selection. When we try to build the verifier, we find there's no default provider and err.

Working on a fix.