Individual setters for persistent key attributes #109

2019-05-15T09:17:23-05:00

gilles-peskine-arm commented

2019-05-15 09:17:23 -05:00

(Migrated from github.com)

Use individual setters for the id and lifetime fields of an attribute structure, like the other attributes.

Patater (Migrated from github.com) reviewed

2019-05-15 09:56:40 -05:00

include/psa/crypto.h

					
				@ -229,2 +257,3 @@

				                                    psa_key_lifetime_t lifetime);

				static void psa_set_key_lifetime(psa_key_attributes_t *attributes,

				                                 psa_key_lifetime_t lifetime);

What should happen if the caller specifies id 0 here? Is this invalid? I think we need some more documentation here.

What should happen if the caller specifies `id` 0 here? Is this invalid? I think we need some more documentation here.

include/psa/crypto_struct.h

So, 0 is now officially an invalid id? What's the best place to document this?

include/psa/crypto_values.h

					
				@ -663,10 +663,8 @@

				 * Then you may create and use a key as follows:

Missing &attributes here?

- psa_set_key_algorithm(PSA_xxx_SIGNATURE(PSA_ALG_ANY_HASH));
+ psa_set_key_algorithm(&attributes, PSA_xxx_SIGNATURE(PSA_ALG_ANY_HASH));

Missing `&attributes` here? ```diff - psa_set_key_algorithm(PSA_xxx_SIGNATURE(PSA_ALG_ANY_HASH)); + psa_set_key_algorithm(&attributes, PSA_xxx_SIGNATURE(PSA_ALG_ANY_HASH)); ```

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-15 10:01:29 -05:00

include/psa/crypto_struct.h

Rework of https://github.com/ARMmbed/mbed-crypto/pull/104 coming up.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-15 10:04:47 -05:00

include/psa/crypto.h

					
				@ -229,2 +257,3 @@

				                                    psa_key_lifetime_t lifetime);

				static void psa_set_key_lifetime(psa_key_attributes_t *attributes,

				                                 psa_key_lifetime_t lifetime);

The functions have special cases to preserve the invariant that id = 0 if lifetime = VOLATILE. But id = 0 with lifetime ≠ VOLATILE is ok as the intermediate state if you call set_key_lifetime first, then set_key_id. Do you think there should be a special case for id=0?

The functions have special cases to preserve the invariant that `id = 0` if `lifetime = VOLATILE`. But `id = 0` with `lifetime ≠ VOLATILE` is ok as the intermediate state if you call `set_key_lifetime` first, then `set_key_id`. Do you think there should be a special case for `id=0`?

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-15 12:14:44 -05:00

include/psa/crypto_values.h

					
				@ -663,10 +663,8 @@

				 * Then you may create and use a key as follows:

I amended the corresponding commit.

Patater (Migrated from github.com) reviewed

2019-05-16 08:55:03 -05:00

include/psa/crypto.h

					
				@ -229,2 +257,3 @@

				                                    psa_key_lifetime_t lifetime);

				static void psa_set_key_lifetime(psa_key_attributes_t *attributes,

				                                 psa_key_lifetime_t lifetime);

id = 0 should be allowed as an intermediate state, and when the attributes are actually used, an error can be returned if the id is invalid.

`id = 0` should be allowed as an intermediate state, and when the attributes are actually used, an error can be returned if the id is invalid.

Patater (Migrated from github.com) reviewed

2019-05-16 08:58:42 -05:00

include/psa/crypto.h

					
				@ -203,30 +206,56 @@ psa_status_t psa_crypto_init(void);

				 */

- structure with given value.
+ structure with the given value.

or, better

- structure with given value.
+ structure with the given \p id.

```diff - structure with given value. + structure with the given value. ``` or, better ```diff - structure with given value. + structure with the given \p id. ```

- structure with given value.
+ structure with the given \p lifetime.

```diff - structure with given value. + structure with the given \p lifetime. ```

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-16 09:12:01 -05:00

include/psa/crypto.h

					
				@ -203,30 +206,56 @@ psa_status_t psa_crypto_init(void);

				 */

I don't think it's useful to specify what's going into the structure in this sentence. But I fixed the grammar and clarified a few things.

Patater (Migrated from github.com) approved these changes

2019-05-16 10:55:02 -05:00

Patater commented

2019-05-16 11:28:49 -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.

Sign in to join this conversation.

No reviewers

Patater