DO NOT MERGE - Update Mbed TLS to the current development version of Mbed TLS #9779

k-stachowiak · 2019-02-20T16:01:23Z

Description

This is a placeholder PR for the next Mbed TLS feature update

Pull request type

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

Reviewers

@hanno-arm

Release Notes

ciarmcom · 2019-02-20T18:00:53Z

@k-stachowiak, thank you for your changes.
@hanno-arm @ARMmbed/mbed-os-tls @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2019-02-25T10:40:03Z

Note the failure in travis littlefs: ./features/mbedtls/inc/mbedtls/check_config.h:527:2: error: #error "MBEDTLS_PSA_CRYPTO_STORAGE_C defined, but not all prerequisites"

0xc0170 · 2019-02-25T10:41:39Z

If we can get that one fixed, we can run early CI job to get the test results

k-stachowiak · 2019-02-25T10:56:33Z

Copying @mpg

…lopment code

k-stachowiak · 2019-02-25T13:08:10Z

I have updated this PR with more recent Mbed TLS code and a configuration fix, however, I was not able to determine if we want to fix the config in this particular way, so I would leave that to be established in terms of a review.

mpg

Looks good to me. I think the way you fixed the config is appropriate.

mpg · 2019-02-26T08:52:27Z

@Patater @gilles-peskine-arm could one of you comment on the config question? And also have a look at the current CI failures as they seem to involve crypto.

From the travis-ci/pr log (similar error in one of the travis-ci/littlefs runs):

Compile [ 11.9%]: DeviceKey.cpp
[Fatal Error] crypto.h@2382,10: crypto_struct.h: No such file or directory
[ERROR] In file included from ./features/mbedtls/inc/mbedtls/pk.h:49:0,
                 from ./features/mbedtls/inc/mbedtls/ssl_ciphersuites.h:33,
                 from ./features/mbedtls/inc/mbedtls/ssl.h:36,
                 from ./features/mbedtls/inc/mbedtls/ssl_internal.h:33,
                 from ./features/device_key/source/DeviceKey.cpp:34:
./features/mbedtls/mbed-crypto/inc/psa/crypto.h:2382:10: fatal error: crypto_struct.h: No such file or directory
 #include "crypto_struct.h"
          ^~~~~~~~~~~~~~~~~
compilation terminated.
The command "python tools/make.py -t GCC_ARM -m K82F --source=. --build=BUILD/K82F/GCC_ARM -j0" exited with 1.

gilles-peskine-arm · 2019-02-26T09:23:20Z

@mpg crypto_struct.h has two versions depending on whether the library is built for use inside applications (non-PSA platform) or inside the crypto service (PSA platform). Looks like something is wrong with the importer or with the build scripts and neither version is caught. @NirSonnenschein and @Patater are more familiar with Mbed OS integration than I am.

0xc0170 · 2019-02-26T09:29:47Z

@davidsaada We already seen this error in another PR. isn't this a bug in crypto.h file? As litttlefs is building K82F GCC_ARM application.

davidsaada · 2019-02-26T09:37:52Z

@0xc0170 and all: Reason for this build failure is the fact that one can only include psa_crypto.h in PSA enabled boards. K82F is not one of these (like many others). This means that effectively we can't use PSA crypto in the near future before either enabling PSA for a great number of boards or having an additional layer on top of mbedtls/psa-crypto (just sharing a bit of our insights here, following a few internal discussions).

k-stachowiak · 2019-02-26T14:25:22Z

@Patater @mpg @gilles-peskine-arm I wonder if we can/should fix the issue in this PR. I can imagine selectively enabling and disabling PSA in particular targets' configuration, which could be part of the PR.

Patater · 2019-02-26T10:11:49Z

features/mbedtls/importer/adjust-config.sh

@@ -148,6 +148,8 @@ conf set MBEDTLS_MPI_MAX_SIZE     512

 # The following configurations are needed for Mbed Crypto.
 # They are related to the persistent key storage feature.
+conf set MBEDTLS_PSA_CRYPTO_C


We can't universally enable MBEDTLS_PSA_CRYPTO_C, as MBEDTLS_PSA_CRYPTO_C depends on an entropy source and not all boards have one. We need to leave it up to software that uses PSA Crypto to enable it for itself, like e.g. DeviceKey.cpp.

Boards that don't have entropy sources should use config-no-entropy.h shouldn't they? If so, then maybe everything that uses config.h does have an entropy source and we are OK with this change.

I don't see a reason we couldn't turn on MBEDTLS_PSA_CRYPTO_C universally for boards with entropy sources, but that's a big change to land so late in 5.12 development.

Ok, so if we back up a little bit, I think the motivation for this was us observing that check_config.h didn't like that MBEDTLS_PSA_CRYPTO_STORAGE_C was enabled but some of its prerequisites (which include MBEDTLS_PSA_CRYPTO_C) were not.

So if I understand your comment correctly, it would probably be safer to go about it the other way round: keep MBEDTLS_PSA_CRYPTO_C disabled by default, but also disable by default everything that depends on it, to keep check_config.h happy. @Patater wdyt?

Yes, please keep MBEDTLS_PSA_CRYPTO_C (and things that depend on it) disabled by default. Specific PSA targets enable it in targets.json, and specific applications that can guarantee an entropy source enable it in mbed_app.json or a custom Mbed TLS configuration.

0xc0170 · 2019-02-28T10:49:43Z

@k-stachowiak What is the status here? Can this PR be finalized today?

k-stachowiak · 2019-02-28T12:56:04Z

@0xc0170 I'm afraid I cannot say for certain. Maybe @Patater or @sbutcher-arm can answer?

simonbutcher · 2019-03-01T09:07:46Z

Hey 0xc0170. We were hoping to ship last nigh, but at the last moment found a couple of issues that failed our CI. We're now fixing them and the plan is to make the release this morning.

0xc0170 · 2019-03-01T09:28:31Z

Thanks for the update. As soon as its' ready will go into CI after approvals

k-stachowiak · 2019-03-01T14:00:55Z

I have just pushed an update to his PR, but please note, it still is at a do not merge stage.

0xc0170 · 2019-03-01T14:07:07Z

I have just pushed an update to his PR, but please note, it still is at a do not merge stage.

If it passes Travis checks, we can schedule CI at least

hanno-becker

Looks good to me, thanks @k-stachowiak!

0xc0170 · 2019-03-01T16:11:44Z

CI started

adbridge · 2019-03-01T16:12:43Z

CI restarted

simonbutcher · 2019-03-01T16:42:13Z

features/mbedtls/VERSION.txt

@@ -1 +1 @@
-mbedtls-2.15.1


This must be development not development-proposed. That's just a transient branch used for testing. This must be changed.

simonbutcher · 2019-03-01T16:43:23Z

features/mbedtls/importer/Makefile

@@ -27,7 +27,7 @@
 #

 # Set the mbed TLS release to import (this can/should be edited before import)
-MBED_TLS_RELEASE ?= mbedtls-2.15.1


Again, wrong branch. May have been better to use a commit id then a branch.

mbed-ci · 2019-03-01T19:01:06Z

Test run: FAILED

Summary: 2 of 14 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_tools-test
jenkins-ci/mbed-os-ci_greentea-test

mbed-ci · 2019-03-01T19:37:42Z

Test run: FAILED

Summary: 3 of 9 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-ARMC5
jenkins-ci/mbed-os-ci_build-IAR8

0xc0170 · 2019-03-02T15:50:51Z

Tests needs to be restarted. The failure should not be on master anymore. cc @cmonr

simonbutcher · 2019-03-03T00:52:18Z

So we have a problem with this PR.

The problem starts with this commit which defines the storage API as being present on all targets.

I don't know why, but TLS/ @AndrzejKurek disabled storage by default in this commit, and above @Patater asked for MBEDTLS_PSA_CRYPTO_C to not be enabled universally.

So now TLS is designed to not assume storage is enabled, and has integrity checks for it, (which seems sensible as it won't be present everywhere), and the PSA Crypto API isn't enabled everywhere, and yet the storage options are being universally enabled on all platforms, even when MBEDTLS_PSA_CRYPTO_C isn’t defined.

This is a contradiction - you can’t have MBEDTLS_PSA_CRYPTO_C defined in targets.json on a per-target basis and then have all of those storage options set universally.

This PR appears to breaks TLS on ALL non-PSA platforms, because the storage options are set when PSA is absent.

This single issue is holding up this PR from being merged.

I propose we revert this commit, but that also means that all platforms that need storage will need to be re-enabled in targets.json.

cc: @Patater, @dannybenor, @0xc0170

0xc0170 · 2019-03-03T18:48:54Z

I propose we revert this commit, but that also means that all platforms that need storage will need to be re-enabled in targets.json.

@dannybenor @Patater Please advise and address this asap.

simonbutcher · 2019-03-04T00:35:31Z

I can't update this PR, so I've created a new one of #9920. That addresses the problem above, and the review comment I gave about the version in the Makefile.

I now propose this PR is closed in deference to #9920.

Concerning the issue of the storage config options - my solution in commit 21c6c90, has been to make the storage configurations - (setting
MBEDTLS_PSA_CRYPTO_STORAGE_C, MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C and unsetting
MBEDTLS_PSA_CRYPTO_STORAGE_FILE_C), dependent on the PSA label being defined for
the target.

Previously these symbols were always defined for all platforms which could cause problems for targets that don't yet support PSA.

This makes my Mbed TLS release work on all targets, and it solves my problem of how to get Mbed TLS merged quickly, but its not, in my view, the correct solution. If MBEDTLS_PSA_CRYPTO_C is going to be set per target, then these configurations should also be set per target, and shouldn't be set universally for a group of targets. It might work now, but it will break later as the set of PSA targets expands. Not every target that supports PSA will have storage.

cc: @Patater, @dannybenor, @itayzafrir, @0xc0170

0xc0170 · 2019-03-04T09:43:27Z

Closing as replacement is up.

Update Mbed TLS to the current development version of Mbed TLS

a8f81b7

cmonr added do not merge release-version: 5.12.0-rc1 labels Feb 20, 2019

ciarmcom requested review from hanno-becker and a team February 20, 2019 18:00

ciarmcom added the needs: review label Feb 20, 2019

0xc0170 added needs: work and removed needs: review labels Feb 25, 2019

k-stachowiak added 2 commits February 25, 2019 12:36

Update source to the current mbedtls/development

e35ec41

Adjust configuration to use PSA crypto with the current Mbed TLS deve…

7f30a02

…lopment code

cmonr added the risk: A label Feb 25, 2019

mpg approved these changes Feb 26, 2019

View reviewed changes

Patater reviewed Feb 26, 2019

View reviewed changes

0xc0170 added risk: R and removed risk: A labels Feb 28, 2019

Update Mbed TLS to the proposed release version

7d8ed4b

Update to the new release candidate

8fc0579

hanno-becker approved these changes Mar 1, 2019

View reviewed changes

simonbutcher suggested changes Mar 1, 2019

View reviewed changes

simonbutcher mentioned this pull request Mar 4, 2019

Update Mbed TLS for 5.12 release to Mbed TLS development HEAD #9920

Merged

0xc0170 closed this Mar 4, 2019

0xc0170 removed do not merge needs: work release-version: 5.12.0-rc1 risk: R labels Mar 4, 2019

DO NOT MERGE - Update Mbed TLS to the current development version of Mbed TLS #9779

DO NOT MERGE - Update Mbed TLS to the current development version of Mbed TLS #9779

Uh oh!

Conversation

k-stachowiak commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Feb 20, 2019

Uh oh!

0xc0170 commented Feb 25, 2019

Uh oh!

0xc0170 commented Feb 25, 2019

Uh oh!

k-stachowiak commented Feb 25, 2019

Uh oh!

k-stachowiak commented Feb 25, 2019

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

mpg commented Feb 26, 2019

Uh oh!

gilles-peskine-arm commented Feb 26, 2019

Uh oh!

0xc0170 commented Feb 26, 2019

Uh oh!

davidsaada commented Feb 26, 2019

Uh oh!

k-stachowiak commented Feb 26, 2019

Uh oh!

Patater Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

mpg Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Feb 28, 2019

Uh oh!

k-stachowiak commented Feb 28, 2019

Uh oh!

simonbutcher commented Mar 1, 2019

Uh oh!

0xc0170 commented Mar 1, 2019

Uh oh!

k-stachowiak commented Mar 1, 2019

Uh oh!

0xc0170 commented Mar 1, 2019

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Mar 1, 2019

Uh oh!

adbridge commented Mar 1, 2019

Uh oh!

simonbutcher Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

simonbutcher Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

mbed-ci commented Mar 1, 2019

Test run: FAILED

Uh oh!

mbed-ci commented Mar 1, 2019

Test run: FAILED

Uh oh!

0xc0170 commented Mar 2, 2019

Uh oh!

simonbutcher commented Mar 3, 2019

Uh oh!

0xc0170 commented Mar 3, 2019

Uh oh!

k-stachowiak commented Feb 20, 2019 •

edited

Loading