-
Notifications
You must be signed in to change notification settings - Fork 631
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
Change crate default features to use aws-lc-rs #1780
Conversation
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes LGTM.
I think there's some documentation to update in lib.rs
. For e.g. it still describes ring as being used by default in a couple places:
Lines 54 to 70 in 07747f6
//! ### Platform support | |
//! | |
//! While Rustls itself is platform independent, by default it uses [`ring`] for implementing | |
//! the cryptography in TLS. As a result, rustls only runs on platforms | |
//! supported by `ring`. At the time of writing, this means 32-bit ARM, Aarch64 (64-bit ARM), | |
//! x86, x86-64, LoongArch64, 32-bit & 64-bit Little Endian MIPS, 32-bit PowerPC (Big Endian), | |
//! 64-bit PowerPC (Big and Little Endian), 64-bit RISC-V, and s390x. We do not presently | |
//! support WebAssembly. | |
//! For more information, see [the supported `ring` target platforms][ring-target-platforms]. | |
//! | |
//! By providing a custom instance of the [`crypto::CryptoProvider`] struct, you | |
//! can replace all cryptography dependencies of rustls. This is a route to being portable | |
//! to a wider set of architectures and environments, or compliance requirements. See the | |
//! [`crypto::CryptoProvider`] documentation for more details. | |
//! | |
//! Specifying `default-features = false` when depending on rustls will remove the | |
//! dependency on *ring*. |
Lines 81 to 82 in 07747f6
//! that Rustls uses. This may be appealing if you have specific platform, compliance or feature | |
//! requirements that aren't met by the default provider, [`ring`]. |
Lines 92 to 93 in 07747f6
//! * [`ring`] - enabled by default, available with the `ring` feature flag enabled. This | |
//! provider is used by default when an explicit provider is not specified. |
Lines 307 to 309 in 07747f6
//! - `ring` (enabled by default): makes the rustls crate depend on the *ring* crate, which is | |
//! used for cryptography by default. Without this feature, these items must be provided | |
//! externally to the core rustls crate: see [`CryptoProvider`]. |
Erm, yes, I've missed quite a bit of the meat of this. |
b9e0d19
to
bb47ff2
Compare
Spotted two more stale doc refs (really wish GitHub allowed leaving review feedback on arbitrary lines/files!): Lines 143 to 144 in 07747f6
rustls/rustls/src/crypto/mod.rs Lines 69 to 74 in 07747f6
|
096b1bf
to
2085e79
Compare
Thanks -- addressed these in the latest push. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably do a documentation/example pass to advertise using the process default everywhere?
aws-lc-rs also introduces a new build-time dependency on |
This will introduce quite bit of friction in my opinion. Many rust developer wants to use rustls because it is purely rust (meaning it doesn't need any additional outside build dep). That should be higher priority than optimal performance |
@sehz Neither ring nor aws-lc-rs are pure Rust, they both have components that require a native toolchain to compile. |
We have Lines 316 to 317 in 95067cb
I think we'll also reiterate this in release notes, and give advice on how to stick with ring if people so desire. |
That was the part I was suggesting, yes. You may also want to mention in the README that a default build requires |
This is probably just a matter of adding cmake to whatever container the "cross" project is using, but aws-lc-rs doesn't build with cross while ring does, so it did introduce a bit of friction at least here :) With that said, it's not overly complicated to keep using ring, but an alternative to default with ring might be appreciated so one doesn't have to keep track of "rustls default features, minus aws_lc_rs, plus ring" (e.g. being able to do Thanks! |
The goal here is to provide the optimum performance by default. Naturally the
ring
feature remains available.