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 Service - keys access control #9638

Merged
merged 5 commits into from Feb 27, 2019

Conversation

Projects
None yet
8 participants
@itayzafrir
Copy link
Contributor

commented Feb 7, 2019

Description

Access control for crypto keys in crypto service.

This is in continuation of PR #9575 which ensured that only the key creator is allowed to open a key & obtain a handle to it.

This PR, validates that the psa_key_handle_t argument (passed in as an input argument to the crypto APIs) is associated with the calling partition, i.e. that the API caller is also the key owner/creator.

Please do not merge or run CI - needs preceding PR #9575

Reviewers, reviews should start from c9130428951d60d865826e431eb400d92223b163, previous commits are from #9575 and are being reviewed there.

Pull request type

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

Reviewers

@avolinski

@ciarmcom ciarmcom requested review from avolinski and ARMmbed/mbed-os-maintainers Feb 7, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

@itayzafrir, thank you for your changes.
@avolinski @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-maintainers please review.

Show resolved Hide resolved components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c Outdated
Show resolved Hide resolved components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c Outdated
@avolinski
Copy link
Contributor

left a comment

part 0.5

Show resolved Hide resolved ...nts/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_access_control.c Outdated
Show resolved Hide resolved ...nts/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_access_control.c

@itayzafrir itayzafrir changed the title WIP Crypto Service - keys access control Crypto Service - keys access control Feb 10, 2019

Show resolved Hide resolved ...nts/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_access_control.h Outdated
components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/crypto_types.h Outdated
@@ -0,0 +1,5 @@
#ifdef PSA_CRYPTO_SECURE

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 12, 2019

Member

license header files in each file

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Feb 13, 2019

Author Contributor

@0xc0170 these files were introduced in PR #9575, the commits relevant to this PR start from c913042. I've added the license in #9575 (3bc5f90), this branch will be rebased once 9575 goes through.

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

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

this branch will be rebased once #9575 goes through.

Just leaving a more visible note.

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

@0xc0170 your review comment seems to be fixed, please re-review.
@ARMmbed/mbed-os-crypto - this PR has been opened for a while, please review or comment on why you aren't going to.

Will be rereviewed once rebased

@itayzafrir itayzafrir force-pushed the itayzafrir:crypto-access-control branch Feb 20, 2019

@itayzafrir

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

@avolinski rebased on master, please take another look

#include "psa_crypto_core.h"
#include "crypto_platform.h"

void psa_crypto_access_control_init(void);

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 20, 2019

Member

these functions should contain documentation?

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Feb 20, 2019

Author Contributor

I don't think so, this is internal only. These could have been implemented as static functions in psa_crypto_partition.c...

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 20, 2019

Member

we could use \internal or using _internal suffix in the header or at least a comment in this header file.

Even internal functions should have documentation, hidden from public (using #if !defined(DOXYGEN_ONLY)). IF you look at some drivers headers like SPI.h

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Feb 20, 2019

Author Contributor

Added in 5ac58b84dfa06f1c6cd76293dd4f01a5511124a0

@itayzafrir

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

PSA spec suggests abort() should be called

@alzix it's been used like that repeatedly as of cf3fd85

#define PSA_CRYPTO_ACCESS_CONTROL_RESET() (memset(crypto_access_control_arr, 0, sizeof(crypto_access_control_arr)))
static inline void psa_crypto_access_control_reset()
{
memset(crypto_access_control_arr, 0, sizeof(crypto_access_control_arr));

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 21, 2019

Contributor

isn't it
psa_crypto_access_control_reset();?

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Feb 24, 2019

Author Contributor

The macro was removed in 6475bc65d818fc9ff4fc6c2c02de3d1b7ecde24a in favor of the static inline function.

@itayzafrir itayzafrir force-pushed the itayzafrir:crypto-access-control branch to 37cc257 Feb 24, 2019

@itayzafrir

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

Rebased on master

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

@0xc0170 @alzix @ARMmbed/mbed-os-crypto please review/re-review (rebased and comments addressed).

@cmonr cmonr added risk: R risk: A and removed risk: R labels Feb 25, 2019

@NirSonnenschein NirSonnenschein removed the request for review from ARMmbed/mbed-os-crypto Feb 26, 2019

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

removing @ARMmbed/mbed-os-crypto as a reviewer. if you would still like to review please do so ASAP

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@0xc0170 @alzix , please re-review after the rebase

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Please do not merge or run CI - needs preceding PR #9575

Merged, this should be ready for review and CI

@itayzafrir

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Merged, this should be ready for review and CI

Yes this is unblocked now

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 27, 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 : 1
Build artifacts

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@alzix Happy with the latest update? This is ready to land otherwise

@itayzafrir

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Not sure about the availability of @alzix, he might be OOO for a while...

@0xc0170 0xc0170 merged commit 7656891 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(-36 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 9308 cycles (-899 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.