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

feat: change default crypto provider to match rustls' #50

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

jbr
Copy link
Contributor

@jbr jbr commented Mar 5, 2024

It would be confusing for rustls docs to say "by default it uses aws-lc-rs for implementing the cryptography in TLS" but discover that tokio-rustls changes the default.

@jbr jbr force-pushed the default-crypto-provider-aws-lc-rs branch from ca3a2e4 to bde59e1 Compare March 5, 2024 22:47
@quininer
Copy link
Member

quininer commented Mar 6, 2024

If we want to match rustls we should remove default-features = false and default = [].

I thought about it again, even if we forward it as is, or don't forward the feature, it is still necessary to configure default-features = false for rustls. otherwise, the default-features of rustls will not be turned off when tokio-rustls exists. like libp2p/rust-libp2p#1986

@ctz
Copy link
Member

ctz commented Mar 6, 2024

Any thoughts on this crate not setting (or re-exposing) any of the ring/aws_lc_rs crate features? Given the top-level API here requires an application to supply a ClientConfig/ServerConfig, all decisions relating to providers have already been made.

In fact, I think this crates features can be reduced to just early-data? Everything else reexposes rustls features, which are better set directly elsewhere in the dependency tree.

(Naturally for tests and example code, it would be necessary to depend on rustls with default features as a dev-dependency.)

@djc
Copy link
Member

djc commented Mar 6, 2024

I would be in favor of that.

@jbr
Copy link
Contributor Author

jbr commented Mar 6, 2024

Everything else reexposes rustls features, which are better set directly elsewhere in the dependency tree.

I'll make the case for surfacing all rustls features in this crate as a distinct concern from what the default features are: Currently tokio-rustls and futures-rustls reexport rustls, which means that a consuming crate can just add the async rustls flavor to their Cargo.toml as a façade for rustls. User code doesn't need to keep two independent versions of rustls synchronized in Cargo.toml, and there are no "same name, different types" errors when one version bumps before the other.

If I'm a downstream crate depending on tokio-rustls or futures-rustls, this is rustls from my perspective and I wouldn't necessarily need to depend directly on rustls other than to enable features.

This is a cargo problem in that ideally we'd be able to enable features on transitive dependencies with something like tokio-rustls = { version = "0.25", features = ["rustls/ring"] }, but in the absence of that, it's awkward for people to have to add a transitive dependency as a direct dependency only to enable features, especially since there's no way to say "I want the same version that tokio-rustls is using, whatever that is."

@djc
Copy link
Member

djc commented Mar 7, 2024

Sorry, yes, I also agree with that. I think tokio-rustls often acts as a kind of wrapper crate, so we should actually mirror rustls' defaults and features -- the sibling transitive dependency is definitely a bit of an anti-pattern.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a CI job to test the ring crypto provider? Presumably since we don't actually have crypto provider-dependent code in here it's not actually necessary?

@quininer quininer merged commit 330d287 into rustls:main Mar 7, 2024
6 checks passed
@quininer
Copy link
Member

quininer commented Mar 7, 2024

Thank you!

@cpu cpu mentioned this pull request Mar 21, 2024
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

Successfully merging this pull request may close these issues.

4 participants