PSA key identifiers rework #3527

ronald-cron-arm · 2020-07-29T15:54:56Z

Description

This PR is the first part of the work related to #3265.

With openless PSA APIs, transient key identifier will be introduced for volatile keys thus the need to changes things around key identifiers in the libray. This PR doesn't introduce the transient key identifiers but pave the way to their introduction.

This PRs addresses two main things:

Always define psa_key_id_t as defined in PSA crypto spec. Before this PR, this is not the case when the MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER configuration flag is set.
Make the library compiles when the MBEDTLS_PSA_CRYPTO_KEY_EXTENDED_ID (renaming of MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER) and test it with that configuration flag set
thus test the PSA APIs when the key identifiers are extended to contain a key owner identifier.

Status

Needs review and expected to need rework as well.

Requires Backporting

No, PSA only work.

ronald-cron-arm · 2020-07-30T07:24:57Z

The CI is as good as it can. Apart from Mbed OS Testing and Travis current issues, "merge TLS Testing" and "pr-merge" are falling because of the "ABI-API" testing as expected as this PR changes the definition of API parameter types.

paul-elliott-arm

Nothing major here for me, just a typo and some style issues. Functionality-wise, looks great

I would make a (very) minor point about structures being copied around by =, which may be interpreted less than optimally, performance wise by some compilers. Given the whole struct is 64 bits long, this will hopefully end up in a single register copy on a 64 bit machine, however I would fear it could turn into a memcpy on 32 bit. Certainly, should the extended Id struct get any bigger, it could certainly be worth looking at a copy function in another patch.

paul-elliott-arm · 2020-08-20T14:42:19Z

library/psa_crypto_slot_management.c

- *                      0 to allow only key ids in the application range.
+ * \param key_id     The key identifier to check.
+ * \param vendor_ok  Nonzero to allow key identifiers in the vendor range.
+ *                   0 to allow only key dentifiers in the application range.


nit: typo identifiers

library/psa_crypto_storage.c

paul-elliott-arm · 2020-08-20T15:27:07Z

library/psa_crypto_storage.c

    /* Encode the owner in the upper 32 bits. This means that if
     * owner values are nonzero (as they are on a PSA platform),
     * no key file will ever have a value less than 0x100000000, so
     * the whole range 0..0xffffffff is available for non-key files. */
-    uint32_t unsigned_owner = (uint32_t) file_id.owner;
-    return( (uint64_t) unsigned_owner << 32 | file_id.key_id );
+    uint32_t unsigned_owner_id = (uint32_t) key_id.owner_id;


would it not be better to use the access macros here? Would standardise things and make it easier should anything change in the future.

I've done it in a new commit.

paul-elliott-arm · 2020-08-20T15:35:22Z

tests/suites/test_suite_psa_crypto.function

    psa_key_lifetime_t lifetime = lifetime_arg;
    psa_key_usage_t usage_flags = usage_flags_arg;
    psa_algorithm_t alg = alg_arg;
    psa_key_type_t type = type_arg;
    size_t bits = bits_arg;

-    TEST_EQUAL( psa_get_key_id( &attributes ), 0 );
+    #if !defined(MBEDTLS_PSA_CRYPTO_KEY_EXTENDED_ID)


nit: I would argue that this should be at the beginning of the function. It is rather odd looking as it looks as if owner_id_arg is actually used, but of course the macro doesn't use it in this case.

I've introduced a psa_key_id_init() inline function to get rid of this code.

paul-elliott-arm · 2020-08-20T15:38:55Z

tests/suites/test_suite_psa_crypto.function

    psa_key_lifetime_t expected_lifetime = expected_lifetime_arg;

+    #if !defined(MBEDTLS_PSA_CRYPTO_KEY_EXTENDED_ID)


Nit: Again, I would argue that this should be at the beginning of the function.

ditto, not there anymore.

paul-elliott-arm · 2020-08-20T15:44:38Z

tests/suites/test_suite_psa_crypto_se_driver_hal.function

    /* The tests may have potentially created key ids from 1 to
     * MAX_KEY_ID_FOR_TEST. In addition, run the destroy function on key id
     * 0, which file-based storage uses as a temporary file. */
    for( id = 0; id <= MAX_KEY_ID_FOR_TEST; id++ )
-        psa_destroy_persistent_key( id );
+    {
+        key_id = (PSA_KEY_ID_T)PSA_KEY_ID_INIT( 1, id );


Why is a cast required here?

The cast was not necessary. Removed.

paul-elliott-arm · 2020-08-20T15:46:20Z

tests/suites/test_suite_psa_crypto_se_driver_hal.function

    size_t bit_size = 48;
    psa_key_slot_number_t wanted_slot = 0x123456789;
    psa_key_handle_t handle = 0;
    psa_status_t status;

+    #if !defined(MBEDTLS_PSA_CRYPTO_KEY_EXTENDED_ID)


Nit: same as usual

paul-elliott-arm · 2020-08-20T15:47:49Z

tests/suites/test_suite_psa_crypto_se_driver_hal_mocks.function

    /* The tests may have potentially created key ids from 1 to
     * MAX_KEY_ID_FOR_TEST. In addition, run the destroy function on key id
     * 0, which file-based storage uses as a temporary file. */
    for( id = 0; id <= MAX_KEY_ID_FOR_TEST; id++ )
-        psa_destroy_persistent_key( id );
+    {
+        key_id = (PSA_KEY_ID_T)PSA_KEY_ID_INIT( 1, id );


Again, why the cast here?

Cast not necessary, removed.

paul-elliott-arm · 2020-08-20T17:13:34Z

tests/suites/test_suite_psa_crypto_slot_management.function

@@ -307,6 +328,10 @@ void create_existent( int lifetime_arg, int id_arg,
    size_t reexported_length;
    reopen_policy_t reopen_policy = reopen_policy_arg;

+    #if !defined(MBEDTLS_PSA_CRYPTO_KEY_EXTENDED_ID)


Nit:Usual complaint about this belonging at the beginning of the function.

Usual reply.

paul-elliott-arm · 2020-08-20T17:14:49Z

tests/suites/test_suite_psa_crypto_slot_management.function

@@ -437,17 +464,20 @@ void copy_across_lifetimes( int source_lifetime_arg, int source_id_arg,
    psa_algorithm_t expected_alg2 = expected_alg2_arg;
    uint8_t *export_buffer = NULL;

+    #if !defined(MBEDTLS_PSA_CRYPTO_KEY_EXTENDED_ID)


gilles-peskine-arm

I like the reorganization of the abstraction for key ids, but not the new names. They are meaningless: “key id of key id”, KEY_ID vs key_id, “extended”. The old names were meaningful. I think you should mostly keep the old names and clean up the way they're used.

I don't think including crypto_types.h before crypto_platform.h is right. Some of the types, including key ids, depend on the platform. You've introduced platform dependencies into crypto_types.h.

It's unfortunate that this change has a significant impact on the test code, which should generally be as close as possible to application code in the way it uses the API. But I guess this is unavoidable because on a platform with multiple clients, we don't have a way to run test code in multiple clients: all the test code is running on the keystore side. But maybe we could define a few functions that provide a more abstract interface under which we could add RPC later?

gilles-peskine-arm · 2020-08-21T19:22:14Z

include/mbedtls/config.h

@@ -1260,20 +1260,20 @@
 */
 //#define MBEDTLS_ENTROPY_NV_SEED

-/* MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER


TF-M uses MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER, so if we remove it or change its semantics, we need to provide a transition path or work this out with the TF-M development team. Sure, we did say that this option was meant for internal use only, but this is another TF project, which is close to internal use, and it's the heir of the project that the option was meant for (Mbed OS PSA).

When the TF-M team updates to a new version of the present library it is easy for them to change such a define if necessary, changing it shouldn't be a problem for them.

include/mbedtls/config.h

include/psa/crypto.h

include/psa/crypto_platform.h

tests/suites/test_suite_psa_crypto_se_driver_hal.function

tests/suites/test_suite_psa_crypto_slot_management.function

ronald-cron-arm · 2020-08-25T16:25:12Z

I've rebased on top of development and addressed most of the issues, not all though. The previous version of the PR can be found here.

ronald-cron-arm · 2020-08-27T06:38:35Z

The CI testing is back to "as good as it can". Apart from Mbed OS Testing current issues, "merge TLS Testing" and "pr-merge" are falling because of the "ABI-API" testing as expected as this PR changes the definition of API parameter types.

@paul-elliott-arm I've addressed all your comments.

@gilles-peskine-arm I've addressed most of your comments. For the remaining ones (renaming and include order), I've tried to clarify the intent or asked questions in trying to find a way forward. Please have a look.

I am putting back the label "needs: review" as I am now waiting for replies to my clarifications and questions.

To test the proper handling of owner identifier as of key identifiers, add owner identifier(s) to tests having key identifier(s) as test parameters. Just don't do it for tests related to tests invalid values of key identifiers as there is no owner identifier invalid values. Signed-off-by: Ronald Cron <[email protected]>

Add checks involving unknown key owner identifiers in tests related to SE and persistent keys. Signed-off-by: Ronald Cron <[email protected]>

Use macros instead of accessing directly the key identifier fields for coding consistency and ease maintenance. Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm · 2020-09-02T13:36:15Z

I've addressed all comments and the CI is back to as good as it can. Please review again. Previous reviewed version of the PR can be found here.

gilles-peskine-arm

I'm happy overall. I only have a few localized comments. (This is a review of 79ca427)

include/psa/crypto_types.h

gilles-peskine-arm · 2020-09-10T18:20:53Z

include/psa/crypto_extra.h

@@ -232,9 +232,9 @@ typedef struct mbedtls_psa_stats_s
    /** Number of slots that are not used for anything. */
    size_t empty_slots;
    /** Largest key id value among open keys in internal persistent storage. */
-    psa_app_key_id_t max_open_internal_key_id;
+    psa_key_id_t max_open_internal_key_id;


Minor: the commit message “Define always psa_key_id_t as defined in PSA crypto spec” should be “Always define …”. Not worth rebasing for just that. Please don't change it unless you need to rebase for some other reason anyway.

include/mbedtls/config.h

scripts/config.py

include/psa/crypto_platform.h

include/psa/crypto_types.h

Define inline for some compiling environment only in crypto_platform.h. Signed-off-by: Ronald Cron <[email protected]>

Signed-off-by: Ronald Cron <[email protected]>

No obvious reason to not enable owner identifier encoding in baremetal as multi-client support is expected to be needed for some embedded platforms. Thus enable it. Signed-off-by: Ronald Cron <[email protected]>

Move key identifier related macros and functions from crypto_types.h to crypto_values.h as the latter is the intended file to put them in. Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm · 2020-09-15T11:35:06Z

@gilles-peskine-arm and @paul-elliott-arm, CI is back to as good as it can and I've addressed all comments. Please have another look.

paul-elliott-arm

Looks good to me!

ronald-cron-arm self-assigned this Jul 29, 2020

ronald-cron-arm added component-crypto Crypto primitives and low-level interfaces enhancement mbed TLS team needs-ci Needs to pass CI tests labels Jul 29, 2020

ronald-cron-arm added this to the July 2020 Sprint milestone Jul 29, 2020

ronald-cron-arm linked an issue Jul 29, 2020 that may be closed by this pull request

Implement the openless crypto API #3265

Closed

ronald-cron-arm changed the title ~~Key extended~~ Key extended identifiers rework Jul 29, 2020

ronald-cron-arm changed the title ~~Key extended identifiers rework~~ PSA key identifiers rework Jul 29, 2020

ronald-cron-arm force-pushed the key-extended-id branch from 0c9a8e3 to 41560ae Compare July 29, 2020 16:44

ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Jul 30, 2020

danh-arm modified the milestones: July 2020 Sprint, August 2020 Sprint Aug 3, 2020

ronald-cron-arm mentioned this pull request Aug 7, 2020

Openless PSA crypto APIs implementation #3547

Merged

paul-elliott-arm self-requested a review August 19, 2020 09:08

paul-elliott-arm requested changes Aug 20, 2020

View reviewed changes

gilles-peskine-arm requested changes Aug 21, 2020

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 21, 2020

ronald-cron-arm force-pushed the key-extended-id branch from 41560ae to 96f4cc4 Compare August 25, 2020 16:11

ronald-cron-arm force-pushed the key-extended-id branch 2 times, most recently from 79ba409 to fccad13 Compare August 26, 2020 14:45

ronald-cron-arm added the needs-review Every commit must be reviewed by at least two team members, label Aug 27, 2020

ronald-cron-arm added 3 commits September 2, 2020 14:27

tests: psa: Add checks involving unknown key owner ids

f1c9a55

Add checks involving unknown key owner identifiers in tests related to SE and persistent keys. Signed-off-by: Ronald Cron <[email protected]>

psa: storage: Use key id macros to compute ITS file identifier

79ca427

Use macros instead of accessing directly the key identifier fields for coding consistency and ease maintenance. Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm force-pushed the key-extended-id branch from 1aa90dc to 79ca427 Compare September 2, 2020 12:30

ronald-cron-arm removed the needs-work label Sep 2, 2020

daverodgman added the HwDrivers label Sep 2, 2020

daverodgman modified the milestones: August 2020 Sprint, September 2020 Sprint Sep 2, 2020

gilles-peskine-arm requested changes Sep 10, 2020

View reviewed changes

gilles-peskine-arm requested a review from paul-elliott-arm September 10, 2020 19:58

ronald-cron-arm added 4 commits September 15, 2020 08:48

Remove unnecessary inline definition

5eba579

Define inline for some compiling environment only in crypto_platform.h. Signed-off-by: Ronald Cron <[email protected]>

Fix key owner identifier documentation

9a2511e

Signed-off-by: Ronald Cron <[email protected]>

Set MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER in baremetal configuration

cb54610

No obvious reason to not enable owner identifier encoding in baremetal as multi-client support is expected to be needed for some embedded platforms. Thus enable it. Signed-off-by: Ronald Cron <[email protected]>

psa: Move key identifier related macros and functions

7424f0d

Move key identifier related macros and functions from crypto_types.h to crypto_values.h as the latter is the intended file to put them in. Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm force-pushed the key-extended-id branch from 89ca10f to 7424f0d Compare September 15, 2020 06:51

ronald-cron-arm requested a review from gilles-peskine-arm September 15, 2020 11:35

gilles-peskine-arm approved these changes Sep 15, 2020

View reviewed changes

paul-elliott-arm approved these changes Sep 15, 2020

View reviewed changes

gilles-peskine-arm merged commit dc57c25 into Mbed-TLS:development Sep 15, 2020

ronald-cron-arm mentioned this pull request Sep 15, 2020

Implement the openless crypto API #3265

Closed

gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Sep 16, 2020

ronald-cron-arm deleted the key-extended-id branch July 21, 2023 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSA key identifiers rework #3527

PSA key identifiers rework #3527

ronald-cron-arm commented Jul 29, 2020 •

edited

Loading

ronald-cron-arm commented Jul 30, 2020

paul-elliott-arm left a comment

paul-elliott-arm Aug 20, 2020

ronald-cron-arm Aug 25, 2020

paul-elliott-arm Aug 20, 2020

ronald-cron-arm Aug 25, 2020

paul-elliott-arm Aug 20, 2020

ronald-cron-arm Aug 25, 2020 •

edited

Loading

paul-elliott-arm Aug 20, 2020

ronald-cron-arm Aug 25, 2020

paul-elliott-arm Aug 20, 2020

ronald-cron-arm Aug 25, 2020

paul-elliott-arm Aug 20, 2020

ronald-cron-arm Aug 25, 2020

paul-elliott-arm Aug 20, 2020

ronald-cron-arm Aug 25, 2020

paul-elliott-arm Aug 20, 2020

ronald-cron-arm Aug 25, 2020

paul-elliott-arm Aug 20, 2020

ronald-cron-arm Aug 25, 2020

gilles-peskine-arm left a comment

gilles-peskine-arm Aug 21, 2020

ronald-cron-arm Aug 25, 2020

ronald-cron-arm commented Aug 25, 2020

ronald-cron-arm commented Aug 27, 2020

ronald-cron-arm commented Sep 2, 2020

gilles-peskine-arm left a comment •

edited

Loading

gilles-peskine-arm Sep 10, 2020

ronald-cron-arm commented Sep 15, 2020

paul-elliott-arm left a comment

		psa_key_lifetime_t expected_lifetime = expected_lifetime_arg;

		#if !defined(MBEDTLS_PSA_CRYPTO_KEY_EXTENDED_ID)

PSA key identifiers rework #3527

PSA key identifiers rework #3527

Conversation

ronald-cron-arm commented Jul 29, 2020 • edited Loading

Description

Status

Requires Backporting

ronald-cron-arm commented Jul 30, 2020

paul-elliott-arm left a 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

ronald-cron-arm Aug 25, 2020 • edited Loading

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

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

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm commented Aug 25, 2020

ronald-cron-arm commented Aug 27, 2020

ronald-cron-arm commented Sep 2, 2020

gilles-peskine-arm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm commented Sep 15, 2020

paul-elliott-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm commented Jul 29, 2020 •

edited

Loading

ronald-cron-arm Aug 25, 2020 •

edited

Loading

gilles-peskine-arm left a comment •

edited

Loading