Add hooks for generate and sign in a secure element #211

gilles-peskine-arm · 2019-08-07T19:09:39Z

Update the import and generate methods in the secure element driver interface to use `psa_key_attributes_t.

Implement hooks to call a secure element for key generation and asymmetric signature and verification.

k-stachowiak · 2019-08-08T13:00:19Z

include/psa/crypto_se_driver.h

+ *                              psa_get_key_algorithm() to access this
+ *                              information.
+ * \param[in] data              Buffer containing the key data.
+ * \param[in] data_length       Size of the `data` buffer in bytes.


Nitpick: data is referred to with a /p prefix earlier in the comment.

Since I was rebasing anyway, I amended the commit that rewrote that line.

AndrzejKurek · 2019-08-09T07:21:38Z

include/psa/crypto_se_driver.h

+ * to the \p pubkey buffer.
+ * This is optional, intended for secure elements that output the
+ * public key at generation time and that cannot export the public key
+ * later. Drivers that do not  need this feature should leave


Double whitespace after not

Since I was rebasing anyway, I amended the commit that introduced that line.

AndrzejKurek · 2019-08-09T07:25:22Z

include/psa/crypto_se_driver.h

+ * later. Drivers that do not  need this feature should leave
+ * \p *pubkey_length set to 0 and should
+ * implement the psa_drv_key_management_t::p_export_public function.
+ * Some implementations do not support this feature, in which case


NULL can also be passed in a case when the user is not interested in receiving the public key, right? This information is missing.

No, there's no such thing as “the user is not interested in receiving the public key”. psa_generate_key never outputs the public key. If the driver outputs the public key, the core stores it in case the user ever calls psa_export_public_key later.

AndrzejKurek

I would suggest reworking it a bit to use key exporting instead of/in addition to passing raw key material to both of the handles.
I would see a following test here:

Generate key / Import Key (sw / driver to choose);
Export public key (sw/driver);
Import key (sw/driver);
Sign (sw/driver);
Verify (sw driver);

This way we could run the following cases:

HW stack only (importing into a different slot);
HW generate, export public, and sign, SW rest;
SW generate, export public and sign, HW rest;

Maybe it's too much to handle with one test, but I'd suggest adding public key exporting to the pipeline in any test that you will consider the most readable.

gilles-peskine-arm · 2019-08-09T14:03:12Z

There's a trivial conflict with PRs merged since I made this one, and a non-trivial conflict with #183. I'm going to rebase on top of #183.

The methods to import and generate a key in a secure element drivers were written for an earlier version of the application-side interface. Now that there is a psa_key_attributes_t structure that combines all key metadata including its lifetime (location), type, size, policy and extra type-specific data (domain parameters), pass that to drivers instead of separate arguments for each piece of metadata. This makes the interface less cluttered. Update parameter names and descriptions to follow general conventions. Document the public-key output on key generation more precisely. Explain that it is optional in a driver, and when a driver would implement it. Declare that it is optional in the core, too (which means that a crypto core might not support drivers for secure elements that do need this feature). Update the implementation and the tests accordingly.

Factor common code of ram_import and ram_fake_generate into a common auxiliary function. Reject key types that aren't supported by this test code. Report the bit size correctly for EC key pairs.

Add a flow where the key is imported or fake-generated in the secure element, then call psa_export_public_key and do the software verification with the public key.

k-stachowiak

I didn't find any blocking issues with the rebased version. I only made a note on a minor inconsistency.

k-stachowiak · 2019-08-09T14:49:45Z

include/psa/crypto_se_driver.h

+ *                              This is \c NULL when generating a symmetric
+ *                              key or if the core does not support
+ *                              exporting the public key at generation time.
+ * \param pubkey_size           The size of the `pubkey` buffer in bytes.


Non-blocker: the pubkey parameter is referenced here in an inconsistent way.

I'm sure it isn't the only place, and it's preexisting. I'll fix it if I change something else.

gilles-peskine-arm · 2019-08-09T15:46:13Z

@AndrzejKurek I forgot to mention: I added a test that goes through export_public. I didn't add all the test you requested because I think that's beyond the scope of this PR: here the goal is to have some basic tests, not good test coverage. In particular, adding support for working with public keys in the fake driver would be a lot of effort for very little gain.

AndrzejKurek

I'm happy with the introduced enhancements.

gilles-peskine-arm added enhancement New feature or request api-spec Issue or PR about the PSA specifications labels Aug 7, 2019

gilles-peskine-arm requested a review from AndrzejKurek August 7, 2019 19:09

gilles-peskine-arm added the needs: review The pull request is ready for review. This generally means that it has no known issues. label Aug 8, 2019

gilles-peskine-arm force-pushed the psa-se_driver-generate_key branch from f87796a to feb9910 Compare August 8, 2019 09:06

k-stachowiak previously approved these changes Aug 8, 2019

View reviewed changes

gilles-peskine-arm mentioned this pull request Aug 8, 2019

Add mock tests for hooks for secure element drivers #174

Merged

AndrzejKurek reviewed Aug 9, 2019

View reviewed changes

AndrzejKurek suggested changes Aug 9, 2019

View reviewed changes

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

gilles-peskine-arm dismissed k-stachowiak’s stale review via 5c6dadd August 9, 2019 13:45

gilles-peskine-arm added 7 commits August 9, 2019 16:43

SE keys: implement and smoke-test p_generate

1179208

SE driver support: Implement sign and verify hooks

edc6424

SE support: Test sign and verify hooks with a passthrough driver

eecadde

RAM test driver: improve key creation

c068ded

Factor common code of ram_import and ram_fake_generate into a common auxiliary function. Reject key types that aren't supported by this test code. Report the bit size correctly for EC key pairs.

RAM test driver: implement export_public

af906f8

SE generate/sign/verify tests: also test export_public

8df72f2

Add a flow where the key is imported or fake-generated in the secure element, then call psa_export_public_key and do the software verification with the public key.

gilles-peskine-arm force-pushed the psa-se_driver-generate_key branch from 5c6dadd to 8df72f2 Compare August 9, 2019 14:44

gilles-peskine-arm requested review from AndrzejKurek and k-stachowiak August 9, 2019 14:45

gilles-peskine-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 9, 2019

k-stachowiak approved these changes Aug 9, 2019

View reviewed changes

AndrzejKurek approved these changes Aug 12, 2019

View reviewed changes

gilles-peskine-arm removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Aug 12, 2019

gilles-peskine-arm merged commit 30e13eb into ARMmbed:psa-api-1.0-beta Aug 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add hooks for generate and sign in a secure element #211

Add hooks for generate and sign in a secure element #211

Uh oh!

gilles-peskine-arm commented Aug 7, 2019 •

edited

Loading

Uh oh!

k-stachowiak Aug 8, 2019

Uh oh!

gilles-peskine-arm Aug 9, 2019

Uh oh!

AndrzejKurek Aug 9, 2019 •

edited

Loading

Uh oh!

gilles-peskine-arm Aug 9, 2019

Uh oh!

AndrzejKurek Aug 9, 2019 •

edited

Loading

Uh oh!

gilles-peskine-arm Aug 9, 2019

Uh oh!

AndrzejKurek left a comment •

edited

Loading

Uh oh!

gilles-peskine-arm commented Aug 9, 2019

Uh oh!

k-stachowiak left a comment

Uh oh!

k-stachowiak Aug 9, 2019

Uh oh!

gilles-peskine-arm Aug 9, 2019

Uh oh!

gilles-peskine-arm commented Aug 9, 2019

Uh oh!

AndrzejKurek left a comment

Uh oh!

Uh oh!

Add hooks for generate and sign in a secure element #211

Add hooks for generate and sign in a secure element #211

Uh oh!

Conversation

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

Uh oh!

Uh oh!

k-stachowiak Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek Aug 9, 2019 • 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!

gilles-peskine-arm Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek Aug 9, 2019 • 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!

gilles-peskine-arm Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek left a comment • 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!

gilles-peskine-arm commented Aug 9, 2019

Uh oh!

k-stachowiak left a comment

Choose a reason for hiding this comment

Uh oh!

k-stachowiak Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Aug 9, 2019

Uh oh!

AndrzejKurek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilles-peskine-arm commented Aug 7, 2019 •

edited

Loading

AndrzejKurek Aug 9, 2019 •

edited

Loading

AndrzejKurek Aug 9, 2019 •

edited

Loading

AndrzejKurek left a comment •

edited

Loading