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

Update mbedtls 2.18.0 rc1 #10675

Merged
merged 8 commits into from May 28, 2019

Conversation

Projects
None yet
8 participants
@0xc0170
Copy link
Member

commented May 27, 2019

Description

Same as #10642, one additional revert to fix CI issue.

cc @Patater

Pull request type

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

Reviewers

Release Notes

Patater and others added some commits May 10, 2019

mbedtls: Source crypto from Mbed Crypto
Use Mbed Crypto implementations of crypto. For example, use aes.c from
Mbed Crypto instead of the Mbed TLS copy.
mbedtls: Make imported version tag more verbose
When importing development releases of Mbed TLS into Mbed OS, it is
useful to be able to know a the particular git commit hash that was
imported. This change avoids ever creating a VERSION.txt for Mbed TLS
containing only "development", which is fairly useless since one doesn't
know where the development branch was at the time of import.
mbedtls: Fetch instead of pull
Do a git fetch of mbedtls instead of a pull. We don't need to checkout
development, only the release specified.
mbedtls: Update submodules after checkout
When importing, after checking out the specified release, update any
submodules present.
mbedtls: Update to Mbed TLS 2.18.0-rc1
Update Mbed TLS to 2.18.0-rc1. Update Mbed Crypto to 1.1.0d0.
Revert "Check mbed-crypto-example with fork"
This reverts commit ff18a64.
Use the official repository
@0xc0170

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

CI started

@Patater
Copy link
Contributor

left a comment

Ugh, what a confusing problem to track down. Nice find!

LGTM

@mbed-ci

This comment has been minimized.

Copy link

commented May 27, 2019

Test run: FAILED

Summary: 3 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-ARM
@0xc0170

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

@k-stachowiak is looking at the failures

@0xc0170 0xc0170 added needs: work and removed needs: CI labels May 27, 2019

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers May 27, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented May 27, 2019

@0xc0170 0xc0170 removed the needs: review label May 27, 2019

@0xc0170 0xc0170 removed request for ARMmbed/mbed-os-tls May 27, 2019

SPE: fix inject entropy macro
Use new TLS macro
@0xc0170

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

thanks to @orenc17 , found missing macro redefinition, locally the test passed. Restarting CI

@@ -1479,7 +1479,7 @@ static void psa_entropy_operation(void)
mbedtls_free(seed);
#else
status = PSA_ERROR_NOT_SUPPORTED;
#endif /* MBEDTLS_ENTROPY_NV_SEED && MBEDTLS_PSA_HAS_ITS_IO*/
#endif /* MBEDTLS_PSA_INJECT_ENTROPY */

This comment has been minimized.

Copy link
@0xc0170

0xc0170 May 27, 2019

Author Member

I followed the change from this PR in another PSA file.

@Patater @k-stachowiak Is this correct?

This comment has been minimized.

Copy link
@0xc0170

0xc0170 May 27, 2019

Author Member

Matches a change in features/mbedtls/mbed-crypto/platform/TARGET_PSA/COMPONENT_PSA_SRV_IMPL/psa_crypto.c

This comment has been minimized.

Copy link
@k-stachowiak

k-stachowiak May 27, 2019

Contributor

It makes sense to align the macros guarding APIs use with the macros guarding its definition. I haven't been following the development of this feature, but to me, this change looks correct.

This comment has been minimized.

Copy link
@gilles-peskine-arm

gilles-peskine-arm May 27, 2019

I confirm that the compile-time guard for mbedtls_psa_inject_entropy has changed to MBEDTLS_PSA_INJECT_ENTROPY. I haven't reviewed the rest of the PR.

@k-stachowiak
Copy link
Contributor

left a comment

It may be advisable to wait for @Patater 's confirmation, but based on his approval up to this point, I approve the most recent commit, and therefore the PR.

@0xc0170 0xc0170 requested a review from Patater May 27, 2019

@@ -1,4 +1,4 @@
{
"name": "mbed-crypto",
"macros": ["MBEDTLS_PSA_HAS_ITS_IO"]
"macros": ["MBEDTLS_PSA_INJECT_ENTROPY"]

This comment has been minimized.

Copy link
@gilles-peskine-arm

gilles-peskine-arm May 27, 2019

To build Mbed Crypto with the entropy injection feature, you need:

  • Storage support in Mbed Crypto: MBEDTLS_PSA_CRYPTO_STORAGE_C must be enabled. This requirement already existed in Mbed OS 5.12 (Mbed Crypto 2.17).
  • ITS storage backend support in Mbed Crypto. In Mbed OS 5.12, this required MBEDTLS_PSA_HAS_ITS_IO. With Mbed Crypto 2.18, you don't need anything beyond MBEDTLS_PSA_CRYPTO_STORAGE_C, but you need to make sure that MBEDTLS_PSA_ITS_FILE_C is not enabled since Mbed OS comes with its own ITS storage implementation.
  • Entropy file support must be enabled, i.e. MBEDTLS_ENTROPY_NV_SEED must be defined. This requirement was already present before.
  • Default entropy sources must be disabled, i.e. MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES must be defined. This requirement was already present before.
  • MBEDTLS_PSA_INJECT_ENTROPY must be defined. Before, the feature was automatically enabled if both MBEDTLS_PSA_HAS_ITS_IO and MBEDTLS_ENTROPY_NV_SEED were enabled.

So I don't understand why it worked before. What caused MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES, MBEDTLS_ENTROPY_NV_SEED, and for that matter MBEDTLS_PSA_CRYPTO_STORAGE_C and ``MBEDTLS_PSA_CRYPTO_C` to be enabled? Any idea @k-stachowiak ?

This comment has been minimized.

Copy link
@gilles-peskine-arm

gilles-peskine-arm May 27, 2019

I found that MBEDTLS_PSA_CRYPTO_STORAGE_C is enabled conditionally in features/mbedtls/platform/inc/platform_mbed.h. In this file, #define MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C is now obsolete, but harmless.

MBEDTLS_PSA_INJECT_ENTROPY should not be enabled by default for Mbed Crypto. It should only be enabled on targets that use this feature. In 5.12, it was automatically enabled on targets where TARGET_PSA and MBEDTLS_ENTROPY_NV_SEED were both enabled. It would have failed if such a target didn't also define MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES, so I must still be missing something, because I can't see any place where this gets defined.

Now MBEDTLS_PSA_INJECT_ENTROPY needs to be enabled explicitly on the targets that want this feature. I don't know which targets want it. This has to be a subset of the targets where it works.

To make this more backward compatible with 5.12, it would be possible to enable the feature where it's supported by modifying the configuration at the end of features/mbedtls/platform/inc/platform_mbed.h:

/* Enable entropy injection if it is supported. */
#if defined(MBEDTLS_ENTROPY_NV_SEED) && \
    defined(MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES) && \
    defined(MBEDTLS_PSA_CRYPTO_STORAGE_C)
#define MBEDTLS_PSA_INJECT_ENTROPY
#endif

But I don't think this is desirable, because this feature adds a few hundred bytes of code and increases the attack surface and the risk of misconfiguration that could lead to bricking a device.

This comment has been minimized.

Copy link
@k-stachowiak

k-stachowiak May 27, 2019

Contributor

I don't know the history of the aforementioned options beyond the fact that I have seen them being switched on and off in the past. However, based on this

MBEDTLS_PSA_INJECT_ENTROPY should not be enabled by default for Mbed Crypto

which makes sense to me, why don't we just remove the macro we are now commenting on?
Right now I'm testing that approach locally.

This comment has been minimized.

Copy link
@k-stachowiak

k-stachowiak May 27, 2019

Contributor

It builds with the feature switched off, but as far as I know, we actually need it in. Unfortunately, I don't know a simple way of enabling it without causing the trouble other than doing it for particular targets, as Gilles has mentioned earlier.

This comment has been minimized.

Copy link
@0xc0170

0xc0170 May 27, 2019

Author Member

@alzix @orenc17 Any suggestions ?

This comment has been minimized.

Copy link
@Patater

Patater May 28, 2019

Contributor

I've come up with a short term fix in enabling the entropy injection API on PSA targets if the user or applications has chosen to enable NV Seed, only on PSA targets. This is compatible with Mbed OS 5.12.

Longer term, we need to implement entropy mixing in Mbed Crypto to support using both TRNG and NV Seed at the same time.

This comment has been minimized.

Copy link
@Patater

Patater May 28, 2019

Contributor

I've tested on NUCLEO_F429ZI, K64F, and CY8CKIT_062_WIFI_BT.

@0xc0170 0xc0170 added needs: work and removed needs: review labels May 28, 2019

@0xc0170

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

If any new development, let me know. I can grant you access to my fork to keep this up to date (currently @k-stachowiak has it can push).

@Patater Patater force-pushed the 0xc0170:update-mbedtls-2.18.0-rc1 branch from be696bb to 473b4ce May 28, 2019

@0xc0170

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

CI started

@Patater

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Rebased to remove enabling PSA Entropy Injection unconditionally. Added commits to remove stale configuration options and to conditionally enable MBEDTLS_PSA_INJECT_ENTROPY for PSA targets if MBEDTLS_ENTROPY_NV_SEED is set.

@Patater Patater force-pushed the 0xc0170:update-mbedtls-2.18.0-rc1 branch from 473b4ce to 84e0b12 May 28, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented May 28, 2019

Test run: FAILED

Summary: 3 of 11 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_greentea-test
  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

CI restarted

@Patater

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Rebased to re-add the ITS configuration option (by dropping c044804), which PAL was depending on. Mbed Crypto no longer uses that option, and it shouldn't be necessary to depend on that option even if PAL needs to use ITS: PAL can assume if the target is PSA, that ITS is available.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels May 28, 2019

@Patater Patater force-pushed the 0xc0170:update-mbedtls-2.18.0-rc1 branch from 84e0b12 to 948d437 May 28, 2019

@0xc0170

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented May 28, 2019

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 6
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
@mbed-ci

This comment has been minimized.

Copy link

commented May 28, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 7
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@Patater

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

CI was unable to get a license when building one of the files (ecdh.c) during the exporter test. This made it look like an error from Mbed Crypto at first glance, but we also see the following in the log, which caused the build of that file to fail.

Unable to connect to the license server. Check that ARMLMD_LICENSE_FILE is set correctly, and the license server is available.```
@cmonr

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

CI job restarted: jenkins-ci/mbed-os-ci_exporter

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels May 28, 2019

@0xc0170

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

Ready for integration

@0xc0170 0xc0170 merged commit 8fc2a3c into ARMmbed:master May 28, 2019

26 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-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
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 Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8530 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
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.