[framework] tests: pk: add a common function to create a PSA key out of predefined keys #257

valeriosetti · 2025-12-17T09:41:19Z

Description

Prerequisite for Mbed-TLS/TF-PSA-Crypto#600

PR checklist

TF-PSA-Crypto PR provided [tf-psa-crypto] tests: pk: add a common function to create a PSA key out of predefined keys TF-PSA-Crypto#600
development PR not required because: no change there
3.6 PR not required because: no backport (at least not for now)

Signed-off-by: Valerio Setti <[email protected]>

…ader - declare all arrays and structures as static - add guards to the header file This allows multiple inclusions of the generated header file. Signed-off-by: Valerio Setti <[email protected]>

gilles-peskine-arm

A few remarks and suggestions

gilles-peskine-arm · 2025-12-22T20:51:06Z

scripts/generate_test_keys.py

+#ifndef TEST_TEST_KEYS_H
+#define TEST_TEST_KEYS_H
+
+#if !defined(MBEDTLS_VERSION_MAJOR) || MBEDTLS_VERSION_MAJOR >= 4


That works, but mbedtls/private/ecp.h never existed in Mbed TLS, so it's a strange way to put it. How about

Suggested change

#if !defined(MBEDTLS_VERSION_MAJOR) || MBEDTLS_VERSION_MAJOR >= 4

#if TF_PSA_CRYPTO_VERSION_MAJOR >= 1

Same in pk_helpers.c.

gilles-peskine-arm · 2025-12-22T20:53:03Z

scripts/generate_test_keys.py

+#if defined(__GNUC__) || defined(__clang__)
+#define UNUSED __attribute__((unused))
+#else
+#define UNUSED
+#endif


We already have MBEDTLS_MAYBE_UNUSED

gilles-peskine-arm · 2025-12-22T20:54:43Z

scripts/generate_test_keys.py

+#define UNUSED
+#endif
+
+static struct predefined_key_element predefined_keys[] UNUSED = {{


We normally put MBEDTLS_MAYBE_UNUSED at the very beginning of the definition, before static and the type. It doesn't matter with GCC-like compilers, but it might matter with other compilers.

gilles-peskine-arm · 2025-12-22T20:55:40Z

tests/include/test/pk_helpers.h

+    TEST_PK_COPY_PUBLIC_FROM_PSA,
+} pk_context_populate_method_t;
+
+int pk_helpers_get_predefined_key_data(int is_ec, int group_id_or_keybits,


Please document any function in a public header of the subproject. (It's a good idea to document all declarations in headers, really.) framework/tests/include is public for the framework.

gilles-peskine-arm · 2025-12-22T20:56:31Z

tests/include/test/pk_helpers.h

+    TEST_PK_COPY_PUBLIC_FROM_PSA,
+} pk_context_populate_method_t;
+
+int pk_helpers_get_predefined_key_data(int is_ec, int group_id_or_keybits,


All linkable functions should have one of the prefixes mbedtls_, psa_ or tf_psa_crypto_.

gilles-peskine-arm · 2025-12-23T16:54:56Z

tests/include/test/pk_helpers.h

@@ -0,0 +1,38 @@
+/*
+ * Helper functions for PK


Please document that this is only for TF-PSA-Crypto 1.0 and above.

gilles-peskine-arm · 2025-12-23T16:58:46Z

tests/src/pk_helpers.c

+            if (group_id_or_keybits == predefined_keys[i].group_id) {
+                predefined_key = &predefined_keys[i];
+            }
+        } else if (group_id_or_keybits == predefined_keys[i].keybits) {


Missing: check that it's an ECC key, and not an RSA key that happens to have the right size (or in the future ML-DSA, etc.).

On a related note, the check for the EC case above assumes that predefined_keys[i].group_id for a non-EC key cannot be equal to group_id_or_keybits for an EC key, which is currently true, but is fragile.

gilles-peskine-arm · 2025-12-23T17:11:05Z

tests/src/pk_helpers.c

+        return 0;
+    }
+
+    TEST_FAIL("Unsupported key");


If this failure point is reached, the only feedback you get from the test output is:

that this point was reached;

the message “Unsupported key”;

the test step, if set with mbedtls_test_set_step();

which test case is currently executing.

In particular, there's no indication of the key type and size that was sought.

Unfortunately, the argument to TEST_FAIL must be a string literal, because the memory containing the string has to still be alive when the test function returns. But there's a way to smuggle some extra information in two additional lines with a length limit. The functions to do that directly aren't exposed and aren't thread-safe. But you can at least easily smuggle an extra integer (or even two) with TEST_EQUAL. So you can write something like

if (ec) { int ec_group_found = 0; TEST_EQUAL(group_id_or_key_bits, found); } else { int rsa_bits_found = 0; TEST_EQUAL(group_id_or_key_bits, rsa_bits_found); }

gilles-peskine-arm · 2025-12-23T17:12:25Z

tests/src/pk_helpers.c

+    if (PSA_KEY_TYPE_IS_RSA(key_type)) {
+        ret = pk_helpers_get_predefined_key_data(0, key_bits, &priv_key, &priv_key_len,
+                                                 &pub_key, &pub_key_len);
+        TEST_EQUAL(ret, 0);


If pk_helpers_get_predefined_key_data returns nonzero, it already marks the test case as failed. So if (ret != 0) goto exit would have the same effect.

gilles-peskine-arm · 2025-12-23T17:14:09Z

tests/src/pk_helpers.c

+            TEST_FAIL("Unknown method");
+    }
+
+exit:; /* Needed to make compiler happy */


If pk_helpers_populate_context fails, you probably want to let the caller know so that it can goto exit.

valeriosetti mentioned this pull request Dec 17, 2025

[tf-psa-crypto] tests: pk: add a common function to create a PSA key out of predefined keys Mbed-TLS/TF-PSA-Crypto#600

Open

4 tasks

valeriosetti added needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Dec 17, 2025

valeriosetti added this to Roadmap pull requests (new board) Dec 17, 2025

github-project-automation bot moved this to In Development in Roadmap pull requests (new board) Dec 17, 2025

valeriosetti force-pushed the issue598-framework branch 4 times, most recently from 0408ec1 to 8532c59 Compare December 19, 2025 08:34

tests: src: move PK helpers out of tf-psa-crypto to this repo

86ac8fb

Signed-off-by: Valerio Setti <[email protected]>

valeriosetti force-pushed the issue598-framework branch 4 times, most recently from 29b9600 to 267f3d2 Compare December 19, 2025 14:09

scripts: generate_test_keys: allow multiple usage of the generated he…

b984d1c

…ader - declare all arrays and structures as static - add guards to the header file This allows multiple inclusions of the generated header file. Signed-off-by: Valerio Setti <[email protected]>

valeriosetti force-pushed the issue598-framework branch from 267f3d2 to b984d1c Compare December 19, 2025 17:27

valeriosetti added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Dec 22, 2025

bjwtaylor approved these changes Dec 22, 2025

View reviewed changes

gilles-peskine-arm self-requested a review December 22, 2025 20:50

gilles-peskine-arm requested changes Dec 23, 2025

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Dec 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[framework] tests: pk: add a common function to create a PSA key out of predefined keys #257

[framework] tests: pk: add a common function to create a PSA key out of predefined keys #257

valeriosetti commented Dec 17, 2025

Uh oh!

gilles-peskine-arm left a comment

Uh oh!

gilles-peskine-arm Dec 22, 2025

Uh oh!

gilles-peskine-arm Dec 22, 2025

Uh oh!

gilles-peskine-arm Dec 22, 2025

Uh oh!

gilles-peskine-arm Dec 22, 2025

Uh oh!

gilles-peskine-arm Dec 22, 2025

Uh oh!

gilles-peskine-arm Dec 23, 2025

Uh oh!

gilles-peskine-arm Dec 23, 2025

Uh oh!

gilles-peskine-arm Dec 23, 2025

Uh oh!

gilles-peskine-arm Dec 23, 2025

Uh oh!

gilles-peskine-arm Dec 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	#if !defined(MBEDTLS_VERSION_MAJOR) \|\| MBEDTLS_VERSION_MAJOR >= 4
	#if TF_PSA_CRYPTO_VERSION_MAJOR >= 1

[framework] tests: pk: add a common function to create a PSA key out of predefined keys #257

Are you sure you want to change the base?

[framework] tests: pk: add a common function to create a PSA key out of predefined keys #257

Conversation

valeriosetti commented Dec 17, 2025

Description

PR checklist

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants