Make number of key slots externally configurable #9

2019-01-08T10:51:24-06:00

NirSonnenschein commented

2019-01-08 10:51:24 -06:00

(Migrated from github.com)

changes to make number of key slots externally configurable

changed define name from PSA_KEY_SLOT_COUNT to MBEDTLS_PSA_KEY_SLOT_COUNT
moved define to crypto_extra.h and made it possible to externally override

depends on fixes from (and is rebased on) : https://github.com/ARMmbed/mbed-crypto/pull/8

changes to make number of key slots externally configurable changed define name from PSA_KEY_SLOT_COUNT to MBEDTLS_PSA_KEY_SLOT_COUNT moved define to crypto_extra.h and made it possible to externally override depends on fixes from (and is rebased on) : https://github.com/ARMmbed/mbed-crypto/pull/8

alekshex (Migrated from github.com) requested changes

2019-01-08 11:20:16 -06:00

include/psa/crypto_extra.h

					
				@ -121,6 +121,15 @@ psa_status_t mbedtls_psa_inject_entropy(const unsigned char *seed,

				                                        size_t seed_size);

#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

```suggestion #if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT) ```

not needed if above accepted

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

i understand this is the naming convention for external define.
but still mbedtls has nothing to do with psa key slots number.
can we not have the mbedtls prefix? have some other?
why not leave it PSA_KEY_SLOT_COUNT?

i understand this is the naming convention for external define. but still mbedtls has nothing to do with psa key slots number. can we not have the mbedtls prefix? have some other? why not leave it PSA_KEY_SLOT_COUNT?

NirSonnenschein (Migrated from github.com) reviewed

2019-01-08 16:41:39 -06:00

include/psa/crypto_extra.h

					
				@ -121,6 +121,15 @@ psa_status_t mbedtls_psa_inject_entropy(const unsigned char *seed,

				                                        size_t seed_size);

I was told the preffernce was for positive ifdef checks as opposed to negative ones.
I agree that the negative route is shorter. I don't mind changing if this doesn't present a problem.

I was told the preffernce was for positive ifdef checks as opposed to negative ones. I agree that the negative route is shorter. I don't mind changing if this doesn't present a problem.

see above

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

I asked @gilles-peskine-arm and @Patater on their inputs on this and this is the name @gilles-peskine-arm requested.

gilles-peskine-arm (Migrated from github.com) requested changes

2019-01-09 03:24:52 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

Unless there's a good reason not to put MBEDTLS_PSA_KEY_SLOT_COUNT in config.h, please do that.

Unless there's a good reason not to put `MBEDTLS_PSA_KEY_SLOT_COUNT` in `config.h`, please do that.

include/psa/crypto_extra.h

					
				@ -121,6 +121,15 @@ psa_status_t mbedtls_psa_inject_entropy(const unsigned char *seed,

				                                        size_t seed_size);

Please avoid #ifdef !xxx if there's an #else. If there's no #else, just write the condition you need. In all cases the preference is for the simplest way to write the code.

Please avoid `#ifdef !xxx` _if there's an `#else`_. If there's no `#else`, just write the condition you need. In all cases the preference is for the simplest way to write the code.

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

We currently have two prefixes: PSA_xxx and MBEDTLS_xxx (and the lowercase versions). To avoid confusion, for the time being, please reserve PSA_xxx or psa_xxx names in public headers for official parts of the PSA API, and use MBEDTLS_xxx or mbedtls_xxx for implementation-specific names. I expect that we'll change the conventions at some point, but not before we've finished splitting Mbed Crypto from Mbed TLS and released PSA Crypto as a specification separate from its reference implementation.

We currently have two prefixes: `PSA_xxx` and `MBEDTLS_xxx` (and the lowercase versions). To avoid confusion, for the time being, please reserve `PSA_xxx` or `psa_xxx` names in public headers for official parts of the PSA API, and use `MBEDTLS_xxx` or `mbedtls_xxx` for implementation-specific names. I expect that we'll change the conventions at some point, but not before we've finished splitting Mbed Crypto from Mbed TLS and released PSA Crypto as a specification separate from its reference implementation.

Since this is a build-time configuration option, I think it should be in include/mbedtls/config.h. See that file for other similar options (near the end, after the boolean options).

Since this is a build-time configuration option, I think it should be in `include/mbedtls/config.h`. See that file for other similar options (near the end, after the boolean options).

NirSonnenschein (Migrated from github.com) reviewed

2019-01-09 03:33:01 -06:00

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

Hi @gilles-peskine-arm,
regarding adding this flag to config.h, you wrote in the email chain:
"I'm not keen on adding it to mbedtls/config.h because it isn't necessarily something that we'll maintain backward compatibility with. "

This is the reason it was left out, I can add it now if you prefer

Hi @gilles-peskine-arm, regarding adding this flag to config.h, you wrote in the email chain: "I'm not keen on adding it to mbedtls/config.h because it isn't necessarily something that we'll maintain backward compatibility with. " This is the reason it was left out, I can add it now if you prefer

NirSonnenschein (Migrated from github.com) reviewed

2019-01-09 03:33:30 -06:00

include/psa/crypto_extra.h

					
				@ -121,6 +121,15 @@ psa_status_t mbedtls_psa_inject_entropy(const unsigned char *seed,

				                                        size_t seed_size);

great, thanks @gilles-peskine-arm, will fix this to the simpler version

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

2019-01-09 03:38:09 -06:00

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

Hmm, yes, that is a good point, we aren't going to promise backward compatibility for this, so it shouldn't be in config.h, or at least not until we revise (and document!) our backward compatibility rules. So please keep it in crypto_extra.h, and add a comment that states that this is an implementation detail that may change or be removed in future versions. By default we do promise to maintain backward compatibility with things defined in crypto_extra.h.

Hmm, yes, that is a good point, we aren't going to promise backward compatibility for this, so it shouldn't be in `config.h`, or at least not until we revise (and document!) our backward compatibility rules. So please keep it in `crypto_extra.h`, and add a comment that states that this is an implementation detail that may change or be removed in future versions. By default we _do_ promise to maintain backward compatibility with things defined in `crypto_extra.h`.

NirSonnenschein (Migrated from github.com) reviewed

2019-01-09 03:56:51 -06:00

include/psa/crypto_extra.h

					
				@ -121,6 +121,15 @@ psa_status_t mbedtls_psa_inject_entropy(const unsigned char *seed,

				                                        size_t seed_size);

fixed

NirSonnenschein (Migrated from github.com) reviewed

2019-01-09 03:56:58 -06:00

include/psa/crypto_extra.h

					
				@ -121,6 +121,15 @@ psa_status_t mbedtls_psa_inject_entropy(const unsigned char *seed,

				                                        size_t seed_size);

fixed

NirSonnenschein (Migrated from github.com) reviewed

2019-01-09 04:35:56 -06:00

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

sure, comment added

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

2019-01-09 04:47:29 -06:00

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

We do have precedents for defining options in config.h that we don't intend to preserve backward compatibility for: MBEDTLS_PSA_HAS_ITS_IO, MBEDTLS_PSA_CRYPTO_SPM, the PSA storage modules. We intend to preserve the functionality, but not necessarily the configuration interface.

@Patater What's your opinion on this?

We do have precedents for defining options in `config.h` that we don't intend to preserve backward compatibility for: `MBEDTLS_PSA_HAS_ITS_IO`, `MBEDTLS_PSA_CRYPTO_SPM`, the PSA storage modules. We intend to preserve the functionality, but not necessarily the configuration interface. @Patater What's your opinion on this?

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

2019-01-09 04:52:01 -06:00

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

More generally, in Mbed Crypto, we don't intend to preserve compatibility with MBEDTLS_xxx APIs in the long term. So I think my view of backward compatibility is behind the times and the configuration option should go in config.h.

More generally, in Mbed Crypto, we don't intend to preserve compatibility with `MBEDTLS_xxx` APIs in the long term. So I think my view of backward compatibility is behind the times and the configuration option should go in `config.h`.

NirSonnenschein (Migrated from github.com) reviewed

2019-01-10 05:39:35 -06:00

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

@gilles-peskine-arm and @Patater , what is the final ruling on this? (add to config.h or not)

alekshex (Migrated from github.com) reviewed

2019-01-10 09:11:00 -06:00

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

slot count is public define now, so it can be PSA_xxx ...

alekshex (Migrated from github.com) reviewed

2019-01-10 09:12:58 -06:00

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

how about default number of 16 or 20?
cc @gilles-peskine-arm

how about default number of 16 or 20? cc @gilles-peskine-arm

NirSonnenschein (Migrated from github.com) reviewed

2019-01-10 09:16:15 -06:00

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

I think @gilles-peskine-arm said that PSA_XXX is only for public PSA API and this is not as it is a configuration for MbedCrypto that other implementations do not currently have to support.

NirSonnenschein (Migrated from github.com) reviewed

2019-01-10 09:20:04 -06:00

include/psa/crypto_extra.h

					
				@ -124,0 +126,4 @@

				 * Note: this configuration flag is an implementation detail that may change

				 * or be removed in future versions */

				#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

				    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32

as was mentioned in the email discussion, we need to see some testing on real world use (e.g. mbed client usage) to decide on a new default value. So for now I've left the default as it is. If we can decide on a new value I will set it.

Patater commented

2019-09-04 02:49:46 -05:00

(Migrated from github.com)

Closing as stale. The check-names updates have been merged already. The benefit of moving the number of slots configuration option from one impelentation-specifc header to another is of questionable benefit. Better to move into config.h if we want this user configurable.