From 22275773f24b7c8635b17fe8564b8b44d6c12a16 Mon Sep 17 00:00:00 2001
From: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Date: Sun, 26 Jun 2022 18:04:55 -0400
Subject: [PATCH] Apply review follow-ups of PR #19819 on FabricTable (#19970)

* Don't notify attribute changes on commit

* Apply follow-up comments from @bzbarsky-apple, all style/nits

* Simplify AddOrUpdateInner from @msandstedt comment

* Restyled

* Fix comment editorials from @bzbarsky-apple

* Add test for AddNOC fail-safe handling

* Add test for UpdateNOC failsafe handling

* Fix https://github.com/project-chip/connectedhomeip/pull/19819#discussion_r906373699

* Add Persistence Unit test

* Restyled

* Update src/credentials/FabricTable.cpp

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Fix CI

* Restyled

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
---
 .../operational-credentials-server.cpp        |   5 +-
 src/credentials/FabricTable.cpp               | 167 ++--
 src/credentials/FabricTable.h                 |  59 +-
 src/credentials/tests/TestFabricTable.cpp     | 856 +++++++++++++++++-
 .../support/TestPersistentStorageDelegate.h   |  18 +
 5 files changed, 953 insertions(+), 152 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
index bc190e56aae2c1..e7f71c0e3a67cf 100644
--- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
+++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
@@ -402,6 +402,7 @@ class OpCredsFabricTableDelegate : public chip::FabricTable::Delegate
         NotifyFabricTableChanged();
     }
 
+    // Gets called when a fabric is added/updated, but not necessarily committed to storage
     void OnFabricUpdated(const FabricTable & fabricTable, FabricIndex fabricIndex) override { NotifyFabricTableChanged(); }
 
     // Gets called when a fabric in FabricTable is persisted to storage
@@ -412,12 +413,10 @@ class OpCredsFabricTableDelegate : public chip::FabricTable::Delegate
         VerifyOrReturn(fabric != nullptr);
 
         ChipLogProgress(Zcl,
-                        "OpCreds: Fabric  index 0x%x was committed to storage. Compressed Fabric Id 0x" ChipLogFormatX64
+                        "OpCreds: Fabric index 0x%x was committed to storage. Compressed Fabric Id 0x" ChipLogFormatX64
                         ", FabricId " ChipLogFormatX64 ", NodeId " ChipLogFormatX64 ", VendorId 0x%04X",
                         static_cast<unsigned>(fabric->GetFabricIndex()), ChipLogValueX64(fabric->GetCompressedFabricId()),
                         ChipLogValueX64(fabric->GetFabricId()), ChipLogValueX64(fabric->GetNodeId()), fabric->GetVendorId());
-
-        NotifyFabricTableChanged();
     }
 
 private:
diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp
index 8763397ec8521c..52a714e82cfeee 100644
--- a/src/credentials/FabricTable.cpp
+++ b/src/credentials/FabricTable.cpp
@@ -95,17 +95,11 @@ void FabricInfo::operator=(FabricInfo && other)
 
     SetFabricLabel(other.GetFabricLabel());
 
-    if (other.mOperationalKey != nullptr)
-    {
-        if (other.mHasExternallyOwnedOperationalKey)
-        {
-            VerifyOrDie(SetExternallyOwnedOperationalKeypair(other.mOperationalKey) == CHIP_NO_ERROR);
-        }
-        else
-        {
-            VerifyOrDie(SetOperationalKeypair(other.mOperationalKey) == CHIP_NO_ERROR);
-        }
-    }
+    // Transfer ownership of operational keypair (if it was nullptr, it stays that way).
+    mOperationalKey                         = other.mOperationalKey;
+    mHasExternallyOwnedOperationalKey       = other.mHasExternallyOwnedOperationalKey;
+    other.mOperationalKey                   = nullptr;
+    other.mHasExternallyOwnedOperationalKey = false;
 
     other.Reset();
 }
@@ -259,7 +253,8 @@ CHIP_ERROR FabricInfo::SetExternallyOwnedOperationalKeypair(P256Keypair * keyPai
 CHIP_ERROR FabricTable::ValidateIncomingNOCChain(const ByteSpan & noc, const ByteSpan & icac, const ByteSpan & rcac,
                                                  FabricId existingFabricId, Credentials::CertificateValidityPolicy * policy,
                                                  CompressedFabricId & outCompressedFabricId, FabricId & outFabricId,
-                                                 NodeId & outNodeId, Crypto::P256PublicKey & outNocPubkey) const
+                                                 NodeId & outNodeId, Crypto::P256PublicKey & outNocPubkey,
+                                                 Crypto::P256PublicKey & outRootPubkey)
 {
     Credentials::ValidationContext validContext;
 
@@ -287,7 +282,7 @@ CHIP_ERROR FabricTable::ValidateIncomingNOCChain(const ByteSpan & noc, const Byt
 
     ChipLogProgress(FabricProvisioning, "Validating NOC chain");
     CHIP_ERROR err = FabricTable::VerifyCredentials(noc, icac, rcac, validContext, outCompressedFabricId, outFabricId, outNodeId,
-                                                    outNocPubkey, nullptr);
+                                                    outNocPubkey, &outRootPubkey);
     if (err != CHIP_NO_ERROR && err != CHIP_ERROR_WRONG_NODE_ID)
     {
         err = CHIP_ERROR_UNSUPPORTED_CERT_FORMAT;
@@ -413,7 +408,7 @@ CHIP_ERROR FabricTable::VerifyCredentials(const ByteSpan & noc, const ByteSpan &
         }
     }
 
-    outNocPubkey = certificates.GetLastCert()[0].mPublicKey;
+    outNocPubkey = certificates.GetLastCert()->mPublicKey;
 
     return CHIP_NO_ERROR;
 }
@@ -423,9 +418,7 @@ const FabricInfo * FabricTable::FindFabric(const Crypto::P256PublicKey & rootPub
     P256PublicKey candidatePubKey;
 
     // Try to match pending fabric first if available
-    bool hasPendingFabric =
-        mPendingFabric.IsInitialized() && mStateFlags.HasAll(StateFlags::kIsPendingFabricDataPresent, StateFlags::kIsUpdatePending);
-    if (hasPendingFabric)
+    if (HasPendingFabricUpdate())
     {
         bool pubKeyAvailable = (mPendingFabric.FetchRootPubkey(candidatePubKey) == CHIP_NO_ERROR);
         if (pubKeyAvailable && rootPubKey.Matches(candidatePubKey) && fabricId == mPendingFabric.GetFabricId())
@@ -456,10 +449,7 @@ const FabricInfo * FabricTable::FindFabric(const Crypto::P256PublicKey & rootPub
 FabricInfo * FabricTable::FindFabricWithIndex(FabricIndex fabricIndex)
 {
     // Try to match pending fabric first if available
-    bool hasPendingFabric =
-        mPendingFabric.IsInitialized() && mStateFlags.HasAll(StateFlags::kIsPendingFabricDataPresent, StateFlags::kIsUpdatePending);
-
-    if (hasPendingFabric && (mPendingFabric.GetFabricIndex() == fabricIndex))
+    if (HasPendingFabricUpdate() && (mPendingFabric.GetFabricIndex() == fabricIndex))
     {
         return &mPendingFabric;
     }
@@ -483,10 +473,7 @@ FabricInfo * FabricTable::FindFabricWithIndex(FabricIndex fabricIndex)
 const FabricInfo * FabricTable::FindFabricWithIndex(FabricIndex fabricIndex) const
 {
     // Try to match pending fabric first if available
-    bool hasPendingFabric =
-        mPendingFabric.IsInitialized() && mStateFlags.HasAll(StateFlags::kIsPendingFabricDataPresent, StateFlags::kIsUpdatePending);
-
-    if (hasPendingFabric && (mPendingFabric.GetFabricIndex() == fabricIndex))
+    if (HasPendingFabricUpdate() && (mPendingFabric.GetFabricIndex() == fabricIndex))
     {
         return &mPendingFabric;
     }
@@ -510,10 +497,7 @@ const FabricInfo * FabricTable::FindFabricWithIndex(FabricIndex fabricIndex) con
 const FabricInfo * FabricTable::FindFabricWithCompressedId(CompressedFabricId compressedFabricId) const
 {
     // Try to match pending fabric first if available
-    bool hasPendingFabric =
-        mPendingFabric.IsInitialized() && mStateFlags.HasAll(StateFlags::kIsPendingFabricDataPresent, StateFlags::kIsUpdatePending);
-
-    if (hasPendingFabric && (mPendingFabric.GetCompressedFabricId() == compressedFabricId))
+    if (HasPendingFabricUpdate() && (mPendingFabric.GetCompressedFabricId() == compressedFabricId))
     {
         return &mPendingFabric;
     }
@@ -542,7 +526,6 @@ CHIP_ERROR FabricTable::FetchRootCert(FabricIndex fabricIndex, MutableByteSpan &
 CHIP_ERROR FabricTable::FetchICACert(FabricIndex fabricIndex, MutableByteSpan & outCert) const
 {
     VerifyOrReturnError(mOpCertStore != nullptr, CHIP_ERROR_INCORRECT_STATE);
-    VerifyOrReturnError(!outCert.empty(), CHIP_ERROR_INVALID_ARGUMENT);
 
     CHIP_ERROR err = mOpCertStore->GetCertificate(fabricIndex, CertChainElement::kIcac, outCert);
     if (err == CHIP_ERROR_NOT_FOUND)
@@ -591,12 +574,7 @@ CHIP_ERROR FabricTable::StoreFabricMetadata(const FabricInfo * fabricInfo) const
 CHIP_ERROR FabricTable::LoadFromStorage(FabricInfo * fabric, FabricIndex newFabricIndex)
 {
     VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-
-    if (fabric->IsInitialized())
-    {
-        // TODO: When/how does this occur?
-        return CHIP_NO_ERROR;
-    }
+    VerifyOrReturnError(!fabric->IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
 
     uint8_t nocBuf[kMaxCHIPCertLength];
     MutableByteSpan nocSpan{ nocBuf };
@@ -618,8 +596,8 @@ CHIP_ERROR FabricTable::LoadFromStorage(FabricInfo * fabric, FabricIndex newFabr
 
     if (err != CHIP_NO_ERROR)
     {
-        ChipLogError(FabricProvisioning, "Fabric failed to load Fabric (0x%x): %" CHIP_ERROR_FORMAT,
-                     static_cast<unsigned>(newFabricIndex), err.Format());
+        ChipLogError(FabricProvisioning, "Failed to load Fabric (0x%x): %" CHIP_ERROR_FORMAT, static_cast<unsigned>(newFabricIndex),
+                     err.Format());
         fabric->Reset();
         return err;
     }
@@ -646,6 +624,8 @@ CHIP_ERROR FabricTable::AddNewFabricForTest(const ByteSpan & rootCert, const Byt
     Crypto::P256Keypair * opKey = nullptr;
     if (!opKeySpan.empty())
     {
+        VerifyOrReturnError(opKeySpan.size() == injectedOpKeysSerialized.Capacity(), CHIP_ERROR_INVALID_ARGUMENT);
+
         memcpy(injectedOpKeysSerialized.Bytes(), opKeySpan.data(), opKeySpan.size());
         SuccessOrExit(err = injectedOpKeysSerialized.SetLength(opKeySpan.size()));
         SuccessOrExit(err = injectedOpKey.Deserialize(injectedOpKeysSerialized));
@@ -723,13 +703,11 @@ CHIP_ERROR FabricTable::NotifyFabricCommitted(FabricIndex fabricIndex)
 }
 
 CHIP_ERROR
-FabricTable::AddOrUpdateInner(FabricIndex fabricIndex, Crypto::P256Keypair * existingOpKey, bool isExistingOpKeyExternallyOwned,
-                              uint16_t vendorId, FabricIndex * outputIndex)
+FabricTable::AddOrUpdateInner(FabricIndex fabricIndex, bool isAddition, Crypto::P256Keypair * existingOpKey,
+                              bool isExistingOpKeyExternallyOwned, uint16_t vendorId)
 {
     // All parameters pre-validated before we get here
 
-    bool isAddition = (fabricIndex == kUndefinedFabricIndex);
-
     FabricInfo::InitParams newFabricInfo;
     FabricInfo * fabricEntry    = nullptr;
     FabricId fabricIdToValidate = kUndefinedFabricId;
@@ -739,15 +717,6 @@ FabricTable::AddOrUpdateInner(FabricIndex fabricIndex, Crypto::P256Keypair * exi
     {
         // Initialization for Adding a fabric
 
-        // Make sure we have an available fabric index
-        if (!mNextAvailableFabricIndex.HasValue())
-        {
-            // No more indices available.  Bail out.
-            return CHIP_ERROR_NO_MEMORY;
-        }
-
-        fabricIndex = mNextAvailableFabricIndex.Value();
-
         // Find an available slot.
         for (auto & fabric : mStates)
         {
@@ -804,11 +773,7 @@ FabricTable::AddOrUpdateInner(FabricIndex fabricIndex, Crypto::P256Keypair * exi
 
         ReturnErrorOnFailure(ValidateIncomingNOCChain(nocSpan, icacSpan, rcacSpan, fabricIdToValidate, &notBeforeCollector,
                                                       newFabricInfo.compressedFabricId, newFabricInfo.fabricId,
-                                                      newFabricInfo.nodeId, nocPubKey));
-
-        P256PublicKeySpan rootPubKeySpan;
-        ReturnErrorOnFailure(ExtractPublicKeyFromChipCert(rcacSpan, rootPubKeySpan));
-        newFabricInfo.rootPublicKey = rootPubKeySpan;
+                                                      newFabricInfo.nodeId, nocPubKey, newFabricInfo.rootPublicKey));
     }
 
     if (existingOpKey != nullptr)
@@ -816,9 +781,7 @@ FabricTable::AddOrUpdateInner(FabricIndex fabricIndex, Crypto::P256Keypair * exi
         // Verify that public key in NOC matches public key of the provided keypair.
         // When operational key is not injected (e.g. when mOperationalKeystore != nullptr)
         // the check is done by the keystore in `ActivateOpKeypairForFabric`.
-        VerifyOrReturnError(existingOpKey->Pubkey().Length() == nocPubKey.Length(), CHIP_ERROR_INVALID_PUBLIC_KEY);
-        VerifyOrReturnError(memcmp(existingOpKey->Pubkey().ConstBytes(), nocPubKey.ConstBytes(), nocPubKey.Length()) == 0,
-                            CHIP_ERROR_INVALID_PUBLIC_KEY);
+        VerifyOrReturnError(existingOpKey->Pubkey().Matches(nocPubKey), CHIP_ERROR_INVALID_PUBLIC_KEY);
 
         newFabricInfo.operationalKeypair        = existingOpKey;
         newFabricInfo.hasExternallyOwnedKeypair = isExistingOpKeyExternallyOwned;
@@ -872,8 +835,6 @@ FabricTable::AddOrUpdateInner(FabricIndex fabricIndex, Crypto::P256Keypair * exi
         ChipLogError(FabricProvisioning, "Failed to update pending Last Known Good Time: %" CHIP_ERROR_FORMAT, lkgtErr.Format());
     }
 
-    *outputIndex = fabricIndex;
-
     // Must be the last thing before we return, as this is undone later on error handling within Delete.
     if (isAddition)
     {
@@ -1394,7 +1355,7 @@ CHIP_ERROR FabricTable::AllocatePendingOperationalKey(Optional<FabricIndex> fabr
         // Check we not are trying to do an update but also change the root: forbidden
         ReturnErrorCodeIf(mStateFlags.Has(StateFlags::kIsTrustedRootPending), CHIP_ERROR_INCORRECT_STATE);
 
-        // Fabric udpate case (e.g. UpdateNOC): we already know the fabric index
+        // Fabric update case (e.g. UpdateNOC): we already know the fabric index
         fabricIndexToUse = fabricIndex.Value();
         mStateFlags.Set(StateFlags::kIsPendingKeyForUpdateNoc);
     }
@@ -1432,7 +1393,6 @@ CHIP_ERROR FabricTable::AddNewPendingTrustedRootCert(const ByteSpan & rcac)
 
     if (mNextAvailableFabricIndex.HasValue())
     {
-        // Mark we have some pending data for a given fabric.
         fabricIndexToUse = mNextAvailableFabricIndex.Value();
     }
     else
@@ -1511,7 +1471,6 @@ CHIP_ERROR FabricTable::AddNewPendingFabricCommon(const ByteSpan & noc, const By
     FabricIndex fabricIndexToUse = kUndefinedFabricIndex;
     if (mNextAvailableFabricIndex.HasValue())
     {
-        // Mark we have some pending data for a given fabric.
         fabricIndexToUse = mNextAvailableFabricIndex.Value();
     }
     else
@@ -1520,6 +1479,10 @@ CHIP_ERROR FabricTable::AddNewPendingFabricCommon(const ByteSpan & noc, const By
         return CHIP_ERROR_NO_MEMORY;
     }
 
+    // Internal consistency check that mNextAvailableFabricIndex is indeed properly updated...
+    // TODO: Centralize this a bit.
+    VerifyOrReturnError(IsValidFabricIndex(fabricIndexToUse), CHIP_ERROR_INVALID_FABRIC_INDEX);
+
     if (existingOpKey == nullptr)
     {
         // If existing operational key not provided, we need to have a keystore present.
@@ -1531,8 +1494,6 @@ CHIP_ERROR FabricTable::AddNewPendingFabricCommon(const ByteSpan & noc, const By
                             CHIP_ERROR_KEY_NOT_FOUND);
     }
 
-    VerifyOrReturnError(IsValidFabricIndex(fabricIndexToUse), CHIP_ERROR_INVALID_FABRIC_INDEX);
-
     // Check for new fabric colliding with an existing fabric
     if (!mStateFlags.Has(StateFlags::kAreCollidingFabricsIgnored))
     {
@@ -1546,30 +1507,21 @@ CHIP_ERROR FabricTable::AddNewPendingFabricCommon(const ByteSpan & noc, const By
     VerifyOrReturnError(SetPendingDataFabricIndex(fabricIndexToUse), CHIP_ERROR_INCORRECT_STATE);
 
     CHIP_ERROR err =
-        AddOrUpdateInner(kUndefinedFabricIndex, existingOpKey, isExistingOpKeyExternallyOwned, vendorId, outNewFabricIndex);
+        AddOrUpdateInner(fabricIndexToUse, /* isAddition = */ true, existingOpKey, isExistingOpKeyExternallyOwned, vendorId);
     if (err != CHIP_NO_ERROR)
     {
         // Revert partial state added on error
-        mOpCertStore->RevertPendingOpCertsExceptRoot();
+        RevertPendingOpCertsExceptRoot();
         return err;
     }
 
-    if (fabricIndexToUse != *outNewFabricIndex)
-    {
-        ChipLogError(FabricProvisioning,
-                     "Fabric addition inconsistency! Determined we needed to add index 0x%x but added 0x%x. Reverting!",
-                     static_cast<unsigned>(fabricIndexToUse), static_cast<unsigned>(*outNewFabricIndex));
-        RevertPendingFabricData();
-
-        // After reverting, let's fatal if possible, as this should never happen.
-        VerifyOrDie(fabricIndexToUse == *outNewFabricIndex);
-
-        return CHIP_ERROR_INTERNAL;
-    }
-
     mStateFlags.Set(StateFlags::kIsAddPending);
     mStateFlags.Set(StateFlags::kIsPendingFabricDataPresent);
 
+    // Notify that NOC was added (at least transiently)
+    *outNewFabricIndex = fabricIndexToUse;
+    NotifyFabricUpdated(fabricIndexToUse);
+
     return CHIP_NO_ERROR;
 }
 
@@ -1611,29 +1563,17 @@ CHIP_ERROR FabricTable::UpdatePendingFabricCommon(FabricIndex fabricIndex, const
     ReturnErrorOnFailure(mOpCertStore->UpdateOpCertsForFabric(fabricIndex, noc, icac));
     VerifyOrReturnError(SetPendingDataFabricIndex(fabricIndex), CHIP_ERROR_INCORRECT_STATE);
 
-    FabricIndex newFabricIndex = kUndefinedFabricIndex;
-    CHIP_ERROR err =
-        AddOrUpdateInner(fabricIndex, existingOpKey, isExistingOpKeyExternallyOwned, fabricInfo->GetVendorId(), &newFabricIndex);
+    CHIP_ERROR err = AddOrUpdateInner(fabricIndex, /* isAddition = */ false, existingOpKey, isExistingOpKeyExternallyOwned,
+                                      fabricInfo->GetVendorId());
     if (err != CHIP_NO_ERROR)
     {
         // Revert partial state added on error
-        mOpCertStore->RevertPendingOpCertsExceptRoot();
+        // TODO: Figure-out if there is a better way. We need to make sure we are not inconsistent on elements
+        //       other than the opcerts.
+        RevertPendingOpCertsExceptRoot();
         return err;
     }
 
-    if (fabricIndex != newFabricIndex)
-    {
-        ChipLogError(FabricProvisioning,
-                     "Fabric update inconsistency! Determined we needed to update index 0x%x but added 0x%x. Reverting!",
-                     static_cast<unsigned>(fabricIndex), static_cast<unsigned>(newFabricIndex));
-        RevertPendingFabricData();
-
-        // After reverting, let's fatal if possible, as this should never happen.
-        VerifyOrDie(fabricIndex == newFabricIndex);
-
-        return CHIP_ERROR_INTERNAL;
-    }
-
     mStateFlags.Set(StateFlags::kIsUpdatePending);
     mStateFlags.Set(StateFlags::kIsPendingFabricDataPresent);
 
@@ -1653,7 +1593,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
     bool hasPending              = mStateFlags.Has(StateFlags::kIsPendingFabricDataPresent);
     bool hasInvalidInternalState = hasPending && (!IsValidFabricIndex(mFabricIndexWithPendingState) || !(isAdding || isUpdating));
 
-    FabricIndex fabricIndex = mFabricIndexWithPendingState;
+    FabricIndex fabricIndexBeingCommitted = mFabricIndexWithPendingState;
 
     // Proceed with Update/Add pre-flight checks
     if (hasPending && !hasInvalidInternalState)
@@ -1668,11 +1608,12 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
     }
 
     // Make sure we actually have a pending fabric
-    FabricInfo * pendingFabricEntry = FindFabricWithIndex(fabricIndex);
+    FabricInfo * pendingFabricEntry = FindFabricWithIndex(fabricIndexBeingCommitted);
 
     if (isUpdating && hasPending && !hasInvalidInternalState)
     {
-        if (!mPendingFabric.IsInitialized() || (mPendingFabric.GetFabricIndex() != fabricIndex) || (pendingFabricEntry == nullptr))
+        if (!mPendingFabric.IsInitialized() || (mPendingFabric.GetFabricIndex() != fabricIndexBeingCommitted) ||
+            (pendingFabricEntry == nullptr))
         {
             ChipLogError(FabricProvisioning, "Missing pending fabric on update during commit!");
             hasInvalidInternalState = true;
@@ -1681,7 +1622,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
 
     if (isAdding && hasPending && !hasInvalidInternalState)
     {
-        bool opCertStoreHasRoot = mOpCertStore->HasCertificateForFabric(fabricIndex, CertChainElement::kRcac);
+        bool opCertStoreHasRoot = mOpCertStore->HasCertificateForFabric(fabricIndexBeingCommitted, CertChainElement::kRcac);
         if (!mStateFlags.Has(StateFlags::kIsTrustedRootPending) || !opCertStoreHasRoot)
         {
             ChipLogError(FabricProvisioning, "Missing trusted root for fabric add during commit!");
@@ -1691,7 +1632,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
 
     if ((isAdding || isUpdating) && hasPending && !hasInvalidInternalState)
     {
-        if (!HasOperationalKeyForFabric(fabricIndex))
+        if (!HasOperationalKeyForFabric(fabricIndexBeingCommitted))
         {
             ChipLogError(FabricProvisioning, "Could not find an operational key during commit!");
             hasInvalidInternalState = true;
@@ -1742,14 +1683,14 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
         // This scope block is to illustrate the complete commit transaction
         // state. We can see it contains a LARGE number of items...
 
-        // Atomically assume data no longer pending, since we are commit it. Do so here
+        // Atomically assume data no longer pending, since we are committing it. Do so here
         // so that FindFabricBy* will return real data and never pending.
         mStateFlags.Clear(StateFlags::kIsPendingFabricDataPresent);
 
         if (isUpdating)
         {
             // This will get the non-pending fabric
-            FabricInfo * existingFabricToUpdate = FindFabricWithIndex(fabricIndex);
+            FabricInfo * existingFabricToUpdate = FindFabricWithIndex(fabricIndexBeingCommitted);
 
             // Multiple interlocks validated the below, so it's fatal if we are somehow incoherent here
             VerifyOrDie((existingFabricToUpdate != nullptr) && (existingFabricToUpdate != &mPendingFabric));
@@ -1760,7 +1701,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
         }
 
         // Store pending metadata first
-        FabricInfo * liveFabricEntry = FindFabricWithIndex(fabricIndex);
+        FabricInfo * liveFabricEntry = FindFabricWithIndex(fabricIndexBeingCommitted);
         VerifyOrDie(liveFabricEntry != nullptr);
 
         CHIP_ERROR metadataErr = StoreFabricMetadata(liveFabricEntry);
@@ -1772,10 +1713,10 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
 
         // We can only manage commissionable pending fail-safe state if we have a keystore
         CHIP_ERROR keyErr = CHIP_NO_ERROR;
-        if ((mOperationalKeystore != nullptr) && mOperationalKeystore->HasOpKeypairForFabric(fabricIndex) &&
+        if ((mOperationalKeystore != nullptr) && mOperationalKeystore->HasOpKeypairForFabric(fabricIndexBeingCommitted) &&
             mOperationalKeystore->HasPendingOpKeypair())
         {
-            keyErr = mOperationalKeystore->CommitOpKeypairForFabric(fabricIndex);
+            keyErr = mOperationalKeystore->CommitOpKeypairForFabric(fabricIndexBeingCommitted);
             if (keyErr != CHIP_NO_ERROR)
             {
                 ChipLogError(FabricProvisioning, "Failed to commit pending operational keypair %" CHIP_ERROR_FORMAT,
@@ -1786,7 +1727,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
         stickyError = (stickyError != CHIP_NO_ERROR) ? stickyError : keyErr;
 
         // Commit operational certs
-        CHIP_ERROR opCertErr = mOpCertStore->CommitOpCertsForFabric(fabricIndex);
+        CHIP_ERROR opCertErr = mOpCertStore->CommitOpCertsForFabric(fabricIndexBeingCommitted);
         if (opCertErr != CHIP_NO_ERROR)
         {
             ChipLogError(FabricProvisioning, "Failed to commit pending operational certificates %" CHIP_ERROR_FORMAT,
@@ -1821,9 +1762,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
         stickyError = (stickyError != CHIP_NO_ERROR) ? stickyError : fabricIndexErr;
     }
 
-    FabricIndex previouslyPendingFabricIndex = fabricIndex;
-
-    // Must have same side-effect as reverting all pending data
+    // Commit must have same side-effect as reverting all pending data
     mStateFlags.ClearAll();
     mFabricIndexWithPendingState = kUndefinedFabricIndex;
     mPendingFabric.Reset();
@@ -1832,13 +1771,13 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
     {
         // Blow-away everything if we got past any storage, even on Update: system state is broken
         // TODO: Develop a way to properly revert in the future, but this is very difficult
-        Delete(previouslyPendingFabricIndex);
+        Delete(fabricIndexBeingCommitted);
 
         RevertPendingFabricData();
     }
     else
     {
-        NotifyFabricCommitted(fabricIndex);
+        NotifyFabricCommitted(fabricIndexBeingCommitted);
     }
 
     return stickyError;
diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h
index 32308d0c7371b0..445a9cfa9865f4 100644
--- a/src/credentials/FabricTable.h
+++ b/src/credentials/FabricTable.h
@@ -279,19 +279,14 @@ class ConstFabricIterator
 
     const FabricInfo & operator*() const
     {
-        if (IsAtEnd())
-        {
-            return mStart[mIndex];
-        }
+        VerifyOrDie(!IsAtEnd());
 
         return *GetCurrent();
     }
     const FabricInfo * operator->() const
     {
-        if (IsAtEnd())
-        {
-            return mStart + mIndex;
-        }
+        VerifyOrDie(!IsAtEnd());
+
         return GetCurrent();
     }
 
@@ -428,8 +423,8 @@ class DLL_EXPORT FabricTable
     void Shutdown();
 
     // Forget a fabric in memory: doesn't delete any persistent state, just
-    // reverts any pending state (blindly) and then make the fabric table
-    // entry get reset.
+    // reverts any pending state (blindly) and then resets the fabric table
+    // entry.
     //
     // TODO: We have to determine if we should remove this call.
     void Forget(FabricIndex fabricIndex);
@@ -494,7 +489,7 @@ class DLL_EXPORT FabricTable
     /**
      * @brief Get the RCAC (operational root certificate) associated with a fabric.
      *
-     * If a root is pending from `AddNewPendingTrustedRootCert`, it is returned.
+     * If a root is pending for `fabricIndex` from `AddNewPendingTrustedRootCert`, it is returned.
      *
      * @param fabricIndex - Fabric for which to get the RCAC
      * @param outCert - MutableByteSpan to receive the certificate. Resized to actual size.
@@ -508,8 +503,8 @@ class DLL_EXPORT FabricTable
     /**
      * @brief Get the ICAC (operational intermediate certificate) associated with a fabric.
      *
-     * If a fabric is pending from add/update operation for the given `fabricIndex`, it is
-     * returned.
+     * If a fabric is pending from add/update operation for the given `fabricIndex`, its
+     * ICAC is returned.
      *
      * If an NOC exists, but the ICAC is not present in the chain, CHIP_NO_ERROR is
      * returned and `outCert` is resized to 0 length so that its `empty()` method returns true.
@@ -526,8 +521,8 @@ class DLL_EXPORT FabricTable
     /**
      * @brief Get the NOC (Node Operational Certificate) associated with a fabric.
      *
-     * If a fabric is pending from add/update operation for the given `fabricIndex`, it is
-     * returned.
+     * If a fabric is pending from add/update operation for the given `fabricIndex`, its
+     * NOC is returned.
      *
      * @param fabricIndex - Fabric for which to get the NOC
      * @param outCert - MutableByteSpan to receive the certificate. Resized to actual size.
@@ -594,7 +589,7 @@ class DLL_EXPORT FabricTable
 
     /**
      * @brief Returns whether an operational key is pending (true if `AllocatePendingOperationalKey` was
-     *        previously successfully called, false otherwise.
+     *        previously successfully called, false otherwise).
      *
      * @param outIsPendingKeyForUpdateNoc this is set to true if the `AllocatePendingOperationalKey` had an
      *                                    associated fabric index attached, indicating it's for UpdateNoc
@@ -702,7 +697,7 @@ class DLL_EXPORT FabricTable
      * Operational key is assumed to be pending or committed in the associated mOperationalKeystore.
      *
      * The new NOC chain becomes temporarily active for purposes of `Fetch*` and `SignWithOpKeyPair`, etc.
-     * The RCAC remains as before. To succeed this method call, NOC chain must chain back to an existing RCAC.
+     * The RCAC remains as before. For this method call to succeed, NOC chain must chain back to the existing RCAC.
      * The update fabric becomes permanent/persisted on successful `CommitPendingFabricData`. Changes revert
      * on `RevertPendingFabricData` or `RevertPendingOpCertsExceptRoot`. FabricId CANNOT be updated, but
      * CAT tags and Node ID in NOC can change between previous and new NOC for a given FabricId.
@@ -855,21 +850,25 @@ class DLL_EXPORT FabricTable
         return TLV::EstimateStructOverhead(sizeof(FabricIndex), CHIP_CONFIG_MAX_FABRICS * (1 + sizeof(FabricIndex)) + 1);
     }
 
-    // Load a FabricInfo metatada item from storage for a given new fabric index Returns internal error on failure.
+    // Load a FabricInfo metatada item from storage for a given new fabric index. Returns internal error on failure.
     CHIP_ERROR LoadFromStorage(FabricInfo * fabric, FabricIndex newFabricIndex);
 
     // Store a given fabric metadata directly/immediately. Used by internal operations.
     CHIP_ERROR StoreFabricMetadata(const FabricInfo * fabricInfo) const;
 
-    // Tries to set `mFabricIndexWithPendingState` and returns false if there's a clash
+    // Tries to set `mFabricIndexWithPendingState` and returns false if there's a clash.
     bool SetPendingDataFabricIndex(FabricIndex fabricIndex);
 
-    CHIP_ERROR AddOrUpdateInner(FabricIndex fabricIndex, Crypto::P256Keypair * existingOpKey, bool isExistingOpKeyExternallyOwned,
-                                uint16_t vendorId, FabricIndex * outputIndex);
+    // Core validation logic for fabric additions/updates
+    CHIP_ERROR AddOrUpdateInner(FabricIndex fabricIndex, bool isAddition, Crypto::P256Keypair * existingOpKey,
+                                bool isExistingOpKeyExternallyOwned, uint16_t vendorId);
 
+    // Common code for fabric addition, for either OperationalKeystore or injected key scenarios.
     CHIP_ERROR AddNewPendingFabricCommon(const ByteSpan & noc, const ByteSpan & icac, uint16_t vendorId,
                                          Crypto::P256Keypair * existingOpKey, bool isExistingOpKeyExternallyOwned,
                                          FabricIndex * outNewFabricIndex);
+
+    // Common code for fabric updates, for either OperationalKeystore or injected key scenarios.
     CHIP_ERROR UpdatePendingFabricCommon(FabricIndex fabricIndex, const ByteSpan & noc, const ByteSpan & icac,
                                          Crypto::P256Keypair * existingOpKey, bool isExistingOpKeyExternallyOwned);
 
@@ -921,12 +920,13 @@ class DLL_EXPORT FabricTable
      *
      * @return a pointer to the shadow pending fabric or nullptr if none is active.
      */
-    const FabricInfo * GetShadowPendingFabricEntry() const
+    const FabricInfo * GetShadowPendingFabricEntry() const { return HasPendingFabricUpdate() ? &mPendingFabric : nullptr; }
+
+    // Returns true if we have a shadow entry pending for a fabruc update.
+    bool HasPendingFabricUpdate() const
     {
-        bool hasPendingFabric = mPendingFabric.IsInitialized() &&
+        return mPendingFabric.IsInitialized() &&
             mStateFlags.HasAll(StateFlags::kIsPendingFabricDataPresent, StateFlags::kIsUpdatePending);
-
-        return hasPendingFabric ? &mPendingFabric : nullptr;
     }
 
     // Verifies credentials, using the provided root certificate.
@@ -940,10 +940,11 @@ class DLL_EXPORT FabricTable
     // The `existingFabricId` is passed for UpdateNOC, and must match the Fabric, to make sure that we are
     // not trying to change FabricID with UpdateNOC. If set to kUndefinedFabricId, we are doing AddNOC and
     // we don't need to check match to pre-existing fabric.
-    CHIP_ERROR ValidateIncomingNOCChain(const ByteSpan & noc, const ByteSpan & icac, const ByteSpan & rcac,
-                                        FabricId existingFabricId, Credentials::CertificateValidityPolicy * policy,
-                                        CompressedFabricId & outCompressedFabricId, FabricId & outFabricId, NodeId & outNodeId,
-                                        Crypto::P256PublicKey & outNocPubkey) const;
+    static CHIP_ERROR ValidateIncomingNOCChain(const ByteSpan & noc, const ByteSpan & icac, const ByteSpan & rcac,
+                                               FabricId existingFabricId, Credentials::CertificateValidityPolicy * policy,
+                                               CompressedFabricId & outCompressedFabricId, FabricId & outFabricId,
+                                               NodeId & outNodeId, Crypto::P256PublicKey & outNocPubkey,
+                                               Crypto::P256PublicKey & outRootPubkey);
 
     /**
      * Read our fabric index info from the given TLV reader and set up the
diff --git a/src/credentials/tests/TestFabricTable.cpp b/src/credentials/tests/TestFabricTable.cpp
index 3c3e79a87a2fd6..8fa0a5aed55aa9 100644
--- a/src/credentials/tests/TestFabricTable.cpp
+++ b/src/credentials/tests/TestFabricTable.cpp
@@ -181,7 +181,7 @@ void TestUpdateLastKnownGoodTime(nlTestSuite * inSuite, void * inContext)
             NL_TEST_ASSERT(inSuite, fabricTable.GetLastKnownGoodChipEpochTime(lastKnownGoodTime) == CHIP_NO_ERROR);
             NL_TEST_ASSERT(inSuite, lastKnownGoodTime == buildTime);
 
-            // Verify that calling the fail-safe roll back interface does change
+            // Verify that calling the fail-safe roll back interface does not change
             // last known good time, as it hadn't been updated in the first place.
             fabricTable.RevertPendingFabricData();
             NL_TEST_ASSERT(inSuite, fabricTable.GetLastKnownGoodChipEpochTime(lastKnownGoodTime) == CHIP_NO_ERROR);
@@ -873,29 +873,871 @@ void TestPersistence(nlTestSuite * inSuite, void * inContext)
      *     and verify you can sign and verify messages with the opkey
      *
      */
+
+    Crypto::P256PublicKey fIdx1PublicKey;
+    Crypto::P256PublicKey fIdx2PublicKey;
+
+    Credentials::TestOnlyLocalCertificateAuthority fabricCertAuthority;
+
+    chip::TestPersistentStorageDelegate storage;
+
+    NL_TEST_ASSERT(inSuite, fabricCertAuthority.Init().IsSuccess());
+
+    constexpr uint16_t kVendorId = 0xFFF1u;
+
+    // First scope: add 2 fabrics with same root: (1111, 2222), commit them, keep track of public keys
+    {
+        // Initialize a FabricTable
+        ScopedFabricTable fabricTableHolder;
+        NL_TEST_ASSERT(inSuite, fabricTableHolder.Init(&storage) == CHIP_NO_ERROR);
+        FabricTable & fabricTable = fabricTableHolder.GetFabricTable();
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0);
+
+        // Add Fabric 1111 Node Id 55
+        {
+            FabricId fabricId = 1111;
+            NodeId nodeId     = 55;
+
+            uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length];
+            MutableByteSpan csrSpan{ csrBuf };
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan));
+
+            NL_TEST_ASSERT_SUCCESS(
+                inSuite, fabricCertAuthority.SetIncludeIcac(true).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus());
+            ByteSpan rcac = fabricCertAuthority.GetRcac();
+            ByteSpan icac = fabricCertAuthority.GetIcac();
+            ByteSpan noc  = fabricCertAuthority.GetNoc();
+
+            NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0);
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac));
+            FabricIndex newFabricIndex = kUndefinedFabricIndex;
+            NL_TEST_ASSERT_SUCCESS(inSuite,
+                                   fabricTable.AddNewPendingFabricWithOperationalKeystore(noc, icac, kVendorId, &newFabricIndex));
+            NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+            NL_TEST_ASSERT(inSuite, newFabricIndex == 1);
+
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.CommitPendingFabricData());
+
+            // Validate contents
+            const auto * fabricInfo = fabricTable.FindFabricWithIndex(1);
+            NL_TEST_ASSERT(inSuite, fabricInfo != nullptr);
+            if (fabricInfo != nullptr)
+            {
+                Credentials::ChipCertificateSet certificates;
+                NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1));
+                NL_TEST_ASSERT_SUCCESS(inSuite,
+                                       certificates.LoadCert(rcac, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));
+                Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey);
+
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 1);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 55);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 1111);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0);
+
+                Crypto::P256PublicKey rootPublicKeyOfFabric;
+                NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(newFabricIndex, rootPublicKeyOfFabric));
+                NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey));
+            }
+
+            // Validate that fabric has the correct operational key by verifying a signature
+            {
+                Crypto::P256ECDSASignature sig;
+                uint8_t message[] = { 'm', 's', 'g' };
+
+                NL_TEST_ASSERT_SUCCESS(inSuite, VerifyCertificateSigningRequest(csrSpan.data(), csrSpan.size(), fIdx1PublicKey));
+
+                NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SignWithOpKeypair(newFabricIndex, ByteSpan{ message }, sig));
+                NL_TEST_ASSERT_SUCCESS(inSuite, fIdx1PublicKey.ECDSA_validate_msg_signature(&message[0], sizeof(message), sig));
+            }
+        }
+
+        // Add Fabric 2222 Node Id 66, no ICAC
+        {
+            FabricId fabricId = 2222;
+            NodeId nodeId     = 66;
+
+            uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length];
+            MutableByteSpan csrSpan{ csrBuf };
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan));
+
+            NL_TEST_ASSERT_SUCCESS(
+                inSuite, fabricCertAuthority.SetIncludeIcac(false).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus());
+            ByteSpan rcac = fabricCertAuthority.GetRcac();
+            ByteSpan noc  = fabricCertAuthority.GetNoc();
+
+            NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac));
+            FabricIndex newFabricIndex = kUndefinedFabricIndex;
+            NL_TEST_ASSERT_SUCCESS(
+                inSuite, fabricTable.AddNewPendingFabricWithOperationalKeystore(noc, ByteSpan{}, kVendorId, &newFabricIndex));
+            NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 2);
+            NL_TEST_ASSERT(inSuite, newFabricIndex == 2);
+
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.CommitPendingFabricData());
+
+            // Validate contents
+            const auto * fabricInfo = fabricTable.FindFabricWithIndex(2);
+            NL_TEST_ASSERT(inSuite, fabricInfo != nullptr);
+            if (fabricInfo != nullptr)
+            {
+                Credentials::ChipCertificateSet certificates;
+                NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1));
+                NL_TEST_ASSERT_SUCCESS(inSuite,
+                                       certificates.LoadCert(rcac, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));
+                Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey);
+
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 2);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 66);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 2222);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0);
+
+                Crypto::P256PublicKey rootPublicKeyOfFabric;
+                NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(newFabricIndex, rootPublicKeyOfFabric));
+                NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey));
+            }
+
+            // Validate that fabric has the correct operational key by verifying a signature
+            {
+                Crypto::P256ECDSASignature sig;
+                uint8_t message[] = { 'm', 's', 'g' };
+
+                NL_TEST_ASSERT_SUCCESS(inSuite, VerifyCertificateSigningRequest(csrSpan.data(), csrSpan.size(), fIdx2PublicKey));
+
+                NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SignWithOpKeypair(newFabricIndex, ByteSpan{ message }, sig));
+                NL_TEST_ASSERT_SUCCESS(inSuite, fIdx2PublicKey.ECDSA_validate_msg_signature(&message[0], sizeof(message), sig));
+            }
+        }
+
+        NL_TEST_ASSERT(inSuite, fabricTable.FabricCount() == 2);
+
+        // Verify we can now see 2 fabrics with the iterator
+        {
+            size_t numFabricsIterated = 0;
+            bool saw1                 = false;
+            bool saw2                 = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 55);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 1111);
+                    saw1 = true;
+                }
+                if (iterFabricInfo.GetFabricIndex() == 2)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 66);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 2222);
+                    saw2 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 2);
+            NL_TEST_ASSERT(inSuite, saw1 == true);
+            NL_TEST_ASSERT(inSuite, saw2 == true);
+        }
+    }
+
+    // Global: Last known good time + fabric index = 2
+    // Fabric 1111: Metadata, 1 opkey, RCAC/ICAC/NOC = 5
+    // Fabric 2222: Metadata, 1 opkey, RCAC/NOC = 4
+    NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == (2 + 5 + 4));
+
+    // Second scope: Validate that a fresh FabricTable loads the previously committed fabrics on Init.
+    {
+        // Initialize a FabricTable
+        ScopedFabricTable fabricTableHolder;
+        NL_TEST_ASSERT(inSuite, fabricTableHolder.Init(&storage) == CHIP_NO_ERROR);
+        FabricTable & fabricTable = fabricTableHolder.GetFabricTable();
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 2);
+
+        // Verify we can see 2 fabrics with the iterator
+        {
+            size_t numFabricsIterated = 0;
+            bool saw1                 = false;
+            bool saw2                 = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 55);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 1111);
+                    saw1 = true;
+                }
+                if (iterFabricInfo.GetFabricIndex() == 2)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 66);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 2222);
+                    saw2 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 2);
+            NL_TEST_ASSERT(inSuite, saw1 == true);
+            NL_TEST_ASSERT(inSuite, saw2 == true);
+        }
+
+        // Validate contents of Fabric 2222
+        {
+            uint8_t rcacBuf[Credentials::kMaxCHIPCertLength];
+            MutableByteSpan rcacSpan{ rcacBuf };
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootCert(2, rcacSpan));
+
+            const auto * fabricInfo = fabricTable.FindFabricWithIndex(2);
+            NL_TEST_ASSERT(inSuite, fabricInfo != nullptr);
+            if (fabricInfo != nullptr)
+            {
+                Credentials::ChipCertificateSet certificates;
+                NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1));
+                NL_TEST_ASSERT_SUCCESS(inSuite,
+                                       certificates.LoadCert(rcacSpan, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));
+                Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey);
+
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 2);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 66);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 2222);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0);
+
+                Crypto::P256PublicKey rootPublicKeyOfFabric;
+                NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(2, rootPublicKeyOfFabric));
+                NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey));
+            }
+
+            // Validate that fabric has the correct operational key by verifying a signature
+            {
+                Crypto::P256ECDSASignature sig;
+                uint8_t message[] = { 'm', 's', 'g' };
+
+                NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SignWithOpKeypair(2, ByteSpan{ message }, sig));
+                NL_TEST_ASSERT_SUCCESS(inSuite, fIdx2PublicKey.ECDSA_validate_msg_signature(&message[0], sizeof(message), sig));
+            }
+        }
+
+        // Validate contents of Fabric 1111
+        {
+            uint8_t rcacBuf[Credentials::kMaxCHIPCertLength];
+            MutableByteSpan rcacSpan{ rcacBuf };
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootCert(1, rcacSpan));
+
+            const auto * fabricInfo = fabricTable.FindFabricWithIndex(1);
+            NL_TEST_ASSERT(inSuite, fabricInfo != nullptr);
+            if (fabricInfo != nullptr)
+            {
+                Credentials::ChipCertificateSet certificates;
+                NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1));
+                NL_TEST_ASSERT_SUCCESS(inSuite,
+                                       certificates.LoadCert(rcacSpan, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));
+                Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey);
+
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 1);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 55);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 1111);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId);
+                NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0);
+
+                Crypto::P256PublicKey rootPublicKeyOfFabric;
+                NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(1, rootPublicKeyOfFabric));
+                NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey));
+            }
+
+            // Validate that fabric has the correct operational key by verifying a signature
+            {
+                Crypto::P256ECDSASignature sig;
+                uint8_t message[] = { 'm', 's', 'g' };
+
+                NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SignWithOpKeypair(1, ByteSpan{ message }, sig));
+                NL_TEST_ASSERT_SUCCESS(inSuite, fIdx1PublicKey.ECDSA_validate_msg_signature(&message[0], sizeof(message), sig));
+            }
+
+            // Validate that signing with Fabric index 2 fails to verify with fabric index 1
+            {
+                Crypto::P256ECDSASignature sig;
+                uint8_t message[] = { 'm', 's', 'g' };
+
+                NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SignWithOpKeypair(2, ByteSpan{ message }, sig));
+                NL_TEST_ASSERT(inSuite,
+                               fIdx1PublicKey.ECDSA_validate_msg_signature(&message[0], sizeof(message), sig) ==
+                                   CHIP_ERROR_INVALID_SIGNATURE);
+            }
+        }
+    }
+}
+
+void TestAddNocFailSafe(nlTestSuite * inSuite, void * inContext)
+{
+    Credentials::TestOnlyLocalCertificateAuthority fabric11CertAuthority;
+    Credentials::TestOnlyLocalCertificateAuthority fabric44CertAuthority;
+
+    chip::TestPersistentStorageDelegate storage;
+
+    NL_TEST_ASSERT(inSuite, fabric11CertAuthority.Init().IsSuccess());
+    NL_TEST_ASSERT(inSuite, fabric44CertAuthority.Init().IsSuccess());
+
+    constexpr uint16_t kVendorId = 0xFFF1u;
+
+    // Initialize a fabric table.
+    ScopedFabricTable fabricTableHolder;
+    NL_TEST_ASSERT(inSuite, fabricTableHolder.Init(&storage) == CHIP_NO_ERROR);
+    FabricTable & fabricTable = fabricTableHolder.GetFabricTable();
+
+    NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0);
+
+    size_t numFabricsIterated = 0;
+
+    size_t numStorageKeysAtStart = storage.GetNumKeys();
+
+    // Sequence 1: Add node ID 55 on fabric 11, see that pending works, and that revert works
+    {
+        FabricId fabricId = 11;
+        NodeId nodeId     = 55;
+
+        uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length];
+        MutableByteSpan csrSpan{ csrBuf };
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan));
+
+        NL_TEST_ASSERT_SUCCESS(inSuite,
+                               fabric11CertAuthority.SetIncludeIcac(false).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus());
+        ByteSpan rcac = fabric11CertAuthority.GetRcac();
+        ByteSpan noc  = fabric11CertAuthority.GetNoc();
+
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac));
+        FabricIndex newFabricIndex = kUndefinedFabricIndex;
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0);
+        NL_TEST_ASSERT_SUCCESS(inSuite,
+                               fabricTable.AddNewPendingFabricWithOperationalKeystore(noc, ByteSpan{}, kVendorId, &newFabricIndex));
+        NL_TEST_ASSERT(inSuite, newFabricIndex == 1);
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+
+        // No storage yet
+        NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == numStorageKeysAtStart); // Nothing yet
+
+        // Validate iterator sees pending
+        {
+            numFabricsIterated = 0;
+            bool saw1          = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == nodeId);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == fabricId);
+                    saw1 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 1);
+            NL_TEST_ASSERT(inSuite, saw1 == true);
+        }
+
+        // Revert, should see nothing yet
+        fabricTable.RevertPendingFabricData();
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0);
+
+        // No started except fabric index metadata
+        NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == (numStorageKeysAtStart + 1));
+
+        // Validate iterator sees nothing
+        {
+            numFabricsIterated = 0;
+            bool saw1          = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    saw1 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 0);
+            NL_TEST_ASSERT(inSuite, saw1 == false);
+        }
+    }
+
+    size_t numStorageAfterRevert = storage.GetNumKeys();
+
+    // Sequence 2: Add node ID 999 on fabric 44, using operational keystore and ICAC --> Yield fabricIndex 1
+    {
+        FabricId fabricId = 44;
+        NodeId nodeId     = 999;
+
+        uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length];
+        MutableByteSpan csrSpan{ csrBuf };
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan));
+
+        NL_TEST_ASSERT_SUCCESS(inSuite,
+                               fabric44CertAuthority.SetIncludeIcac(true).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus());
+        ByteSpan rcac = fabric44CertAuthority.GetRcac();
+        ByteSpan icac = fabric44CertAuthority.GetIcac();
+        ByteSpan noc  = fabric44CertAuthority.GetNoc();
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0);
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac));
+        FabricIndex newFabricIndex = kUndefinedFabricIndex;
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0);
+        NL_TEST_ASSERT_SUCCESS(inSuite,
+                               fabricTable.AddNewPendingFabricWithOperationalKeystore(noc, icac, kVendorId, &newFabricIndex));
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+        NL_TEST_ASSERT(inSuite, newFabricIndex == 1);
+        // No storage yet
+        NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == numStorageAfterRevert);
+
+        // Commit, now storage should have keys
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.CommitPendingFabricData());
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+
+        NL_TEST_ASSERT_EQUALS(inSuite, storage.GetNumKeys(),
+                              (numStorageAfterRevert + 5)); // 3 opcerts + fabric metadata + 1 operational key
+
+        // Validate contents
+        const auto * fabricInfo = fabricTable.FindFabricWithIndex(1);
+        NL_TEST_ASSERT(inSuite, fabricInfo != nullptr);
+        if (fabricInfo != nullptr)
+        {
+            Credentials::ChipCertificateSet certificates;
+            NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1));
+            NL_TEST_ASSERT_SUCCESS(inSuite,
+                                   certificates.LoadCert(rcac, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));
+            Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey);
+
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == newFabricIndex);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == nodeId);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == fabricId);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0);
+
+            Crypto::P256PublicKey rootPublicKeyOfFabric;
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(newFabricIndex, rootPublicKeyOfFabric));
+            NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey));
+        }
+
+        // Validate that fabric has the correct operational key by verifying a signature
+        {
+            Crypto::P256ECDSASignature sig;
+            uint8_t message[] = { 'm', 's', 'g' };
+
+            Crypto::P256PublicKey nocPubKey;
+            NL_TEST_ASSERT_SUCCESS(inSuite, VerifyCertificateSigningRequest(csrSpan.data(), csrSpan.size(), nocPubKey));
+
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SignWithOpKeypair(newFabricIndex, ByteSpan{ message }, sig));
+            NL_TEST_ASSERT_SUCCESS(inSuite, nocPubKey.ECDSA_validate_msg_signature(&message[0], sizeof(message), sig));
+        }
+
+        // Verify we can now see the fabric with the iterator
+        {
+            numFabricsIterated = 0;
+            bool saw1          = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 999);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 44);
+                    saw1 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 1);
+            NL_TEST_ASSERT(inSuite, saw1 == true);
+        }
+    }
+
+    size_t numStorageAfterAdd = storage.GetNumKeys();
+
+    // Sequence 3: Do a RevertPendingFabricData() again, see that it doesn't affect existing fabric
+
+    {
+        // Revert, should should look like a no-op
+        fabricTable.RevertPendingFabricData();
+
+        // No change of storage
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+        NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == numStorageAfterAdd);
+
+        // Verify we can still see the fabric with the iterator
+        {
+            numFabricsIterated = 0;
+            bool saw1          = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 999);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 44);
+                    saw1 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 1);
+            NL_TEST_ASSERT(inSuite, saw1 == true);
+        }
+    }
+}
+
+void TestUpdateNocFailSafe(nlTestSuite * inSuite, void * inContext)
+{
+    Credentials::TestOnlyLocalCertificateAuthority fabric11CertAuthority;
+    Credentials::TestOnlyLocalCertificateAuthority fabric44CertAuthority;
+
+    chip::TestPersistentStorageDelegate storage;
+
+    storage.SetLoggingLevel(chip::TestPersistentStorageDelegate::LoggingLevel::kLogMutation);
+
+    NL_TEST_ASSERT(inSuite, fabric11CertAuthority.Init().IsSuccess());
+    NL_TEST_ASSERT(inSuite, fabric44CertAuthority.Init().IsSuccess());
+
+    constexpr uint16_t kVendorId = 0xFFF1u;
+
+    // Initialize a fabric table.
+    ScopedFabricTable fabricTableHolder;
+    NL_TEST_ASSERT(inSuite, fabricTableHolder.Init(&storage) == CHIP_NO_ERROR);
+    FabricTable & fabricTable = fabricTableHolder.GetFabricTable();
+
+    NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0);
+
+    size_t numFabricsIterated = 0;
+
+    size_t numStorageKeysAtStart = storage.GetNumKeys();
+
+    // Sequence 1: Add node ID 999 on fabric 44, using operational keystore and ICAC --> Yield fabricIndex 1
+    {
+        FabricId fabricId = 44;
+        NodeId nodeId     = 999;
+
+        uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length];
+        MutableByteSpan csrSpan{ csrBuf };
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan));
+
+        NL_TEST_ASSERT_SUCCESS(inSuite,
+                               fabric44CertAuthority.SetIncludeIcac(true).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus());
+        ByteSpan rcac = fabric44CertAuthority.GetRcac();
+        ByteSpan icac = fabric44CertAuthority.GetIcac();
+        ByteSpan noc  = fabric44CertAuthority.GetNoc();
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0);
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac));
+        FabricIndex newFabricIndex = kUndefinedFabricIndex;
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0);
+        NL_TEST_ASSERT_SUCCESS(inSuite,
+                               fabricTable.AddNewPendingFabricWithOperationalKeystore(noc, icac, kVendorId, &newFabricIndex));
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+        NL_TEST_ASSERT(inSuite, newFabricIndex == 1);
+        // No storage yet
+        NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == numStorageKeysAtStart);
+
+        // Commit, now storage should have keys
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.CommitPendingFabricData());
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+
+        NL_TEST_ASSERT_EQUALS(inSuite, storage.GetNumKeys(),
+                              (numStorageKeysAtStart + 6)); // 3 opcerts + fabric metadata + 1 operational key + LKGT + fabric index
+
+        // Validate contents
+        const auto * fabricInfo = fabricTable.FindFabricWithIndex(1);
+        NL_TEST_ASSERT(inSuite, fabricInfo != nullptr);
+        if (fabricInfo != nullptr)
+        {
+            Credentials::ChipCertificateSet certificates;
+            NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1));
+            NL_TEST_ASSERT_SUCCESS(inSuite,
+                                   certificates.LoadCert(rcac, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));
+            Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey);
+
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == newFabricIndex);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == nodeId);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == fabricId);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0);
+
+            Crypto::P256PublicKey rootPublicKeyOfFabric;
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(newFabricIndex, rootPublicKeyOfFabric));
+            NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey));
+        }
+
+        // Validate that fabric has the correct operational key by verifying a signature
+        {
+            Crypto::P256ECDSASignature sig;
+            uint8_t message[] = { 'm', 's', 'g' };
+
+            Crypto::P256PublicKey nocPubKey;
+            NL_TEST_ASSERT_SUCCESS(inSuite, VerifyCertificateSigningRequest(csrSpan.data(), csrSpan.size(), nocPubKey));
+
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SignWithOpKeypair(newFabricIndex, ByteSpan{ message }, sig));
+            NL_TEST_ASSERT_SUCCESS(inSuite, nocPubKey.ECDSA_validate_msg_signature(&message[0], sizeof(message), sig));
+        }
+
+        // Verify we can now see the fabric with the iterator
+        {
+            numFabricsIterated = 0;
+            bool saw1          = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 999);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 44);
+                    saw1 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 1);
+            NL_TEST_ASSERT(inSuite, saw1 == true);
+        }
+    }
+
+    size_t numStorageAfterAdd = storage.GetNumKeys();
+
+    // Sequence 2: Do an Update to NodeId 1000, with no ICAC, but revert it
+    {
+        FabricId fabricId       = 44;
+        NodeId nodeId           = 1000;
+        FabricIndex fabricIndex = 1;
+
+        uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length];
+        MutableByteSpan csrSpan{ csrBuf };
+
+        // Make sure to tag fabric index to pending opkey: otherwise the UpdateNOC fails
+        NL_TEST_ASSERT_SUCCESS(inSuite,
+                               fabricTable.AllocatePendingOperationalKey(chip::MakeOptional(static_cast<FabricIndex>(1)), csrSpan));
+
+        NL_TEST_ASSERT_SUCCESS(inSuite,
+                               fabric44CertAuthority.SetIncludeIcac(false).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus());
+        ByteSpan rcac = fabric44CertAuthority.GetRcac();
+        ByteSpan noc  = fabric44CertAuthority.GetNoc();
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.UpdatePendingFabricWithOperationalKeystore(1, noc, ByteSpan{}));
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+
+        // No storage yet
+        NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == numStorageAfterAdd);
+
+        // Validate iterator sees the pending data
+        {
+            numFabricsIterated = 0;
+            bool saw1          = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 1000);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 44);
+                    saw1 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 1);
+            NL_TEST_ASSERT(inSuite, saw1 == true);
+        }
+
+        // Revert, should see Node ID 999 again
+        fabricTable.RevertPendingFabricData();
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+
+        NL_TEST_ASSERT_EQUALS(inSuite, storage.GetNumKeys(), numStorageAfterAdd);
+
+        // Validate contents
+        const auto * fabricInfo = fabricTable.FindFabricWithIndex(1);
+        NL_TEST_ASSERT(inSuite, fabricInfo != nullptr);
+        if (fabricInfo != nullptr)
+        {
+            Credentials::ChipCertificateSet certificates;
+            NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1));
+            NL_TEST_ASSERT_SUCCESS(inSuite,
+                                   certificates.LoadCert(rcac, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));
+            Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey);
+
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == fabricIndex);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 999);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 44);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0);
+
+            Crypto::P256PublicKey rootPublicKeyOfFabric;
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(fabricIndex, rootPublicKeyOfFabric));
+            NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey));
+        }
+
+        // Validate that fabric has the correct operational key by verifying a signature
+        {
+            uint8_t nocBuf[Credentials::kMaxCHIPCertLength];
+            MutableByteSpan nocSpan{ nocBuf };
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchNOCCert(1, nocSpan));
+
+            Credentials::ChipCertificateSet certificates;
+            NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1));
+            NL_TEST_ASSERT_SUCCESS(inSuite,
+                                   certificates.LoadCert(nocSpan, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));
+            Crypto::P256PublicKey nocPubKey(certificates.GetCertSet()[0].mPublicKey);
+
+            Crypto::P256ECDSASignature sig;
+            uint8_t message[] = { 'm', 's', 'g' };
+
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SignWithOpKeypair(fabricIndex, ByteSpan{ message }, sig));
+            NL_TEST_ASSERT_SUCCESS(inSuite, nocPubKey.ECDSA_validate_msg_signature(&message[0], sizeof(message), sig));
+        }
+
+        // Validate iterator sees the previous fabric
+        {
+            numFabricsIterated = 0;
+            bool saw1          = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 999);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 44);
+                    saw1 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 1);
+            NL_TEST_ASSERT(inSuite, saw1 == true);
+        }
+    }
+
+    // Sequence 3: Do an Update to NodeId 1001, with no ICAC, but commit it
+    {
+        FabricId fabricId       = 44;
+        NodeId nodeId           = 1001;
+        FabricIndex fabricIndex = 1;
+
+        uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length];
+        MutableByteSpan csrSpan{ csrBuf };
+
+        // Make sure to tag fabric index to pending opkey: otherwise the UpdateNOC fails
+        NL_TEST_ASSERT_SUCCESS(inSuite,
+                               fabricTable.AllocatePendingOperationalKey(chip::MakeOptional(static_cast<FabricIndex>(1)), csrSpan));
+
+        NL_TEST_ASSERT_SUCCESS(inSuite,
+                               fabric44CertAuthority.SetIncludeIcac(false).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus());
+        ByteSpan rcac = fabric44CertAuthority.GetRcac();
+        ByteSpan noc  = fabric44CertAuthority.GetNoc();
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.UpdatePendingFabricWithOperationalKeystore(1, noc, ByteSpan{}));
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+
+        // No storage yet
+        NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == numStorageAfterAdd);
+
+        // Validate iterator sees the pending data
+        {
+            numFabricsIterated = 0;
+            bool saw1          = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 1001);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 44);
+                    saw1 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 1);
+            NL_TEST_ASSERT(inSuite, saw1 == true);
+        }
+
+        // Commit, should see Node ID 1001, and 1 less cert in the storage
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.CommitPendingFabricData());
+
+        NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1);
+        NL_TEST_ASSERT_EQUALS(inSuite, storage.GetNumKeys(), numStorageAfterAdd - 1);
+
+        // Validate contents
+        const auto * fabricInfo = fabricTable.FindFabricWithIndex(1);
+        NL_TEST_ASSERT(inSuite, fabricInfo != nullptr);
+        if (fabricInfo != nullptr)
+        {
+            Credentials::ChipCertificateSet certificates;
+            NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1));
+            NL_TEST_ASSERT_SUCCESS(inSuite,
+                                   certificates.LoadCert(rcac, BitFlags<CertDecodeFlags>(CertDecodeFlags::kIsTrustAnchor)));
+            Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey);
+
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == fabricIndex);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 1001);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 44);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId);
+            NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0);
+
+            Crypto::P256PublicKey rootPublicKeyOfFabric;
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(fabricIndex, rootPublicKeyOfFabric));
+            NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey));
+        }
+
+        // Validate that fabric has the correct operational key by verifying a signature
+        {
+            Crypto::P256PublicKey nocPubKey;
+            NL_TEST_ASSERT_SUCCESS(inSuite, VerifyCertificateSigningRequest(csrSpan.data(), csrSpan.size(), nocPubKey));
+
+            Crypto::P256ECDSASignature sig;
+            uint8_t message[] = { 'm', 's', 'g' };
+
+            NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SignWithOpKeypair(fabricIndex, ByteSpan{ message }, sig));
+            NL_TEST_ASSERT_SUCCESS(inSuite, nocPubKey.ECDSA_validate_msg_signature(&message[0], sizeof(message), sig));
+        }
+
+        // Validate iterator sees the updated fabric
+        {
+            numFabricsIterated = 0;
+            bool saw1          = false;
+            for (const auto & iterFabricInfo : fabricTable)
+            {
+                ++numFabricsIterated;
+                if (iterFabricInfo.GetFabricIndex() == 1)
+                {
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetNodeId() == 1001);
+                    NL_TEST_ASSERT(inSuite, iterFabricInfo.GetFabricId() == 44);
+                    saw1 = true;
+                }
+            }
+
+            NL_TEST_ASSERT(inSuite, numFabricsIterated == 1);
+            NL_TEST_ASSERT(inSuite, saw1 == true);
+        }
+    }
 }
 
-void TestAddNocFailSafe(nlTestSuite * inSuite, void * inContext)
+void TestSequenceErrors(nlTestSuite * inSuite, void * inContext)
 {
     // TODO: Write test
 }
 
-void TestUpdateNocFailSafe(nlTestSuite * inSuite, void * inContext)
+void TestFabricLabelChange(nlTestSuite * inSuite, void * inContext)
 {
     // TODO: Write test
 }
 
-void TestSequenceErrors(nlTestSuite * inSuite, void * inContext)
+void TestCompressedFabricId(nlTestSuite * inSuite, void * inContext)
 {
     // TODO: Write test
 }
 
-void TestFabricLabelChange(nlTestSuite * inSuite, void * inContext)
+void TestAddNocRootCollision(nlTestSuite * inSuite, void * inContext)
 {
     // TODO: Write test
 }
 
-void TestCompressedFabricId(nlTestSuite * inSuite, void * inContext)
+void TestInvalidChaining(nlTestSuite * inSuite, void * inContext)
 {
     // TODO: Write test
 }
@@ -919,6 +1761,8 @@ static const nlTest sTests[] =
     NL_TEST_DEF("Test interlock sequencing errors", TestSequenceErrors),
     NL_TEST_DEF("Test fabric label changes", TestFabricLabelChange),
     NL_TEST_DEF("Test compressed fabric ID is properly generated", TestCompressedFabricId),
+    NL_TEST_DEF("Test AddNOC root collision", TestAddNocRootCollision),
+    NL_TEST_DEF("Test invalid chaining in AddNOC and UpdateNOC", TestInvalidChaining),
 
     NL_TEST_SENTINEL()
 };
diff --git a/src/lib/support/TestPersistentStorageDelegate.h b/src/lib/support/TestPersistentStorageDelegate.h
index 96067b978e14c3..c04b339c36e8c9 100644
--- a/src/lib/support/TestPersistentStorageDelegate.h
+++ b/src/lib/support/TestPersistentStorageDelegate.h
@@ -176,6 +176,24 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
      */
     virtual void SetLoggingLevel(LoggingLevel loggingLevel) { mLoggingLevel = loggingLevel; }
 
+    /**
+     * @brief Dump a list of all storage keys, sorted alphabetically
+     */
+    virtual void DumpKeys()
+    {
+        ChipLogError(Test, "TestPersistentStorageDelegate::DumpKeys: %u keys", static_cast<unsigned>(GetNumKeys()));
+
+        auto allKeys = GetKeys();
+        std::vector<std::string> allKeysSorted(allKeys.cbegin(), allKeys.cend());
+        std::sort(allKeysSorted.begin(), allKeysSorted.end());
+
+        for (const std::string & key : allKeysSorted)
+        {
+            (void) key.c_str(); // Guard against log level disabling  error logging which would make `key` unused.
+            ChipLogError(Test, "  -> %s", key.c_str());
+        }
+    }
+
 protected:
     virtual CHIP_ERROR SyncGetKeyValueInternal(const char * key, void * buffer, uint16_t & size)
     {