Support key file IDs encoding the key owner #59

2019-02-18T11:05:41-06:00

gilles-peskine-arm commented

2019-02-18 11:05:41 -06:00

(Migrated from github.com)

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 .

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 (Migrated from github.com) requested changes

2019-02-18 13:10:56 -06:00

include/psa/crypto_platform.h

					
				@ -52,0 +80,4 @@

				{

				    uint32_t key_id;

				    psa_key_owner_id_t owner;

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

library/psa_crypto_slot_management.c

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.

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.

library/psa_crypto_storage_its.c

					
				@ -37,12 +36,31 @@

				#include "mbedtls/platform.h"

Style: Add leasing * on this line.

Style: Add leasing `*` on this line.

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

"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.`

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

2019-02-18 13:31:20 -06:00

library/psa_crypto_slot_management.c

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.

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

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

2019-02-18 13:46:43 -06:00

include/psa/crypto_platform.h

					
				@ -52,0 +80,4 @@

				{

				    uint32_t key_id;

				    psa_key_owner_id_t owner;

				} psa_key_file_id_t;

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.

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.

Patater (Migrated from github.com) reviewed

2019-02-19 03:15:24 -06:00

include/psa/crypto_platform.h

					
				@ -52,0 +80,4 @@

				{

				    uint32_t key_id;

				    psa_key_owner_id_t owner;

				} psa_key_file_id_t;

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.

itayzafrir (Migrated from github.com) requested changes

2019-02-19 03:51:15 -06:00

itayzafrir (Migrated from github.com) left a comment

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

itayzafrir (Migrated from github.com) reviewed

2019-02-19 03:53:54 -06:00

include/psa/crypto_types.h

				`@ -90,7 +90,14 @@ typedef uint32_t psa_key_lifetime_t;`

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.

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

itayzafrir (Migrated from github.com) reviewed

2019-02-19 04:06:50 -06:00

include/psa/crypto_types.h

				`@ -90,7 +90,14 @@ typedef uint32_t psa_key_lifetime_t;`

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

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

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

2019-02-19 06:02:19 -06:00

include/psa/crypto_types.h

				`@ -90,7 +90,14 @@ typedef uint32_t psa_key_lifetime_t;`

Ok, I'll change that.

gilles-peskine-arm commented

2019-02-19 06:03:19 -06:00

(Migrated from github.com)

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.

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.

👍 1

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

2019-02-19 06:04:37 -06:00

include/psa/crypto_platform.h

					
				@ -52,0 +60,4 @@

				 * application, but also the the identity of the application.

				 *

				 * Note that this is an internal concept of the library and the remote

				 * call layer. The application itself never sees anything other than

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

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

2019-02-19 07:34:07 -06:00

library/psa_crypto_storage_its.c

					
				@ -37,12 +36,31 @@

				#include "mbedtls/platform.h"

Fixed in the new history.

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

2019-02-19 07:34:18 -06:00

library/psa_crypto_storage_its.c

					
				@ -37,12 +36,31 @@

				#include "mbedtls/platform.h"

Reworded in the new history.

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

2019-02-19 07:37:18 -06:00

include/psa/crypto_platform.h

					
				@ -52,0 +80,4 @@

				{

				    uint32_t key_id;

				    psa_key_owner_id_t owner;

				} psa_key_file_id_t;

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.

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.

gilles-peskine-arm commented

2019-02-19 07:45:41 -06:00

(Migrated from github.com)

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.

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 (Migrated from github.com) reviewed

2019-02-19 12:25:12 -06:00

itayzafrir (Migrated from github.com) left a comment

No description provided.

LGTM, thanks.

gilles-peskine-arm commented

2019-02-19 13:09:11 -06:00

(Migrated from github.com)

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

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 (Migrated from github.com) reviewed

2019-02-20 02:43:07 -06:00

itayzafrir (Migrated from github.com) left a comment

No description provided.

LGTM

dgreen-arm (Migrated from github.com) reviewed

2019-02-20 03:07:51 -06:00

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

No description provided.

LGTM

Patater (Migrated from github.com) reviewed

2019-02-20 03:12:11 -06:00

Patater (Migrated from github.com) left a comment

No description provided.

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 commented

2019-02-20 03:16:08 -06:00

(Migrated from github.com)

This needs CI to pass before we can merge it.

gilles-peskine-arm commented

2019-02-20 03:54:49 -06:00

(Migrated from github.com)

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

gilles-peskine-arm commented

2019-02-20 03:56:22 -06:00

(Migrated from github.com)

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.

> 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 commented

2019-02-20 05:41:24 -06:00

(Migrated from github.com)

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

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

Patater commented

2019-02-20 05:44:18 -06:00

(Migrated from github.com)

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

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 commented

2019-02-20 05:49:59 -06:00

(Migrated from github.com)