Clarify how Aes128KeyHandle works

[yunhanw-google]

          This is assuming, as part of this API, that the actual key bytes are stored in the key handle in the other info, no?  Why is this a good assumption?  It seems like it would preclude secure key stores...

And in particular, doesn't whether a keyhandle is in fact a Aes128KeyByteArray depend on the crypto implementation involved?

Or do I completely misunderstand how Aes128KeyHandle works?

@Damian-Nordic how can this be set up to work?

Followup is fine to sort this out.

_Originally posted by @bzbarsky-apple in https://github.com/project-chip/connectedhomeip/pull/29783#discussion_r1409830647_

[Damian-Nordic]

Aes128KeyHandle is an opaque type which can either hold Aes128KeyByteArray directly or it can hold just a key reference, in which case the key is stored securely in HSM or secure zone.

I agree with Boris' comment that the common code should not assume the key handle always holds a raw key. We expect that in the future there's going to be push towards better isolation of applications from security material, so it's worth designing interfaces allowing for that.

I think that you could probably write this code in a way that copying is not necessary. Perhaps, we could just make Aes128KeyHandle movable to be able to use ICDClientInfo in std::vector. Btw. we probably will need another implementation of the storage as we avoid using STL dynamic containers in embedded to keep to code size reasonable.

[bzbarsky-apple]

So right now ICD code assumes that As<Aes128KeyByteArray> works; e.g. see src/app/icd/ICDMonitoringTable.cpp and src/app/icd/ICDCheckInSender.cpp.

We need to figure out how to make those bits work without that, or to make it OK to do that, right?

In particular, unlike session keys the symmetric keys used for ICD need to be persistable somehow...

@Damian-Nordic @mkardous-silabs

[bzbarsky-apple]

Also, src/protocols/secure_channel/CheckinMessage.cpp has bits like:

ReturnErrorOnFailure(shaHandler.HMAC_SHA256(key.As<Aes128KeyByteArray>(), sizeof(Aes128KeyByteArray), appDataStartPtr,...

which assume that the raw key bytes are available, right?

[Damian-Nordic]

Hmm, right, this HMAC usage also looks incorrect. I think that we have the following options: a) easy, non-secure: Keep the ICD key as raw bytes in ICDCheckinSender and ICDClientInfo, and only import the key to the session keystore each time when a check-in message is sent (and then immediately remove it from the session keystore). It doesn't provide a good isolation of the keys but maybe for the ICD use-case this is acceptable, and for sure this is more correct than assuming that Aes128KeyHandle contains raw bytes.

b) hard, secure: Add a new HMAC key abstraction and then adjusts ICDClientStorage interface so that it supports key isolation. For example, let ICDClientStorage::StoreEntry take a raw key and output Aes128KeyHandle + HmacKeyHandle. Note that we cannot assume that the same key can be used for both AES and HMAC as e.g. PSA crypto API allows to associate only one algorithm with a single key: https://arm-software.github.io/psa-api/crypto/1.1/api/keys/policy.html#c.psa_set_key_algorithm.

If we have limited manpower, it may also be a good idea to go with a) for the time being and plan for b) in the future.

[jepenven-silabs]

Sooo For the HMAC part, seems like we are missing an API that takes the KeyHandle directly and do the conversion between KeyID for Hard Secure and Raw key depending on the Implementation specified for Crypto. For all other Crypto APIs using the keyhandle directly prevent this kind of issue.

Moreover as discussed on Slack, we need a way to copy/store/retrieve KeyHandle for Symmetric KeyStore. As of now there is nothing perfect for all parts of code that uses symmetric KeyStore with this implementation being the "Least worst"

E.G. For Groups Symmetric Keys are stored in Plain Text no matter way kind of Crypto Implementation you're using. At boot they are loaded from Persistent Storage and then a KeyHandle is created from the plain text and stored in RAM for the rest of the life of the program.

For Checkin Message, we are creating a new key from a PlainText data only once. Afterwards we are storing the content of the KeyHandle in Persistent storage and hacking our way at each load/save by copy pasting the content of one KeyHandle t another one. This works because we don't actually care if the content that we are copying is a KeyId or a Plain Text key.

However looks like some API absolutely needs a plain text key which is absurd and we definitively need to fix those api to make usage of a KeyHandle

[mkardous-silabs]

For the server side implementation for the ICD management of keys, these are the two issues i see

• Lifetime of the key is not set correctly which means PSA defaults to volatile storage when ICD keys need to be persisted
• When we create the key, handle is not configured to be used for HMAC. Need to add a new create handle

Based on this, i don't think we need to implemente a "new" key back end. We would just need to update the available APIs in the SessionKeystore. @Damian-Nordic @bzbarsky-apple opinions?