Make enrollement "optional" in pk_can_do_ext()

[valeriosetti]

This PR introduces guards (MBEDTLS_PSA_CRYPTO_CLIENT) in PK module for using key enrollment functions.

Resolves #8602

PR checklist

• changelog not required. This is an experimental feature with no official support, so no ChangeLog is required.
• backport not required since it's an enhancement
• tests not required. Since there is no official support for this feature, no test is required here.

[mpg]

Thanks! Normally I request tests in PRs that lack them, this is the rare instance where it's the other way round ;) But thanks for testing anyway!

[mpg]

Thanks for trying it out, I generally insist on automated tests, but in this instance, I think this is overkill. In particular, I'm not a big fan of having to mark a bunch of cases as depending on MBEDTLS_PSA_CRYPTO_CLIENT as this might send the wrong message.

I think for the level of support we want now (which is no official support), it's better not to test this.

[valeriosetti]

I understand :)
Side question: what about the fixes I did in the tests? Without anything that can test those depends_on additions it's hard to provide a clear justification for them. I would say that also the fixes I did for the tests are to be removed. Wdyt?

[mpg]

Yes, actually removing those is the primary reason I'd like to remove this.

[mpg]

Minor: I'd prefer this comment to be just before the first #if defined(MBEDTLS_PSA_CRYPTO_CLIENT), in order to be more discoverable and avoiding people getting the wrong impression when reading this #if` without the comment.

[mpg]

LGTM

[joerchan]

@valeriosetti This PR only addresess use of psa_get_key_enrollment_algorithm, There are still uses of psa_set_key_enrollment_algorithm that needs to be fixed to be compatible with TF-M.

[valeriosetti]

@valeriosetti This PR only addresess use of psa_get_key_enrollment_algorithm, There are still uses of psa_set_key_enrollment_algorithm that needs to be fixed to be compatible with TF-M.

Assuming #8612 will be merged soon, the only usages I still see are in mbedtls_pk_wrap_as_opaque, am I wrong?
This latter will be managed here as mentioned here. Is there anything we are missing?

[joerchan]

@valeriosetti This PR only addresess use of psa_get_key_enrollment_algorithm, There are still uses of psa_set_key_enrollment_algorithm that needs to be fixed to be compatible with TF-M.

Assuming #8612 will be merged soon, the only usages I still see are in mbedtls_pk_wrap_as_opaque, am I wrong? This latter will be managed here as mentioned here. Is there anything we are missing?

I think that is the only one, atleast the only one I saw when compiling for TF-M.

[gabor-mezei-arm]

LGTM

[mpg]

Removing from the merge queue and labeling needs-design-approval as I've got indirect feedback that the approach does not satisfy everyone.

@frkv Can you make your point here so that we can discuss it directly?

[mpg]

For the record of my current view of how it is or will be possible to build things and what we can assume in each case (I may misunderstand things, but I'm trying to make my view explicit to support discussion):

1. Builds with MBEDTLS_PSA_CRYPTO_C enabled (and MBEDTLS_PSA_CRYPTO_CLIENT too as a consequence: the implementation of PSA Crypto is Mbed TLS, and it's available locally.
2. Builds with MBEDTLS_PSA_CRYPTO_CLIENT && !MBEDTLS_PSA_CRYPTO_C: the implementation of PSA Crypto is not available locally (there's a security boundary between crypto client(s) and server, for example using Trustzone.) but it's still Mbed TLS. In that case I'd expect headers exported to applications to be fully compatible with those provided in the case above - in particular psa/crypto_extra.h should declare the same functions.
3. Builds without MBEDTLS_PSA_CRYPTO_CLIENT: the implementation of PSA Crypto is not Mbed TLS (including tf-psa-crypto), and may be local or have separation - this would be determined by ways specific to that implementation. Code that wants to also work with that kind of setup should only rely on PSA Crypto standard APIs.

Currently, only (1) is officially supported. The situation about (2) is a bit more complex: it's not officially supported by Mbed TLS mostly because we have not tests for this (though that's planned for next year), but TF-M is already working on a demo doing this. Then (3) is neither officially supported nor planned in the short to medium term, though it's certainly something we want to support in the long run.

A consequence of the above is that in every build that has MBEDTLS_PSA_CRYPTO_CLIENT defined, crypto_extra.h is Mbed TLS's version, so in particular it declares psa_get_key_enrollment_algorithm(). It's on that basis that I approved this PR.

[frkv]

I have a different take on the proposed defines and their meaning

1. There is a need to indicate that PSA core needs to be built (locally, using Mbed TLS sources). I've had the expectation that MBEDTLS_PSA_CRYPTO_C is a valid and accurate signal for this, similar to how we add crypto features features in Mbed TLS (for example MBEDTLS_AES_C)
2. The naming of MBEDTLS_PSA_CRYPTO_CLIENTcorresponds to the naturalistic usage of the term "client" which by name gives an indication at there is a "server" giving the actual support. This matches quite nicely with the fact that Arm PSA implemented by Trusted Firmware-M is provided as "secure services" in the non-secure prosessing environment. "secure services" is a term that is very close to a "server".
3. Services locally built or remotely provided gives access to the PSA crypto APIs with an expectancy that the APIs are compatible to ensure portable code.

So for the logical relationship between the two current proposed defines:

• If MBEDTLS_PSA_CRYPTO_C is defined then the PSA core is built/provided locally, and MBEDTLS_PSA_CRYPTO_CLIENT is a given
• If MBEDTLS_PSA_CRYPTO_CLIENT is defined but not MBEDTLS_PSA_CRYPTO_C then the PSA core is compiled elsewhere and the PSA crypto API is provided as a service (e.g. from TF-M)

[mpg]

Thanks for clarifying your views. We seem to be in agreement about the meaning of MBEDTLS_PSA_CRYPTO_C, but to have different views about the meaning of MBEDTLS_PSA_CRYPTO_CLIENT [edit: without MBEDTLS_PSA_CRYPTO_C]:

• IMO it means there's a server providing the actual support and this server is based on the Mbed TLS (aka tf-psa-crypto) implementation of PSA crypto, in particular it provides the same extensions.
• While you seem to take it as: there's a server providing the actual support and the client can assume nothing about this server beyond what's promised by the PSA crypto spec.

Do you think that's an accurate summary of the differences in our views?

I think it is useful to distinguish between the case where the server is based on Mbed TLS and the case where we make no assumptions about the server. While the later is indeed a reasonable expectation, and a long-term goal we have, it's currently not on our roadmap*. The former on the other hand is something that should be happening next year, and it is IMO an important intermediate step towards the later.

*As I noted elsewhere feel free to request we put in our our roadmap, but please be explicit about it - I think both from a technical and planning perspective we really need to distinguish between the server being based on Mbed TLS or not.

[gilles-peskine-arm]

the PSA crypto API is provided as a service (e.g. from TF-M)

Not just “the PSA crypto API” as in an implementation of the PSA Crypto API specification, but the PSA crypto API as implemented by Mbed TLS. We have never designed Mbed TLS to support using the X.509/TLS part with a different implementation of the crypto part. This feature is on our engineering roadmap as a far-future thing.

You can either build Mbed TLS as a single library, or in two parts with separation: crypto/keystore and pk/X.509/TLS. If you build with separation, then:

• On the client side, enable MBEDTLS_PSA_CRYPTO_CLIENT, disable MBEDTLS_PSA_CRYPTO_C, and link with implementations of the PSA API functions that make RPC calls.
• On the server side, enable MBEDTLS_PSA_CRYTPTO_C and MBEDTLS_PSA_CRYPTO_SPM, and link with an RPC server that calls the PSA API functions.
• Both sides need to have the same set of configured algorithms (PSA_WANT_xxx, assuming MBEDTLS_PSA_CRYPTO_CONFIG is enabled).

MBEDTLS_PSA_CRYPTO_CLIENT is not intended to be a standalone thing where you just build Mbed TLS with it. It's meant for one of two builds of Mbed TLS, the other one having MBEDTLS_PSA_CRYPTO_SPM enabled and the same crypto configuration.

We are now making a few adjustments to enable the X.509/TLS part of the library to be used with an alternative implementation of the PSA crypto specification, but these are ad hoc adjustments that we are doing on a case-by-case basis to help some Arm partners. Whether the alternative implementation of the crypto API is client-server or not is an implementation detail that is not directly relevant. However, since these are merely ad hoc adjustments, it's ok if they happen to only work when the alternative implementation is a client-server one.

Thus, interpreting MBEDTLS_PSA_CRYPTO_CLIENT as “there is an implementation of the PSA crypto API which is not the one built into Mbed TLS” is not a design goal. But it can be ok for an ad hoc change that we make with no expectation of backward compatibility across minor versions of Mbed TLS.