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 to mbedtls 2.18.0rc3 #10802

Merged
merged 4 commits into from Jun 12, 2019

Conversation

@Patater
Copy link
Contributor

commented Jun 10, 2019

Description

Update Mbed TLS to 2.18.0 rc3 and make Mbed OS specific modifications to allow all Mbed TLS DRBGs to use Mbed Crypto's PSA Entropy Injection feature.

Fixes #10787
Fixes #10788

Pull request type

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

Reviewers

@teetak01

CC @JanneKiiskila

@Patater

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@ARMmbed/mbed-os-maintainers Should we close #10801 in favor of this? or is testing far enough along that we should merge #10801 first?

If we merge #10801 first, I'll rebase this PR on top.

@ciarmcom ciarmcom requested review from JanneKiiskila, teetak01 and ARMmbed/mbed-os-maintainers Jun 10, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Let's focus on this one. I've initially thought these 2 PRs are separate (was wondering why). I'll review and CI this one rather

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

what rc3 and rc4 are fixing, to get these details I need to go to mbedtls ?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Ci started

@teetak01
Copy link
Contributor

left a comment

Thanks @Patater

I verified that this fixes both the #10787 and #10788

@bulislaw
Copy link
Member

left a comment

Approved for rc3

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 11, 2019

Test run: SUCCESS

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

@Patater

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Please wait for crypto team review before merge.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Please wait for crypto team review before merge.

@Patater I would like to generate RC3 this afternoon so any chance we can prioritise this review (if not already happening) ?

mbedtls: PSA entropy is compatible with other entropy
When using Mbed Crypto's PSA Entropy Injection feature on Mbed OS, it is
not required to opt out of having entropy sources added to your entropy
contexts by default (via MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES).

As integrated in Mbed OS, MBEDTLS_PSA_INJECT_ENTROPY is compatible with
actual entropy sources. PSA entropy injection is implemented using the
standard Mbed TLS NV Seed feature, and is as compatible with other
entropy sources as the standard Mbed TLS NV Seed feature which does
support entropy mixing.

@Patater Patater force-pushed the Patater:update-to-mbedtls-2.18.0rc4 branch from 7419c3d to 1470d06 Jun 11, 2019

@Patater

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Force pushed to remove rc4 version tag, as this version does not exist. The changes made to allow PSA entropy to be compatible with other entropy sources are Mbed OS only changes.

@Patater Patater changed the title Update to mbedtls 2.18.0rc4 Update to mbedtls 2.18.0rc3 Jun 11, 2019

@yanesca
Copy link
Contributor

left a comment

LGTM

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Jun 11, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

CI started

@Patater Patater referenced this pull request Jun 11, 2019

Closed

psa: PSA entropy is compatible with other entropy #2685

1 of 1 task complete
@TeroJaasko

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

This brings in dozens of warnings for PSA related macro redefinitions. Testing with mbed-os-example-blinky, GCC and K64F.

Compile [ 14.5%]: pal_client_api_empty_intf.c
[Warning] pal_client_api_intf.h@35,0: "PSA_SUCCESS" redefined
Compile [ 14.6%]: pal_client_api_intf.c
Compile [ 14.7%]: DeviceKey.cpp
Compile [ 14.9%]: pal_internal_trusted_storage_intf.c
[Warning] pal_internal_trusted_storage_intf.c@45,9: 'psa_its_set' is deprecated: PS specific types should not be used [-Wdeprecated-declarations]
Compile [ 15.3%]: val_attestation.c
[Warning] client.h@40,0: "PSA_VERSION_NONE" redefined
<..>
Compile [ 33.3%]: asn1parse.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 33.5%]: aes.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 33.6%]: asn1write.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 33.7%]: psa_crypto.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 33.8%]: blowfish.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 33.9%]: camellia.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 34.0%]: base64.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 34.2%]: ccm.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 34.3%]: chacha20.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 34.4%]: chachapoly.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 34.5%]: cipher_wrap.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 34.6%]: cmac.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 34.7%]: cipher.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 34.9%]: bignum.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 35.0%]: des.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 35.1%]: dhm.c
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 35.2%]: ctr_drbg.c
<..>
Compile [ 70.9%]: EthernetInterface.cpp
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 71.0%]: InternetSocket.cpp
Compile [ 71.1%]: L3IPInterface.cpp
[Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
Compile [ 71.2%]: NetworkInterface.cpp
@Patater

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@TeroJaasko

I believe that's a pre-existing issue, tracked by internal reference https://jira.arm.com/browse/IOTCRYPT-783

@Patater

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@TeroJaasko I've added a commit to eliminate the warnings.

Would be nice to have Mbed OS build with -Werror...

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 11, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Test failure could be from the previous run

@Patater

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@adbridge

Yes, failures are from run 2. Run 3 is currently ongoing.

The run 2 failures appear to be assertion failures during filesystem tests on K66F. Have we seen that before?

mbedgt: test suite report:
| target       | platform_name | test suite                                                                   | result | elapsed_time (sec) | copy_method |
|--------------|---------------|------------------------------------------------------------------------------|--------|--------------------|-------------|
| K66F-GCC_ARM | K66F          | components-storage-blockdevice-component_sd-tests-filesystem-dirs            | OK     | 86.94              | default     |
| K66F-GCC_ARM | K66F          | components-storage-blockdevice-component_sd-tests-filesystem-files           | FAIL   | 76.97              | default     |
| K66F-GCC_ARM | K66F          | components-storage-blockdevice-component_sd-tests-filesystem-fopen           | OK     | 93.23              | default     |
| K66F-GCC_ARM | K66F          | components-storage-blockdevice-component_sd-tests-filesystem-parallel        | FAIL   | 77.57              | default     |
@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@adbridge

Yes, failures are from run 2. Run 3 is currently ongoing.

The run 2 failures appear to be assertion failures during filesystem tests on K66F. Have we seen that before?

We get a few 'random' ci failures due to boards in a bad state etc, so it may have been seen previously. Probably not something to worry about if run 3 passes ok.

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 11, 2019

Test run: FAILED

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

Failed test jobs:

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

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Test failed above but then CI posted build failures (nothing in the logs, not on VPN to verify the hooks). Restart?

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 12, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR

@Patater Patater force-pushed the Patater:update-to-mbedtls-2.18.0rc4 branch from 02ac141 to e61e92c Jun 12, 2019

@Patater

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Updated to to fix its test failures hopefully. Re-added the definition of MBEDTLS_PSA_CRYPTO_C for PSA SPE targets as it appears that some files (not sure which yet) used by PSA SPE targets are not including the mbedtls/config.h as they should.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

CI restarted

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

@Patater looks like a couple more valid failures

@gilles-peskine-arm
Copy link

left a comment

LGTM but I didn't review the configuration changes on each platform individually.

psa: Avoid re-definition of MBEDTLS_PSA_CRYPTO_C
Mbed TLS now enables PSA APIs by default on all targets. It's not
necessary to explicitly enable MBEDTLS_PSA_CRYPTO_C, as that can be
gotten from the Mbed TLS config.h.

However, many PSA targets depend on `-DMBEDTLS_PSA_CRYPTO_C` being
defined by the Mbed OS json configuration system and are not yet
properly including the Mbed TLS configuration; for these PSA targets,
warnings may remain until this issue is fixed.

Avoiding re-definition will eliminate warnings like the following, when
building mbed-os-example-blinky:

    Compile [ 14.5%]: pal_client_api_empty_intf.c
    [Warning] pal_client_api_intf.h@35,0: "PSA_SUCCESS" redefined
    Compile [ 14.6%]: pal_client_api_intf.c
    Compile [ 14.7%]: DeviceKey.cpp
    Compile [ 14.9%]: pal_internal_trusted_storage_intf.c
    [Warning] pal_internal_trusted_storage_intf.c@45,9: 'psa_its_set' is deprecated: PS specific types should not be used [-Wdeprecated-declarations]
    Compile [ 15.3%]: val_attestation.c
    [Warning] client.h@40,0: "PSA_VERSION_NONE" redefined
    <..>
    Compile [ 33.3%]: asn1parse.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 33.5%]: aes.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 33.6%]: asn1write.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 33.7%]: psa_crypto.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 33.8%]: blowfish.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 33.9%]: camellia.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 34.0%]: base64.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 34.2%]: ccm.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 34.3%]: chacha20.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 34.4%]: chachapoly.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 34.5%]: cipher_wrap.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 34.6%]: cmac.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 34.7%]: cipher.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 34.9%]: bignum.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 35.0%]: des.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 35.1%]: dhm.c
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 35.2%]: ctr_drbg.c
    <..>
    Compile [ 70.9%]: EthernetInterface.cpp
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 71.0%]: InternetSocket.cpp
    Compile [ 71.1%]: L3IPInterface.cpp
    [Warning] config.h@2838,0: "MBEDTLS_PSA_CRYPTO_C" redefined
    Compile [ 71.2%]: NetworkInterface.cpp

@Patater Patater force-pushed the Patater:update-to-mbedtls-2.18.0rc4 branch from e61e92c to 39ea40f Jun 12, 2019

@Patater

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Updated to re-add MBEDTLS_PSA_CRYPTO_C via targets.json, as many PSA targets, both SPE and NSPE, are depending on -DMBEDTLS_PSA_CRYPTO_C being defined by the Mbed OS configuration system instead of using the Mbed TLS config.h.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Ci started

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 12, 2019

Test run: FAILED

Summary: 4 of 7 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

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

This comment has been minimized.

Copy link

commented Jun 12, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 6
Build artifacts

@adbridge adbridge added ready for merge and removed needs: CI labels Jun 12, 2019

@adbridge adbridge merged commit 4ad71c4 into ARMmbed:master Jun 12, 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 RTOS ROM(-63941 bytes) RAM(-33358 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 Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8532 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.