Add mock tests for hooks for secure element drivers #174

dgreen-arm · 2019-07-15T14:21:04Z

This PR add hooks for generating keys, signing and verification using secure element driver code.

AndrzejKurek · 2019-07-22T07:55:28Z

include/psa/crypto_se_driver.h

-                                                  uint8_t *p_pubkey_out,
-                                                  size_t pubkey_out_size,
-                                                  size_t *p_pubkey_length);
+                                                  size_t bits);


Why did you drop support for pubkey exporting during key generation?

I've fixed this based on the discussion we had.

AndrzejKurek · 2019-07-22T07:58:54Z

library/psa_crypto.c

+        status = drv->asymmetric->p_sign(
+            slot->data.se.slot_number, alg, hash, hash_length, signature,
+            signature_size, signature_length );
+        goto exit;


Do we need this goto?

No, it goes to exit at this point anyway, I'll remove it.

AndrzejKurek · 2019-07-22T07:58:58Z

library/psa_crypto.c

+            drv->asymmetric->p_sign == NULL )
+        {
+            status = PSA_ERROR_NOT_SUPPORTED;
+            goto exit;


Do we need this goto?

I believe we need this one, as there isn't anything more we can do at this point.

AndrzejKurek · 2019-07-22T08:04:10Z

tests/suites/test_suite_psa_crypto_se_driver_hal_mocks.function

-    mock_import_data.usage = usage;
-    mock_import_data.data_length = data_length;
+
+    if( mock_import_data.return_value == PSA_SUCCESS )


This should be set regardlesss of the result.

Do we want to set all these values even on failure? The test seems to indicate that type at least should remain unset if mock_import_return_value isn't PSA_SUCCESS

The mock structures only indicate with what arguments was the function called, regardless of any error.

AndrzejKurek

I would only suggest always filling the mock in the tests, since it should only indicate with what arguments was the function called. If necessary, the tests should also be adjusted.

Patater · 2019-07-31T10:04:09Z

@dgreen-arm I've rebased this branch on the latest API branch. Available at https://github.com/Patater/mbed-crypto/tree/psa-se-driver-hooks. Would recommend updating this PR with that branch, changing the target branch of this PR, and continuing the review from there.

dgreen-arm · 2019-08-02T08:54:37Z

There are currently two issues to note with this PR. Firstly, the tests aren't currently being run as MBEDTLS_PSA_CRYPTO_SE_C isn't enabled by default. Would adding a test case in all.sh be the best approach for this?
Second, the mock import failure test currently doesn't work with how the mocks are constructed. Either the test or the mock needs updating.

gilles-peskine-arm · 2019-08-02T09:05:51Z

There's an all.sh job with SE support enabled since #157 which is merged so rebasing on top of the target branch would give you that.

dgreen-arm · 2019-08-02T09:07:45Z

Oh, I hadn't realized that. This is based on #157, so that issue is already solved.

gilles-peskine-arm · 2019-08-08T17:18:04Z

I broke off the hooks for generate/sign/verify in #211, without the mock tests, to get a basis to make the whole stack work on real hardware. This PR should be rebased on top of a merge of #211, to add the tests.

AndrzejKurek · 2019-08-19T14:22:07Z

library/psa_crypto.c

@@ -1835,7 +1835,7 @@ psa_status_t psa_import_key( const psa_key_attributes_t *attributes,
    if( driver != NULL )
    {
        const psa_drv_se_t *drv = psa_get_se_driver_methods( driver );
-        size_t bits;
+        size_t bits = PSA_MAX_KEY_BITS + 1;


Why is this change needed?

Please leave a comment in the code explaining the reason, to improve the code's readability. Others will have the same question.

I see this change was added in commit "Parametrize key bits in import mock test", which doesn't seem right if all one intends to do is updating a test as the title would imply. If the core has to be updated together with the test change, revise the commit message. If the core has a bug, explain it in the commit message and make a change to fix the bug separate from the mock test updating.

I've split this out into a separate commit and added an explanation.

Please add a comment in the code about why you chose the value you did.

Added a code comment.

AndrzejKurek

Just for the sake of another reviewer - I would suggest renaming this PR and describing it accordingly.

tests/suites/test_suite_psa_crypto_se_driver_hal_mocks.function

Patater · 2019-08-20T07:55:16Z

library/psa_crypto.c

@@ -1835,7 +1835,7 @@ psa_status_t psa_import_key( const psa_key_attributes_t *attributes,
    if( driver != NULL )
    {
        const psa_drv_se_t *drv = psa_get_se_driver_methods( driver );
-        size_t bits;
+        size_t bits = PSA_MAX_KEY_BITS + 1;


I see this change was added in commit "Parametrize key bits in import mock test", which doesn't seem right if all one intends to do is updating a test as the title would imply. If the core has to be updated together with the test change, revise the commit message. If the core has a bug, explain it in the commit message and make a change to fix the bug separate from the mock test updating.

dgreen-arm · 2019-08-20T12:32:31Z

The ABI check is failing as the fix hasn't been merged into the psa-api-1.0-beta branch yet, however I've run it locally and there are no changes, as expected.

Mock key importing and exporting

In psa_import_key, the key bits value was uninitialized before calling the secure element driver import function. There is a potential issue if the driver returns PSA_SUCCESS without setting the key bits. This shouldn't happen, but shouldn't be discounted either, so we initialize the key bits to an invalid issue.

dgreen-arm · 2019-08-21T16:03:28Z

I've reworked this to save and check all the attributes in the import and generate mock tests. I've also rebased it onto development to (hopefully) get the PR-head tests passing.

Previous version available here: https://github.com/dgreen-arm/mbed-crypto/tree/psa-se-driver-hooks-3
Updated attributes here, but before rebase here: https://github.com/dgreen-arm/mbed-crypto/tree/psa-se-driver-hooks-4

Patater

LGTM

Review comments addressed

dgreen-arm requested a review from AndrzejKurek July 15, 2019 14:21

AndrzejKurek reviewed Jul 22, 2019

View reviewed changes

gilles-peskine-arm added the feature branch This issue or PR is for a short-term feature branch which doesn't have its own label. label Jul 22, 2019

dgreen-arm force-pushed the psa-se-driver-hooks branch from bde7708 to 284eca2 Compare July 23, 2019 14:33

AndrzejKurek suggested changes Jul 24, 2019

View reviewed changes

gilles-peskine-arm mentioned this pull request Jul 25, 2019

Secure element key creation foundation #157

Merged

dgreen-arm changed the base branch from dev/shared/secure-element to psa-api-1.0-beta August 2, 2019 08:47

dgreen-arm force-pushed the psa-se-driver-hooks branch from 284eca2 to 37a41ec Compare August 2, 2019 08:48

This was referenced Aug 7, 2019

Update the example to the new driver API ARMmbed/mbed-os-example-atecc608a#10

Merged

Adjust the driver to the new PSA API ARMmbed/mbed-os-atecc608a#7

Merged

dgreen-arm force-pushed the psa-se-driver-hooks branch from 37a41ec to 917ab8a Compare August 19, 2019 08:04

AndrzejKurek reviewed Aug 19, 2019

View reviewed changes

AndrzejKurek previously requested changes Aug 19, 2019

View reviewed changes

Patater suggested changes Aug 20, 2019

View reviewed changes

dgreen-arm force-pushed the psa-se-driver-hooks branch from 917ab8a to af793a8 Compare August 20, 2019 12:11

dgreen-arm force-pushed the psa-se-driver-hooks branch from af793a8 to dd55ea5 Compare August 20, 2019 13:57

dgreen-arm changed the base branch from psa-api-1.0-beta to development August 21, 2019 10:52

dgreen-arm changed the title ~~Add hooks for secure element drivers~~ Add mock tests for hooks for secure element drivers Aug 21, 2019

AndrzejKurek and others added 9 commits August 21, 2019 16:56

crypto_se_driver: add mock tests

c9ad591

Mock key importing and exporting

crypto_se_driver: add key generation mock and test

9fd6b0c

crypto_se_driver: add an error injection mechanism to the mocks

903b5da

crypto_se_driver: add public key exporting test

136901c

crypto_se_driver: add signing mock test

b7656a8

crypto_se_driver: add verification mock test

f740b0a

Clean up core storage between tests

4abb40c

Parametrize key bits in import mock test

74c932e

dgreen-arm force-pushed the psa-se-driver-hooks branch from dd55ea5 to 0892d0f Compare August 21, 2019 16:03

dgreen-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Aug 21, 2019

Patater approved these changes Aug 21, 2019

View reviewed changes

Patater requested a review from AndrzejKurek August 21, 2019 19:08

k-stachowiak self-requested a review August 22, 2019 11:40

k-stachowiak approved these changes Aug 22, 2019

View reviewed changes

Patater removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Aug 22, 2019

Patater merged commit de4453d into ARMmbed:development Aug 22, 2019

Add mock tests for hooks for secure element drivers #174

Add mock tests for hooks for secure element drivers #174

Uh oh!

Conversation

dgreen-arm commented Jul 15, 2019

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!

AndrzejKurek left a comment

Choose a reason for hiding this comment

Uh oh!

Patater commented Jul 31, 2019

Uh oh!

dgreen-arm commented Aug 2, 2019

Uh oh!

gilles-peskine-arm commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgreen-arm commented Aug 2, 2019

Uh oh!

gilles-peskine-arm commented Aug 8, 2019

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!

AndrzejKurek 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!

Choose a reason for hiding this comment

Uh oh!

dgreen-arm commented Aug 20, 2019

Uh oh!

dgreen-arm commented Aug 21, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilles-peskine-arm commented Aug 2, 2019 •

edited

Loading