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

Consolidate EVP_PKEY usage; Add (unstable) ML-DSA API #679

Closed
wants to merge 6 commits into from

Conversation

justsmth
Copy link
Contributor

@justsmth justsmth commented Jan 31, 2025

Description of changes:

  • Consolidates various places we call EVP_PKEY_keygen
  • Consolidates various placeswe call EVP_DigestSign and EVP_DigestVerify.
  • Adds new API for ML-DSA under the unstable module.

Call-outs:

  • RSA key generation will now use EVP_PKEY_keygen. When the "fips" feature is enabled, the FIPS-approved key generation mechanism will be used for all RSA keys.

Testing:

  • Tests cover almost all of the new ML-DSA APIs.
  • TODO: More ML-DSA tests.
  • TODO: Review available ML-DSA KAT tests from AWS-LC. Add those appropriate to aws-lc-rs.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@justsmth justsmth force-pushed the api-ml-dsa branch 3 times, most recently from b78d1ff to 4dc893c Compare February 3, 2025 21:13
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 88.97059% with 60 lines in your changes missing coverage. Please review.

Project coverage is 93.00%. Comparing base (c358484) to head (d2f1dd0).
Report is 167 commits behind head on main.

Files with missing lines Patch % Lines
aws-lc-rs/src/pqdsa/key_pair.rs 87.93% 3 Missing and 18 partials ⚠️
aws-lc-rs/src/evp_pkey.rs 91.17% 8 Missing and 7 partials ⚠️
aws-lc-rs/src/ed25519.rs 33.33% 4 Missing and 2 partials ⚠️
aws-lc-rs/src/rsa/key.rs 75.00% 2 Missing and 2 partials ⚠️
aws-lc-rs/src/pqdsa/signature.rs 92.68% 0 Missing and 3 partials ⚠️
aws-lc-rs/src/ec/key_pair.rs 75.00% 0 Missing and 2 partials ⚠️
aws-lc-rs/src/pqdsa.rs 98.00% 2 Missing ⚠️
aws-lc-rs/src/rsa/encryption.rs 0.00% 1 Missing and 1 partial ⚠️
aws-lc-rs/src/ec.rs 85.71% 1 Missing ⚠️
aws-lc-rs/src/ec/encoding.rs 66.66% 0 Missing and 1 partial ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #679      +/-   ##
==========================================
- Coverage   95.80%   93.00%   -2.80%     
==========================================
  Files          61       71      +10     
  Lines        8143     9754    +1611     
  Branches        0     9754    +9754     
==========================================
+ Hits         7801     9072    +1271     
- Misses        342      395      +53     
- Partials        0      287     +287     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justsmth justsmth changed the title [DRAFT] (Unstable) ML-DSA API Consolidate EVP_PKEY usage; Add (unstable) ML-DSA API Feb 6, 2025
@justsmth justsmth marked this pull request as ready for review February 6, 2025 17:45
@justsmth justsmth requested a review from a team as a code owner February 6, 2025 17:45
Comment on lines +261 to 265
/// The longest signature is for ML-DSA-87
pub(crate) const MAX_LEN: usize = 4627;

/// A public key signature returned from a signing operation.
#[derive(Clone, Copy)]
Copy link
Contributor Author

@justsmth justsmth Feb 6, 2025

Choose a reason for hiding this comment

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

We have a signature::Signature struct intended to be the type for signatures we return; it's currently used by EcdsaKeyPair::sign and Ed25519KeyPair::sign, but not RsaKeyPair::sign. I setup PqdsaKeyPair::sign to use it. Unfortunately, the signature::Signature type is Copy so its use requires that a nearly 5KB object be passed around on the stack.

I'm wondering whether we should create a separate type (e.g, PqdsaSignature) that's not Copy, or follow the pattern used for RSA and have the consumer provide the buffer that we write the signature into.

@justsmth
Copy link
Contributor Author

justsmth commented Feb 6, 2025

Closing this PR. I'm planning to split this into two PRs.

@justsmth justsmth closed this Feb 6, 2025
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.

2 participants