From 5d735ec90981973352f2fb805c1b807159b4c162 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Fri, 7 Jun 2024 14:38:06 -0700 Subject: [PATCH] Address review comments --- src/controller/CommissioningWindowOpener.cpp | 72 ++++++++++---------- src/controller/CommissioningWindowOpener.h | 56 +++++++-------- 2 files changed, 60 insertions(+), 68 deletions(-) diff --git a/src/controller/CommissioningWindowOpener.cpp b/src/controller/CommissioningWindowOpener.cpp index dd5da38bcfa4ee..dbd305ffe61918 100644 --- a/src/controller/CommissioningWindowOpener.cpp +++ b/src/controller/CommissioningWindowOpener.cpp @@ -47,7 +47,7 @@ CHIP_ERROR CommissioningWindowOpener::OpenBasicCommissioningWindow(NodeId device mBasicCommissioningWindowCallback = callback; mCommissioningWindowCallback = nullptr; mNodeId = deviceId; - mEndpointId = kRootEndpointId; + mTargetEndpointId = kRootEndpointId; mCommissioningWindowTimeout = timeout; mNextStep = Step::kOpenCommissioningWindow; @@ -60,58 +60,58 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(NodeId deviceId, S Callback::Callback * callback, SetupPayload & payload, bool readVIDPIDAttributes) { - return OpenCommissioningWindowImpl(deviceId, kRootEndpointId, timeout, iteration, discriminator, setupPIN, salt, callback, - payload, readVIDPIDAttributes); + CommissioningWindowParams params = { + .deviceId = deviceId, + .endpointId = kRootEndpointId, + .timeout = timeout, + .iteration = iteration, + .discriminator = discriminator, + .setupPIN = setupPIN, + .salt = salt, + .callback = callback, + }; + + return OpenCommissioningWindowImpl(params, payload, readVIDPIDAttributes); } -CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(NodeId deviceId, EndpointId endpointId, Seconds16 timeout, - uint32_t iteration, uint16_t discriminator, - Optional setupPIN, Optional salt, - Callback::Callback * callback, - SetupPayload & payload) +CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(const CommissioningWindowParams & params, SetupPayload & payload) { - - VerifyOrReturnError(endpointId != kRootEndpointId, CHIP_ERROR_INVALID_ARGUMENT); - return OpenCommissioningWindowImpl(deviceId, endpointId, timeout, iteration, discriminator, setupPIN, salt, callback, payload, - false); + return OpenCommissioningWindowImpl(params, payload, false); } -CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowImpl(NodeId deviceId, EndpointId endpointId, Seconds16 timeout, - uint32_t iteration, uint16_t discriminator, - Optional setupPIN, Optional salt, - Callback::Callback * callback, - SetupPayload & payload, bool readVIDPIDAttributes) +CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowImpl(const CommissioningWindowParams & params, SetupPayload & payload, + bool readVIDPIDAttributes) { VerifyOrReturnError(mNextStep == Step::kAcceptCommissioningStart, CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(kSpake2p_Min_PBKDF_Iterations <= iteration && iteration <= kSpake2p_Max_PBKDF_Iterations, + VerifyOrReturnError(kSpake2p_Min_PBKDF_Iterations <= params.iteration && params.iteration <= kSpake2p_Max_PBKDF_Iterations, + CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(!params.salt.HasValue() || + (params.salt.Value().size() >= kSpake2p_Min_PBKDF_Salt_Length && + params.salt.Value().size() <= kSpake2p_Max_PBKDF_Salt_Length), CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError( - !salt.HasValue() || - (salt.Value().size() >= kSpake2p_Min_PBKDF_Salt_Length && salt.Value().size() <= kSpake2p_Max_PBKDF_Salt_Length), - CHIP_ERROR_INVALID_ARGUMENT); mSetupPayload = SetupPayload(); - if (setupPIN.HasValue()) + if (params.setupPIN.HasValue()) { - if (!SetupPayload::IsValidSetupPIN(setupPIN.Value())) + if (!SetupPayload::IsValidSetupPIN(params.setupPIN.Value())) { return CHIP_ERROR_INVALID_ARGUMENT; } mCommissioningWindowOption = CommissioningWindowOption::kTokenWithProvidedPIN; - mSetupPayload.setUpPINCode = setupPIN.Value(); + mSetupPayload.setUpPINCode = params.setupPIN.Value(); } else { mCommissioningWindowOption = CommissioningWindowOption::kTokenWithRandomPIN; } - if (salt.HasValue()) + if (params.salt.HasValue()) { - memcpy(mPBKDFSaltBuffer, salt.Value().data(), salt.Value().size()); - mPBKDFSalt = ByteSpan(mPBKDFSaltBuffer, salt.Value().size()); + memcpy(mPBKDFSaltBuffer, params.salt.Value().data(), params.salt.Value().size()); + mPBKDFSalt = ByteSpan(mPBKDFSaltBuffer, params.salt.Value().size()); } else { @@ -120,17 +120,17 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowImpl(NodeId deviceI } mSetupPayload.version = 0; - mSetupPayload.discriminator.SetLongValue(discriminator); + mSetupPayload.discriminator.SetLongValue(params.discriminator); mSetupPayload.rendezvousInformation.SetValue(RendezvousInformationFlag::kOnNetwork); - mCommissioningWindowCallback = callback; + mCommissioningWindowCallback = params.callback; mBasicCommissioningWindowCallback = nullptr; - mNodeId = deviceId; - mEndpointId = endpointId; - mCommissioningWindowTimeout = timeout; - mPBKDFIterations = iteration; + mNodeId = params.deviceId; + mTargetEndpointId = params.endpointId; + mCommissioningWindowTimeout = params.timeout; + mPBKDFIterations = params.iteration; - bool randomSetupPIN = !setupPIN.HasValue(); + bool randomSetupPIN = !params.setupPIN.HasValue(); ReturnErrorOnFailure( PASESession::GeneratePASEVerifier(mVerifier, mPBKDFIterations, mPBKDFSalt, randomSetupPIN, mSetupPayload.setUpPINCode)); @@ -153,7 +153,7 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowInternal(Messaging: { ChipLogProgress(Controller, "OpenCommissioningWindow for device ID 0x" ChipLogFormatX64, ChipLogValueX64(mNodeId)); - ClusterBase cluster(exchangeMgr, sessionHandle, mEndpointId); + ClusterBase cluster(exchangeMgr, sessionHandle, mTargetEndpointId); if (mCommissioningWindowOption != CommissioningWindowOption::kOriginalSetupCode) { diff --git a/src/controller/CommissioningWindowOpener.h b/src/controller/CommissioningWindowOpener.h index 16f84193925b1a..a6f32332d8a22f 100644 --- a/src/controller/CommissioningWindowOpener.h +++ b/src/controller/CommissioningWindowOpener.h @@ -42,6 +42,21 @@ typedef void (*OnOpenBasicCommissioningWindow)(void * context, NodeId deviceId, class CommissioningWindowOpener { public: + struct CommissioningWindowParams + { + NodeId deviceId; // The device Id of the node. + EndpointId endpointId; // The endpoint Id of the node. + System::Clock::Seconds16 timeout; // The duration for which the commissioning window should remain open. + uint32_t iteration; // The PAKE iteration count associated with the PAKE Passcode ID and ephemeral PAKE passcode + // verifier to be used for this commissioning. + uint16_t discriminator; // The long discriminator for the DNS-SD advertisement. + Optional setupPIN; // The setup PIN to use, or NullOptional to use a randomly-generated one. + Optional salt; // The salt to use, or NullOptional to use a randomly-generated one. If provided, must be at least + // kSpake2p_Min_PBKDF_Salt_Length bytes and at most kSpake2p_Max_PBKDF_Salt_Length bytes in length. + Callback::Callback * + callback; // The function to be called on success or failure of opening the commissioning window. + }; + CommissioningWindowOpener(DeviceController * controller) : mController(controller), mDeviceConnected(&OnDeviceConnectedCallback, this), mDeviceConnectionFailure(&OnDeviceConnectionFailureCallback, this) @@ -109,36 +124,15 @@ class CommissioningWindowOpener /** * @brief - * Try to look up the bridged device on the bridge with the given node id - * of the bridge and endpoint id of the bridged device and ask it to re-enter - * commissioning mode with a PASE verifier derived from the given information - * and the given discriminator. The bridged device will exit commissioning mode - * after a successful commissioning, or after the given `timeout` time. + * This method sends an OpenCommissioningWindow command with the given parameters + * to the specified endpoint of the given node. * - * @param[in] deviceId The device Id of the bridge. - * @param[in] endpoinId The endpoint Id of the bridged node on the bridge.* - * @param[in] timeout The commissioning mode should terminate after this much time. - * @param[in] iteration The PAKE iteration count associated with the PAKE Passcode ID and ephemeral - * PAKE passcode verifier to be used for this commissioning. - * @param[in] discriminator The long discriminator for the DNS-SD advertisement. - * @param[in] setupPIN The setup PIN to use, or NullOptional to use a randomly-generated one. - * @param[in] salt The salt to use, or NullOptional to use a - * randomly-generated one. If provided, must be at - * least kSpake2p_Min_PBKDF_Salt_Length bytes and - * at most kSpake2p_Max_PBKDF_Salt_Length bytes in - * length. - * @param[in] callback The function to be called on success or failure of opening of commissioning window. - * @param[out] payload The setup payload, not including the VID/PID bits, - * even if those were asked for, that is generated - * based on the passed-in information. The payload - * provided to the callback function, unlike this - * out parameter, will include the VID/PID bits if - * readVIDPIDAttributes is true. + * @param[in] params The parameters required to open the commissioning window. + * @param[out] payload The generated setup payload. * + * @return CHIP_ERROR Returns a CHIP_ERROR indicating the status of the operation. */ - CHIP_ERROR OpenCommissioningWindow(NodeId deviceId, EndpointId endpointId, System::Clock::Seconds16 timeout, uint32_t iteration, - uint16_t discriminator, Optional setupPIN, Optional salt, - Callback::Callback * callback, SetupPayload & payload); + CHIP_ERROR OpenCommissioningWindow(const CommissioningWindowParams & params, SetupPayload & payload); private: enum class Step : uint8_t @@ -153,10 +147,8 @@ class CommissioningWindowOpener kOpenCommissioningWindow, }; - CHIP_ERROR OpenCommissioningWindowImpl(NodeId deviceId, EndpointId endpointId, System::Clock::Seconds16 timeout, - uint32_t iteration, uint16_t discriminator, Optional setupPIN, - Optional salt, Callback::Callback * callback, - SetupPayload & payload, bool readVIDPIDAttributes); + CHIP_ERROR OpenCommissioningWindowImpl(const CommissioningWindowParams & params, SetupPayload & payload, + bool readVIDPIDAttributes); CHIP_ERROR OpenCommissioningWindowInternal(Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle); static void OnPIDReadResponse(void * context, uint16_t value); static void OnVIDReadResponse(void * context, VendorId value); @@ -174,7 +166,7 @@ class CommissioningWindowOpener Callback::Callback * mBasicCommissioningWindowCallback = nullptr; SetupPayload mSetupPayload; NodeId mNodeId = kUndefinedNodeId; - EndpointId mTargetEndpointId = kInvalidEndpointId; + EndpointId mTargetEndpointId = kInvalidEndpointId; System::Clock::Seconds16 mCommissioningWindowTimeout = System::Clock::kZero; CommissioningWindowOption mCommissioningWindowOption = CommissioningWindowOption::kOriginalSetupCode; Crypto::Spake2pVerifier mVerifier; // Used for non-basic commissioning.