Implement psa_sign_message and psa_verify_message #4357

gabor-mezei-arm · 2021-04-16T12:36:41Z

Description

Implement the PSA Crypto API 1.0.0 functions psa_sign_messsage and psa_verify_message.

Since Mbed TLS only supports hash-and-sign algorithms, these are simple wrappers around psa_hash_compute and psa_sign_hash/psa_verify_hash.

Also part of the goal of this task is to implement the corresponding policies PSA_KEY_USAGE_SIGN_MESSAGE and PSA_KEY_USAGE_VERIFY_MESSAGE.

Resolve #3267

Requires Forwardporting

mbedtls-3.0 [Forwardport 3.0] Implement psa_sign_message and psa_verify_message #4510

gilles-peskine-arm

I made a first review pass, just for design. I did not review the documentation, the code or the test data in detail. I'm happy with the design. The test coverage looks ok (but I may ask for a few specific tests once I've done a detailed review).

Some things are missing. They can be done either in this PR or in a follow-up.

Drivers can have a sign/verify-message entry point. So there needs to be a psa_driver_wrapper_sign_message and a psa_driver_wrapper_sign_hash. If there is no all-in-one method for the given algorithm, then fall back to the code you've already written that hashes then signs. It may be better to first merge this PR, then implement the new driver dispatch at once for both sign-hash and sign-message — what do you think @ronald-cron-arm?
psa_exercise_key should have cases for keys that have the SIGN_MESSAGE or VERIFY_MESSAGE usage.

library/psa_crypto.c

tests/suites/test_suite_psa_crypto.data

include/psa/crypto_values.h

library/psa_crypto.c

stevew817 · 2021-04-19T08:23:33Z

It may be better to first merge this PR, then implement the new driver dispatch at once for both sign-hash and sign-message.

There's plenty of examples for functionality that has been converted to driver dispatch already. Conversion for this PR should be the same as following the footpath already created by the dispatch of psa_sign_hash and psa_verify_hash. I'd very much like for that to happen in this PR, since it's not all that much work and avoids the lead-time of yet another PR.

stevew817 · 2021-04-19T14:07:03Z

library/psa_crypto.c

+        return( PSA_ERROR_BUFFER_TOO_SMALL );
+
+    status = psa_get_and_lock_key_slot_with_policy( key, &slot,
+                                                    PSA_KEY_USAGE_SIGN_MESSAGE,


@gilles-peskine-arm Shouldn't keys registered with PSA_KEY_USAGE_SIGN_HASH also be able to be used with sign_message, since SIGN_HASH is more permissive than SIGN_MESSAGE?

Asking in the interest of compatibility, i.e. keys that have so far been created with SIGN_HASH permission cannot be upgraded to SIGN_MESSAGE, so the next best thing would be to allow them to be used for the all-in-one operation too.

Indeed, and likewise with VERIFY. Though this isn't the best place to implement it, since according to the specification

if an application sets the flag PSA_KEY_USAGE_SIGN_HASH when creating a key, then the key always has the permissions conveyed by PSA_KEY_USAGE_SIGN_MESSAGE, and the flag PSA_KEY_USAGE_SIGN_MESSAGE will also be present when the application queries the usage flags of the key.

So the right place to implement this is when setting the attributes of a new key. Otherwise psa_get_key_attributes won't do the right thing. This is part of the scope of #3267, but it's up to @gabor-mezei-arm whether to do it in this pull request or in a follow-up.

Upgrading existing keys in persistent storage was not in scope, so I've filed a separate issue for it.

It will done in a separate PR.

gabor-mezei-arm · 2021-04-20T11:20:44Z

@stevew817 @ronald-cron-arm The driver wrapper of psa_sign_hash is calling the crypto SE driver p_sign if crypto SE is enabled. Is there a need for a similar driver for the psa_sign_message or just use the p_sign for the signing step?

ronald-cron-arm · 2021-04-21T08:38:48Z

@stevew817 @ronald-cron-arm The driver wrapper of psa_sign_hash is calling the crypto SE driver p_sign if crypto SE is enabled. Is there a need for a similar driver for the psa_sign_message or just use the p_sign for the signing step?

The current approach regarding the dynamically registered drivers is just to not break the current support, no need to add additional support. Thus here I don't think you need to do anything regarding the dynamically registered drivers.

gabor-mezei-arm · 2021-04-27T15:22:12Z

Rebased to development_2.x, need to be forwardported to the development branch.

stevew817

Looks even better to me after these last 4 commits 👍

gilles-peskine-arm · 2021-05-12T14:55:56Z

Thanks for the updates, looks good to me as well. Please make a PR for 3.0.

gilles-peskine-arm · 2021-05-13T14:44:10Z

tests/suites/test_suite_psa_crypto_driver_wrappers.function

+
+    PSA_ASSERT( psa_crypto_init( ) );
+    psa_set_key_type( &attributes,
+                      PSA_KEY_TYPE_ECC_KEY_PAIR( PSA_ECC_CURVE_SECP_R1 ) );


Please replace PSA_ECC_CURVE_xxx by PSA_ECC_FAMILY_xxx as well. PSA_ECC_CURVE_xxx are deprecated in 2.x, even if they aren't removed yet.

CI only let this through because this code isn't built in the full config.

Signed-off-by: gabor-mezei-arm <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

Backport 2.x: Add changelog entry for #4357

gabor-mezei-arm added enhancement component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests PSA compliance labels Apr 16, 2021

gabor-mezei-arm self-assigned this Apr 16, 2021

gabor-mezei-arm force-pushed the 3267_Implement_psa_sign_message_and_verify branch from e80a36e to 2198a7e Compare April 16, 2021 14:41

gilles-peskine-arm requested changes Apr 16, 2021

View reviewed changes

daverodgman added the HwDrivers label Apr 19, 2021

gabor-mezei-arm force-pushed the 3267_Implement_psa_sign_message_and_verify branch from 2198a7e to b8e985e Compare April 19, 2021 10:51

gabor-mezei-arm added needs-work and removed needs-ci Needs to pass CI tests labels Apr 19, 2021

stevew817 reviewed Apr 19, 2021

View reviewed changes

gabor-mezei-arm force-pushed the 3267_Implement_psa_sign_message_and_verify branch 5 times, most recently from 9ed23a7 to e48e4bd Compare April 27, 2021 09:14

gabor-mezei-arm changed the base branch from development to development_2.x April 27, 2021 15:08

gabor-mezei-arm force-pushed the 3267_Implement_psa_sign_message_and_verify branch from c80d7a2 to 66a8b9b Compare April 27, 2021 15:10

gabor-mezei-arm added needs-ci Needs to pass CI tests 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-work needs-ci Needs to pass CI tests labels Apr 27, 2021

gabor-mezei-arm dismissed stevew817’s stale review via f25c976 May 12, 2021 09:15

stevew817 previously approved these changes May 12, 2021

View reviewed changes

gilles-peskine-arm approved these changes May 12, 2021

View reviewed changes

gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label May 12, 2021

gabor-mezei-arm mentioned this pull request May 13, 2021

[Forwardport 3.0] Implement psa_sign_message and psa_verify_message #4510

Merged

gilles-peskine-arm requested changes May 13, 2021

View reviewed changes

gabor-mezei-arm dismissed stevew817’s stale review via fc51f43 May 14, 2021 08:08

stevew817 previously approved these changes May 14, 2021

View reviewed changes

Update key type name

c97b8ab

Signed-off-by: gabor-mezei-arm <[email protected]>

gabor-mezei-arm dismissed stevew817’s stale review via c97b8ab May 14, 2021 08:12

gabor-mezei-arm force-pushed the 3267_Implement_psa_sign_message_and_verify branch from fc51f43 to c97b8ab Compare May 14, 2021 08:12

gabor-mezei-arm requested review from gilles-peskine-arm and stevew817 May 14, 2021 08:12

stevew817 approved these changes May 14, 2021

View reviewed changes

gabor-mezei-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-backports Backports are missing or are pending review and approval. labels May 14, 2021

gilles-peskine-arm approved these changes May 17, 2021

View reviewed changes

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels May 17, 2021

gilles-peskine-arm merged commit bed4e9e into Mbed-TLS:development_2.x May 17, 2021

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request May 17, 2021

Add changelog entry for Mbed-TLS#4357

40ffee4

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request May 17, 2021

Add changelog entry for Mbed-TLS#4357

6e0b043

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm mentioned this pull request May 17, 2021

Backport 2.x: Add changelog entry for #4357 #4525

Merged

gilles-peskine-arm added a commit that referenced this pull request May 19, 2021

Merge pull request #4525 from gilles-peskine-arm/pr_4357-changelog

e14e839

Backport 2.x: Add changelog entry for #4357

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement psa_sign_message and psa_verify_message #4357

Implement psa_sign_message and psa_verify_message #4357

gabor-mezei-arm commented Apr 16, 2021 •

edited

Loading

gilles-peskine-arm left a comment •

edited by gabor-mezei-arm

Loading

stevew817 commented Apr 19, 2021

stevew817 Apr 19, 2021

gilles-peskine-arm Apr 19, 2021

gabor-mezei-arm Apr 28, 2021

gabor-mezei-arm commented Apr 20, 2021

ronald-cron-arm commented Apr 21, 2021

gabor-mezei-arm commented Apr 27, 2021

stevew817 left a comment

gilles-peskine-arm commented May 12, 2021

gilles-peskine-arm May 13, 2021

Implement psa_sign_message and psa_verify_message #4357

Implement psa_sign_message and psa_verify_message #4357

Conversation

gabor-mezei-arm commented Apr 16, 2021 • edited Loading

Description

Requires Forwardporting

gilles-peskine-arm left a comment • edited by gabor-mezei-arm Loading

Choose a reason for hiding this comment

stevew817 commented Apr 19, 2021

stevew817 Apr 19, 2021

Choose a reason for hiding this comment

gilles-peskine-arm Apr 19, 2021

Choose a reason for hiding this comment

gabor-mezei-arm Apr 28, 2021

Choose a reason for hiding this comment

gabor-mezei-arm commented Apr 20, 2021

ronald-cron-arm commented Apr 21, 2021

gabor-mezei-arm commented Apr 27, 2021

stevew817 left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented May 12, 2021

gilles-peskine-arm May 13, 2021

Choose a reason for hiding this comment

gabor-mezei-arm commented Apr 16, 2021 •

edited

Loading

gilles-peskine-arm left a comment •

edited by gabor-mezei-arm

Loading