MLDSA: support creating expanded keys #871
No reviewers
Labels
No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-Mbed-TLS/TF-PSA-Crypto!871
Loading…
Reference in a new issue
No description provided.
Delete branch "mldsa-driver-generate-expanded-key"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
generate_keytests totest_suite_dispatch_transparent.drivers/pqcp/builtin/src.tf_psa_crypto_mldsa_expand_private_key()to convert an ML-DSA seed to a joined format (seed + expanded). This function is only declared in ``drivers/pqcp/builtin/src/psa_crypto_mldsa.h` for now. It is meant to become the driver's key import function when the design of that interface progresses.Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/862
Needs preceding PR
.functionfiles: https://github.com/Mbed-TLS/mbedtls-framework/pull/66PR checklist
Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.
@ -27,0 +91,4 @@psa_custom_key_parameters_t custom = PSA_CUSTOM_KEY_PARAMETERS_INIT;custom.flags = custom_flags_arg;size_t output_size = PSA_EXPORT_KEY_OUTPUT_SIZE(type, bits);size_t output_length1 = SIZE_MAX;This would benefit from a comment, something like:
@ -1,3 +1,15 @@generate_key: ECC secp256r1 key pairWouldn't this fail intermittently with a probability of 1/2^240? This is vanishingly small, but still, what is the purpose of this length check?
@ -18,12 +18,172 @@* defined unconditionally. */@ -21,0 +33,4 @@/* For simplicity, we don't support values of e that might need a multi-byte* length encoding in ASN.1 (more than 0x7f digits in base 256, or exactly* 0x7f where the most significant bit is 1). */TEST_LE_U(expected_e_length, 0x7e);Why
0x7eand not0x7f?Still need to do a full review, though some initial comments
@ -104,6 +143,8 @@ psa_status_t tf_psa_crypto_mldsa_export_public_key(* \param message_length The length of \p message, in bytes.* \param[out] signature On success, the exported key.* \param signature_size The size of \p signature, in bytes.* It must be at leastThe documented input-size requirement appears incorrect. For ML-DSA-87, the accepted input representations are exactly:
The 2592-byte value is the encoded public-key size:
32 + (8 × 320) = 2592
It therefore applies to the output buffer (data_size), not key_buffer_size.
Could this document key_buffer_size as exactly TF_PSA_CRYPTO_PQCP_MLDSA_SEED_SIZE or TF_PSA_CRYPTO_PQCP_MLDSA_JOINED_PRIVATE_KEY_SIZE(bits), and retain
TF_PSA_CRYPTO_PQCP_MLDSA_PUBLIC_KEY_SIZE(bits) as the required output size?
@ -1,3 +1,79 @@MLDSA size macrosThe comment here says the test is with bits 56, however the test used 44. Is there a mismatch issue?
@ -1,3 +1,15 @@generate_key: ECC secp256r1 key pairI wanted some validation on the output of
generate_key. Obviously there's no way to fully validate the content. So I wanted to at least validate that the length is sensible, and that the output isn't all-zeros or the same on every call.@ -21,0 +33,4 @@/* For simplicity, we don't support values of e that might need a multi-byte* length encoding in ASN.1 (more than 0x7f digits in base 256, or exactly* 0x7f where the most significant bit is 1). */TEST_LE_U(expected_e_length, 0x7e);0x7e is the maximum length of an integer in base 256 that can be represented in ASN.1 with a single byte for the length. 0x7f doesn't fit if the top bit is 1.
@ -104,6 +143,8 @@ psa_status_t tf_psa_crypto_mldsa_export_public_key(* \param message_length The length of \p message, in bytes.* \param[out] signature On success, the exported key.* \param signature_size The size of \p signature, in bytes.* It must be at leastYes, I must have put this sentence on the wrong line.
@ -21,0 +33,4 @@/* For simplicity, we don't support values of e that might need a multi-byte* length encoding in ASN.1 (more than 0x7f digits in base 256, or exactly* 0x7f where the most significant bit is 1). */TEST_LE_U(expected_e_length, 0x7e);I am sorry, I still don't understand, here it says that the short form is max 127: https://letsencrypt.org/docs/a-warm-welcome-to-asn1-and-der/#length. What am I missing?
Some initial comments. Janos and I are pair-reviewing commit by commit, hence the sporadic comments.
@ -18,12 +18,172 @@* defined unconditionally. */If
expected_e_length > 0and there are no 0s inexpected_e, this will run off the end of the buffer:Could you add the specific name of the public key format from the standard here? It would help to understand this part.
@ -1,3 +1,15 @@generate_key: ECC secp256r1 key pairShould this be 'no more than a handful of bytes shorter'? Or have I misunderstood.
There are 2 points:
PSA_KEY_EXPORT_RSA_KEY_PAIR_MAX_SIZEis a generous overapproximation, so we should expect more than a handful of bytes smaller.@ -1,3 +1,15 @@generate_key: ECC secp256r1 key pairIt's the sum of:
PSA_VENDOR_RSA_GENERATE_MIN_KEY_BITS, becausePSA_KEY_EXPORT_RSA_KEY_PAIR_MAX_SIZEoverapproximates.I'll expand the comment.
The CI is failing because I put a non-ASCII character in a
.functionfile. I can either merge https://github.com/Mbed-TLS/mbedtls-framework/pull/66 (needs reviewers) or work around it.@ -0,0 +20,4 @@/* Define macros for key types and algorithms here in a private header,* rather than in a public header, because ML-DSA is not yet supported* through the API. Notably:This sentence fragment "through the API" needs to be competed or removed.
@ -0,0 +1,111 @@/**Typo: Here and elsewhere:
I found some typos, but other than that, it looks good to me.
Apologies for the delay in getting this reviewed, and the resulting conflict. A lot of new context to load since I haven't looked at ML-DSA much before.
I have one relatively minor question but LGTM otherwise.
@ -1,3 +1,79 @@MLDSA size macrosJust to clarify: Is this meant to test a standard size that is not supported or a non-standard size?
I'm just asking since "56" -> "65" has happened elsewhere.
@ -1,3 +1,79 @@MLDSA size macrosThat one is meant to be a standard size like the one above, not a non-standard size like the one below, thanks for catching it.
The dependencies and test names also don't seem quite right for test cases with bits≠87.
I've rebased on top of
developmentbecause there was a merge conflict on the framework update. No framework update is needed now.I'll fix the bits≠87 test case(s) in a new commit.
LGTM, thanks!
LGTM