Make number of key slots externally configurable #9

NirSonnenschein · 2019-01-08T16:51:24Z

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) : #8

fixed processing of PSA macros in check names script. This required changes in: *list-macros.sh to scan the PSA headers *check-names to scan PSA files and allow PSA_* macro names

changes to make number of key slots externally configurable changed define name from PSA_KEY_SLOT_COUNT to MBEDTLS_PSA_KEY_SLOT_COUNT

alekshex · 2019-01-08T17:13:45Z

include/psa/crypto_extra.h

@@ -121,6 +121,14 @@ psa_status_t mbedtls_psa_inject_entropy(const unsigned char *seed,
                                        size_t seed_size);


+/* The Number of key slots (plus one because 0 is not used).
+ * The value is a compile-time constant for now, for simplicity. */
+#if defined(MBEDTLS_PSA_KEY_SLOT_COUNT)


Suggested change

#if defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

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.

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.

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

alekshex · 2019-01-08T17:14:03Z

include/psa/crypto_extra.h

+/* The Number of key slots (plus one because 0 is not used).
+ * The value is a compile-time constant for now, for simplicity. */
+#if defined(MBEDTLS_PSA_KEY_SLOT_COUNT)
+#else


not needed if above accepted

alekshex · 2019-01-08T17:17:03Z

include/psa/crypto_extra.h

+ * The value is a compile-time constant for now, for simplicity. */
+#if defined(MBEDTLS_PSA_KEY_SLOT_COUNT)
+#else
+    #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 asked @gilles-peskine-arm and @Patater on their inputs on this and this is the name @gilles-peskine-arm requested.

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.

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

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.

gilles-peskine-arm

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

gilles-peskine-arm · 2019-01-09T09:18:55Z

include/psa/crypto_extra.h

@@ -121,6 +121,14 @@ psa_status_t mbedtls_psa_inject_entropy(const unsigned char *seed,
                                        size_t seed_size);


+/* The Number of key slots (plus one because 0 is not used).
+ * The value is a compile-time constant for now, for simplicity. */
+#if defined(MBEDTLS_PSA_KEY_SLOT_COUNT)


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.

gilles-peskine-arm · 2019-01-09T09:22:02Z

include/psa/crypto_extra.h

+ * The value is a compile-time constant for now, for simplicity. */
+#if defined(MBEDTLS_PSA_KEY_SLOT_COUNT)
+#else
+    #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.

gilles-peskine-arm · 2019-01-09T09:23:43Z

include/psa/crypto_extra.h

+ * The value is a compile-time constant for now, for simplicity. */
+#if defined(MBEDTLS_PSA_KEY_SLOT_COUNT)
+#else
+    #define MBEDTLS_PSA_KEY_SLOT_COUNT 32


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).

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

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.

sure, comment added

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?

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.

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

alekshex · 2019-01-10T15:12:57Z

include/psa/crypto_extra.h

+ * 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

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 · 2019-09-04T07:49:46Z

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.

Nir Sonnenschein added 2 commits January 8, 2019 18:15

modify check-names.sh and list-macros.sh to work with PSA constants

03091d1

fixed processing of PSA macros in check names script. This required changes in: *list-macros.sh to scan the PSA headers *check-names to scan PSA files and allow PSA_* macro names

Make number of key slots externally configurable

c8ebadb

changes to make number of key slots externally configurable changed define name from PSA_KEY_SLOT_COUNT to MBEDTLS_PSA_KEY_SLOT_COUNT

alekshex suggested changes Jan 8, 2019

View reviewed changes

gilles-peskine-arm requested changes Jan 9, 2019

View reviewed changes

simplify ifdef

9c728e6

NirSonnenschein force-pushed the add_external_slot_count_config branch from b22d6cc to 9c728e6 Compare January 9, 2019 09:53

add comment regarding possible lack of future support for flag

5059c72

alekshex reviewed Jan 10, 2019

View reviewed changes

Patater closed this Sep 4, 2019

Patater added the stale This PR is not being actively worked on. It can be considered abandoned work. label Sep 4, 2019

	#if defined(MBEDTLS_PSA_KEY_SLOT_COUNT)
	#if !defined(MBEDTLS_PSA_KEY_SLOT_COUNT)

Make number of key slots externally configurable #9

Make number of key slots externally configurable #9

Uh oh!

Conversation

NirSonnenschein commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NirSonnenschein Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater commented Sep 4, 2019

Uh oh!

Uh oh!

NirSonnenschein commented Jan 8, 2019 •

edited

Loading

NirSonnenschein Jan 9, 2019 •

edited

Loading