Ensuring that a provider based on the one built-in is used about rustls HOT 12 CLOSED

Concretely, what are the failure modes you've seen? Do people pull in an extra dependency that builds a ClientConfig/ServerConfig without specifying any provider? Do people write code using ClientConfig::builder() and ServerConfig::builder() without making sure a/your provider has been installed?

Maybe it would be feasible to ask cargo-deny if they could check for features or upstream dependency edges, so you could enforce that no crates (other than perhaps your own) rely on rustls/aws-lc-rs or rustls/ring? (My experience with filing issues against cargo-deny has been pretty good, so it seems worth a try.)

from rustls.

If I submit a PR moving the ring and aws-lc-rs provider to their own crate, would you be open to review this?

Based on the naming of rustls-postquantum provider, should I name the new crates rustls-ring and rustls-aws-lc-rs?

from rustls.

That seems like a large architectural change best done by the maintainers.

What are your aims in wanting do to that?

It may be worth revisiting the discussion on #1184 which proposed a similar multiple-crates solution. We didn't go that direction because a) people seemed to dislike using Cargo.toml [patch] directives to control their dependency's dependencies, b) it moves the feature mess into a different but equally annoying dimension of crate selection.

from rustls.

Concretely, what are the failure modes you've seen? Do people pull in an extra dependency that builds a ClientConfig/ServerConfig without specifying any provider? Do people write code using ClientConfig::builder() and ServerConfig::builder() without making sure a/your provider has been installed?

I've seen people using ClientConfig::builder() but without making sure my provider has been installed. So they were using the aws-lc-rs provider because the feature was enabled at the workspace level.

Maybe it would be feasible to ask cargo-deny if they could check for features or upstream dependency edges, so you could enforce that no crates (other than perhaps your own) rely on rustls/aws-lc-rs or rustls/ring? (My experience with filing issues against cargo-deny has been pretty good, so it seems worth a try.)

Even if I was able to ensure that only my crate activates the aws-ls-rs feature, because of feature unification at the workspace level, people could still use the aws-ls-rs provider. That's actually what happened, they were using tokio_rustls with the default_features = false, so not relying on rustls/aws-lc-rs directly. I also have a good experience filling issues against cargo deny, maybe I can propose a feature to the tool to solve my issue, even if i have no idea which one to propose yet.

It may be worth revisiting the discussion on #1184 which proposed a similar multiple-crates solution. We didn't go that direction because a) people seemed to dislike using Cargo.toml [patch] directives to control their dependency's dependencies, b) it moves the feature mess into a different but equally annoying dimension of crate selection.

After reading through the discussion, I think that my proposition is different. From what I understand, people would have to choose which crate the rustls package is matching, using patch ( ex: [patch] rustls = { package = "rustls-wasi-crypto" } ). I propose that we keep the feature system as it is today, which only one rustls crate and people depending on this one.
What I want to change, is that when we activate the aws-lc-rs feature, instead of activating code inside the rustls crate, it activates a dependency to a rustls-aws-lc-rs crate, and we re-export it inside the rustls code base.
By doing so, I could create a standalone provider that depends on rustls-aws-lc-rs (and rustls for the provider traits) but without activating the feature on the rustls side. By not activating the feature, if people forget to install my provider, this will be sound because get_default_or_install_from_crate_features will panic.

from rustls.

What I want to change, is that when we activate the aws-lc-rs feature, instead of activating code inside the rustls crate, it activates a dependency to a rustls-aws-lc-rs crate, and we re-export it inside the rustls code base.
By doing so, I could create a standalone provider that depends on rustls-aws-lc-rs (and rustls for the provider traits) but without activating the feature on the rustls side. By not activating the feature, if people forget to install my provider, this will be sound because get_default_or_install_from_crate_features will panic.

This feels like a very niche concern... I think it might be better to try the avenue of getting cargo-deny to warn about some features/dependency edges.

from rustls.

I'm not so sure it is so niche, I suppose that the vast majority of people that want to customize the provider will do that based on the ones that are built-in into rustls. For instance, we just remove the AES 128 from the cipher suite, because it's not advised for post-quantum security.
But maybe most people will just use the included provider, and in that case you are right, this can be a niche.

I don't know how cargo deny could detect that, the feature might be activated or not on the dependency edge and the problem would still be the same because of feature unification. Anyway, thanks for taking the time to answer and propose a solution.

from rustls.

I don't know how cargo deny could detect that, the feature might be activated or not on the dependency edge and the problem would still be the same because of feature unification. Anyway, thanks for taking the time to answer and propose a solution.

I mean, I think you could "see" whether the optional dependency edge is enabled in Cargo.lock, so I think it might be feasible to do something with that -- if Cargo.lock shows a dependency edge from rustls to aws-lc-rs, fail.

from rustls.

I think there is not enough information in the Cargo.lock to differentiate the case if the feature is activated by a crate which is not the custom provider:

[[package]]
name = "foo"
version = "0.1.0"
dependencies  = [
  "rustls",
]

[[package]]
name = "foo-custom-provider"
version = "0.1.0"
dependencies  = [
  "rustls",
]

[[package]]
name = "rustls"
version = "0.23.8"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7fecbfb7b1444f477b345853b1fce097a2c6fb637b2bfb87e6bc5db0f043fae4"
dependencies = [
 "aws-lc-rs", 
 "once_cell",
 "rustls-pki-types",
 "rustls-webpki",
 "subtle",
 "zeroize",
]

We could imagine that cargo deny support adding exception to the features ban. Maybe adding a "skip" or something like that.

[[ban.features]]
crate = "rustls"
deny = ["ring", "aws-lc-rs", "aws_lc_rs"]
skip = [ { crate = "foo-custom-provider" } ] 

But even in that case, maybe it will just detect that the feature is not activated by anything other than the custom provider but, because of feature unification, get_default_or_install_from_crate_features might still be unsound if the wrong provider is used.

Maybe the workaround is to separate the custom provider from the workspace to prevent feature unification and then ban the aws-lc-rs and ring features in the whole workspace. EDIT: this last idea doesn't work because excluding the custom provider from the workspace doesn't prevent the features unification

from rustls.

After giving it more thought, I think even if I had the best tool, I could not make get_default_or_install_from_crate_features sound in my case.

If we imagine we have a tool that will be able to report that only my custom provider is activating the aws-lc-rs feature, that won't be sufficient.

This tool could be something like this

cargo metadata | jq '.packages[] | select(.dependencies[] | .name == "rustls" and (.uses_default_features == true or (.features[] | . == "ring" or . == "aws-lc-rs" or . == "aws_lc_rs"))) | .name'

This specific command won't work perfectly because it will include weak dependencies as the result of rust-lang/cargo#10801 . I guess I could use the krates crate, that is used by cargo deny under the hood, that seems to correct the problem (EmbarkStudios/krates#41)

Even with this tool, it will still be possible to use the vanilla aws_lc_rs provider because of feature unification.

In rcgen, we had a similar problem (rustls/rcgen#206) and we could just test to compile as standalone package (cargo build -p ) to detect this. But here, because the check is at runtime, I would have to launch all my binary in a CI until they hit get_default_or_install_from_crate_features and check that it is actually panicking and for the good reason. This is not really appealing to me.

Envisaged solutions

• if aws-lc-rs provider where a separated crate, I could just ban the use of the aws-lc-rs and ring features for every crate. Then, within my provider, I could use the aws-lc-rs provider crate, without passing by the rustls feature. I would have the guarantee that get_default_or_install_from_crate_features will panic if my provider is not used.
• Create my own crate that copy the implementation of the aws_lc_rs provider and also ban the use of the aws-lc-rs and ring features for every crate. I guess I could just create a sub-module to keep it in sync with rustls/src/crypto/aws_lc_rs folder . It's kind of a light fork but without changing the code.

from rustls.

• Create my own crate that copy the implementation of the aws_lc_rs provider and also ban the use of the aws-lc-rs and ring features for every crate. I guess I could just create a sub-module to keep it in sync with rustls/src/crypto/aws_lc_rs folder . It's kind of a light fork but without changing the code.

This seems kind of reasonable?

from rustls.

I think we would like to avoid this kind of forking, because of the maintenance cost, especially because of the fact it is crypto related.

We have a very big project with a lot of standalone binary and dependencies and we would like to ensure that all of them use rustls-postquantum.
We want to guarantees that all handshakes are done in post-quantum safe way, and we want this to be valid long term.

It might not be obvious for future coworker to always install rustls-post-quantum before using rustls, and their code will work just fine because they will use directly use aws-lc-rs. It would be very reassuring if we could have a check, at least a runtime panic, to ensure this not happen.

Another solution, suggested by a coworker, might be to add an optional feature "custom-provider" to rustls and modify from_crate_features this way:

fn from_crate_features() -> Option<Self> {
        #[cfg(all(feature = "ring", not(feature = "aws_lc_rs"), not(feature = "custom-provider")))]
        {
            return Some(ring::default_provider());
        }

        #[cfg(all(feature = "aws_lc_rs", not(feature = "ring"), not(feature = "custom-provider")))]
        {
            return Some(aws_lc_rs::default_provider());
        }

        #[allow(unreachable_code)]
        None
    }

This way we can ensure that aws-lc-rs is never installed silently if someone forgot to install rustls-postquantum first.

from rustls.

Hmm, sounds reasonable to me.

from rustls.