Skip to content

use ring if no default crypto provider is found #2034

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.

Sign up for GitHub

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

Jump to bottom
Merged
tobz merged 4 commits into from
Nov 7, 2024
Merged

use ring if no default crypto provider is found #2034

tobz merged 4 commits into from
Nov 7, 2024

Conversation

@conradludgate
Copy link
Contributor

@conradludgate conradludgate commented Nov 3, 2024

Motivation

Tonic hard codes the ring feature in rustls. When another crate depends on default rustls features they will additionally have the aws-lc-rs feature enabled in rustls. Having both
of these features enabled makes rustls not work "out of the box" as there will no longer
be a default crypto provider.

Solution

Make tonic use the ring provider as a fallback if no default provider is found.

@conradludgate conradludgate mentioned this pull request Nov 3, 2024
Merged
tobz
tobz requested changes Nov 7, 2024
View reviewed changes
@conradludgate
Copy link
Contributor Author

conradludgate commented Nov 7, 2024

Looks like the assumptions are no longer valid since #2008 merged.

@conradludgate
Copy link
Contributor Author

conradludgate commented Nov 7, 2024

Now that it's configurable, I'm not sure there is still much use in this feature.

@tobz
Copy link
Collaborator

tobz commented Nov 7, 2024

While that's a fair point, I do still think there's value here.

If tonic is configured with one of the two provider options, and the other option is transitively configured, then there's no default for us to get at all, so we still end up having to make an explicit choice on which one to use. That's fine for rustls, but the user already told us they wanted to use AWS-LC or Ring, so we should honor that and use it directly.

@tobz tobz added this pull request to the merge queue Nov 7, 2024
Merged via the queue into hyperium:master with commit 5ae578e Nov 7, 2024
@mikemorris mikemorris mentioned this pull request Feb 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tobz tobz tobz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@conradludgate @tobz