Skip to content

Move public legacy headers to /include/mbedtls #247

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

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

felixc-arm
Copy link
Contributor

@felixc-arm felixc-arm commented Apr 4, 2025

Moves public headers from /drivers/builtin/include/mbedtls to /include/mbedtls to make it clear that they are public.
Resolves #223, although there may be further work to move these headers somewhere else, e.g. /include/tf-psa-crypto.

List of files moved for reference:

asn1.h
asn1write.h
base64.h
constant_time.h
lms.h
memory_buffer_alloc.h
nist_kw.h
pem.h
pk.h
platform.h
platform_time.h
platform_util.h
psa_util.h
threading.h

PR checklist

@felixc-arm felixc-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 size-xs Estimated task size: extra small (a few hours at most) needs-ci Needs to pass CI tests needs-work labels Apr 4, 2025
@gilles-peskine-arm gilles-peskine-arm added priority-high High priority - will be reviewed soon and removed 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 7, 2025
@gilles-peskine-arm
Copy link
Contributor

There are probably build scripts that need to be updated. For example include/CMakeLists.txt has

    file(GLOB tf-psa-crypto_headers "tf-psa-crypto/*.h")
    file(GLOB mbedtls_crypto_headers "../drivers/builtin/include/mbedtls/*.h")

and presumably there needs to be a line for "mbedtls/*.h". I don't know whether it should be one of these variables or yet another one: I don't know what the difference is between those variables.

@felixc-arm
Copy link
Contributor Author

The companion PR Mbed-TLS/mbedtls#10122 has now passed the CI

@felixc-arm felixc-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-work needs-ci Needs to pass CI tests labels Apr 10, 2025
@bjwtaylor bjwtaylor self-requested a review April 14, 2025 09:49
bjwtaylor
bjwtaylor previously approved these changes Apr 14, 2025
@valeriosetti valeriosetti self-requested a review April 15, 2025 11:35
Copy link
Contributor

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks mostly OK to me. I only left 1 question

valeriosetti
valeriosetti previously approved these changes Apr 15, 2025
@github-project-automation github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Apr 15, 2025
@felixc-arm felixc-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 needs-reviewer This PR needs someone to pick it up for review labels Apr 15, 2025
@gilles-peskine-arm
Copy link
Contributor

The CI is unhappy here. Is the companion mbedtls pull request needed to make it happy?

@felixc-arm
Copy link
Contributor Author

felixc-arm commented Apr 16, 2025

Yep, there's Mbed-TLS/mbedtls#10122 & Mbed-TLS/mbedtls-framework#156. Both of those should be merge-able without any dependencies, so I guess the process would be merge framework -> update mbedtls PR's tf-psa-crypto pointer to development head rather than this patch & update framework pointer to the merge commit -> wait for mbedtls to pass CI without this tf-psa-crypto commit -> merge mbedtls PR -> update framework pointer to the merge commit for this tf-psa-crypto PR which should then make this CI green -> merge this PR.

Does this seem right?

@gilles-peskine-arm
Copy link
Contributor

@felixc-arm The framework PR can be merged on its own. But the mbedtls PR updates the crypto submodule, so it can't be merged until the crypto PR is merged. Is there a reasonable intermediate step where we can update mbedtls and crypto independently?

@felixc-arm
Copy link
Contributor Author

@gilles-peskine-arm If I change the crypto pointer in the mbedtls PR back to development rather than this PR, then it should still be green and be able to be merged.

Then this crypto PR can be merged as the CI will use the new mbedtls from the now-merged mbedtls PR.

I guess after that is done then we can do a new patch that just updates mbedtls' crypto pointer to use the merge commit from this crypto PR, although that might not be needed if we don't care about the change being picked up immediately (although it would probably be better to do this so that tf-psa-crypto from the mbedtls repo is the same as TF-PSA-Crypto from the TF-PSA-Crypto repo)

This is the only way I can see to get stuff merged without (temporarily) breaking things for other people.

@gilles-peskine-arm
Copy link
Contributor

@felixc-arm That's great! Please do this then. When it's just a matter of splitting PR like this, it's less disruptive than requiring branch updates to be synchronized — that tends to break many pull requests for unrelated work in progress.

@gilles-peskine-arm
Copy link
Contributor

There's a merge conflict in the framework submodule, please update it.

There are also CI failures which look like they might be related to this change.

@gilles-peskine-arm gilles-peskine-arm added needs-work needs-ci Needs to pass CI tests and removed approved Design and code approved - may be waiting for CI or backports labels Apr 23, 2025
@felixc-arm
Copy link
Contributor Author

@gilles-peskine-arm Yep, just waiting on Mbed-TLS/mbedtls#10122 to go in so that the mbedtls changes are in place, then this patch should be green. (+ I'll update the framework pointer at that point as well)

@felixc-arm felixc-arm added the needs-preceding-pr Requires another PR to be merged first label Apr 24, 2025
@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-work labels Apr 24, 2025
@felixc-arm felixc-arm dismissed stale reviews from valeriosetti and bjwtaylor via 63b5717 April 25, 2025 09:16
@felixc-arm felixc-arm force-pushed the move-public-headers branch from ced1c6d to 63b5717 Compare April 25, 2025 09:16
@felixc-arm felixc-arm added needs-review Every commit must be reviewed by at least two team members and removed needs-preceding-pr Requires another PR to be merged first needs-ci Needs to pass CI tests labels Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports needs-review Every commit must be reviewed by at least two team members priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
Development

Successfully merging this pull request may close these issues.

Move public legacy headers to /include
4 participants