mbedtls: configurable PSA ITS provider

[JordanYates]

Make the PSA ITS provider configurable. It defaults to the Zephyr secure storage subsystem as per currently (when TF-M is disabled), but allows overriding to the the MbedTLS file implementations on ARCH_POSIX.

Alternative implementation to #85840, nominally decoupled from the secure storage subsystem.

This is a better solution since the API being implemented by mbedtls is the PSA ITS API, not the zephyr secure storage API.
Similarly to secure storage this copies the internal_trusted_storage.h header from MbedTLS so that it is available to applications, but without the additions that subsystem makes.

[tomi-font]

Getting there! I think if allowing Mbed TLS to use its own ITS implementation for persistent keys we shouldn't try to expose it to other code (as it's not needed) and so shouldn't talk of PSA ITS at all in that case, rather of PSA Crypto persistent keys.

[tomi-font]

In your other PR you were removing imply/select statements but here you are introducing some?

[JordanYates]

MBEDTLS is select'd in a whole range of code where it really shouldn't be, but again the more immediate problem is the inconsistency between depends on and select.

[tomi-font]

In some places we also have depends on MBEDTLS. Why not instead just have depends on for everything here? I don't really get your rationale here.

[JordanYates]

Because your current secure storage backends end up enabling FLASH_AREA_CHECK_INTEGRITY, where one of the choices does select MBEDTLS, attempting to use depends on MBEDTLS results in Kconfig dependency loops.

[JordanYates]

Or there was some link over a long chain of options that ended up back there (probably through config HAWKBIT). I forget the exact chain

[tomi-font]

Because your current secure storage backends end up enabling FLASH_AREA_CHECK_INTEGRITY, where one of the choices does select MBEDTLS, attempting to use depends on MBEDTLS results in Kconfig dependency loops.

Ah okay. Yeah that's annoying. Something likely needs to be reworked but that's out of scope.

[tomi-font]

I am really not a fan of copying header files, especially considering we already have a psa/internal_trusted_storage.h in Zephyr. (And it was not copied from Mbed TLS, it was made from the PSA Secure Storage spec.)

Mbed TLS has made it private probably for a good reason. And they have no plans whatsoever to make it public.
Allowing to use the Mbed TLS ITS implementation directly doesn't seem quite right if it hasn't been made available by the library itself (through an exposed header file).
Also, as per your comment here all you really care about is persistent keys in the PSA Crypto API. For that we don't need to be able to call the psa_its_*() functions directly, we just need to tell Mbed TLS to use its own internal implementation.

[tomi-font]

I think it makes sense to turn those into Kconfig options. Especially PSA_CRYPTO_STORAGE_C (which I am adding in this PR of mine, feel free to take the changes into your PR) and MBEDTLS_PSA_ITS_FILE_C (which could then be used as a condition for SECURE_STORAGE).

[tomi-font]

It seems those choice options don't really bring value. I think we could just have the option to enable the persistent keys in the Mbed TLS PSA Crypto core (by enabling its own internal ITS implementation).
Though that would mean introducing a (helper) Kconfig option that wouldn't have a corresponding build symbol in Mbed TLS.
The alternative would be to just expose the 3 Mbed TLS defines that are needed as Kconfig options and let whoever wants to use that enable them.

[JordanYates]

The thing the application cares about is support for the PSA key operations (psa_generate_key, etc).
To support PSA_KEY_LIFETIME_PERSISTENT, there needs to be a single implementation of the PSA ITS API.
That seems like the perfect use of a choice symbol to me, even if some options are only used implicitly (MBEDTLS_PSA_ITS_BACKEND_TFM), since Kconfig is now doing the work of ensuring the mutual exclusion.
Otherwise when implementation N of the ITS comes along, every other implementation needs to have a depends on !ITS_N added.

[tomi-font]

I see the logic, but I don't think we'll see any other ITS implementation come along, and all we really need here is a single Kconfig option to handle the Mbed TLS ITS implementation you want to make available.
Also for example for the TF-M case it's more complex than that. TF-M could be enabled but without the ITS partition, which means there wouldn't be ITS even though there is TF-M.
As of now I don't see benefits in having a choice as the TF-M and secure storage options are already handled by preexisting configuration.

[JordanYates]

Maybe not upstream, but there are enough problems currently with the Secure Storage API (see #86177) that in the short term at least my downstream is more likely to implement the ITS API than the Secure Storage one.

[tomi-font]

In some places we also have depends on MBEDTLS. Why not instead just have depends on for everything here? I don't really get your rationale here.

[tomi-font]

I think this should also depend on !SECURE_STORAGE.

[tomi-font]

I see the logic, but I don't think we'll see any other ITS implementation come along, and all we really need here is a single Kconfig option to handle the Mbed TLS ITS implementation you want to make available.
Also for example for the TF-M case it's more complex than that. TF-M could be enabled but without the ITS partition, which means there wouldn't be ITS even though there is TF-M.
As of now I don't see benefits in having a choice as the TF-M and secure storage options are already handled by preexisting configuration.

[valeriosetti]

Technically speaking the PR might be OK, but to me this is a bit overkilling.
I'm requesting changes on this PR because I would like to know also @tomi-font opinion on the following points since he's the maintainer of the secure storage subsystem.

IMO the main goal of secure storage is to provide data security. I know that the final security level depends on the platform's capabilities (ex: an HW key management unit), but even the default implementation provides some sort of security out of the box.
The Mbed TLS one instead (MBEDTLS_PSA_CRYPTO_STORAGE_C + MBEDTLS_PSA_ITS_FILE_C) does not. So I really can't think why the user would prefer the Mbed TLS solution over the current secure storage one.

The only reason I can think of is that on native_sim the user can store keys as file on the host PC without any need to emulate flash memory. But if that's the purpose, then can't we get the same using the file backend with the settings subsystem?
Of course the plus with this solution is that data is also encrypted in the file.

If instead the reason is that you want to run a Zephyr app using Mbed TLS on native_sim and you don't care about key data encryption, then why don't simply add a new Mbed TLS Kconfig in Kconfig.tls-generic that enables MBEDTLS_PSA_CRYPTO_STORAGE_C + MBEDTLS_PSA_ITS_FILE_C + MBEDTLS_FS_IO)?