Support keypair calculation for PQDSA PKEY #2145

jakemas · 2025-01-28T20:04:32Z

Issues:

Resolves #CryptoAlg-2868

Description of changes:

Following from the first part of this PR to add low level support for ML-DSA public key generation from private keys. This PR uses the new pqdsa method pqdsa->pqdsa_pack_pk_from_sk. We supply ml_dsa_44_pack_pk_from_sk, ml_dsa_65_pack_pk_from_sk, and ml_dsa_87_pack_pk_from_sk from ml_dsa.h to provide this functionality for ML-DSA.

As we use EVP_parse_private_key to import PQDSA keys into AWS-LC, we need to modify the PQDSA asn.1 private key decoding function to additionally populate the corresponding public key.

The pqdsa_priv_decode function in p_pqdsa_asn1.c has been modified to attempt to generate a public key from the provided secret key, should the method be defined for that pqdsa structure. As ML-DSA is the only current PQDSA, the library currently does implement methods for all PQDSA PKEY types.

Call-outs:

what happens if a new PQDSA algorithm is introduced to aws-lc, and doesn't support pk calculation from sk?

We populate the methods as NULL and will modify the G.test that expects the public key to be generated. We can make this change simply if/when we add to the PQDSA capabilites with an algorithm that doesn't support pk from sk.

Testing:

To test functionality, I have adapted the PQDSAParameterTest G.test MarshalParse to now verify that the public key is populated after calling EVP_parse_private_key. We then verify that the public key calculated is equal to the original public key.

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.

andrewhop · 2025-01-28T20:14:01Z

crypto/evp_extra/p_pqdsa_asn1.c

+    return 0;
+  }
+
+  // Calculate public key from private key if method exists


How expensive is this calculation? What customer workflow needs the public key given the private key? Could this be lazily initialized?

Other types of EVP_PKEY (which have public/private components) ensure that the public key is populated whenever a private key is deserialized. For example, see EC and ED25519.

Other EVP_PKEY_xxx methods (such as EVP_PKEY_cmp) assume that the public key is populated for any EVP_PKEY that has functions available for public keys. (In fact EVP_PKEY_cmp can currently SEGFAULT for EVP_PKEY_PQDSA due to this.)

There might be other places for this computation to take place, but it needs to be done when deserializing a private key.

Ahhhh good point, looking at ED25519 a little more should we add raw support?

priv_decode -> ed25519_priv_decode decodes the private key and calls ed25519_set_priv_raw

set_priv_raw -> ed25519_set_priv_raw calculates the public key from the private key, and compares it to the supplied public key if provided

Here in ML-DSA:

priv_decode -> pqdsa_priv_decode decodes the private key and calculates the public key and sets it

set_prive_raw -> not implemented for pqdsa

We do implement set priv raw for pqdsa, just not as part of the EVP methods...

Some more background: set_priv_raw is not implemented for ML-DSA or ML-KEM. In ED25519 things are a little different, as there are no "parameter sets" for ED25519, it's just one algorithm. In ML-DSA/ML-KEM we have multiple NIDs to define algorithm versions.

As such, we implement EVP_PKEY_pqdsa_new_raw_private_key at https://github.com/aws/aws-lc/blob/main/include/openssl/evp.h#L960-L970 just like for KEMs. I suppose your suggestion is then to remove EVP_PKEY_pqdsa_new_raw_private_key / EVP_PKEY_pqdsa_new_raw_public_key and support via EVP_PKEY_new_raw_private_key/ EVP_PKEY_new_raw_public_key? The problem is, EVP_PKEY_new_raw_private_key expects type not nid and type = EVP_PKEY_PQDSA is not specific enough to know which NID it is... there's no OID, and we can't use the length to determine uniqueness as another scheme could have the same length.

In the aws-lc-rs, I'm just using the length of the "raw" key to differentiate between the NIDs under the EVP_PKEY_PQDSA type: justsmth/aws-lc-rs@088e116

Would this be an acceptable approach for more cohesive integration into EVP_PKEY_new_raw_private_key and EVP_PKEY_new_raw_public_key?

Yeah I considered that too, it works, but it does present an issue should another algorithm be added that has the same length of raw keys -- which may sound unlikely, but would present issues with HashML-DSA that has a different OID/NID but same lengths. I do have some ideas -- but likely out of scope for this PR.

andrewhop · 2025-01-28T20:16:51Z

crypto/evp_extra/p_pqdsa_asn1.c

+    }
+    // set the public key
+    int ret = PQDSA_KEY_set_raw_public_key(out->pkey.pqdsa_key, public_key);
+    OPENSSL_free(public_key);


Since public_key was allocated internally can the PKEY just take ownership of it without having to copy it?

I don't see this as being possible, as the size of the public_key is dynamic, and defined by out->pkey.pqdsa_key->pqdsa->public_key_len. Am I missing something?

Yeah, you allocate the key on L162, looking at PQDSA_KEY_set_raw_public_key it just calls OPENSSL_memdup which allocates a new buffer and memcpy the data into that new buffer, then this function frees public_key. Why not store public_key in out->pkey.pqdsa_key->public_key?

got it! 54224d2

codecov-commenter · 2025-01-28T20:32:26Z

Codecov Report

Attention: Patch coverage is 53.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.96%. Comparing base (ea58b3f) to head (54224d2).

Files with missing lines	Patch %	Lines
crypto/evp_extra/p_pqdsa_asn1.c	50.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2145      +/-   ##
==========================================
- Coverage   78.97%   78.96%   -0.02%     
==========================================
  Files         611      611              
  Lines      105500   105514      +14     
  Branches    14938    14941       +3     
==========================================
- Hits        83316    83314       -2     
- Misses      21531    21547      +16     
  Partials      653      653

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

justsmth · 2025-01-28T20:14:41Z

crypto/evp_extra/p_pqdsa_asn1.c

+  }
+
+  // Calculate public key from private key if method exists
+  if (out->pkey.pqdsa_key->pqdsa->method->pqdsa_pack_pk_from_sk != NULL) {


Shouldn't we always return failure (0) if we're unable to initialize the public key? I assume the pqdsa_pack_pk_from_sk function ptr should always be non-NULL here (line 360) so this condition would never fail?

Upon seeing a successful return code, a caller assumes the EVP_PKEY is initialized and might subsequently SEGFAULT on a call to (e.g.) EVP_PKEY_cmp.

Ah I see. I was trying to prevent against a failure if the ptr is non-null, as you say. Removed this check in cd8da04 and cleaned up the surrounding logic to only return SUCESS when both priv and pub are populated.

justsmth · 2025-01-28T20:40:01Z

crypto/evp_extra/p_pqdsa_asn1.c

+    return 0;
+  }
+
+  // Calculate public key from private key if method exists


Other types of EVP_PKEY (which have public/private components) ensure that the public key is populated whenever a private key is deserialized. For example, see EC and ED25519.

Other EVP_PKEY_xxx methods (such as EVP_PKEY_cmp) assume that the public key is populated for any EVP_PKEY that has functions available for public keys. (In fact EVP_PKEY_cmp can currently SEGFAULT for EVP_PKEY_PQDSA due to this.)

There might be other places for this computation to take place, but it needs to be done when deserializing a private key.

… into pqdsa-keypair-consistency

### Issues: Resolves #CryptoAlg-2868 ### Description of changes: Following from the first part of this PR to add low level support for ML-DSA public key generation from private keys. This PR uses the new `pqdsa` method `pqdsa->pqdsa_pack_pk_from_sk`. We supply `ml_dsa_44_pack_pk_from_sk`, `ml_dsa_65_pack_pk_from_sk`, and `ml_dsa_87_pack_pk_from_sk` from `ml_dsa.h` to provide this functionality for ML-DSA. As we use `EVP_parse_private_key` to import `PQDSA` keys into AWS-LC, we need to modify the `PQDSA` asn.1 private key decoding function to additionally populate the corresponding public key. The `pqdsa_priv_decode` function in `p_pqdsa_asn1.c` has been modified to attempt to generate a public key from the provided secret key, should the method be defined for that `pqdsa` structure. As ML-DSA is the only current `PQDSA`, the library currently does implement methods for all `PQDSA` PKEY types. ### Call-outs: > what happens if a new `PQDSA` algorithm is introduced to aws-lc, and doesn't support `pk` calculation from `sk`? We populate the methods as `NULL` and will modify the G.test that expects the public key to be generated. We can make this change simply if/when we add to the `PQDSA` capabilites with an algorithm that doesn't support `pk` from `sk`. ### Testing: To test functionality, I have adapted the `PQDSAParameterTest` G.test `MarshalParse` to now verify that the public key is populated after calling `EVP_parse_private_key`. We then verify that the public key calculated is equal to the original public key. 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.

added keypair calculation to pqdsa pkey

3d1bff4

jakemas requested a review from a team as a code owner January 28, 2025 20:04

jakemas added 3 commits January 28, 2025 12:04

spacing nit

09abb11

spelling mistake

6756bfe

Merge branch 'main' into pqdsa-keypair-consistency

a8c9562

andrewhop reviewed Jan 28, 2025

View reviewed changes

justsmth previously approved these changes Jan 28, 2025

View reviewed changes

jakemas added 2 commits January 28, 2025 13:34

CR fixes

cd8da04

Merge branch 'pqdsa-keypair-consistency' of github.com:jakemas/aws-lc…

aa87eeb

… into pqdsa-keypair-consistency

jakemas dismissed justsmth’s stale review via aa87eeb January 28, 2025 21:35

justsmth previously approved these changes Jan 28, 2025

View reviewed changes

skmcgrail previously approved these changes Jan 28, 2025

View reviewed changes

free allocated memory

db900c0

jakemas dismissed stale reviews from skmcgrail and justsmth via db900c0 January 28, 2025 22:46

CR fix

54224d2

justsmth approved these changes Jan 28, 2025

View reviewed changes

andrewhop approved these changes Jan 29, 2025

View reviewed changes

justsmth merged commit 695c3a0 into aws:main Jan 29, 2025
122 of 126 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support keypair calculation for PQDSA PKEY #2145

Support keypair calculation for PQDSA PKEY #2145

jakemas commented Jan 28, 2025

andrewhop Jan 28, 2025

justsmth Jan 28, 2025

andrewhop Jan 28, 2025

jakemas Jan 28, 2025 •

edited

Loading

justsmth Jan 29, 2025

jakemas Jan 29, 2025 •

edited

Loading

andrewhop Jan 28, 2025

jakemas Jan 28, 2025

andrewhop Jan 28, 2025

jakemas Jan 28, 2025

codecov-commenter commented Jan 28, 2025 •

edited

Loading

justsmth Jan 28, 2025

jakemas Jan 28, 2025

justsmth Jan 28, 2025

Support keypair calculation for PQDSA PKEY #2145

Support keypair calculation for PQDSA PKEY #2145

Conversation

jakemas commented Jan 28, 2025

Issues:

Description of changes:

Call-outs:

Testing:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemas Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemas Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 28, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemas Jan 28, 2025 •

edited

Loading

jakemas Jan 29, 2025 •

edited

Loading

codecov-commenter commented Jan 28, 2025 •

edited

Loading