md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled #9562

valeriosetti · 2024-09-13T09:04:01Z

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

valeriosetti · 2024-09-13T09:13:07Z

@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 ;)

valeriosetti · 2024-09-16T09:22:30Z

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.

valeriosetti · 2024-09-16T16:00:33Z

CI green (a part from the ABI-API failure mentioned above), so I think the PR is ready for reviews

mpg · 2024-09-17T07:58:00Z

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 :)

mpg · 2024-09-17T07:59:45Z

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

mpg · 2024-09-17T08:05:27Z

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.

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?

mpg

#9562 (comment)

valeriosetti · 2024-09-17T08:39:28Z

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.

gilles-peskine-arm · 2024-09-17T08:59:55Z

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.

Move the auto-enabling of CRYPTO_CLIENT when CRYPTO_C at the beginning of the file so that all that becomes later is aware of this. Signed-off-by: Valerio Setti <[email protected]>

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. This commit also: - fixes disparities in hmac accel/ref comparison; - add proper stubs for the PSA crypto build without a PSA provider (CRYPTO_CLIENT && !CRYPTO_C scenario); - add proper guards for tests trying to use MD+PSA dispatch. Signed-off-by: Valerio Setti <[email protected]>

The previous change that replaced CRYPTO_C with CRYPTO_CLIENT caused an increase of the mbedtls_md struct in scenarios where the hash related PSA_WANTs were enabled, but not accelerated. This caused an ABI-API break which is not allowed for an LTS branch. Since the main goal here is to allow PSA dispatch in a "pure crypto client" scenario, we partially revert the previous change to config_adjust_legacy_crypto.h and add an extra condition for "CRYPTO_CLIENT && !CRYPTO_C". This commit also reverts changes done in analyze_outcomes.py because they are no more necessary. Signed-off-by: Valerio Setti <[email protected]>

valeriosetti · 2024-09-17T12:50:44Z

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.

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. #8450 (but without HMAC functionality). #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.

mpg

LGTM, thanks!

gilles-peskine-arm

I'm ok with this change in principle. And I agree that since this is targeting a configuration that is not officially supported (MBEDTLS_PSA_CRYPTO_CLIENT without MBEDTLS_PSA_CRYPTO_C), we should in principle not add a changelog entry. But still, I want more documentation for two audiences:

For maintainers, there should be an explanation of when md dispatches to PSA in plain English (as opposed to C code spread over multiple files). This is in principle covered by docs/architecture/psa-migration/md-cipher-dispatch.md, though I'm not sure it was fully up to date even before this pull request. Please update that document. And I'd like a summary, or a reference to that file, at least in md.c.
For users who use CLIENT without C (and who enable at least one hash), there is an incompatible change: they now need to implement psa_can_do_hash. We should document that somewhere, even if we keep saying that it's experimental. I guess in the documentation of MBEDTLS_PSA_CRYPTO_CLIENT?

Note that I'm not convinced that we shouldn't add a changelog entry. This is a long-time support branch, so the threshold for considering a change to be important it low. We do sometimes announce things like “The undocumented ability … has been removed” in the changelog. I think now is one of those times.

mpg · 2024-11-06T08:31:56Z

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!

valeriosetti self-assigned this Sep 13, 2024

valeriosetti added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-xs Estimated task size: extra small (a few hours at most) labels Sep 13, 2024

valeriosetti requested a review from mpg September 13, 2024 09:11

valeriosetti force-pushed the md-psa-dispatch-3.6 branch 2 times, most recently from a011d15 to 51772c4 Compare September 16, 2024 08:55

valeriosetti force-pushed the md-psa-dispatch-3.6 branch 3 times, most recently from acd4c96 to e43f2e1 Compare September 16, 2024 13:27

valeriosetti removed the needs-ci Needs to pass CI tests label Sep 16, 2024

mpg added the needs-work label Sep 17, 2024

mpg requested changes Sep 17, 2024

View reviewed changes

valeriosetti added 3 commits September 17, 2024 11:32

adjust_legacy_crypto: move auto-enabling of CRYPTO_CLIENT when CRYPTO_C

3a33ac7

Move the auto-enabling of CRYPTO_CLIENT when CRYPTO_C at the beginning of the file so that all that becomes later is aware of this. Signed-off-by: Valerio Setti <[email protected]>

valeriosetti force-pushed the md-psa-dispatch-3.6 branch from e43f2e1 to de2f6f9 Compare September 17, 2024 10:00

valeriosetti removed the needs-work label Sep 17, 2024

mpg approved these changes Sep 19, 2024

View reviewed changes

gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Oct 28, 2024

gilles-peskine-arm added priority-medium Medium priority - this can be reviewed as time permits component-psa PSA keystore/dispatch layer (storage, drivers, …) labels Oct 28, 2024

gilles-peskine-arm self-requested a review October 28, 2024 10:44

gilles-peskine-arm requested changes Nov 5, 2024

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 Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled #9562

md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled #9562

valeriosetti commented Sep 13, 2024 •

edited

Loading

valeriosetti commented Sep 13, 2024

valeriosetti commented Sep 16, 2024

valeriosetti commented Sep 16, 2024

mpg commented Sep 17, 2024

mpg commented Sep 17, 2024

mpg commented Sep 17, 2024

mpg left a comment

valeriosetti commented Sep 17, 2024

gilles-peskine-arm commented Sep 17, 2024 •

edited

Loading

valeriosetti commented Sep 17, 2024 •

edited

Loading

mpg left a comment

gilles-peskine-arm left a comment

mpg commented Nov 6, 2024

md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled #9562

Are you sure you want to change the base?

md: allow dispatch to PSA whenever CRYPTO_CLIENT is enabled #9562

Conversation

valeriosetti commented Sep 13, 2024 • edited Loading

Description

PR checklist

valeriosetti commented Sep 13, 2024

valeriosetti commented Sep 16, 2024

valeriosetti commented Sep 16, 2024

mpg commented Sep 17, 2024

mpg commented Sep 17, 2024

mpg commented Sep 17, 2024

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Sep 17, 2024

gilles-peskine-arm commented Sep 17, 2024 • edited Loading

valeriosetti commented Sep 17, 2024 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

mpg commented Nov 6, 2024

valeriosetti commented Sep 13, 2024 •

edited

Loading

gilles-peskine-arm commented Sep 17, 2024 •

edited

Loading

valeriosetti commented Sep 17, 2024 •

edited

Loading