secure_storage: re-add MbedTLS backend

[JordanYates]

Re-add the option to use the MbedTLS ITS backend on posix. This backend is much simpler to setup than the Zephyr backends in a testing environment. This was previously removed in: #82319

[valeriosetti]

I only left a minor comment/suggestion for not increasing the mess in config-tls-generic.h further.

[valeriosetti]

Question: can't we make a new Mbed TLS specific Kconfig to be used as guard here instead of using a secure-storage one? Something like CONFIG_MBEDTLS_ITS_FILE or similar for example.

[JordanYates]

Of course we could, but that symbol would just be:

config MBEDTLS_ITS_FILE
    bool
    default y if SECURE_STORAGE_ITS_IMPLEMENTATION_MBEDTLS

It just smears the definitions across more files.

[valeriosetti]

True, but then you would have secure-storage Kconfig enabling an Mbed TLS Kconfig which then "internally" sets the build symbols to build Mbed TLS. In other words, in this way we would keep relations between different subsystems/modules at Kconfig level. It just looks cleaner to me, but I guess it's a matter of preference, so I'll let the maintaner decide what to do.

[JordanYates]

Happy to change the symbols if needed, but literally 2 lines above my change there is already a:

#if defined(CONFIG_SECURE_STORAGE)
#define MBEDTLS_PSA_CRYPTO_STORAGE_C
#endif

[valeriosetti]

Oh damn, that's right. Then I guess that it will take one or more specific PRs to fix all the mess in that header file, but that's out of scope for this one. Please ignore my comment and sorry for the noise.

[tomi-font]

FYI I am changing that in #85735 (introducing CONFIG_MBEDTLS_PSA_CRYPTO_STORAGE_C).

[alxelax]

@JordanYates, just FYI, that we faced using mbedtls posix backend. This is not thread safe. If you have multiple instances on your test PC all of them compete for the same file with keys. There are key ID collisions those misbehave test scenarios.
I created bug report about this: #59362 but seems this is not priority, and it was closed as Stale.
In mesh we had to use Zephyr backends in secure storage to not have this misbehavior.
Furthermore, we removed (and continue removing: #85069) all technological stubs to run Mesh bsim tests on native Zephyr secure storage backend to improve test coverage.

do not impact mesh

[JordanYates]

@JordanYates, just FYI, that we faced using mbedtls posix backend. This is not thread safe. If you have multiple instances on your test PC all of them compete for the same file with keys. There are key ID collisions those misbehave test scenarios. I created bug report about this: #59362 but seems this is not priority, and it was closed as Stale. In mesh we had to use Zephyr backends in secure storage to not have this misbehavior. Furthermore, we removed (and continue removing: #85069) all technological stubs to run Mesh bsim tests on native Zephyr secure storage backend to improve test coverage.

Thats interesting. Wouldn't that also apply to the secure storage backend that uses the settings subsystem and the file backend?

[alxelax]

@JordanYates, just FYI, that we faced using mbedtls posix backend. This is not thread safe. If you have multiple instances on your test PC all of them compete for the same file with keys. There are key ID collisions those misbehave test scenarios. I created bug report about this: #59362 but seems this is not priority, and it was closed as Stale. In mesh we had to use Zephyr backends in secure storage to not have this misbehavior. Furthermore, we removed (and continue removing: #85069) all technological stubs to run Mesh bsim tests on native Zephyr secure storage backend to improve test coverage.

Thats interesting. Wouldn't that also apply to the secure storage backend that uses the settings subsystem and the file backend?

I'm not familiar with how the settings subsystem uses the file in form of backends. If this is not instantiated, then the issue is still possible.

[tomi-font]

I don't know, this feels like a step backwards.

The Mbed TLS ITS implementation has never been part of the secure storage subsystem. It was a kludge for BT Mesh testing that only lived in the Mbed TLS configuration file.

What is the problem with using secure storage on POSIX exactly? As @alxelax said it improves test coverage. Going for the Mbed TLS ITS implementation comes with its own problems and basically circumvents the whole secure storage subsystem.

(On top of that, I don't see how this even works in practice. From what header file do you get your PSA ITS function prototypes? But anyway this is not the question here.)

[tomi-font]

FYI I am changing that in #85735 (introducing CONFIG_MBEDTLS_PSA_CRYPTO_STORAGE_C).

[JordanYates]

I don't know, this feels like a step backwards.

The Mbed TLS ITS implementation has never been part of the secure storage subsystem. It was a kludge for BT Mesh testing that only lived in the Mbed TLS configuration file.

What is the problem with using secure storage on POSIX exactly? As @alxelax said it improves test coverage. Going for the Mbed TLS ITS implementation comes with its own problems and basically circumvents the whole secure storage subsystem.

(On top of that, I don't see how this even works in practice. From what header file do you get your PSA ITS function prototypes? But anyway this is not the question here.)

The MbedTLS ITS implementation enables PSA_KEY_LIFETIME_PERSISTENT using the standard PSA API's without any other dependencies. e.g.

		/* ECDH, Curve25519 */
		psa_set_key_usage_flags(&key_attributes, PSA_KEY_USAGE_DERIVE);
		psa_set_key_type(&key_attributes, PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_MONTGOMERY));
		psa_set_key_algorithm(&key_attributes, PSA_ALG_ECDH);
		psa_set_key_bits(&key_attributes, 255);

		psa_set_key_lifetime(&key_attributes, PSA_KEY_LIFETIME_PERSISTENT);
		psa_set_key_id(&key_attributes, INFUSE_ROOT_ECC_KEY_ID);

		status = psa_generate_key(&key_attributes, &key_id);

Without the MbedTLS ITS implementation, that code snippets fails due to PSA_KEY_LIFETIME_PERSISTENT not being supported. Making it work with the Zephyr secure storage backend means either enabling and initializing the settings subsystem (which is not otherwise used in my applications and conflicts with NVS anyway) or the newer ZMS backend (which I suspect also conflicts with NVS).

[tomi-font]

The MbedTLS ITS implementation enables PSA_KEY_LIFETIME_PERSISTENT using the standard PSA API's without any other dependencies. e.g.

Yeah okay, it will allow you to use the ITS indirectly (through persistent keys in the PSA Crypto API) because Mbed TLS will use its internal psa_crypto_its.h, but trying to use the ITS or PS functions directly won't play nice.

Without the MbedTLS ITS implementation, that code snippets fails due to PSA_KEY_LIFETIME_PERSISTENT not being supported. Making it work with the Zephyr secure storage backend means either enabling and initializing the settings subsystem (which is not otherwise used in my applications and conflicts with NVS anyway) or the newer ZMS backend (which I suspect also conflicts with NVS).

Of course PSA_KEY_LIFETIME_PERSISTENT is not supported without an ITS implementation, this is what it needs.
And no, the ZMS backend won't conflict. Not even with a different ZMS instance. It makes you define a separate partition dedicated to secure storage:

zephyr/subsys/secure_storage/Kconfig.its_store

Lines 20 to 21 in 45a54ba

	It needs a `secure_storage_its_partition` devicetree chosen property that points
	to a fixed storage partition that will be dedicated to the ITS. It has lower

[JordanYates]

Yeah okay, it will allow you to use the ITS indirectly (through persistent keys in the PSA Crypto API) because Mbed TLS will use its internal psa_crypto_its.h, but trying to use the ITS or PS functions directly won't play nice.

That's fine, storing a root identity is critical to a lot of my tests, using ITS and PS functions is not.

And no, the ZMS backend won't conflict. Not even with a different ZMS instance. It makes you define a separate partition dedicated to secure storage:

Thats fine, but for a lot of tests it's an additional overlay file and options, when the MbedTLS ITS implementation does everything the test needs by itself.

[JordanYates]

Perhaps an alternate path forward would be something along these lines?

choice PSA_PERSISTENT_KEY_STORAGE
 
config PSA_PERSISTENT_KEY_STORAGE_SECURE_STORAGE
    depends on X

config PSA_PERSISTENT_KEY_STORAGE_MBEDTLS_FILE
   depends on MBEDTLS && ARCH_POSIX

enchoice

[JordanYates]

Improved solution in #86184