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

Implement PSA protected storage & restructure PSA storage implementation #9708

Merged
merged 6 commits into from Feb 21, 2019

Conversation

Projects
None yet
@davidsaada
Copy link
Contributor

commented Feb 13, 2019

Description

This PR's main purpose is the implementation of the PSA Protected Storage (PS) feature. As this implementation is almost identical to the one we already have in PSA's Internal Trusted Storage (ITS), it triggered a deeper change in the entire implementation of PSA storage.

So, changes included here are:

  • Move all PSA storage code under psa/storage directory
  • Create a global PSA error codes header, eliminating ITS specific ones (backed by a change in the PSA spec)
  • Create a common header file for PSA storage type definitions, eliminating ITS specific ones (backed by the same change in the PSA spec)
  • Create a common implementation for PS & ITS
  • Implement protected storage feature
  • Change ITS test to be common to PS as well
  • Modify affected PSA crypto code (temporary, for CI sake only)

Important notes:
- Changes in the mbed-crypto code, included in this PR, are temporary. Changes should eventually be taken from PR #55 in the mbed-crypto repo. They are included here for CI sake only. After both PRs are approved, they should be merged together.

  • This PR is based on the current PSA spec. There's an ongoing discussion regarding the way PSA get functions should behave that's still open. As we strive to push this PR for 5.12, we can't wait for this discussion to be resolved, but if it's resolved in a reasonable time frame, PR may change accordingly.

Tested with all PSA tests on K64F and FUTURE_SEQUANA boards.

Pull request type

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

Release Notes

  • Restructure PSA storage implementation (directory structure and common code)
  • Add the PSA protected storage feature

@davidsaada davidsaada force-pushed the davidsaada:david_protected_storage branch 2 times, most recently Feb 13, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Feb 13, 2019

@NirSonnenschein NirSonnenschein requested a review from ARMmbed/mbed-os-psa Feb 13, 2019

@sbutcher-arm

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

This has no impact on @ARMmbed/mbed-os-tls so please remove as approvers.

@0xc0170 0xc0170 removed the request for review from ARMmbed/mbed-os-tls Feb 13, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@davidsaada davidsaada force-pushed the davidsaada:david_protected_storage branch Feb 13, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

This depends on #9702 now in order to fix travis issues.

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

@davidsaada Can you look at #9653 and see what is the impact of the changes there on this PR (or the opposite, whichever is merged first)

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

This depends on #9702 now in order to fix travis issues.

@davidsaada Travis issues?

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

This depends on #9702 now in order to fix travis issues.

@davidsaada Travis issues?

Should have been more specific: It failed the psa-autogen travis check before depending on that PR.

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

@davidsaada Can you look at #9653 and see what is the impact of the changes there on this PR (or the opposite, whichever is merged first)

@mikisch81 The PRs are almost orthogonal. They only collide in its_init, where you added get_its_kvstore_instance, which can easily be merged (in either direction).

@davidsaada davidsaada force-pushed the davidsaada:david_protected_storage branch Feb 14, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

Rebased, no longer depends on #9702 .
In addition, split the change into two commits, where mbed-crypto files are separated, so they can easily be removed from change once brought from mbed-crypto repo.

@davidsaada davidsaada force-pushed the davidsaada:david_protected_storage branch Feb 18, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

Implement PSA protected storage & restructure PSA storage implementation - why was this squashed into one commit?

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

Implement PSA protected storage & restructure PSA storage implementation - why was this squashed into one commit?

These were never squashed - they were all part of the same commit from the beginning (the second commit only includes the mbed-crypto files, which should be discarded after CI is successful and before merge). This is also explained in the PR description: Most of the work includes the restructuring and turning the previous ITS code to become the common code. The Protected Storage is just a very thin layer on top of it.

@Patater

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

Hi @davidsaada

Could you please remove Temporary commit for mbed-crypto files. and instead cherry-pick Patater@3638862 and then Patater@5c5b6b4 into this PR? We can update Mbed Crypto with the necessary modifications though this PR, now that the changes have landed in Mbed Crypto.

@davidsaada davidsaada force-pushed the davidsaada:david_protected_storage branch to 3291fc3 Feb 18, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

Hi @davidsaada

Could you please remove Temporary commit for mbed-crypto files. and instead cherry-pick Patater@3638862 and then Patater@5c5b6b4 into this PR? We can update Mbed Crypto with the necessary modifications though this PR, now that the changes have landed in Mbed Crypto.

Done. Temporary commit removed in favour of these two.
Note to @ARMmbed/mbed-os-maintainers: This will now fail build until rebased tomorrow on top of expected changes coming from mbed-crypto.

@0xc0170 0xc0170 requested a review from AnotherButler Feb 21, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

@AnotherButler Review the docs please

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

CI started

Whoops. Stopped the accidental restart.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

CI started

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Sneaky sneaky.

The success results were for the build before the rebase.

@cmonr cmonr added needs: CI and removed needs: review labels Feb 21, 2019

@AnotherButler

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@0xc0170 Please feel free to test and merge without our review. We do not have the bandwidth to review this much Doxygen at this point in time.

@AnotherButler

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@davidsaada Should any of these classes be added to our published, rendered documentation?

@dannybenor

This comment has been minimized.

Copy link

commented Feb 21, 2019

@AnotherButler A pointer to the spec of protected storage (like in ITS) will be good enough

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

@ARMmbed/mbed-os-maintainers Was just told that I needed to add few more deprecated definitions for our PSA compliance tests. This should have no effect on current code (only used by the external code). Will add them after CI finishes, so please don't stop it (so I can see if there's other stuff I need to fix if it fails). If it succeeds, please don't perform the merge yet.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@davidsaada Thanks for the heads up. Will try to keep CI light today so that we can accelerate this through.

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 21, 2019

Test run: SUCCESS

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

davidsaada and others added some commits Feb 7, 2019

Implement PSA protected storage & restructure PSA storage implementation
- Move all PSA storage code under psa/storage directory
- Create a global PSA error codes header, eliminating ITS specific ones
- Create a common header file for PSA storage type definitions,
  eliminating ITS specific ones
- Create a common implementation for PS & ITS
- Implement protected storage feature
- Change ITS test to be common to PS as well

@davidsaada davidsaada force-pushed the davidsaada:david_protected_storage branch to 3c5c205 Feb 21, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Pushed an update including deprecated ITS error codes (squashed into main commit) following tests success. Please rerun CI.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 21, 2019

Test run: SUCCESS

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

@cmonr cmonr merged commit 870bd05 into ARMmbed:master Feb 21, 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-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+48 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-IAR 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 9996 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
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

@cmonr cmonr removed the needs: CI label Feb 21, 2019

@davidsaada davidsaada deleted the davidsaada:david_protected_storage branch Mar 16, 2019

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.