Fix spec-compliance of AddNOC by removing stale code (#17360)

- With Issue #13711, it was assumed only PASE sessions needed to adopt
  FabricIndex of new fabric after AddNOC, but the door was left open
  until #13711 was fixed. It is now fixed so the TODO is gone and
  PASE-only is enforced.

This PR:

- Removes the TODO and associated comment, and enforces PASE
- Renames "NewFabric" to "AdoptFabricIndex" in SecureSession
- Cleans-up some logging in NOC cluster and FabricTable
  that was found while doing this work

Issue #13711

Testing done:
- All cert tests still pass
- Unit tests pass
- It's not possible to do a full commissioning over CASE yet
  with YAML so I can't make a YAML test case, the code delta
  on logic is such that by inspection it would do the right thing.

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

@@ -260,7 +260,7 @@ CHIP_ERROR ComputeAttestationSignature(app::CommandHandler * commandObj,
 FabricInfo * RetrieveCurrentFabric(CommandHandler * aCommandHandler)
 {
     FabricIndex index = aCommandHandler->GetAccessingFabricIndex();
-    emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: Finding fabric with fabricIndex %d", index);
+    emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: Finding fabric with fabricIndex 0x%x", static_cast<unsigned>(index));
     return Server::GetInstance().GetFabricTable().FindFabricWithIndex(index);
 }
 
@@ -356,9 +356,10 @@ class OpCredsFabricTableDelegate : public FabricTableDelegate
 {
 
     // Gets called when a fabric is deleted from KVS store
-    void OnFabricDeletedFromStorage(CompressedFabricId compressedFabricId, FabricIndex fabricId) override
+    void OnFabricDeletedFromStorage(CompressedFabricId compressedFabricId, FabricIndex fabricIndex) override
     {
-        emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: Fabric 0x%u was deleted from fabric storage.", fabricId);
+        emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: Fabric index 0x%x was deleted from fabric storage.",
+                       static_cast<unsigned>(fabricIndex));
         fabricListChanged();
 
         // The Leave event SHOULD be emitted by a Node prior to permanently
@@ -376,24 +377,24 @@ class OpCredsFabricTableDelegate : public FabricTableDelegate
         }
     }
 
-    // Gets called when a fabric is loaded into the FabricTable from KVS store.
+    // Gets called when a fabric is loaded into the FabricTable from storage
     void OnFabricRetrievedFromStorage(FabricInfo * fabric) override
     {
         emberAfPrintln(EMBER_AF_PRINT_DEBUG,
-                       "OpCreds: Fabric 0x%u was retrieved from storage. FabricId 0x" ChipLogFormatX64
+                       "OpCreds: Fabric index 0x%x was retrieved from storage. FabricId 0x" ChipLogFormatX64
                        ", NodeId 0x" ChipLogFormatX64 ", VendorId 0x%04" PRIX16,
-                       fabric->GetFabricIndex(), ChipLogValueX64(fabric->GetFabricId()),
+                       static_cast<unsigned>(fabric->GetFabricIndex()), ChipLogValueX64(fabric->GetFabricId()),
                        ChipLogValueX64(fabric->GetPeerId().GetNodeId()), fabric->GetVendorId());
         fabricListChanged();
     }
 
-    // Gets called when a fabric in FabricTable is persisted to KVS store.
+    // Gets called when a fabric in FabricTable is persisted to storage
     void OnFabricPersistedToStorage(FabricInfo * fabric) override
     {
         emberAfPrintln(EMBER_AF_PRINT_DEBUG,
-                       "OpCreds: Fabric %X was persisted to storage. FabricId " ChipLogFormatX64 ", NodeId " ChipLogFormatX64
-                       ", VendorId 0x%04" PRIX16,
-                       fabric->GetFabricIndex(), ChipLogValueX64(fabric->GetFabricId()),
+                       "OpCreds: Fabric  index 0x%x was persisted to storage. FabricId " ChipLogFormatX64
+                       ", NodeId " ChipLogFormatX64 ", VendorId 0x%04" PRIX16,
+                       static_cast<unsigned>(fabric->GetFabricIndex()), ChipLogValueX64(fabric->GetFabricId()),
                        ChipLogValueX64(fabric->GetPeerId().GetNodeId()), fabric->GetVendorId());
         fabricListChanged();
     }
@@ -594,24 +595,26 @@ bool emberAfOperationalCredentialsClusterAddNOCCallback(app::CommandHandler * co
                                                         const Commands::AddNOC::DecodableType & commandData)
 {
     MATTER_TRACE_EVENT_SCOPE("AddNOC", "OperationalCredentials");
-    auto & NOCValue      = commandData.NOCValue;
-    auto & ICACValue     = commandData.ICACValue;
-    auto & adminVendorId = commandData.adminVendorId;
-    auto & ipkValue      = commandData.IPKValue;
-    auto * groups        = Credentials::GetGroupDataProvider();
-    auto nocResponse     = OperationalCertStatus::kSuccess;
+    auto & NOCValue          = commandData.NOCValue;
+    auto & ICACValue         = commandData.ICACValue;
+    auto & adminVendorId     = commandData.adminVendorId;
+    auto & ipkValue          = commandData.IPKValue;
+    auto * groupDataProvider = Credentials::GetGroupDataProvider();
+    auto nocResponse         = OperationalCertStatus::kSuccess;
 
     CHIP_ERROR err          = CHIP_NO_ERROR;
     FabricIndex fabricIndex = 0;
     Credentials::GroupDataProvider::KeySet keyset;
     FabricInfo * newFabricInfo = nullptr;
 
+    auto * secureSession = commandObj->GetExchangeContext()->GetSessionHandle()->AsSecureSession();
+
     uint8_t compressed_fabric_id_buffer[sizeof(uint64_t)];
     MutableByteSpan compressed_fabric_id(compressed_fabric_id_buffer);
 
-    emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: commissioner has added a NOC");
+    emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: Received AddNOC command");
 
-    if (nullptr == groups)
+    if (nullptr == groupDataProvider)
     {
         LogErrorOnFailure(commandObj->AddStatus(commandPath, Status::Failure));
         return true;
@@ -657,9 +660,6 @@ bool emberAfOperationalCredentialsClusterAddNOCCallback(app::CommandHandler * co
     err = CreateAccessControlEntryForNewFabricAdministrator(fabricIndex, commandData.caseAdminNode);
     VerifyOrExit(err == CHIP_NO_ERROR, nocResponse = ConvertToNOCResponseStatus(err));
 
-    // Notify the secure session of the new fabric.
-    commandObj->GetExchangeContext()->GetSessionHandle()->AsSecureSession()->NewFabric(fabricIndex);
-
     // Set the Identity Protection Key (IPK)
     VerifyOrExit(ipkValue.size() == Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES,
                  nocResponse = ConvertToNOCResponseStatus(CHIP_ERROR_INVALID_ARGUMENT));
@@ -674,9 +674,27 @@ bool emberAfOperationalCredentialsClusterAddNOCCallback(app::CommandHandler * co
     err = newFabricInfo->GetCompressedId(compressed_fabric_id);
     VerifyOrExit(err == CHIP_NO_ERROR, nocResponse = ConvertToNOCResponseStatus(err));
 
-    err = groups->SetKeySet(fabricIndex, compressed_fabric_id, keyset);
+    err = groupDataProvider->SetKeySet(fabricIndex, compressed_fabric_id, keyset);
     VerifyOrExit(err == CHIP_NO_ERROR, nocResponse = ConvertToNOCResponseStatus(err));
 
+    /**
+     * . If the current secure session was established with PASE,
+     *   the receiver SHALL:
+     *     .. Augment the secure session context with the `FabricIndex` generated above
+     *        such that subsequent interactions have the proper accessing fabric.
+     *
+     * . If the current secure session was established with CASE, subsequent configuration
+     *   of the newly installed Fabric requires the opening of a new CASE session from the
+     *   Administrator from the Fabric just installed. This Administrator is the one listed
+     *   in the `CaseAdminNode` argument.
+     *
+     */
+    if (secureSession->GetSecureSessionType() == SecureSession::Type::kPASE)
+    {
+        err = secureSession->AdoptFabricIndex(fabricIndex);
+        VerifyOrExit(err == CHIP_NO_ERROR, nocResponse = ConvertToNOCResponseStatus(err));
+    }
+
     // We might have a new operational identity, so we should start advertising it right away.
     app::DnssdServer::Instance().AdvertiseOperational();
 
@@ -691,7 +709,8 @@ bool emberAfOperationalCredentialsClusterAddNOCCallback(app::CommandHandler * co
     }
     else
     {
-        emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: successfully added a NOC");
+        emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: successfully created fabric index %u via AddNOC",
+                       static_cast<unsigned>(fabricIndex));
     }
 
     return true;

src/credentials/FabricTable.cpp

@@ -153,7 +153,7 @@ CHIP_ERROR FabricInfo::LoadFromStorage(PersistentStorageDelegate * storage)
 {
     DefaultStorageKeyAllocator keyAlloc;
 
-    ChipLogProgress(Inet, "Loading from storage for fabric index %u", mFabricIndex);
+    ChipLogProgress(Inet, "Loading from storage for fabric index 0x%x", static_cast<unsigned>(mFabricIndex));
 
     // Scopes for "size" so we don't forget to re-initialize it between gets,
     // since each get modifies it.
@@ -195,11 +195,14 @@ CHIP_ERROR FabricInfo::LoadFromStorage(PersistentStorageDelegate * storage)
     }
 
     {
-        uint8_t buf[OpKeyTLVMaxSize()];
-        uint16_t size = sizeof(buf);
-        ReturnErrorOnFailure(storage->SyncGetKeyValue(keyAlloc.FabricOpKey(mFabricIndex), buf, size));
+        // Use a CapacityBoundBuffer to get RAII secret data clearing on scope exit.
+        Crypto::CapacityBoundBuffer<OpKeyTLVMaxSize()> buf;
+        uint16_t size = static_cast<uint16_t>(buf.Capacity());
+        ReturnErrorOnFailure(storage->SyncGetKeyValue(keyAlloc.FabricOpKey(mFabricIndex), buf.Bytes(), size));
+        buf.SetLength(static_cast<size_t>(size));
+
         TLV::ContiguousBufferTLVReader reader;
-        reader.Init(buf, size);
+        reader.Init(buf.Bytes(), buf.Length());
 
         ReturnErrorOnFailure(reader.Next(TLV::kTLVType_Structure, TLV::AnonymousTag()));
         TLV::TLVType containerType;
@@ -565,7 +568,8 @@ CHIP_ERROR FabricInfo::SetFabricInfo(FabricInfo & newFabric)
     SetNOCCert(newFabric.mNOCCert);
     SetVendorId(newFabric.GetVendorId());
     SetFabricLabel(newFabric.GetFabricLabel());
-    ChipLogProgress(Discovery, "Added new fabric at index: %d, Initialized: %d", GetFabricIndex(), IsInitialized());
+    ChipLogProgress(Discovery, "Added new fabric at index: 0x%x, Initialized: %d", static_cast<unsigned>(GetFabricIndex()),
+                    IsInitialized());
     ChipLogProgress(Discovery, "Assigned compressed fabric ID: 0x" ChipLogFormatX64 ", node ID: 0x" ChipLogFormatX64,
                     ChipLogValueX64(mOperationalId.GetCompressedFabricId()), ChipLogValueX64(mOperationalId.GetNodeId()));
     return CHIP_NO_ERROR;
@@ -689,13 +693,13 @@ CHIP_ERROR FabricTable::Delete(FabricIndex index)
     {
         if (mFabricCount == 0)
         {
-            ChipLogError(Discovery, "!!Trying to delete a fabric, but the current fabric count is already 0");
+            ChipLogError(Discovery, "Trying to delete a fabric, but the current fabric count is already 0");
         }
         else
         {
             mFabricCount--;
+            ChipLogProgress(Discovery, "Fabric (0x%x) deleted. Calling OnFabricDeletedFromStorage", static_cast<unsigned>(index));
         }
-        ChipLogProgress(Discovery, "Fabric (%d) deleted. Calling OnFabricDeletedFromStorage", index);
 
         FabricTableDelegate * delegate = mDelegate;
         while (delegate)

src/transport/SecureSession.h

@@ -152,20 +152,16 @@ class SecureSession : public Session
     uint16_t GetLocalSessionId() const { return mLocalSessionId; }
     uint16_t GetPeerSessionId() const { return mPeerSessionId; }
 
-    // Should only be called for PASE sessions, which start with undefined fabric,
-    // to migrate to a newly commissioned fabric after successful
-    // OperationalCredentialsCluster::AddNOC
-    CHIP_ERROR NewFabric(FabricIndex fabricIndex)
+    // Called when AddNOC has gone through sufficient success that we need to switch the
+    // session to reflect a new fabric if it was a PASE session
+    CHIP_ERROR AdoptFabricIndex(FabricIndex fabricIndex)
     {
-#if 0
-        // TODO(#13711): this check won't work until the issue is addressed
-        if (mSecureSessionType == Type::kPASE)
+        // It's not legal to augment session type for non-PASE
+        if (mSecureSessionType != Type::kPASE)
         {
-            SetFabricIndex(fabricIndex);
+            return CHIP_ERROR_INVALID_ARGUMENT;
         }
-#else
         SetFabricIndex(fabricIndex);
-#endif
         return CHIP_NO_ERROR;
     }