From 98dbd416df2097643cadbaeba432d4a4bb8a8426 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Tue, 17 Sep 2024 13:52:22 -0700 Subject: [PATCH] Address review comments --- .../commands/common/CHIPCommand.cpp | 2 +- .../fabric-sync/FabricSyncCommand.cpp | 2 +- .../device_manager/DeviceManager.cpp | 1 - .../device_manager/PairingManager.cpp | 105 +++++++++++------- .../device_manager/PairingManager.h | 53 +++++++-- 5 files changed, 110 insertions(+), 53 deletions(-) diff --git a/examples/fabric-admin/commands/common/CHIPCommand.cpp b/examples/fabric-admin/commands/common/CHIPCommand.cpp index 4c955f846882da..b18eb9f2de472a 100644 --- a/examples/fabric-admin/commands/common/CHIPCommand.cpp +++ b/examples/fabric-admin/commands/common/CHIPCommand.cpp @@ -182,7 +182,7 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack() mCredIssuerCmds->SetCredentialIssuerOption(CredentialIssuerCommands::CredentialIssuerOptions::kAllowTestCdSigningKey, allowTestCdSigningKey); - ReturnLogErrorOnFailure(PairingManager::Instance().Init(&CurrentCommissioner())); + PairingManager::Instance().Init(&CurrentCommissioner()); return CHIP_NO_ERROR; } diff --git a/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp b/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp index 9a07d3b2e368eb..64d43cce11f3ac 100644 --- a/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp +++ b/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp @@ -344,7 +344,7 @@ CHIP_ERROR FabricSyncDeviceCommand::RunCommand(EndpointId remoteId) return CHIP_NO_ERROR; } - PairingManager::Instance().RegisterOpenCommissioningWindowDelegate(this); + PairingManager::Instance().SetOpenCommissioningWindowDelegate(this); DeviceMgr().OpenRemoteDeviceCommissioningWindow(remoteId); diff --git a/examples/fabric-admin/device_manager/DeviceManager.cpp b/examples/fabric-admin/device_manager/DeviceManager.cpp index c04d5d6291f78e..c35170ae509e1e 100644 --- a/examples/fabric-admin/device_manager/DeviceManager.cpp +++ b/examples/fabric-admin/device_manager/DeviceManager.cpp @@ -134,7 +134,6 @@ void DeviceManager::OpenRemoteDeviceCommissioningWindow(EndpointId remoteEndpoin // Open the commissioning window of a device from another fabric via its fabric bridge. // This method constructs and sends a command to open the commissioning window for a device // that is part of a different fabric, accessed through a fabric bridge. - StringBuilder commandBuilder; // Use random discriminator to have less chance of collision. uint16_t discriminator = diff --git a/examples/fabric-admin/device_manager/PairingManager.cpp b/examples/fabric-admin/device_manager/PairingManager.cpp index 0a711970f421cb..addc2effb12c28 100644 --- a/examples/fabric-admin/device_manager/PairingManager.cpp +++ b/examples/fabric-admin/device_manager/PairingManager.cpp @@ -24,26 +24,35 @@ using namespace ::chip; -Platform::UniquePtr PairingManager::mWindowOpener; - PairingManager::PairingManager() : mOnOpenCommissioningWindowCallback(OnOpenCommissioningWindowResponse, this), mOnOpenCommissioningWindowVerifierCallback(OnOpenCommissioningWindowVerifierResponse, this) {} -CHIP_ERROR PairingManager::Init(chip::Controller::DeviceCommissioner * commissioner) +void PairingManager::Init(Controller::DeviceCommissioner * commissioner) { + VerifyOrDie(mCommissioner == nullptr); mCommissioner = commissioner; - return CHIP_NO_ERROR; } CHIP_ERROR PairingManager::OpenCommissioningWindow(NodeId nodeId, EndpointId endpointId, uint16_t commissioningTimeout, uint32_t iterations, uint16_t discriminator, const ByteSpan & salt, const ByteSpan & verifier) { - VerifyOrReturnError(mCommissioner != nullptr, CHIP_ERROR_INCORRECT_STATE); + if (mCommissioner == nullptr) + { + ChipLogError(NotSpecified, "Commissioner is null, cannot open commissioning window"); + return CHIP_ERROR_INCORRECT_STATE; + } + + // Check if a window is already open + if (mWindowOpener != nullptr) + { + ChipLogError(NotSpecified, "A commissioning window is already open"); + return CHIP_ERROR_INCORRECT_STATE; + } - auto * params = new CommissioningWindowParams(); + auto params = Platform::MakeUnique(); params->nodeId = nodeId; params->endpointId = endpointId; params->commissioningWindowTimeout = commissioningTimeout; @@ -52,87 +61,99 @@ CHIP_ERROR PairingManager::OpenCommissioningWindow(NodeId nodeId, EndpointId end if (!salt.empty()) { + if (salt.size() > Crypto::kSpake2p_Max_PBKDF_Salt_Length) + { + ChipLogError(NotSpecified, "Salt size exceeds buffer capacity"); + return CHIP_ERROR_BUFFER_TOO_SMALL; + } + memcpy(params->saltBuffer, salt.data(), salt.size()); params->salt = ByteSpan(params->saltBuffer, salt.size()); } if (!verifier.empty()) { + if (verifier.size() > Crypto::kSpake2p_VerifierSerialized_Length) + { + ChipLogError(NotSpecified, "Verifier size exceeds buffer capacity"); + return CHIP_ERROR_BUFFER_TOO_SMALL; + } + memcpy(params->verifierBuffer, verifier.data(), verifier.size()); params->verifier = ByteSpan(params->verifierBuffer, verifier.size()); } // Schedule work on the Matter thread - chip::DeviceLayer::PlatformMgr().ScheduleWork(OnOpenCommissioningWindow, reinterpret_cast(params)); - - return CHIP_NO_ERROR; + return DeviceLayer::PlatformMgr().ScheduleWork(OnOpenCommissioningWindow, reinterpret_cast(params.release())); } void PairingManager::OnOpenCommissioningWindow(intptr_t context) { - auto * params = reinterpret_cast(context); + Platform::UniquePtr params(reinterpret_cast(context)); PairingManager & self = PairingManager::Instance(); if (self.mCommissioner == nullptr) { - ChipLogError(AppServer, "Commissioner is null, cannot open commissioning window"); + ChipLogError(NotSpecified, "Commissioner is null, cannot open commissioning window"); return; } - mWindowOpener = Platform::MakeUnique(self.mCommissioner); + self.mWindowOpener = Platform::MakeUnique(self.mCommissioner); if (!params->verifier.empty()) { if (params->salt.empty()) { - ChipLogError(AppServer, "Salt is required when verifier is set"); + ChipLogError(NotSpecified, "Salt is required when verifier is set"); + self.mWindowOpener.reset(); return; } - CHIP_ERROR err = mWindowOpener->OpenCommissioningWindow(Controller::CommissioningWindowVerifierParams() - .SetNodeId(params->nodeId) - .SetEndpointId(params->endpointId) - .SetTimeout(params->commissioningWindowTimeout) - .SetIteration(params->iteration) - .SetDiscriminator(params->discriminator) - .SetVerifier(params->verifier) - .SetSalt(params->salt) - .SetCallback(&self.mOnOpenCommissioningWindowVerifierCallback)); + CHIP_ERROR err = + self.mWindowOpener->OpenCommissioningWindow(Controller::CommissioningWindowVerifierParams() + .SetNodeId(params->nodeId) + .SetEndpointId(params->endpointId) + .SetTimeout(params->commissioningWindowTimeout) + .SetIteration(params->iteration) + .SetDiscriminator(params->discriminator) + .SetVerifier(params->verifier) + .SetSalt(params->salt) + .SetCallback(&self.mOnOpenCommissioningWindowVerifierCallback)); if (err != CHIP_NO_ERROR) { - ChipLogError(AppServer, "Failed to open commissioning window with verifier: %s", ErrorStr(err)); + ChipLogError(NotSpecified, "Failed to open commissioning window with verifier: %s", ErrorStr(err)); + self.mWindowOpener.reset(); } } else { SetupPayload ignored; - CHIP_ERROR err = mWindowOpener->OpenCommissioningWindow(Controller::CommissioningWindowPasscodeParams() - .SetNodeId(params->nodeId) - .SetEndpointId(params->endpointId) - .SetTimeout(params->commissioningWindowTimeout) - .SetIteration(params->iteration) - .SetDiscriminator(params->discriminator) - .SetSetupPIN(NullOptional) - .SetSalt(NullOptional) - .SetCallback(&self.mOnOpenCommissioningWindowCallback), - ignored); + CHIP_ERROR err = self.mWindowOpener->OpenCommissioningWindow(Controller::CommissioningWindowPasscodeParams() + .SetNodeId(params->nodeId) + .SetEndpointId(params->endpointId) + .SetTimeout(params->commissioningWindowTimeout) + .SetIteration(params->iteration) + .SetDiscriminator(params->discriminator) + .SetSetupPIN(NullOptional) + .SetSalt(NullOptional) + .SetCallback(&self.mOnOpenCommissioningWindowCallback), + ignored); if (err != CHIP_NO_ERROR) { - ChipLogError(AppServer, "Failed to open commissioning window with passcode: %s", ErrorStr(err)); + ChipLogError(NotSpecified, "Failed to open commissioning window with passcode: %s", ErrorStr(err)); + self.mWindowOpener.reset(); } } - - // Clean up params - delete params; } -void PairingManager::OnOpenCommissioningWindowResponse(void * context, NodeId remoteId, CHIP_ERROR err, chip::SetupPayload payload) +void PairingManager::OnOpenCommissioningWindowResponse(void * context, NodeId remoteId, CHIP_ERROR err, SetupPayload payload) { + VerifyOrDie(context != nullptr); PairingManager * self = static_cast(context); if (self->mCommissioningWindowDelegate) { self->mCommissioningWindowDelegate->OnCommissioningWindowOpened(remoteId, err, payload); - self->UnregisterOpenCommissioningWindowDelegate(); + self->SetOpenCommissioningWindowDelegate(nullptr); } OnOpenCommissioningWindowVerifierResponse(context, remoteId, err); @@ -140,8 +161,10 @@ void PairingManager::OnOpenCommissioningWindowResponse(void * context, NodeId re void PairingManager::OnOpenCommissioningWindowVerifierResponse(void * context, NodeId remoteId, CHIP_ERROR err) { + VerifyOrDie(context != nullptr); + PairingManager * self = static_cast(context); LogErrorOnFailure(err); - PairingManager * self = reinterpret_cast(context); - VerifyOrReturn(self != nullptr, ChipLogError(NotSpecified, "OnOpenCommissioningWindowCommand: context is null")); + // Reset the window opener once the window operation is complete + self->mWindowOpener.reset(); } diff --git a/examples/fabric-admin/device_manager/PairingManager.h b/examples/fabric-admin/device_manager/PairingManager.h index 015392d4f8e08c..3ff54cf1392095 100644 --- a/examples/fabric-admin/device_manager/PairingManager.h +++ b/examples/fabric-admin/device_manager/PairingManager.h @@ -29,6 +29,26 @@ class CommissioningWindowDelegate virtual ~CommissioningWindowDelegate() = default; }; +/** + * The PairingManager class is responsible for managing the commissioning and pairing process + * of Matter devices. PairingManager is designed to be used as a singleton, meaning that there + * should only be one instance of it running at any given time. + * + * Usage: + * + * 1. The class should be initialized when the system starts up, typically by invoking the static + * instance method to get the singleton. + * 2. To open a commissioning window, the appropriate method should be called on the PairingManager instance. + * 3. The PairingManager will handle the lifecycle of the CommissioningWindowOpener and ensure that + * resources are cleaned up appropriately when pairing is complete or the process is aborted. + * + * Example: + * + * @code + * PairingManager& manager = PairingManager::Instance(); + * manager.OpenCommissioningWindow(); + * @endcode + */ class PairingManager { public: @@ -38,19 +58,28 @@ class PairingManager return instance; } - CHIP_ERROR Init(chip::Controller::DeviceCommissioner * commissioner); + void Init(chip::Controller::DeviceCommissioner * commissioner); + /** + * Opens a commissioning window on the specified node and endpoint. + * Only one commissioning window can be active at a time. If a commissioning + * window is already open, this function will return an error. + * + * @param nodeId The target node ID for commissioning. + * @param endpointId The target endpoint ID for commissioning. + * @param commissioningTimeout Timeout for the commissioning window in seconds. + * @param iterations Iterations for PBKDF calculations. + * @param discriminator Discriminator for commissioning. + * @param salt Optional salt for verifier-based commissioning. + * @param verifier Optional verifier for enhanced commissioning security. + * + * @return CHIP_ERROR_INCORRECT_STATE if a commissioning window is already open. + */ CHIP_ERROR OpenCommissioningWindow(chip::NodeId nodeId, chip::EndpointId endpointId, uint16_t commissioningTimeout, uint32_t iterations, uint16_t discriminator, const chip::ByteSpan & salt, const chip::ByteSpan & verifier); - void RegisterOpenCommissioningWindowDelegate(CommissioningWindowDelegate * delegate) - { - ChipLogProgress(NotSpecified, "yujuan: PairingManager::RegisterOpenCommissioningWindowDelegate"); - mCommissioningWindowDelegate = delegate; - } - - void UnregisterOpenCommissioningWindowDelegate() { mCommissioningWindowDelegate = nullptr; } + void SetOpenCommissioningWindowDelegate(CommissioningWindowDelegate * delegate) { mCommissioningWindowDelegate = delegate; } private: PairingManager(); @@ -76,7 +105,13 @@ class PairingManager CommissioningWindowDelegate * mCommissioningWindowDelegate = nullptr; - static chip::Platform::UniquePtr mWindowOpener; + /** + * Holds the unique_ptr to the current CommissioningWindowOpener. + * Only one commissioning window opener can be active at a time. + * The pointer is reset when the commissioning window is closed or when an error occurs. + */ + chip::Platform::UniquePtr mWindowOpener; + static void OnOpenCommissioningWindow(intptr_t context); static void OnOpenCommissioningWindowResponse(void * context, chip::NodeId deviceId, CHIP_ERROR status, chip::SetupPayload payload);