Move private headers to a private subdirectory
In both TF-PSA-Crypto and mbedtls, move private headers from include/mbedtls to include/mbedtls/private. Adjust #include directives accordingly.
This requires coordinated action in all three repositories, since Mbed TLS can include private TF-PSA-Crypto headers and mbedtls-framework can include private headers from both projects.
This is a single task, rather than done piece by piece, because each move of a header that is used in another project is an incompatible change. So we need to either prepare a transition period (which would be a lot more work), or deal with multiple small breaking changes (which is a lot of work because each breaking change is a hassle for the whole team, not just the task itself), or make a single breaking change (which is only a hassle once).
This task should be done with the help of a search-and-replace script. Include the script in the changelog message for the corresponding commit, which should consist purely of the changes made by the script.
This may require changes to build scripts. Keep them to a minimum, so that this task doesn't balloon with out-of-scope changes.
The following crypto headers are becoming private:
aes.h
aria.h
bignum.h
block_cipher.h
camellia.h
ccm.h
chacha20.h
chachapoly.h
cipher.h
cmac.h
config_adjust_legacy_from_psa.h
config_adjust_psa_superset_legacy.h
config_adjust_test_accelerators.h
config_psa.h
ctr_drbg.h
des.h
dhm.h
ecdh.h
ecdsa.h
ecjpake.h
ecp.h
entropy.h
error_common.h
gcm.h
hmac_drbg.h
md5.h
oid.h
pkcs12.h
pkcs5.h
poly1305.h
ripemd160.h
rsa.h
sha1.h
sha256.h
sha3.h
sha512.h
The following mbedtls headers are becoming private:
config_adjust_ssl.h
config_adjust_x509.h
debug.h
This does not seem to have consensus (yet?), see https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/145#discussion_r2011895912 and https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/223#issuecomment-2747366503 so I'm changing from "Implementation needed" to "Design needed" until we've reached a consensus.
@mpg But it looks like we're going ahead with the Everest part of this task? https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/229 is well under way.
I still think we should do this. It's a simple way of conveying which parts of installed headers are public interfaces. Not all users figure out the APIs from the Doxygen rendered documentation.
Let's look at the alternatives. If we don't do this, how do we convey that all the content in e.g. cipher.h is private? We ensure that function calls won't build through https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/220 and friends, so technically we have conveyed it. But in a very unfriendly way: users will reasonably think that mbedtls_cipher_xxx() are public functions when they look at the header files (we can't expect readers to follow the structure of preprocessor conditionals, there are far too many of them), then they'll be puzzled when their code doesn't build.
At a very minimum, if we don't convey the non-public nature of these headers through the path, I think we should remove all Doxygen comments from them. Which is not that much less work.
At this point, I think just doing this will be less costly than debating its exact priority (I agree it's something we want ideally, I think the point being debated is just whether we need it) - especially considering part of it (Everest) has already been started and is in active development, as you correctly pointed out.
I'm not sure how to properly attach the PR to this issue, so let me just reference it in this comment: mbed-tls/mbedtls#10224; mbed-tls/tf-psa-crypto#318
@amtkarm1 Following some discussion on Slack, I'd like to suggest the following plan that avoids having 3 mutually-dependent PRs with complex conditions on what we expect from the CI, and the need for a merge ban for synchronised merged, as is currently the case.
- TF-PSA-Crypto PR: starting from the current PR as it is now, add a new commit that, for each file
xxx.hthat was moved, creates a new file in the old location that contains just#include private/xxx.h(plus license headers as I think we need that to pass sanity checks) - ie, transitional "proxy headers". With this new commit, this PR should have a green CI on its own (currently its CI can't be green without changes in mbedtls and the framework), so we can review it and get it merged. - framework PR: once the above PR has been merged, apply this suggestion - the CI should be green, then we can review and merge this PR.
- mbedtls PR: once the above has been merged, just update the crypto and framework pointers (possibly rebasing if there are conflicts), hopefully the CI should be green, review and merge.
- Once the above has been merged, make an additional tf-psa-crypto PR that removes the proxies introduced in step 1 (ideally, they were introduced in a single commit, so we can just revert that commit). If there are CI failures, if they can be fixed in tf-psa-crypto, good. Otherwise, we might need to pause this PR, make another framework or mbedtls PR that fixes the issue, get it green CI, approval, merge, then come back to this PR (update the framework pointer if the issue was there) and now get a green CI, review, merge, the end.
Please let me know if anything's not clear about this plan.
https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/318 was merged. Still https://github.com/Mbed-TLS/mbedtls-framework/pull/175 and https://github.com/Mbed-TLS/mbedtls/pull/10224 to merge.
https://github.com/Mbed-TLS/mbedtls/pull/10224 was merged. Still https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/432 to merge