mbedtls
mbedtls copied to clipboard
Mbed-TLS
md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled
Description
Instead of allowing PSA dispatching only when CRYPTO_C is set and some MBEDTLS_PSA_ACCEL_ALG_xxx is set, we enable dispatching when CRYPTO_CLIENT and PSA_WANT_ALG_xxx are set. This makes the feature more useful in cases where the PSA support is provided externally, like for example TF-M in Zephyr.
PR checklist
- [ ] changelog not required
- [ ] development PR can be started only once MD will stabilize in that branch
- [ ] framework PR not required
- [ ] 3.6 PR not required because this is it
- [ ] 2.28 PR not required
- [ ] tests not required because this does not affect the behavior of MD and its features are already tested
@mpg I took the liberty to add you as reviewer because you were the one who improved MD last year, allowing for the PSA dispatch, and we also discussed about this change in Slack. However if you don't have enough review bandwidth for this, please let me know and I'll remove you ;)
The ABI-API break is expected because struct mbedtls_md_context_t will change its size depending on PSA_WANT_ instead of MBEDTLS_PSA_ACCEL_ symbols.
- changelog TBD, but I would like to have some feedback from reviewers about this change before doing it.
IMO not ChangeLog for this, as support for CLIENT && !C is not official yet, and when it will become official it will have a ChangeLog entry with a scope much broader than just this :)
- development PR not required because, if I'm not wrong, the MD module is internal in
development, so the user is not able to make direct usage of it.
The current plan is for MD to remain public in 4.0, so I think we want to forward-port this to development. However there might be other changes planned (or already done) in development that would make this moot, I'm not sure. Cc @gilles-peskine-arm
The ABI-API break is expected because
struct mbedtls_md_context_twill change its size depending onPSA_WANT_instead ofMBEDTLS_PSA_ACCEL_symbols.
Aw, I'm afraid this is a blocker. We promise not to change the ABI in LTS branches (which 3.6 now is) unless we can't find another way to fix a security issue. So, changing the size of struct mbedtls_md_context_t in the default configuration is not something we can do in 3.6.
Would there be a way to achieve what you're after while keeping the structure unchanged in the default config, and only changing it in a few specific configs?
The current plan is for MD to remain public in 4.0, so I think we want to forward-port this to development.
Oh, I see. I based my previous comment on the fact that md.h header is now in tf-psa-crypto/drivers/builtin/include/mbedtls/md.h which looks like a quite "hidden" place not meant to be included by end user's code directly. However I surely missed something, so please apologize for the wrong assumption.
Would there be a way to achieve what you're after while keeping the structure unchanged in the default config, and only changing it in a few specific configs?
I think the easiest way will be to enable PSA dispatching based on PSA_WANTs only in the usual CRYPTO_CLIENT && !CRYPTO_C configuration. This is still relevant for Mbed TLS users like Zephyr, but it does not affect the default configuration. I will try to rework the PR in this direction.
The current plan is for MD to remain public in 4.0, so I think we want to forward-port this to development.
Oh, I see. I based my previous comment on the fact that md.h header is now in tf-psa-crypto/drivers/builtin/include/mbedtls/md.h which looks like a quite "hidden" place not meant to be included by end user's code directly. However I surely missed something, so please apologize for the wrong assumption.
Currently all of the legacy crypto headers have moved as part of the work to split the repositories. We'll move some of them again as part of the work to evolve the API. md.h will remain public (but without HMAC functionality). Most other crypto headers will become private or unstable.
Sorry about the lack of clarity. We're still working on clarifying what's going to happen and making a plan for it to happen in time.
Would there be a way to achieve what you're after while keeping the structure unchanged in the default config, and only changing it in a few specific configs?
I think the easiest way will be to enable PSA dispatching based on PSA_WANTs only in the usual
CRYPTO_CLIENT && !CRYPTO_Cconfiguration. This is still relevant for Mbed TLS users like Zephyr, but it does not affect the default configuration. I will try to rework the PR in this direction.
Thanks to this change the ABI-API failure disappeared and the CI is fully green now :)
The current plan is for MD to remain public in 4.0, so I think we want to forward-port this to development.
Oh, I see. I based my previous comment on the fact that md.h header is now in tf-psa-crypto/drivers/builtin/include/mbedtls/md.h which looks like a quite "hidden" place not meant to be included by end user's code directly. However I surely missed something, so please apologize for the wrong assumption.
Currently all of the legacy crypto headers have moved as part of the work to split the repositories. We'll move some of them again as part of the work to evolve the API. https://github.com/Mbed-TLS/mbedtls/issues/8450 (but without HMAC functionality). https://github.com/Mbed-TLS/mbedtls/issues/8663.
Thanks a lot for the update! So I think that the forward port of this fix will need to wait until MD design is stabilized/planned on development.
Ok, I'm not opposed to a ChangeLog entry as long as it reminds readers that CLIENT && !C is still not officially supported.
Good point about updating the documentation, thanks for noticing!
The last force-push was mostly to reset the head of this PR on top of the current mbedtls-3.6 because I have not been working on this PR for a long time and it was quite outdated.
Note that the CI is failing in test_default_psa_crypto_client_without_crypto_provider which looks related.
Note that the CI is failing in
test_default_psa_crypto_client_without_crypto_providerwhich looks related.
Yes, this is due to a change that I recently introduced https://github.com/Mbed-TLS/mbedtls/pull/9562/commits/cf8bbb88043b0fde4911ab561c65b8677dcc24f3. To me that made sense, but apparently this is triggering a lot of failures in that test scenarios because the library code "thinks" that the PSA Crypto supports hash functions, whereas they are all stub functions. I need to think a bit more on how to overcome this problem. In the worst case I'll just drop the commit.
Note that the CI is failing in
test_default_psa_crypto_client_without_crypto_providerwhich looks related.Yes, this is due to a change that I recently introduced cf8bbb8. To me that made sense, but apparently this is triggering a lot of failures in that test scenarios because the library code "thinks" that the PSA Crypto supports hash functions, whereas they are all stub functions. I need to think a bit more on how to overcome this problem. In the worst case I'll just drop the commit.
I think I've found a workaround for the problem I mentioned above -> https://github.com/Mbed-TLS/mbedtls/compare/304bd4f3efd424f762455b226a7afa28bbdcef12..5368386524f9432fb21179e7800dfdb4c861c824. Wdyt?
@mpg @gilles-peskine-arm the CI is green again, so the PR should be ready for another round of reviews when/if you have some spare time :)
We deem this essential to ensure Zephyr supports TF-M out-of-the-box for TLS/DTLS and X.509 use-cases using the LTS branch (of TF-M and Mbed TLS)
Zephyr actively uses MBEDTLS_PSA_CRYPTO_CLIENT
Please prioritize this for Mbed TLS 3.6.3. This seems like a very low hanging fruit for better PSA crypto adoption (and by extension essential for TF-M support)
The last update was just a rebase to solve conflicts on the framework reference.
@valeriosetti can you update the submodule pointer now that https://github.com/Mbed-TLS/mbedtls-framework/pull/140 has been merged?
Can you please make a PR to development? Sure, the md interface will change, but that doesn't affect the changes here.
Can you please make a PR to development? Sure, the md interface will change, but that doesn't affect the changes here.
I was about to start the forward porting few days ago, but then went checking the current status of development. There MD module is internal in TF-PSA-Crypto repo so in theory end users should not include/use it directly. As far as I saw TLS code only imports md.h to have new types definition and few #defines.
In short, I didn't saw any benefit in forward porting this PR so I stopped. Do you think it's still worth it? If so I can start the forward port anyway
md.h will remain public in TF-PSA-Crypto 1.x, with a subset of functionality: just hash calculation, not HMAC or metadata. Basically the current MBEDTLS_MD_LIGHT.