Support committed generated test data and generate PQCP test data #278

2026-02-11T11:26:51-06:00

gilles-peskine-arm commented

2026-02-11 11:26:51 -06:00

(Migrated from github.com)

Prepare to support more committed generated files.
Prepare to have additional Python scripts in framework/util, according to the design proposed in https://github.com/Mbed-TLS/mbedtls-framework/pull/272.
New script to generate ML-DSA test data, in partial fulfillment of https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/633. Requires steps 1 and 2.

Framework part of https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/690.

PR checklist

Please add the numbers (or links) of the associated pull requests for consuming branches. You can omit branches where this pull request is not needed.

TF-PSA-Crypto PR provided https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/690
development PR TODO
3.6 PR not required because: 3.6 doesn't use any of the new stuff

1. Prepare to support more committed generated files. 2. Prepare to have additional Python scripts in `framework/util`, according to the design proposed in https://github.com/Mbed-TLS/mbedtls-framework/pull/272. 3. New script to generate ML-DSA test data, in partial fulfillment of https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/633. Requires steps 1 and 2. Framework part of https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/690. ## PR checklist Please add the numbers (or links) of the associated pull requests for consuming branches. You can omit branches where this pull request is not needed. - [x] **TF-PSA-Crypto PR** provided https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/690 - [ ] **development PR** TODO - [x] **3.6 PR** not required because: 3.6 doesn't use any of the new stuff

bjwtaylor (Migrated from github.com) reviewed

2026-02-14 03:49:35 -06:00

bjwtaylor (Migrated from github.com) left a comment

Not had time to do a full review, however added a couple of initial comments/questions.

scripts/mbedtls_framework/crypto_knowledge.py

Is there a byte/bit mismatch here as all entries in the array appear to be in bytes not bits, so should the SM3 entry be 32 not 256?

Is there a bits/bytes mistmatch here most of the entries in the table appear to be bytes. ie. MD5 16 etc. However SM3 is 256, however if I've understood correctly this is the bitlen and this value should the byte length ie 32?

util/requirements.txt

					
				@ -0,0 +1,5 @@

				# Python module requirements for maintainer utilities

Does this file need a SPDX copywrite notice?

gilles-peskine-arm (Migrated from github.com) reviewed

2026-02-16 06:08:56 -06:00

util/requirements.txt

					
				@ -0,0 +1,5 @@

				# Python module requirements for maintainer utilities

Ideally yes, but there are no notices in the other requirements.txt files, and we routinely create new files that don't have notices (e.g. tests/suites/*.data and tests/suites/*.function), so this is fine.

Ideally yes, but there are no notices in the other `requirements.txt` files, and we routinely create new files that don't have notices (e.g. `tests/suites/*.data` and `tests/suites/*.function`), so this is fine.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-02-16 06:09:23 -06:00

scripts/mbedtls_framework/crypto_knowledge.py

Handled in https://github.com/Mbed-TLS/mbedtls-framework/pull/265.

valeriosetti commented

2026-02-18 10:32:05 -06:00

(Migrated from github.com)

Since #265 has been merged, you might rebase this PR so that we can get it merged and unlock https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/690 as well ;)

gilles-peskine-arm commented

2026-02-18 11:39:01 -06:00

(Migrated from github.com)

Oh, I hadn't rebased because there was no conflict, but now there is one! Ronald and I simultaneously created a file with the same name... I'll rework this then.

gilles-peskine-arm commented

2026-02-19 11:27:30 -06:00

(Migrated from github.com)

I've rebased on top of the merge of https://github.com/Mbed-TLS/mbedtls-framework/pull/265. I also renamed scripts/mbedtls_framework/generated_files.py to scripts/mbedtls_framework/generate_files_helper.py to disambiguate it from the scripts/mbedtls_framework/generated_files.py that Ronald added in parallel. This is ready for review if the CI agrees.

I've rebased on top of the merge of https://github.com/Mbed-TLS/mbedtls-framework/pull/265. I also renamed `scripts/mbedtls_framework/generated_files.py` to `scripts/mbedtls_framework/generate_files_helper.py` to disambiguate it from the `scripts/mbedtls_framework/generated_files.py` that Ronald added in parallel. This is ready for review if the CI agrees.

valeriosetti (Migrated from github.com) reviewed

2026-02-20 04:56:00 -06:00

mpg (Migrated from github.com) reviewed

2026-02-24 06:03:31 -06:00

mpg (Migrated from github.com) left a comment

(Taking over from Ben as a reviewer while he's away.)

Looking pretty good to me. Just a minor point that can be ignored, and a question that's probably just me missing something but I'd still like it answered before I formally approve.

(Taking over from Ben as a reviewer while he's away.) Looking pretty good to me. Just a minor point that can be ignored, and a question that's probably just me missing something but I'd still like it answered before I formally approve.

util/generate_mldsa_tests.py

					
				@ -0,0 +40,4 @@

				    #pylint: disable=too-few-public-methods

				    def __init__(self, kl: int, seed: bytes) -> None:

				        self.kl = kl #pylint: disable=invalid-name

Minor: rather than silencing pylint, we could also call it something more explicit. I'm not sure what kl stands for tbh. I'm assuming 87 means k =8 and l = 7 or something like this?

Minor: rather than silencing pylint, we could also call it something more explicit. I'm not sure what `kl` stands for tbh. I'm assuming 87 means k =8 and l = 7 or something like this?

util/generate_mldsa_tests.py

					
				@ -0,0 +144,4 @@

				def one_mldsa_verify_pure(api: API,

				                          key: Key,

				                          message: bytes,

				                          deterministic: bool,

Sorry, this is probably just me lacking contextual knowledge, but how does "deterministic" make sense for the verification?

gilles-peskine-arm (Migrated from github.com) reviewed

2026-02-24 08:56:45 -06:00

util/generate_mldsa_tests.py

					
				@ -0,0 +40,4 @@

				    #pylint: disable=too-few-public-methods

				    def __init__(self, kl: int, seed: bytes) -> None:

				        self.kl = kl #pylint: disable=invalid-name

Indeed, “87” is a string, not a number, representing the pair of integers (8, 7). Those integers are called (k, l) in mathematical descriptions. We use 87 as a ”key bit-size“ in the PSA API, but with a note that we're repurposing an existing field, and the number is not actually the size of anything.

When describing ML-DSA, (k, l) is usually called the “parameter set”. I found parameter_set to be a bit long, even for me. Also, to be more precise, it's the designation of the part of the parameter set for which several variants have been made official — the underlying lattice construction has more parameters. I guess k_l would also meet the rule that we've set for variable names, without being unwieldy or vague.

Indeed, “87” is a string, not a number, representing the pair of integers (8, 7). Those integers are called (k, l) in mathematical descriptions. We use 87 as a ”key bit-size“ in the PSA API, but with a note that we're repurposing an existing field, and the number is not actually the size of anything. When describing ML-DSA, (k, l) is usually called the “parameter set”. I found `parameter_set` to be a bit long, even for me. Also, to be more precise, it's the designation of the part of the parameter set for which several variants have been made official — the underlying lattice construction has more parameters. I guess `k_l` would also meet the rule that we've set for variable names, without being unwieldy or vague.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-02-24 09:02:28 -06:00

util/generate_mldsa_tests.py

					
				@ -0,0 +40,4 @@

				    #pylint: disable=too-few-public-methods

				    def __init__(self, kl: int, seed: bytes) -> None:

				        self.kl = kl #pylint: disable=invalid-name

Switching to k_l makes me think that the data is a pair (k, l), but the variable actually contains the number, because the number is what you need to pass to various APIs. So I think kl is a better name. I'd rather not make the code less readable for humans just to satisfy a rule whose general intent is to make code more readable, but that doesn't handle this specific case well.

[Switching to `k_l`](https://github.com/gilles-peskine-arm/mbedtls-framework/tree/generate_mldsa_tests-create-k_l) makes me think that the data is a pair `(k, l)`, but the variable actually contains the number, because the number is what you need to pass to various APIs. So I think `kl` is a better name. I'd rather not make the code less readable for humans just to satisfy a rule whose general intent is to make code more readable, but that doesn't handle this specific case well.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-02-24 09:08:26 -06:00

util/generate_mldsa_tests.py

					
				@ -0,0 +144,4 @@

				def one_mldsa_verify_pure(api: API,

				                          key: Key,

				                          message: bytes,

				                          deterministic: bool,

It's about the data, not the algorithm. My intent was to have test data be either a valid deterministic signature, or some other valid signature. As I went to write this in the function documentation, I realized that I hadn't implemented the non-deterministic case correctly, so I went and did that.

mpg (Migrated from github.com) reviewed

2026-02-26 02:24:38 -06:00

util/generate_mldsa_tests.py

					
				@ -0,0 +40,4 @@

				    #pylint: disable=too-few-public-methods

				    def __init__(self, kl: int, seed: bytes) -> None:

				        self.kl = kl #pylint: disable=invalid-name

Ok, let's stick with kl then.

I'd rather not make the code less readable for humans just to satisfy a rule whose general intent is to make code more readable, but that doesn't handle this specific case well.

Sure! It's just that with the little context I had, I didn't find kl super readable, so I thought the rule was handling this case well. But after discussion, kl seems to be the best choice, so let's stick with it.

Ok, let's stick with kl then. > I'd rather not make the code less readable for humans just to satisfy a rule whose general intent is to make code more readable, but that doesn't handle this specific case well. Sure! It's just that with the little context I had, I didn't find `kl` super readable, so I thought the rule was handling this case well. But after discussion, `kl` seems to be the best choice, so let's stick with it.

mpg (Migrated from github.com) reviewed

2026-02-26 02:29:34 -06:00

util/generate_mldsa_tests.py

					
				@ -0,0 +144,4 @@

				def one_mldsa_verify_pure(api: API,

				                          key: Key,

				                          message: bytes,

				                          deterministic: bool,

Ok, it's still not clear to me why we want to test verification on both sets of data. Perhaps I have the wrong intuition here (mostly transposed from ECDSA), but it seems to me the verifier can't know whether the signature was generated using the deterministic scheme or not (that would require knowing the private key), so it seems unlikely to me that both sets of data would exercise different paths in the verification code.

I'm not opposed to having both, in fact I'm almost never opposed to having more test data. I'm just trying to understand if there's something about ML-DSA I'm missing that makes this more useful than I feel it is.

Ok, it's still not clear to me why we want to test verification on both sets of data. Perhaps I have the wrong intuition here (mostly transposed from ECDSA), but it seems to me the verifier can't know whether the signature was generated using the deterministic scheme or not (that would require knowing the private key), so it seems unlikely to me that both sets of data would exercise different paths in the verification code. I'm not opposed to having both, in fact I'm almost never opposed to having more test data. I'm just trying to understand if there's something about ML-DSA I'm missing that makes this more useful than I feel it is.

mpg (Migrated from github.com) approved these changes

2026-02-26 02:31:51 -06:00

mpg (Migrated from github.com) left a comment

LGTM, thanks!

gilles-peskine-arm (Migrated from github.com) reviewed

2026-02-26 03:09:01 -06:00

util/generate_mldsa_tests.py

					
				@ -0,0 +144,4 @@

				def one_mldsa_verify_pure(api: API,

				                          key: Key,

				                          message: bytes,

				                          deterministic: bool,

It's not strictly necessary, but hardly anything about test coverage ever is. My thinking is, there are many valid signatures for a given (key, message) pair, so we should test multiple signatures. Of the valid signatures, one has a special distinction (deterministic) and the others are parametrized by a random value, so we should test the special one and one of the non-special ones.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-02-26 04:53:18 -06:00

util/generate_mldsa_tests.py

					
				@ -0,0 +144,4 @@

				def one_mldsa_verify_pure(api: API,

				                          key: Key,

				                          message: bytes,

				                          deterministic: bool,

To clarify, from a gray-box perspective, it's impossible to distinguish the deterministic signature from any other valid signature unless you have the private key, so a verification function that only has the public key and accepts the deterministic signature must also accept all the randomized signatures. But from a black-box perspective, I prefer to also test a signature that isn't the deterministic one.

valeriosetti (Migrated from github.com) approved these changes

2026-02-27 04:12:28 -06:00

Sign in to join this conversation.

No reviewers

mpg

valeriosetti

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/mbedtls-framework!278

				`@ -0,0 +1,5 @@`
				`# Python module requirements for maintainer utilities`

Rows
Columns