Move everest headers #235

bjwtaylor · 2025-03-24T13:09:21Z

Description

Move everest headers. Depends Mbed-TLS/mbedtls-framework#153

PR checklist

changelog provided
framework PR not required
mbedtls development PR provided Move everest headers mbedtls#10091
mbedtls 3.6 PR not required because: No Backports
mbedtls 2.28 PR not required because: No Backports
tests not required because: No changes

ChangeLog.d/change-everest-headers-path.txt

drivers/everest/CMakeLists.txt

Signed-off-by: Ben Taylor <[email protected]>

gilles-peskine-arm

Looks good to me apart from .gitmodules, and apart from some issues which are at least partly preexisting and I'm not sure whether they're in scope right now or need to be part of some larger fix.

gilles-peskine-arm · 2025-04-24T11:49:01Z

.gitmodules

@@ -1,3 +1,3 @@
 [submodule "framework"]
 	path = framework
-	url = https://github.com/Mbed-TLS/mbedtls-framework
+	url = git@github.com:bjwtaylor/mbedtls-framework.git


As in mbedtls, no change to .gitmodules is desirable or necessary.

This change is temporary, it's just to test the dependent PR. It will be removed prior to the merge.

gilles-peskine-arm · 2025-04-24T11:55:06Z

drivers/p256-m/CMakeLists.txt

@@ -11,7 +11,7 @@ target_include_directories(${p256m_target}
         $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/p256-m>
         $<BUILD_INTERFACE:${TF_PSA_CRYPTO_DIR}/include>
         $<BUILD_INTERFACE:${TF_PSA_CRYPTO_DIR}/drivers/builtin/include>
-         $<BUILD_INTERFACE:${TF_PSA_CRYPTO_DIR}/drivers/everest/include>
+         $<BUILD_INTERFACE:${TF_PSA_CRYPTO_DIR}/drivers/everest/include/tf-psa-crypto/private/>


I suppose that's out of scope, but I find this line weird: why would users of p256-m be built with any knowledge of Everest? This looks like a global change that shouldn't have been applied to this file. Can you please try just removing this line?

Everest headers do need to be available privately when building p256-m, however. There's no functional dependency, but p256-m.c calls psa_generate_random, which is declared in psa/crypto.h, which indirectly includes the header where Everest defines its ECDH context type. So maybe the line needs to be moved to the PRIVATE section and not removed.

Ok, I'll give it a go removing it.

Seems to work without it, so I removed it.

Interesting, this change seems to cause a failure when MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED is defined. Not sure if it's related to the next issue, I shall investigate.

This caught my eyes as it is indeed strange to have an everest include path for p256-m. Looking at it, it seems it is because p256-m code include psa/crypto.h which when MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED is enabled includes eventually everest/everest.h. It is probably somehow wrong for a driver to include psa/crypto.h but that's not an easy thing to change I'd say. The everest include path should be private though.

Thanks for the additional info, I've reverted the removal now. I shall look into making the path private.

gilles-peskine-arm · 2025-04-24T12:09:44Z

drivers/everest/CMakeLists.txt

@@ -11,8 +11,9 @@ target_include_directories(${everest_target}
         $<BUILD_INTERFACE:${TF_PSA_CRYPTO_DIR}/include>
         $<BUILD_INTERFACE:${TF_PSA_CRYPTO_DIR}/drivers/builtin/include>
         $<INSTALL_INTERFACE:include>
-  PRIVATE include/everest
-          include/everest/kremlib
+  PRIVATE $<BUILD_INTERFACE:${TF_PSA_CRYPTO_DIR}/drivers/everest/include/tf-psa-crypto/private/>


This may be preexisting, but the declaration of include paths is wrong here: at least some of these directories need to be public. Note that there's a clash in terminology:

In the 1.0/4.0 work, I've called APIs “public” if they're documented and meant to be used in application code, and “private” otherwise. Everest is private since it's an implementation detail. However, it needs to be “exposed” because there's an opaque struct that has a field whose type is defined by an Everest header.

In CMake, a “private” header is one that is only needed when compiling the project, while a “public” header is one that consuming projects need to have access to. At least part of Everest is public in this sense — that's what “exposed” means for a header.

To test that, run cmake --install . --prefix /tmp/235 after building with MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED and try compiling a source file containing just #include <psa/crypto.h> with cc -I /tmp/235/include. This fails because #include "everest/everest.h" cannot be fulfilled. You can also see that it's missing from the log of cmake --install.

Using Mbed TLS from another project is currently broken in other ways and untested, so this may be an issue that we file for later. These issues are critical for the 1.0/4.0 release.

So, there was another issue with the install path which I have now corrected. I have also moved the header to public as requested. Let me know what you think and lets see if the CI highlights anything though.

Signed-off-by: Ben Taylor <[email protected]>

bjwtaylor force-pushed the move-everest-headers branch 8 times, most recently from cbfabef to 91fe5e9 Compare March 31, 2025 07:55

bjwtaylor mentioned this pull request Apr 2, 2025

Move everest headers Mbed-TLS/mbedtls-framework#153

Open

3 tasks

bjwtaylor force-pushed the move-everest-headers branch 2 times, most recently from 36b5120 to 248d479 Compare April 2, 2025 08:42

bjwtaylor mentioned this pull request Apr 2, 2025

Move everest headers Mbed-TLS/mbedtls#10091

Open

6 tasks

bjwtaylor force-pushed the move-everest-headers branch from 248d479 to 0cae869 Compare April 3, 2025 09:52

bjwtaylor marked this pull request as ready for review April 4, 2025 06:43

bjwtaylor 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 labels Apr 8, 2025

felixc-arm self-requested a review April 15, 2025 10:01

felixc-arm requested changes Apr 15, 2025

View reviewed changes

ChangeLog.d/change-everest-headers-path.txt Outdated Show resolved Hide resolved

drivers/everest/CMakeLists.txt Outdated Show resolved Hide resolved

bjwtaylor force-pushed the move-everest-headers branch 5 times, most recently from 3108d9d to a4ce107 Compare April 22, 2025 08:34

Ben Taylor added 7 commits April 22, 2025 09:34

Move everest headers

7e3746e

Signed-off-by: Ben Taylor <[email protected]>

Update everest include paths in CMakeLists

698bb98

Signed-off-by: Ben Taylor <[email protected]>

corrected include path in Makefile.inc

cae1ada

Signed-off-by: Ben Taylor <[email protected]>

add additional include paths for everest header relocation

6633c6f

Signed-off-by: Ben Taylor <[email protected]>

Add ChangeLog for everest headers

2c6ee8d

Signed-off-by: Ben Taylor <[email protected]>

correct changelog

9461852

Signed-off-by: Ben Taylor <[email protected]>

remove unnecessary paths for everest headers

efdf1fc

Signed-off-by: Ben Taylor <[email protected]>

Remove ChangeLog as it is not required

c087193

Signed-off-by: Ben Taylor <[email protected]>

bjwtaylor force-pushed the move-everest-headers branch 4 times, most recently from d824ad0 to 1897fad Compare April 23, 2025 06:56

update submodules for test

ca5bc62

Signed-off-by: Ben Taylor <[email protected]>

bjwtaylor force-pushed the move-everest-headers branch from 1897fad to ca5bc62 Compare April 23, 2025 11:53

gilles-peskine-arm self-requested a review April 24, 2025 09:32

gilles-peskine-arm added priority-high High priority - will be reviewed soon and removed needs-reviewer This PR needs someone to pick it up for review labels Apr 24, 2025

gilles-peskine-arm added this to Roadmap pull requests (new board) Apr 24, 2025

github-project-automation bot moved this to In Development in Roadmap pull requests (new board) Apr 24, 2025

gilles-peskine-arm requested changes Apr 24, 2025

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members labels Apr 24, 2025

Ben Taylor added 7 commits April 24, 2025 15:26

Remove everest header path from p256-m as it is not required

50aa7b4

Signed-off-by: Ben Taylor <[email protected]>

revert removal of everest path from p256 as it is currently required

35d1c87

Signed-off-by: Ben Taylor <[email protected]>

fixed install issues for everest headers

21fde05

Signed-off-by: Ben Taylor <[email protected]>

restored guards and made everest headers public

20ef876

Signed-off-by: Ben Taylor <[email protected]>

Correct tab style in everest CMakeFiles

789b400

Signed-off-by: Ben Taylor <[email protected]>

Make everest paths private in p256-m

4408e3a

Signed-off-by: Ben Taylor <[email protected]>

update submodules for test

30f64cc

Signed-off-by: Ben Taylor <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move everest headers #235

Move everest headers #235

bjwtaylor commented Mar 24, 2025 •

edited

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm Apr 24, 2025

bjwtaylor Apr 24, 2025

gilles-peskine-arm Apr 24, 2025

bjwtaylor Apr 24, 2025

bjwtaylor Apr 24, 2025

bjwtaylor Apr 25, 2025

ronald-cron-arm Apr 25, 2025 •

edited

Loading

bjwtaylor Apr 25, 2025

gilles-peskine-arm Apr 24, 2025

bjwtaylor Apr 25, 2025

Move everest headers #235

Are you sure you want to change the base?

Move everest headers #235

Conversation

bjwtaylor commented Mar 24, 2025 • edited Loading

Description

PR checklist

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjwtaylor commented Mar 24, 2025 •

edited

Loading

ronald-cron-arm Apr 25, 2025 •

edited

Loading