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

Restore MBEDTLS_PSA_CRYPTO_C for PSA targets #9605

Merged
merged 1 commit into from Feb 12, 2019

Conversation

Projects
None yet
@alzix
Copy link
Contributor

commented Feb 4, 2019

Description

Enable PSA Crypto APIs on boards used by Pelion.
Removed by mistake in 763cb4c as part of #9195

Note: PSA Crypto APIs on other boards can still be enabled via mbed_app.json

Pull request type

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

Reviewers

@jenia81 @Patater

@ciarmcom ciarmcom requested review from jenia81, Patater and ARMmbed/mbed-os-maintainers Feb 4, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

@alzix, thank you for your changes.
@Patater @jenia81 @ARMmbed/mbed-os-maintainers please review.

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

seems that this already needs a rebase

@alzix alzix force-pushed the kfnta:alzix/pelion-psa branch to 911d138 Feb 5, 2019

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

rebased

@jenia81

jenia81 approved these changes Feb 5, 2019

@Patater

Patater approved these changes Feb 8, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Ci started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 11, 2019

Test run: SUCCESS

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

@cmonr cmonr merged commit 87a9d7c into ARMmbed:master Feb 12, 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 9227 cycles (-1241 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

@cmonr cmonr removed the ready for merge label Feb 12, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@alzix This is causing client to fail, can you please check? I'll share internal test url.

This is the error being reported from compilation

[Error] check_config.h@502,0: #35: #error directive: "MBEDTLS_PSA_CRYPTO_C defined, but not all prerequisites"

target NUCLEO_F411RE

cc @TeemuKultala

@shelib01

This comment has been minimized.

Copy link

commented Feb 12, 2019

@teetak01 , the version used in these tests includes integrated entropy injection feature ?

@teetak01

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

This is reproducable with public mbed-cloud-client-example. I believe this broke the non-TRNG configuration.

@Patater

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Hi @shelib01 and @teetak01

Mbed OS doesn't declare entropy as present in a device by default. How entropy ends up in a device shipped out is quite dependent on the device, how it's manufactured, and so forth. It'd be pretty dangerous for Mbed OS to claim that there is any injected entropy for devices without TRNGs when there is no entropy source present. Let's work together and have a look at the mbed-cloud-client-example configuration to ensure that for devices where client knows the device has entropy injected already, or plans to inject it, we have the right configuration. See https://github.com/ARMmbed/mbed-os-example-mbed-crypto/#factory-injection-of-entropy for an example mbed_app.json and further explanation.

Thanks

@Patater

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

I've raised ARMmbed/mbed-cloud-client-example#37 to resolve the build issues on the NUCLEO_F411RE for the mbed-cloud-client-example. Other examples or programs will need to make similar changes as desired, being careful to ensure that the feature is only enabled for devices that are manufactured properly.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Based on the feedback, we changed this to [X] Functionality change and 5.12 release.

@orenc17 will add Release notes

@orenc17

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Release notes

Adding PSA crypto support for the following single-v7 boards:

  • K64F
  • K66F
  • NUCLEO_F411RE
  • NUCLEO_F429ZI
  • UBLOX_EVK_ODIN_W2

Note: NUCLEO_F411RE does not have TRNG support and requires entropy injection through NVSeed

@alzix

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

@orenc17,
I would suggest rephrasing the last sentence in release notes.
Note: NUCLEO_F411RE does not have TRNG support and requires entropy injection by calling psa_inject... (I do not recall the exact API name - please fix). In case entropy was not injected psa_crypto_init() API will fail with xxx return code.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

#9710 should fix this and this shall not be functional change anymore as I understood from the conversation. Marking this for 5.11.5 and all fixes needed as well.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Moved to 5.12 due to failures with 5.11 branch

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.