tests: crypto: Add support for testing wrapped keys in PSA crypto tests. #108

Aasimshaik11 · 2025-09-10T10:45:50Z

Added a new Kconfig option TEST_WRAPPED_KEYS under siwx917 SoC to enable testing with wrapped keys.
Updated AEAD, Cipher, Hash, Key Agreement, and Sign test cases to:
- Include sl_si91x_psa_wrap.h when CONFIG_TEST_WRAPPED_KEYS is set.
- Configure key lifetime with PSA_KEY_VOLATILE_PERSISTENT_WRAP_IMPORT for wrapped key tests.
- Print debug message when wrapper key mode is enabled.
Improved test clarity by updating assertion error messages and adding formatting for readability.

asmellby

Added a few comments on some fundamental problems. These apply to all the files, not just where the comments are added.

tests/crypto/psa_crypto/src/aead.c

tests/crypto/psa_crypto/src/cipher.c

tests/crypto/psa_crypto/src/aead.c

soc/silabs/silabs_siwx917/Kconfig

tests/crypto/psa_crypto/src/aead.c

jerome-pouiller

Please ensure your PR follow the contribution workflow: https://docs.zephyrproject.org/latest/contribute/guidelines.html#contribution-workflow

tests/crypto/psa_crypto/src/aead.c

tests/crypto/psa_crypto/src/cipher.c

soc/silabs/silabs_siwx917/Kconfig

jerome-pouiller · 2025-10-22T14:57:32Z

@Aasimshaik11 this PR still does not comply with the contribution workflow:

14. If reviewers do request changes to your patch, you can interactively rebase commit(s)
    to fix review issues. In your development repo:

        git rebase -i <offending-commit-id>^

    In the interactive rebase editor, replace pick with edit to select a specific commit
    (if there’s more than one in your pull request), or remove the line to delete a commit
    entirely. Then edit files to fix the issues in the review.

Compliance also complains about the author names in the commit. Your user.name in gitconfig is probably not correct (Aasimshaik11while it should be Aasim Shaik). You have also have to change he authors of the existing commit with something like git commit --amend --author="John Doe <[email protected]>" during your rebase.

tests/crypto/psa_crypto/src/aead.c

tests/crypto/psa_crypto/src/cipher.c

This commit consolidates and refactors wrapped key test functionality in the PSA crypto test suite, improving maintainability and readability. Key updates include: 1. Added support for testing wrapped keys in PSA crypto tests. 2. Introduced a new Kconfig option TEST_WRAPPED_KEYS to enable wrapped key testing. 3. Updated AEAD, Cipher, Hash, Key Agreement, and Sign test cases to: - Include sl_si91x_psa_wrap.h when CONFIG_TEST_WRAPPED_KEYS is set. - Configure key lifetime using PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION for wrapped key tests. - Import wrapped keys for AES, ChaChaPoly, and ECDSA when wrapper mode is active. - Print debug messages when wrapper key mode is enabled. 4. Moved wrapper key configuration to the 917-specific implementation and cleaned up redundant SoC-level handling. 5. Replaced preprocessor checks with IS_ENABLED() to improve readability and ensure both code paths are compiled and type-checked. 6. Cleaned up redundant preprocessor blocks, fixed indentation, and resolved build and style compliance issues per Zephyr guidelines. 7. Normalized comment alignment and indentation to meet clang-format and checkpatch requirements. These updates enhance test coverage, maintain consistency, and simplify wrapped key configuration across PSA crypto test modules. Signed-off-by: Aasim Shaik <[email protected]>

asmellby · 2025-10-16T09:42:03Z

soc/silabs/silabs_siwx917/Kconfig

 	default "sl_mbedtls_config_zephyr.h" \
 		if PSA_CRYPTO_DRIVER_SILABS_SIWX91X

+if SOC_SERIES_SIWG917


As mentioned before, there should be no change to this file.

The MACRO "CONFIG_TEST_WRAPPED_KEYS" or "TEST_WRAPPED_KEYS" has to be made available to the compiler from kconfig file. the inclusion of this in testcase.yaml just includes the test. So, I think either both can be there or it can be added only in the kconfig. Yaml file is optional. Is my understanding wrong?

This file defines the 917 SoC, it makes no sense for the TEST_WRAPPED_KEYS definition to escape the test. It's already set in the test as a CMake variable, what is the purpose of adding a new Kconfig for this (and not even replacing the existing variable, just adding an extra one)?

asmellby · 2025-10-28T08:12:18Z

tests/crypto/psa_crypto/src/cipher.c

 	psa_set_key_type(&attributes, PSA_KEY_TYPE_AES);
 	psa_set_key_bits(&attributes, 256);
-	if (IS_ENABLED(TEST_WRAPPED_KEYS)) {
+	if (IS_ENABLED(CONFIG_TEST_WRAPPED_KEYS)) {


Don't do this. The way it was is correct, the define is conditionally set by the testcase.

asmellby · 2025-10-28T08:12:44Z

tests/crypto/psa_crypto/src/cipher.c

+	if (IS_ENABLED(CONFIG_TEST_WRAPPED_KEYS)) {
 		psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION(
-							  PSA_KEY_PERSISTENCE_VOLATILE, 1));
+							  PSA_KEY_PERSISTENCE_VOLATILE, 0));


This is wrong. Location 1 is the default SE location, and it should stay that way. Using location 0 does not exercise opaque drivers (=wrapped keys).

@asmellby , We discussed during the meeting that location 1 does not generate wrap the keys. Location 0 generates the keys and gets it wrapped and stored in Host RAM(local storage). So, it should be 0. To make it consistent with PSA, we would have to modify the wrapper file and as we discussed, we dont want to have conflicting code in Wiseconnect and Zephyr. 1 is used only for import local key

Location 0 does not wrap the keys. By changing to location 0 you are no longer testing what you want to test.

asmellby · 2025-10-28T08:13:07Z

tests/crypto/psa_crypto/src/cipher.c

 	zassert_equal(decrypted_len, sizeof(decrypted), "Decrypted length mismatch");
 	zassert_mem_equal(decrypted, plaintext, sizeof(plaintext));
-
-	ciphertext_buffer_256[0] += 1;


Why are you removing tests?

@asmellby , This should not be needed, right? I think this was added by you, not sure what was the purpose. Can you let us know

This is a negative test demonstrating that decryption doesn't wrongly succeed on the wrong ciphertext. It is not a duplicate.

jerome-pouiller · 2025-10-28T08:14:26Z

soc/silabs/silabs_siwx917/Kconfig

 	default "sl_mbedtls_config_zephyr.h" \
 		if PSA_CRYPTO_DRIVER_SILABS_SIWX91X

+if SOC_SERIES_SIWG917


This statement is already under if SOC_FAMILY_SILABS_SIWX91X. Is it useful to add this condition?

jerome-pouiller · 2025-10-28T08:16:47Z

soc/silabs/silabs_siwx917/Kconfig


+if SOC_SERIES_SIWG917
+config TEST_WRAPPED_KEYS
+	bool "Enable Wrapper keys - specific to SiWG917"


This item will only appears when SILABS_SIWX91X is selected. You don't need to specify - specific to SiWG917.

I believe this symbols is misplaced. I understand right, it is an option of test/crypto/psa_crypto application.

jerome-pouiller · 2025-10-28T08:19:31Z

tests/crypto/psa_crypto/src/cipher.c

 #include <psa/crypto.h>
-
+#include <zephyr/sys/util.h>   /* for IS_ENABLED() */
 #include "test_vectors.h"


Keep code consistent: group <zephyr/...> headers.

BTW, I am surprised you need this include while the original code didn't need it.

jerome-pouiller · 2025-10-28T08:21:31Z

tests/crypto/psa_crypto/src/key_agreement.c

 	0xEF, 0x6E, 0xD4, 0xDA, 0x71, 0xBF, 0xED, 0x32, 0x0D, 0x76,
 };
-static const uint8_t expected_shared_secret[] = {
+static const uint8_t expected_shared_secret[] __unused = {


Usually, one of the benefits of if (IS_ENABLED(CONFIG_XXXX)) is you don't need these annotations. Can you confirm you need this change?

jerome-pouiller · 2025-10-28T08:21:47Z

tests/crypto/psa_crypto/src/key_agreement.c

 	0x12, 0xFB, 0x12, 0x17, 0x31, 0x32, 0x1D, 0x79, 0xAF, 0x0A, 0x9F,
 	0xAB, 0xAD, 0x34, 0x05, 0xA2, 0x07, 0x39, 0x9C, 0x5F, 0x15,
 };
-


Original code was correct.

jerome-pouiller · 2025-10-28T08:22:44Z

tests/crypto/psa_crypto/src/key_agreement.c

+	if (!IS_ENABLED(CONFIG_TEST_WRAPPED_KEYS)) {
+		/* Verify shared secret */
+		zassert_mem_equal(shared_secret_buf, expected_shared_secret,
+				sizeof(expected_shared_secret),


Indent with opening parenthesis.

jerome-pouiller · 2025-10-28T09:00:42Z

tests/crypto/psa_crypto/testcase.yaml

      - xg29_rb4412a
+      - siwx917_rb4338a
    extra_args:
      - TEST_WRAPPED_KEYS=1


I am surprize this statement works. I believe you mean:

extra_configs: CONFIG_TEST_WRAPPED_KEYS=y

Alternately, if you don't want to use CONFIG_ mechanism, you can use

extra_args: EXTRA_CFLAGS=-DTEST_WRAPPED_KEYS=1

I am surprize this statement works.

I missed the content of CMakelists.txt. If we consider the comment in CMakelist.txt, CONFIG_TEST_WRAPPED_KEYS is not an option.

silabs-AmerM · 2025-10-29T07:08:36Z

tests/crypto/psa_crypto/src/cipher.c

+	if (IS_ENABLED(CONFIG_TEST_WRAPPED_KEYS)) {
 		psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION(
-							  PSA_KEY_PERSISTENCE_VOLATILE, 1));
+							  PSA_KEY_PERSISTENCE_VOLATILE, 0));


@asmellby , We discussed during the meeting that location 1 does not generate wrap the keys. Location 0 generates the keys and gets it wrapped and stored in Host RAM(local storage). So, it should be 0. To make it consistent with PSA, we would have to modify the wrapper file and as we discussed, we dont want to have conflicting code in Wiseconnect and Zephyr. 1 is used only for import local key

silabs-AmerM · 2025-10-29T07:09:55Z

tests/crypto/psa_crypto/src/cipher.c

 	zassert_equal(decrypted_len, sizeof(decrypted), "Decrypted length mismatch");
 	zassert_mem_equal(decrypted, plaintext, sizeof(plaintext));
-
-	ciphertext_buffer_256[0] += 1;


@asmellby , This should not be needed, right? I think this was added by you, not sure what was the purpose. Can you let us know

silabs-AmerM · 2025-10-29T07:12:18Z

tests/crypto/psa_crypto/src/cipher.c

+							  PSA_KEY_PERSISTENCE_VOLATILE, 0));
 		zassert_equal(psa_generate_key(&attributes, &key_id), PSA_SUCCESS,
 			      "Failed to generate key");
 	} else {


@asmellby , as per you message on teams - you want us to remove the else part where import of local keys is being done, is my understanding right?

Removed conditional check for SOC_SERIES_SIWG917 around TEST_WRAPPED_KEYS configuration. Key updates include: 1. Edited Kconfig condition Signed-off-by: Aasim Shaik <[email protected]>

Aasimshaik11 requested a review from a team September 10, 2025 10:45

asmellby requested changes Sep 10, 2025

View reviewed changes

tests/crypto/psa_crypto/src/aead.c Outdated Show resolved Hide resolved

tests/crypto/psa_crypto/src/cipher.c Show resolved Hide resolved

tests/crypto/psa_crypto/src/aead.c Outdated Show resolved Hide resolved

Aasimshaik11 force-pushed the zephyr-silabs-psa-crypto branch 2 times, most recently from 1192c41 to dcad7b7 Compare September 11, 2025 09:36

jhedberg reviewed Sep 24, 2025

View reviewed changes

tests/crypto/psa_crypto/src/aead.c Outdated Show resolved Hide resolved

soc/silabs/silabs_siwx917/Kconfig Outdated Show resolved Hide resolved

asmellby requested changes Sep 30, 2025

View reviewed changes

tests/crypto/psa_crypto/src/aead.c Outdated Show resolved Hide resolved

tests/crypto/psa_crypto/src/aead.c Outdated Show resolved Hide resolved

Aasimshaik11 requested a review from a team as a code owner October 9, 2025 10:46

jerome-pouiller requested changes Oct 10, 2025

View reviewed changes

Aasimshaik11 force-pushed the zephyr-silabs-psa-crypto branch from 2fb07ad to 87ee066 Compare October 21, 2025 05:32

Aasimshaik11 force-pushed the zephyr-silabs-psa-crypto branch 12 times, most recently from 94b5f0f to 8c02821 Compare October 23, 2025 12:11

Aasimshaik11 requested review from asmellby, jerome-pouiller, jhedberg and silabs-AmerM October 23, 2025 12:33

jerome-pouiller reviewed Oct 23, 2025

View reviewed changes

tests/crypto/psa_crypto/src/aead.c Outdated Show resolved Hide resolved

jerome-pouiller reviewed Oct 23, 2025

View reviewed changes

tests/crypto/psa_crypto/src/cipher.c Outdated Show resolved Hide resolved

Aasimshaik11 force-pushed the zephyr-silabs-psa-crypto branch from b1b08a6 to ee71bcf Compare October 24, 2025 07:41

Aasimshaik11 requested a review from jerome-pouiller October 24, 2025 07:51

Aasimshaik11 force-pushed the zephyr-silabs-psa-crypto branch from 6cf5509 to dce4bb2 Compare October 27, 2025 10:03

asmellby requested changes Oct 28, 2025

View reviewed changes

jerome-pouiller reviewed Oct 28, 2025

View reviewed changes

silabs-AmerM reviewed Oct 29, 2025

View reviewed changes

tests: crypto: Refactor TEST_WRAPPED_KEYS configuration

601309e

Removed conditional check for SOC_SERIES_SIWG917 around TEST_WRAPPED_KEYS configuration. Key updates include: 1. Edited Kconfig condition Signed-off-by: Aasim Shaik <[email protected]>

Aasimshaik11 requested review from asmellby and jerome-pouiller October 29, 2025 11:03

tests: crypto: Add support for testing wrapped keys in PSA crypto tests. #108

Are you sure you want to change the base?

tests: crypto: Add support for testing wrapped keys in PSA crypto tests. #108

Conversation

Aasimshaik11 commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asmellby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerome-pouiller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerome-pouiller commented Oct 22, 2025

Uh oh!

Uh oh!

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!

asmellby Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

jerome-pouiller Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

Aasimshaik11 commented Sep 10, 2025 •

edited

Loading

asmellby Oct 28, 2025 •

edited

Loading

jerome-pouiller Oct 28, 2025 •

edited

Loading