[ICD] Remove PSA Crypto exclusion from Unit test (project-chip#29753)

* Remove PSA Crypto exclusion

* fix PSA test failure

* Add more comments

* Fix comments

* Add isKeySet flag

* address comments

src/app/icd/ICDManagementServer.cpp

@@ -39,7 +39,12 @@ Status ICDManagementServer::RegisterClient(PersistentStorageDelegate & storage,
     // Save
     entry.checkInNodeID    = node_id;
     entry.monitoredSubject = monitored_subject;
-    err                    = entry.SetKey(key);
+    if (entry.keyHandleValid)
+    {
+        entry.DeleteKey();
+    }
+
+    err = entry.SetKey(key);
     VerifyOrReturnError(CHIP_ERROR_INVALID_ARGUMENT != err, InteractionModel::Status::ConstraintError);
     VerifyOrReturnError(CHIP_NO_ERROR == err, InteractionModel::Status::Failure);
     err = table.Set(entry.index, entry);

src/app/icd/ICDMonitoringTable.cpp

@@ -72,7 +72,12 @@ CHIP_ERROR ICDMonitoringEntry::Deserialize(TLV::TLVReader & reader)
             case to_underlying(Fields::kKey): {
                 ByteSpan buf(key.AsMutable<Crypto::Aes128KeyByteArray>());
                 ReturnErrorOnFailure(reader.Get(buf));
-                ReturnErrorOnFailure(this->SetKey(buf));
+                // Since we are storing either the raw key or a key ID, we must
+                // simply copy the data as is in the keyHandle.
+                // Calling SetKey here would create another key in storage and will cause
+                // key leakage in some implementation.
+                memcpy(key.AsMutable<Crypto::Aes128KeyByteArray>(), buf.data(), sizeof(Crypto::Aes128KeyByteArray));
+                keyHandleValid = true;
             }
             break;
             default:
@@ -90,24 +95,20 @@ void ICDMonitoringEntry::Clear()
 {
     this->checkInNodeID    = kUndefinedNodeId;
     this->monitoredSubject = kUndefinedNodeId;
-    if (symmetricKeystore != nullptr)
-    {
-        symmetricKeystore->DestroyKey(this->key);
-    }
+    this->keyHandleValid   = false;
 }
 
 CHIP_ERROR ICDMonitoringEntry::SetKey(ByteSpan keyData)
 {
     VerifyOrReturnError(keyData.size() == sizeof(Crypto::Aes128KeyByteArray), CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(symmetricKeystore != nullptr, CHIP_ERROR_INTERNAL);
-
-    // DeleteKey in case the handle was already used.
-    DeleteKey();
+    VerifyOrReturnError(!keyHandleValid, CHIP_ERROR_INTERNAL);
 
     Crypto::Aes128KeyByteArray keyMaterial;
     memcpy(keyMaterial, keyData.data(), sizeof(Crypto::Aes128KeyByteArray));
 
     ReturnErrorOnFailure(symmetricKeystore->CreateKey(keyMaterial, key));
+    keyHandleValid = true;
 
     return CHIP_NO_ERROR;
 }
@@ -116,13 +117,15 @@ CHIP_ERROR ICDMonitoringEntry::DeleteKey()
 {
     VerifyOrReturnError(symmetricKeystore != nullptr, CHIP_ERROR_INTERNAL);
     symmetricKeystore->DestroyKey(this->key);
+    keyHandleValid = false;
     return CHIP_NO_ERROR;
 }
 
 bool ICDMonitoringEntry::IsKeyEquivalent(ByteSpan keyData)
 {
     VerifyOrReturnValue(keyData.size() == Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES, false);
     VerifyOrReturnValue(symmetricKeystore != nullptr, false);
+    VerifyOrReturnValue(keyHandleValid, false);
 
     ICDMonitoringEntry tempEntry(symmetricKeystore);
 
@@ -187,6 +190,7 @@ CHIP_ERROR ICDMonitoringTable::Set(uint16_t index, const ICDMonitoringEntry & en
     VerifyOrReturnError(index < this->Limit(), CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(kUndefinedNodeId != entry.checkInNodeID, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(kUndefinedNodeId != entry.monitoredSubject, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(entry.keyHandleValid, CHIP_ERROR_INVALID_ARGUMENT);
     ICDMonitoringEntry e(this->mFabric, index);
     e.checkInNodeID    = entry.checkInNodeID;
     e.monitoredSubject = entry.monitoredSubject;
@@ -209,7 +213,7 @@ CHIP_ERROR ICDMonitoringTable::Remove(uint16_t index)
     // Shift remaining entries down one position
     while (CHIP_NO_ERROR == this->Get(static_cast<uint16_t>(index + 1), entry))
     {
-        this->Set(index++, entry);
+        ReturnErrorOnFailure(this->Set(index++, entry));
     }
 
     // Remove last entry

src/app/icd/ICDMonitoringTable.h

@@ -59,6 +59,22 @@ struct ICDMonitoringEntry : public PersistentData<kICDMonitoringBufferSize>
     CHIP_ERROR Serialize(TLV::TLVWriter & writer) const override;
     CHIP_ERROR Deserialize(TLV::TLVReader & reader) override;
     void Clear() override;
+    /**
+     * @brief Set the Key object
+     *        This method will create a new keyHandle. The key handle might contain either
+     *        the raw key or a keyID depending on which Crypto implementation is used.
+     *        To avoid leaking keys, API consumers must either call the DeleteKey method
+     *        or save the entry within the ICDMonitoring Table before this object goes out of scope.
+     *
+     *        A new entry object should be used for each key when adding entries to the ICDMonitoring
+     *        table.
+     *
+     * @param keyData A byte span containing the raw key
+     * @return CHIP_ERROR CHIP_NO_ERROR     success
+     *         CHIP_ERROR_INVALID_ARGUMENT  wrong size of the raw key
+     *         CHIP_ERROR_INTERNAL          No KeyStore for the entry or Key Handle already present
+     *         CHIP_ERROR_XXX               Crypto API related failure
+     */
     CHIP_ERROR SetKey(ByteSpan keyData);
     CHIP_ERROR DeleteKey(void);
 
@@ -83,6 +99,7 @@ struct ICDMonitoringEntry : public PersistentData<kICDMonitoringBufferSize>
     chip::NodeId checkInNodeID                    = kUndefinedNodeId;
     uint64_t monitoredSubject                     = static_cast<uint64_t>(0);
     Crypto::Aes128KeyHandle key                   = Crypto::Aes128KeyHandle();
+    bool keyHandleValid                           = false;
     uint16_t index                                = 0;
     Crypto::SymmetricKeystore * symmetricKeystore = nullptr;
 };