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

Ensuring that a provider based on the one built-in is used #1920

Open
1 task done
stormshield-gt opened this issue Apr 25, 2024 · 7 comments
Open
1 task done

Ensuring that a provider based on the one built-in is used #1920

stormshield-gt opened this issue Apr 25, 2024 · 7 comments

Comments

@stormshield-gt
Copy link

Checklist

  • I've searched the issue tracker for similar requests

Is your feature request related to a problem? Please describe.

I work on a very big project with multiple applications that uses rustls and which dependencies also use rustls. We have a custom provider based on aws-lc-rs, that we want to be used by all of our crates.
Besides, some of our dependency relies on a static provider has been installed in the current process.

My problem is sometimes, when new codes that need tls is added, the original aws-lc-rs is silently used instead of our custom one.

For instance, if in a crate someone does rustls_platform_verifier::tls_config without installing our custom provider before, no error will be thrown and this will use the classic aws-lc-rs provider

I thought how I could detect this with some tooling, like clippy or cargo deny and I didn't figure it out.

Describe the solution you'd like

I would like that ring and aws-lc-rs provider to be moved on a separate crate. They could still be added as dependency to ruslts under feature flag to keep the current behavior.

If aws-lc-rs provider were in a separated crate, I could use this crate directly for my custom provider. I could then deny the feature aws-lc-rs and ring with cargo deny

[[bans.features]]
name = "rustls"
deny = ["aws-lc-rs","aws_lc_rs", "ring"]
reason = "we use our own provider"

Then if somebody forgets to install the custom provider, it will be caught thanks to a panic in get_default_or_install_from_crate_features() . This can work right now for providers that aren't based on ring or aws-lc-rs, but in my case I must activate the aws-lc-rs feature so I can base my custom provider on it.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Option A: force users to always use builder_with_provider

I can tell users to not use the process default provider and add this to my clippy.toml:

disallowed-methods = [
    { path = "rustls::ClientConfig::builder", reason = "can silently use a wrong provider" },
    { path = "rustls::ServerConfig::builder", reason = "can silently use a wrong provider" },
    # But those methodes can also be called transitively..
    { path = "rustls_platform_verifier::tls_config", reason = "can silently use a wrong provider" },  
]

The problem is that I must also scan dependencies that my use the default provider and so each time a new version is issued.

Besides we have a patched dependency which relies on the process default because it makes the patch a lot simpler.

Option B: reexport rustls and dependencies that use rustls under an inner crate

Another solution is to have an inner crate that has rustls has a private dependency and force use to use my crate with cargo deny

deny = [
    { crate = "rustls", reason = "must not be used directly", use-instead = "inner-crate", wrappers = ["inner-crate"]},
]

This requires less auditing of dependencies, because I only have to do a cargo tree to see the one that depends of rustls. In the clippy solution I would also have to check if they use ClientConfig::builder() internally.

Additional context

I´m not a provider expert and maybe I missed something. If so I would be grateful if you can provide additional guidance. Otherwise the proposed solution, moving the providers to a crate, that I remember to have already been discussed, seems to be a reasonable option.

@stormshield-gt
Copy link
Author

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?

@ctz
Copy link
Member

ctz commented May 14, 2024

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.

@djc
Copy link
Member

djc commented May 14, 2024

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

@stormshield-gt
Copy link
Author

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.

@djc
Copy link
Member

djc commented May 22, 2024

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.

@stormshield-gt
Copy link
Author

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.

@djc
Copy link
Member

djc commented May 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants