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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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;
};

To export, this PR supports:

  • EVP_marshal_private_key to export full format private key
  • EVP_marshal_private_key_v2 to export seed format private key
  • PQDSA_KEY_get_priv_raw_seed to export raw format in seed form

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.

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:

Once we have alignment on the EVP API functionality, I will add documentation.

Testing:

Added new test suite Marshalv2ParseSeed for parsing as well as additional test for parsing seeds in RawFunctions

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 58.53659% with 34 lines in your changes missing coverage. Please review.

Project coverage is 79.01%. Comparing base (7518c78) to head (c307952).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
crypto/fipsmodule/pqdsa/pqdsa.c 28.57% 20 Missing ⚠️
crypto/evp_extra/p_pqdsa_test.cc 67.74% 6 Missing and 4 partials ⚠️
crypto/evp_extra/p_pqdsa_asn1.c 78.94% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2194      +/-   ##
==========================================
- Coverage   79.04%   79.01%   -0.03%     
==========================================
  Files         612      612              
  Lines      106067   106239     +172     
  Branches    14985    15013      +28     
==========================================
+ Hits        83842    83950     +108     
- Misses      21571    21632      +61     
- Partials      654      657       +3     

☔ 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.

Comment on lines 21 to 22
static int pqdsa_get_priv_raw_seed(PQDSA_KEY *key, const PQDSA *pqdsa,
uint8_t *out,size_t *out_len) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static int pqdsa_get_priv_raw_seed(PQDSA_KEY *key, const PQDSA *pqdsa,
uint8_t *out,size_t *out_len) {
static int pqdsa_get_priv_raw_seed(PQDSA_KEY *key, const PQDSA *pqdsa,
uint8_t *out,0 size_t *out_len) {
GUARD_PTR(key);
GUARD_PTR(out_len);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to PQDSA_KEY_get_priv_raw_seed in c307952

return 0;
}

if (*out_len != key->pqdsa->keygen_seed_len) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (*out_len != key->pqdsa->keygen_seed_len) {
if (*out_len < key->pqdsa->keygen_seed_len) {

A buffer should be allowed to be bigger I'd imagine? The point is that we set *out_len the actual value of what we wrote. Though I do realize this i gated later, I don't know if it needs to be gated here per-se.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, added in c307952

Comment on lines 38 to 39
static int pqdsa_get_priv_raw_key(PQDSA_KEY *key, const PQDSA *pqdsa,
uint8_t *out,size_t *out_len) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static int pqdsa_get_priv_raw_key(PQDSA_KEY *key, const PQDSA *pqdsa,
uint8_t *out,size_t *out_len) {
static int pqdsa_get_priv_raw_key(PQDSA_KEY *key, const PQDSA *pqdsa,
uint8_t *out, size_t *out_len) {
GUARD_PTR(key);
GUARD_PTR(out_len);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to pqdsa_get_priv_raw in c307952

crypto/evp_extra/p_pqdsa_asn1.c Show resolved Hide resolved
@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.

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