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;
  }