Design discussion: add new symbol for PSA key enrollment functions

[valeriosetti]

Description

This PR adds a new build symbol named MBEDTLS_PSA_ENABLE_KEY_ENROLLMENT to enable PSA key enrollment functions:

• psa_set_key_enrollment_algorithm()
• psa_get_key_enrollment_algorithm()

Reasons for this change

This change was raised in last biweekly meeting by Nordic and it is strongly requested for TF-M support.

PR checklist

• changelog question to reviewers: these functions are not part of the standard PSA interface and they are part of the Mbed TLS customization. Do we need to track this change anyway?
• backport not required as it's an improvement
• tests a new test component is provided to test the new configuration symbol

[gilles-peskine-arm]

The current implementation breaks backward compatibility.

Also, the reason we would want this is unclear. This feature doesn't cost a lot of code size.

[gilles-peskine-arm]

For backward compatibility, the feature has to be available by default. So we need to make this a negative option, off by default.

[gilles-peskine-arm]

How much code size will this save (once it's finished)?

[valeriosetti]

For this question I replied in the comment below

[frkv]

This is not about code size. This is about compliance with the PSA crypto APIs implemented as services e.g. by TF-M. alg2 and the key enrollment concept doesn't exist in TF-M.

[gilles-peskine-arm]

The existence of this feature doesn't break compliance with the PSA crypto API, and services built on top of Mbed TLS don't need to support setting the enrollment algorithm.

Can you please describe the problem you're facing? If it's not code size then I suspect you're trying to fix it in the wrong place.

[gilles-peskine-arm]

Please don't use preprocessor conditionals on unbalanced code fragments..

[gilles-peskine-arm]

Missing: removing the alg2 field from the context, and the code in psa_crypto.c that deals with it.

[valeriosetti]

True, that's something I noticed as well. I didn't modify that part because, as I explained in my comment below, I was mostly focused on having a TLS/DTLS implementation which could work with standard PSA interface.
However I agree that this is something I can improve as well. I will try to fix this ASAP

[valeriosetti]

The current implementation breaks backward compatibility.

I see, that's what I mentioned also in the description indeed. I implemented it as positive symbol because that's what I've been asked, but we can find a different solution for that. Making it negative it's surely an option, but what about keeping it positive and having it automatically enabled by CRYPTO_C for example? Backward compatibility should be kept in this case.
[Personal opinion] Adding new functions/customization to a standard interface such as PSA is fine, but I would have guarded those add-ons with proper guards since the very beginning, so that it was always easily possible to build with both interfaces.

Also, the reason we would want this is unclear. This feature doesn't cost a lot of code size.

Here I will provide my answer, but I'm pretty sure Nordic and/or TFM group can bring more arguments to the discussion.
I think that with this PR they are not really focusing on code size, but rather to answer the question: "what if I want to use a PSA library developed on our own which provides proper accelerations for our microcontrollers?". I know that the proper answer might be "you should implement it by replacing driver_wrappers with your implementation", but I think that also the approach of totally replacing the PSA part is (or should be) acceptable. After all, as we said, PSA is a standard interface so it should be feasible to keep TLS/DTLS and "high-level" modules from mbedTLS (with USE_PSA and CRYPTO_CLIENT enabled) and link a totally different PSA implementation.
I know, this is something we cannot achieve now because there are code changes required to get this, but I'm speaking of something we can point at.
With this goal I think that this PR makes sense: optionally remove extra PSA customization which are not in the standard so that TLS/DTLS can work with some external PSA implementation. This implementation can then be just a library or calls to some secure image (ex: TFM in Zephyr).

[gilles-peskine-arm]

what if I want to use a PSA library developed on our own which provides proper accelerations for our microcontrollers?". (…) After all, as we said, PSA is a standard interface so it should be feasible to keep TLS/DTLS and "high-level" modules from mbedTLS (with USE_PSA and CRYPTO_CLIENT enabled) and link a totally different PSA implementation.

That's true, but it is not something that Mbed TLS currently supports. It's on our mental roadmap, currently thinking we might deliver this some time in the 4.x series or maybe in 5.0, but it's not scheduled yet and we're currently prioritizing other features over this.

[gilles-peskine-arm]

I see that you're continuing to work on this pull request, but it's still not clear to me why we should accept it. By default, we don't want new compilation options: they have to have a clear benefit. Saving a significant amount of code size would be an acceptable benefit, but apparently that's not it, then what?

[valeriosetti]

I see that you're continuing to work on this pull request, but it's still not clear to me why we should accept it. By default, we don't want new compilation options: they have to have a clear benefit. Saving a significant amount of code size would be an acceptable benefit, but apparently that's not it, then what?

As I explained here, Nordic is concerned about the possibility to use TLS and related "middle/high level modules" together with some other PSA implementation, i.e. building TLS with USE_PSA and CRYPTO_CLIENT and totally replacing what is included from CRYPTO_C. Nowadays trying to do so would result in those functions being missing in the linked external PSA library/implementation.

I know you said this is not in the roadmap for near future, but on one hand Nordic is interested in this feature and, on the other one, since AFAIU it is still something you would like to do sooner or later, we're just anticipating this work a bit.

[gilles-peskine-arm]

building TLS with USE_PSA and CRYPTO_CLIENT and totally replacing what is included from CRYPTO_C.

Ok. That's a desirable feature, and we can support improvements in that direction, even if we have no engineering time beyond reviewing your work.

Nowadays trying to do so would result in those functions being missing in the linked external PSA library/implementation.

psa_set_key_enrollment_algorithm and psa_get_key_enrollment_algorithm are static inline functions. I don't understand where they would come up as needing to be linkable functions.

It seems to me that you're looking in the wrong place to solve your problem.

[valeriosetti]

psa_set_key_enrollment_algorithm and psa_get_key_enrollment_algorithm are static inline functions. I don't understand where they would come up as needing to be linkable functions.

That's correct, but if the end user does not add MbedTLS version of crypto_extra.h, then those functions would not be defined. This can happen for example because the 3rd party PSA library they are integrating does not have or have a different crypto_extra.h file.
Even if she/he does include it, it seems to me that those functions are trying to read/write a field of attributes structure (actually attributes->core->policy) whose internal data is implementation specific. In other words, the attributes structure provided by a 3rd party PSA library might differ so the fields that those functions are trying to access might not exist.
Am I wrong?

[gilles-peskine-arm]

That's correct, but if the end user does not add MbedTLS version of crypto_extra.h, then those functions would not be defined. This can happen for example because the 3rd party PSA library they are integrating does not have or have a different crypto_extra.h file.

That's fine: the X.509 and TLS code don't call those functions.

Are you trying to use an Mbed TLS build with MBEDTLS_PSA_CRYPTO_CLIENT together with a different server-side implementation? That is not supported and we have no intent to support it. What we intend to support at some point in the future is to make the X.509 and TLS layers work with other implementations of the PSA Crypto API.

If you have your own PSA Crypto implementation, it's up to you to implement the API. The client-side functions of Mbed TLS aren't going to help you much anyway, since in a client-server implementation the client-side code is pretty much IPC calls which you have to implement based on your IPC mechanism.

the attributes structure provided by a 3rd party PSA library might differ so the fields that those functions are trying to access might not exist.

Yes, and that's true for all the fields: type, usage_flags, etc. Which is why the third-party PSA library must provide all the psa_{get,set}_key_xxx functions that the application uses (including the Mbed TLS X.509/TLS stack). The enrollment algorithm functions are not different from the standard functions with respect to the attribute structure. They are different from the standard functions because X.509/TLS don't use them.

[mpg]

I'm afraid I agree with Gilles and am unconvinced this is something we want at the moment. Each additional compilation option is a cost, and I'm not seeing a benefit that would justify that cost now or in the near future.

After all, as we said, PSA is a standard interface so it should be feasible to keep TLS/DTLS and "high-level" modules from mbedTLS (with USE_PSA and CRYPTO_CLIENT enabled) and link a totally different PSA implementation.

I think the above sentence shows a confusion between two distinct things.

1. Have TLS work on top of MBEDTLS_PSA_CRYPTO_CLIENT without MBEDTLS_PSA_CRYPTO_C. This allows to put a security boundary between the crypto provider and crypto consumers, but both sides are still supposed to be the Mbed TLS implementation of PSA Crypto (note the option name starts with MBEDTLS_.) As I understand it, that's a goal we have in the medium term (like, next year), but I don't think the present PR is about this.
2. Have TLS work on top of a totally different PSA implementation - regardless of whether that implementation include a security boundary somewhere. As Gilles wrote, this is not currently on our roadmap, though it's been on our mind as long-term goal for a while. I'll note the way this will work is unlikely to include setting MBEDTLS_PSA_CRYPTO_CLIENT as this does enable bits of the Mbed TLS implementation of PSA (again, note the prefix in the name).

I think this PR could be useful only as a step toward the 2nd goal. However, seeing as it's in the distant future, I don't think this is enough to justify the cost of an additional config option right now.

One more thing. If Nordic (or anyone else) would like us to put that 2nd goal on our roadmap, please let us know, but:

• Please be specific about it, because I got the distinct feeling that there has been discussions where some people were talking about (1) and others about (2) and the distinction was not clearly made.
• This invitation is not a promise that (2) will happen soon if asked! I think there are a number of other things that need to happen first before we can tackle (2) seriously.

(On a personal note, I guess (2) might be a bit similar to driver-only, in that both are obvious on paper ("I have a driver for X, why would I need the built-in implementation?", resp. "PSA Crypto is a standard interface, TLS should be able to work with any compliant implementation") but turn out to be a lot of work in practice. For driver-only, we've been at it for more than one year now and we've not done yet. Hopefully some things will become easier after 4.0, though.)

[valeriosetti]

OK, thanks for the very detailed explanation and distinction between points (1) and (2). In particular the sentence

both sides are still supposed to be the Mbed TLS implementation of PSA Crypto

helped a lot in the clarification.

Now that you provided these details I would say that Nordic's goal is to get point (2). But again, this is just my hypothesis, so I will let @frkv correct me if I'm wrong.

If Nordic (or anyone else) would like us to put that 2nd goal on our roadmap, please let us know

This PR is unlikely the right place to discuss long term goals. I think that the bi-weekly meeting is a better option for this since it involves also management.

[Vge0rge]

I'm afraid I agree with Gilles and am unconvinced this is something we want at the moment. Each additional compilation option is a cost, and I'm not seeing a benefit that would justify that cost now or in the near future.

After all, as we said, PSA is a standard interface so it should be feasible to keep TLS/DTLS and "high-level" modules from mbedTLS (with USE_PSA and CRYPTO_CLIENT enabled) and link a totally different PSA implementation.

I think the above sentence shows a confusion between two distinct things.

1. Have TLS work on top of MBEDTLS_PSA_CRYPTO_CLIENT without MBEDTLS_PSA_CRYPTO_C. This allows to put a security boundary between the crypto provider and crypto consumers, but both sides are still supposed to be the Mbed TLS implementation of PSA Crypto (note the option name starts with MBEDTLS_.) As I understand it, that's a goal we have in the medium term (like, next year), but I don't think the present PR is about this.
2. Have TLS work on top of a totally different PSA implementation - regardless of whether that implementation include a security boundary somewhere. As Gilles wrote, this is not currently on our roadmap, though it's been on our mind as long-term goal for a while. I'll note the way this will work is unlikely to include setting MBEDTLS_PSA_CRYPTO_CLIENT as this does enable bits of the Mbed TLS implementation of PSA (again, note the prefix in the name).

I think this PR could be useful only as a step toward the 2nd goal. However, seeing as it's in the distant future, I don't think this is enough to justify the cost of an additional config option right now.

One more thing. If Nordic (or anyone else) would like us to put that 2nd goal on our roadmap, please let us know, but:

• Please be specific about it, because I got the distinct feeling that there has been discussions where some people were talking about (1) and others about (2) and the distinction was not clearly made.
• This invitation is not a promise that (2) will happen soon if asked! I think there are a number of other things that need to happen first before we can tackle (2) seriously.

(On a personal note, I guess (2) might be a bit similar to driver-only, in that both are obvious on paper ("I have a driver for X, why would I need the built-in implementation?", resp. "PSA Crypto is a standard interface, TLS should be able to work with any compliant implementation") but turn out to be a lot of work in practice. For driver-only, we've been at it for more than one year now and we've not done yet. Hopefully some things will become easier after 4.0, though.)

@mpg Could you tell me your opinion on where TF-M sits in this comparison as a PSA crypto provider?
Assuming that TF-M is built using the PSA crypto implementation of Mbed-TLS my view is that we are still under the category (1) that you described. Using non-standard PSA APIs makes the TLS incompatible with this use case, right?

[mpg]

@mpg Could you tell me your opinion on where TF-M sits in this comparison as a PSA crypto provider?
Assuming that TF-M is built using the PSA crypto implementation of Mbed-TLS my view is that we are still under the category (1) that you described.

Yes, that's my view as well.

Using non-standard PSA APIs makes the TLS incompatible with this use case, right?

Indeed. But unless I'm mistaken, neither the TLS not the X.509 library are directly using the non-standard APIs that this PR is about. Those APIs are only used in PK, which is then used by TLS and X.509. However this is likely to change in 4.0 - either the internal structure of PK (and perhaps part of its interface), or its use by TLS & X.509, or both. (Edit: I've made a note to make sure we take this in consideration as part of our work on PK for 4.0.) So, it doesn't change my assessment that this PR, despite its best intentions, is not really a useful step towards that goal.

[gilles-peskine-arm]

Those APIs are only used in PK

They shouldn't be! The pk module does not need to support two algorithms in the same PSA key. It's news to me that pk*.c contains uses of psa_get_key_enrollment_algorithm and psa_set_key_enrollment_algorithm. This changes my opinion on the need for a new compilation option: not for removing those functions, but for removing the use of these functions from pk*.c. So the option would control a subset of MBEDTLS_USE_PSA_CRYPTO, not a subset of MBEDTLS_PSA_CRYPTO_C.

We can and should remove some of those uses, but I'm not sure if we can remove all of them.

• In mbedtls_pk_can_do_ext: just removing support would break backward compatibility. Mind you, the way the function is implemented reinvents the wheel, so arguably it should call some different implementation-specific PSA function. The function can be implemented on top of standard PSA functions, but at a significant CPU cost: the standard way to test if a key can do something is to actually attempt to do it.
• In mbedtls_pk_wrap_as_opaque: that function should take an attribute structure as argument rather than a few of its fields. That's an API change, but that's ok since we document this function as experimental. This function is one we need to stabilize in some form in 3.6 for it to be viable to work with PSA keys anyway.
• In pk_parse_key_sec1_der: this is a hack that happens to work because pk wants two algorithm families per EC key (ECDH and either ECDSA+any-hash or deterministic-ECDSA+any-hash), and our PSA implementation supports two algorithm families per key. It's weird that parsing code is concerned about policy, but I suspect that it needs this to preserve compatibility with the pk interface (which has policies in the form of MBEDTLS_PK_ECKEY_DH and MBEDTLS_PK_ECDSA, but largely bypasses them through MBEDTLS_PK_ECKEY).
• In ecdsa_sign_psa, but that code is just overly complicated: it does some complex analysis to decide between randomized and deterministic ECDSA, but it should instead just try psa_sign_hash twice.

[mpg]

I agree that at some point we should stop using those non-standard APIs in PK, but it's not really clear to me if that's something we want to (or even can!) do now with a compile time option, as opposed to 4.0 as part of bigger changes (either removing PK or reworking it).

• The function [mbedtls_pk_can_do_ext] can be implemented on top of standard PSA functions, but at a significant CPU cost: the standard way to test if a key can do something is to actually attempt to do it.

That's a problem for TLS and I suspect for any protocol that does similar negotiation: TLS needs to know in advance what operations it will be able to do with the provisioned private key(s), in order to offer the corresponding algorithms. I think PSA Crypto should really offer a standard, non-CPU-intensive way of doing that. (The alternative would be to require TLS users to provide the information to the TLS stack, but then users would complain that it's silly because the information is right there in the key, and I'd agree with them.) (That's probably a separate discussion though.)

• In mbedtls_pk_wrap_as_opaque: that function should take an attribute structure as argument rather than a few of its fields. That's an API change, but that's ok since we document this function as experimental. This function is one we need to stabilize in some form in 3.6 for it to be viable to work with PSA keys anyway.

Agree. I'll note that in a way it will move the call out of the library and into the user's hands: people (like us in our SSL test programs like ssl_server2) who rely on being able to do more than one alg with the same key will still have to use set_key_enrollment_algorithm() to prepare the attributes structure to pass to the new pk_wrap_as_opaque() function.

But in this case, moving the call out of the library and into the user's hands is exactly what we want: users get to decide whether to use the non-standard function or not.

• In pk_parse_key_sec1_der: this is a hack [...]

Agreed. And this kind of hack doesn't work for RSA where we could possibly want v15-crypt, v15-sign and pss-sign if we wanted the key to be usable for all the key exchanges we support in TLS 1.2+1.3. Right know I don't remember how we handled that in our SSL test programs

It's weird that parsing code is concerned about policy, but I suspect that it needs this to preserve compatibility with the pk interface

Precisely. When you parse an EC key, you get an ECKEY which as you say corresponds to an unrestricted ECC key, and we wanted to preserve that.

We could probably get away with allowing only (deterministic) ECDSA, since PK can't do ECDH anyway, so in order to to ECDH we need to somehow get the key out of the PK context (this happens in TLS 1.2). At the point we do that, we could export the key and re-import it with an ECDH policy, instead of just extracting the key ID from the PK context - after all, in the !MBEDTLS_PK_USE_PSA_EC_DATA we already do mbedtls_ecp_write_key() + psa_import_key() so we could also do psa_export_key() + psa_import_key() here.

That would move the complexity/hack out or PKparse and into TLS 1.2, but more importantly it would avoid use of a non-standard function in either place.

In ecdsa_sign_psa, but that code is just overly complicated: it does some complex analysis to decide between randomized and deterministic ECDSA, but it should instead just try psa_sign_hash twice.

Ah, good point.

[mpg]

We can and should remove some of those uses, but I'm not sure if we can remove all of them.

* In [`mbedtls_pk_can_do_ext`](https://github.com/Mbed-TLS/mbedtls/blob/mbedtls-3.5.0/library/pk.c#L335): 

I'm thinking here we could just guard the call to that function with #if defined(MBEDTLS_PSA_CRYPTO_CLIENT) - which indicates that the Mbed TLS implementation of PSA Crypto is being used. (With a comment explaining that while we don't officially support using other implementations of PSA Crypto with TLS & X.509 now, we're still trying to simplify the life of people who would like to try it before it's officially supported, or something to that effect.)

I've created #8602 based on that - please shout if you disagree!

* In [`mbedtls_pk_wrap_as_opaque`](https://github.com/Mbed-TLS/mbedtls/blob/mbedtls-3.5.0/library/pk.c#L869): 

I think that will be done as part of #7760.

* In [`pk_parse_key_sec1_der`](https://github.com/Mbed-TLS/mbedtls/blob/mbedtls-3.5.0/library/pkparse.c#L1284): 

Now tracked in #8601

* In [`ecdsa_sign_psa`](https://github.com/Mbed-TLS/mbedtls/blob/564bc1bb960cd4d67a3d58488fff53565cf74ef2/library/pk_wrap.c#L989), but that code is just overly complicated: it does some complex analysis to decide between randomized and deterministic ECDSA, but it should instead just try `psa_sign_hash` twice.

Now tracked in #8600

[mpg]

I believe we have an agreement that we don't want to introduce a new symbol at this point, but the PR and discussion have highlighted other changes we would like to make. I believe all those changes are now tracked in separate issues, so I'm closing this PR.