Update Mbed OS with Mbed Crypto merged into Mbed TLS #12757

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.

Sign up for GitHub

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

Jump to bottom

Merged

0xc0170 merged 7 commits into ARMmbed:master from dgreen-arm:test-remerge

Apr 30, 2020

Contributor

dgreen-arm commented Apr 3, 2020 •

edited by 0xc0170

Loading

Summary of changes

Update Mbed TLS to a version that brings in Mbed Crypto remerged into Mbed TLS.
Remove Mbed Crypto.

Impact of changes

Migration actions required

Documentation

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

mergify bot added the do not merge label

ciarmcom requested review from a team

April 3, 2020 15:00

Member

ciarmcom commented Apr 3, 2020

@dgreen-arm, thank you for your changes.
@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-tls @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-test please review.

ciarmcom added the needs: review label

dgreen-arm force-pushed the test-remerge branch from 4a95abc to 354541c Compare

April 6, 2020 08:37

0xc0170 added needs: CI mergify skip and removed needs: review labels

Contributor

0xc0170 commented Apr 6, 2020

I set this to try first run CI, but due to the CI load, t his might get in about 2-3 days.

mbed-ci commented Apr 7, 2020

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests-lts
jenkins-ci/mbed-os-ci_build-GCC_ARM-lts
jenkins-ci/mbed-os-ci_build-ARM-lts

Contributor

0xc0170 commented Apr 7, 2020

CI lts not related to this PR, will be cleared and proper CI restarted after 5.15 PRs are all integrated

dgreen-arm force-pushed the test-remerge branch from 354541c to bc9b173 Compare

April 16, 2020 11:19

Contributor

0xc0170 commented Apr 16, 2020

CI restarted

mbed-ci commented Apr 16, 2020

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests
jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM

Patater previously requested changes

View reviewed changes

features/mbedtls/src/psa_crypto_core.h Outdated

		@@ -0,0 +1,175 @@
		/*
		* PSA crypto core internal interfaces

Contributor

Patater Apr 17, 2020

Any idea why this isn't being picked up as a move/rename by git? Was it actually not present in Mbed OS before? Or did we forget to remove the old copy? (in commit "Update Mbed TLS to latest version")

Contributor Author

dgreen-arm Apr 17, 2020

I think I might have made a mistake when updating the Makefile. There look to be two copies of this file (and the others you've pointed this out on) now, one in src and one in platform/COMPONENT_PSA_SRV_IMPL. Is there something I missed with the Makefile changes?

Contributor

Patater Apr 17, 2020

If the Makefile places this in a new location, you'll have to manually delete it using git. The Makefile won't run git to add or remove files.

Contributor Author

dgreen-arm Apr 21, 2020

I think I've worked it out, I had combined the Makefiles badly so I was duplicating all the PSA specific headers. There shouldn't be duplicated files now.

Contributor Author

dgreen-arm Apr 21, 2020

crypto_compat.h still seems to be deleting and remaking, rather than doing a move, not sure why. Maybe the file changed too much?

features/mbedtls/src/psa_crypto_invasive.h Outdated

		@@ -0,0 +1,79 @@
		/**
		* \file psa_crypto_invasive.h

Contributor

Patater Apr 17, 2020

Shouldn't we see this as a move/rename?

Contributor Author

dgreen-arm Apr 21, 2020

Fixed.

features/mbedtls/src/psa_crypto_its.h Outdated

		@@ -0,0 +1,144 @@
		/** \file psa_crypto_its.h

Contributor

Patater Apr 17, 2020

Shouldn't we see this as a move/rename?

Contributor Author

dgreen-arm Apr 21, 2020

Fixed.

features/mbedtls/src/psa_crypto_se.h Outdated

		@@ -0,0 +1,190 @@
		/*
		* PSA crypto support for secure element drivers

Contributor

Patater Apr 17, 2020

Shouldn't we see this as a move/rename?

Contributor Author

dgreen-arm Apr 21, 2020

Fixed.

features/mbedtls/src/psa_crypto_service_integration.h Outdated

+               *  This file is part of mbed TLS (https://tls.mbed.org)
+               */
+              #ifndef PSA_CRYPTO_SERVICE_INTEGRATION_H

Contributor

Patater Apr 17, 2020

Shouldn't we see this as a move/rename?

Contributor Author

dgreen-arm Apr 21, 2020

Fixed.

features/mbedtls/src/psa_crypto_slot_management.h Outdated

		@@ -0,0 +1,129 @@
		/*
		* PSA crypto layer on top of Mbed TLS crypto

Contributor

Patater Apr 17, 2020

Shouldn't we see this as a move/rename?

Contributor Author

dgreen-arm Apr 21, 2020

Fixed.

features/mbedtls/src/psa_crypto_storage.h Outdated

		@@ -0,0 +1,390 @@
		/**
		* \file psa_crypto_storage.h

Contributor

Patater Apr 17, 2020

Shouldn't we see this as a move/rename?

Contributor Author

dgreen-arm Apr 21, 2020

Fixed.

mergify bot added needs: work and removed needs: CI labels

dgreen-arm force-pushed the test-remerge branch from bc9b173 to 7aabfa6 Compare

April 21, 2020 13:26

mergify bot dismissed Patater’s stale review

April 21, 2020 13:26

Pull request has been modified.

Patater previously approved these changes

View reviewed changes

Contributor

Patater left a comment

LGTM

mergify bot added needs: CI and removed needs: work labels

Contributor

0xc0170 commented Apr 21, 2020

is this still WIP? I'll review shortly

Contributor

Patater commented Apr 21, 2020

It looks no longer WIP to me. @dgreen-arm Could you comment what testing you've done with this? Thanks.

Contributor Author

dgreen-arm commented Apr 21, 2020

I've tested it against the following:
TLS examples: https://jenkins-internal.mbed.com/job/mbed-os-example-pr-test-parametrized/24/
Crypto example: https://jenkins-internal.mbed.com/job/mbed-os-example-pr-test-parametrized/25/
Atecc608a example: https://jenkins-internal.mbed.com/job/mbed-os-example-pr-test-parametrized/26/

I've also run it in the Mbed TLS release job here: https://jenkins-internal.mbed.com/job/mbedtls-release-new/732/

dgreen-arm changed the title ~~WIP: Update Mbed OS with Mbed Crypto merged into Mbed TLS~~ Update Mbed OS with Mbed Crypto merged into Mbed TLS

0xc0170 added ready for merge and removed needs: review labels

mergify bot added the needs: work label

mergify bot commented Apr 30, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

0xc0170 removed the ready for merge label

Contributor

0xc0170 commented Apr 30, 2020

@dgreen-arm the latest PSA removal PR created conflicts here. Can you please rebase, we will restart tests

dgreen-arm added 7 commits

April 30, 2020 11:27


          Adjust Mbed TLS importer and config script

b91fe62

Mbed Crypto has been remerged back into Mbed TLS. Update the
Mbed TLS importer script with the relevant parts of the
Mbed Crypto importer.

Signed-off-by: Darryl Green <darryl.green@arm.com>


          Update Mbed TLS to latest version

fc07e5b

As Mbed Crypto has been remerged into Mbed TLS, remove Mbed Crypto
at the same time.

Signed-off-by: Darryl Green <darryl.green@arm.com>


          Use updated ECC curve macros

53260eb

Signed-off-by: Darryl Green <darryl.green@arm.com>


          Move Mbed Crypto Greentea tests to Mbed TLS

d64c9e5

Signed-off-by: Darryl Green <darryl.green@arm.com>


          Remove references to mbed-crypto

0d16ca4

Signed-off-by: Darryl Green <darryl.green@arm.com>


          Define mbedtls_ecc_group_to_psa inline for NS world

1a6a849

Signed-off-by: Darryl Green <darryl.green@arm.com>


          Fix assertation ordering in tests

7fbc143

Signed-off-by: Darryl Green <darryl.green@arm.com>

dgreen-arm force-pushed the test-remerge branch from 50f5d77 to 7fbc143 Compare

April 30, 2020 10:29

mergify bot dismissed 0xc0170’s stale review

April 30, 2020 10:29

Pull request has been modified.

Contributor Author

dgreen-arm commented Apr 30, 2020

I've rebased the PR, conflicts should be resolved now.

0xc0170 added needs: CI and removed needs: work labels

Contributor

0xc0170 commented Apr 30, 2020

CI started

mbed-ci commented Apr 30, 2020

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

mergify bot added needs: work and removed needs: CI labels

0xc0170 added needs: CI and removed needs: work labels

0xc0170 approved these changes

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels

0xc0170 merged commit af4c8a9 into ARMmbed:master

mergify bot removed the ready for merge label

Patater mentioned this pull request

[Mbed OS 5.15] Upgrade to Mbed TLS 2.22.0 #13033

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-version: 6.0.0-beta-1