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

RollUp PR Crypto with ITS #9529

Merged
merged 35 commits into from Feb 4, 2019

Conversation

@orenc17
Copy link
Contributor

commented Jan 29, 2019

Description

This is a Roll up PR that includes the following PR's:

Pull request type

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

Reviewers

@Patater @alzix @itayzafrir @mikisch81 @ARMmbed/mbed-os-maintainers

Release notes

Crypto

Upon upgrading the version of Mbed Crypto within Mbed OS to Mbed Crypto 1.0.0d1, the following API changes in PSA Crypto are brought into Mbed OS. There is no backwards compatibility with the PSA Crypto alpha APIs as provided for preview purposes in Mbed OS 5.11. Users of the previous PSA Crypto APIs will need to update their code to use the new PSA Crypto APIs (which are still a moving target).

  • Simplify the ECC and RSA public key formats
  • Replace manual key slot allocation with dynamic key slot allocation and key handles (affects most PSA Crypto API functions)
  • Add and require initializers for PSA Crypto contexts

ITS

  • ITS change key id to uid (uint32_t -> psa_its_uid_t (uint64_t))

Patater and others added some commits Jan 22, 2019

tls/crypto: Make mbed-crypto importer independent
Obtain the version of Mbed Crypto to use not from the Mbed TLS
submodule, but independently through the Mbed Crypto importer instead.
crypto: Re-import Mbed Crypto 0.1.0b2
Use the Mbed-Crypto-specific importer script to re-import Mbed Crypto
0.1.0b2 to its new location.
crypto: importer: Update with fetch
Instead of doing a "pull --rebase" to update to the latest development
branch, do a "fetch" followed by a "checkout" to update to the specified
release. This enables us to get any new tags created since the last
update to the development branch, and removes the noise of updating a
local "development" branch.
crypto: Update tests to use key handles
Update tests in TESTS/mbed-crypto/sanity/main.cpp
crypto: Add test of key handles
Test key handles by adding a test to TESTS/mbed-crypto/sanity/main.cpp
crypto service: Implement IPC according to Mbed Crypto 1.0.0d1 API
1. Removed obsolete crypto APIs from IPC implementation.
2. Updated existing crypto APIs in IPC implementation.
3. Added new crypto APIs to IPC implemntation (except for psa_hash_clone).
crypto: Add test of hash clone
Test hash clone by adding a test to TESTS/mbed-crypto/sanity/main.cpp

@orenc17 orenc17 force-pushed the kfnta:its_crypto branch from 7f238e3 to b54e649 Jan 29, 2019

@ciarmcom ciarmcom requested review from alzix, itayzafrir, mikisch81, Patater and ARMmbed/mbed-os-maintainers Jan 29, 2019

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-crypto Jan 29, 2019

@alzix

alzix approved these changes Jan 29, 2019

Copy link
Contributor

left a comment

LGTM

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

running CI to screen for errors while this is reviewed.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

[X] World War 3

Please fix the PR template.

What's the intention with this rollup? Are all PR involved already approved?

@orenc17

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

@0xc0170 all 5 PR's are inter-dependent and must be run through CI together
This PR also solves all the merge conflicts
P.S. not all PR's are approved

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 29, 2019

Test run: FAILED

Summary: 4 of 8 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARMC6
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

So this is the review status:

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 29, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 removed the ready for merge label Feb 1, 2019

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

This PR is marked as breaking change, nobody even flinched... There's no description of what is being broken, what's the impact on users, how are they going to cope with that or anything else. I had a look at the "sub-PRs" and they don't have this info.

@bulislaw See this comment from @orenc17, the breaking change was in the relationship between psa-crypto and it's, that's why we needed a roll up PR in the first place.

@bulislaw

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

@mikisch81 @0xc0170 the breaking change in this PR is mainly the key type that has been chnaged but I don't know what does that mean, who is it affecting, what do they need to do to have their code working and what to put in release notes. We have policy of no breaking changes in Mbed OS, and we can grant exception when it's required, but it shouldn't be done lightly.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

@mikisch81 This req will go live soon ARMmbed/mbed-os-5-docs#933 (check how to describe functionality change), Can you add this description here?

This needs to describe every functional change in this PR (look at the docs PR, we need to understand the impact and how users would migrate their code). Why did we decide to break it ?

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

@orenc17 @itayzafrir @alzix can you address @bulislaw and @0xc0170 concerns.

@bridadan
Copy link
Contributor

left a comment

One tiny concern regarding compatibility with the online compiler, but it may only be limited to one target.

@@ -38,7 +38,7 @@
if '_NAME_' not in filename]
MANIFEST_FILE_PATTERN = '*_psa.json'
MBED_OS_ROOT = os.path.abspath(path_join(SCRIPT_DIR, os.pardir, os.pardir))
SPM_CORE_ROOT = path_join(MBED_OS_ROOT, 'components', 'TARGET_PSA', 'spm')
SPM_CORE_ROOT = path_join(MBED_OS_ROOT, 'components', 'TARGET_PSA', 'TARGET_MBED_SPM')

This comment has been minimized.

Copy link
@bridadan

bridadan Feb 1, 2019

Contributor

Any concerns with this @theotherjimmy? Thinking about the online compiler. However it may just be limited to the FUTURE_SEQUANA target.

This comment has been minimized.

Copy link
@cmonr

cmonr Feb 2, 2019

Contributor

@orenc17 Also, making sure this doesn't break things inadvertently.
@lrusinowicz thoughts?

(Should be fine, just being cautious)

This comment has been minimized.

Copy link
@orenc17

orenc17 Feb 2, 2019

Author Contributor

No, this tool should only be run by a developer changing PSA partitions

The online compiler doesn't and should never run this tool

@orenc17

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

@bulislaw @0xc0170 I think you are over reacting to a small type change.
Yes this a "breaking change" on an API that went from an architect's dream to a public release (@dreemkiller no offense plz)

Yes there are lots of changes in the only customer of this API (crypto), but I think you guys can handle and have handled many changes in crypo already just fine

Are there going to be more changes and PR's like this one ? Probably yes.

I think you should get a better understanding of PSA, as the comming release probably will hurt everyone like the first thing that come up on a Google search for PSA

As for the changes themselves, they have been checked thoroughly by everyone who has been asked for review on the matter, not to mention the next PR's that are already waiting for this one to be merged.

So I don't see any reason left not to merge this

All aboard the PSA train!!!!!, This ride is going to be bumpy as hell.

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

@ARMmbed/mbed-os-maintainers Can you please tell us exactly what is missing in this PR to be merged?
It is a very long running PR with a lot of pending PRs waiting for it already.
It was very complicated to get all the inter-dependencies between crypto solved, but @orenc17 and @itayzafrir managed to pull it together.
I was under the impression that @orenc17 answer to @0xc0170 regarding the breaking change of the its PR was Ok.
As this is something which only affect PSA targets (which currently there is only 1, and with the help of THIS PR there will be many more) I suggest merging it and open a new PR to handle your concerns.
There is not a lot of time left till 5.12 code freeze, and we don't want bulk of PR knocking in the door in the last minute.

@cmonr cmonr removed the request for review from ARMmbed/mbed-os-tls Feb 2, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

Hmm, it looks like a majority of reviews have been completed. I think we're looking for something like this to be ammended to the PR's description: ARMmbed/mbed-os-5-docs#933 (comment)

Either that, or I'm missing something else that hopefully @bulislaw can clarify.

I think you should get a better understanding of PSA, as the comming release probably will hurt everyone like the first thing that come up on a Google search for PSA

@orenc17 Joke unclear. I use duckduckgo 😄

@orenc17

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

release notes added

P.S. added a link to practical joke. @cmonr on duckduckgo its the 3rd result

@itayzafrir
Copy link
Contributor

left a comment

LGTM

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

@0xc0170 @bulislaw
Release note regarding breaking change in ITS was added, @ARMmbed/mbed-os-crypto can you please add the breaking changes that the new mbed-crypto added?

@Patater

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Background
Mbed Crypto is Arm's implementation of the PSA Crypto APIs. We shipped an implementation of a draft version of the PSA Crypto API in Mbed OS 5.11 as a preview. The PSA Crypto API has continued to develop and change over the past few months and a number of breaking changes have been made since the draft we based our Mbed Crypto implementation on.

What is being broken?
Upon upgrading the version of Mbed Crypto within Mbed OS to Mbed Crypto 1.0.0d1, the following API changes in PSA Crypto are brought into Mbed OS.

PSA Crypto API 1.0b1 API breaking changes, added to Mbed OS in this PR

  • Simplify the ECC and RSA public key formats
  • Replace manual key slot allocation with dynamic key slot allocation and key handles (affects most PSA Crypto API functions)
  • Add and require initializers for PSA Crypto contexts

Why is it being broken?
Mbed Crypto needs to track the upstream PSA Crypto API. The PSA Crypto API was not yet finalized in Mbed OS 5.11 and continues to evolve. We shipped an implementation in Mbed OS 5.11 to enable earlier integration with PSA APIs in Mbed OS.

Analysis of impact on users
There is no backwards compatibility with the PSA Crypto alpha APIs as provided for preview purposes in Mbed OS 5.11.

Alternatives
We could continue to provide the version of the PSA Crypto API shipped with Mbed OS 5.11, but to save flash size and reduce the maintenance burden of maintaining a former draft version of the PSA API, this alternative was rejected.

Mitigation and migration path for users
Users of the previous PSA Crypto APIs must continually update their code to use the new PSA Crypto APIs as the API evolves.

@orenc17

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

@bulislaw @ARMmbed/mbed-os-maintainers
Final release notes have been updated

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

The release notes 👍

@0xc0170

0xc0170 approved these changes Feb 4, 2019

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

@0xc0170 Is it ready for merge or are we waiting for @bulislaw approval oranother CI run?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

@bulislaw approval is needed.

@bulislaw
Copy link
Member

left a comment

Thanks guys for adding the explanations and release notes.

@bulislaw @0xc0170 I think you are over reacting to a small type change.
Yes this a "breaking change" on an API that went from an architect's dream to a public release (@dreemkiller no offense plz)

Yes there are lots of changes in the only customer of this API (crypto), but I think you guys can handle and have handled many changes in crypo already just fine

Are there going to be more changes and PR's like this one ? Probably yes.

I think you should get a better understanding of PSA, as the comming release probably will hurt everyone like the first thing that come up on a Google search for PSA

As for the changes themselves, they have been checked thoroughly by everyone who has been asked for review on the matter, not to mention the next PR's that are already waiting for this one to be merged.

So I don't see any reason left not to merge this

All aboard the PSA train!!!!!, This ride is going to be bumpy as hell.

The problem is that without knowing the details, maintainers and people not involved in this work can't make a meaningful decision. Even small breaking changes can be painful for the wider ecosystem. As a team we are rather trigger happy when it comes to breaking things. Rather than figuring things out before we start coding or later managing the change in meaningful way we just happily break things. I hope that explanation of what is being broken and why is not too big thing to ask for. If anything it should force the submitter to think again whether we really need to break something. And in some cases the answer is yes, but we shouldn't be jumping into conclusions without thinking or understanding why are we doing it and what impact will it have.

@orenc17

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

@0xc0170 Is it ready for merge or are we waiting for @bulislaw approval oranother CI run?

@bulislaw approval is needed.

@0xc0170 can we open those bottles of champagne?

@0xc0170 0xc0170 added ready for merge and removed needs: work labels Feb 4, 2019

@0xc0170 0xc0170 merged commit dcba5ff into ARMmbed:master Feb 4, 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(+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-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 9030 cycles (-202 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

@0xc0170 0xc0170 removed the ready for merge label Feb 4, 2019

@0xc0170 0xc0170 referenced this pull request Feb 4, 2019

Closed

PSA ITS update #9192

@AnotherButler

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Because this mentions breaking changes, I'd expect it to affect our documentation. Could someone please work with @Elise-Kaminski and me to update our docs?

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

@ARMmbed/mbed-os-psa Is someone already working with docs to get the above comment addressed?

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Because this mentions breaking changes, I'd expect it to affect our documentation. Could someone please work with @Elise-Kaminski and me to update our docs?

@ARMmbed/mbed-os-psa Is someone already working with docs to get the above comment addressed?

@orenc17 @Patater @Elise-Kaminski

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.