Skip to content

Commit

Permalink
[ICD] ICD registration in AutoCommissioner (project-chip#30260)
Browse files Browse the repository at this point in the history
* [ICD] ICD registration in AutoCommissioner

* Fix tests

* Update according to comments

* Fix

* restyle

* Fix Ci

* Update

* Regen

* Update

* Fix Test

* Update

* Update

* Apply suggestions from code review

Co-authored-by: mkardous-silabs <[email protected]>

* Fix typo

* Allow setting ICD keys during commissioning async

* Update

* Fix

* Fix

* Bypass darwin-tests for ICD Management Cluster

* Add SendStayActive

* Persist symmetric key

* revert mICDClientStorage

* revert mICDClientStorage

* Address comments

* Alter commissioning stage order

* Update

---------

Co-authored-by: yunhanw-google <[email protected]>
Co-authored-by: mkardous-silabs <[email protected]>
  • Loading branch information
3 people authored Dec 6, 2023
1 parent 94ba5c6 commit fb47c90
Show file tree
Hide file tree
Showing 13 changed files with 306 additions and 1,028 deletions.
46 changes: 46 additions & 0 deletions examples/chip-tool/commands/pairing/PairingCommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,30 @@ CommissioningParameters PairingCommand::GetCommissioningParameters()
params.SetDSTOffsets(mDSTOffsetList);
}

if (!mSkipICDRegistration.ValueOr(false))
{
params.SetICDRegistrationStrategy(ICDRegistrationStrategy::kBeforeComplete);

if (!mICDSymmetricKey.HasValue())
{
chip::Crypto::DRBG_get_bytes(mRandomGeneratedICDSymmetricKey, sizeof(mRandomGeneratedICDSymmetricKey));
mICDSymmetricKey.SetValue(ByteSpan(mRandomGeneratedICDSymmetricKey));
}
if (!mICDCheckInNodeId.HasValue())
{
mICDCheckInNodeId.SetValue(CurrentCommissioner().GetNodeId());
}
if (!mICDMonitoredSubject.HasValue())
{
mICDMonitoredSubject.SetValue(mICDCheckInNodeId.Value());
}
// These Optionals must have values now.
// The commissioner will verify these values.
params.SetICDSymmetricKey(mICDSymmetricKey.Value());
params.SetICDCheckInNodeId(mICDCheckInNodeId.Value());
params.SetICDMonitoredSubject(mICDMonitoredSubject.Value());
}

return params;
}

Expand Down Expand Up @@ -367,6 +391,28 @@ void PairingCommand::OnCommissioningComplete(NodeId nodeId, CHIP_ERROR err)
SetCommandExitStatus(err);
}

void PairingCommand::OnICDRegistrationInfoRequired()
{
// Since we compute our ICD Registration info up front, we can call ICDRegistrationInfoReady() directly.
CurrentCommissioner().ICDRegistrationInfoReady();
}

void PairingCommand::OnICDRegistrationComplete(NodeId nodeId, uint32_t icdCounter)
{
char icdSymmetricKeyHex[chip::Crypto::kAES_CCM128_Key_Length * 2 + 1];

chip::Encoding::BytesToHex(mICDSymmetricKey.Value().data(), mICDSymmetricKey.Value().size(), icdSymmetricKeyHex,
sizeof(icdSymmetricKeyHex), chip::Encoding::HexFlags::kNullTerminate);

// TODO: Persist symmetric key.

ChipLogProgress(chipTool,
"ICD Registration Complete for device " ChipLogFormatX64 " / Check-In NodeID: " ChipLogFormatX64
" / Monitored Subject: " ChipLogFormatX64 " / Symmetric Key: %s",
ChipLogValueX64(nodeId), ChipLogValueX64(mICDCheckInNodeId.Value()),
ChipLogValueX64(mICDMonitoredSubject.Value()), icdSymmetricKeyHex);
}

void PairingCommand::OnDiscoveredDevice(const chip::Dnssd::DiscoveredNodeData & nodeData)
{
// Ignore nodes with closed commissioning window
Expand Down
16 changes: 16 additions & 0 deletions examples/chip-tool/commands/pairing/PairingCommand.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ class PairingCommand : public CHIPCommand,
"Bypass the attestation verifier. If not provided or false, the attestation verifier is not bypassed."
" If true, the commissioning will continue in case of attestation verification failure.");
AddArgument("case-auth-tags", 1, UINT32_MAX, &mCASEAuthTags, "The CATs to be encoded in the NOC sent to the commissionee");
AddArgument("skip-icd-registration", 0, 1, &mSkipICDRegistration,
"Skip registering for check-ins from ICDs during commissioning. Default: false");
AddArgument("icd-check-in-nodeid", 0, UINT64_MAX, &mICDCheckInNodeId,
"The check-in node id for the ICD, default: node id of the commissioner.");
AddArgument("icd-monitored-subject", 0, UINT64_MAX, &mICDMonitoredSubject,
"The monitored subject of the ICD, default: The node id used for icd-check-in-nodeid.");
AddArgument("icd-symmetric-key", &mICDSymmetricKey, "The 16 bytes ICD symmetric key, default: randomly generated.");

switch (networkType)
{
Expand Down Expand Up @@ -188,6 +195,8 @@ class PairingCommand : public CHIPCommand,
void OnPairingComplete(CHIP_ERROR error) override;
void OnPairingDeleted(CHIP_ERROR error) override;
void OnCommissioningComplete(NodeId deviceId, CHIP_ERROR error) override;
void OnICDRegistrationInfoRequired() override;
void OnICDRegistrationComplete(NodeId deviceId, uint32_t icdCounter) override;

/////////// DeviceDiscoveryDelegate Interface /////////
void OnDiscoveredDevice(const chip::Dnssd::DiscoveredNodeData & nodeData) override;
Expand Down Expand Up @@ -222,12 +231,17 @@ class PairingCommand : public CHIPCommand,
chip::Optional<bool> mBypassAttestationVerifier;
chip::Optional<std::vector<uint32_t>> mCASEAuthTags;
chip::Optional<char *> mCountryCode;
chip::Optional<bool> mSkipICDRegistration;
chip::Optional<NodeId> mICDCheckInNodeId;
chip::Optional<chip::ByteSpan> mICDSymmetricKey;
chip::Optional<uint64_t> mICDMonitoredSubject;
chip::app::DataModel::List<chip::app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type> mTimeZoneList;
TypedComplexArgument<chip::app::DataModel::List<chip::app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type>>
mComplex_TimeZones;
chip::app::DataModel::List<chip::app::Clusters::TimeSynchronization::Structs::DSTOffsetStruct::Type> mDSTOffsetList;
TypedComplexArgument<chip::app::DataModel::List<chip::app::Clusters::TimeSynchronization::Structs::DSTOffsetStruct::Type>>
mComplex_DSTOffsets;

uint16_t mRemotePort;
uint16_t mDiscriminator;
uint32_t mSetupPINCode;
Expand All @@ -239,6 +253,8 @@ class PairingCommand : public CHIPCommand,
uint64_t mDiscoveryFilterCode;
char * mDiscoveryFilterInstanceName;

uint8_t mRandomGeneratedICDSymmetricKey[chip::Crypto::kAES_CCM128_Key_Length];

// For unpair
chip::Platform::UniquePtr<chip::Controller::CurrentFabricRemover> mCurrentFabricRemover;
chip::Callback::Callback<chip::Controller::OnCurrentFabricRemove> mCurrentFabricRemoveCallback;
Expand Down
4 changes: 3 additions & 1 deletion examples/darwin-framework-tool/templates/tests/ciTests.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@
"Disabled because darwin-framework-tool does not handle substraction in parameters",
"Test_TC_S_2_3",
"TestScenesMultiFabric",
"TestScenesFabricSceneInfo"
"TestScenesFabricSceneInfo",
"#30759: Darwin chip-tool does not support ICD registration during commissioning",
"TestIcdManagementCluster"
]
}
1 change: 1 addition & 0 deletions scripts/tests/chiptest/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ def _GetChipReplUnsupportedTests() -> Set[str]:
"Test_TC_ACE_1_6.yaml", # Test fails only in chip-repl: Refer--> https://github.com/project-chip/connectedhomeip/pull/27910#issuecomment-1632485584
"Test_TC_IDM_1_2.yaml", # chip-repl does not support AnyCommands (19/07/2023)
"TestGroupKeyManagementCluster.yaml", # chip-repl does not support EqualityCommands (2023-08-04)
"TestIcdManagementCluster.yaml", # TODO(#30430): add ICD registration support in chip-repl
"Test_TC_S_2_2.yaml", # chip-repl does not support EqualityCommands pseudo-cluster
"Test_TC_MOD_3_1.yaml", # chip-repl does not support EqualityCommands pseudo-cluster
"Test_TC_MOD_3_2.yaml", # chip-repl does not support EqualityCommands pseudo-cluster
Expand Down
26 changes: 24 additions & 2 deletions src/app/tests/suites/TestIcdManagementCluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ config:
endpoint: 0

tests:
- label: "Read the commissioner node ID"
cluster: "CommissionerCommands"
command: "GetCommissionerNodeId"
response:
values:
- name: "nodeId"
saveAs: commissionerNodeId

- label: "Wait for the commissioned device to be retrieved"
cluster: "DelayCommands"
command: "WaitForCommissionee"
Expand Down Expand Up @@ -94,11 +102,25 @@ tests:
response:
error: NOT_FOUND

- label: "Read RegisteredClients"
# chip-tool will register itself with the ICD during the tests.
- label: "Read RegisteredClients For Registration During Commissioning"
command: "readAttribute"
attribute: "RegisteredClients"
response:
value: []
value:
[
{
CheckInNodeID: commissionerNodeId,
MonitoredSubject: commissionerNodeId,
},
]

- label: "Unregister Client Registered During Commissioning"
command: "UnregisterClient"
arguments:
values:
- name: "CheckInNodeID"
value: commissionerNodeId

- label: "Register 1.0 (key too short)"
command: "RegisterClient"
Expand Down
66 changes: 63 additions & 3 deletions src/controller/AutoCommissioner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

#include <controller/AutoCommissioner.h>

#include <cstring>

#include <app/InteractionModelTimeout.h>
#include <controller/CHIPDeviceController.h>
#include <credentials/CHIPCert.h>
Expand Down Expand Up @@ -65,6 +67,32 @@ static bool IsUnsafeSpan(const Optional<SpanType> & maybeUnsafeSpan, const Optio
return maybeUnsafeSpan.Value().data() != knownSafeSpan.Value().data();
}

CHIP_ERROR AutoCommissioner::VerifyICDRegistrationInfo(const CommissioningParameters & params)
{
ChipLogProgress(Controller, "Checking ICD registration parameters");
if (!params.GetICDSymmetricKey().HasValue())
{
ChipLogError(Controller, "Missing ICD symmetric key!");
return CHIP_ERROR_INVALID_ARGUMENT;
}
if (params.GetICDSymmetricKey().Value().size() != sizeof(mICDSymmetricKey))
{
ChipLogError(Controller, "Invalid ICD symmetric key length!");
return CHIP_ERROR_INVALID_ARGUMENT;
}
if (!params.GetICDCheckInNodeId().HasValue())
{
ChipLogError(Controller, "Missing ICD check-in node id!");
return CHIP_ERROR_INVALID_ARGUMENT;
}
if (!params.GetICDMonitoredSubject().HasValue())
{
ChipLogError(Controller, "Missing ICD monitored subject!");
return CHIP_ERROR_INVALID_ARGUMENT;
}
return CHIP_NO_ERROR;
}

CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParameters & params)
{
// Make sure any members that point to buffers that we are not pointing to
Expand All @@ -90,6 +118,7 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
IsUnsafeSpan(params.GetPAI(), mParams.GetPAI()) || IsUnsafeSpan(params.GetDAC(), mParams.GetDAC()) ||
IsUnsafeSpan(params.GetTimeZone(), mParams.GetTimeZone()) ||
IsUnsafeSpan(params.GetDSTOffsets(), mParams.GetDSTOffsets()) ||
IsUnsafeSpan(params.GetICDSymmetricKey(), mParams.GetICDSymmetricKey()) ||
(params.GetDefaultNTP().HasValue() && !params.GetDefaultNTP().Value().IsNull() &&
params.GetDefaultNTP().Value().Value().data() != mDefaultNtp));

Expand Down Expand Up @@ -229,6 +258,17 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
}
}

if (params.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore && params.GetICDSymmetricKey().HasValue())
{
ReturnErrorOnFailure(VerifyICDRegistrationInfo(params));

// The values must be valid now.
memcpy(mICDSymmetricKey, params.GetICDSymmetricKey().Value().data(), params.GetICDSymmetricKey().Value().size());
mParams.SetICDSymmetricKey(ByteSpan(mICDSymmetricKey));
mParams.SetICDCheckInNodeId(params.GetICDCheckInNodeId().Value());
mParams.SetICDMonitoredSubject(params.GetICDMonitoredSubject().Value());
}

return CHIP_NO_ERROR;
}

Expand Down Expand Up @@ -367,6 +407,17 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
return GetNextCommissioningStageInternal(CommissioningStage::kConfigureTrustedTimeSource, lastErr);
}
case CommissioningStage::kConfigureTrustedTimeSource:
if (mNeedIcdRegistration)
{
return CommissioningStage::kICDGetRegistrationInfo;
}
return GetNextCommissioningStageInternal(CommissioningStage::kICDSendStayActive, lastErr);
case CommissioningStage::kICDGetRegistrationInfo:
return CommissioningStage::kICDRegistration;
case CommissioningStage::kICDRegistration:
// TODO(#24259): StayActiveRequest is not supported by server. We may want to SendStayActive after OpDiscovery.
return CommissioningStage::kICDSendStayActive;
case CommissioningStage::kICDSendStayActive:
// TODO(cecille): device attestation casues operational cert provisioning to happen, This should be a separate stage.
// For thread and wifi, this should go to network setup then enable. For on-network we can skip right to finding the
// operational network because the provisioning of certificates will trigger the device to start operational advertising.
Expand Down Expand Up @@ -709,10 +760,10 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio

if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore)
{
if (commissioningInfo.isIcd)
if (commissioningInfo.isIcd && commissioningInfo.checkInProtocolSupport)
{
mNeedIcdRegistraion = true;
ChipLogDetail(Controller, "AutoCommissioner: Device is ICD");
mNeedIcdRegistration = true;
ChipLogDetail(Controller, "AutoCommissioner: ICD supports the check-in protocol.");
}
}
break;
Expand Down Expand Up @@ -776,6 +827,15 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
// storing the returned certs, so just return here without triggering the next stage.
return NOCChainGenerated(report.Get<NocChain>().noc, report.Get<NocChain>().icac, report.Get<NocChain>().rcac,
report.Get<NocChain>().ipk, report.Get<NocChain>().adminSubject);
case CommissioningStage::kICDGetRegistrationInfo:
// Noting to od. The ICD registation info is handled elsewhere.
break;
case CommissioningStage::kICDRegistration:
// Noting to od. DevicePairingDelegate will handle this.
break;
case CommissioningStage::kICDSendStayActive:
// Nothing to do.
break;
case CommissioningStage::kFindOperational:
mOperationalDeviceProxy = report.Get<OperationalNodeFoundData>().operationalProxy;
break;
Expand Down
6 changes: 5 additions & 1 deletion src/controller/AutoCommissioner.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ class AutoCommissioner : public CommissioningDelegate
EndpointId GetEndpoint(const CommissioningStage & stage) const;
CommissioningStage GetNextCommissioningStageInternal(CommissioningStage currentStage, CHIP_ERROR & lastErr);

CHIP_ERROR VerifyICDRegistrationInfo(const CommissioningParameters & params);

// Helper function to determine whether next stage should be kWiFiNetworkSetup,
// kThreadNetworkSetup or kCleanup, depending whether network information has
// been provided that matches the thread/wifi endpoint of the target.
Expand Down Expand Up @@ -120,7 +122,7 @@ class AutoCommissioner : public CommissioningDelegate
ReadCommissioningInfo mDeviceCommissioningInfo;
bool mNeedsDST = false;

bool mNeedIcdRegistraion = false;
bool mNeedIcdRegistration = false;

// TODO: Why were the nonces statically allocated, but the certs dynamically allocated?
uint8_t * mDAC = nullptr;
Expand All @@ -136,6 +138,8 @@ class AutoCommissioner : public CommissioningDelegate
uint8_t mAttestationElements[Credentials::kMaxRspLen];
uint16_t mAttestationSignatureLen = 0;
uint8_t mAttestationSignature[Crypto::kMax_ECDSA_Signature_Length];

uint8_t mICDSymmetricKey[Crypto::kAES_CCM128_Key_Length];
};
} // namespace Controller
} // namespace chip
64 changes: 64 additions & 0 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1173,6 +1173,25 @@ void DeviceCommissioner::OnFailedToExtendedArmFailSafeDeviceAttestation(void * c
commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
}

void DeviceCommissioner::OnICDManagementRegisterClientResponse(
void * context, const app::Clusters::IcdManagement::Commands::RegisterClientResponse::DecodableType & data)
{
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);

CommissioningDelegate::CommissioningReport report;
auto pairingDelegate = commissioner->GetPairingDelegate();
auto deviceBeingCommissioned = commissioner->mDeviceBeingCommissioned;
if (pairingDelegate != nullptr && deviceBeingCommissioned != nullptr)
{
pairingDelegate->OnICDRegistrationComplete(deviceBeingCommissioned->GetDeviceId(), data.ICDCounter);
commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report);
}
else
{
commissioner->CommissioningStageComplete(CHIP_ERROR_INCORRECT_STATE, report);
}
}

bool DeviceCommissioner::ExtendArmFailSafe(DeviceProxy * proxy, CommissioningStage step, uint16_t armFailSafeTimeout,
Optional<System::Clock::Timeout> commandTimeout, OnExtendFailsafeSuccess onSuccess,
OnExtendFailsafeFailure onFailure)
Expand Down Expand Up @@ -2362,6 +2381,16 @@ CHIP_ERROR DeviceCommissioner::NetworkCredentialsReady()
return CHIP_NO_ERROR;
}

CHIP_ERROR DeviceCommissioner::ICDRegistrationInfoReady()
{
ReturnErrorCodeIf(mCommissioningStage != CommissioningStage::kICDGetRegistrationInfo, CHIP_ERROR_INCORRECT_STATE);

// need to advance to next step
CommissioningStageComplete(CHIP_NO_ERROR);

return CHIP_NO_ERROR;
}

void DeviceCommissioner::OnNetworkConfigResponse(void * context,
const NetworkCommissioning::Commands::NetworkConfigResponse::DecodableType & data)
{
Expand Down Expand Up @@ -3007,6 +3036,41 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
}
}
break;
case CommissioningStage::kICDGetRegistrationInfo: {
GetPairingDelegate()->OnICDRegistrationInfoRequired();
return;
}
break;
case CommissioningStage::kICDRegistration: {
IcdManagement::Commands::RegisterClient::Type request;

if (!(params.GetICDCheckInNodeId().HasValue() && params.GetICDMonitoredSubject().HasValue() &&
params.GetICDSymmetricKey().HasValue()))
{
ChipLogError(Controller, "No ICD Registration information provided!");
CommissioningStageComplete(CHIP_ERROR_INCORRECT_STATE);
return;
}

request.checkInNodeID = params.GetICDCheckInNodeId().Value();
request.monitoredSubject = params.GetICDMonitoredSubject().Value();
request.key = params.GetICDSymmetricKey().Value();

CHIP_ERROR err = SendCommand(proxy, request, OnICDManagementRegisterClientResponse, OnBasicFailure, endpoint, timeout);
if (err != CHIP_NO_ERROR)
{
// We won't get any async callbacks here, so just complete our stage.
ChipLogError(Controller, "Failed to send IcdManagement.RegisterClient command: %" CHIP_ERROR_FORMAT, err.Format());
CommissioningStageComplete(err);
return;
}
}
break;
case CommissioningStage::kICDSendStayActive: {
// TODO(#24259): Send StayActiveRequest once server supports this.
CommissioningStageComplete(CHIP_NO_ERROR);
}
break;
case CommissioningStage::kFindOperational: {
// If there is an error, CommissioningStageComplete will be called from OnDeviceConnectionFailureFn.
auto scopedPeerId = GetPeerScopedId(proxy->GetDeviceId());
Expand Down
Loading

0 comments on commit fb47c90

Please sign in to comment.