Secure element key registration #183

gilles-peskine-arm · 2019-07-19T12:44:45Z

Add an API method that allows applications to register a key in a secure element.

Successor of #202. The first new commit is "psa_start_key_creation: take the method as a parameter".

This PR has undergone significant rework. See #183 (comment) . Previous iterations are available as 1, 2, 3, 4.

AndrzejKurek · 2019-07-22T09:40:04Z

include/psa/crypto_extra.h

@@ -154,6 +154,60 @@ typedef struct mbedtls_psa_stats_s
 */
 void mbedtls_psa_get_stats( mbedtls_psa_stats_t *stats );


Typo in commit c3d4baa - "pplications"

AndrzejKurek · 2019-07-22T09:44:18Z

include/psa/crypto_extra.h

+ *
+ * \retval #PSA_SUCCESS
+ *         The key was successfully registered.
+ *         Note that depending on the driver's


Unfinished sentence.

AndrzejKurek · 2019-07-22T09:44:37Z

include/psa/crypto_extra.h

+ *         The key was successfully registered.
+ *         Note that depending on the driver's
+ * \retval #PSA_ERROR_ALREADY_EXISTS
+ *         There is already a key with thw identifier specified in


Typo - thw -> the

AndrzejKurek · 2019-07-22T09:45:01Z

include/psa/crypto_extra.h

+ *         \p attributes.
+ * \retval #PSA_ERROR_INVALID_ARGUMENT
+ *         \p attributes specifies a lifetime which is not located
+ *         in a secure eleement.


Typo - eleement -> element

AndrzejKurek · 2019-07-22T09:48:14Z

include/psa/crypto_se_driver.h

+ * \param[in] key_slot      Slot where the key is to be stored.
+ *
+ * \retval #PSA_SUCCESS
+ *         The given slot number is valid.for a key with the given


Typo - . ->

AndrzejKurek · 2019-07-22T09:55:33Z

library/psa_crypto.c

+                                               &slot->data.se.slot_number );
+            if( status != PSA_SUCCESS )
+                return( status );
+        }


Maybe a call to p_check_slot could be made here if the method is PSA_KEY_CREATION_REGISTER? Just to check that what the application is trying to register is sane.

Yes, but not as is: p_check_slot might say “no, you can't create a key here because it's occupied”. I lean towards adding a psa_key_creation_method_t to p_allocate and p_check_slot, which would allow the driver to enforce constraints like “this slot is locked against import and can only contain internally-generated, non-exportable keys”.

Done in the newest rebase. On a related note, I renamed p_check_slot to p_validate_slot_number in #202.

AndrzejKurek

This design looks promising. What part of the PSA depends on the key type being PSA_KEY_TYPE_RAW_DATA to work properly with secure elements, if any? I'm asking because I'm considering a case where you pass the exact key type that can/will be handled in a given slot during registration.

gilles-peskine-arm · 2019-07-22T10:03:27Z

What part of the PSA depends on the key type being PSA_KEY_TYPE_RAW_DATA to work properly with secure elements, if any?

I'm not sure I understand the question. A secure element doesn't have to support PSA_KEY_TYPE_RAW_DATA. I expect that most do: it's of very limited usefulness since the SE can't use that data, but it helps deployment to use a secure element slot to store a certificate for a private key that's in the secure element, and that slot would have the type PSA_KEY_TYPE_RAW_DATA.

athoelke

Not sure about the "unspecified/undefined" API behaviour if the key attributes has no valid key slot number? (see comments for further thoughts)

athoelke · 2019-07-22T10:05:55Z

include/psa/crypto_extra.h

+    attributes->has_slot_number = 1;
+}
+
+/** Retrieve the enrollment algorithm policy from key attributes.


Cut and paste bug - description copied from psa_get_key_policy

athoelke · 2019-07-22T10:10:59Z

include/psa/crypto_extra.h

+ * \return The slot containing the specified key.
+ *         If the key is not in a secure element,
+ *         or if the caller is not permitted to observe slot numbers,
+ *         the value is unspecified.


Slightly 'euww!'

Can we define a symbolic name for PSA_KEY_SLOT_NUMBER_INVALID (or similar), and have the implementation define it as a constant value of type psa_key_slot_number_t? - this API will return that value if the key slot number is not set (or doesn't exist in this implementation).

That way we get implementation efficiency, precision in the spec, and still permit implementations to define the type of psa_key_slot_number_t and use 0 as a valid slot number.

Right now, all 2^64 potential values for a slot number are considered valid. I'm not sure this is the best design. I'm sure every driver can afford one invalid value. For consistency with the rest of PSA crypto, 0 should be invalid. Making 0 invalid would also avoid the annoying extra has_slot_number bit. But typical secure elements number their slots from 0 to N-1, so it would require most drivers to do some shifting. So declare all-bits-one invalid (with a symbolic constant of course) and mandate implementations to return it when not applicable?

I've changed psa_get_key_slot_number to return an error if there is no recorded slot number. See #201.

athoelke · 2019-07-22T10:13:33Z

include/psa/crypto_extra.h

+static inline psa_key_slot_number_t psa_get_key_slot_number(
+    const psa_key_attributes_t *attributes)
+{
+    return( attributes->slot_number );


Will compiler-error if MBEDTLS_PSA_CRYPTO_SE_C is not defined. Yes I know you say "undefined" but this is a painful way to inform developers that they misunderstand the API and how to fix their mistake.

I'll fix such errors in a batch when I add a build with MBEDTLS_PSA_CRYPTO_SE_C not defined.

athoelke · 2019-07-22T10:17:58Z

include/psa/crypto_extra.h

+ * \retval #PSA_ERROR_INVALID_ARGUMENT
+ *         \p attributes specifies a lifetime which is not located
+ *         in a secure eleement.
+ * \retval #PSA_ERROR_INVALID_ARGUMENT


Duplicate error code - this might be confusing in the API documentation? (or is this pattern used elsewhere)

This pattern is used elsewhere: each \retval is an error reason, and some error reasons can map to the same error code. The alternative is something like

* \retval #PSA_ERROR_INVALID_ARGUMENT * \p attributes specifies a lifetime which is not located * in a secure eleement. * * Alternatively, …

I have no strong feelings either way, but I'd only change this if we change it elsewhere.

athoelke · 2019-07-22T10:21:39Z

include/psa/crypto_se_driver.h

@@ -114,10 +114,14 @@ typedef psa_status_t (*psa_drv_se_init_t)(psa_drv_se_context_t *drv_context,
                                          void *persistent_data,
                                          psa_key_lifetime_t lifetime);

+#ifdef __DOXYGEN_ONLY__
+/* Mbed Crypto defines this type in crypto_extra.h because it is also
+ * available to applications through implementation-specific extensions. */


Is it worth a comment to indicate that this type is also defined in crypto_types.h - and the definitions need to be identical?

I'd prefer to create a new header file. I made this hack to avoid having to mess with build scripts (including the API build scripts).

For the time being, I'm not adding a header file. I think I'll just wait until the header files are no longer used to generate the documentation so this becomes moot. But I did improve the comment.

athoelke · 2019-07-22T10:28:22Z

library/psa_crypto.c

+#if defined(MBEDTLS_PSA_CRYPTO_SE_C)
+    if( psa_get_se_driver( slot->lifetime, &drv, NULL ) )
+    {
+        attributes->slot_number = slot->data.se.slot_number;


Should this also set has_slot_number?

Yes. Fixed.

gilles-peskine-arm · 2019-07-22T18:14:53Z

Thanks for the reviews. I'll fix the documentation mistakes in a batch later, probably rebased against a new version of #157.

gilles-peskine-arm · 2019-07-24T19:48:49Z

I've rebased this work on top of the latest version of #157 . I also made significant changes to the history, to address review comments, to avoid breaking tests, and also to make some design changes.

@athoelke @AndrzejKurek As per our habit I marked the review comments that I've handled with 🚀. I'm still planning to make an improvement pass, but perhaps only after yet another rebase on top of #157 once I've addressed the review comments there.

gilles-peskine-arm · 2019-07-26T14:00:26Z

Now that #157 is merged, I've rebased on top of that and switched the target branch to psa-api-1.0-beta. I'm not planning any more rebases, but I am planning further changes to address review feedback.

gilles-peskine-arm · 2019-07-29T09:01:46Z

On second thoughts, I am planning to rebase and even split this PR. It does ~~three~~ ~~four~~ five things which would be better done separately and in the right sequence:

Store the bit size of a key in a secure element, and make sure that psa_get_key_attributes works for an SE key after a restart. → Secure element keys: save the key size #191
Clean up the code a bit. Much more in Tidy up attribute management inside psa_crypto #197
Report the slot number in key attributes. → Add slot number attribute #201
Allow applications to choose a slot number. → Let applications create a key in a specific secure element slot #202
Key registration. → this PR

k-stachowiak

I went through the changes and ran the test suites, and didn't find issues. I've only found two suspicious documentation changes, noted below.

library/psa_crypto.c

include/psa/crypto_se_driver.h

AndrzejKurek · 2019-08-08T11:54:39Z

include/psa/crypto_se_driver.h

+    PSA_KEY_CREATION_COPY, /**< During psa_copy_key() */
+
+#ifndef __DOXYGEN_ONLY__
+    /** A key is being registered with mbedtls_psa_register_se_key().


Why is this in the doxygen ifndef block?

Because it's an Mbed Crypto extension, so it shouldn't be included in the PSA specification. We normally resolve this by using different files, but this is an enum so it can't be defined partly in one file and partly in another. __DOXYGEN_ONLY__ is not defined when generating the Mbed Crypto documentation.

Let psa_start_key_creation know what type of key creation this is. This will be used at least for key registration in a secure element, which is a peculiar kind of creation since it uses existing key material.

Pass the key creation method (import/generate/derive/copy) to the driver methods to allocate or validate a slot number. This allows drivers to enforce policies such as "this key slot can only be used for keys generated inside the secure element".

Register an existing key in a secure element. Minimal implementation that doesn't call any driver method and just lets the application declare whatever it wants.

When registering a key in a secure element, if the driver has a p_validate_slot_number method, call it.

gilles-peskine-arm · 2019-08-09T13:11:04Z

I rebased again to get rid of the commits from preceding PRs, to resolve a trivial conflict in crypto_se_driver.h due to the late changes of the documentation of psa_drv_se_allocate_key_t in #202, and also to reorder commits (I moved "Pass the key creation method to drivers" before "New function mbedtls_psa_register_se_key") to avoid having an unbuildable commit in the middle.

gilles-peskine-arm added enhancement New feature or request needs: design review DO NOT MERGE The PR is not intended to be merged (yet). Usually used for a review of worked in progress. api-spec Issue or PR about the PSA specifications labels Jul 19, 2019

AndrzejKurek self-requested a review July 22, 2019 08:15

AndrzejKurek reviewed Jul 22, 2019

View reviewed changes

athoelke suggested changes Jul 22, 2019

View reviewed changes

gilles-peskine-arm force-pushed the psa-se_driver-register_key branch from 06ca0b2 to 1dfa905 Compare July 24, 2019 19:38

gilles-peskine-arm changed the base branch from dev/gilles-peskine/psa-se_driver-create_key to psa-api-1.0-beta July 24, 2019 19:43

gilles-peskine-arm changed the base branch from psa-api-1.0-beta to dev/gilles-peskine-arm/psa-se_driver-create_key July 24, 2019 19:45

gilles-peskine-arm added the needs: work The pull request needs rework before it can be merged. label Jul 24, 2019

gilles-peskine-arm mentioned this pull request Jul 25, 2019

Secure element key creation foundation #157

Merged

gilles-peskine-arm force-pushed the psa-se_driver-register_key branch from 1dfa905 to 6ce6e65 Compare July 26, 2019 13:58

gilles-peskine-arm changed the base branch from dev/gilles-peskine-arm/psa-se_driver-create_key to psa-api-1.0-beta July 26, 2019 13:59

gilles-peskine-arm mentioned this pull request Jul 29, 2019

Secure element keys: save the key size #191

Merged

gilles-peskine-arm force-pushed the psa-se_driver-register_key branch from 6ce6e65 to 828963c Compare August 5, 2019 15:18

gilles-peskine-arm removed DO NOT MERGE The PR is not intended to be merged (yet). Usually used for a review of worked in progress. needs: design review needs: work The pull request needs rework before it can be merged. labels Aug 5, 2019

gilles-peskine-arm added needs: preceding PR Requires another PR to be merged first needs: review The pull request is ready for review. This generally means that it has no known issues. labels Aug 5, 2019

gilles-peskine-arm force-pushed the psa-se_driver-register_key branch from 828963c to afb289c Compare August 5, 2019 16:17

k-stachowiak reviewed Aug 6, 2019

View reviewed changes

library/psa_crypto.c Outdated Show resolved Hide resolved

include/psa/crypto_se_driver.h Show resolved Hide resolved

This was referenced Aug 7, 2019

Update the example to the new driver API ARMmbed/mbed-os-example-atecc608a#10

Merged

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

Merged

gilles-peskine-arm force-pushed the psa-se_driver-register_key branch from afb289c to 0252f8d Compare August 8, 2019 09:10

AndrzejKurek reviewed Aug 8, 2019

View reviewed changes

AndrzejKurek previously approved these changes Aug 8, 2019

View reviewed changes

k-stachowiak previously approved these changes Aug 8, 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 9, 2019

gilles-peskine-arm added 5 commits August 9, 2019 14:54

psa_start_key_creation: take the method as a parameter

df17914

Let psa_start_key_creation know what type of key creation this is. This will be used at least for key registration in a secure element, which is a peculiar kind of creation since it uses existing key material.

Pass the key creation method to drivers

e88c2c1

Pass the key creation method (import/generate/derive/copy) to the driver methods to allocate or validate a slot number. This allows drivers to enforce policies such as "this key slot can only be used for keys generated inside the secure element".

New function mbedtls_psa_register_se_key

d772958

Register an existing key in a secure element. Minimal implementation that doesn't call any driver method and just lets the application declare whatever it wants.

SE key registration: call p_validate_slot_number

a5f8749

When registering a key in a secure element, if the driver has a p_validate_slot_number method, call it.

Test the call to p_validate_slot_number when registering a key

49bd582

gilles-peskine-arm dismissed stale reviews from k-stachowiak and AndrzejKurek via 49bd582 August 9, 2019 13:06

gilles-peskine-arm force-pushed the psa-se_driver-register_key branch from 0252f8d to 49bd582 Compare August 9, 2019 13:06

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: preceding PR Requires another PR to be merged first labels Aug 9, 2019

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

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

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

Merged

AndrzejKurek approved these changes Aug 9, 2019

View reviewed changes

k-stachowiak approved these changes Aug 9, 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 9, 2019

gilles-peskine-arm merged commit 5a2d152 into ARMmbed:psa-api-1.0-beta Aug 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secure element key registration #183

Secure element key registration #183

gilles-peskine-arm commented Jul 19, 2019 •

edited

Loading

AndrzejKurek Jul 22, 2019

AndrzejKurek Jul 22, 2019

AndrzejKurek Jul 22, 2019

AndrzejKurek Jul 22, 2019

AndrzejKurek Jul 22, 2019

AndrzejKurek Jul 22, 2019

gilles-peskine-arm Jul 22, 2019

gilles-peskine-arm Aug 5, 2019

AndrzejKurek left a comment •

edited

Loading

gilles-peskine-arm commented Jul 22, 2019

athoelke left a comment

athoelke Jul 22, 2019

athoelke Jul 22, 2019

gilles-peskine-arm Jul 22, 2019

gilles-peskine-arm Aug 5, 2019

athoelke Jul 22, 2019

gilles-peskine-arm Jul 22, 2019

athoelke Jul 22, 2019

gilles-peskine-arm Jul 22, 2019

athoelke Jul 22, 2019

gilles-peskine-arm Jul 22, 2019 •

edited

Loading

gilles-peskine-arm Aug 5, 2019 •

edited

Loading

athoelke Jul 22, 2019

gilles-peskine-arm Jul 22, 2019

gilles-peskine-arm commented Jul 22, 2019

gilles-peskine-arm commented Jul 24, 2019

gilles-peskine-arm commented Jul 26, 2019 •

edited

Loading

gilles-peskine-arm commented Jul 29, 2019 •

edited

Loading

k-stachowiak left a comment

AndrzejKurek Aug 8, 2019

gilles-peskine-arm Aug 9, 2019

gilles-peskine-arm commented Aug 9, 2019

		@@ -154,6 +154,60 @@ typedef struct mbedtls_psa_stats_s
		*/
		void mbedtls_psa_get_stats( mbedtls_psa_stats_t *stats );

Secure element key registration #183

Secure element key registration #183

Conversation

gilles-peskine-arm commented Jul 19, 2019 • 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

AndrzejKurek left a comment • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm commented Jul 22, 2019

athoelke 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

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 Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jul 22, 2019

gilles-peskine-arm commented Jul 24, 2019

gilles-peskine-arm commented Jul 26, 2019 • edited Loading

gilles-peskine-arm commented Jul 29, 2019 • edited Loading

k-stachowiak left a 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 commented Aug 9, 2019

gilles-peskine-arm commented Jul 19, 2019 •

edited

Loading

AndrzejKurek left a comment •

edited

Loading

gilles-peskine-arm Jul 22, 2019 •

edited

Loading

gilles-peskine-arm Aug 5, 2019 •

edited

Loading

gilles-peskine-arm commented Jul 26, 2019 •

edited

Loading

gilles-peskine-arm commented Jul 29, 2019 •

edited

Loading