Code Review:

- Now, operational keys are migrated on Case establishment per each fabric, not for each fabric at the Server start. - Removed BufferWriter from PersistentP256Keypair::Deserialize method - Added suggested comments to PersistentStorageOperationalKeystore. - Now the migration depends only on the Case session, not on Server init so if something goes wrong the Case session will not be established.
ArekBalysNordic · Jan 9, 2024 · b33541c · b33541c
1 parent 565ed79
commit b33541c
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 31 deletions.
diff --git a/src/crypto/PSAOperationalKeystore.cpp b/src/crypto/PSAOperationalKeystore.cpp
@@ -27,7 +27,9 @@ namespace chip {
 namespace Crypto {
 
 namespace {
-// Tags for our operational keypair storage copied from PersistentStorageOperationalKeystore.cpp
+// Tags and version of operational keypair storage copied from PersistentStorageOperationalKeystore.cpp
+// These values must be compatible with values in the PersistentStorageOperationalKeystore.cpp file to perform a proper keys
+// migration.
 constexpr TLV::Tag kOpKeyVersionTag = TLV::ContextTag(0);
 constexpr TLV::Tag kOpKeyDataTag    = TLV::ContextTag(1);
 constexpr uint16_t kOpKeyVersion    = 1;
@@ -111,6 +113,9 @@ bool PSAOperationalKeystore::HasOpKeypairForFabric(FabricIndex fabricIndex) cons
 {
     VerifyOrReturnError(IsValidFabricIndex(fabricIndex), false);
 
+    // check if the key exists in the PersistentStorageOperationalKeystore exist, and if so move it to the PSA ITS secure storage.
+    VerifyOrReturnError(CHIP_NO_ERROR == MoveOpKeysFromKVSToITS(fabricIndex), false);
+
     if (mPendingFabricIndex == fabricIndex)
     {
         return mIsPendingKeypairActive;
@@ -150,7 +155,7 @@ CHIP_ERROR PSAOperationalKeystore::PersistentP256Keypair::Deserialize(P256Serial
     psa_status_t status             = PSA_SUCCESS;
     psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
     psa_key_id_t keyId              = 0;
-    Encoding::BufferWriter bbuf(mPublicKey, mPublicKey.Length());
+    VerifyOrReturnError(input.Length() == mPublicKey.Length() + kP256_PrivateKey_Length, CHIP_ERROR_INVALID_ARGUMENT);
 
     Destroy();
 
@@ -165,12 +170,7 @@ CHIP_ERROR PSAOperationalKeystore::PersistentP256Keypair::Deserialize(P256Serial
     status = psa_import_key(&attributes, input.ConstBytes() + mPublicKey.Length(), kP256_PrivateKey_Length, &keyId);
     VerifyOrExit(status == PSA_SUCCESS, error = CHIP_ERROR_INTERNAL);
 
-    bbuf.Put(input.ConstBytes(), mPublicKey.Length());
-    if (!bbuf.Fit())
-    {
-        psa_destroy_key(keyId);
-        error = CHIP_ERROR_NO_MEMORY;
-    }
+    memcpy(mPublicKey.Bytes(), input.ConstBytes(), mPublicKey.Length());
 
 exit:
     psa_reset_key_attributes(&attributes);
@@ -221,6 +221,8 @@ CHIP_ERROR PSAOperationalKeystore::SignWithOpKeypair(FabricIndex fabricIndex, co
                                                      Crypto::P256ECDSASignature & outSignature) const
 {
     VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX);
+    // Check if the key already exists and need to be migrated to PSA ITS
+    VerifyOrReturnError(HasOpKeypairForFabric(fabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX);
 
     if (mPendingFabricIndex == fabricIndex)
     {
@@ -252,25 +254,18 @@ void PSAOperationalKeystore::ReleasePendingKeypair()
     mIsPendingKeypairActive = false;
 }
 
-CHIP_ERROR PSAOperationalKeystore::MoveOpKeysFromKVSToITS(PersistentStorageDelegate * storage)
+CHIP_ERROR PSAOperationalKeystore::MoveOpKeysFromKVSToITS(FabricIndex fabricIndex) const
 {
     Crypto::SensitiveDataBuffer<TLV::EstimateStructOverhead(sizeof(uint16_t), P256SerializedKeypair::Capacity())> buf;
     uint16_t keySize = static_cast<uint16_t>(buf.Capacity());
 
-    for (FabricIndex index = 0; index < CHIP_CONFIG_MAX_FABRICS; index++)
+    if (CHIP_NO_ERROR ==
+        mStorage->SyncGetKeyValue(DefaultStorageKeyAllocator::FabricOpKey(fabricIndex).KeyName(), buf.Bytes(), keySize))
     {
-        CHIP_ERROR err = storage->SyncGetKeyValue(DefaultStorageKeyAllocator::FabricOpKey(index).KeyName(), buf.Bytes(), keySize);
-        if (CHIP_NO_ERROR == err)
+        PersistentP256Keypair keypair(fabricIndex);
+        // Do not allow overwriting the existing key and just remove it from KVS
+        if (!keypair.Exists())
         {
-            PersistentP256Keypair keyPair(index);
-
-            // Do not allow overwriting the existing key and just remove it from KVS
-            if (keyPair.Exists())
-            {
-                ReturnErrorOnFailure(storage->SyncDeleteKeyValue(DefaultStorageKeyAllocator::FabricOpKey(index).KeyName()));
-                continue;
-            }
-
             // Read the operational private and public keys from the TLV entry
             buf.SetLength(static_cast<size_t>(keySize));
             TLV::ContiguousBufferTLVReader reader;
@@ -291,13 +286,18 @@ CHIP_ERROR PSAOperationalKeystore::MoveOpKeysFromKVSToITS(PersistentStorageDeleg
 
             if (keyData.size() == kP256_PrivateKey_Length + kP256_PublicKey_Length)
             {
+                // Migrate the operational keys
                 P256SerializedKeypair serializedKeypair;
                 ReturnErrorOnFailure(serializedKeypair.SetLength(kP256_PrivateKey_Length + kP256_PublicKey_Length));
                 memcpy(serializedKeypair.Bytes(), keyData.data(), kP256_PrivateKey_Length + kP256_PublicKey_Length);
-                ReturnErrorOnFailure(keyPair.Deserialize(serializedKeypair));
-                ReturnErrorOnFailure(storage->SyncDeleteKeyValue(DefaultStorageKeyAllocator::FabricOpKey(index).KeyName()));
+                ReturnErrorOnFailure(keypair.Deserialize(serializedKeypair));
+                ReturnErrorOnFailure(mStorage->SyncDeleteKeyValue(DefaultStorageKeyAllocator::FabricOpKey(fabricIndex).KeyName()));
             }
         }
+        else
+        {
+            ReturnErrorOnFailure(mStorage->SyncDeleteKeyValue(DefaultStorageKeyAllocator::FabricOpKey(fabricIndex).KeyName()));
+        }
     }
 
     return CHIP_NO_ERROR;

diff --git a/src/crypto/PSAOperationalKeystore.h b/src/crypto/PSAOperationalKeystore.h
@@ -28,18 +28,22 @@ class PSAOperationalKeystore final : public OperationalKeystore
 {
 public:
     /**
-     * @brief Initialize the PSA Operational Keystore and move all sensitive keys from storage to ITS secure storage.
+     * @brief Initialize the PSA Operational Keystore to save the persistent storage delegate.
      *
-     * After moving all sensitive keys to ITS, the old one stored in the storage will be removed and not available anymore.
+     * The PersistentStorageDelegate object will be needed to perform operational keys migration from the Persistent Storage
+     * Operational Keystore database to PSA ITS secure storage.
      *
-     * @param storage Pointer to persistent storage delegate to use.
-     * @retval CHIP_NO_ERROR on success
-     * @retval CHIP_ERROR_INCORRECT_STATE if already initialized
+     * NOTE: The migration of the operational keys will occur on each HasOpKeypairForFabric method call for specific fabric index.
+     *
+     * @param storage Pointer to persistent storage delegate to use. Must outlive this instance.
+     * @retval CHIP_NO_ERROR on success.
+     * @retval CHIP_ERROR_INCORRECT_STATE if already initialized.
      */
     CHIP_ERROR Init(PersistentStorageDelegate * storage)
     {
-        VerifyOrReturnError(storage != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-        return MoveOpKeysFromKVSToITS(storage);
+        VerifyOrReturnError(mStorage == nullptr, CHIP_ERROR_INCORRECT_STATE);
+        mStorage = storage;
+        return CHIP_NO_ERROR;
     }
 
     bool HasPendingOpKeypair() const override;
@@ -77,9 +81,10 @@ class PSAOperationalKeystore final : public OperationalKeystore
     PersistentP256Keypair * mPendingKeypair = nullptr;
     FabricIndex mPendingFabricIndex         = kUndefinedFabricIndex;
     bool mIsPendingKeypairActive            = false;
+    PersistentStorageDelegate * mStorage    = nullptr;
 
 private:
-    CHIP_ERROR MoveOpKeysFromKVSToITS(PersistentStorageDelegate * storage);
+    CHIP_ERROR MoveOpKeysFromKVSToITS(FabricIndex fabricIndex) const;
 };
 
 } // namespace Crypto

diff --git a/src/crypto/PersistentStorageOperationalKeystore.cpp b/src/crypto/PersistentStorageOperationalKeystore.cpp
@@ -34,11 +34,15 @@ using namespace chip::Crypto;
 namespace {
 
 // Tags for our operational keypair storage.
+// If the tags changes, change also the copy in the PSAOperationalKeystore.cpp to keep migration of the operational keys to
+// PSA ITS properly
 constexpr TLV::Tag kOpKeyVersionTag = TLV::ContextTag(0);
 constexpr TLV::Tag kOpKeyDataTag    = TLV::ContextTag(1);
 
 // If this version grows beyond UINT16_MAX, adjust OpKeypairTLVMaxSize
 // accordingly.
+// If this version changes, change also the copy in the PSAOperationalKeystore.cpp to keep migration of the operational keys to
+// PSA ITS properly
 constexpr uint16_t kOpKeyVersion = 1;
 
 constexpr size_t OpKeyTLVMaxSize()