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

choose a crypto_provider for rustls_cert_gen #206

Merged

Conversation

stormshield-gt
Copy link
Contributor

@stormshield-gt stormshield-gt commented Jan 11, 2024

From the release 0.12 of rcgen #202 , we must now choose ring or aws_lc_rc as a feature.
Because rustls_cert_gen has default-features = false, its build is currently broken.

This PR activate aws_lc_rc feature, because soon it will get RSA key generation support aws/aws-lc-rs#296 , so it will provide more options. Edit: we switched to using ring to match the rcgen default.

But we could also change it to default to whatever rcgen default or use ring as default depending on your preferences.

@cpu
Copy link
Member

cpu commented Jan 11, 2024

Thanks for the PR!

Because rustls_cert_gen has default-features = false, its build is currently broken.

It sounds like there's CI coverage missing for this tool that would detect this broken state :-( Would you be willing to update your PR to add that coverage as well?

@stormshield-gt
Copy link
Contributor Author

It's a feature unification problem, when we build at a workspace level, all features get unified so the ring feature from rcgen is also added to rustls_cert_gen that's why the CI doesn't catch it.

One solution is to add a CI job with cargo build --package rustls-cert-gen or to use an external tools like cargo hack, as shown in this thread

Which solution do you prefer?

@cpu
Copy link
Member

cpu commented Jan 11, 2024

It's a feature unification problem

Aha, that makes sense 💡 Thank you

One solution is to add a CI job with cargo build --package rustls-cert-gen

That sounds good to me. We're using cargo hack in some other Rustls projects so I'm not opposed to that solution if another maintainer prefers it, but I think we can start with the simplest option.

rustls-cert-gen/Cargo.toml Outdated Show resolved Hide resolved
@stormshield-gt stormshield-gt force-pushed the choose_crypto_provider_for_rustls_cert_gen branch from a92df3e to 35d8c6f Compare January 12, 2024 07:39
@stormshield-gt
Copy link
Contributor Author

stormshield-gt commented Jan 12, 2024

We see that's the new job failed the CI as expected https://github.com/rustls/rcgen/actions/runs/7499266190 , I will now push the fix

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks!

@cpu
Copy link
Member

cpu commented Jan 12, 2024

This feels small and obvious enough to merge without blocking on an additional review. If there's any post-merge feedback I will commit to implementing it.

@cpu cpu added this pull request to the merge queue Jan 12, 2024
Merged via the queue into rustls:main with commit a3831c9 Jan 12, 2024
21 checks passed
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.

3 participants