Hkdf integration #383

kkasperczyk-no · 2024-02-09T10:49:43Z

Pulled the most recent changes in CHIPCryptoPAL to integrate usage of HKDF key handle instead of raw data.

…e array (#30802) * Rename aes key byte array to symmetric key byte array * Restyled by clang-format --------- Co-authored-by: Restyled.io <commits@restyled.io>

…829) * Rename Aes128KeyHandle to Aes128BitsKeyHandle * create foundation for HMAC algo * Restyled by clang-format * add missing getter * Fix set algo for handle * Apply suggestions from code review Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> --------- Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Add new API for HMAC with key handle * Rename Aes128BitsKeyHandle to Aes128KeyHandle * Rename Hmac128BitsKeyHandle to Hmac128KeyHandle * Replace virtual destructor with a protected one * key algo creation --------- Co-authored-by: Mathieu Kardous <mathieu.kardous@silabs.com>

…1311) * [crypto] Add HKDF key handle and use it during PASE Current SPAKE2+ interface assumes that raw shared secret is extracted and used by the application to derive session keys. This prevents using secure crypto APIs, such as PSA, to perform SPAKE2+ and do the key derivation in a secure environment, and isolate the application from key material. 1. Add Hkdf128KeyHandle type and add methods for deriving session keys from an HKDF key. 2. Change SPAKE2+ interface to return HKDF key handle instead of raw key secret. A similar approach can be taken to improve CASE security in the future though we would need 256-bit HKDF key support in such a case. * Change HKDF key handle to hold key of any length * Code review

src/crypto/PSASpake2p.cpp

LuDuda · 2024-02-12T09:21:22Z

As discussed F2F, this PR could come without the requested optimization. So for a time being, removing request change.

f2f

kkasperczyk-no · 2024-02-14T13:58:15Z

@LuDuda I pushed an updated version that does not require using oberon context. Please take a look once again.

Damian-Nordic

Reviewed the last commit only. Approved provided the 3 issues are fixed.

Damian-Nordic · 2024-02-14T13:58:59Z

src/platform/nrfconnect/CHIPPlatformConfig.h

@@ -40,7 +40,8 @@

 #ifdef CONFIG_CHIP_CRYPTO_PSA
 #define CHIP_CONFIG_SHA256_CONTEXT_SIZE sizeof(psa_hash_operation_t)
-#define CHIP_CONFIG_HKDF_KEY_HANDLE_CONTEXT_SIZE sizeof(psa_key_id_t)
+// Alignment to sizeof(PsaHkdfKeyHandle) from crypto/CHIPCryptoPALPSA.h.
+#define CHIP_CONFIG_HKDF_KEY_HANDLE_CONTEXT_SIZE (sizeof(psa_key_id_t) + sizeof(bool))


It will effectively be 8 due to padding

Right, but is it a problem? Or you mean updating the comment to be more precise?

A bit confusing because CHIP_CONFIG_HKDF_KEY_HANDLE_CONTEXT_SIZE is 5 and we cast the handle to a structure that is 8 bytes. But maybe I'm too paranoid so let's leave it as it is :). After all, the cast is statically verified so it should be safe.

Ah, now I see what you mean. Yeah, indeed it looks incorrect. The best option would be to use the sizeof(PsaHkdfKeyHandle), but include is not possible. Another option could be to declare the structure of the same content, as PsaHkdfKeyHandle, but I'm not sure. Maybe for now it's ok to leave it, as we are going to revert it soon.

If it works, then fine! But don't we have any macro or util to align to 8-bytes in Matter?

There are cpp macros to generally align the data structure, or return the aligned size of specific type, but to be honest I don't know the macro that would return aligned size from the numerical unaligned size.

src/crypto/PSASessionKeystore.cpp

src/crypto/PSASpake2p.cpp

ArekBalysNordic

Two minor comments, overall LGTM : :)

src/crypto/CHIPCryptoPALPSA.h

src/crypto/PSASpake2p.h

LuDuda

Looks solid! Good work.

src/crypto/CHIPCryptoPALPSA.h

LuDuda · 2024-02-15T08:17:07Z

src/crypto/PSASpake2p.cpp


-    memcpy(out, oberonCtx.shared, oberonCtx.hash_len / 2);
-    *out_len = oberonCtx.hash_len / 2;
+    kdfPtr.release();


I guess it's fine and more clean.. but do we need to release it explicitly?

Could you clarify? We have to release it to inform the UniquePtr that the memory will be deleted in some other place (by the DestroyKey method). If we won't do that, the UniquePtr will delete key just after we leave the scope, which is ok only if we faced an error.

LuDuda · 2024-02-15T08:23:43Z

src/platform/nrfconnect/CHIPPlatformConfig.h

@@ -40,7 +40,8 @@

 #ifdef CONFIG_CHIP_CRYPTO_PSA
 #define CHIP_CONFIG_SHA256_CONTEXT_SIZE sizeof(psa_hash_operation_t)
-#define CHIP_CONFIG_HKDF_KEY_HANDLE_CONTEXT_SIZE sizeof(psa_key_id_t)
+// Alignment to sizeof(PsaHkdfKeyHandle) from crypto/CHIPCryptoPALPSA.h.
+#define CHIP_CONFIG_HKDF_KEY_HANDLE_CONTEXT_SIZE (sizeof(psa_key_id_t) + sizeof(bool))


If it works, then fine! But don't we have any macro or util to align to 8-bytes in Matter?

* Aligned Spake2+ implementation with the new CHIPCryptoPAL API using HKDF key handle instead of raw data. * Removed workaround extracting raw key data from oberon context. * Removed const from GetKeys and DeriveSecureSession methods, as PSA API requires access to non-const members to obtains shared secret. Signed-off-by: Kamil Kasperczyk <kamil.kasperczyk@nordicsemi.no>

mkardous-silabs and others added 4 commits February 9, 2024 11:23

[nrf fromtree][Crypto] Rename aes key byte array to symmetric key byt…

f9a4827

…e array (#30802) * Rename aes key byte array to symmetric key byte array * Restyled by clang-format --------- Co-authored-by: Restyled.io <commits@restyled.io>

github-actions bot added platform nrf connect app lib core crypto transport protocols silabs icd labels Feb 9, 2024

kkasperczyk-no requested review from Damian-Nordic, ArekBalysNordic, LuDuda and markaj-nordic and removed request for Damian-Nordic February 9, 2024 14:27

NordicBuilder mentioned this pull request Feb 9, 2024

manifest: Updated Matter revision to pull HKDF crypto update nrfconnect/sdk-nrf#13944

Closed

LuDuda previously requested changes Feb 9, 2024

View reviewed changes

src/crypto/PSASpake2p.cpp Outdated Show resolved Hide resolved

kkasperczyk-no force-pushed the hkdf_integration branch from ee88af1 to efcdfd7 Compare February 14, 2024 13:55

kkasperczyk-no force-pushed the hkdf_integration branch from efcdfd7 to 2e3ca4c Compare February 14, 2024 14:08

Damian-Nordic approved these changes Feb 14, 2024

View reviewed changes

kkasperczyk-no force-pushed the hkdf_integration branch 2 times, most recently from 3a8936a to 2a4fb92 Compare February 15, 2024 07:16

kkasperczyk-no force-pushed the hkdf_integration branch from 2a4fb92 to 50b0fd7 Compare February 15, 2024 07:20

ArekBalysNordic approved these changes Feb 15, 2024

View reviewed changes

src/crypto/CHIPCryptoPALPSA.h Outdated Show resolved Hide resolved

src/crypto/PSASpake2p.h Outdated Show resolved Hide resolved

kkasperczyk-no force-pushed the hkdf_integration branch from 50b0fd7 to 4cd2da5 Compare February 15, 2024 08:02

LuDuda approved these changes Feb 15, 2024

View reviewed changes

kkasperczyk-no force-pushed the hkdf_integration branch from 4cd2da5 to 4bbfbfe Compare February 15, 2024 09:55

kkasperczyk-no merged commit 1fa4e7c into nrfconnect:master Feb 16, 2024
4 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hkdf integration #383

Hkdf integration #383

kkasperczyk-no commented Feb 9, 2024 •

edited

Loading

LuDuda commented Feb 12, 2024

kkasperczyk-no commented Feb 14, 2024

Damian-Nordic left a comment

Damian-Nordic Feb 14, 2024

kkasperczyk-no Feb 14, 2024

Damian-Nordic Feb 14, 2024 •

edited

Loading

kkasperczyk-no Feb 15, 2024

LuDuda Feb 15, 2024

kkasperczyk-no Feb 15, 2024

ArekBalysNordic left a comment

LuDuda left a comment

LuDuda Feb 15, 2024

kkasperczyk-no Feb 15, 2024 •

edited

Loading

LuDuda Feb 15, 2024

Hkdf integration #383

Hkdf integration #383

Conversation

kkasperczyk-no commented Feb 9, 2024 • edited Loading

LuDuda commented Feb 12, 2024

kkasperczyk-no commented Feb 14, 2024

Damian-Nordic left a comment

Choose a reason for hiding this comment

Damian-Nordic Feb 14, 2024

Choose a reason for hiding this comment

kkasperczyk-no Feb 14, 2024

Choose a reason for hiding this comment

Damian-Nordic Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

kkasperczyk-no Feb 15, 2024

Choose a reason for hiding this comment

LuDuda Feb 15, 2024

Choose a reason for hiding this comment

kkasperczyk-no Feb 15, 2024

Choose a reason for hiding this comment

ArekBalysNordic left a comment

Choose a reason for hiding this comment

LuDuda left a comment

Choose a reason for hiding this comment

LuDuda Feb 15, 2024

Choose a reason for hiding this comment

kkasperczyk-no Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

LuDuda Feb 15, 2024

Choose a reason for hiding this comment

kkasperczyk-no commented Feb 9, 2024 •

edited

Loading

Damian-Nordic Feb 14, 2024 •

edited

Loading

kkasperczyk-no Feb 15, 2024 •

edited

Loading