gh-ARMmbed/mbed-crypto
Watch
1
0
Fork
You've already forked mbed-crypto
 0
Code Issues 36 Pull requests 1 Releases 7 Activity

Make key ids global and define their range #104

Merged
gilles-peskine-arm merged 7 commits from psa-global_key_id into psa-api-1.0-beta 2019-05-16 11:12:00 -05:00
Conversation 27 Commits 7 Files changed 11 +158 -92
gilles-peskine-arm commented 2019-05-06 12:04:32 -05:00 (Migrated from github.com)

Make key ids global: you no longer need to specify a key's lifetime when you open it, the key id is enough. This is the “global model” of key ids, as opposed to the former “drive letter” model. Internal ref: https://github.com/ARMmbed/psa-crypto/issues/138

Carve the range of possible key ids into 3 sub-ranges: one for the application, one for the implementation and one reserved for future use. Internal ref: https://github.com/ARMmbed/psa-crypto/issues/136

Make key ids global: you no longer need to specify a key's lifetime when you open it, the key id is enough. This is the “global model” of key ids, as opposed to the former “drive letter” model. Internal ref: https://github.com/ARMmbed/psa-crypto/issues/138 Carve the range of possible key ids into 3 sub-ranges: one for the application, one for the implementation and one reserved for future use. Internal ref: https://github.com/ARMmbed/psa-crypto/issues/136
athoelke (Migrated from github.com) reviewed 2019-05-07 05:19:53 -05:00
athoelke (Migrated from github.com) left a comment

The decision to permit key id zero is not discussed anywhere - not sure if this is deliberately to leave that decision to the application?

The decision to permit key id zero is not discussed anywhere - not sure if this is deliberately to leave that decision to the application?
include/psa/crypto_types.h
@ -85,10 +85,30 @@ typedef uint32_t psa_algorithm_t;
*/
athoelke (Migrated from github.com) commented 2019-05-07 05:10:41 -05:00

Suggest to remove the first "future" in the sentence.

Suggest to remove the first "future" in the sentence.
athoelke (Migrated from github.com) commented 2019-05-07 05:12:25 -05:00

I cannot recall - did we decide that key id of zero was valid or always invalid? - or if we should have a key id that is always invalid/null.

Or is this now an application decision whether or not to allocate a key with id of zero? - is this worth mentioning at all?

I cannot recall - did we decide that key id of zero was valid or always invalid? - or if we should have a key id that is always invalid/null. Or is this now an application decision whether or not to allocate a key with id of zero? - is this worth mentioning at all?
include/psa/crypto_values.h
@ -1440,6 +1440,19 @@
*/
athoelke (Migrated from github.com) commented 2019-05-07 05:15:08 -05:00

Needs a u suffix. Same for the following three constants

Needs a `u` suffix. Same for the following three constants
Patater (Migrated from github.com) reviewed 2019-05-07 11:11:24 -05:00
include/psa/crypto_values.h
@ -1443,0 +1445,4 @@
#define PSA_KEY_ID_USER_MIN ((psa_key_id_t)0x00000001)
/** The maximum value for a key identifier chosen by the application.
*/
#define PSA_KEY_ID_USER_MAX ((psa_key_id_t)0x3fffffff)
Patater (Migrated from github.com) commented 2019-05-07 11:11:17 -05:00

Are these reserved ranges implementation defined or part of PSA spec?

Are these reserved ranges implementation defined or part of PSA spec?
library/psa_crypto_slot_management.c
Patater (Migrated from github.com) commented 2019-05-07 11:01:36 -05:00

Would nobody ever open a key that doesn't have a #PSA_KEY_LIFETIME_PERSISTENT lifetime? Is open not used for volatile keys? Is this obvious enough from the function documentation?

Would anybody ever try this, (perhaps from separate threads or processes)?

  1. psa_create_key(PSA_KEY_LIFETIME_VOLATILE, someid, &handle1);
  2. psa_open_key(someid, &handle2);

Two issues:

  • They might not realize that the create does an implicit open (although this is documented already)
  • Key ownership and sharing implications potentially come into scope, in a service model. e.g. if we allow opening of a volatile key, do we need to block opening a volatile key that is already open? do we share the key somehow?
Would nobody ever open a key that doesn't have a `#PSA_KEY_LIFETIME_PERSISTENT` lifetime? Is open not used for volatile keys? Is this obvious enough from the function documentation? Would anybody ever try this, (perhaps from separate threads or processes)? 1. psa_create_key(PSA_KEY_LIFETIME_VOLATILE, someid, &handle1); 1. psa_open_key(someid, &handle2); Two issues: - They might not realize that the create does an implicit open (although this is documented already) - Key ownership and sharing implications potentially come into scope, in a service model. e.g. if we allow opening of a volatile key, do we need to block opening a volatile key that is already open? do we share the key somehow?
tests/suites/test_suite_psa_crypto_slot_management.function
@ -48,0 +55,4 @@
TEST_ASSERT( test_uses_key_id( key_id ) )
/* Destroy all key ids that may have been created by the current test case. */
static void psa_purge_key_storage( void )
Patater (Migrated from github.com) commented 2019-05-07 10:40:11 -05:00

What does it mean to return 1 here instead of 0? Isn't it a test error to use a key_id greater than PSA_MAX_PERSISTENT_KEY_IDENTIFIER? Or do we want all tests to look like they are using all keys outside this range?

What does it mean to return 1 here instead of 0? Isn't it a test error to use a key_id greater than PSA_MAX_PERSISTENT_KEY_IDENTIFIER? Or do we want all tests to look like they are using all keys outside this range?
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-07 17:57:42 -05:00
include/psa/crypto_types.h
@ -85,10 +85,30 @@ typedef uint32_t psa_algorithm_t;
*/
gilles-peskine-arm (Migrated from github.com) commented 2019-05-07 17:57:42 -05:00

I'm in two minds about id=0. On the one hand, allowing it violates the general principle that 0 is a “safe” invalid value. On the other hand, forbidding it means that you can't recognize a valid key id by a simple bit mask, and you can calculate a key id hash by just taking an n-bit hash.

I'd rather keep PSA_KEY_ID_VENDOR_MIN ≤ 1 to not gratuitously break existing code. Is 1..0x3fffffff a sensible range? Should it be 1..0x40000000? Or am I just imagining things and 1..0x3fffffff (or 1..0x7fffffff) is perfectly fine?

I'm in two minds about id=0. On the one hand, allowing it violates the general principle that 0 is a “safe” invalid value. On the other hand, forbidding it means that you can't recognize a valid key id by a simple bit mask, and you can calculate a key id hash by just taking an _n_-bit hash. I'd rather keep `PSA_KEY_ID_VENDOR_MIN ≤ 1` to not gratuitously break existing code. Is 1..0x3fffffff a sensible range? Should it be 1..0x40000000? Or am I just imagining things and 1..0x3fffffff (or 1..0x7fffffff) is perfectly fine?
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-07 17:59:07 -05:00
library/psa_crypto_slot_management.c
gilles-peskine-arm (Migrated from github.com) commented 2019-05-07 17:59:07 -05:00

Volatile keys don't have a key id. I think that's now clearly explained in the doc of psa_key_id_t. Maybe it needs to be explained elsewhere as well?

Since volatile keys don't have a key id, you can't open them.

Volatile keys don't have a key id. I think that's now clearly explained in the doc of `psa_key_id_t`. Maybe it needs to be explained elsewhere as well? Since volatile keys don't have a key id, you can't open them.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-07 17:59:37 -05:00
include/psa/crypto_values.h
@ -1443,0 +1445,4 @@
#define PSA_KEY_ID_USER_MIN ((psa_key_id_t)0x00000001)
/** The maximum value for a key identifier chosen by the application.
*/
#define PSA_KEY_ID_USER_MAX ((psa_key_id_t)0x3fffffff)
gilles-peskine-arm (Migrated from github.com) commented 2019-05-07 17:59:37 -05:00

Part of PSA spec. An application needs to know what key ids it can use, and so does an implementation.

Part of PSA spec. An application needs to know what key ids it can use, and so does an implementation.
Patater (Migrated from github.com) reviewed 2019-05-08 08:34:02 -05:00
library/psa_crypto_slot_management.c
Patater (Migrated from github.com) commented 2019-05-08 08:34:02 -05:00

This seems limiting. No way to share volatile keys?

This seems limiting. No way to share volatile keys?
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-08 15:39:24 -05:00
library/psa_crypto_slot_management.c
gilles-peskine-arm (Migrated from github.com) commented 2019-05-08 15:39:24 -05:00

What's the connection with sharing? Why would sharing require a persistent id?

What's the current API for key sharing ? We discussed proposals in https://github.com/ARMmbed/psa-crypto/issues/72 , but that was done by the services team, and I can't find any design document now.

What's the connection with sharing? Why would sharing require a persistent id? What's the current API for key sharing ? We discussed proposals in https://github.com/ARMmbed/psa-crypto/issues/72 , but that was done by the services team, and I can't find any design document now.
athoelke (Migrated from github.com) reviewed 2019-05-09 10:01:28 -05:00
include/psa/crypto_types.h
@ -85,10 +85,30 @@ typedef uint32_t psa_algorithm_t;
*/
athoelke (Migrated from github.com) commented 2019-05-09 10:01:27 -05:00

How many places does an implementation need to verify that a valid key id has been provided? - just during key creation and key opening it seems, so the need to check both ends of the range rather than just a one sided range isn't going to set off the code bloat alarm.

If we implement ARMmbed/psa-crypto#158 to unify handles and key ids then "zero is special" returns anyway to represent the NULL handle.

How many places does an implementation need to verify that a valid key id has been provided? - just during key creation and key opening it seems, so the need to check both ends of the range rather than just a one sided range isn't going to set off the code bloat alarm. If we implement ARMmbed/psa-crypto#158 to unify handles and key ids then "zero is special" returns anyway to represent the NULL handle.
Patater (Migrated from github.com) reviewed 2019-05-15 05:23:55 -05:00
library/psa_crypto_slot_management.c
Patater (Migrated from github.com) commented 2019-05-15 05:23:55 -05:00

How can you know you are opening the same volatile key without having a persistent ID? Do we prevent opening multiple handles to the same key?

How can you know you are opening the same volatile key without having a persistent ID? Do we prevent opening multiple handles to the same key?
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-15 06:38:13 -05:00
library/psa_crypto_slot_management.c
gilles-peskine-arm (Migrated from github.com) commented 2019-05-15 06:38:12 -05:00

You can't open a volatile key. Opening requires a persistent key id.

You can't open a volatile key. Opening requires a persistent key id.
Patater (Migrated from github.com) reviewed 2019-05-15 06:55:30 -05:00
library/psa_crypto_slot_management.c
Patater (Migrated from github.com) commented 2019-05-15 06:55:29 -05:00

Yes, understood. Because you can't open a volatile key, I'm highlighting potential difficulty in obtaining multiple handles to a volatile key. It's easy to get multiple handles to a persistent key; I don't believe we prevent obtaining multiple handles to a persistent key.

Why would sharing require a persistent id?

Because you can't open a volatile key.

Yes, understood. Because you can't open a volatile key, I'm highlighting potential difficulty in obtaining multiple handles to a volatile key. It's easy to get multiple handles to a persistent key; I don't believe we prevent obtaining multiple handles to a persistent key. > Why would sharing require a persistent id? Because you can't open a volatile key.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-15 07:14:52 -05:00
library/psa_crypto_slot_management.c
gilles-peskine-arm (Migrated from github.com) commented 2019-05-15 07:14:52 -05:00

There's no way to obtain multiple handles to the same volatile key. Sharing creates a separate handle which (at least conceptually) points to a copy of the key material.

There's no way to obtain multiple handles to the same volatile key. Sharing creates a separate handle which (at least conceptually) points to a copy of the key material.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-15 11:02:07 -05:00
include/psa/crypto_types.h
@ -85,10 +85,30 @@ typedef uint32_t psa_algorithm_t;
*/
gilles-peskine-arm (Migrated from github.com) commented 2019-05-15 11:02:06 -05:00

“Reserved for future use” is an idiom, so I'd rather keep it. Should I just stop the sentence there?

“Reserved for future use” is an idiom, so I'd rather keep it. Should I just stop the sentence there?
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-15 11:03:07 -05:00
include/psa/crypto_values.h
@ -1440,6 +1440,19 @@
*/
gilles-peskine-arm (Migrated from github.com) commented 2019-05-15 11:03:06 -05:00

Does this actually matter, given that the constants are being cast? If it does we need to change all the literals in the specification, not just these.

Does this actually matter, given that the constants are being cast? If it does we need to change all the literals in the specification, not just these.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-15 11:06:42 -05:00
tests/suites/test_suite_psa_crypto_slot_management.function
@ -48,0 +55,4 @@
TEST_ASSERT( test_uses_key_id( key_id ) )
/* Destroy all key ids that may have been created by the current test case. */
static void psa_purge_key_storage( void )
gilles-peskine-arm (Migrated from github.com) commented 2019-05-15 11:06:42 -05:00

It's an error to use such a key id, but not a test error: there's at least one test case that does it precisely to check that the implementation returns the expected error code. The cleanup code calls psa_destroy_persistent_key which is an internal function and may not enforce the limitation, so the code that sets up the information used by the cleanup code arranges not to call psa_destroy_persistent_key out of range.

It's an error to use such a key id, but not a _test error_: there's at least one test case that does it precisely to check that the implementation returns the expected error code. The cleanup code calls `psa_destroy_persistent_key` which is an internal function and may not enforce the limitation, so the code that sets up the information used by the cleanup code arranges not to call `psa_destroy_persistent_key` out of range.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-15 11:46:14 -05:00
include/psa/crypto_types.h
@ -85,10 +85,30 @@ typedef uint32_t psa_algorithm_t;
*/
gilles-peskine-arm (Migrated from github.com) commented 2019-05-15 11:46:13 -05:00

I decided that being consistent with “0 is invalid”, and protecting against the application forgetting to set a value that it really should have set (which is part of the reason for “0 is invalid”), are more important than making the permitted range exactly describable with a power of 2. So I've changed PSA_KEY_ID_VENDOR_MIN to 0.

I decided that being consistent with “0 is invalid”, and protecting against the application forgetting to set a value that it really should have set (which is part of the reason for “0 is invalid”), are more important than making the permitted range exactly describable with a power of 2. So I've changed `PSA_KEY_ID_VENDOR_MIN` to 0.
gilles-peskine-arm commented 2019-05-15 11:49:10 -05:00 (Migrated from github.com)

@athoelke @Patater The PR is now ready for re-review. I haven't addressed all your comments, but for some of them I'm waiting for clarifications since I'm not sure whether you're requesting a change.

@athoelke @Patater The PR is now ready for re-review. I haven't addressed all your comments, but for some of them I'm waiting for clarifications since I'm not sure whether you're requesting a change.
Patater (Migrated from github.com) reviewed 2019-05-16 08:19:15 -05:00
library/psa_crypto_slot_management.c
Patater (Migrated from github.com) commented 2019-05-16 08:19:15 -05:00

Thanks for answering my questions. Sharing is out of scope for this PR, so resolving this conversation.

Thanks for answering my questions. Sharing is out of scope for this PR, so resolving this conversation.
Patater (Migrated from github.com) reviewed 2019-05-16 08:21:49 -05:00
Patater (Migrated from github.com) left a comment
No description provided.
LGTM
AndrzejKurek (Migrated from github.com) reviewed 2019-05-16 08:24:54 -05:00
include/psa/crypto_values.h
@ -1440,6 +1440,19 @@
*/
AndrzejKurek (Migrated from github.com) commented 2019-05-16 08:24:54 -05:00

Maximal*

Maximal*
AndrzejKurek (Migrated from github.com) reviewed 2019-05-16 08:25:42 -05:00
include/psa/crypto_values.h
@ -1440,6 +1440,19 @@
*/
AndrzejKurek (Migrated from github.com) commented 2019-05-16 08:25:41 -05:00

Maximal*

Maximal*
Patater (Migrated from github.com) reviewed 2019-05-16 08:26:15 -05:00
include/psa/crypto_values.h
@ -1440,6 +1440,19 @@
*/
Patater (Migrated from github.com) commented 2019-05-16 08:26:15 -05:00

Maximum would be good. Minimal/Maximal; Minimum/Maximum.

Maximum would be good. Minimal/Maximal; Minimum/Maximum.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-16 08:28:02 -05:00
include/psa/crypto_values.h
@ -1440,6 +1440,19 @@
*/
gilles-peskine-arm (Migrated from github.com) commented 2019-05-16 08:28:02 -05:00

Thanks. Fixed, also application → implementation.

Thanks. Fixed, also application → implementation.
Patater (Migrated from github.com) approved these changes 2019-05-16 08:30:49 -05:00
Patater commented 2019-05-16 11:06:23 -05:00 (Migrated from github.com)

CI failure is ABI job (known to fail until https://github.com/ARMmbed/mbedtls/pull/2636 lands in the development branch) and USE_PSA_CRYPTO, which doesn't work on the API branch.

CI failure is ABI job (known to fail until https://github.com/ARMmbed/mbedtls/pull/2636 lands in the development branch) and `USE_PSA_CRYPTO`, which doesn't work on the API branch.
Patater (Migrated from github.com) reviewed 2019-05-16 11:08:55 -05:00
include/psa/crypto_types.h
@ -85,10 +85,30 @@ typedef uint32_t psa_algorithm_t;
*/
Patater (Migrated from github.com) commented 2019-05-16 11:08:55 -05:00

Repetition of "future" has been eliminated.

Repetition of "future" has been eliminated.
Patater (Migrated from github.com) reviewed 2019-05-16 11:11:50 -05:00
include/psa/crypto_values.h
@ -1440,6 +1440,19 @@
*/
Patater (Migrated from github.com) commented 2019-05-16 11:11:49 -05:00

Can be changed later (in a different PR), if deemed necessary.

Can be changed later (in a different PR), if deemed necessary.
Sign in to join this conversation.
Reviewers
No reviewers
Patater
Labels
Clear labels   
api-spec
Issue or PR about the PSA specifications

  
bug
Something isn't working

  
CLA requested

   
CLA valid

   
compliance
Discrepancy between the library and the PSA Crypto API specification

  
DO NOT MERGE
The PR is not intended to be merged (yet). Usually used for a review of worked in progress.

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
feature branch
This issue or PR is for a short-term feature branch which doesn't have its own label.

  
good first issue
Good for newcomers

  
HEADER FORMATTING ISSUE

   
help wanted
Extra attention is needed

  
Information

   
invalid
This doesn't seem right

  
Jira status: AWAITING REVIEW

   
Jira status: CLOSED

   
Jira status: DONE

   
Jira status: IN PROGRESS

   
Jira status: IN REVIEW

   
Jira status: OPEN

   
Jira status: REOPENED

   
Jira status: RESOLVED

   
mirrored

   
mirror internally

   
needs: backports
Needs backports to Mbed TLS branches

  
needs: ci
Needs a passing full CI run

  
needs: design review

   
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.

  
needs: work
The pull request needs rework before it can be merged.

  
notabug
The bug is not considered to be a defect, but by design.

  
open for community contribution

   
Product Backlog

   
question
Further information is requested

  
ready for merge
Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only.

  
security
Security expertise requested in review

  
stale
This PR is not being actively worked on. It can be considered abandoned work.

  
type: bug

   
type: enhancement

   
type: question

   
web pages
The /docs directory in the master branch is shown on https://armmbed.github.io/mbed-crypto

  
wontfix
The cost of fixing the bug or managing it in the bug list is not justified.

  
writing
Documentation wording and formatting

No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
Milestone
Clear milestone
No items
No milestone
api-1.0-beta-3
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-ARMmbed/mbed-crypto!104
No description provided.