Skip to content

Make number of key slots externally configurable #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Make number of key slots externally configurable #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
03091d1
modify check-names.sh and list-macros.sh to work with PSA constants
Jan 8, 2019
c8ebadb
Make number of key slots externally configurable
Jan 6, 2019
9c728e6
simplify ifdef
Jan 9, 2019
5059c72
add comment regarding possible lack of future support for flag
Jan 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/psa/crypto_extra.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,15 @@ 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.
* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@NirSonnenschein NirSonnenschein Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, comment added

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif /* !defined(MBEDTLS_PSA_KEY_SLOT_COUNT) */


#ifdef __cplusplus
}
#endif
Expand Down
6 changes: 3 additions & 3 deletions library/psa_crypto_slot_management.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@

typedef struct
{
psa_key_slot_t key_slots[PSA_KEY_SLOT_COUNT];
psa_key_slot_t key_slots[MBEDTLS_PSA_KEY_SLOT_COUNT];
unsigned key_slots_initialized : 1;
} psa_global_data_t;

Expand Down Expand Up @@ -90,7 +90,7 @@ psa_status_t psa_initialize_key_slots( void )
void psa_wipe_all_key_slots( void )
{
psa_key_handle_t key;
for( key = 1; key <= PSA_KEY_SLOT_COUNT; key++ )
for( key = 1; key <= MBEDTLS_PSA_KEY_SLOT_COUNT; key++ )
{
psa_key_slot_t *slot = &global_data.key_slots[key - 1];
(void) psa_wipe_key_slot( slot );
Expand All @@ -108,7 +108,7 @@ void psa_wipe_all_key_slots( void )
*/
static psa_status_t psa_internal_allocate_key_slot( psa_key_handle_t *handle )
{
for( *handle = PSA_KEY_SLOT_COUNT; *handle != 0; --( *handle ) )
for( *handle = MBEDTLS_PSA_KEY_SLOT_COUNT; *handle != 0; --( *handle ) )
{
psa_key_slot_t *slot = &global_data.key_slots[*handle - 1];
if( ! slot->allocated )
Expand Down
4 changes: 0 additions & 4 deletions library/psa_crypto_slot_management.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@
#ifndef PSA_CRYPTO_SLOT_MANAGEMENT_H
#define PSA_CRYPTO_SLOT_MANAGEMENT_H

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

/** Access a key slot at the given handle.
*
* \param handle Key handle to query.
Expand Down
2 changes: 1 addition & 1 deletion library/psa_crypto_storage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ extern "C" {
* - Using the ITS backend, all key ids are ok except 0xFFFFFF52
* (#PSA_CRYPTO_ITS_RANDOM_SEED_UID) for which the file contains the
* device's random seed (if this feature is enabled).
* - Only key ids from 1 to #PSA_KEY_SLOT_COUNT are actually used.
* - Only key ids from 1 to #MBEDTLS_PSA_KEY_SLOT_COUNT are actually used.
*
* Since we need to preserve the random seed, avoid using that key slot.
* Reserve a whole range of key slots just in case something else comes up.
Expand Down
6 changes: 4 additions & 2 deletions tests/scripts/check-names.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ diff macros identifiers | sed -n -e 's/< //p' > actual-macros
for THING in actual-macros enum-consts; do
printf "Names of $THING: "
test -r $THING
BAD=$( grep -v '^MBEDTLS_[0-9A-Z_]*[0-9A-Z]$' $THING || true )
BAD=$( grep -E -v '^(MBEDTLS|PSA)_[0-9A-Z_]*[0-9A-Z]$' $THING || true )
if [ "x$BAD" = "x" ]; then
echo "PASS"
else
Expand All @@ -65,12 +65,14 @@ done

printf "Likely typos: "
sort -u actual-macros enum-consts > _caps
HEADERS=$( ls include/mbedtls/*.h | egrep -v 'compat-1\.3\.h' )
HEADERS=$( ls include/mbedtls/*.h include/psa/*.h | egrep -v 'compat-1\.3\.h' )
NL='
'
sed -n 's/MBED..._[A-Z0-9_]*/\'"$NL"'&\'"$NL"/gp \
$HEADERS library/*.c \
| grep MBEDTLS | sort -u > _MBEDTLS_XXX


TYPOS=$( diff _caps _MBEDTLS_XXX | sed -n 's/^> //p' \
| egrep -v 'XXX|__|_$|^MBEDTLS_.*CONFIG_FILE$' || true )
rm _MBEDTLS_XXX _caps
Expand Down
2 changes: 1 addition & 1 deletion tests/scripts/list-macros.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ if [ -d include/mbedtls ]; then :; else
exit 1
fi

HEADERS=$( ls include/mbedtls/*.h | egrep -v 'compat-1\.3\.h' )
HEADERS=$( ls include/mbedtls/*.h include/psa/*.h | egrep -v 'compat-1\.3\.h' )

sed -n -e 's/.*#define \([a-zA-Z0-9_]*\).*/\1/p' $HEADERS \
| egrep -v '^(asm|inline|EMIT|_CRT_SECURE_NO_DEPRECATE)$|^MULADDC_' \
Expand Down