Do not perform adjustments on legacy crypto from PSA, when MBEDTLS_PSA_CRYPTO_CLIENT && !MBEDTLS_PSA_CRYPTO_C #9138
Conversation
valeriosetti
added
needs-review
include/mbedtls/config_psa.h
Outdated
| * (i.e. PSA_CRYPTO_CLIENT && !PSA_CRYPTO_C) if TLS/X509/PK rely on PSA APIs | ||
| * (i.e. USE_PSA_CRYPTO). */ | ||
| #if defined(MBEDTLS_PSA_CRYPTO_CLIENT) && !defined(MBEDTLS_PSA_CRYPTO_C) && \ | ||
| defined(MBEDTLS_USE_PSA_CRYPTO) |
There was a problem hiding this comment.
is it really mandatory to gate this with MBEDTLS_USE_PSA_CRYPTO as well? I am thinking about a use case where the client is just interested in doing Crypto, without really the need of doing TLS, so they won't care much about setting (or not setting) MBEDTLS_USE_PSA_CRYPTO. I guess it's more of a question for @gilles-peskine-arm though
There was a problem hiding this comment.
If it were only for crypto operations then I would agree with you, but for what concerns PK/TLS (and perhaps also MD/cipher) they might size buffers internally depending on USE_PSA or not. In other words, in a case where !USE_PSA, if legacy symbols are not set from the PSA ones, TLS/PK might under-estimate the size of some buffers.
This is just my guess though. I'm sure @gilles-peskine-arm or @mpg can provide a better answer :)
There was a problem hiding this comment.
I agree with Valerio's assessment. Note that in 4.0 USE_PSA will go away, and when that happens things will get simpler, but in the meantime (and in 3.6) I think we still need it to gate this.
There was a problem hiding this comment.
Understood, thanks. It makes sense.
There was a problem hiding this comment.
Upon reflection, the goal of config_adjust_legacy_from_psa.h is to ensure that if PSA wants a crypto feature then it's provided somehow, either through a PSA driver or through the a built-in implementation. This is independent of USE_PSA_CRYPTO. When PSA_CRYPTO_CLIENT && !PSA_CRYPTO_C, all PSA crypto feature come from the server, so config_adjust_legacy_from_psa.h is useless, regardless of what's using it.
they might size buffers internally depending on USE_PSA or not
That would also break driver-only configurations. So this is only a concern for mechanisms where driver-only configurations are not supported. The only mechanism for which the limitations affect basic key management (as opposed to more advanced capabilities) is RSA. The pk/x509/ssl code gates its RSA support on whether MBEDTLS_RSA_C is enabled, not on PSA_WANT_*RSA*, whether or not MBEDTLS_USE_PSA_CRYPTO is enabled. So when PSA crypto is client-server, on the client, you won't be able to use the PSA RSA if MBEDTLS_RSA_C is disabled, but there should be no undersized buffers.
This does mean that there's an incompatible change here: if you had PSA_WANT_ALG_RSA_xxx and PSA_WANT_KEY_PAIR_RSA_xxx and MBEDTLS_PSA_CRYPTO_CLIENT and !MBEDTLS_PSA_CRYPTO_C and !MBEDTLS_USE_PSA_CRYPTO in 3.6.0, then MBEDTLS_RSA_C was automatically enabled and RSA worked (through the local built-in implementation). (With the same configuration but USE_PSA_CRYPTO enabled, RSA would not work in pk/x509/TLS since the code would call PSA functions.) We can make this change, even in an LTS, because CLIENT && !C is documented as experimental, it's a weird configuration in the first place, and there's an easy workaround of explicitly requesting MBEDTLS_RSA_C. But we should document it in a changelog entry.
Therefore, I think (but I'd like @mpg's opinion on this as well) the best course of action is:
- Don't check
MBEDTLS_USE_PSA_CRYPTOhere. - Explain the consequences of this check in a comment.
- Add a changelog entry.
There was a problem hiding this comment.
I think (but I'd like @mpg's opinion on this as well) the best course of action is
@mpg do you agree with the plan than @gilles-peskine-arm proposed? :)
There was a problem hiding this comment.
Hi @mpg / @valeriosetti / @gilles-peskine-arm , just a little nudge to keep the review alive.
There was a problem hiding this comment.
I'm convinced by Gilles's reasonning.
valeriosetti
force-pushed
the
issue9126
branch
2 times, most recently
from
da4a65d to
f235d16
Compare
valeriosetti
removed
needs-ci
include/mbedtls/config_psa.h
Outdated
| @@ -32,7 +32,13 @@ | |||
| * before we deduce what built-ins are required. */ | |||
| #include "psa/crypto_adjust_config_key_pair_types.h" | |||
|
|
|||
| #if defined(MBEDTLS_PSA_CRYPTO_CLIENT) && !defined(MBEDTLS_PSA_CRYPTO_C) | |||
There was a problem hiding this comment.
I'm not sure I like the way this is written. How about:
#if defined(MBEDTLS_PSA_CRYPTO_C)
/* If we are implementing PSA crypto ourselves, then we want to enable the required built-ins.
* Otherwise, PSA features will be provided by the server. */
#include "mbedtls/config_adjust_legacy_from_psa.h"
#endifThe comment can be tuned, but my points are:
- I don't think we need to check for
CLIENThere as I don't think we're ever readingconfig_psa.hwithCLIENTdisabled. - Then I prefer "if condition then" to "if !condition else".
Signed-off-by: Valerio Setti <[email protected]>
valeriosetti
added
needs-review
ChangeLog.d/9126.txt
Outdated
| @@ -0,0 +1,5 @@ | |||
| Default behavior changes | |||
| * In a PSA-client-only build (i.e. MBEDTLS_PSA_CRYPTO_CLIENT && | |||
| !MBEDTLS_PSA_CRYPTO_C) no built-in support will be provided for | |||
There was a problem hiding this comment.
I still find “no built-in support will be provided” to be unclear and potentially misleading. You can build with local legacy crypto: the change is that you can now also build without. How about this?
In a PSA-client-only build (i.e. MBEDTLS_PSA_CRYPTO_CLIENT && !MBEDTLS_PSA_CRYPTO_C), do not automatically enable local crypto when the corresponding PSA mechanism is enabled, since the server provides the crypto. Fixes #9126.
Signed-off-by: Valerio Setti <[email protected]>
mpg
added
approved
mpg
removed
the
needs-backports
This commits belongs to PR 9138 of the official MbedTLS repo (Mbed-TLS/mbedtls#9138). It will be included both in the next LTS release (3.6.1) as well as the next standard one (4.0) so it can be dropped at the next Zephyr's downstream version bump. Signed-off-by: Valerio Setti <[email protected]>
Description
Resolves #9126.
PR checklist