Support key file IDs encoding the key owner #59

gilles-peskine-arm · 2019-02-18T17:05:41Z

Add support for key file identifier that encode the key owner. Use the type psa_key_file_id_t to refer to key files, as opposed to psa_key_id_t which is the key identifier specified by the application. The two types are identical except when building the library for a multi-client service (currently: only the PSA crypto service, under the build option MBEDTLS_PSA_CRYPTO_SPM).

Because psa/crypto.h and psa/crypto_types.h are part of the source of the PSA Crypto API specification, use the standard types there. Define psa_key_file_id_t in psa/crypto_platform.h depending on whether MBEDTLS_PSA_CRYPTO_SPM is defined. We can make this cleaner by using psa_key_file_id_t in psa/crypto.h once the specification is separated from the implementation.

The file storage backend, and all of the tests, do not support MBEDTLS_PSA_CRYPTO_SPM. I have manually checked that the build with MBEDTLS_PSA_CRYPTO_SPM and MBEDTLS_PSA_STORAGE_ITS_C works with dummy psa/error.h and psa/internal_trusted_storage.h. Testing without a board would require updating and mergin https://github.com/ARMmbed/mbedtls-psa/pull/204 .

Patater · 2019-02-18T18:36:13Z

include/psa/crypto_platform.h

+{
+    int32_t owner;
+    uint32_t key_id;
+} psa_key_file_id_t;


Not super important, but maybe the compiler will generate slightly more efficient code if this struct has the same binary representation as the UID we send to ITS.

Indeed, putting the key id first saves a few instructions. From make CC=arm-linux-gnueabi-gcc CFLAGS='-Os -mthumb' (not exactly v7m but should be close enough):

-rw-rw-r-- 1 gilpes01 gilpes01 1948 Feb 18 20:43 library.spm/key_owner/psa_crypto_storage_its.o -rw-rw-r-- 1 gilpes01 gilpes01 1964 Feb 18 20:44 library.spm/owner_key/psa_crypto_storage_its.o

I'm not fluent enough in arm assembly to explain the difference without spending more time than I care to right now.

I don't think it's so Arm specific. If the binary representation is the same, the program doesn't have to swap the order around before passing the 64-bit value on to ITS and may be able to use the binary representation as-is, assuming alignment works out and so forth.

I put key_id first in the rebased version. It's likely to be neutral or a tiny improvement on any little-endian platform. How much it saves depends on the ABI and of course on what the compiler makes of the code.

Patater · 2019-02-18T18:39:30Z

library/psa_crypto_storage_its.c

+    /* Encode the owner in the upper 32 bits. This means that if
+     * owner values are nonzero (as they are on a PSA platform),
+     * key files don't clash with file names such as the random seed
+       file which have an identifier in the range 0..0xffffffff. */


Style: Add leasing * on this line.

Fixed in the new history.

Patater · 2019-02-18T18:45:04Z

library/psa_crypto_storage_its.c

+    /* Encode the owner in the upper 32 bits. This means that if
+     * owner values are nonzero (as they are on a PSA platform),
+     * key files don't clash with file names such as the random seed
+       file which have an identifier in the range 0..0xffffffff. */


"the random seed file which have an identifier" parsing error. This should be "has"? But, that doesn't seem to have the right meaning. I don't believe the random seed file has an identifier in the range 0 through 0xffffffff.

Suggest adding commas and "for" like so:
key files don't clash with file names, such as for the random seed file, which have an identifier in the range 0..0xffffffff.

Reworded in the new history.

Patater · 2019-02-18T19:10:39Z

library/psa_crypto_slot_management.c

@@ -236,7 +244,7 @@ static psa_status_t psa_internal_make_key_persistent( psa_key_handle_t handle,
 }

 static psa_status_t persistent_key_setup( psa_key_lifetime_t lifetime,
-                                          psa_key_id_t id,
+                                          psa_key_file_id_t id,


Is changing psa_key_id_t to psa_key_file_id_t everywhere really the best option?

If we keep this PR as-is and then do the following, we wouldn't require changing to psa_key_file_id_t everywhere. Would this be a workable improvement?

Remove typedef uint32_t psa_key_id_t; from crypto_types.h

Add typedef psa_app_key_id_t psa_key_id_t to crypto_platform.h for non-SPM platforms

Then we'd have the following situation:

Using psa_key_file_id_t as psa_key_id_t on SPM platforms (what we sort of have in this PR now, but don't use it)

Using psa_app_key_id_t as psa_key_id_t on non-SPM platforms

Maybe there is a reason to keep defining psa_key_id_t in crypto_types.h that I'm not seeing. In which case, we can keep that as-is and actually use that type definition instead of psa_key_file_id_t everywhere.

Currently, crypto_types.h and crypto.h are used to generate the specification, so they have to use the standard definitions. I wanted to minimize the incursion of MbedCrypto-specific stuff there.

Once the specification and the implementation are fully split (which should take a couple of weeks), I think we should define psa_key_file_id_t in crypto_types.h and use it in crypto.h.

itayzafrir

Hi @gilles-peskine-arm, please see my comment.

itayzafrir · 2019-02-19T09:53:53Z

include/psa/crypto_types.h

+ * used when building Mbed Crypto as a library inside the PSA Cryptography
+ * service. Applications that call the service see the standard API with a
+ * 32-bit type. */
+#if !defined(MBEDTLS_PSA_CRYPTO_SPM)


MBEDTLS_PSA_CRYPTO_SPM is defined for the entire secure side. This means that if another secure partition will try to communicate with crypto, this client will see psa_key_id_t as the full struct (i.e. psa_key_file_id_t) as not as uint32_t.

This could work if the ifdefs in the header files are tested for PSA_CRYPTO_SECURE instead of MBEDTLS_PSA_CRYPTO_SPM.

Ok, I'll change that.

gilles-peskine-arm · 2019-02-19T12:03:19Z

I'm going to update this PR with a new commit history that splits up the original large commit that both renames psa_key_id_t and adds owner-in-key-id support.

gilles-peskine-arm · 2019-02-19T12:04:37Z

include/psa/crypto_platform.h

@@ -63,7 +63,6 @@ typedef struct
 } psa_key_file_id_t;
 typedef psa_key_file_id_t psa_key_id_t;
 #define PSA_KEY_FILE_GET_KEY_ID( file_id ) ( ( file_id ).key_id )
-#endif


Stray leftover from a partial commit, I'll fix this in a rebase.

…g ITS" This reverts commit 7b6f37f. This revert is due to PR ARMmbed#59.

gilles-peskine-arm · 2019-02-19T13:45:41Z

I have pushed an updated version with a new, more granular history. The differences:

There is now a compilation option MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER to gate the nonstandard definition of psa_key_file_id_t. This makes the code easier to follow than depending only on PSA_CRYPTO_SECURE. The crypto service must enable this option.
Use PSA_CRYPTO_SECURE instead of MBEDTLS_PSA_CRYPTO_SPM to detect that the service is being built, as noted by Itay.
Put the key ID first in psa_key_file_id_t.
Some documentation improvements.

itayzafrir

LGTM, thanks.

gilles-peskine-arm · 2019-02-19T19:09:11Z

I made another, minor rebase to fix some issues reported by the CI:

An option in config.h must not have Doxygen formatting if it isn't enabled by scripts/config.pl all.
I'd forgotten to run generate_version_features.pl after adding an option to config.h.
Misplaced compilation guards caused an unused function in some configurations.

Previous history at https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-its-64_bit_internal_key_id-3

itayzafrir

LGTM

dgreen-arm

LGTM

Patater

I still am not a fan of changing psa_key_id_t to psa_key_file_id_t in so many places.

Let's revisit after the specification is split out from the implementation.

Patater · 2019-02-20T09:16:08Z

This needs CI to pass before we can merge it.

gilles-peskine-arm · 2019-02-20T09:54:49Z

I fixed a typo in “Support encoding an owner in key file IDs” which should let the CI do its job.

gilles-peskine-arm · 2019-02-20T09:56:22Z

I still am not a fan of changing psa_key_id_t to psa_key_file_id_t in so many places.

Neither am I, but I'm even less a fan of writing psa_key_id_t where we mean something that isn't a key id.

Let's revisit after the specification is split out from the implementation.

Actually, at that point, we'll change more occurrences of psa_key_id_t to psa_key_file_id_t, in crypto.h.

gilles-peskine-arm · 2019-02-20T11:41:24Z

I fixed another typo in “Support encoding an owner in key file IDs” which should let the CI do its job this time.

Patater · 2019-02-20T11:44:18Z

Minor style: Now the alignment is off, since key_id and file_id don't have the name number of characters.

library/psa_crypto_slot_management.c:192

gilles-peskine-arm · 2019-02-20T11:49:59Z

Since I've been rebasing anyway, I'll rebase again to fix the alignment.

PSA_MAX_PERSISTENT_KEY_IDENTIFIER was actually one plus the maximum key identifier. Change it to be the maximum value, and change the code that uses it accordingly. There is no semantic change here (the maximum value hasn't changed). This commit only makes the implementation clearer.

Differentiate between _key identifiers_, which are always `uint32_t`, and _key file identifiers_, which are platform-dependent. Normally, the two are the same. In `psa/crypto_platform.h`, define `psa_app_key_id_t` (which is always 32 bits, the standard key identifier type) and `psa_key_file_id_t` (which will be different in some service builds). A subsequent commit will introduce a platform where the two are different. It would make sense for the function declarations in `psa/crypto.h` to use `psa_key_file_id_t`. However this file is currently part of the PSA Crypto API specification, so it must stick to the standard type `psa_key_id_t`. Hence, as long as the specification and Mbed Crypto are not separate, use the implementation-specific file `psa/crypto_platform.h` to define `psa_key_id_t` as `psa_key_file_id_t`. In the library, systematically use `psa_key_file_id_t`. perl -i -pe 's/psa_key_id_t/psa_key_file_id_t/g' library/*.[hc]

Declare the owner as psa_key_owner_id_t, of which an implementation must be provided separately. Make this a configuration option MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER, to make the conditional compilation flow easier to follow. Declare it in config.h to pacify check_names.sh. Support for a specific implementation of psa_key_owner_id_t in storage backends will come in a subsequent commit.

When building for the PSA crypto service (defined(PSA_CRYPTO_SECURE)), define psa_key_owner_id_t as int32_t, which is how a PSA platform encodes partition identity. Note that this only takes effect when the build option MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER is active. Support this configuration in the ITS backend.

gilles-peskine-arm · 2019-02-20T11:53:42Z

Aligned.

Patater

OK

itayzafrir

LGTM

gilles-peskine-arm added enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. needs: design review labels Feb 18, 2019

gilles-peskine-arm requested review from Patater, dgreen-arm and itayzafrir February 18, 2019 17:05

Patater suggested changes Feb 18, 2019

View reviewed changes

itayzafrir suggested changes Feb 19, 2019

View reviewed changes

itayzafrir reviewed Feb 19, 2019

View reviewed changes

gilles-peskine-arm added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Feb 19, 2019

gilles-peskine-arm commented Feb 19, 2019

View reviewed changes

itayzafrir pushed a commit to itayzafrir/mbed-crypto that referenced this pull request Feb 19, 2019

Revert "Change PSA_MAX_PERSISTENT_KEY_IDENTIFIER to 64 bits when usin…

abf46c2

…g ITS" This reverts commit 7b6f37f. This revert is due to PR ARMmbed#59.

itayzafrir pushed a commit to itayzafrir/mbed-crypto that referenced this pull request Feb 19, 2019

Revert "Change PSA_MAX_PERSISTENT_KEY_IDENTIFIER to 64 bits when usin…

ff24f59

…g ITS" This reverts commit 7b6f37f. This revert is due to PR ARMmbed#59.

itayzafrir mentioned this pull request Feb 19, 2019

Crypto IPC 64 bit key ids for ITS ARMmbed/mbed-os#9575

Merged

gilles-peskine-arm force-pushed the psa-its-64_bit_internal_key_id branch 2 times, most recently from 9a01397 to bf7c8a3 Compare February 19, 2019 13:43

gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Feb 19, 2019

itayzafrir previously approved these changes Feb 19, 2019

View reviewed changes

gilles-peskine-arm removed the needs: design review label Feb 19, 2019

gilles-peskine-arm dismissed itayzafrir’s stale review via ec19596 February 19, 2019 19:07

gilles-peskine-arm force-pushed the psa-its-64_bit_internal_key_id branch from bf7c8a3 to ec19596 Compare February 19, 2019 19:07

itayzafrir previously approved these changes Feb 20, 2019

View reviewed changes

dgreen-arm previously approved these changes Feb 20, 2019

View reviewed changes

Patater previously approved these changes Feb 20, 2019

View reviewed changes

Patater added needs: ci Needs a passing full CI run and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Feb 20, 2019

gilles-peskine-arm dismissed stale reviews from Patater, dgreen-arm, and itayzafrir via eda4ffd February 20, 2019 09:54

gilles-peskine-arm force-pushed the psa-its-64_bit_internal_key_id branch from ec19596 to eda4ffd Compare February 20, 2019 09:54

gilles-peskine-arm mentioned this pull request Feb 20, 2019

Prepare support for 64 bit key ids under SPM #32

Merged

gilles-peskine-arm force-pushed the psa-its-64_bit_internal_key_id branch from eda4ffd to 8ee2669 Compare February 20, 2019 11:40

gilles-peskine-arm added 2 commits February 20, 2019 12:51

Move key id validity check into its own function

c8569bc

gilles-peskine-arm force-pushed the psa-its-64_bit_internal_key_id branch from 8ee2669 to 572f067 Compare February 20, 2019 11:51

gilles-peskine-arm added 3 commits February 20, 2019 12:52

Patater approved these changes Feb 20, 2019

View reviewed changes

itayzafrir approved these changes Feb 20, 2019

View reviewed changes

dgreen-arm approved these changes Feb 20, 2019

View reviewed changes

Patater merged commit 72f40c6 into ARMmbed:development Feb 20, 2019

gilles-peskine-arm mentioned this pull request Feb 20, 2019

Update the architecture document regarding key file identifiers #64

Merged

Support key file IDs encoding the key owner #59

Support key file IDs encoding the key owner #59

Uh oh!

Conversation

gilles-peskine-arm commented Feb 18, 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!

gilles-peskine-arm Feb 18, 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!

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!

itayzafrir 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!

gilles-peskine-arm commented Feb 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Feb 19, 2019

Uh oh!

itayzafrir left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itayzafrir left a comment

Choose a reason for hiding this comment

Uh oh!

dgreen-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Patater commented Feb 20, 2019

Uh oh!

gilles-peskine-arm commented Feb 20, 2019

Uh oh!

gilles-peskine-arm commented Feb 20, 2019

Uh oh!

gilles-peskine-arm commented Feb 20, 2019

Uh oh!

Patater commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm commented Feb 20, 2019

Uh oh!

gilles-peskine-arm commented Feb 20, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

itayzafrir left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Feb 18, 2019 •

edited

Loading

gilles-peskine-arm Feb 18, 2019 •

edited

Loading

gilles-peskine-arm commented Feb 19, 2019 •

edited

Loading

Patater commented Feb 20, 2019 •

edited

Loading