CryptoProvider support

[jsha]

Rustls' upcoming 0.22 release has support for a CryptoProvider trait that can be used to provide alternate crypto backends. We should support it in rustls-ffi. We can think of this as two stages:

Stage 1

Allow choosing between rustls' first party supported providers, ring, and aws-lc-rs. Requirements:

• Compile time
  • It must be possible to build with each of: ring, aws-lc-rs, and "both ring and aws-lc-rs".
  • If one of the solo options is selected, no code from the other option should be compiled in.
• Run time
  • It must be possible to select one of ring or aws-lc-rs for any given config, including selecting different backends for different configs.

Stage 2

Allow implementation of third-party CryptoProviders through rustls-ffi.

The path to do this would be to expose each of the 15 traits I mentioned in rustls/rustls#1603. The pattern to implement traits in FFI is to effectively do the same thing as trait objects in Rust: define a vtable and do dynamic dispatch.

• For each method in the trait, define a corresponding C function signature, including a void *userdata.
• Define a struct whose fields are function pointers with each of those signatures. This is the vtable, and it's usually defined statically, though it doesn't have to be.
• The "trait object" is the combination of a specific *userdata with the vtable.

So, there's a path to exposing all the traits of the CryptoProvider API. But I actually think we shouldn't do this. I think the FFI should go the other way. If there's an existing C library that provides crypto implementations (e.g. boringssl), someone should write Rust code that calls out to that library with FFI in order to provide the traits that CryptoProvider wants. This is how the demo boring-rustls-provider does it.

That leaves the question: say a rustls-ffi users wants to use neither ring nor aws-lc-rs, but some third thing like boring-rustls-provider. How do they do that?

I think it should work like this:

• rustls-ffi accepts a *rustls_crypto_provider.
• The preconditions for that pointer are "it must have come from a Rust &'static &'static dyn CryptoProvider" (the two statics handle the DSTs problem we were just discussing).
• third party CryptoProvider implementations can either expose an FFI method themselves to get a pointer of the appropriate type, or someone else can provide it.
• the third party implementation will be linked in as a separate library.

[cpu]

(Was this meant to be filed on rustls/rustls-ffi instead of this repo?)

[jsha]

(Was this meant to be filed on rustls/rustls-ffi instead of this repo?)

Yep, thanks!

[cpu]

@jsha I started working on the "Stage I" plan and have a prototype branch that I think gets us most of the way there: cpu-choose-your-own-crypto-adventure.

• make is our status-quo: rustls-ffi is built with only *ring*, and uses it as the default crypto provider.
• make CRYPTO_PROVIDER=aws_lc_rs will build with only aws_lc_rs and uses it as the default crypto provider.
• make CRYPTO_PROVIDER=all will build with both *ring* and aws_lc_rs and uses *ring* as the default crypto provider.

The rustls_client_config_builder_new and rustls_server_config_builder_new fns use the default provider based on the build args. In addition there's two new fns, rustls_client_config_builder_new_with_provider and rustls_server_config_builder_new_with_provider, that can be used to explicitly chose a provider.

If built with *ring* support (e.g. no CRYPTO_PROVIDER make arg or CRYPTO_PROVIDER=all) then there is a rustls_crypto_provider_ring_new function to use with the _with_provider fns. If built with aws_lc_rs support (e.g. CRYPTO_PROVIDER=aws_lc_rs or CRYPTO_PROVIDER=all) then there is a rustls_crypto_provider_aws_lc_rs_new function for choosing that provider explicitly.

I've been using nm to look at the symbols in the built client/server binaries with different build configs and it looks to be doing what I'd hope w.r.t not statically linking a provider that isn't specified by build args. Integration tests pass with both providers (but I haven't updated CI for this).

Some places that I could use feedback:

• Is this CRYPTO_PROVIDER Makefile arg roughly what you had in mind for selecting the crate features + provider options at build time?
• I think the ciphersuite bits are going to need more thought. In particular:
  • rustls_connection_get_negotiated_ciphersuite uses ALL_CIPHER_SUITES, but I think we need to associate a specific crypto provider with the rustls_connection to make sure we're matching the SupportedCipherSuite correctly instead of always using ALL_CIPHER_SUITES from the default provider.
  • Ditto for RUSTLS_ALL_CIPHER_SUITES and RUSTLS_DEFAULT_CIPHER_SUITES - I think we'll want fns on the rustls_crypto_provider that return this information per-provider. The existing items are being populated based on which default provider is used.
• rustls_certified_key_build is using the default crypto provider for any_supported_signing_key - should there be a second rustls_certified_key_build_with_provider fn for this that accepts an explciit rustls_crypto_provider?

I'll keep poking at this in the meantime and update here if I get any further along. Thanks!

[jsha]

Is this CRYPTO_PROVIDER Makefile arg roughly what you had in mind for selecting the crate features + provider options at build time?

Yes, this is good.

rustls_connection_get_negotiated_ciphersuite uses ALL_CIPHER_SUITES, but I think we need to associate a specific crypto provider with the rustls_connection to make sure we're matching the SupportedCipherSuite correctly instead of always using ALL_CIPHER_SUITES from the default provider.

Right now the only thing you can do with a *rustls_supported_ciphersuite is get its IANA registered number, or its name. I think we could cut out the *rustls_supported_ciphersuite type entirely and offer direct accessors to get the name and/or number. That is, rustls_connection_get_negotiated_ciphersuite_name and rustls_connection_get_negotiated_ciphersuite_id.

To put another way: SupportedCipherSuite is actually about implementation rather than configuration; the only item of value to the end user is the configuration info buried deep inside (the cipher suite id).

Ditto for RUSTLS_ALL_CIPHER_SUITES and RUSTLS_DEFAULT_CIPHER_SUITES - I think we'll want fns on the rustls_crypto_provider that return this information per-provider. The existing items are being populated based on which default provider is used.

Maybe we don't need these at all? If people want the defaults, they can just rely on them being default. We should do some brief spelunking to see if there was more motivation to offer these.

rustls_certified_key_build is using the default crypto provider for any_supported_signing_key - should there be a second rustls_certified_key_build_with_provider fn for this that accepts an explciit rustls_crypto_provider?

Yeah, this seems like the best way forward, though I don't love it. It means asking users to pick a crypto provider once to build their config, and a second time to build their certified key.

[cpu]

Thanks for the feedback!

Right now the only thing you can do with a *rustls_supported_ciphersuite is get its IANA registered number, or its name. I think we could cut out the *rustls_supported_ciphersuite type entirely and offer direct accessors to get the name and/or number.

That type is also used for the rustls_{client|server}_config_builder_new_custom cipher_suites argument. Whether or not we keep the type, I think there needs to be a way to get the provider's cipher_suites's across the FFI boundary to be able to select a subset for the custom configuration (either as rustls_supported_ciphersuite entries, or u16s).

I'm imagining something like:

  #[no_mangle]
  pub extern "C" fn rustls_crypto_provider_cipher_suites(
      provider: *const rustls_crypto_provider,
      cipher_suites: *mut *const *const rustls_supported_ciphersuite,
      cipher_suites_len: *mut size_t,
  ) -> rustls_result {
     ...
  }

I tried a quick initial implementation without introducing a new Castable and I think I'm running into issues because I'm converting the frame-local Vec from the provider into a pointer passed over the FFI boundary and so break the invariant that the Vec lives longer than the as_ptr result - probably I need to make a new type and Castable with Ownership = OwnershipArc and RustType = Vec<SupportedCipherSuite> 🤔 I'll revisit tomorrow when I have more mental energy.

[jsha]

That type [rustls_supported_cipher_suite] is also used for the rustls_{client|server}_config_builder_new_custom cipher_suites argument.

Oops, you are correct. Okay, an amendment:

When calling get_negotiated_ciphersuite, the user only cares about the configuration aspect of SupportedCipherSuite; the name and ID. They don't care about the implementation aspect, which is most of what SupportedCipherSuite carries.

So for get_negotiated_ciphersuite we could return something different; just the ID, or just the name, or and object from which you can get the ID and name. Basically rustls::CipherSuite.

However, for configuring a connection, you would still need to pass *rustls_supported_ciphersuite because that carries the implementation. And we need to pass implementation when configuring any non-default ciphersuites, because that's how we accomplish link-time removal of unused ciphersuites.

So *rustls_supported_ciphersuite as a type would stay, we just wouldn't return it from get_negotiated_ciphersuite.

[cpu]

Here's a checkpoint branch that's a bit further along (but still missing lots of polish).

• The connection type now lets you return the suite ID or name of the negotiated ciphersuite, but not the implementation
• The all/default ciphersuite bits are removed.
• There's a builder API for the crypto provider, and this is where you can customize ciphersuite implementations (reducing from the default, or mixing/matching between providers)
• Built providers offer a way to get the ciphersuite implementations they support (so they can be used as arguments to the ciphersuite customization of another crypto provider builder)
• The rustls_client_config_builder_new_custom and rustls_server_config_builder_new_custom fns no longer take ciphersuites as args, they take a rustls_crypto_provider, and a choice of tls versions. The ciphersuites are provided when building the rustls_crypto_provider.

I also tried something new and added a unit tests module in crypto.rs that uses the FFI interfaces with unsafe blocks to try and test the more interesting bits of the crypto provider without needing to figure out how to write portable C unit tests. Another option I considered was writing tests in a separate crate that doesn't take a rustls-ffi dependency, but uses the .a and rust-bindgen to test the C API from Rust in a way that can't "cheat" using rustls-ffi Rust code/helpers. I'm not sure how you feel about these ideas, maybe you've avoided this on purpose? My thinking here is that it would be nice to have something in-between pure Rust tests and continuing to hack more and more awkward C code into server.c and client.c.

Some places for improvement:

• Having the CryptoProvider type hold a Vec<*const rustls_supported_ciphersuites> seems off, but I couldn't find a workable way to go from Vec<SupportedCipherSuite> to *mut *const *const rustls_supported_ciphersuite/*mut size_t for passing over the FFI interface without making this Vec ourselves and keeping it owned by the CryptoProvider so the lifetime of the returned ciphersuites array will match that of the provider.
• The client binary is a bit hacked up right now to build the default crypto provider, and then select out only the TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 ciphersuite - this was just me testing the API for getting ciphersuites + customizing a crypt provider. We could use an optional env var for the ciphersuite selection, or rip this out.
• The Miri task is unhappy, indicating I'm leaking memory somewhere (no doubt true!)

What do you think about this general direction for the API?

[jsha]

Generally looks right. rustls_certified_key_build_with_provider isn't a match for the Rust API of CertifiedKey. The C function takes a whole crypto provider, when CertifiedKey::new just takes an Arc<dyn SigningKey>. To match the Rust API I think we need to expose a type for Arc<dyn SigningKey> and a function that parses PEM. That function should collapse some of the complications of rustls-pemfile being a different crate, and the intermediate enums in rustls-pki-types.

I also tried something new and added a unit tests module in crypto.rs that uses the FFI interfaces with unsafe blocks to try and test the more interesting bits of the crypto provider without needing to figure out how to write portable C unit tests. Another option I considered was writing tests in a separate crate that doesn't take a rustls-ffi dependency, but uses the .a and rust-bindgen to test the C API from Rust in a way that can't "cheat" using rustls-ffi Rust code/helpers. I'm not sure how you feel about these ideas, maybe you've avoided this on purpose?

Yep, this is excellent and the way forward. We discussed this a bit here: #209. One of the benefits of Rust tests is that we get the benefit of Miri, at least in code paths that don't hit FFI (which Miri doesn't like). The reason you don't see a lot of these is just that we don't have a lot of unittest coverage. My initial testing priority was to get a sort of working example and see how the API felt in C. That's of course a downside of writing unittests in Rust; since we're not exercising the code in C we won't see right away if we've made an awkward API. But I think that's an acceptable tradeoff. I haven't found any unittesting frameworks in C that looked great.

[cpu]

To match the Rust API I think we need to expose a type for Arc and a function that parses PEM. That function should collapse some of the complications of rustls-pemfile being a different crate, and the intermediate enums in rustls-pki-types.

That makes sense. I'll take a look at fixing that up.

Yep, this is excellent and the way forward

Awesome. What you're saying about the trade-offs makes sense to me too.

[cpu]

I was able to find time to come back around at this. PTAL: #441