Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+26 −50
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dce4bb2
tests: crypto: Add and refactor wrapped key support in PSA crypto tests
Aasimshaik11 Aug 12, 2025
601309e
tests: crypto: Refactor TEST_WRAPPED_KEYS configuration
Aasimshaik11 Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions soc/silabs/silabs_siwx917/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,8 @@ config MBEDTLS_USER_CONFIG_FILE
default "sl_mbedtls_config_zephyr.h" \
if PSA_CRYPTO_DRIVER_SILABS_SIWX91X

config TEST_WRAPPED_KEYS
bool "Enable Wrapper keys - specific to SiWG917"
Copy link
Contributor

@jerome-pouiller jerome-pouiller Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


endif
endif
35 changes: 7 additions & 28 deletions tests/crypto/psa_crypto/src/cipher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@

#include <zephyr/ztest.h>
#include <psa/crypto.h>

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

Check notice on line 10 in tests/crypto/psa_crypto/src/cipher.c

View workflow job for this annotation

GitHub Actions / compliance

You may want to run clang-format on this change 

tests/crypto/psa_crypto/src/cipher.c:10 
-#include <zephyr/sys/util.h>   /* for IS_ENABLED() */
+#include <zephyr/sys/util.h> /* for IS_ENABLED() */
Copy link
Contributor

@jerome-pouiller jerome-pouiller Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jerome-pouiller jerome-pouiller Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


uint8_t key_256[32] = {
0x31, 0x4b, 0x66, 0x77, 0x19, 0x39, 0x73, 0x17, 0x7d, 0xaf, 0x98,
Expand Down Expand Up @@ -40,9 +40,9 @@
psa_set_key_algorithm(&attributes, alg);
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)) {
Copy link
Contributor

@asmellby asmellby Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION(
PSA_KEY_PERSISTENCE_VOLATILE, 1));
PSA_KEY_PERSISTENCE_VOLATILE, 0));
Copy link
Contributor

@asmellby asmellby Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link

@silabs-AmerM silabs-AmerM Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@asmellby asmellby Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

zassert_equal(psa_generate_key(&attributes, &key_id), PSA_SUCCESS,
"Failed to generate key");
} else {
Expand Down Expand Up @@ -96,9 +96,9 @@
psa_set_key_algorithm(&attributes, alg);
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)) {
psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION(
PSA_KEY_PERSISTENCE_VOLATILE, 1));
PSA_KEY_PERSISTENCE_VOLATILE, 0));
zassert_equal(psa_generate_key(&attributes, &key_id), PSA_SUCCESS,
"Failed to generate key");
} else {
Copy link

@silabs-AmerM silabs-AmerM Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Expand All @@ -121,17 +121,6 @@

zassert_equal(decrypted_len, sizeof(decrypted), "Decrypted length mismatch");
zassert_mem_equal(decrypted, plaintext, sizeof(plaintext));

ciphertext_buffer_256[0] += 1;
Copy link
Contributor

@asmellby asmellby Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing tests?

Copy link

@silabs-AmerM silabs-AmerM Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@asmellby asmellby Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

zassert_equal(psa_cipher_decrypt(key_id, alg, ciphertext_buffer_256, ciphertext_len,
decrypted, sizeof(decrypted), &decrypted_len),
PSA_SUCCESS,
"Failed to perform one-shot decrypt operation with modified ciphertext");

zassert_equal(decrypted_len, sizeof(decrypted), "Decrypted length mismatch");
zassert(memcmp(decrypted, plaintext, sizeof(plaintext)) != 0,
"Decrypted modified data identical to original plaintext");

psa_destroy_key(key_id);
}

Expand All @@ -145,9 +134,9 @@
psa_set_key_algorithm(&attributes, alg);
psa_set_key_type(&attributes, PSA_KEY_TYPE_AES);
psa_set_key_bits(&attributes, 128);
if (IS_ENABLED(TEST_WRAPPED_KEYS)) {
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));

zassert_equal(psa_generate_key(&attributes, &key_id), PSA_SUCCESS,
"Failed to generate key");
Expand All @@ -170,16 +159,6 @@
zassert_equal(decrypted_len, sizeof(decrypted), "Decrypted length mismatch");
zassert_mem_equal(decrypted, plaintext, sizeof(plaintext));

ciphertext[0] += 1;
zassert_equal(psa_cipher_decrypt(key_id, alg, ciphertext, ciphertext_len, decrypted,
sizeof(decrypted), &decrypted_len),
PSA_SUCCESS,
"Failed to perform one-shot decrypt operation with modified ciphertext");

zassert_equal(decrypted_len, sizeof(decrypted), "Decrypted length mismatch");
zassert(memcmp(decrypted, plaintext, sizeof(plaintext)) != 0,
"Decrypted modified data identical to original plaintext");

psa_destroy_key(key_id);
}

Expand Down
22 changes: 12 additions & 10 deletions tests/crypto/psa_crypto/src/key_agreement.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

#include <zephyr/ztest.h>
#include <psa/crypto.h>
#include <zephyr/sys/util.h> /* for IS_ENABLED() */

Check notice on line 10 in tests/crypto/psa_crypto/src/key_agreement.c

View workflow job for this annotation

GitHub Actions / compliance

You may want to run clang-format on this change 

tests/crypto/psa_crypto/src/key_agreement.c:10 
-#include <zephyr/sys/util.h>   /* for IS_ENABLED() */
+#include <zephyr/sys/util.h> /* for IS_ENABLED() */
static const uint8_t client_private_key[] = {
0xB0, 0x76, 0x51, 0xEA, 0x20, 0xF0, 0x28, 0xA8, 0x16, 0xEE, 0x01,
0xB0, 0xD1, 0x06, 0x2A, 0x7C, 0x81, 0x58, 0xE8, 0x84, 0xE9, 0xBC,
Expand All @@ -27,12 +28,11 @@
0x8D, 0x49, 0x85, 0x8C, 0x7A, 0x9F, 0xC1, 0x46, 0xDA, 0xCC, 0x96,
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 = {
Copy link
Contributor

@jerome-pouiller jerome-pouiller Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

0xF2, 0xE6, 0x0E, 0x1C, 0xB7, 0x64, 0xBC, 0x48, 0xF2, 0x9D, 0xBB,
0x12, 0xFB, 0x12, 0x17, 0x31, 0x32, 0x1D, 0x79, 0xAF, 0x0A, 0x9F,
0xAB, 0xAD, 0x34, 0x05, 0xA2, 0x07, 0x39, 0x9C, 0x5F, 0x15,
};

Copy link
Contributor

@jerome-pouiller jerome-pouiller Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original code was correct.

ZTEST(psa_crypto_test, test_key_agreement_ecdh_25519)
{
uint8_t shared_secret_buf[32];
Expand All @@ -44,9 +44,9 @@
psa_set_key_type(&attributes, PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_MONTGOMERY));
psa_set_key_usage_flags(&attributes, PSA_KEY_USAGE_DERIVE);
psa_set_key_algorithm(&attributes, PSA_ALG_ECDH);
if (IS_ENABLED(TEST_WRAPPED_KEYS)) {
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));
}
zassert_equal(psa_import_key(&attributes, client_private_key, sizeof(client_private_key),
&key_id),
Expand All @@ -64,9 +64,9 @@
psa_set_key_type(&attributes, PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_MONTGOMERY));
psa_set_key_usage_flags(&attributes, PSA_KEY_USAGE_DERIVE);
psa_set_key_algorithm(&attributes, PSA_ALG_ECDH);
if (IS_ENABLED(TEST_WRAPPED_KEYS)) {
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));
}
zassert_equal(psa_import_key(&attributes, server_private_key, sizeof(server_private_key),
&key_id),
Expand All @@ -78,8 +78,10 @@
sizeof(shared_secret_buf), &shared_secret_len),
PSA_SUCCESS, "Failed to perform key agreement with client");
zassert_equal(psa_destroy_key(key_id), PSA_SUCCESS, "Failed to destroy server key");

/* Verify shared secret */
zassert_mem_equal(shared_secret_buf, expected_shared_secret, sizeof(expected_shared_secret),
"Key agreement did not resolve the correct shared secret");
if (!IS_ENABLED(CONFIG_TEST_WRAPPED_KEYS)) {
/* Verify shared secret */
zassert_mem_equal(shared_secret_buf, expected_shared_secret,
sizeof(expected_shared_secret),
Copy link
Contributor

@jerome-pouiller jerome-pouiller Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent with opening parenthesis.

"Key agreement did not resolve the correct shared secret");
}

Check notice on line 86 in tests/crypto/psa_crypto/src/key_agreement.c

View workflow job for this annotation

GitHub Actions / compliance

You may want to run clang-format on this change 

tests/crypto/psa_crypto/src/key_agreement.c:86 
-				sizeof(expected_shared_secret),
-				"Key agreement did not resolve the correct shared secret");
+				  sizeof(expected_shared_secret),
+				  "Key agreement did not resolve the correct shared secret");
}
12 changes: 0 additions & 12 deletions tests/crypto/psa_crypto/src/sign.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ ZTEST(psa_crypto_test, test_sign_ecdsa_secp256r1)
psa_set_key_usage_flags(&attributes,
PSA_KEY_USAGE_SIGN_MESSAGE | PSA_KEY_USAGE_VERIFY_MESSAGE);
psa_set_key_algorithm(&attributes, PSA_ALG_ECDSA(PSA_ALG_ANY_HASH));
if (IS_ENABLED(TEST_WRAPPED_KEYS)) {
psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION(
PSA_KEY_PERSISTENCE_VOLATILE, 1));
}

zassert_equal(psa_generate_key(&attributes, &key_id), PSA_SUCCESS,
"Failed to generate private key");

Expand All @@ -53,11 +48,4 @@ ZTEST(psa_crypto_test, test_sign_ecdsa_secp256r1)
zassert_equal(psa_verify_message(key_id, PSA_ALG_ECDSA(PSA_ALG_SHA_256), plaintext,
MESSAGE_SIZE, signature, signature_len),
PSA_SUCCESS, "Failed to verify signature");

signature[0] += 1;
zassert_not_equal(psa_verify_message(key_id, PSA_ALG_ECDSA(PSA_ALG_SHA_256), plaintext,
MESSAGE_SIZE, signature, signature_len),
PSA_SUCCESS, "Signature incorrectly successfully verified");

zassert_equal(psa_destroy_key(key_id), PSA_SUCCESS, "Failed to destroy key");
}
4 changes: 4 additions & 0 deletions tests/crypto/psa_crypto/testcase.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
# SPDX-License-Identifier: Apache-2.0
# SPDX-FileCopyrightText: 2025 Silicon Laboratories Inc.

common:
tags:
- crypto
Expand Down Expand Up @@ -31,6 +34,7 @@ tests:
- xg24_dk2601b
- xg24_rb4187c
- xg29_rb4412a
- siwx917_rb4338a
extra_args:
- TEST_WRAPPED_KEYS=1
Copy link
Contributor

@jerome-pouiller jerome-pouiller Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jerome-pouiller jerome-pouiller Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

extra_configs:
Expand Down
Loading