Let applications create a key in a specific secure element slot #202

gilles-peskine-arm · 2019-08-05T13:02:27Z

Successor of #201. The first new commit is "Improve documentation of the allocate method".

k-stachowiak

I didn't find any issues with the code. I only have two minor suggestions regarding the test code.

tests/suites/test_suite_psa_crypto_se_driver_hal.function

AndrzejKurek · 2019-08-07T15:14:32Z

include/psa/crypto_se_driver.h

+     * if the platform is reset after this function returns, the core
+     * may either subsequently call
+     * psa_drv_se_key_management_t::p_destroy or may behave as if the
+     * last call to this function had not taken place.


What do you mean by "behave as if the last call to this function had not taken place"? Should the driver store persistent data about allocation separately? Should it be able to work in the following situation:

p_allocate called, slot 1 returned;

hardware resets;

p_generate called with slot 1?

I'm actually not sure about that case. I'm going to try to come up with clearer explanations, but first I need to figure out in more detail which behaviors are possible.

I'd like to merge this PR without necessarily getting all the corner cases right (and there's a precedent: this function has been there for several weeks with incomplete documentation). But I'm planning to make at least one update here.

I've reworked the documentation. I don't think it covers all the subtleties that can arise if an error or a reset happens, but I think it's good enough for now.

AndrzejKurek · 2019-08-07T15:16:32Z

include/psa/crypto_se_driver.h

-    /** Function that allocates a slot. */
+    /** Function that allocates a slot for a key.
+     *
+     * The core calls this function to determine a slot number, then


Why is this documentation here, not next to psa_drv_se_allocate_key_t?

I'm not sure how to structure the documentation between the type and the function pointer. I think it makes more sense to document in the documentation of the type, which is where the parameters and return type are spelled out. But I don't think it makes sense to document the context in which the function is called there, and especially not information like “this function is called after that function” — from a type, it doesn't really make sense to reference a specific function rather than another type.

I'm open to suggestions to structure the documentation better.

I would propose leaving just short notes here, like Function that allocates a slot for a key., and provide all the documentation next to psa_drv_se_allocate_key_t. With the current structure, if someone wants to use this function, he/she has to read about it in both of these places, and that's confusing.

Add infrastructure for internal, external and dual-use flags, with a compile-time check (if static_assert is available) to ensure that the same numerical value doesn't get declared for two different purposes in crypto_struct.h (external or dual-use) and psa_crypto_core.h (internal).

Add a slot_number field to psa_key_attributes_t and getter/setter functions. Since slot numbers can have the value 0, indicate the presence of the field via a separate flag. In psa_get_key_attributes(), report the slot number if the key is in a secure element. When creating a key, for now, applications cannot choose a slot number. A subsequent commit will add this capability in the secure element HAL.

Test the behavior of the getter/setter functions. Test that psa_get_key_slot_number() reports a slot number for a key in a secure element, and doesn't report a slot number for a key that is not in a secure element. Test that psa_get_key_slot_number() reports the correct slot number for a key in a secure element.

check-names.sh rejects MBEDTLS_XXX identifiers that are not defined in a public header.

This didn't break anything now, but would have broken things once we start to add internal flags.

This function no longer modifies anything, so it doesn't actually allocate the slot. Now, it just returns the empty key slot, and it's up to the caller to cause the slot to be in use (or not).

Pave the way for allowing the application to choose the slot number in a secure element, rather than always letting the driver choose.

Allow the application to choose the slot number in a secure element, rather than always letting the driver choose. With this commit, any application may request any slot. In an implementation with isolation, it's up to the service to filter key creation requests and apply policies to limit which applications can request which slot.

gilles-peskine-arm · 2019-08-08T09:05:18Z

I've rebased on top of the target branch now that #197 is merged. The rebase shows a large diff because other things were also merged, including development There were no conflicts except for "Rename internal macro to pass check-names.sh" which had been done both in #201 and here and which I therefore skipped in the rebase. The previous version is in https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-se_driver-choose_key_slot_number-2.

AndrzejKurek

Documentation changes suggested.

gilles-peskine-arm added enhancement New feature or request 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. api-spec Issue or PR about the PSA specifications labels Aug 5, 2019

This was referenced Aug 5, 2019

Add slot number attribute #201

Merged

Secure element key registration #183

Merged

gilles-peskine-arm force-pushed the psa-se_driver-choose_key_slot_number branch from e425596 to fcfd4f3 Compare August 5, 2019 14:34

k-stachowiak self-requested a review August 7, 2019 12:36

k-stachowiak previously approved these changes Aug 7, 2019

View reviewed changes

tests/suites/test_suite_psa_crypto_se_driver_hal.function Show resolved Hide resolved

tests/suites/test_suite_psa_crypto_se_driver_hal.function Show resolved Hide resolved

AndrzejKurek reviewed Aug 7, 2019

View reviewed changes

gilles-peskine-arm added 12 commits August 8, 2019 10:58

Add missing guard around a union field

74f3352

Rename internal macro to pass check-names.sh

5a68056

check-names.sh rejects MBEDTLS_XXX identifiers that are not defined in a public header.

Fix erasure of external flags

013f547

This didn't break anything now, but would have broken things once we start to add internal flags.

Fix copypasta

094dac1

Rename psa_internal_allocate_key_slot to psa_get_empty_key_slot

edbed56

This function no longer modifies anything, so it doesn't actually allocate the slot. Now, it just returns the empty key slot, and it's up to the caller to cause the slot to be in use (or not).

Improve documentation of the allocate method

0a23322

Add validate_slot_number method to SE drivers

ae9964d

Pave the way for allowing the application to choose the slot number in a secure element, rather than always letting the driver choose.

Test restarting after creating a key in a specific slot

0a11044

gilles-peskine-arm dismissed k-stachowiak’s stale review via 0a11044 August 8, 2019 09:05

gilles-peskine-arm force-pushed the psa-se_driver-choose_key_slot_number branch from 54e9bd8 to 0a11044 Compare August 8, 2019 09:05

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

Clarify and expand the documentation of the allocate/create sequence

9d75202

gilles-peskine-arm dismissed k-stachowiak’s stale review via 9d75202 August 9, 2019 09:34

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

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 needs: work The pull request needs rework before it can be merged. labels Aug 9, 2019

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 b231d99 into ARMmbed:psa-api-1.0-beta Aug 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Let applications create a key in a specific secure element slot #202

Let applications create a key in a specific secure element slot #202

Uh oh!

gilles-peskine-arm commented Aug 5, 2019

Uh oh!

k-stachowiak left a comment

Uh oh!

Uh oh!

Uh oh!

AndrzejKurek Aug 7, 2019

Uh oh!

gilles-peskine-arm Aug 7, 2019

Uh oh!

gilles-peskine-arm Aug 9, 2019

Uh oh!

AndrzejKurek Aug 7, 2019

Uh oh!

gilles-peskine-arm Aug 7, 2019

Uh oh!

AndrzejKurek Aug 8, 2019

Uh oh!

gilles-peskine-arm commented Aug 8, 2019

Uh oh!

AndrzejKurek left a comment

Uh oh!

Uh oh!

Let applications create a key in a specific secure element slot #202

Let applications create a key in a specific secure element slot #202

Uh oh!

Conversation

gilles-peskine-arm commented Aug 5, 2019

Uh oh!

k-stachowiak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndrzejKurek Aug 7, 2019

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Aug 7, 2019

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Aug 8, 2019

Uh oh!

AndrzejKurek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!