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

Add support for aws_lc_rs as crypto backend #201

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

BiagioFesta
Copy link
Contributor

@BiagioFesta BiagioFesta commented Dec 13, 2023

Starting from 0.22.0, rustls, ecosystem has started supporting aws-lc-rs as crypto back-end. This latter replaces crypto functionalities provided by ring.

The main reason behind this is to provide FIPS compliance in the future via aws-lc:

Optional support for cryptography from aws-lc-rs. Once the certification process completes , we will support FIPS mode using aws-lc-rs.


This PR is implementing the same supporting mechanism.

In short:

  • if cfg(feature = "ring") -> ring (this will be the default)
  • if cfg(feature = "aws-lc-rc") -> aws-lc-rs
  • if both -> compile_error! EDIT: ring
  • if none -> compile_error!

@djc
Copy link
Member

djc commented Dec 13, 2023

Given that Cargo features are supposed to be additive, I'm not a fan of this:

  • if both -> compile_error!

I would suggest just picking ring in that case, or providing explicit access to a crypto provider via some kind of trait (not sure how invasive that would be to the API).

@cpu
Copy link
Member

cpu commented Dec 13, 2023

I was hoping the aws-lc-rs backend would unblock RSA key generation support, but it looks as though there's upstream work required. I've filed a feature req: aws/aws-lc-rs#296

@BiagioFesta
Copy link
Contributor Author

Given that Cargo features are supposed to be additive, I'm not a fan of this:

  • if both -> compile_error!

I would suggest just picking ring in that case, or providing explicit access to a crypto provider via some kind of trait (not sure how invasive that would be to the API).

Fixed, CI still fails also because some --no-default-features tests. What about that case?

@est31
Copy link
Member

est31 commented Dec 13, 2023

compile_error is fine in that case, tests should be changed to either specify ring or aws_lc_rs. The main important property is that cargo features stay additive.

rcgen/src/lib.rs Outdated Show resolved Hide resolved
@est31
Copy link
Member

est31 commented Dec 13, 2023

Maybe the name can be bikeshedded but otherwise this is fine to go.

@BiagioFesta BiagioFesta force-pushed the aws-lc-support branch 2 times, most recently from 914584d to 8883c41 Compare December 13, 2023 21:24
@BiagioFesta
Copy link
Contributor Author

Codecoverage test fails but I think it is not fair :)

@cpu
Copy link
Member

cpu commented Dec 13, 2023

Codecoverage test fails but I think it is not fair :)

Yup! The flagged lines are missing coverage on main, don't worry about that one 👍

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 for taking this on 🚀

I think it would be interesting to consider how the KeyPair APIs might need to change to allow runtime selection of the backing crypto library but it might make sense to hash that out in a follow-up issue.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
rcgen/src/key_pair.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
rcgen/src/ring_like.rs Outdated Show resolved Hide resolved
rcgen/src/ring_like.rs Outdated Show resolved Hide resolved
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Nothing to add to @cpu's feedback, nice work!

@BiagioFesta BiagioFesta force-pushed the aws-lc-support branch 3 times, most recently from 92c24b5 to 5b668a8 Compare December 14, 2023 11:57
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 for iterating on this. There's just one last thing to fix:

The Windows CI task fails right now with the aws-lc-rs backend feature because it needs nasm as a build time dependency on that OS. Could you add something like this to the relevant bit of CI?

There's an upstream issue that I hope will let us remove this in the future.

@cpu cpu enabled auto-merge December 14, 2023 18:49
@cpu cpu added this pull request to the merge queue Dec 14, 2023
@cpu
Copy link
Member

cpu commented Dec 14, 2023

Thanks!

Merged via the queue into rustls:main with commit acec387 Dec 14, 2023
16 of 17 checks passed
@BiagioFesta BiagioFesta deleted the aws-lc-support branch December 14, 2023 19:10
@est31 est31 mentioned this pull request Dec 15, 2023
@BiagioFesta BiagioFesta mentioned this pull request Mar 4, 2024
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.

None yet

4 participants