rustls / tokio-rustls

Async TLS for the Tokio runtime
Apache License 2.0
126 stars 70 forks source link

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

Closed jbr closed 8 months ago

jbr commented 8 months ago

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.

quininer commented 8 months ago

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 https://github.com/libp2p/rust-libp2p/pull/1986

ctz commented 8 months ago

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 commented 8 months ago

I would be in favor of that.

jbr commented 8 months ago

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 commented 8 months ago

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.

quininer commented 8 months ago

Thank you!