Fix predicates for storage presence temporarily

issue project-chip#16958
tcarmelveilleux · Jun 23, 2022 · cbbf425 · cbbf425
1 parent 4935173
commit cbbf425
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 36 deletions.
diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
@@ -758,6 +758,9 @@ bool emberAfOperationalCredentialsClusterAddNOCCallback(app::CommandHandler * co
 exit:
     if (needRevert)
     {
+        // Here, on revert, we DO NOT call FabricTable::Delete as this would also remove the existing
+        // trusted root previously added. It possibly got reverted in case of the worst kinds of errors,
+        // but a better impl of the innards of FabricTable::CommitPendingFabricData would make it work.
         fabricTable.RevertPendingOpCertsExceptRoot();
 
         // Revert IPK and ACL entries added, ignoring errors, since some steps may have been skipped

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -164,14 +164,9 @@ CHIP_ERROR DeviceController::InitControllerNOCChain(const ControllerInitParams &
     //    serialize/deserialize.
     // 3) We have no keypair at all, and the fabric table has been initialized
     //    with a key store.
-    if (params.hasExternallyOwnedOperationalKeypair)
+    if (params.operationalKeypair != nullptr)
     {
-        hasExternallyOwnedKeypair  = true;
-        externalOperationalKeypair = params.operationalKeypair;
-    }
-    else if (params.operationalKeypair)
-    {
-        hasExternallyOwnedKeypair  = false;
+        hasExternallyOwnedKeypair  = params.hasExternallyOwnedOperationalKeypair;
         externalOperationalKeypair = params.operationalKeypair;
     }
 

diff --git a/src/credentials/PersistentStorageOpCertStore.cpp b/src/credentials/PersistentStorageOpCertStore.cpp
@@ -67,22 +67,15 @@ bool StorageHasCertificate(PersistentStorageDelegate * storage, FabricIndex fabr
         return false;
     }
 
-    uint16_t keySize = 0;
-    CHIP_ERROR err   = storage->SyncGetKeyValue(storageKey, nullptr, keySize);
+    // TODO(#16958): need to actually read the cert to know if it's there due to platforms not
+    //               properly enforcing CHIP_ERROR_BUFFER_TOO_SMALL behavior needed by
+    //               PersistentStorageDelegate.
+    uint8_t placeHolderCertBuffer[kMaxCHIPCertLength];
 
-    if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
-    {
-        // Obviously not found
-        return false;
-    }
-
-    if (err == CHIP_ERROR_BUFFER_TOO_SMALL)
-    {
-        // On found, we actually expect an "error", since we didn't want to read it out.
-        return true;
-    }
+    uint16_t keySize = sizeof(placeHolderCertBuffer);
+    CHIP_ERROR err   = storage->SyncGetKeyValue(storageKey, &placeHolderCertBuffer[0], keySize);
 
-    return false;
+    return (err == CHIP_NO_ERROR);
 }
 
 CHIP_ERROR LoadCertFromStorage(PersistentStorageDelegate * storage, FabricIndex fabricIndex, CertChainElement element,

diff --git a/src/crypto/PersistentStorageOperationalKeystore.cpp b/src/crypto/PersistentStorageOperationalKeystore.cpp
@@ -173,23 +173,18 @@ bool PersistentStorageOperationalKeystore::HasOpKeypairForFabric(FabricIndex fab
         return true;
     }
 
-    DefaultStorageKeyAllocator keyAlloc;
-    uint16_t keySize = 0;
-    CHIP_ERROR err   = mStorage->SyncGetKeyValue(keyAlloc.FabricOpKey(fabricIndex), nullptr, keySize);
+    // TODO(#16958): need to actually read the key to know if it's there due to platforms not
+    //               properly enforcing CHIP_ERROR_BUFFER_TOO_SMALL behavior needed by
+    //               PersistentStorageDelegate. Very unfortunate, needs fixing ASAP.
 
-    if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
-    {
-        // Obviously not found
-        return false;
-    }
-    if ((err == CHIP_ERROR_BUFFER_TOO_SMALL) && (keySize > 0))
-    {
-        // On found, we actually expect an "error", since we didn't want to read it out.
-        return true;
-    }
+    // Use a CapacityBoundBuffer to get RAII secret data clearing on scope exit.
+    Crypto::CapacityBoundBuffer<OpKeyTLVMaxSize()> buf;
+
+    DefaultStorageKeyAllocator keyAlloc;
+    uint16_t keySize = static_cast<uint16_t>(buf.Capacity());
+    CHIP_ERROR err   = mStorage->SyncGetKeyValue(keyAlloc.FabricOpKey(fabricIndex), buf.Bytes(), keySize);
 
-    // On any other error, we consider the key not found
-    return false;
+    return (err == CHIP_NO_ERROR);
 }
 
 CHIP_ERROR PersistentStorageOperationalKeystore::NewOpKeypairForFabric(FabricIndex fabricIndex,