Update the example to the new driver API #10

AndrzejKurek · 2019-08-07T08:19:02Z

The key slot numbers used in PSA currently map directly to the hardware slots.
Because of this, as slot 0 is reserved in PSA, slot 1 is used for private key instead.
Change direct low-level driver calls to calls to the PSA API.
Remove the public key exporting during private key generation test, since this functionality is not present anymore.
The storage is purged after the first key generation test for two reasons:
first - so that the same key slot can be reused and second - so that key
registration can be tested as a second way to have a key in a PSA slot.

This PR is based on:

Adjust the driver to the new PSA API mbed-os-atecc608a#7 - driver changes;
Secure element key creation foundation mbed-crypto#157 - key foundation;
Secure element key registration mbed-crypto#183 - key registration;
Add mock tests for hooks for secure element drivers mbed-crypto#174 - hooks.

Patater · 2019-09-05T10:31:59Z

Could we please rebase this to apply fixups and remove the merge commits?

k-stachowiak · 2019-09-05T10:49:13Z

I have tested this, with the mbed-os-atecc608a.lib containing:

https://github.com/AndrzejKurek/mbed-os-atecc608a/#7cc48d30f16474fde86b12d052a6384a14926205,

Which currently is the head of Andrzej's key-registration-api branch.

I confirm it works and passes all the tests on K64F, built with GCC_ARM.

AndrzejKurek · 2019-09-05T10:55:26Z

@Patater done.

k-stachowiak · 2019-09-05T11:26:05Z

I have repeated the test after the rebase and the tests still pass.

atecc608a/main.c

AndrzejKurek · 2019-09-06T11:27:43Z

Please note that before this PR is merged, the mbed-os-atecc608a.lib file has to point to the result of merging ARMmbed/mbed-os-atecc608a#7.

atecc608a/main.c

gilles-peskine-arm

The handle initialization is not right (I only checked one function but please check them all). Otherwise LGTM.

gilles-peskine-arm · 2019-09-06T12:57:45Z

atecc608a/main.c

 exit:
+    psa_close_key(*private_handle);
+    psa_close_key(verify_handle);


verify_handle may be uninitialized if there was an error before the call to psa_open_key that uses it. Initialize it to 0.

gilles-peskine-arm · 2019-09-06T13:00:01Z

atecc608a/main.c

 exit:
+    psa_close_key(*private_handle);


*private_handle is guaranteed to be initialized because nothing in this function can fail before the call to psa_open_key that uses it. But this is very fragile. Please initialize *private_handle to 0 before doing anything that can fail.

Patater · 2019-09-06T13:07:06Z

The commit "Revert incorredt differentiation between priv..." should be applied as a fixup to the commit that caused the trouble. Does the example work after this reversion? The key size change was made to get the example working, but maybe we fixed that in another way.

gilles-peskine-arm · 2019-09-06T13:09:24Z

Please avoid fixups for nontrivial issues found during a review.

I haven't checked how the code could work with incorrect sizes, but I suspect that it did because the sizes were never actually used in any calculation. For an ECC key, the key type determines everything and the bit size is redundant.

Patater · 2019-09-06T13:11:49Z

Please avoid adding dead end wrong solutions to the permanent history. Keeps mistakes made on your own branches within your own branches. This issue is dumb enough that we don't even need a code comment explaining our past failed attempt. The key size is always 256 bits.

The key slot numbers used in PSA currently map directly to the hardware slots. Because of this, as slot 0 is reserved in PSA, slot 1 is used for private key instead. Change direct low-level driver calls to calls to the PSA API. Remove the public key exporting during private key generation test, since this functionality is not present anymore. The storage is purged after the first key generation test for two reasons: first - so that the same key slot can be reused and second - so that key registration can be tested as a second way to have a key in a PSA slot. Update the pointer to the new driver library in mbed-os-atecc608a.lib

AndrzejKurek · 2019-09-06T14:31:12Z

Cleared the history and addressed all of the comments. @gilles-peskine-arm, @Patater - please re-review.

Patater · 2019-09-06T14:50:08Z

What testing was done with this?

Patater · 2019-09-06T14:50:52Z

Here's what I get

$ mbed compile -m K64F -t GCC_ARM --flash --sterm
...
Private key slot in use: 1, public: 9
Running tests...
test_hash_sha256 succesful!
test_sign_verify succesful!
test_export_import succesful!
test_psa_import_verify succesful!
assertion failed at ./main.c:392 (actual=109 expected=0)


Available commands:
 - info - print configuration information;
 - test - run all tests on the device;
 - exit - exit the interactive loop;
 - generate_private[=%d] - generate a private key in a given slot (0-15),
                           default slot - 0.
 - generate_public=%d_%d - generate a public key in a given slot

Patater · 2019-09-06T14:56:42Z

After running lock_data, I was able to proceed.

test
Running tests...
test_hash_sha256 succesful!
test_sign_verify succesful!
test_export_import succesful!
test_psa_import_verify succesful!
test_write_read_slot succesful!

Patater

LGTM

Patater · 2019-09-06T14:59:10Z

Tested on K64F+HAS_CRYPTOSHIELD

AndrzejKurek · 2019-09-09T05:57:19Z

This was also tested by me on K64F with ATECC608A connected - I've run the test suites in succession and the tests passed.

AndrzejKurek mentioned this pull request Aug 7, 2019

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

Merged

Patater changed the title ~~Update the example to the new driver API.~~ Update the example to the new driver API Aug 15, 2019

AndrzejKurek force-pushed the psa-key-registration branch from be221a1 to 8c0459e Compare September 5, 2019 10:54

k-stachowiak previously approved these changes Sep 5, 2019

View reviewed changes

Patater self-requested a review September 5, 2019 17:40

Patater suggested changes Sep 5, 2019

View reviewed changes

atecc608a/main.c Outdated Show resolved Hide resolved

atecc608a/main.c Outdated Show resolved Hide resolved

atecc608a/main.c Show resolved Hide resolved

atecc608a/main.c Show resolved Hide resolved

Patater reviewed Sep 5, 2019

View reviewed changes

atecc608a/main.c Show resolved Hide resolved

k-stachowiak dismissed their stale review via bb051fa September 6, 2019 10:55

k-stachowiak mentioned this pull request Sep 6, 2019

Differentiate private and public key sizes Patater/mbed-os-example-atecc608a#1

Closed

gilles-peskine-arm reviewed Sep 6, 2019

View reviewed changes

atecc608a/main.c Show resolved Hide resolved

atecc608a/main.c Outdated Show resolved Hide resolved

atecc608a/main.c Outdated Show resolved Hide resolved

atecc608a/main.c Show resolved Hide resolved

gilles-peskine-arm suggested changes Sep 6, 2019

View reviewed changes

Andrzej Kurek and others added 3 commits September 6, 2019 16:08

Enable SE support in Mbed Crypto

3443286

Rely on the public PSA API to reset security state

d5ab484

AndrzejKurek force-pushed the psa-key-registration branch from e84f4e6 to 0762c1e Compare September 6, 2019 14:30

AndrzejKurek requested review from Patater, k-stachowiak and gilles-peskine-arm September 6, 2019 14:32

Change the format of key slot printing

a2f9133

AndrzejKurek force-pushed the psa-key-registration branch from 0762c1e to a2f9133 Compare September 6, 2019 14:33

gilles-peskine-arm approved these changes Sep 6, 2019

View reviewed changes

Patater approved these changes Sep 6, 2019

View reviewed changes

Patater merged commit 4322b6e into master Sep 6, 2019

AndrzejKurek deleted the psa-key-registration branch September 9, 2019 05:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the example to the new driver API #10

Update the example to the new driver API #10

AndrzejKurek commented Aug 7, 2019 •

edited

Loading

Patater commented Sep 5, 2019

k-stachowiak commented Sep 5, 2019

AndrzejKurek commented Sep 5, 2019

k-stachowiak commented Sep 5, 2019

AndrzejKurek commented Sep 6, 2019 •

edited

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm Sep 6, 2019

gilles-peskine-arm Sep 6, 2019

Patater commented Sep 6, 2019

gilles-peskine-arm commented Sep 6, 2019

Patater commented Sep 6, 2019

AndrzejKurek commented Sep 6, 2019

Patater commented Sep 6, 2019

Patater commented Sep 6, 2019

Patater commented Sep 6, 2019

Patater left a comment

Patater commented Sep 6, 2019

AndrzejKurek commented Sep 9, 2019

Update the example to the new driver API #10

Update the example to the new driver API #10

Conversation

AndrzejKurek commented Aug 7, 2019 • edited Loading

Patater commented Sep 5, 2019

k-stachowiak commented Sep 5, 2019

AndrzejKurek commented Sep 5, 2019

k-stachowiak commented Sep 5, 2019

AndrzejKurek commented Sep 6, 2019 • edited Loading

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Sep 6, 2019

Choose a reason for hiding this comment

gilles-peskine-arm Sep 6, 2019

Choose a reason for hiding this comment

Patater commented Sep 6, 2019

gilles-peskine-arm commented Sep 6, 2019

Patater commented Sep 6, 2019

AndrzejKurek commented Sep 6, 2019

Patater commented Sep 6, 2019

Patater commented Sep 6, 2019

Patater commented Sep 6, 2019

Patater left a comment

Choose a reason for hiding this comment

Patater commented Sep 6, 2019

AndrzejKurek commented Sep 9, 2019

AndrzejKurek commented Aug 7, 2019 •

edited

Loading

AndrzejKurek commented Sep 6, 2019 •

edited

Loading