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

[Task] Get rid of ring dependency in core #3226

Open
Yury-Fridlyand opened this issue Feb 21, 2025 · 7 comments · May be fixed by #3200
Open

[Task] Get rid of ring dependency in core #3226

Yury-Fridlyand opened this issue Feb 21, 2025 · 7 comments · May be fixed by #3200
Assignees
Labels
Core changes Used to label a PR as PR with significant changes that should trigger a full matrix tests. dependency Dependency management Rust core redis-rs/glide-core matter
Milestone

Comments

@Yury-Fridlyand
Copy link
Collaborator

Yury-Fridlyand commented Feb 21, 2025

Description

Old version of ring is presend in cargo dependency tree (cargo tree), all CI fails now: https://github.com/valkey-io/valkey-glide/actions/runs/13466704614/job/37633907140#step:3:878.
It is a dependency of rustls and rustls-webpki, but even most recent version of rustls uses the same version of ring (one, two).

This crate is only used to partially validate certificates on unsecure TLS connections. This API (using non-secure TLS, e.g. self-signed certificates) isn't exposed to wrappers and to end users.

match (insecure, cfg!(feature = "tls-rustls-insecure")) {
#[cfg(feature = "tls-rustls-insecure")]
(true, true) => {
let mut config = config;
config.enable_sni = false;
// nosemgrep
config
.dangerous()
.set_certificate_verifier(Arc::new(NoCertificateVerification {
supported: rustls::crypto::ring::default_provider()
.signature_verification_algorithms,
}));
Ok(config)

Solution could be:

  • Contribue to rustls and rustls-webpki by updating ring dependency, wait for newer version of rustls and rustls-webpki released, then update these dependencies in GLIDE
  • Remove this validations (set_certificate_verifier(Arc::new(NoCertificateVerification {})))
  • Replace with another implementation, which doesn't use ring (e.g. https://gist.github.com/doroved/2c92ddd5e33f257f901c763b728d1b61)

Checklist

No response

Additional Notes

No response

@Yury-Fridlyand Yury-Fridlyand added Rust core redis-rs/glide-core matter Core changes Used to label a PR as PR with significant changes that should trigger a full matrix tests. dependency Dependency management labels Feb 21, 2025
@avifenesh
Copy link
Member

rustls/rustls#2349

@avifenesh
Copy link
Member

avifenesh commented Feb 22, 2025

I'm putting it as (same visibility, just an arrangement thing) part of the rework of the core, and cleaning it, which is very much needed.
The TLS pieces we have are a mess, and there is no team design done about them, they require a general reassessment on top of the current issue.

@avifenesh avifenesh added this to the 1.4 milestone Feb 22, 2025
@cpu
Copy link

cpu commented Feb 22, 2025

It is a dependency of rustls and rustls-webpki, but even most recent version of rustls uses the same version of ring

To be maximally clear: it's an optional and non-default dependency. aws-lc-rs is the default cryptography provider unless you specifically opt-in to using ring.

@avifenesh
Copy link
Member

@cpu Hi,
Did some reading, and it seems that aws-lc-rs gives better performance and wider support, overall seems to be a better choice.
But checking the sizes, there is a massive different between the two.
Do you know to say, roughly, how is it affecting the size of rustls as a "blind" (can't strip, or change the chain) dependency?

Overall, while you are already here, what is your take on the pros and cons?

@avifenesh
Copy link
Member

@cpu @Yury-Fridlyand I noticed that ring released 24 hours ago, I guess it's solved the issue.
@cpu Will still like to hear your opinion :)

@cpu
Copy link

cpu commented Feb 22, 2025

But checking the sizes, there is a massive different between the two.

I'm not sure which dimension you're comparing. Lines of code? Size of produced build artifacts? I don't have any particular insight into this aspect but would expect the two to be fairly comparable for these points.

Overall, while you are already here, what is your take on the pros and cons?

Advantages-wise, the aws-lc-rs provider generally has better performance, and offers more features you may care about (for example post-quantum key exchange support, P-521 curve support, FIPS support, the primitives needed for Encrypted Client Hello, etc). I've had very positive experiences interacting with the team behind this crate. I think it's the right default choice for rustls.

Disadvantages-wise, I think there are some more esoteric targets that aws-lc-rs doesn't support that *ring* does. aws-lc-rs also requires cmake for Windows builds, and some folks find that requirement especially onerous.

Overall *ring* is a quality cryptography provider and the Rustls team has agreed to do limited maintenance going forward so if it's better for your use-case you can continue using it without much worry. The main disadvantages are the lack of certain features and the velocity of upstream work. Given the current maintenance status I expect those feature gaps are not going to be addressed anytime soon.

Hope that helps,

@avifenesh
Copy link
Member

@cpu Checked on crate.io the metadata, but checking locally the build size it's apparently not true (while they are both on default);
Plus, we weren't dependent on it directly, but we just didn't update to the last rustls.
Updated now and saw the usage of ring errors.
Thanks!

@avifenesh avifenesh linked a pull request Feb 23, 2025 that will close this issue
6 tasks
@avifenesh avifenesh linked a pull request Feb 23, 2025 that will close this issue
6 tasks
@avifenesh avifenesh self-assigned this Feb 23, 2025
@avifenesh avifenesh moved this to In Progress in Valkey-GLIDE - internal Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core changes Used to label a PR as PR with significant changes that should trigger a full matrix tests. dependency Dependency management Rust core redis-rs/glide-core matter
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

3 participants