Skip to content

Commit

Permalink
Fix --commissioner-nodeid handling in interactive mode. (#22496)
Browse files Browse the repository at this point in the history 
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 #22454
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Aug 21, 2023
1 parent 4bdf46d commit 3869190
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 22 deletions.
63 changes: 46 additions & 17 deletions examples/chip-tool/commands/common/CHIPCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
#include "TraceHandlers.h"
#endif // CHIP_CONFIG_TRANSPORT_TRACE_ENABLED

std::map<std::string, std::unique_ptr<chip::Controller::DeviceCommissioner>> CHIPCommand::mCommissioners;
std::map<CHIPCommand::CommissionerIdentity, std::unique_ptr<chip::Controller::DeviceCommissioner>> CHIPCommand::mCommissioners;
std::set<CHIPCommand *> CHIPCommand::sDeferredCleanups;

using DeviceControllerFactory = chip::Controller::DeviceControllerFactory;
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -205,7 +209,7 @@ CHIP_ERROR CHIPCommand::EnsureCommissionerForIdentity(std::string identity)
}
}

return InitializeCommissioner(identity, fabricId);
return InitializeCommissioner(lookupKey, fabricId);
}

CHIP_ERROR CHIPCommand::Run()
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<uint8_t> noc;
chip::Platform::ScopedMemoryBuffer<uint8_t> icac;
Expand All @@ -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;
Expand All @@ -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
Expand All @@ -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();
Expand All @@ -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;
}
Expand Down
24 changes: 21 additions & 3 deletions examples/chip-tool/commands/common/CHIPCommand.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand All @@ -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<std::string, std::unique_ptr<ChipDeviceCommissioner>> mCommissioners;

static std::map<CommissionerIdentity, std::unique_ptr<ChipDeviceCommissioner>> mCommissioners;
static std::set<CHIPCommand *> sDeferredCleanups;

chip::Optional<char *> mCommissionerName;
Expand Down
5 changes: 3 additions & 2 deletions examples/tv-casting-app/tv-casting-common/commands/common/CHIPCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string, std::unique_ptr<chip::Controller::DeviceCommissioner>> CHIPCommand::mCommissioners;
std::map<CHIPCommand::CommissionerIdentity, std::unique_ptr<chip::Controller::DeviceCommissioner>> CHIPCommand::mCommissioners;

std::string CHIPCommand::GetIdentity()
{
Expand Down

0 comments on commit 3869190

Please sign in to comment.