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 to export ML-DSA key-pairs in seed format #2194

Merged
merged 42 commits into from
Feb 28, 2025

Conversation

jakemas
Copy link
Contributor

@jakemas jakemas commented Feb 13, 2025

Issues:

Resolves #CryptoAlg-2918
#ACCP-130

Description of changes:

Support the ability to export ML-DSA key seeds. We modify the core algorithm implementation to store the seed used during key generation. This will allow the key pair to be reconstructed at a later stage from just the seed.

This is performed within ml_dsa_keypair, which has been modified to accept an addition argument seed that is a pointer to output array of ML_DSA_SEEDBYTES bytes.

int ml_dsa_keypair(ml_dsa_params *params, uint8_t *pk, uint8_t *sk, uint8_t *seed) {
- uint8_t seed[ML_DSA_SEEDBYTES];
  if (!RAND_bytes(seed, ML_DSA_SEEDBYTES)) {
    return -1;
  }
  ml_dsa_keypair_internal(params, pk, sk, seed);
- OPENSSL_cleanse(seed, sizeof(seed));
  return 0;
}

These changes bubble up to the ml_dsa.c definitions of keygen, that are now modified to support the provided buffer to store the seed:

int ml_dsa_44_keypair(uint8_t *public_key   /* OUT */,
                      uint8_t *private_key  /* OUT */,
+                     uint8_t *seed         /* OUT */) {
  ml_dsa_params params;
  ml_dsa_44_params_init(&params);
  return (ml_dsa_keypair(&params, public_key, private_key, seed) == 0);
}

We store the seed in the PQDSA_KEY struct during pkey_pqdsa_keygen:

struct pqdsa_key_st {
  const PQDSA *pqdsa;
  uint8_t *public_key;
  uint8_t *private_key;
+ uint8_t *seed;
};

API Changes

This PR modifies the ASN.1 encoding function for PQDSA keys. The function pqdsa_priv_encode now encodes the associated pqdsa->key->seed. As such the EVP API EVP_marshal_private_key will export the private seed. This has been noted in documentation.

Performance Impact

Converting to seed-based storage for both public and private keys yields the following improvements:

ML-DSA-44: 99.17% (121x) reduction (3,840 bytes saved per key-pair).
ML-DSA-65: 99.46% (187x) reduction (5,952 bytes saved per key-pair).
ML-DSA-87: 99.57% (234x) reduction (7,456 bytes saved per key-pair).

Converting to seed-based storage for private keys yields the following improvements:

ML-DSA-44: 98.75% (80x) reduction (2,528 bytes saved per private key).
ML-DSA-65: 99.21% (126x) reduction (5,952 bytes saved per private key).
ML-DSA-87: 99.35% (153x) reduction (7,456 bytes saved per private key).

The proposed seed-based approach achieves an average storage reduction of 99.4% across all ML-DSA variants.

Call-outs:

  • FIPS Compliance: I'm glad you're asking, yes this is compliant with FIPS, NIST have published PQC FAQs specifically to address this exact implementation: https://csrc.nist.gov/Projects/post-quantum-cryptography/faqs#Rdc7.

  • 3692f72 implemented PQDSA_KEY_get_priv_raw_seed if it is needed in future.

  • Modified ASN.1 method signatures to include oid parameter for better format handling

Testing:

Added a failure mode test to ParsePrivateKey for the case that a key does not have an associated seed.

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.

@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 77.92208% with 17 lines in your changes missing coverage. Please review.

Project coverage is 79.04%. Comparing base (4898adb) to head (f718c29).

Files with missing lines Patch % Lines
crypto/fipsmodule/pqdsa/pqdsa.c 42.10% 11 Missing ⚠️
crypto/evp_extra/p_pqdsa_asn1.c 78.57% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2194      +/-   ##
==========================================
- Coverage   79.06%   79.04%   -0.03%     
==========================================
  Files         612      612              
  Lines      106483   106512      +29     
  Branches    15050    15052       +2     
==========================================
- Hits        84192    84189       -3     
- Misses      21639    21669      +30     
- Partials      652      654       +2     

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

@jakemas jakemas marked this pull request as ready for review February 13, 2025 22:07
@jakemas jakemas requested a review from a team as a code owner February 13, 2025 22:07
Copy link
Member

@skmcgrail skmcgrail left a comment

Choose a reason for hiding this comment

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

Have we just considered exposing access to the opaque PQDSA_KEY type stored in the EVP and then having more proper access functions like get_private_key_seed etc using that type? I'm not a big fan of the "raw" key meaning on EVP because it differs based on the key types, and now can very on the size dimension which seems really goofy.

@jakemas
Copy link
Contributor Author

jakemas commented Feb 18, 2025

Have we just considered exposing access to the opaque PQDSA_KEY type stored in the EVP and then having more proper access functions like get_private_key_seed etc using that type? I'm not a big fan of the "raw" key meaning on EVP because it differs based on the key types, and now can very on the size dimension which seems really goofy.

Agreed, thank you for the feedback. I've updated the implementation (c307952) so that EVP_PKEY_get_raw_private_key is unchanged. Instead, I created PQDSA_KEY_get_priv_raw_seed in pqdsa.c so that we can access the seed through this function. I haven't exported or exposed that new function yet, as I am under the impression that exporting these seeds as DER encodings using EVP_marshal_private_key_v2 is sufficient for now. It would be simple to add an external API from here, but we might like to land on a few decisions around that part first.

@WillChilds-Klein WillChilds-Klein self-requested a review February 20, 2025 14:55
@WillChilds-Klein
Copy link
Contributor

@jakemas -- what happens if we parse a private key in "expanded" form, then try to extract the seed? It seems to me this should fail.

@WillChilds-Klein WillChilds-Klein self-requested a review February 20, 2025 21:10
@jakemas
Copy link
Contributor Author

jakemas commented Feb 20, 2025

@jakemas -- what happens if we parse a private key in "expanded" form, then try to extract the seed? It seems to me this should fail.

Correct, asserted that with a failure mode test in 6d6127f

@WillChilds-Klein
Copy link
Contributor

@jakemas -- I think we need to update documentation here.

@jakemas
Copy link
Contributor Author

jakemas commented Feb 24, 2025

f178391

Thank you, added here f178391

@WillChilds-Klein WillChilds-Klein enabled auto-merge (squash) February 28, 2025 19:42
@nebeid nebeid disabled auto-merge February 28, 2025 20:12
@nebeid nebeid enabled auto-merge (squash) February 28, 2025 20:16
@nebeid nebeid merged commit cbcba97 into aws:main Feb 28, 2025
110 of 114 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.

6 participants