From 013421df4cd0ba0cfe166416a52a37941d548900 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Thu, 14 Apr 2022 10:33:32 -0400 Subject: [PATCH] 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. --- .../operational-credentials-server.cpp | 65 ++++++++++++------- src/credentials/FabricTable.cpp | 20 +++--- src/transport/SecureSession.h | 16 ++--- 3 files changed, 60 insertions(+), 41 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 166db42222cdde..94086dd97026ac 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/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(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(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(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(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(fabricIndex)); } return true; diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index 67cbec87fb19fa..e046391a38511b 100644 --- a/src/credentials/FabricTable.cpp +++ b/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(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 buf; + uint16_t size = static_cast(buf.Capacity()); + ReturnErrorOnFailure(storage->SyncGetKeyValue(keyAlloc.FabricOpKey(mFabricIndex), buf.Bytes(), size)); + buf.SetLength(static_cast(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(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(index)); } - ChipLogProgress(Discovery, "Fabric (%d) deleted. Calling OnFabricDeletedFromStorage", index); FabricTableDelegate * delegate = mDelegate; while (delegate) diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h index 8ae0b00bc94bda..1db5b6a550a697 100644 --- a/src/transport/SecureSession.h +++ b/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; }