From a8e5f74e411cb52fe9264a2cd9c05b0e1f773b75 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 9 Sep 2022 14:41:40 -0400 Subject: [PATCH] Fix --commissioner-nodeid handling in interactive mode. (#22496) 1. Instead of keying commissioners by just name in chip-tool, key them by the (name, commissioner-node-id) pair. 2. Allow multiple commissioners for the same fabric (as long as they have different node ids). Fixes https://github.com/project-chip/connectedhomeip/issues/22454 --- .../chip-tool/commands/common/CHIPCommand.cpp | 63 ++++++++++++++----- .../chip-tool/commands/common/CHIPCommand.h | 24 ++++++- .../commands/common/CHIPCommand.cpp | 5 +- 3 files changed, 70 insertions(+), 22 deletions(-) diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index 4baf3e15c8d07d..5c56a75c81da52 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -31,7 +31,7 @@ #include "TraceHandlers.h" #endif // CHIP_CONFIG_TRANSPORT_TRACE_ENABLED -std::map> CHIPCommand::mCommissioners; +std::map> CHIPCommand::mCommissioners; std::set CHIPCommand::sDeferredCleanups; using DeviceControllerFactory = chip::Controller::DeviceControllerFactory; @@ -125,7 +125,8 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack() ReturnErrorOnFailure(GetAttestationTrustStore(mPaaTrustStorePath.ValueOr(nullptr), &sTrustStore)); - ReturnLogErrorOnFailure(InitializeCommissioner(kIdentityNull, kIdentityNullFabricId)); + CommissionerIdentity nullIdentity{ kIdentityNull, chip::kUndefinedNodeId }; + ReturnLogErrorOnFailure(InitializeCommissioner(nullIdentity, kIdentityNullFabricId)); // After initializing first commissioner, add the additional CD certs once { @@ -176,7 +177,10 @@ void CHIPCommand::MaybeTearDownStack() CHIP_ERROR CHIPCommand::EnsureCommissionerForIdentity(std::string identity) { - if (mCommissioners.find(identity) != mCommissioners.end()) + chip::NodeId nodeId; + ReturnErrorOnFailure(GetCommissionerNodeId(identity, &nodeId)); + CommissionerIdentity lookupKey{ identity, nodeId }; + if (mCommissioners.find(lookupKey) != mCommissioners.end()) { return CHIP_NO_ERROR; } @@ -205,7 +209,7 @@ CHIP_ERROR CHIPCommand::EnsureCommissionerForIdentity(std::string identity) } } - return InitializeCommissioner(identity, fabricId); + return InitializeCommissioner(lookupKey, fabricId); } CHIP_ERROR CHIPCommand::Run() @@ -306,6 +310,27 @@ std::string CHIPCommand::GetIdentity() return name; } +CHIP_ERROR CHIPCommand::GetCommissionerNodeId(std::string identity, chip::NodeId * nodeId) +{ + if (mCommissionerNodeId.HasValue()) + { + *nodeId = mCommissionerNodeId.Value(); + return CHIP_NO_ERROR; + } + + if (identity == kIdentityNull) + { + *nodeId = chip::kUndefinedNodeId; + return CHIP_NO_ERROR; + } + + ReturnLogErrorOnFailure(mCommissionerStorage.Init(identity.c_str())); + + *nodeId = mCommissionerStorage.GetLocalNodeId(); + + return CHIP_NO_ERROR; +} + chip::FabricId CHIPCommand::CurrentCommissionerId() { chip::FabricId id; @@ -348,17 +373,20 @@ chip::Controller::DeviceCommissioner & CHIPCommand::GetCommissioner(std::string // identities. VerifyOrDie(EnsureCommissionerForIdentity(identity) == CHIP_NO_ERROR); - auto item = mCommissioners.find(identity); + chip::NodeId nodeId; + VerifyOrDie(GetCommissionerNodeId(identity, &nodeId) == CHIP_NO_ERROR); + CommissionerIdentity lookupKey{ identity, nodeId }; + auto item = mCommissioners.find(lookupKey); VerifyOrDie(item != mCommissioners.end()); return *item->second; } -void CHIPCommand::ShutdownCommissioner(std::string key) +void CHIPCommand::ShutdownCommissioner(const CommissionerIdentity & key) { mCommissioners[key].get()->Shutdown(); } -CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId fabricId) +CHIP_ERROR CHIPCommand::InitializeCommissioner(const CommissionerIdentity & identity, chip::FabricId fabricId) { chip::Platform::ScopedMemoryBuffer noc; chip::Platform::ScopedMemoryBuffer icac; @@ -381,7 +409,7 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId f // 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(mCommissionerStorage.Init(identity.mName.c_str())); if (mUseMaxSizedCerts.HasValue()) { auto option = CredentialIssuerCommands::CredentialIssuerOptions::kMaximizeCertificateSizes; @@ -395,14 +423,15 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId f chip::MutableByteSpan rcacSpan(rcac.Get(), chip::Controller::kMaxCHIPDERCertLength); ReturnLogErrorOnFailure(ephemeralKey.Initialize()); - chip::NodeId nodeId = mCommissionerNodeId.ValueOr(mCommissionerStorage.GetLocalNodeId()); - ReturnLogErrorOnFailure(mCredIssuerCmds->GenerateControllerNOCChain( - nodeId, fabricId, mCommissionerStorage.GetCommissionerCATs(), ephemeralKey, rcacSpan, icacSpan, nocSpan)); - commissionerParams.operationalKeypair = &ephemeralKey; - commissionerParams.controllerRCAC = rcacSpan; - commissionerParams.controllerICAC = icacSpan; - commissionerParams.controllerNOC = nocSpan; + ReturnLogErrorOnFailure(mCredIssuerCmds->GenerateControllerNOCChain(identity.mLocalNodeId, fabricId, + mCommissionerStorage.GetCommissionerCATs(), + ephemeralKey, rcacSpan, icacSpan, nocSpan)); + commissionerParams.operationalKeypair = &ephemeralKey; + commissionerParams.controllerRCAC = rcacSpan; + commissionerParams.controllerICAC = icacSpan; + commissionerParams.controllerNOC = nocSpan; + commissionerParams.permitMultiControllerFabrics = true; } // TODO: Initialize IPK epoch key in ExampleOperationalCredentials issuer rather than relying on DefaultIpkValue @@ -411,7 +440,7 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId f ReturnLogErrorOnFailure(DeviceControllerFactory::GetInstance().SetupCommissioner(commissionerParams, *(commissioner.get()))); - if (key != kIdentityNull) + if (identity.mName != kIdentityNull) { // Initialize Group Data, including IPK chip::FabricIndex fabricIndex = commissioner->GetFabricIndex(); @@ -428,7 +457,7 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId f chip::Credentials::SetSingleIpkEpochKey(&sGroupDataProvider, fabricIndex, defaultIpk, compressed_fabric_id_span)); } - mCommissioners[key] = std::move(commissioner); + mCommissioners[identity] = std::move(commissioner); return CHIP_NO_ERROR; } diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h index 35f562ce2ce7e8..3972b165c05e57 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.h +++ b/examples/chip-tool/commands/common/CHIPCommand.h @@ -128,6 +128,8 @@ class CHIPCommand : public Command #ifdef CONFIG_USE_LOCAL_STORAGE PersistentStorage mDefaultStorage; + // TODO: It's pretty weird that we re-init mCommissionerStorage for every + // identity without shutting it down or something in between... PersistentStorage mCommissionerStorage; #endif // CONFIG_USE_LOCAL_STORAGE chip::PersistentStorageOperationalKeystore mOperationalKeystore; @@ -137,6 +139,7 @@ class CHIPCommand : public Command CredentialIssuerCommands * mCredIssuerCmds; std::string GetIdentity(); + CHIP_ERROR GetCommissionerNodeId(std::string identity, chip::NodeId * nodeId); void SetIdentity(const char * name); // This method returns the commissioner instance to be used for running the command. @@ -152,10 +155,25 @@ class CHIPCommand : public Command CHIP_ERROR EnsureCommissionerForIdentity(std::string identity); - CHIP_ERROR InitializeCommissioner(std::string key, chip::FabricId fabricId); - void ShutdownCommissioner(std::string key); + // Commissioners are keyed by name and local node id. + struct CommissionerIdentity + { + bool operator<(const CommissionerIdentity & other) const + { + return mName < other.mName || (mName == other.mName && mLocalNodeId < other.mLocalNodeId); + } + std::string mName; + chip::NodeId mLocalNodeId; + }; + + // InitializeCommissioner uses various members, so can't be static. This is + // obviously a little odd, since the commissioners are then shared across + // multiple commands in interactive mode... + CHIP_ERROR InitializeCommissioner(const CommissionerIdentity & identity, chip::FabricId fabricId); + void ShutdownCommissioner(const CommissionerIdentity & key); chip::FabricId CurrentCommissionerId(); - static std::map> mCommissioners; + + static std::map> mCommissioners; static std::set sDeferredCleanups; chip::Optional mCommissionerName; diff --git a/examples/tv-casting-app/tv-casting-common/commands/common/CHIPCommand.cpp b/examples/tv-casting-app/tv-casting-common/commands/common/CHIPCommand.cpp index c99273641728ec..7eb9894a106fd9 100644 --- a/examples/tv-casting-app/tv-casting-common/commands/common/CHIPCommand.cpp +++ b/examples/tv-casting-app/tv-casting-common/commands/common/CHIPCommand.cpp @@ -86,12 +86,13 @@ void CHIPCommand::StopWaiting() chip::Controller::DeviceCommissioner & CHIPCommand::CurrentCommissioner() { - auto item = mCommissioners.find(GetIdentity()); + CommissionerIdentity identity{ GetIdentity(), chip::kUndefinedNodeId }; + auto item = mCommissioners.find(identity); return *item->second; } constexpr chip::FabricId kIdentityOtherFabricId = 4; -std::map> CHIPCommand::mCommissioners; +std::map> CHIPCommand::mCommissioners; std::string CHIPCommand::GetIdentity() {