Skip to content
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.

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

Crypto IPC 64 bit key ids for ITS #9575

Merged
merged 4 commits into from Feb 27, 2019

Conversation

Projects
None yet
9 participants
@itayzafrir
Copy link
Contributor

commented Jan 31, 2019

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

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

@ciarmcom ciarmcom requested review from avolinski, Patater and ARMmbed/mbed-os-maintainers Jan 31, 2019

@itayzafrir itayzafrir force-pushed the itayzafrir:crypto-64-bit-key-ids branch 5 times, most recently Feb 1, 2019

@itayzafrir

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

Rebased on master, please take a look guys.

@itayzafrir itayzafrir force-pushed the itayzafrir:crypto-64-bit-key-ids branch Feb 5, 2019

Show resolved Hide resolved ...onents/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c Outdated
Show resolved Hide resolved ...onents/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c Outdated
Show resolved Hide resolved ...onents/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c Outdated
Show resolved Hide resolved ...onents/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c Outdated
Show resolved Hide resolved ...onents/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c Outdated
Show resolved Hide resolved ...onents/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c Outdated
Show resolved Hide resolved ...onents/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c Outdated
Show resolved Hide resolved ...onents/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c Outdated
Show resolved Hide resolved components/TARGET_PSA/services/crypto/COMPONENT_SPE/crypto_types_spe.h Outdated

@itayzafrir itayzafrir force-pushed the itayzafrir:crypto-64-bit-key-ids branch to d2947e0 Feb 5, 2019

@itayzafrir

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@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
Copy link
Contributor

left a comment

There are no tool changes in this PR

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

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

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 24, 2019

Test run: SUCCESS

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

This PR is waiting for reviewers to complete their reviews.

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

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

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.

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

itayzafrir added some commits Jan 30, 2019

itayzafrir
Define MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER
This enables crypto encoding an owner in key file IDs.
Added a static assert check in client side proxy.
itayzafrir
Remove duplicate inclusion of header files
Remove duplicate inclusion of psa/client.h and psa/service.h

@itayzafrir itayzafrir force-pushed the itayzafrir:crypto-64-bit-key-ids branch from 7d7cc46 to fc2b072 Feb 26, 2019

@itayzafrir

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

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

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

@Patater
Copy link
Contributor

left a comment

OK


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]);

This comment has been minimized.

Copy link
@Patater

Patater Feb 26, 2019

Contributor

Consider using PSA_KEY_FILE_GET_KEY_ID to get the key ID.

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Feb 26, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@Patater

Patater Feb 26, 2019

Contributor

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

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Feb 27, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@Patater

Patater Feb 27, 2019

Contributor

OK

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

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 27, 2019

Test run: SUCCESS

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

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

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Holding for merge.

@itayzafrir @Patater Is this comment resolved? #9575

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

@Patater

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Is this comment resolved?

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

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

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10126 cycles (+184 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.