Openless PSA crypto APIs implementation #3547

ronald-cron-arm · 2020-08-07T16:36:32Z

Description

This PR is the second part of the work related to #3265, the first part being #3527.

This PR implements the openless PSA crypo APIs that take as input or return a key identifier instead of a key handle (see #3265 for details).

Status

Done

Requires Backporting

No, PSA only

ronald-cron-arm · 2020-09-18T15:14:20Z

The CI is as good as it can. Apart from Mbed OS tests, "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.

ronald-cron-arm · 2020-11-19T18:01:07Z

Forced push from 2a61649 to eb9a307:
2a61649 can be found here.
Starting from 2a61649 I have removed the commit "tests: psa: Reset systematically key attributes" (873e688) (no conflict during this history rewrite) and then added the new commit that add psa_reset_key_attributes() where needed.

gilles-peskine-arm

Approved. I'd prefer to improve the lock/acquire terminology, but since this is purely internal, I'm ok with doing that in a follow-up.

ronald-cron-arm · 2020-11-20T07:44:50Z

Approved. I'd prefer to improve the lock/acquire terminology, but since this is purely internal, I'm ok with doing that in a follow-up.

Great. I agree with you that the lock/acquire terminology can and must be improved and I am going to work on it in the coming days. @bensze01 ok to approve on your side as well?

bensze01

I've left a couple of suggestions, but they are all just typo fixes in comments.
Since no actual code changes are necessary, I'm approving this PR.

bensze01 · 2020-11-19T10:03:59Z

include/psa/crypto_compat.h

@@ -34,6 +34,40 @@
 extern "C" {
 #endif

+/*
+ * To support temporary both openless APIs and psa_open_key(), define


Suggested change

* To support temporary both openless APIs and psa_open_key(), define

* To support both openless APIs and psa_open_key() temporarily, define

bensze01 · 2020-11-20T10:20:01Z

library/psa_crypto_core.h

+     * key slot.
+     *
+     * This counter is decremented by one each time a library function stops
+     * accessing to the key slot and states it by calling the


Suggested change

* accessing to the key slot and states it by calling the

* accessing the key slot and states it by calling the

bensze01 · 2020-11-20T10:36:04Z

tests/suites/test_suite_psa_crypto_slot_management.function

+
+    /*
+     * Create a new persistent or volatile key. When creating the key,
+     * one of the description of the previously created persistent key


Suggested change

* one of the description of the previously created persistent key

* one of the descriptions of the previously created persistent keys

bensze01 · 2020-11-20T10:58:58Z

library/psa_crypto_core.h

@@ -38,16 +38,15 @@ typedef struct
    psa_core_key_attributes_t attr;

    /*
-     * Number of on-going accesses, read and/or write, to the key slot by the
-     * library.
+     * Number of locks, read and/or write, to the key slot by the library.


Suggested change

* Number of locks, read and/or write, to the key slot by the library.

* Number of locks on the key slot held by the library.

bensze01 · 2020-11-20T11:02:40Z

library/psa_crypto_core.h

 *
- * A key slot is accessed iff its access counter is strickly greater than
- * 0.
+ * A key slot is locked iff its lock counter is strickly greater than 0.


Suggested change

* A key slot is locked iff its lock counter is strickly greater than 0.

* A key slot is locked iff its lock counter is strictly greater than 0.

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

After a call to psa_get_key_attributes() to retrieve the attributes of a key into a psa_key_attributes_t structure, a call to psa_reset_key_attributes() is mandated to free the resources that may be referenced by the psa_key_attributes_t structure. Not calling psa_reset_key_attributes() may result in a memory leak. When a test function calls psa_get_key_parameters() the associated key attributes are systematically reset in the clean-up part of the function with a comment to emphasize the need for the reset and make it more visible. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm · 2020-11-20T13:09:26Z

Force push from eb9a307 to 3a4f0e3:
I've just reworked the penultimate commit "Improve/fix documentation" to address Bence's last comments.
You can find the diff between eb9a307 to 3a4f0e3 here.

gilles-peskine-arm

As before, approved on the condition of a follow-up that improves acquire/lock terminology.

bensze01

All of my comments were addressed, so I'm approving this PR as well.

gilles-peskine-arm · 2020-11-20T17:48:28Z

CI passed except for expected ABI/API changes (psa_key_handle_t → psa_key_identifier_t or mbedtls_svc_key_id_t, changes in internal structures). I filed #3904 for the improvements around acquire/lock naming. This is ready for merge.

gilles-peskine-arm · 2021-05-28T10:58:45Z

include/psa/crypto_values.h

+ */
+static inline int mbedtls_svc_key_id_is_null( mbedtls_svc_key_id_t key )
+{
+    return( ( key.key_id == 0 ) && ( key.owner == 0 ) );


This should be just key.key_id == 0, shouldn't it? If a client passes a null key id, owner information will be attached to it, but owner information about a null key id is not relevant. Fixing in #4392.

ronald-cron-arm self-assigned this Aug 7, 2020

ronald-cron-arm added enhancement mbed TLS team needs-ci Needs to pass CI tests needs-work labels Aug 7, 2020

ronald-cron-arm added this to the August 2020 Sprint milestone Aug 7, 2020

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

Implement the openless crypto API #3265

Closed

ronald-cron-arm added the needs-preceding-pr Requires another PR to be merged first label Aug 7, 2020

ronald-cron-arm force-pushed the psa-openless branch from 80cc786 to b53bef4 Compare August 7, 2020 18:03

daverodgman added the HwDrivers label Sep 2, 2020

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

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

Implement the openless crypto API #3265

Closed

ronald-cron-arm force-pushed the psa-openless branch from b53bef4 to c21e149 Compare September 18, 2020 07:10

ronald-cron-arm removed the needs-preceding-pr Requires another PR to be merged first label Sep 18, 2020

ronald-cron-arm force-pushed the psa-openless branch 2 times, most recently from 9b6be96 to d1f88fc Compare September 18, 2020 12:43

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 needs-work labels Sep 18, 2020

mpg requested review from mpg and removed request for mpg September 22, 2020 08:50

bensze01 added the Product Backlog label Sep 30, 2020

gilles-peskine-arm previously approved these changes Nov 19, 2020

View reviewed changes

bensze01 previously approved these changes Nov 20, 2020

View reviewed changes

ronald-cron-arm added 2 commits November 20, 2020 13:57

Improve/fix documentation

1d12d87

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm dismissed stale reviews from bensze01 and gilles-peskine-arm via 3a4f0e3 November 20, 2020 13:06

ronald-cron-arm force-pushed the psa-openless branch from eb9a307 to 3a4f0e3 Compare November 20, 2020 13:06

ronald-cron-arm requested review from gilles-peskine-arm and bensze01 November 20, 2020 13:10

gilles-peskine-arm approved these changes Nov 20, 2020

View reviewed changes

gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Nov 20, 2020

bensze01 approved these changes Nov 20, 2020

View reviewed changes

bensze01 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 Nov 20, 2020

gilles-peskine-arm mentioned this pull request Nov 20, 2020

Clarify access protection terminology on PSA key slots #3904

Open

gilles-peskine-arm merged commit 662deb3 into Mbed-TLS:development Nov 20, 2020

gilles-peskine-arm mentioned this pull request Nov 20, 2020

Use PSA_ERROR_DATA_INVALID where warranted #3872

Merged

2 tasks

hug-dev mentioned this pull request Dec 11, 2020

Handling key handle closing is noisy and error prone parallaxsecond/rust-psa-crypto#34

Closed

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

Enforce slot lock/unlock pairing robustness through documentation and testing #4464

Open

gilles-peskine-arm reviewed May 28, 2021

View reviewed changes

ronald-cron-arm deleted the psa-openless branch July 21, 2023 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Openless PSA crypto APIs implementation #3547

Openless PSA crypto APIs implementation #3547

ronald-cron-arm commented Aug 7, 2020 •

edited

Loading

ronald-cron-arm commented Sep 18, 2020

ronald-cron-arm commented Nov 19, 2020 •

edited

Loading

gilles-peskine-arm left a comment

ronald-cron-arm commented Nov 20, 2020

bensze01 left a comment •

edited

Loading

bensze01 Nov 19, 2020

bensze01 Nov 20, 2020

bensze01 Nov 20, 2020

bensze01 Nov 20, 2020

bensze01 Nov 20, 2020

ronald-cron-arm commented Nov 20, 2020 •

edited

Loading

gilles-peskine-arm left a comment

bensze01 left a comment

gilles-peskine-arm commented Nov 20, 2020

gilles-peskine-arm May 28, 2021 •

edited

Loading

	* To support temporary both openless APIs and psa_open_key(), define
	* To support both openless APIs and psa_open_key() temporarily, define

	* accessing to the key slot and states it by calling the
	* accessing the key slot and states it by calling the

	* one of the description of the previously created persistent key
	* one of the descriptions of the previously created persistent keys

	* Number of locks, read and/or write, to the key slot by the library.
	* Number of locks on the key slot held by the library.

	* A key slot is locked iff its lock counter is strickly greater than 0.
	* A key slot is locked iff its lock counter is strictly greater than 0.

Openless PSA crypto APIs implementation #3547

Openless PSA crypto APIs implementation #3547

Conversation

ronald-cron-arm commented Aug 7, 2020 • edited Loading

Description

Status

Requires Backporting

ronald-cron-arm commented Sep 18, 2020

ronald-cron-arm commented Nov 19, 2020 • edited Loading

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm commented Nov 20, 2020

bensze01 left a comment • edited Loading

Choose a reason for hiding this comment

bensze01 Nov 19, 2020

Choose a reason for hiding this comment

bensze01 Nov 20, 2020

Choose a reason for hiding this comment

bensze01 Nov 20, 2020

Choose a reason for hiding this comment

bensze01 Nov 20, 2020

Choose a reason for hiding this comment

bensze01 Nov 20, 2020

Choose a reason for hiding this comment

ronald-cron-arm commented Nov 20, 2020 • edited Loading

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

bensze01 left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Nov 20, 2020

gilles-peskine-arm May 28, 2021 • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm commented Aug 7, 2020 •

edited

Loading

ronald-cron-arm commented Nov 19, 2020 •

edited

Loading

bensze01 left a comment •

edited

Loading

ronald-cron-arm commented Nov 20, 2020 •

edited

Loading

gilles-peskine-arm May 28, 2021 •

edited

Loading