[nrfconnect] Add platform crypto for KMU usage

[ArekBalysNordic]

Let's review the code here first, before pushing it to the upstream repo.

Changes:

• Allowed setting a custom SessionKeystore in the Matter server.
• Implemented PSAKeyAllocator
  • If the custom implementation is not provided, the default one is used. The default implementation works in the same way as it was previously done.
  • It allows the injection of the custom implementation, which is crucial for KMU.
  • It allows us to overwrite the key attributes so we can modify them.
• Used the new PSAKeyAllocator in the PSAOperationalKeystore and PSASessionKeystore.
• Implemented KMUKeyAllocator to prepare all keys to be written in the proper slots.
• Overwritten the PersistICDKey method of the PSASessionKeystore class in the KMUSessionKeystore, because currently it is forbidden to use psa_copy_key to copy key from RAM to KMU slot.

[Damian-Nordic]

The solution is nice but I wonder if we can avoid this code duplication even more and also make it easier for customers to make a decision what to use the KMU slots for.

I have an alternative to propose but I'll let you decide which way to go :).

1. Add PSA key allocator interface:

   // PSAKeyAllocator.h
   class PSAKeyAllocator
   {
     virtual psa_key_id_t GetDacKeyId() = 0;
     virtual psa_key_id_t GetOpKeyId(FabricIndex) = 0;
     virtual psa_key_id_t AllocIcdKeyId() = 0;
     virtual psa_key_lifetime_t GetKeyLifetime(psa_key_id_t) = 0;
   };
   

2. Add GN switch chip_crypto_psa_key_allocator. Setting this switch would:
   • enable global SetPSAKeyAllocator and GetPSAKeyAllocator functions for setting a custom key allocator
   • modify existing PSAOperationalKeystore and PSASessionKeystore to use the custom key allocator instead of the default behavior.
3. For KMU support, implement this interface and set it before initializing CHIP.

The benefit is that if customers want to slightly modify the key allocation scheme (because e.g. they have their own keys to store in KMU and they don't care if ICD keys are in KMU or ITS), they don't need to make another copy of the key stores.

[ArekBalysNordic]

@Damian-Nordic I've introduced the new PSAKeyAllocator as you suggested. Thanks to that there is a reduced number of overwrites in our platform. Please take a look at that. Do you think we can go with something like that?

[Damian-Nordic]

Looks great. Some mostly minor suggestions from me.

[Damian-Nordic]

technically it's more like ICD slots per fabric than #slots per ICD key but that's fine :)

[ArekBalysNordic]

That's right, but it is consistent with NOC :) I will add a comment there.

[Damian-Nordic]

The export flag is only needed when creating volatile keys. Is export supported at all when using KMU? Or perhaps, it's not even checked? :)

[ArekBalysNordic]

Even if it is supported in KMU, indeed we should disable it while storing it there :)

[ArekBalysNordic]

I've added a change to the KMUSessionKeystore. Now the attributes for the new KMU keys don't contain these flags.

[Damian-Nordic]

Wonder if we want to base this scheme on the current configuration. If someone upgrades from non-ICD to ICD device, then the KMU slots will change their meaning. But I'm not sure if this scenario needs to be supported.

[ArekBalysNordic]

I'll discuss it with the team shortly if we even plan such a case.

[ArekBalysNordic]

@kkasperczyk-no @LuDuda what do you think? Should we always reserve slots for ICD to be ready for such a conversion?

[LuDuda]

We should somehow allow customer for upgradability (which of course is harder with KMU), even if customer will need to modify manually KMUKeyAllocator implementation to allocate keys from outside of the Matter range . We need to describe it in the documentation thoroughly. If i correctly reading the code, someone would be able to modify the code for it's own purpose and manage key_ids as he wants, right? Even for example to store ICD keys to ITS instead of KMUs (with known side-effects)?

The same issue is also with increasing number of supported Fabrics. So we need to make it clear in documentation later on, that changing these parameters have impact on KMU slot allocation. Wonder.. if we can somehow prevent issues which can be hard to catch during testing.

Not sure if we should always blindly reserve slots which will not be used.. Maybe we need F2F discussion. Is it possible to "move" keys from slot A to slot B?

[Damian-Nordic]

Thanks for the changes to reduce the duplicated code! Approving assuming you will be discussing the non-ICD to ICD upgrade in the team.

[LuDuda]

This is very clean! Great work 👍

Few nits and questions below.

[LuDuda]

So what is the key_id_t returned here? We are making assumption that HMAC(SHA256) is always returned value + 1? Maybe, we should return some structure here actually?

Just a thought from scalability perspective @ArekBalysNordic @Damian-Nordic ?

[ArekBalysNordic]

There is no such requirement. We save the key_id in zephyr settings, so no matter which slot it gets.

[LuDuda]

Let's discuss it F2F - not sure I understand how it works then if we want to make operation HMAC(SHA256). On which Key ID we do this? Even if it's stored in settings, how we get it's number?

[ArekBalysNordic]

During the ICD registration, we create two keys - HMAC(SHA256) and AES CCM. Originally, we put them into RAM and save their key IDs to Zephyr settings. If we need to use them, we get the key id from the specific Zephyr settings entry (used for ICD purposes) and provide it to PSA API. Recently we added moving these keys to PSA ITS (Now to KMU). Now during the ICD registration, we call an additional function "PersistICDKey" on each key and thanks to that we move the keys from RAM to ITS/KMU and we update the key ID saved in settings. So, to sum up, in settings we have something like a look-up table and key IDs assigned to specific ICD settings entries. Therefore, we don't need to take care of a specific slot for those keys, because we already have this information saved in zephyr settings. :)

[LuDuda]

This part i get, but i don't understand what is the Key ID for AES CCM and what is the Key ID for HMAC(SHA256)? Looking at the code, I assume the AllocateICDKeyId returns Key ID for AES CCM slot only - so what happens with HMAC(SHA256)? Or this is not needed to be persist?

[Damian-Nordic]

AllocateICDKeyId just allocates a key ID from a given range. The caller then uses the key ID to either create AES or HMAC key. It then stores in setting that for a given ICD entry, AES key ID is, say, 0x27 and HMAC key ID is, say, 0x32.

[ArekBalysNordic]

Fortunately, both AES and HMAC costs 1 KMU slot, so we can do it in the same way as for ITS. During the registration both keys are created one by one and uses the same allocate method. So Aes is created, allocate metod returns the first available key id within the given range, then HMAC is created and allocate method returns the next available key id in the same range. The difference is during the key creation. We use different attributes for each key, and then we don't need do check the key type during the allocation, just allocate the following ids.

[LuDuda]

Oh, right.. Somehow i thought that during allocation of the slot, we also do need to know/set a proper key_type, but it's more about just get an ID from a range. I think it works then fine.

[LuDuda]

Still wound't be more scalable to return struct with two key_ids already? As you mentioned it just happened that they both use 1 slot of KMU, what if this change for any reason in future? Else, i would expand the documentation to make it a bit more clear, maybe:

 * This method is used to allocate both AES-CCM and HMAC (SHA-256) keys independently.
 * The caller is responsible for storing the key ID in non-volatile memory
 * and setting the appropriate key type.

[ArekBalysNordic]

I will change the documentation then, and we can consider changing it to struct as a follow-up because it will require some changes in the Matter stack.

[LuDuda]

We should somehow allow customer for upgradability (which of course is harder with KMU), even if customer will need to modify manually KMUKeyAllocator implementation to allocate keys from outside of the Matter range . We need to describe it in the documentation thoroughly. If i correctly reading the code, someone would be able to modify the code for it's own purpose and manage key_ids as he wants, right? Even for example to store ICD keys to ITS instead of KMUs (with known side-effects)?

The same issue is also with increasing number of supported Fabrics. So we need to make it clear in documentation later on, that changing these parameters have impact on KMU slot allocation. Wonder.. if we can somehow prevent issues which can be hard to catch during testing.

Not sure if we should always blindly reserve slots which will not be used.. Maybe we need F2F discussion. Is it possible to "move" keys from slot A to slot B?

[LuDuda]

Looks good for me too. Let's discuss F2F the approach for upgradability.