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.

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

Commissioning: Implement time sync requirements #27812

Merged
merged 9 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
1 change: 1 addition & 0 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,7 @@ jobs:
scripts/run_in_python_env.sh out/venv './scripts/tests/run_python_test.py --app out/linux-x64-all-clusters-ipv6only-no-ble-no-wifi-tsan-clang-test/chip-all-clusters-app --factoryreset --app-args "--discriminator 1234 --KVS kvs1 --trace_decode 1" --script "src/python_testing/TC_CGEN_2_4.py" --script-args "--storage-path admin_storage.json --commissioning-method on-network --discriminator 1234 --passcode 20202021"'
scripts/run_in_python_env.sh out/venv './scripts/tests/run_python_test.py --app out/linux-x64-all-clusters-ipv6only-no-ble-no-wifi-tsan-clang-test/chip-all-clusters-app --factoryreset --app-args "--discriminator 1234 --KVS kvs1 --trace_decode 1" --script "src/python_testing/TC_DA_1_2.py" --script-args "--storage-path admin_storage.json --commissioning-method on-network --discriminator 1234 --passcode 20202021 --PICS src/app/tests/suites/certification/ci-pics-values"'
scripts/run_in_python_env.sh out/venv './scripts/tests/run_python_test.py --app out/linux-x64-all-clusters-ipv6only-no-ble-no-wifi-tsan-clang-test/chip-all-clusters-app --factoryreset --app-args "--discriminator 1234 --KVS kvs1 --trace_decode 1" --script "src/python_testing/TC_DA_1_5.py" --script-args "--storage-path admin_storage.json --commissioning-method on-network --discriminator 1234 --passcode 20202021 --PICS src/app/tests/suites/certification/ci-pics-values"'
scripts/run_in_python_env.sh out/venv './scripts/tests/run_python_test.py --app out/linux-x64-all-clusters-ipv6only-no-ble-no-wifi-tsan-clang-test/chip-all-clusters-app --factoryreset --app-args "--discriminator 1234 --KVS kvs1 --trace_decode 1" --script "src/python_testing/TestCommissioningTimeSync.py" --script-args "--storage-path admin_storage.json --commissioning-method on-network --discriminator 1234 --passcode 20202021"'
scripts/run_in_python_env.sh out/venv './scripts/tests/run_python_test.py --script "src/python_testing/TestMatterTestingSupport.py"'
- name: Uploading core files
uses: actions/upload-artifact@v3
Expand Down
4 changes: 4 additions & 0 deletions examples/platform/linux/CommissionerMain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ class PairingCommand : public Controller::DevicePairingDelegate
void OnCommissioningStatusUpdate(PeerId peerId, CommissioningStage stageCompleted, CHIP_ERROR error) override;

void OnReadCommissioningInfo(const ReadCommissioningInfo & info) override;
void OnFabricCheck(const MatchingFabricInfo & info) override;

private:
#if CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED
Expand Down Expand Up @@ -344,7 +345,10 @@ void PairingCommand::OnReadCommissioningInfo(const ReadCommissioningInfo & info)
{
ChipLogProgress(AppServer, "OnReadCommissioningInfo - vendorId=0x%04X productId=0x%04X", info.basic.vendorId,
info.basic.productId);
}

void PairingCommand::OnFabricCheck(const MatchingFabricInfo & info)
{
if (info.nodeId != kUndefinedNodeId)
{
ChipLogProgress(AppServer, "ALREADY ON FABRIC WITH nodeId=0x" ChipLogFormatX64, ChipLogValueX64(info.nodeId));
Expand Down
86 changes: 84 additions & 2 deletions src/controller/AutoCommissioner.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

AutoCommissioner::SetCommissioningParameters needs to copy the new buffer things, no? Otherwise consumers who pass in a CommissioningParameters that we are then just grabbing raw pointers from have no way to control lifetime and we get use-after-free.

Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,53 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
// Per the spec, we restart from after adding the NOC.
return GetNextCommissioningStage(CommissioningStage::kSendNOC, lastErr);
}
if (mParams.GetCheckForMatchingFabric())
{
return CommissioningStage::kCheckForMatchingFabric;
}
return CommissioningStage::kArmFailsafe;
case CommissioningStage::kCheckForMatchingFabric:
return CommissioningStage::kArmFailsafe;
case CommissioningStage::kArmFailsafe:
return CommissioningStage::kConfigRegulatory;
case CommissioningStage::kConfigRegulatory:
if (mDeviceCommissioningInfo.requiresUTC)
{
return CommissioningStage::kConfigureUTCTime;
}
else
{
// Time cluster is not supported, move right to DA
return CommissioningStage::kSendPAICertificateRequest;
}
case CommissioningStage::kConfigureUTCTime:
if (mDeviceCommissioningInfo.requiresTimeZone && mParams.GetTimeZone().HasValue())
{
return kConfigureTimeZone;
}
else
{
return GetNextCommissioningStageInternal(CommissioningStage::kConfigureTimeZone, lastErr);
}
case CommissioningStage::kConfigureTimeZone:
if (mNeedsDST && mParams.GetDSTOffsets().HasValue())
{
return CommissioningStage::kConfigureDSTOffset;
}
else
{
return GetNextCommissioningStageInternal(CommissioningStage::kConfigureDSTOffset, lastErr);
}
case CommissioningStage::kConfigureDSTOffset:
if (mDeviceCommissioningInfo.requiresDefaultNTP && mParams.GetDefaultNTP().HasValue())
{
return CommissioningStage::kConfigureDefaultNTP;
}
else
{
return GetNextCommissioningStageInternal(CommissioningStage::kConfigureDefaultNTP, lastErr);
}
case CommissioningStage::kConfigureDefaultNTP:
return CommissioningStage::kSendPAICertificateRequest;
case CommissioningStage::kSendPAICertificateRequest:
return CommissioningStage::kSendDACCertificateRequest;
Expand All @@ -264,6 +307,15 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
case CommissioningStage::kSendTrustedRootCert:
return CommissioningStage::kSendNOC;
case CommissioningStage::kSendNOC:
if (mDeviceCommissioningInfo.requiresTrustedTimeSource && mParams.GetTrustedTimeSource().HasValue())
{
return CommissioningStage::kConfigureTrustedTimeSource;
}
else
{
return GetNextCommissioningStageInternal(CommissioningStage::kConfigureTrustedTimeSource, lastErr);
}
case CommissioningStage::kConfigureTrustedTimeSource:
// 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 @@ -580,11 +632,20 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
.SetRemoteProductId(mDeviceCommissioningInfo.basic.productId)
.SetDefaultRegulatoryLocation(mDeviceCommissioningInfo.general.currentRegulatoryLocation)
.SetLocationCapability(mDeviceCommissioningInfo.general.locationCapability);
if (mDeviceCommissioningInfo.nodeId != kUndefinedNodeId)
// Don't send DST unless the device says it needs it
mNeedsDST = false;
break;
case CommissioningStage::kCheckForMatchingFabric: {
chip::NodeId nodeId = report.Get<MatchingFabricInfo>().nodeId;
if (nodeId != kUndefinedNodeId)
{
mParams.SetRemoteNodeId(mDeviceCommissioningInfo.nodeId);
mParams.SetRemoteNodeId(nodeId);
}
break;
}
case CommissioningStage::kConfigureTimeZone:
mNeedsDST = report.Get<TimeZoneResponseInfo>().requiresDSTOffsets;
break;
case CommissioningStage::kSendPAICertificateRequest:
SetPAI(report.Get<RequestedCertificate>().certificate);
break;
Expand Down Expand Up @@ -650,6 +711,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
mCommissioneeDeviceProxy = nullptr;
mOperationalDeviceProxy = OperationalDeviceProxy();
mDeviceCommissioningInfo = ReadCommissioningInfo();
mNeedsDST = false;
return CHIP_NO_ERROR;
default:
break;
Expand Down Expand Up @@ -692,6 +754,26 @@ CHIP_ERROR AutoCommissioner::PerformStep(CommissioningStage nextStage)
ChipLogError(Controller, "Invalid device for commissioning");
return CHIP_ERROR_INCORRECT_STATE;
}
// Perform any last minute parameter adjustments before calling the commissioner object
switch (nextStage)
{
case CommissioningStage::kConfigureTimeZone:
if (mParams.GetTimeZone().Value().size() > mDeviceCommissioningInfo.maxTimeZoneSize)
{
mParams.SetTimeZone(app::DataModel::List<app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type>(
mParams.GetTimeZone().Value().SubSpan(0, mDeviceCommissioningInfo.maxTimeZoneSize)));
}
break;
case CommissioningStage::kConfigureDSTOffset:
if (mParams.GetDSTOffsets().Value().size() > mDeviceCommissioningInfo.maxDSTSize)
{
mParams.SetDSTOffsets(app::DataModel::List<app::Clusters::TimeSynchronization::Structs::DSTOffsetStruct::Type>(
mParams.GetDSTOffsets().Value().SubSpan(0, mDeviceCommissioningInfo.maxDSTSize)));
}
break;
default:
break;
}

mCommissioner->PerformCommissioningStep(proxy, nextStage, mParams, this, GetEndpoint(nextStage),
GetCommandTimeout(proxy, nextStage));
Expand Down
3 changes: 2 additions & 1 deletion src/controller/AutoCommissioner.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class AutoCommissioner : public CommissioningDelegate
* be used for sending the relevant command.
*/
Optional<System::Clock::Timeout> GetCommandTimeout(DeviceProxy * device, CommissioningStage stage) const;
CommissioningParameters mParams = CommissioningParameters();

private:
DeviceProxy * GetDeviceProxyForStep(CommissioningStage nextStage);
Expand Down Expand Up @@ -94,7 +95,6 @@ class AutoCommissioner : public CommissioningDelegate
DeviceCommissioner * mCommissioner = nullptr;
CommissioneeDeviceProxy * mCommissioneeDeviceProxy = nullptr;
OperationalCredentialsDelegate * mOperationalCredentialsDelegate = nullptr;
CommissioningParameters mParams = CommissioningParameters();
OperationalDeviceProxy mOperationalDeviceProxy;
// Memory space for the commisisoning parameters that come in as ByteSpans - the caller is not guaranteed to retain this memory
uint8_t mSsid[CommissioningParameters::kMaxSsidLen];
Expand All @@ -104,6 +104,7 @@ class AutoCommissioner : public CommissioningDelegate

bool mNeedsNetworkSetup = false;
ReadCommissioningInfo mDeviceCommissioningInfo;
bool mNeedsDST = false;

// TODO: Why were the nonces statically allocated, but the certs dynamically allocated?
uint8_t * mDAC = nullptr;
Expand Down
Loading
Loading