From cf1f72737b8b981c506525636094bd8cc7992bfe Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Fri, 23 Aug 2024 14:56:51 -0700 Subject: [PATCH 1/4] Add state machine to CommissionerControlDelegate to guard various unexpected commands --- .../linux/CommissionerControl.cpp | 37 ++++++++++++++++++- .../linux/include/CommissionerControl.h | 12 ++++++ .../commissioner-control-server.cpp | 1 + .../commissioner-control-server.h | 9 +++++ 4 files changed, 58 insertions(+), 1 deletion(-) diff --git a/examples/fabric-bridge-app/linux/CommissionerControl.cpp b/examples/fabric-bridge-app/linux/CommissionerControl.cpp index 3613aeec9305e4..eef6aab7e565b6 100644 --- a/examples/fabric-bridge-app/linux/CommissionerControl.cpp +++ b/examples/fabric-bridge-app/linux/CommissionerControl.cpp @@ -46,8 +46,33 @@ namespace app { namespace Clusters { namespace CommissionerControl { +void CommissionerControlDelegate::ResetDelegateState() +{ + // Reset the step to the initial state + mNextStep = Step::kAcceptCommissioningApproval; + + // Reset identifiers and product information + mRequestId = 0; + mClientNodeId = kUndefinedNodeId; + mVendorId = VendorId::Unspecified; + mProductId = 0; + + // Clear the label buffer and optional label + memset(mLabelBuffer, 0, sizeof(mLabelBuffer)); + mLabel.ClearValue(); + + // Reset PBKDF salt and PAKE passcode verifier buffers + mPBKDFSalt = ByteSpan(); + memset(mPBKDFSaltBuffer, 0, sizeof(mPBKDFSaltBuffer)); + + mPAKEPasscodeVerifier = ByteSpan(); + memset(mPAKEPasscodeVerifierBuffer, 0, sizeof(mPAKEPasscodeVerifierBuffer)); +} + CHIP_ERROR CommissionerControlDelegate::HandleCommissioningApprovalRequest(const CommissioningApprovalRequest & request) { + VerifyOrReturnError(mNextStep == Step::kAcceptCommissioningApproval, CHIP_ERROR_BUSY); + CommissionerControl::Events::CommissioningRequestResult::Type result; result.requestId = request.requestId; result.clientNodeId = request.clientNodeId; @@ -86,19 +111,27 @@ CHIP_ERROR CommissionerControlDelegate::HandleCommissioningApprovalRequest(const mLabel.ClearValue(); } - return CommissionerControlServer::Instance().GenerateCommissioningRequestResultEvent(result); + CHIP_ERROR err = CommissionerControlServer::Instance().GenerateCommissioningRequestResultEvent(result); + + mNextStep = (err == CHIP_NO_ERROR) ? Step::kWaitCommissionNodeRequest : Step::kAcceptCommissioningApproval; + + return err; } CHIP_ERROR CommissionerControlDelegate::ValidateCommissionNodeCommand(NodeId clientNodeId, uint64_t requestId) { CHIP_ERROR err = CHIP_NO_ERROR; + VerifyOrReturnError(mNextStep == Step::kWaitCommissionNodeRequest, CHIP_ERROR_INCORRECT_STATE); + // Verify if the CommissionNode command is sent from the same NodeId as the RequestCommissioningApproval. VerifyOrExit(mClientNodeId == clientNodeId, err = CHIP_ERROR_WRONG_NODE_ID); // Verify if the provided RequestId matches the value provided to the RequestCommissioningApproval. VerifyOrExit(mRequestId == requestId, err = CHIP_ERROR_INCORRECT_STATE); + mNextStep = Step::kStartCommissionNode; + exit: return err; } @@ -131,6 +164,8 @@ CHIP_ERROR CommissionerControlDelegate::HandleCommissionNode(const Commissioning { ChipLogProgress(NotSpecified, "CommissionerControlDelegate::HandleCommissionNode"); + VerifyOrReturnError(mNextStep == Step::kStartCommissionNode, CHIP_ERROR_INCORRECT_STATE); + #if defined(PW_RPC_FABRIC_BRIDGE_SERVICE) && PW_RPC_FABRIC_BRIDGE_SERVICE return CommissionNode(Controller::CommissioningWindowPasscodeParams() .SetSetupPIN(kSetupPinCode) diff --git a/examples/fabric-bridge-app/linux/include/CommissionerControl.h b/examples/fabric-bridge-app/linux/include/CommissionerControl.h index 633be761ba1004..5719741f30dd6e 100644 --- a/examples/fabric-bridge-app/linux/include/CommissionerControl.h +++ b/examples/fabric-bridge-app/linux/include/CommissionerControl.h @@ -29,6 +29,7 @@ namespace CommissionerControl { class CommissionerControlDelegate : public Delegate { public: + void ResetDelegateState() override; CHIP_ERROR HandleCommissioningApprovalRequest(const CommissioningApprovalRequest & request) override; CHIP_ERROR ValidateCommissionNodeCommand(NodeId clientNodeId, uint64_t requestId) override; CHIP_ERROR GetCommissioningWindowParams(CommissioningWindowParams & outParams) override; @@ -38,8 +39,19 @@ class CommissionerControlDelegate : public Delegate ~CommissionerControlDelegate() = default; private: + enum class Step : uint8_t + { + // Ready to start reverse commissioning. + kAcceptCommissioningApproval, + // Wait for the commission node command. + kWaitCommissionNodeRequest, + // Need to commission node. + kStartCommissionNode, + }; + static constexpr size_t kLabelBufferSize = 64; + Step mNextStep = Step::kAcceptCommissioningApproval; uint64_t mRequestId = 0; NodeId mClientNodeId = kUndefinedNodeId; VendorId mVendorId = VendorId::Unspecified; diff --git a/src/app/clusters/commissioner-control-server/commissioner-control-server.cpp b/src/app/clusters/commissioner-control-server/commissioner-control-server.cpp index 4df397326894ed..9e40df15841a79 100644 --- a/src/app/clusters/commissioner-control-server/commissioner-control-server.cpp +++ b/src/app/clusters/commissioner-control-server/commissioner-control-server.cpp @@ -265,6 +265,7 @@ bool emberAfCommissionerControlClusterCommissionNodeCallback( { ChipLogError(Zcl, "emberAfCommissionerControlClusterCommissionNodeCallback error: %" CHIP_ERROR_FORMAT, err.Format()); commandObj->AddStatus(commandPath, StatusIB(err).mStatus); + delegate->ResetDelegateState(); } return true; diff --git a/src/app/clusters/commissioner-control-server/commissioner-control-server.h b/src/app/clusters/commissioner-control-server/commissioner-control-server.h index 60a4e81a93c3af..0ee42568bdc507 100644 --- a/src/app/clusters/commissioner-control-server/commissioner-control-server.h +++ b/src/app/clusters/commissioner-control-server/commissioner-control-server.h @@ -137,6 +137,15 @@ class Delegate virtual CHIP_ERROR HandleCommissionNode(const CommissioningWindowParams & params, const Optional & ipAddress, const Optional & port) = 0; + /** + * @brief Reset the state of the delegate. + * + * This method is used to reset the internal state of the delegate to its initial state. + * It should be called whenever the delegate needs to be reinitialized or cleaned up + * before handling a new commissioning sequence. + */ + virtual void ResetDelegateState() = 0; + virtual ~Delegate() = default; }; From e382e0474f739781774aca4418ef03320f9fa67e Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Fri, 23 Aug 2024 21:59:27 +0000 Subject: [PATCH 2/4] Restyled by whitespace --- examples/fabric-bridge-app/linux/CommissionerControl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/fabric-bridge-app/linux/CommissionerControl.cpp b/examples/fabric-bridge-app/linux/CommissionerControl.cpp index eef6aab7e565b6..e7853519c77406 100644 --- a/examples/fabric-bridge-app/linux/CommissionerControl.cpp +++ b/examples/fabric-bridge-app/linux/CommissionerControl.cpp @@ -164,7 +164,7 @@ CHIP_ERROR CommissionerControlDelegate::HandleCommissionNode(const Commissioning { ChipLogProgress(NotSpecified, "CommissionerControlDelegate::HandleCommissionNode"); - VerifyOrReturnError(mNextStep == Step::kStartCommissionNode, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mNextStep == Step::kStartCommissionNode, CHIP_ERROR_INCORRECT_STATE); #if defined(PW_RPC_FABRIC_BRIDGE_SERVICE) && PW_RPC_FABRIC_BRIDGE_SERVICE return CommissionNode(Controller::CommissioningWindowPasscodeParams() From 96b3e077339568ff977d14dbaf578b1c4b3e27e6 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Fri, 23 Aug 2024 21:59:31 +0000 Subject: [PATCH 3/4] Restyled by clang-format --- examples/fabric-bridge-app/linux/CommissionerControl.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/examples/fabric-bridge-app/linux/CommissionerControl.cpp b/examples/fabric-bridge-app/linux/CommissionerControl.cpp index e7853519c77406..0f2cd01adf630d 100644 --- a/examples/fabric-bridge-app/linux/CommissionerControl.cpp +++ b/examples/fabric-bridge-app/linux/CommissionerControl.cpp @@ -52,10 +52,10 @@ void CommissionerControlDelegate::ResetDelegateState() mNextStep = Step::kAcceptCommissioningApproval; // Reset identifiers and product information - mRequestId = 0; + mRequestId = 0; mClientNodeId = kUndefinedNodeId; - mVendorId = VendorId::Unspecified; - mProductId = 0; + mVendorId = VendorId::Unspecified; + mProductId = 0; // Clear the label buffer and optional label memset(mLabelBuffer, 0, sizeof(mLabelBuffer)); From cf4962374e3ce2428be42765c56b67e103db9c45 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Sat, 24 Aug 2024 23:46:54 -0700 Subject: [PATCH 4/4] Do not reset state when commissionNode fail to allow retry --- .../linux/CommissionerControl.cpp | 36 +++++++++++++------ .../linux/include/CommissionerControl.h | 7 ++-- .../commissioner-control-server.cpp | 1 - .../commissioner-control-server.h | 9 ----- 4 files changed, 29 insertions(+), 24 deletions(-) diff --git a/examples/fabric-bridge-app/linux/CommissionerControl.cpp b/examples/fabric-bridge-app/linux/CommissionerControl.cpp index 0f2cd01adf630d..0466ab8b711b69 100644 --- a/examples/fabric-bridge-app/linux/CommissionerControl.cpp +++ b/examples/fabric-bridge-app/linux/CommissionerControl.cpp @@ -49,7 +49,7 @@ namespace CommissionerControl { void CommissionerControlDelegate::ResetDelegateState() { // Reset the step to the initial state - mNextStep = Step::kAcceptCommissioningApproval; + mNextStep = Step::kIdle; // Reset identifiers and product information mRequestId = 0; @@ -71,7 +71,7 @@ void CommissionerControlDelegate::ResetDelegateState() CHIP_ERROR CommissionerControlDelegate::HandleCommissioningApprovalRequest(const CommissioningApprovalRequest & request) { - VerifyOrReturnError(mNextStep == Step::kAcceptCommissioningApproval, CHIP_ERROR_BUSY); + VerifyOrReturnError(mNextStep == Step::kIdle, CHIP_ERROR_INCORRECT_STATE); CommissionerControl::Events::CommissioningRequestResult::Type result; result.requestId = request.requestId; @@ -113,7 +113,14 @@ CHIP_ERROR CommissionerControlDelegate::HandleCommissioningApprovalRequest(const CHIP_ERROR err = CommissionerControlServer::Instance().GenerateCommissioningRequestResultEvent(result); - mNextStep = (err == CHIP_NO_ERROR) ? Step::kWaitCommissionNodeRequest : Step::kAcceptCommissioningApproval; + if (err == CHIP_NO_ERROR) + { + mNextStep = Step::kWaitCommissionNodeRequest; + } + else + { + ResetDelegateState(); + } return err; } @@ -162,22 +169,29 @@ CHIP_ERROR CommissionerControlDelegate::GetCommissioningWindowParams(Commissioni CHIP_ERROR CommissionerControlDelegate::HandleCommissionNode(const CommissioningWindowParams & params, const Optional & ipAddress, const Optional & port) { + CHIP_ERROR err = CHIP_NO_ERROR; + ChipLogProgress(NotSpecified, "CommissionerControlDelegate::HandleCommissionNode"); VerifyOrReturnError(mNextStep == Step::kStartCommissionNode, CHIP_ERROR_INCORRECT_STATE); #if defined(PW_RPC_FABRIC_BRIDGE_SERVICE) && PW_RPC_FABRIC_BRIDGE_SERVICE - return CommissionNode(Controller::CommissioningWindowPasscodeParams() - .SetSetupPIN(kSetupPinCode) - .SetTimeout(params.commissioningTimeout) - .SetDiscriminator(params.discriminator) - .SetIteration(params.iterations) - .SetSalt(params.salt), - mVendorId, mProductId); + err = CommissionNode(Controller::CommissioningWindowPasscodeParams() + .SetSetupPIN(kSetupPinCode) + .SetTimeout(params.commissioningTimeout) + .SetDiscriminator(params.discriminator) + .SetIteration(params.iterations) + .SetSalt(params.salt), + mVendorId, mProductId); #else ChipLogProgress(NotSpecified, "Failed to reverse commission bridge: PW_RPC_FABRIC_BRIDGE_SERVICE not defined"); - return CHIP_ERROR_NOT_IMPLEMENTED; + err = CHIP_ERROR_NOT_IMPLEMENTED; #endif // defined(PW_RPC_FABRIC_BRIDGE_SERVICE) && PW_RPC_FABRIC_BRIDGE_SERVICE + + // Reset the delegate's state to prepare for a new commissioning sequence. + ResetDelegateState(); + + return err; } } // namespace CommissionerControl diff --git a/examples/fabric-bridge-app/linux/include/CommissionerControl.h b/examples/fabric-bridge-app/linux/include/CommissionerControl.h index 5719741f30dd6e..486668ffa212f7 100644 --- a/examples/fabric-bridge-app/linux/include/CommissionerControl.h +++ b/examples/fabric-bridge-app/linux/include/CommissionerControl.h @@ -29,7 +29,6 @@ namespace CommissionerControl { class CommissionerControlDelegate : public Delegate { public: - void ResetDelegateState() override; CHIP_ERROR HandleCommissioningApprovalRequest(const CommissioningApprovalRequest & request) override; CHIP_ERROR ValidateCommissionNodeCommand(NodeId clientNodeId, uint64_t requestId) override; CHIP_ERROR GetCommissioningWindowParams(CommissioningWindowParams & outParams) override; @@ -42,16 +41,18 @@ class CommissionerControlDelegate : public Delegate enum class Step : uint8_t { // Ready to start reverse commissioning. - kAcceptCommissioningApproval, + kIdle, // Wait for the commission node command. kWaitCommissionNodeRequest, // Need to commission node. kStartCommissionNode, }; + void ResetDelegateState(); + static constexpr size_t kLabelBufferSize = 64; - Step mNextStep = Step::kAcceptCommissioningApproval; + Step mNextStep = Step::kIdle; uint64_t mRequestId = 0; NodeId mClientNodeId = kUndefinedNodeId; VendorId mVendorId = VendorId::Unspecified; diff --git a/src/app/clusters/commissioner-control-server/commissioner-control-server.cpp b/src/app/clusters/commissioner-control-server/commissioner-control-server.cpp index 9e40df15841a79..4df397326894ed 100644 --- a/src/app/clusters/commissioner-control-server/commissioner-control-server.cpp +++ b/src/app/clusters/commissioner-control-server/commissioner-control-server.cpp @@ -265,7 +265,6 @@ bool emberAfCommissionerControlClusterCommissionNodeCallback( { ChipLogError(Zcl, "emberAfCommissionerControlClusterCommissionNodeCallback error: %" CHIP_ERROR_FORMAT, err.Format()); commandObj->AddStatus(commandPath, StatusIB(err).mStatus); - delegate->ResetDelegateState(); } return true; diff --git a/src/app/clusters/commissioner-control-server/commissioner-control-server.h b/src/app/clusters/commissioner-control-server/commissioner-control-server.h index 0ee42568bdc507..60a4e81a93c3af 100644 --- a/src/app/clusters/commissioner-control-server/commissioner-control-server.h +++ b/src/app/clusters/commissioner-control-server/commissioner-control-server.h @@ -137,15 +137,6 @@ class Delegate virtual CHIP_ERROR HandleCommissionNode(const CommissioningWindowParams & params, const Optional & ipAddress, const Optional & port) = 0; - /** - * @brief Reset the state of the delegate. - * - * This method is used to reset the internal state of the delegate to its initial state. - * It should be called whenever the delegate needs to be reinitialized or cleaned up - * before handling a new commissioning sequence. - */ - virtual void ResetDelegateState() = 0; - virtual ~Delegate() = default; };