Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows opening a commissioning window on a bridged node #33799

Closed
wants to merge 3 commits into from
Closed

Allows opening a commissioning window on a bridged node #33799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35162b7
Allows opening a commissioning window on a bridged node that is not o…
yufengwangca Jun 7, 2024
8b4756e
Update src/controller/CommissioningWindowOpener.h
yufengwangca Jun 7, 2024
5d735ec
Address review comments
yufengwangca Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/app/clusters/administrator-commissioning-server/administrator-commissioning-server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ bool emberAfAdministratorCommissioningClusterOpenCommissioningWindowCallback(
auto & iterations = commandData.iterations;
auto & salt = commandData.salt;

EndpointId endpointId = commandPath.mEndpointId;

Optional<StatusCode> status = Optional<StatusCode>::Missing();
InteractionModel::Status globalStatus = InteractionModel::Status::Success;
Spake2pVerifier verifier;
Expand All @@ -106,6 +108,7 @@ bool emberAfAdministratorCommissioningClusterOpenCommissioningWindowCallback(
VerifyOrExit(failSafeContext.IsFailSafeFullyDisarmed(), status.Emplace(StatusCode::kBusy));

VerifyOrExit(!commissionMgr.IsCommissioningWindowOpen(), status.Emplace(StatusCode::kBusy));
VerifyOrExit(endpointId == kRootEndpointId, status.Emplace(StatusCode::kPAKEParameterError));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the spec basis for this?

As far as I can tell, the spec has no allowances for this sort of behavior. If you get this command, on any endpoint, you must:

  1. Advertise on DNS-SD.
  2. Enter active mode if you're an ICD.

and so on. Nothing says you have to start listening for PASE connections, though, which is arguably a spec bug. But the behavior being added here is not allowed by the spec as things stand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the implementation of the proposed fabric sync feature. For reference, see section [12.6.3.2. Aggregator Node] of the spec (the text came from https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/9106)

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing in that spec section says "Ignore command invokes on the Administrator Commissioning cluster if it's not on endpoint 0".

And nothing in that section overrides (nor can it override) explicit SHALL requirements in the relevant cluster specification.

Basically, to make this work changes are needed to the actual spec for this cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Registered a ticket to clarify the intended behavior when receives OCW command not directed to Endpoint 0
https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/9354

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Until that's resolved, do not make any changes to this cluster impl, please, because those changes would make it current-spec-not-compliant.

VerifyOrExit(iterations >= kSpake2p_Min_PBKDF_Iterations, status.Emplace(StatusCode::kPAKEParameterError));
VerifyOrExit(iterations <= kSpake2p_Max_PBKDF_Iterations, status.Emplace(StatusCode::kPAKEParameterError));
VerifyOrExit(salt.size() >= kSpake2p_Min_PBKDF_Salt_Length, status.Emplace(StatusCode::kPAKEParameterError));
Expand Down
62 changes: 42 additions & 20 deletions src/controller/CommissioningWindowOpener.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ CHIP_ERROR CommissioningWindowOpener::OpenBasicCommissioningWindow(NodeId device
mBasicCommissioningWindowCallback = callback;
mCommissioningWindowCallback = nullptr;
mNodeId = deviceId;
mTargetEndpointId = kRootEndpointId;
mCommissioningWindowTimeout = timeout;

mNextStep = Step::kOpenCommissioningWindow;
Expand All @@ -58,37 +59,59 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(NodeId deviceId, S
Optional<ByteSpan> salt,
Callback::Callback<OnOpenCommissioningWindow> * callback,
SetupPayload & payload, bool 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(const CommissioningWindowParams & params, SetupPayload & payload)
{
return OpenCommissioningWindowImpl(params, payload, false);
}

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
{
Expand All @@ -97,16 +120,17 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(NodeId deviceId, S
}

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;
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));

Expand All @@ -129,9 +153,7 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowInternal(Messaging:
{
ChipLogProgress(Controller, "OpenCommissioningWindow for device ID 0x" ChipLogFormatX64, ChipLogValueX64(mNodeId));

constexpr EndpointId kAdministratorCommissioningClusterEndpoint = 0;

ClusterBase cluster(exchangeMgr, sessionHandle, kAdministratorCommissioningClusterEndpoint);
ClusterBase cluster(exchangeMgr, sessionHandle, mTargetEndpointId);

if (mCommissioningWindowOption != CommissioningWindowOption::kOriginalSetupCode)
{
Expand Down
30 changes: 30 additions & 0 deletions src/controller/CommissioningWindowOpener.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint32_t> setupPIN; // The setup PIN to use, or NullOptional to use a randomly-generated one.
Optional<ByteSpan> 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<OnOpenCommissioningWindow> *
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)
Expand Down Expand Up @@ -107,6 +122,18 @@ class CommissioningWindowOpener
Callback::Callback<OnOpenCommissioningWindow> * callback, SetupPayload & payload,
bool readVIDPIDAttributes = false);

/**
* @brief
* This method sends an OpenCommissioningWindow command with the given parameters
* to the specified endpoint of the given node.
*
* @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(const CommissioningWindowParams & params, SetupPayload & payload);

private:
enum class Step : uint8_t
{
Expand All @@ -120,6 +147,8 @@ class CommissioningWindowOpener
kOpenCommissioningWindow,
};

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);
Expand All @@ -137,6 +166,7 @@ class CommissioningWindowOpener
Callback::Callback<OnOpenBasicCommissioningWindow> * mBasicCommissioningWindowCallback = nullptr;
SetupPayload mSetupPayload;
NodeId mNodeId = kUndefinedNodeId;
EndpointId mTargetEndpointId = kInvalidEndpointId;
System::Clock::Seconds16 mCommissioningWindowTimeout = System::Clock::kZero;
CommissioningWindowOption mCommissioningWindowOption = CommissioningWindowOption::kOriginalSetupCode;
Crypto::Spake2pVerifier mVerifier; // Used for non-basic commissioning.
Expand Down
Loading