Move mbedtls crypto modules #9340

ronald-cron-arm · 2024-07-02T16:04:12Z

Description

Repo split preparation, move Mbed TLS crypto modules to tf-psa-crypto.
Companion mbedtls-framework PR: Mbed-TLS/mbedtls-framework#32
Fix #9263

PR checklist

changelog not required, we will have one change log for the whole repo split work
3.6 backport [Backport 3.6] Backport of the framework submodule update in PR 9340 #9383
2.28 backport nothing to backport
tests no additional test required

library/.gitignore

tf-psa-crypto/drivers/builtin/CMakeLists.txt

tests/scripts/check-generated-files.sh

tests/scripts/all.sh

scripts/common.make

tests/scripts/check_files.py

ronald-cron-arm · 2024-07-04T09:57:05Z

@davidhorstmann-arm thanks for the very quick review. The CI was passing but I was still changing things locally when you pushed your review thus the force-pushed, sorry about that. The diff between 499a436 and f87e344 is not too big though. I hope it will be okay with you. I've added 3 commits to address some of your comments. One of your comment has been addressed as part of the rework I was doing.
I am checking the CI now and will rebase later to resolve the conflicts.

ronald-cron-arm · 2024-07-04T12:26:27Z

The OpenCI has run successfully, rebasing now.

ronald-cron-arm · 2024-07-04T12:31:28Z

Straightforward rebase, conflicts resolved automatically by Git.

davidhorstmann-arm

Rebase and changes LGTM, thanks!

davidhorstmann-arm · 2024-07-04T13:44:37Z

I expect this will need a few rebases before it gets merged, though. For example, it'll need rebasing after #9214 is merged.

ronald-cron-arm · 2024-07-04T13:49:50Z

I expect this will need a few rebases before it gets merged, though. For example, it'll need rebasing after #9214 is merged.

Yes that's likely.

ronald-cron-arm · 2024-07-08T08:31:48Z

@davidhorstmann-arm @tom-daubney-arm a lot of conflicts but the rebase is still straightforward thus I let it like that for the time being.

tom-daubney-arm

LGTM now, thanks! I will await the rebase.

Move all the modules that constitute the crypto library as well as the associated headers to tf-psa-crypto/core for the PSA core modules and to tf-psa-crypto/drivers/builtin/src for the others. The common.h file is copied instead of being just moved as eventually they will be different in mbedtls and TF-PSA-Crypto. Some parts of it can be shared though but this will be done later, probably when adding the CMake build system in tf-psa-crypto. Signed-off-by: Ronald Cron <[email protected]>

Adjust build systems such as we can built Mbed TLS in the default and full configuration. Signed-off-by: Ronald Cron <[email protected]>

Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm · 2024-07-10T06:22:35Z

@tom-daubney-arm @davidhorstmann-arm I've just rebased against the head of development. I had to resolve only two conflicts manually:

commit "Move crypto modules", the file library/ecp_internal_alt.h was moved by the commit but deleted by Remove asymmetric crypto alt interfaces #9271. Resolution: "git rm" of the file.
commit "Adapt check-generated-files.sh": framework conflict resolved by rebasing on top of main and the framework companion PR32 has been updated accordingly.

Let's see what the CI says now.

Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm · 2024-07-10T10:16:55Z

@tom-daubney-arm OpenCI passed and I have then updated the framework submodule following the merge of #32. It is ready for re-review I believe.

tom-daubney-arm

Rebase (and fw update) LGTM, thanks!

minosgalanakis

LGTM (Icremental review on post rebase changes)

ronald-cron-arm · 2024-07-10T13:57:01Z

LGTM (Icremental review on post rebase changes)

Thanks @minosgalanakis

ronald-cron-arm · 2024-07-10T13:58:22Z

@minosgalanakis please have a look to the very partial backport as well, thanks. #9383

minosgalanakis · 2024-07-10T14:52:56Z

@minosgalanakis please have a look to the very partial backport as well, thanks. #9383

Is that a backport or just a single commit mirroing 1992c9122f ?

ronald-cron-arm · 2024-07-10T14:55:19Z

@minosgalanakis please have a look to the very partial backport as well, thanks. #9383

Is that a backport or just a single commit mirroing 1992c9122f ?

It is the only thing to backport to me. All other changes in #9340 are migration of C modules and their consequences which are not relevant in 3.6.

gilles-peskine-arm · 2024-07-11T10:11:01Z

library/Makefile

+	rm -f $(OBJS_CRYPTO)
 	rm -f $(THIRDPARTY_CRYPTO_OBJECTS)
 else
 	if exist *.o del /Q /F *.o


On Windows, crypto objects in different directories aren't cleaned anymore. What's the plan here? Do nothing and wait until we abandon plain makefile support? A pending fix? I would like to have at least a comment explaining what's up.

I try to not break anything. I am going to fix that in the next PR.

Ok, thanks. I noticed because of a conflict with #9293, so your fix would conflict with that too.

gilles-peskine-arm · 2024-08-07T10:46:09Z

tf-psa-crypto/core/common.h

@@ -0,0 +1,435 @@
+/**


The common.h file is copied instead of
being just moved as eventually they
will be different in mbedtls and
TF-PSA-Crypto. Some parts of it can be
shared though but this will be done later,
probably when adding the CMake build
system in tf-psa-crypto.

sigh Why? Why isn't library/common.h just #include "core/common.h"? Or just omit library/common.h since we don't actually have anything to put there? Why are we spending effort on keeping two files identical? And how will that work when they're no longer in the same repository?

ronald-cron-arm added enhancement component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests labels Jul 2, 2024

ronald-cron-arm force-pushed the move-mbedtls-crypto-modules branch from d5598f0 to 499a436 Compare July 3, 2024 08:58

davidhorstmann-arm removed the needs-ci Needs to pass CI tests label Jul 3, 2024

davidhorstmann-arm requested changes Jul 3, 2024

View reviewed changes

ronald-cron-arm force-pushed the move-mbedtls-crypto-modules branch from 499a436 to 8cd4a40 Compare July 4, 2024 09:49

ronald-cron-arm force-pushed the move-mbedtls-crypto-modules branch from 8cd4a40 to 0eb5e9e Compare July 4, 2024 12:30

ronald-cron-arm added the needs-ci Needs to pass CI tests label Jul 4, 2024

davidhorstmann-arm previously approved these changes Jul 4, 2024

View reviewed changes

ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Jul 4, 2024

ronald-cron-arm requested a review from tom-daubney-arm July 4, 2024 14:35

tom-daubney-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jul 4, 2024

ronald-cron-arm added the needs-backports Backports are missing or are pending review and approval. label Jul 5, 2024

tom-daubney-arm previously approved these changes Jul 9, 2024

View reviewed changes

tom-daubney-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jul 9, 2024

ronald-cron-arm added 5 commits July 10, 2024 08:07

Adjust build systems

c29afb6

Adjust build systems such as we can built Mbed TLS in the default and full configuration. Signed-off-by: Ronald Cron <[email protected]>

Adapt make cscope

52cd108

Signed-off-by: Ronald Cron <[email protected]>

Adapt libtestdriver1 build

b2bdb78

Signed-off-by: Ronald Cron <[email protected]>

Adapt check_names.py

6921d54

Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm dismissed stale reviews from tom-daubney-arm and davidhorstmann-arm via 080ab4f July 10, 2024 06:13

ronald-cron-arm force-pushed the move-mbedtls-crypto-modules branch from 0eb5e9e to 080ab4f Compare July 10, 2024 06:13

ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, and removed approved Design and code approved - may be waiting for CI or backports labels Jul 10, 2024

ronald-cron-arm requested review from davidhorstmann-arm and tom-daubney-arm July 10, 2024 06:14

ronald-cron-arm added needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, labels Jul 10, 2024

Update the framework submodule to the merge of PR32

1992c91

Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Jul 10, 2024

tom-daubney-arm approved these changes Jul 10, 2024

View reviewed changes

minosgalanakis self-requested a review July 10, 2024 13:19

minosgalanakis approved these changes Jul 10, 2024

View reviewed changes

ronald-cron-arm added this pull request to the merge queue Jul 10, 2024

ronald-cron-arm closed this Jul 10, 2024

Merged via the queue into Mbed-TLS:development with commit 1004c9c Jul 10, 2024

ronald-cron-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-backports Backports are missing or are pending review and approval. needs-review Every commit must be reviewed by at least two team members, labels Jul 10, 2024

gilles-peskine-arm reviewed Jul 11, 2024

View reviewed changes

gilles-peskine-arm reviewed Aug 7, 2024

View reviewed changes

Move mbedtls crypto modules #9340

Move mbedtls crypto modules #9340

Uh oh!

Conversation

ronald-cron-arm commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronald-cron-arm commented Jul 4, 2024

Uh oh!

ronald-cron-arm commented Jul 4, 2024

Uh oh!

ronald-cron-arm commented Jul 4, 2024

Uh oh!

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

Uh oh!

davidhorstmann-arm commented Jul 4, 2024

Uh oh!

ronald-cron-arm commented Jul 4, 2024

Uh oh!

ronald-cron-arm commented Jul 8, 2024

Uh oh!

tom-daubney-arm left a comment

Choose a reason for hiding this comment

Uh oh!

ronald-cron-arm commented Jul 10, 2024

Uh oh!

ronald-cron-arm commented Jul 10, 2024

Uh oh!

tom-daubney-arm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

ronald-cron-arm commented Jul 10, 2024

Uh oh!

ronald-cron-arm commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minosgalanakis commented Jul 10, 2024

Uh oh!

ronald-cron-arm commented Jul 10, 2024

Uh oh!

gilles-peskine-arm Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

ronald-cron-arm Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ronald-cron-arm commented Jul 2, 2024 •

edited

Loading

tom-daubney-arm left a comment •

edited

Loading

ronald-cron-arm commented Jul 10, 2024 •

edited

Loading