PSA client-server: support in the unit test framework #9237

valeriosetti · 2024-06-07T14:16:31Z

Description

This is a first attempt to link already existing 'test_suite_xxx' source files to the 'psasim' simulation. So far only a limited subset of test_suite_xxx are tested because of the limited number of PSA APIs functions in psasim. This limit will be lifted once psasim will support all PSA APIs so that's why this PR is to be kept as work-in-progress as long as this support is improved.

Resolves #8968
Resolves #8966

PR checklist

changelog not required: it's only for PSASIM
3.6 backport not required
2.28 backport not required
tests provided

valeriosetti · 2024-06-19T14:49:39Z

Question for reviewers: perhaps 81dd9b0 can be set as a separate PR as we did for other commits which were not strictly related to psasim. Wdyt?

minosgalanakis · 2024-06-19T15:00:41Z

tests/psa-client-server/psasim/test/kill_server.sh

@@ -0,0 +1,8 @@
+#!/bin/bash


Missing copyright headers

minosgalanakis · 2024-06-19T15:00:55Z

tests/psa-client-server/psasim/test/start_server.sh

@@ -0,0 +1,17 @@
+#!/bin/bash


Missing copyright headers

minosgalanakis · 2024-06-20T12:59:57Z

tests/scripts/all.sh


    msg "build library for client"
    helper_crypto_client_build client

    msg "build psasim to test psa_client"
-    rm -f tests/psa-client-server/psasim/test/psa_client        # In case left behind
-    make -C tests/psa-client-server/psasim CFLAGS="$ASAN_CFLAGS" LDFLAGS="$ASAN_CFLAGS" test/psa_client
+    rm -f $PSASIM_PATH/test/psa_client        # In case left behind


This feels like it could be done in the cleanup_before_psasim_client()

No, if kept, this should be before make is called - specifically, because we were changing what was being built as psa_client, we want to make sure any existing such executable is removed, because make uses timestamps to determine what to build, and timestamps on other .c files might be older than when the last psa_client program was built.

If we're getting rid of all that (haven't reviewed in detail) we probably don't need this rm at all

cleanup_before_psasim_client is meant to be called after the server has been built and before building the client. However please note that test/psa_client is only built in test_psasim not with standard test_suite_xxx and since test_suite_xxx is the preferred way of testing for the future, I think we can keep this command only here.
Besides I also think that once we'll have a good coverage of test_suite_xxx in the future we can also remove test_psasim because that will be superseded by those tests.

in the future we can also remove test_psasim

I think it will be useful to keep this as a smoke test, as if something goes wrong (i.e. we break psasim somehow) it will be very hard to debug once the whole test machinery is involved

minosgalanakis · 2024-06-20T13:07:16Z

tests/scripts/all.sh


    msg "test psasim running psa_hash_compute"
-    tests/psa-client-server/psasim/test/run_test.sh
+    $PSASIM_PATH/test/run_test.sh


    # Next APIs under test: psa_hash_*(). Use our copy of the PSA hash example.


This whole block could be a helper function

build_psasim() { msg "build psasim to test $1" rm tests/psa-client-server/psasim/test/psa_client rm $PSASIM_PATH/test/psa_client make -C $PSASIM_PATH CFLAGS="$ASAN_CFLAGS" LDFLAGS="$ASAN_CFLAGS" MAIN="src/$1.c" test/psa_client $PSASIM_PATH/test/run_test.sh }

This could scale easier and we could even use an iterator to go through all the tests

Yeah, I like this

As for the comment above, please note that run_test.sh is only useful for test_psasim component not for test_suite_with_psasim and all other upcoming tests relying on test_suite_xxx. Once all PSA functions will be supported, make PSASIM=1 test will be the way to test with psasim (as it is done in test_suite_with_psasim). No need to manually iterate of test_suite_xxx; this is done "for free" by the current test framework. All test_suite_xxx will be linked against our client side implementation of psasim and they will connect to the server that has previously been launched in background.

What can be moved as helper is starting/stopping the PSA server in test_suite_with_psasim:

( cd tests msg "start server" psa-client-server/psasim/test/start_server.sh )

and

( cd tests msg "terminate server and cleanup" psa-client-server/psasim//test/kill_server.sh )

tom-cosgrove-arm · 2024-06-26T16:08:55Z

scripts/common.make

@@ -132,4 +153,13 @@ MBEDTLS_TLS_TEST_OBJS = $(patsubst %.c,%.o,$(wildcard \
    ${MBEDTLS_TEST_PATH}/src/test_helpers/*.c \
  ))

+# THIS IS A TEMPORARY FIX!!!!!!!
+# IT MUST BE REMOVED AS SOON AS LIBPSACLIENT WILL SUPPORT ALL PSA
+# FUNCTIONS.


So what is preventing this fix being removed now?

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Now that we have PSASIM we can really test CRYPTO_CLIENT functionality and those functions are not needed anymore. Moreover new test suites that are going to rely on CRYPTO_CLIENT && !CRYPTO_C would be tested from test_default_psa_crypto_client_without_crypto_provider() leading to failures due to stub functions being empty. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

- do not try to close a connection that was never started - fix data chunks length for psa_write (prevent memcpy-ing to large blocks of data) Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

This commit includes the following changes: - on the psasim side it clears a certain operation slot on "finish()" and "abort()" operations - in all.sh it builds all the test suites, but it just executes the constant_time_hmac one because it's one of those that quickly show the problem Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

tom-cosgrove-arm · 2024-06-29T15:33:01Z

I've updated psasim: minor fixes to the core so that it includes the necessary changes to psa_sim_generate.pl to regenerate these changes.

I've also removed the changes to psasim: TEMPORARY COMMIT that added psasim_serialize_free_hash_operation() and find_hash_slot_by_operation() - because this is now done a different way.

Instead, on server serialisation, when serialising the operation to send back to the client, we have an extra parameter completed. This is set by abort and finish operations, and not set on others. With this flag, the operation is erased on the server, and we return a handle number of 0 to the client, so that if the client re-uses their operation, we will allocate a new handle for them.

There's also a bit of extra testing code added to aut_psa_hash.c to test this

tom-cosgrove-arm · 2024-06-29T15:34:13Z

Note that test_suite_with_psasim now takes a very long time to run - the OG test_suite_constant_time_hmac takes a long time; running it over the simulator is even slower :(

valeriosetti added needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Jun 7, 2024

valeriosetti requested review from minosgalanakis and tom-cosgrove-arm June 7, 2024 14:16

valeriosetti self-assigned this Jun 7, 2024

valeriosetti added this to To Do in Roadmap Board for Mbed TLS via automation Jun 7, 2024

valeriosetti force-pushed the issue8968 branch 2 times, most recently from cdde639 to 21150f3 Compare June 7, 2024 15:30

minosgalanakis added 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 and removed needs-ci Needs to pass CI tests labels Jun 10, 2024

valeriosetti force-pushed the issue8968 branch 3 times, most recently from 5c8728a to d530744 Compare June 18, 2024 08:28

This was referenced Jun 18, 2024

psa_sim: improve log prints #9278

Merged

psa_sim: make server ping time much faster #9279

Merged

psasim-server: add function to reset operations slots #9280

Merged

valeriosetti force-pushed the issue8968 branch 2 times, most recently from 9f1136c to 2a52677 Compare June 19, 2024 14:38

minosgalanakis reviewed Jun 19, 2024

View reviewed changes

valeriosetti force-pushed the issue8968 branch 3 times, most recently from 2d401be to 587977b Compare June 20, 2024 07:48

valeriosetti requested a review from minosgalanakis June 20, 2024 10:01

valeriosetti moved this from To Do to In Review in Roadmap Board for Mbed TLS Jun 20, 2024

minosgalanakis reviewed Jun 20, 2024

View reviewed changes

valeriosetti added the DO-NOT-MERGE label Jun 21, 2024

valeriosetti force-pushed the issue8968 branch from 44399f1 to ada6255 Compare June 26, 2024 15:56

tom-cosgrove-arm reviewed Jun 26, 2024

View reviewed changes

valeriosetti added 5 commits June 27, 2024 08:06

test_suite_psa_crypto: split AEAD tests to an indipendent file

4b31c3a

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

makefile: allow to build and link test suites against psasim

1c3e433

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

all.sh: add new component to run test suites with psasim

a679cd8

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

psasim: update bash scripts

fd9f14f

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti force-pushed the issue8968 branch from ada6255 to f3b5c7d Compare June 27, 2024 06:07

tests: fix guards in test suites to allow testing with PSASIM

646e097

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti force-pushed the issue8968 branch 2 times, most recently from 8f333c2 to 446acd0 Compare June 28, 2024 11:02

valeriosetti and others added 4 commits June 29, 2024 16:19

psasim: minor fixes to the core

f70f73e

- do not try to close a connection that was never started - fix data chunks length for psa_write (prevent memcpy-ing to large blocks of data) Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

psasim: add a bit of white-box testing to hash operations

1bf7c3c

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

psasim: invalidate operations on abort+finish

bb27fb8

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

tom-cosgrove-arm force-pushed the issue8968 branch from 446acd0 to bb27fb8 Compare June 29, 2024 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSA client-server: support in the unit test framework #9237

PSA client-server: support in the unit test framework #9237

valeriosetti commented Jun 7, 2024 •

edited

Loading

valeriosetti commented Jun 19, 2024

minosgalanakis Jun 19, 2024

minosgalanakis Jun 19, 2024

minosgalanakis Jun 20, 2024

tom-cosgrove-arm Jun 20, 2024

valeriosetti Jun 21, 2024

tom-cosgrove-arm Jun 21, 2024

minosgalanakis Jun 20, 2024

tom-cosgrove-arm Jun 20, 2024

valeriosetti Jun 21, 2024 •

edited

Loading

tom-cosgrove-arm Jun 26, 2024

tom-cosgrove-arm commented Jun 29, 2024 •

edited

Loading

tom-cosgrove-arm commented Jun 29, 2024

PSA client-server: support in the unit test framework #9237

Are you sure you want to change the base?

PSA client-server: support in the unit test framework #9237

Conversation

valeriosetti commented Jun 7, 2024 • edited Loading

Description

PR checklist

valeriosetti commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriosetti Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Jun 29, 2024 • edited Loading

tom-cosgrove-arm commented Jun 29, 2024

valeriosetti commented Jun 7, 2024 •

edited

Loading

valeriosetti Jun 21, 2024 •

edited

Loading

tom-cosgrove-arm commented Jun 29, 2024 •

edited

Loading