Crypto IPC 64 bit key ids for ITS #9575

itayzafrir · 2019-01-31T15:26:45Z

Description

Support 64 bit crypto key ids for TARGET_PSA systems.

Crypto type psa_key_id_t is defined differently between client and server, this PR is here to provide the key owner (i.e. the calling partition) for use by the crypto library.

This PR assures that a crypto key can be opened only by the key owner/creator, i.e. the partition which created the key in the first place.

Please do not merge or run CI - depends on ARMmbed/mbed-crypto#32 and also ARMmbed/mbed-crypto#59.
Once 32 and 59 have been merged to mbed-crypto this PR will need to be re-imported via crypto importer into Mbed OS.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@avolinski @Patater

ciarmcom · 2019-01-31T16:00:55Z

@itayzafrir, thank you for your changes.
@Patater @avolinski @ARMmbed/mbed-os-core @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-test @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-tls please review.

itayzafrir · 2019-02-05T08:47:52Z

Rebased on master, please take a look guys.

components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c

components/TARGET_PSA/services/crypto/COMPONENT_SPE/crypto_types_spe.h

itayzafrir · 2019-02-05T16:40:59Z

@avolinski I've rebased and addressed your comments so far. Old branch @ https://github.com/itayzafrir/mbed-os/tree/crypto-64-bit-key-ids-1

NirSonnenschein · 2019-02-06T13:14:38Z

@ARMmbed/mbed-os-core @ARMmbed/mbed-os-tls @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-test @ARMmbed/mbed-os-crypto please review or comment if you don't wish to review.

bridadan

There are no tool changes in this PR

NirSonnenschein · 2019-02-18T10:06:03Z

Second reminder: This PR has been open for 18 days, and has still not been reviewed.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-tls @ARMmbed/mbed-os-test @ARMmbed/mbed-os-crypto @Patater please review or comment if you don't wish to review.

cc: @0xc0170 @adbridge @cmonr

NirSonnenschein · 2019-02-24T13:22:53Z

@Patater @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-tls would you still like to review this PR (if so please do so) or can it go through.

mbed-ci · 2019-02-24T14:53:05Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

0xc0170 · 2019-02-25T09:04:25Z

This PR is waiting for reviewers to complete their reviews.

cmonr · 2019-02-25T17:25:43Z

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c

This will need a rebase.

@Patater @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-tls Please re-review.

This enables crypto encoding an owner in key file IDs. Added a static assert check in client side proxy.

Remove duplicate inclusion of psa/client.h and psa/service.h

itayzafrir · 2019-02-26T10:31:16Z

Rebased to resolve merge confilcts in components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c

Patater

OK

Patater · 2019-02-26T11:45:47Z

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c


-                    bytes_read = psa_read(msg.handle, 1, &id, msg.in_size[1]);
+                    bytes_read = psa_read(msg.handle, 1, &(id.key_id), msg.in_size[1]);


Consider using PSA_KEY_FILE_GET_KEY_ID to get the key ID.

That wouldn't work here. PSA_KEY_FILE_GET_KEY_ID returns the value of key_id, here we're reading from the incoming message into key_id.

&PSA_KEY_FILE_GET_KEY_ID(id) wouldn't work?

The downside to how this is written now is it doesn't get the advantage of the macro abstraction and may be depending on more implementation detail than it needs to. This isn't a super strong downside now, but you should be aware of and consider it.

My bad, it would work. However I'd like to leave it as is for now, since there's no abstraction for the owner.

cmonr · 2019-02-26T21:54:19Z

CI started

mbed-ci · 2019-02-27T01:41:37Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 2
Build artifacts

cmonr · 2019-02-27T04:53:01Z

Holding for merge.

@itayzafrir @Patater Is this comment resolved? #9575

Patater · 2019-02-27T07:05:36Z

Is this comment resolved?

Yup, it's fine as-is: was just a suggestion of something to consider.

NirSonnenschein added needs: preceding PR do not merge labels Jan 31, 2019

itayzafrir changed the title ~~Crypto IPC 64 bit key ids~~ Crypto IPC 64 bit key ids for ITS Jan 31, 2019

ciarmcom requested review from alekshex, Patater and a team January 31, 2019 16:00

ciarmcom added the needs: review label Jan 31, 2019

itayzafrir force-pushed the crypto-64-bit-key-ids branch 5 times, most recently from ef3f6aa to 636086f Compare February 5, 2019 08:43

itayzafrir force-pushed the crypto-64-bit-key-ids branch from 636086f to 44df786 Compare February 5, 2019 08:51

alekshex suggested changes Feb 5, 2019

View reviewed changes

components/TARGET_PSA/services/crypto/COMPONENT_SPE/crypto_types_spe.h Outdated Show resolved Hide resolved

itayzafrir force-pushed the crypto-64-bit-key-ids branch from 44df786 to d2947e0 Compare February 5, 2019 16:39

alekshex approved these changes Feb 5, 2019

View reviewed changes

itayzafrir mentioned this pull request Feb 6, 2019

Prepare support for 64 bit key ids under SPM ARMmbed/mbed-crypto#32

Merged

bridadan reviewed Feb 6, 2019

View reviewed changes

itayzafrir mentioned this pull request Feb 7, 2019

Crypto Service - keys access control #9638

Merged

0xc0170 added the risk: G label Feb 25, 2019

cmonr added needs: work and removed needs: review labels Feb 25, 2019

itayzafrir added 4 commits February 26, 2019 12:27

crypto service: Remove unused IPC argument and unneeded casts

2b9b294

crypto service: Assemble bit crypto key ids

3992b83

Define MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER

ca94dc8

This enables crypto encoding an owner in key file IDs. Added a static assert check in client side proxy.

Remove duplicate inclusion of header files

fc2b072

Remove duplicate inclusion of psa/client.h and psa/service.h

itayzafrir force-pushed the crypto-64-bit-key-ids branch from 7d7cc46 to fc2b072 Compare February 26, 2019 10:30

0xc0170 added needs: review and removed needs: work labels Feb 26, 2019

Patater approved these changes Feb 26, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Feb 26, 2019

cmonr added ready for merge and removed needs: CI labels Feb 27, 2019

cmonr added needs: review and removed ready for merge labels Feb 27, 2019

NirSonnenschein added ready for merge and removed needs: review labels Feb 27, 2019

0xc0170 merged commit 5ab69d5 into ARMmbed:master Feb 27, 2019

0xc0170 removed the ready for merge label Feb 27, 2019


		bytes_read = psa_read(msg.handle, 1, &id, msg.in_size[1]);
		bytes_read = psa_read(msg.handle, 1, &(id.key_id), msg.in_size[1]);

Crypto IPC 64 bit key ids for ITS #9575

Crypto IPC 64 bit key ids for ITS #9575

Uh oh!

Conversation

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

Uh oh!

Description

Pull request type

Reviewers

Uh oh!

ciarmcom commented Jan 31, 2019

Uh oh!

itayzafrir commented Feb 5, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

itayzafrir commented Feb 5, 2019

Uh oh!

NirSonnenschein commented Feb 6, 2019

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

NirSonnenschein commented Feb 18, 2019

Uh oh!

NirSonnenschein commented Feb 24, 2019

Uh oh!

mbed-ci commented Feb 24, 2019

Test run: SUCCESS

Uh oh!

0xc0170 commented Feb 25, 2019

Uh oh!

cmonr commented Feb 25, 2019

Uh oh!

itayzafrir commented Feb 26, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Patater Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

itayzafrir Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Feb 26, 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!

itayzafrir Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

cmonr commented Feb 26, 2019

Uh oh!

mbed-ci commented Feb 27, 2019

Test run: SUCCESS

Uh oh!

cmonr commented Feb 27, 2019

Uh oh!

Patater commented Feb 27, 2019

Uh oh!

Uh oh!

itayzafrir commented Jan 31, 2019 •

edited

Loading

Patater Feb 26, 2019 •

edited

Loading