From c34cf0bc73248fe5bcf5cb33041d8ee3a102f029 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Tue, 18 Jan 2022 14:14:23 -0600 Subject: [PATCH 1/7] Allow for split commissioner / admin topology It is possible to implement Matter commissioning such that a commissioner node is not on any fabric, and delegates operational communication and invocation of the CommissioningComplete command to a different administrator node. In this configuration, the commissioner only communicates by PASE and may not ever have received operational credentials. To support such a configuration, this commit amends the commissioner to accept initialization to the kUndefinedFabricIndex. In this configuration, the commissioner does not bootstrap itself with any credentials, and is capable of conducting the commissioning procedure until these are needed. When it is found that credentials aren't available, the commissioner gracefully discontinues commissioning. Because CASESessionManager is using fabric association for operational discovery, discontinuation of the commissioning sequence occurs at attempted operational discovery. Fixes #13501 --- .../chip-tool/commands/common/CHIPCommand.cpp | 71 ++++++++++++------- .../chip-tool/commands/common/CHIPCommand.h | 14 +++- src/app/CASESessionManager.cpp | 1 + src/controller/CHIPDeviceController.cpp | 39 ++++++---- src/controller/CHIPDeviceController.h | 3 +- src/controller/CHIPDeviceControllerFactory.h | 2 +- 6 files changed, 85 insertions(+), 45 deletions(-) diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index d1293ccc7bc6bd..6f4cd763598d8e 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -30,9 +30,10 @@ using DeviceControllerFactory = chip::Controller::DeviceControllerFactory; -constexpr chip::FabricId kIdentityAlphaFabricId = 1; -constexpr chip::FabricId kIdentityBetaFabricId = 2; -constexpr chip::FabricId kIdentityGammaFabricId = 3; +constexpr chip::FabricIndex kIdentityNullFabricId = chip::kUndefinedFabricIndex; +constexpr chip::FabricIndex kIdentityAlphaFabricId = 1; +constexpr chip::FabricIndex kIdentityBetaFabricId = 2; +constexpr chip::FabricIndex kIdentityGammaFabricId = 3; CHIP_ERROR CHIPCommand::Run() { @@ -51,6 +52,7 @@ CHIP_ERROR CHIPCommand::Run() factoryInitParams.listenPort = static_cast(mDefaultStorage.GetListenPort() + CurrentCommissionerIndex()); ReturnLogErrorOnFailure(DeviceControllerFactory::GetInstance().Init(factoryInitParams)); + ReturnLogErrorOnFailure(InitializeCommissioner(kIdentityNull, kIdentityNullFabricId)); ReturnLogErrorOnFailure(InitializeCommissioner(kIdentityAlpha, kIdentityAlphaFabricId)); ReturnLogErrorOnFailure(InitializeCommissioner(kIdentityBeta, kIdentityBetaFabricId)); ReturnLogErrorOnFailure(InitializeCommissioner(kIdentityGamma, kIdentityGammaFabricId)); @@ -65,6 +67,7 @@ CHIP_ERROR CHIPCommand::Run() // since the CHIP thread and event queue have been stopped, preventing any thread // races. // + ReturnLogErrorOnFailure(ShutdownCommissioner(kIdentityNull)); ReturnLogErrorOnFailure(ShutdownCommissioner(kIdentityAlpha)); ReturnLogErrorOnFailure(ShutdownCommissioner(kIdentityBeta)); ReturnLogErrorOnFailure(ShutdownCommissioner(kIdentityGamma)); @@ -99,7 +102,8 @@ void CHIPCommand::StopTracing() void CHIPCommand::SetIdentity(const char * identity) { std::string name = std::string(identity); - if (name.compare(kIdentityAlpha) != 0 && name.compare(kIdentityBeta) != 0 && name.compare(kIdentityGamma) != 0) + if (name.compare(kIdentityAlpha) != 0 && name.compare(kIdentityBeta) != 0 && name.compare(kIdentityGamma) != 0 && + name.compare(kIdentityNull)) { ChipLogError(chipTool, "Unknown commissioner name: %s. Supported names are [%s, %s, %s]", name.c_str(), kIdentityAlpha, kIdentityBeta, kIdentityGamma); @@ -112,7 +116,8 @@ void CHIPCommand::SetIdentity(const char * identity) std::string CHIPCommand::GetIdentity() { std::string name = mCommissionerName.HasValue() ? mCommissionerName.Value() : kIdentityAlpha; - if (name.compare(kIdentityAlpha) != 0 && name.compare(kIdentityBeta) != 0 && name.compare(kIdentityGamma) != 0) + if (name.compare(kIdentityAlpha) != 0 && name.compare(kIdentityBeta) != 0 && name.compare(kIdentityGamma) != 0 && + name.compare(kIdentityNull)) { ChipLogError(chipTool, "Unknown commissioner name: %s. Supported names are [%s, %s, %s]", name.c_str(), kIdentityAlpha, kIdentityBeta, kIdentityGamma); @@ -122,9 +127,9 @@ std::string CHIPCommand::GetIdentity() return name; } -uint16_t CHIPCommand::CurrentCommissionerIndex() +chip::FabricIndex CHIPCommand::CurrentCommissionerIndex() { - uint16_t index = 0; + chip::FabricIndex index; std::string name = GetIdentity(); if (name.compare(kIdentityAlpha) == 0) @@ -139,9 +144,16 @@ uint16_t CHIPCommand::CurrentCommissionerIndex() { index = kIdentityGammaFabricId; } + else if (name.compare(kIdentityNull) == 0) + { + index = kIdentityNullFabricId; + } + else + { + VerifyOrDieWithMsg(false, chipTool, "Unknown commissioner name: %s. Supported names are [%s, %s, %s]", name.c_str(), + kIdentityAlpha, kIdentityBeta, kIdentityGamma); + } - VerifyOrDieWithMsg(index != 0, chipTool, "Unknown commissioner name: %s. Supported names are [%s, %s, %s]", name.c_str(), - kIdentityAlpha, kIdentityBeta, kIdentityGamma); return index; } @@ -156,7 +168,7 @@ CHIP_ERROR CHIPCommand::ShutdownCommissioner(std::string key) return mCommissioners[key].get()->Shutdown(); } -CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId fabricId) +CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricIndex fabricIndex) { chip::Platform::ScopedMemoryBuffer noc; chip::Platform::ScopedMemoryBuffer icac; @@ -172,28 +184,33 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId f VerifyOrReturnError(icac.Alloc(chip::Controller::kMaxCHIPDERCertLength), CHIP_ERROR_NO_MEMORY); VerifyOrReturnError(rcac.Alloc(chip::Controller::kMaxCHIPDERCertLength), CHIP_ERROR_NO_MEMORY); - chip::MutableByteSpan nocSpan(noc.Get(), chip::Controller::kMaxCHIPDERCertLength); - chip::MutableByteSpan icacSpan(icac.Get(), chip::Controller::kMaxCHIPDERCertLength); - chip::MutableByteSpan rcacSpan(rcac.Get(), chip::Controller::kMaxCHIPDERCertLength); - chip::Crypto::P256Keypair ephemeralKey; - ReturnLogErrorOnFailure(ephemeralKey.Initialize()); - // TODO - OpCreds should only be generated for pairing command - // store the credentials in persistent storage, and - // generate when not available in the storage. - ReturnLogErrorOnFailure(mCommissionerStorage.Init(key.c_str())); - ReturnLogErrorOnFailure(mCredIssuerCmds->InitializeCredentialsIssuer(mCommissionerStorage)); - ReturnLogErrorOnFailure(mCredIssuerCmds->GenerateControllerNOCChain(mCommissionerStorage.GetLocalNodeId(), fabricId, - ephemeralKey, rcacSpan, icacSpan, nocSpan)); + if (fabricIndex != chip::kUndefinedFabricIndex) + { + + // TODO - OpCreds should only be generated for pairing command + // store the credentials in persistent storage, and + // generate when not available in the storage. + ReturnLogErrorOnFailure(mCommissionerStorage.Init(key.c_str())); + ReturnLogErrorOnFailure(mCredIssuerCmds->InitializeCredentialsIssuer(mCommissionerStorage)); + + chip::MutableByteSpan nocSpan(noc.Get(), chip::Controller::kMaxCHIPDERCertLength); + chip::MutableByteSpan icacSpan(icac.Get(), chip::Controller::kMaxCHIPDERCertLength); + chip::MutableByteSpan rcacSpan(rcac.Get(), chip::Controller::kMaxCHIPDERCertLength); + + ReturnLogErrorOnFailure(ephemeralKey.Initialize()); + ReturnLogErrorOnFailure(mCredIssuerCmds->GenerateControllerNOCChain(mCommissionerStorage.GetLocalNodeId(), fabricIndex, + ephemeralKey, rcacSpan, icacSpan, nocSpan)); + commissionerParams.ephemeralKeypair = &ephemeralKey; + commissionerParams.controllerRCAC = rcacSpan; + commissionerParams.controllerICAC = icacSpan; + commissionerParams.controllerNOC = nocSpan; + } commissionerParams.storageDelegate = &mCommissionerStorage; - commissionerParams.fabricIndex = static_cast(fabricId); + commissionerParams.fabricIndex = fabricIndex; commissionerParams.operationalCredentialsDelegate = mCredIssuerCmds->GetCredentialIssuer(); - commissionerParams.ephemeralKeypair = &ephemeralKey; - commissionerParams.controllerRCAC = rcacSpan; - commissionerParams.controllerICAC = icacSpan; - commissionerParams.controllerNOC = nocSpan; commissionerParams.controllerVendorId = chip::VendorId::TestVendor1; ReturnLogErrorOnFailure(DeviceControllerFactory::GetInstance().SetupCommissioner(commissionerParams, *(commissioner.get()))); diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h index 930f5cd31c10a3..00d6fff2ece9f0 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.h +++ b/examples/chip-tool/commands/common/CHIPCommand.h @@ -30,6 +30,16 @@ class PersistentStorage; constexpr const char kIdentityAlpha[] = "alpha"; constexpr const char kIdentityBeta[] = "beta"; constexpr const char kIdentityGamma[] = "gamma"; +// The null fabric commissioner is a commissioner that isn't on a fabric. +// This is a legal configuration in which the commissioner delegates +// operational communication and invocation of the commssioning complete +// command to a separate on-fabric administrator node. +// +// The null-fabric-commissioner identity is provided here to demonstrate the +// commissioner portion of such an architecture. The null-fabric-commissioner +// can carry a commissioning flow up until the point of operational channel +// (CASE) communcation. +constexpr const char kIdentityNull[] = "null-fabric-commissioner"; class CHIPCommand : public Command { @@ -96,9 +106,9 @@ class CHIPCommand : public Command ChipDeviceCommissioner & CurrentCommissioner(); private: - CHIP_ERROR InitializeCommissioner(std::string key, chip::FabricId fabricId); + CHIP_ERROR InitializeCommissioner(std::string key, chip::FabricIndex fabricIndex); CHIP_ERROR ShutdownCommissioner(std::string key); - uint16_t CurrentCommissionerIndex(); + chip::FabricIndex CurrentCommissionerIndex(); std::map> mCommissioners; chip::Optional mCommissionerName; diff --git a/src/app/CASESessionManager.cpp b/src/app/CASESessionManager.cpp index 26d7e8cf75173e..a183dd424d95a5 100644 --- a/src/app/CASESessionManager.cpp +++ b/src/app/CASESessionManager.cpp @@ -69,6 +69,7 @@ void CASESessionManager::ReleaseSession(PeerId peerId) CHIP_ERROR CASESessionManager::ResolveDeviceAddress(FabricInfo * fabric, NodeId nodeId) { + VerifyOrReturnError(fabric != nullptr, CHIP_ERROR_INCORRECT_STATE); return mConfig.dnsResolver->ResolveNodeId(fabric->GetPeerIdForNode(nodeId), Inet::IPAddressType::kAny, Dnssd::Resolver::CacheBypass::On); } diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 7a8f657ef5dd6c..6bb04d4719a734 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -142,10 +142,13 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params) mOperationalCredentialsDelegate = params.operationalCredentialsDelegate; mFabricIndex = params.fabricIndex; - ReturnErrorOnFailure(ProcessControllerNOCChain(params)); - - mFabricInfo = params.systemState->Fabrics()->FindFabricWithIndex(mFabricIndex); - VerifyOrReturnError(mFabricInfo != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + mVendorId = params.controllerVendorId; + if (mFabricIndex != kUndefinedFabricIndex) + { + ReturnErrorOnFailure(ProcessControllerNOCChain(params)); + mFabricInfo = params.systemState->Fabrics()->FindFabricWithIndex(mFabricIndex); + VerifyOrReturnError(mFabricInfo != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + } DeviceProxyInitParams deviceInitParams = { .sessionManager = params.systemState->SessionMgr(), @@ -215,8 +218,6 @@ CHIP_ERROR DeviceController::ProcessControllerNOCChain(const ControllerInitParam ReturnErrorOnFailure(fabric->SetFabricInfo(newFabric)); mLocalId = fabric->GetPeerId(); - mVendorId = fabric->GetVendorId(); - mFabricId = fabric->GetFabricId(); ChipLogProgress(Controller, "Joined the fabric at index %d. Compressed fabric ID is: 0x" ChipLogFormatX64, mFabricIndex, @@ -273,7 +274,11 @@ void DeviceController::OnFirstMessageDeliveryFailed(const SessionHandle & sessio VerifyOrReturn(mState == State::Initialized, ChipLogError(Controller, "OnFirstMessageDeliveryFailed was called in incorrect state")); VerifyOrReturn(session->GetSessionType() == Transport::Session::SessionType::kSecure); - UpdateDevice(session->AsSecureSession()->GetPeerNodeId()); + CHIP_ERROR err = UpdateDevice(session->AsSecureSession()->GetPeerNodeId()); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Controller, "OnFirstMessageDeliveryFailed was called, but UpdateDevice did not succeed (%s)", ErrorStr(err)); + } } CHIP_ERROR DeviceController::InitializePairedDeviceList() @@ -773,11 +778,8 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re uint16_t keyID = 0; - FabricInfo * fabric = mSystemState->Fabrics()->FindFabricWithIndex(mFabricIndex); - VerifyOrExit(mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE); VerifyOrExit(mDeviceBeingCommissioned == nullptr, err = CHIP_ERROR_INCORRECT_STATE); - VerifyOrExit(fabric != nullptr, err = CHIP_ERROR_INCORRECT_STATE); err = InitializePairedDeviceList(); SuccessOrExit(err); @@ -808,7 +810,7 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re mIsIPRendezvous = (params.GetPeerAddress().GetTransportType() != Transport::Type::kBle); - device->Init(GetControllerDeviceInitParams(), remoteDeviceId, peerAddress, fabric->GetFabricIndex()); + device->Init(GetControllerDeviceInitParams(), remoteDeviceId, peerAddress, mFabricIndex); if (params.GetPeerAddress().GetTransportType() != Transport::Type::kBle) { @@ -1214,8 +1216,11 @@ CHIP_ERROR DeviceCommissioner::ProcessOpCSR(DeviceProxy * proxy, const ByteSpan mOperationalCredentialsDelegate->SetNodeIdForNextNOCRequest(proxy->GetDeviceId()); - FabricInfo * fabric = mSystemState->Fabrics()->FindFabricWithIndex(mFabricIndex); - mOperationalCredentialsDelegate->SetFabricIdForNextNOCRequest(fabric->GetFabricId()); + if (mFabricIndex != kUndefinedFabricIndex) + { + FabricInfo * fabric = mSystemState->Fabrics()->FindFabricWithIndex(mFabricIndex); + mOperationalCredentialsDelegate->SetFabricIdForNextNOCRequest(fabric->GetFabricId()); + } return mOperationalCredentialsDelegate->GenerateNOCChain(NOCSRElements, AttestationSignature, dac, ByteSpan(), ByteSpan(), &mDeviceNOCChainCallback); @@ -1797,7 +1802,13 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } break; case CommissioningStage::kFindOperational: { - UpdateDevice(proxy->GetDeviceId()); + CHIP_ERROR err = UpdateDevice(proxy->GetDeviceId()); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Unable to proceed to operational discovery\n"); + CommissioningStageComplete(err); + return; + } } break; case CommissioningStage::kSendComplete: { diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 53e1d3dbb3651f..6da9ffbf1282b1 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -114,6 +114,7 @@ struct ControllerInitParams uint16_t controllerVendorId; + /* Set fabricIndex to kUndefinedFabricIndex to defeat self commissioning. */ FabricIndex fabricIndex = kMinValidFabricIndex; FabricId fabricId = kUndefinedFabricId; }; @@ -367,7 +368,7 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, void PersistNextKeyId(); - FabricIndex mFabricIndex = kMinValidFabricIndex; + FabricIndex mFabricIndex = kUndefinedFabricIndex; OperationalCredentialsDelegate * mOperationalCredentialsDelegate; diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index 87e1fb79426545..e94c60ef7a18b5 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -55,7 +55,7 @@ struct SetupParams ByteSpan controllerICAC; ByteSpan controllerRCAC; - FabricIndex fabricIndex = kMinValidFabricIndex; + FabricIndex fabricIndex = kUndefinedFabricIndex; FabricId fabricId = kUndefinedFabricId; uint16_t controllerVendorId; From 7a290c57992354e65275bf60d83aa7ce58e873c4 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Tue, 25 Jan 2022 17:38:02 -0600 Subject: [PATCH 2/7] Update src/controller/CHIPDeviceController.cpp Co-authored-by: Boris Zbarsky --- src/controller/CHIPDeviceController.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 6bb04d4719a734..6dc547f35bb732 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -277,7 +277,7 @@ void DeviceController::OnFirstMessageDeliveryFailed(const SessionHandle & sessio CHIP_ERROR err = UpdateDevice(session->AsSecureSession()->GetPeerNodeId()); if (err != CHIP_NO_ERROR) { - ChipLogError(Controller, "OnFirstMessageDeliveryFailed was called, but UpdateDevice did not succeed (%s)", ErrorStr(err)); + ChipLogError(Controller, "OnFirstMessageDeliveryFailed was called, but UpdateDevice did not succeed (%" CHIP_ERROR_FORMAT ")", err.Format()); } } From 9d71c3d09deb8bdc6f4365c718c00ecc5aad2874 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Tue, 25 Jan 2022 17:39:19 -0600 Subject: [PATCH 3/7] per bzbarsky-apple, fix name.compare for kIdentityNull --- examples/chip-tool/commands/common/CHIPCommand.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index 6f4cd763598d8e..c5fcf09ecc7135 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -103,7 +103,7 @@ void CHIPCommand::SetIdentity(const char * identity) { std::string name = std::string(identity); if (name.compare(kIdentityAlpha) != 0 && name.compare(kIdentityBeta) != 0 && name.compare(kIdentityGamma) != 0 && - name.compare(kIdentityNull)) + name.compare(kIdentityNull) != 0) { ChipLogError(chipTool, "Unknown commissioner name: %s. Supported names are [%s, %s, %s]", name.c_str(), kIdentityAlpha, kIdentityBeta, kIdentityGamma); @@ -117,7 +117,7 @@ std::string CHIPCommand::GetIdentity() { std::string name = mCommissionerName.HasValue() ? mCommissionerName.Value() : kIdentityAlpha; if (name.compare(kIdentityAlpha) != 0 && name.compare(kIdentityBeta) != 0 && name.compare(kIdentityGamma) != 0 && - name.compare(kIdentityNull)) + name.compare(kIdentityNull) != 0) { ChipLogError(chipTool, "Unknown commissioner name: %s. Supported names are [%s, %s, %s]", name.c_str(), kIdentityAlpha, kIdentityBeta, kIdentityGamma); From 8bec4794d6b280f130c8d4345fa8f4c7d52af9db Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Tue, 25 Jan 2022 17:49:09 -0600 Subject: [PATCH 4/7] restyle --- src/controller/CHIPDeviceController.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 6dc547f35bb732..0ee83c1b78fc12 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -277,7 +277,9 @@ void DeviceController::OnFirstMessageDeliveryFailed(const SessionHandle & sessio CHIP_ERROR err = UpdateDevice(session->AsSecureSession()->GetPeerNodeId()); if (err != CHIP_NO_ERROR) { - ChipLogError(Controller, "OnFirstMessageDeliveryFailed was called, but UpdateDevice did not succeed (%" CHIP_ERROR_FORMAT ")", err.Format()); + ChipLogError(Controller, + "OnFirstMessageDeliveryFailed was called, but UpdateDevice did not succeed (%" CHIP_ERROR_FORMAT ")", + err.Format()); } } From 3b03017ef4bbc5763370c8042b83fd214146594f Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Wed, 26 Jan 2022 09:21:14 -0600 Subject: [PATCH 5/7] per tcarmelveilleux, chip-tool cannot specify fabric indices Previously, chip-tool had reused fabric ID as fabric index, and was specifying both during commissioner initialization. This is brittle though because it is not practical for chip-tool to always know the fabric index ahead of time before initialization. This commit changes the commissioner to no longer accept a fabric index for initialization. Instead, the operational key pair and NOC chain are optionally passed. If present, these are added or merged into the fabric table. Merge works by identifying an existing, matching fabric in the table by comparing root public key and fabric ID. Once the new certificates are merged, the fabric index is extracted from the fabric table. --- .../chip-tool/commands/common/CHIPCommand.cpp | 31 +++++----- .../chip-tool/commands/common/CHIPCommand.h | 4 +- src/controller/CHIPDeviceController.cpp | 59 +++++++++++-------- src/controller/CHIPDeviceController.h | 21 +++---- .../CHIPDeviceControllerFactory.cpp | 2 - src/controller/CHIPDeviceControllerFactory.h | 3 - src/credentials/CHIPCert.cpp | 9 +++ src/credentials/CHIPCert.h | 15 ++++- src/credentials/FabricTable.cpp | 43 +++++++++++--- src/credentials/FabricTable.h | 5 +- 10 files changed, 120 insertions(+), 72 deletions(-) diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index c5fcf09ecc7135..5795c03f871923 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -30,10 +30,10 @@ using DeviceControllerFactory = chip::Controller::DeviceControllerFactory; -constexpr chip::FabricIndex kIdentityNullFabricId = chip::kUndefinedFabricIndex; -constexpr chip::FabricIndex kIdentityAlphaFabricId = 1; -constexpr chip::FabricIndex kIdentityBetaFabricId = 2; -constexpr chip::FabricIndex kIdentityGammaFabricId = 3; +constexpr chip::FabricId kIdentityNullFabricId = chip::kUndefinedFabricId; +constexpr chip::FabricId kIdentityAlphaFabricId = 1; +constexpr chip::FabricId kIdentityBetaFabricId = 2; +constexpr chip::FabricId kIdentityGammaFabricId = 3; CHIP_ERROR CHIPCommand::Run() { @@ -49,7 +49,7 @@ CHIP_ERROR CHIPCommand::Run() chip::Controller::FactoryInitParams factoryInitParams; factoryInitParams.fabricStorage = &mFabricStorage; - factoryInitParams.listenPort = static_cast(mDefaultStorage.GetListenPort() + CurrentCommissionerIndex()); + factoryInitParams.listenPort = static_cast(mDefaultStorage.GetListenPort() + CurrentCommissionerId()); ReturnLogErrorOnFailure(DeviceControllerFactory::GetInstance().Init(factoryInitParams)); ReturnLogErrorOnFailure(InitializeCommissioner(kIdentityNull, kIdentityNullFabricId)); @@ -127,26 +127,26 @@ std::string CHIPCommand::GetIdentity() return name; } -chip::FabricIndex CHIPCommand::CurrentCommissionerIndex() +chip::FabricId CHIPCommand::CurrentCommissionerId() { - chip::FabricIndex index; + chip::FabricId id; std::string name = GetIdentity(); if (name.compare(kIdentityAlpha) == 0) { - index = kIdentityAlphaFabricId; + id = kIdentityAlphaFabricId; } else if (name.compare(kIdentityBeta) == 0) { - index = kIdentityBetaFabricId; + id = kIdentityBetaFabricId; } else if (name.compare(kIdentityGamma) == 0) { - index = kIdentityGammaFabricId; + id = kIdentityGammaFabricId; } else if (name.compare(kIdentityNull) == 0) { - index = kIdentityNullFabricId; + id = kIdentityNullFabricId; } else { @@ -154,7 +154,7 @@ chip::FabricIndex CHIPCommand::CurrentCommissionerIndex() kIdentityAlpha, kIdentityBeta, kIdentityGamma); } - return index; + return id; } chip::Controller::DeviceCommissioner & CHIPCommand::CurrentCommissioner() @@ -168,7 +168,7 @@ CHIP_ERROR CHIPCommand::ShutdownCommissioner(std::string key) return mCommissioners[key].get()->Shutdown(); } -CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricIndex fabricIndex) +CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId fabricId) { chip::Platform::ScopedMemoryBuffer noc; chip::Platform::ScopedMemoryBuffer icac; @@ -186,7 +186,7 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricInde chip::Crypto::P256Keypair ephemeralKey; - if (fabricIndex != chip::kUndefinedFabricIndex) + if (fabricId != chip::kUndefinedFabricId) { // TODO - OpCreds should only be generated for pairing command @@ -200,7 +200,7 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricInde chip::MutableByteSpan rcacSpan(rcac.Get(), chip::Controller::kMaxCHIPDERCertLength); ReturnLogErrorOnFailure(ephemeralKey.Initialize()); - ReturnLogErrorOnFailure(mCredIssuerCmds->GenerateControllerNOCChain(mCommissionerStorage.GetLocalNodeId(), fabricIndex, + ReturnLogErrorOnFailure(mCredIssuerCmds->GenerateControllerNOCChain(mCommissionerStorage.GetLocalNodeId(), fabricId, ephemeralKey, rcacSpan, icacSpan, nocSpan)); commissionerParams.ephemeralKeypair = &ephemeralKey; commissionerParams.controllerRCAC = rcacSpan; @@ -209,7 +209,6 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricInde } commissionerParams.storageDelegate = &mCommissionerStorage; - commissionerParams.fabricIndex = fabricIndex; commissionerParams.operationalCredentialsDelegate = mCredIssuerCmds->GetCredentialIssuer(); commissionerParams.controllerVendorId = chip::VendorId::TestVendor1; diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h index 00d6fff2ece9f0..2b5d185d9299f2 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.h +++ b/examples/chip-tool/commands/common/CHIPCommand.h @@ -106,9 +106,9 @@ class CHIPCommand : public Command ChipDeviceCommissioner & CurrentCommissioner(); private: - CHIP_ERROR InitializeCommissioner(std::string key, chip::FabricIndex fabricIndex); + CHIP_ERROR InitializeCommissioner(std::string key, chip::FabricId fabricId); CHIP_ERROR ShutdownCommissioner(std::string key); - chip::FabricIndex CurrentCommissionerIndex(); + chip::FabricId CurrentCommissionerId(); std::map> mCommissioners; chip::Optional mCommissionerName; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 0ee83c1b78fc12..c5aa1e1a7e6c73 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -141,13 +141,10 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params) VerifyOrReturnError(params.operationalCredentialsDelegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT); mOperationalCredentialsDelegate = params.operationalCredentialsDelegate; - mFabricIndex = params.fabricIndex; - mVendorId = params.controllerVendorId; - if (mFabricIndex != kUndefinedFabricIndex) + mVendorId = params.controllerVendorId; + if (params.ephemeralKeypair != nullptr || !params.controllerNOC.empty() || !params.controllerRCAC.empty()) { ReturnErrorOnFailure(ProcessControllerNOCChain(params)); - mFabricInfo = params.systemState->Fabrics()->FindFabricWithIndex(mFabricIndex); - VerifyOrReturnError(mFabricInfo != nullptr, CHIP_ERROR_INVALID_ARGUMENT); } DeviceProxyInitParams deviceInitParams = { @@ -182,12 +179,13 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params) CHIP_ERROR DeviceController::ProcessControllerNOCChain(const ControllerInitParams & params) { FabricInfo newFabric; - - ReturnErrorCodeIf(params.ephemeralKeypair == nullptr, CHIP_ERROR_INVALID_ARGUMENT); - newFabric.SetEphemeralKey(params.ephemeralKeypair); - constexpr uint32_t chipCertAllocatedLen = kMaxCHIPCertLength; chip::Platform::ScopedMemoryBuffer chipCert; + Credentials::P256PublicKeySpan rootPublicKey; + FabricId fabricId; + + ReturnErrorOnFailure(newFabric.SetEphemeralKey(params.ephemeralKeypair)); + newFabric.SetVendorId(params.controllerVendorId); ReturnErrorCodeIf(!chipCert.Alloc(chipCertAllocatedLen), CHIP_ERROR_NO_MEMORY); MutableByteSpan chipCertSpan(chipCert.Get(), chipCertAllocatedLen); @@ -211,17 +209,27 @@ CHIP_ERROR DeviceController::ProcessControllerNOCChain(const ControllerInitParam ReturnErrorOnFailure(ConvertX509CertToChipCert(params.controllerNOC, chipCertSpan)); ReturnErrorOnFailure(newFabric.SetNOCCert(chipCertSpan)); - newFabric.SetVendorId(params.controllerVendorId); + ReturnErrorOnFailure(ExtractFabricIdFromCert(chipCertSpan, &fabricId)); - FabricInfo * fabric = params.systemState->Fabrics()->FindFabricWithIndex(mFabricIndex); - ReturnErrorCodeIf(fabric == nullptr, CHIP_ERROR_INCORRECT_STATE); + ReturnErrorOnFailure(newFabric.GetRootPubkey(rootPublicKey)); + mFabricInfo = params.systemState->Fabrics()->FindFabric(rootPublicKey, fabricId); + if (mFabricInfo != nullptr) + { + ReturnErrorOnFailure(mFabricInfo->SetFabricInfo(newFabric)); + } + else + { + FabricIndex fabricIndex; + ReturnErrorOnFailure(params.systemState->Fabrics()->AddNewFabric(newFabric, &fabricIndex)); + mFabricInfo = params.systemState->Fabrics()->FindFabricWithIndex(fabricIndex); + ReturnErrorCodeIf(mFabricInfo == nullptr, CHIP_ERROR_INCORRECT_STATE); + } - ReturnErrorOnFailure(fabric->SetFabricInfo(newFabric)); - mLocalId = fabric->GetPeerId(); - mFabricId = fabric->GetFabricId(); + mLocalId = mFabricInfo->GetPeerId(); + mFabricId = mFabricInfo->GetFabricId(); - ChipLogProgress(Controller, "Joined the fabric at index %d. Compressed fabric ID is: 0x" ChipLogFormatX64, mFabricIndex, - ChipLogValueX64(GetCompressedFabricId())); + ChipLogProgress(Controller, "Joined the fabric at index %d. Compressed fabric ID is: 0x" ChipLogFormatX64, + mFabricInfo->GetFabricIndex(), ChipLogValueX64(GetCompressedFabricId())); return CHIP_NO_ERROR; } @@ -236,7 +244,10 @@ CHIP_ERROR DeviceController::Shutdown() mStorageDelegate = nullptr; - mSystemState->Fabrics()->ReleaseFabricIndex(mFabricIndex); + if (mFabricInfo != nullptr) + { + mFabricInfo->Reset(); + } mSystemState->Release(); mSystemState = nullptr; @@ -264,7 +275,7 @@ bool DeviceController::DoesDevicePairingExist(const PeerId & deviceId) void DeviceController::ReleaseOperationalDevice(NodeId remoteDeviceId) { - VerifyOrReturn(mState == State::Initialized, + VerifyOrReturn(mState == State::Initialized && mFabricInfo != nullptr, ChipLogError(Controller, "ReleaseOperationalDevice was called in incorrect state")); mCASESessionManager->ReleaseSession(mFabricInfo->GetPeerIdForNode(remoteDeviceId)); } @@ -812,7 +823,10 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re mIsIPRendezvous = (params.GetPeerAddress().GetTransportType() != Transport::Type::kBle); - device->Init(GetControllerDeviceInitParams(), remoteDeviceId, peerAddress, mFabricIndex); + { + FabricIndex fabricIndex = mFabricInfo != nullptr ? mFabricInfo->GetFabricIndex() : kUndefinedFabricIndex; + device->Init(GetControllerDeviceInitParams(), remoteDeviceId, peerAddress, fabricIndex); + } if (params.GetPeerAddress().GetTransportType() != Transport::Type::kBle) { @@ -1218,10 +1232,9 @@ CHIP_ERROR DeviceCommissioner::ProcessOpCSR(DeviceProxy * proxy, const ByteSpan mOperationalCredentialsDelegate->SetNodeIdForNextNOCRequest(proxy->GetDeviceId()); - if (mFabricIndex != kUndefinedFabricIndex) + if (mFabricInfo != nullptr) { - FabricInfo * fabric = mSystemState->Fabrics()->FindFabricWithIndex(mFabricIndex); - mOperationalCredentialsDelegate->SetFabricIdForNextNOCRequest(fabric->GetFabricId()); + mOperationalCredentialsDelegate->SetFabricIdForNextNOCRequest(mFabricInfo->GetFabricId()); } return mOperationalCredentialsDelegate->GenerateNOCChain(NOCSRElements, AttestationSignature, dac, ByteSpan(), ByteSpan(), diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 6da9ffbf1282b1..bff5aa9591f9f1 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -113,10 +113,6 @@ struct ControllerInitParams ByteSpan controllerRCAC; uint16_t controllerVendorId; - - /* Set fabricIndex to kUndefinedFabricIndex to defeat self commissioning. */ - FabricIndex fabricIndex = kMinValidFabricIndex; - FabricId fabricId = kUndefinedFabricId; }; class DLL_EXPORT DevicePairingDelegate @@ -221,7 +217,7 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, virtual CHIP_ERROR GetConnectedDevice(NodeId deviceId, Callback::Callback * onConnection, Callback::Callback * onFailure) { - VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mState == State::Initialized && mFabricInfo != nullptr, CHIP_ERROR_INCORRECT_STATE); return mCASESessionManager->FindOrEstablishSession(mFabricInfo->GetPeerIdForNode(deviceId), onConnection, onFailure); } @@ -235,7 +231,7 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, */ CHIP_ERROR UpdateDevice(NodeId deviceId) { - VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mState == State::Initialized && mFabricInfo != nullptr, CHIP_ERROR_INCORRECT_STATE); return mCASESessionManager->ResolveDeviceAddress(mFabricInfo, deviceId); } @@ -347,9 +343,9 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, SerializableU64Set mPairedDevices; bool mPairedDevicesInitialized; - PeerId mLocalId = PeerId(); - FabricId mFabricId = kUndefinedFabricId; - FabricInfo * mFabricInfo; + PeerId mLocalId = PeerId(); + FabricId mFabricId = kUndefinedFabricId; + FabricInfo * mFabricInfo = nullptr; PersistentStorageDelegate * mStorageDelegate = nullptr; #if CHIP_DEVICE_CONFIG_ENABLE_DNSSD @@ -368,8 +364,6 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, void PersistNextKeyId(); - FabricIndex mFabricIndex = kUndefinedFabricIndex; - OperationalCredentialsDelegate * mOperationalCredentialsDelegate; SessionIDAllocator mIDAllocator; @@ -400,7 +394,10 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, CHIP_ERROR OpenCommissioningWindowInternal(); - PeerId GetPeerIdWithCommissioningWindowOpen() { return mFabricInfo->GetPeerIdForNode(mDeviceWithCommissioningWindowOpen); } + PeerId GetPeerIdWithCommissioningWindowOpen() + { + return mFabricInfo ? mFabricInfo->GetPeerIdForNode(mDeviceWithCommissioningWindowOpen) : PeerId(); + } // TODO - Support opening commissioning window simultaneously on multiple devices Callback::Callback * mCommissioningWindowCallback = nullptr; diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index cabf6a4b89ecab..a4fbb9892ac236 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -172,8 +172,6 @@ void DeviceControllerFactory::PopulateInitParams(ControllerInitParams & controll controllerParams.controllerNOC = params.controllerNOC; controllerParams.controllerICAC = params.controllerICAC; controllerParams.controllerRCAC = params.controllerRCAC; - controllerParams.fabricIndex = params.fabricIndex; - controllerParams.fabricId = params.fabricId; controllerParams.storageDelegate = params.storageDelegate; controllerParams.systemState = mSystemState; diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index e94c60ef7a18b5..3cd61d04e563f5 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -55,9 +55,6 @@ struct SetupParams ByteSpan controllerICAC; ByteSpan controllerRCAC; - FabricIndex fabricIndex = kUndefinedFabricIndex; - FabricId fabricId = kUndefinedFabricId; - uint16_t controllerVendorId; // The Device Pairing Delegated used to initialize a Commissioner diff --git a/src/credentials/CHIPCert.cpp b/src/credentials/CHIPCert.cpp index ba3123dd08db95..3a765e9a2c97af 100644 --- a/src/credentials/CHIPCert.cpp +++ b/src/credentials/CHIPCert.cpp @@ -945,6 +945,15 @@ CHIP_ERROR ExtractCATsFromOpCert(const ChipCertificateData & opcert, CATValues & return CHIP_NO_ERROR; } +CHIP_ERROR ExtractFabricIdFromCert(const ByteSpan & opcert, FabricId * fabricId) +{ + ChipCertificateSet certSet; + ChipCertificateData certData; + ReturnErrorOnFailure(certSet.Init(&certData, 1)); + ReturnErrorOnFailure(certSet.LoadCert(opcert, BitFlags())); + return ExtractFabricIdFromCert(certData, fabricId); +} + CHIP_ERROR ExtractNodeIdFabricIdFromOpCert(const ByteSpan & opcert, NodeId * nodeId, FabricId * fabricId) { ChipCertificateSet certSet; diff --git a/src/credentials/CHIPCert.h b/src/credentials/CHIPCert.h index 00b1427c976e8c..68c70cbfe08eab 100644 --- a/src/credentials/CHIPCert.h +++ b/src/credentials/CHIPCert.h @@ -774,9 +774,8 @@ CHIP_ERROR ConvertECDSASignatureRawToDER(P256ECDSASignatureSpan rawSig, ASN1::AS CHIP_ERROR ConvertECDSASignatureDERToRaw(ASN1::ASN1Reader & reader, chip::TLV::TLVWriter & writer, uint64_t tag); /** - * Extract the FabricID from a CHIP certificate in ByteSpan TLV-encoded - * form. This does not perform any sort of validation on the certificate - * structure other than parsing it. + * Extract the Fabric ID from an operational certificate that has already been + * parsed. * * This function can be used to extract Fabric ID from an ICA certificate. * These certificates may not contain a NodeID, so ExtractNodeIdFabricIdFromOpCert() @@ -837,6 +836,16 @@ CHIP_ERROR ExtractCATsFromOpCert(const ByteSpan & opcert, CATValues & cats); */ CHIP_ERROR ExtractCATsFromOpCert(const ChipCertificateData & opcert, CATValues & cats); +/** + * Extract the and Fabric ID from an operational certificate in ByteSpan TLV-encoded + * form. This does not perform any sort of validation on the certificate + * structure other than parsing it. + * + * Can return any error that can be returned from parsing the cert or from the + * ChipCertificateData* version of ExtractNodeIdFabricIdFromOpCert. + */ +CHIP_ERROR ExtractFabricIdFromCert(const ByteSpan & opcert, FabricId * fabricId); + /** * Extract Node ID and Fabric ID from an operational certificate in ByteSpan TLV-encoded * form. This does not perform any sort of validation on the certificate diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index 06fb786e4c2467..f0dea93f6b12e4 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -241,6 +241,7 @@ CHIP_ERROR FabricInfo::GenerateKey(FabricIndex id, char * key, size_t len) CHIP_ERROR FabricInfo::SetEphemeralKey(const P256Keypair * key) { + VerifyOrReturnError(key != nullptr, CHIP_ERROR_INVALID_ARGUMENT); P256SerializedKeypair serialized; ReturnErrorOnFailure(key->Serialize(serialized)); if (mOperationalKey == nullptr) @@ -402,6 +403,30 @@ void FabricTable::ReleaseFabricIndex(FabricIndex fabricIndex) } } +FabricInfo * FabricTable::FindFabric(P256PublicKeySpan rootPubKey, FabricId fabricId) +{ + static_assert(kMaxValidFabricIndex <= UINT8_MAX, "Cannot create more fabrics than UINT8_MAX"); + for (FabricIndex i = kMinValidFabricIndex; i <= kMaxValidFabricIndex; i++) + { + FabricInfo * fabric = FindFabricWithIndex(i); + if (fabric == nullptr) + { + continue; + } + P256PublicKeySpan candidatePubKey; + if (fabric->GetRootPubkey(candidatePubKey) != CHIP_NO_ERROR) + { + continue; + } + if (rootPubKey.data_equal(candidatePubKey) && fabricId == fabric->GetFabricId()) + { + LoadFromStorage(fabric); + return fabric; + } + } + return nullptr; +} + FabricInfo * FabricTable::FindFabricWithIndex(FabricIndex fabricIndex) { if (fabricIndex >= kMinValidFabricIndex && fabricIndex <= kMaxValidFabricIndex) @@ -446,21 +471,21 @@ void FabricTable::Reset() } } -CHIP_ERROR FabricTable::Store(FabricIndex id) +CHIP_ERROR FabricTable::Store(FabricIndex index) { CHIP_ERROR err = CHIP_NO_ERROR; FabricInfo * fabric = nullptr; VerifyOrExit(mStorage != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); - fabric = FindFabricWithIndex(id); + fabric = FindFabricWithIndex(index); VerifyOrExit(fabric != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); err = fabric->CommitToStorage(mStorage); exit: if (err == CHIP_NO_ERROR && mDelegate != nullptr) { - ChipLogProgress(Discovery, "Fabric (%d) persisted to storage. Calling OnFabricPersistedToStorage", id); + ChipLogProgress(Discovery, "Fabric (%d) persisted to storage. Calling OnFabricPersistedToStorage", index); mDelegate->OnFabricPersistedToStorage(fabric); } return err; @@ -561,21 +586,21 @@ CHIP_ERROR FabricTable::AddNewFabric(FabricInfo & newFabric, FabricIndex * outpu return CHIP_ERROR_NO_MEMORY; } -CHIP_ERROR FabricTable::Delete(FabricIndex id) +CHIP_ERROR FabricTable::Delete(FabricIndex index) { FabricInfo * fabric = nullptr; CHIP_ERROR err = CHIP_NO_ERROR; bool fabricIsInitialized = false; VerifyOrExit(mStorage != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); - fabric = FindFabricWithIndex(id); + fabric = FindFabricWithIndex(index); fabricIsInitialized = fabric != nullptr && fabric->IsInitialized(); - err = FabricInfo::DeleteFromStorage(mStorage, id); // Delete from storage regardless + err = FabricInfo::DeleteFromStorage(mStorage, index); // Delete from storage regardless exit: if (err == CHIP_NO_ERROR) { - ReleaseFabricIndex(id); + ReleaseFabricIndex(index); if (mDelegate != nullptr && fabricIsInitialized) { if (mFabricCount == 0) @@ -586,8 +611,8 @@ CHIP_ERROR FabricTable::Delete(FabricIndex id) { mFabricCount--; } - ChipLogProgress(Discovery, "Fabric (%d) deleted. Calling OnFabricDeletedFromStorage", id); - mDelegate->OnFabricDeletedFromStorage(id); + ChipLogProgress(Discovery, "Fabric (%d) deleted. Calling OnFabricDeletedFromStorage", index); + mDelegate->OnFabricDeletedFromStorage(index); } } return CHIP_NO_ERROR; diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index 02f984666f8914..d44ec75a9f882b 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -402,10 +402,10 @@ class DLL_EXPORT FabricTable { public: FabricTable() { Reset(); } - CHIP_ERROR Store(FabricIndex id); + CHIP_ERROR Store(FabricIndex index); CHIP_ERROR LoadFromStorage(FabricInfo * info); - CHIP_ERROR Delete(FabricIndex id); + CHIP_ERROR Delete(FabricIndex index); void DeleteAllFabrics(); /** @@ -421,6 +421,7 @@ class DLL_EXPORT FabricTable void ReleaseFabricIndex(FabricIndex fabricIndex); + FabricInfo * FindFabric(Credentials::P256PublicKeySpan rootPubKey, FabricId fabricId); FabricInfo * FindFabricWithIndex(FabricIndex fabricIndex); FabricInfo * FindFabricWithCompressedId(CompressedFabricId fabricId); From cc847c07e488e34bf57f937d37f18957e5ed02d8 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Wed, 26 Jan 2022 22:27:30 -0600 Subject: [PATCH 6/7] per tcarmelveilleux, add unit tests for new ExtractFabricIdFromCert overload --- src/credentials/tests/TestChipCert.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/credentials/tests/TestChipCert.cpp b/src/credentials/tests/TestChipCert.cpp index 2bc7623d97145e..4416218878b248 100644 --- a/src/credentials/tests/TestChipCert.cpp +++ b/src/credentials/tests/TestChipCert.cpp @@ -1077,6 +1077,19 @@ static void TestChipCert_ExtractNodeIdFabricId(nlTestSuite * inSuite, void * inC certSet.Release(); } + // Test fabric ID extraction from the raw ByteSpan form. + for (auto & testCase : sTestCases) + { + ByteSpan cert; + CHIP_ERROR err = GetTestCert(testCase.Cert, sNullLoadFlag, cert); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + FabricId fabricId; + err = ExtractFabricIdFromCert(cert, &fabricId); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, fabricId == testCase.ExpectedFabricId); + } + // Test fabric ID extraction from the parsed form. for (auto & testCase : sTestCases) { @@ -1093,6 +1106,17 @@ static void TestChipCert_ExtractNodeIdFabricId(nlTestSuite * inSuite, void * inC certSet.Release(); } + // Test fabric ID extraction from the raw ByteSpan form of ICA Cert that doesn't have FabricId. + { + ByteSpan cert; + CHIP_ERROR err = GetTestCert(TestCert::kICA01, sNullLoadFlag, cert); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + FabricId fabricId; + err = ExtractFabricIdFromCert(cert, &fabricId); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); + } + // Test extraction from the parsed form of ICA Cert that doesn't have FabricId. { CHIP_ERROR err = certSet.Init(1); From daa3113e92d268a864fe6ac376f4c60d20a86fb8 Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Wed, 26 Jan 2022 23:08:43 -0600 Subject: [PATCH 7/7] restyle --- examples/chip-tool/commands/common/CHIPCommand.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index 4fff9517c780fd..3fd71824945e8e 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -203,9 +203,9 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId f ReturnLogErrorOnFailure(mCredIssuerCmds->GenerateControllerNOCChain(mCommissionerStorage.GetLocalNodeId(), fabricId, ephemeralKey, rcacSpan, icacSpan, nocSpan)); commissionerParams.operationalKeypair = &ephemeralKey; - commissionerParams.controllerRCAC = rcacSpan; - commissionerParams.controllerICAC = icacSpan; - commissionerParams.controllerNOC = nocSpan; + commissionerParams.controllerRCAC = rcacSpan; + commissionerParams.controllerICAC = icacSpan; + commissionerParams.controllerNOC = nocSpan; } commissionerParams.storageDelegate = &mCommissionerStorage;