From 3d68cb92b18fac05232cc41c51793779accacbca Mon Sep 17 00:00:00 2001 From: C Freeman Date: Fri, 28 Jan 2022 10:52:04 -0500 Subject: [PATCH] Check feature map for network technology (#13829) * Check feature map for network technology Auto commissioner will check the feature map if we're commissioning using BLE, and will check that the supplied arguments match the feature map. This will let vendors proactively just supply both wifi and thread credentials and the commissioner will do the right thing. * Update src/controller/CHIPDeviceController.cpp Co-authored-by: Boris Zbarsky * Use enum flags from zap * Fix print to use PRI format Co-authored-by: Boris Zbarsky --- src/controller/AutoCommissioner.cpp | 83 +++++++++++++++++++++---- src/controller/AutoCommissioner.h | 5 +- src/controller/CHIPDeviceController.cpp | 23 +++++++ src/controller/CHIPDeviceController.h | 6 ++ src/controller/CommissioningDelegate.h | 9 ++- 5 files changed, 112 insertions(+), 14 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 423c77a005a110..4afa8d032e22f4 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -96,7 +96,7 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam return CHIP_NO_ERROR; } -CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStage currentStage, CHIP_ERROR lastErr) +CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStage currentStage, CHIP_ERROR & lastErr) { if (lastErr != CHIP_NO_ERROR) { @@ -107,6 +107,15 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag case CommissioningStage::kSecurePairing: return CommissioningStage::kArmFailsafe; case CommissioningStage::kArmFailsafe: + if (mNeedsNetworkSetup) + { + return CommissioningStage::kGetNetworkTechnology; + } + else + { + return CommissioningStage::kConfigRegulatory; + } + case CommissioningStage::kGetNetworkTechnology: return CommissioningStage::kConfigRegulatory; case CommissioningStage::kConfigRegulatory: return CommissioningStage::kSendPAICertificateRequest; @@ -128,16 +137,30 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag // TODO(cecille): device attestation casues operational cert provisioinging 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. - if (mParams.GetWiFiCredentials().HasValue()) - { - return CommissioningStage::kWiFiNetworkSetup; - } - else if (mParams.GetThreadOperationalDataset().HasValue()) + if (mNeedsNetworkSetup) { - return CommissioningStage::kThreadNetworkSetup; + if (mParams.GetWiFiCredentials().HasValue() && + mNetworkTechnology.Has( + chip::app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kWiFiNetworkInterface)) + { + return CommissioningStage::kWiFiNetworkSetup; + } + else if (mParams.GetThreadOperationalDataset().HasValue() && + mNetworkTechnology.Has( + chip::app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kThreadNetworkInterface)) + { + return CommissioningStage::kThreadNetworkSetup; + } + else + { + ChipLogError(Controller, "Required network information not provided in commissioning parameters"); + lastErr = CHIP_ERROR_INVALID_ARGUMENT; + return CommissioningStage::kCleanup; + } } else { + // TODO: I dont think this is to spec - not sure where we'd have a commissioner that doesn't have dnssd. #if CHIP_DEVICE_CONFIG_ENABLE_DNSSD return CommissioningStage::kFindOperational; #else @@ -145,7 +168,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag #endif } case CommissioningStage::kWiFiNetworkSetup: - if (mParams.GetThreadOperationalDataset().HasValue()) + if (mParams.GetThreadOperationalDataset().HasValue() && + mNetworkTechnology.Has(chip::app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kThreadNetworkInterface)) { return CommissioningStage::kThreadNetworkSetup; } @@ -154,7 +178,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag return CommissioningStage::kWiFiNetworkEnable; } case CommissioningStage::kThreadNetworkSetup: - if (mParams.GetWiFiCredentials().HasValue()) + if (mParams.GetWiFiCredentials().HasValue() && + mNetworkTechnology.Has(chip::app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kWiFiNetworkInterface)) { return CommissioningStage::kWiFiNetworkEnable; } @@ -164,7 +189,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag } case CommissioningStage::kWiFiNetworkEnable: - if (mParams.GetThreadOperationalDataset().HasValue()) + if (mParams.GetThreadOperationalDataset().HasValue() && + mNetworkTechnology.Has(chip::app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kThreadNetworkInterface)) { return CommissioningStage::kThreadNetworkEnable; } @@ -173,7 +199,12 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag return CommissioningStage::kFindOperational; } case CommissioningStage::kThreadNetworkEnable: + // TODO: I dont think this is to spec - not sure where we'd have a commissioner that doesn't have dnssd. +#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD return CommissioningStage::kFindOperational; +#else + return CommissioningStage::kSendComplete; +#endif case CommissioningStage::kFindOperational: return CommissioningStage::kSendComplete; case CommissioningStage::kSendComplete: @@ -193,9 +224,19 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag void AutoCommissioner::StartCommissioning(CommissioneeDeviceProxy * proxy) { // TODO: check that there is no commissioning in progress currently. + + if (proxy == nullptr || !proxy->GetSecureSession().HasValue()) + { + ChipLogError(Controller, "Device proxy secure session error"); + return; + } mCommissioneeDeviceProxy = proxy; - mCommissioner->PerformCommissioningStep(mCommissioneeDeviceProxy, CommissioningStage::kArmFailsafe, mParams, this, 0, - GetCommandTimeout(CommissioningStage::kArmFailsafe)); + mNeedsNetworkSetup = + mCommissioneeDeviceProxy->GetSecureSession().Value()->AsSecureSession()->GetPeerAddress().GetTransportType() == + Transport::Type::kBle; + CHIP_ERROR err = CHIP_NO_ERROR; + CommissioningStage nextStage = GetNextCommissioningStage(CommissioningStage::kSecurePairing, err); + mCommissioner->PerformCommissioningStep(mCommissioneeDeviceProxy, nextStage, mParams, this, 0, GetCommandTimeout(nextStage)); } Optional AutoCommissioner::GetCommandTimeout(CommissioningStage stage) @@ -254,6 +295,24 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio { switch (report.stageCompleted) { + case CommissioningStage::kGetNetworkTechnology: + mNetworkTechnology.SetRaw(report.Get().features); + // Only one of these features can be set at a time. + if (!mNetworkTechnology.HasOnly( + chip::app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kWiFiNetworkInterface) && + !mNetworkTechnology.HasOnly( + chip::app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kThreadNetworkInterface) && + mNetworkTechnology.HasOnly( + chip::app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kEthernetNetworkInterface)) + { + ChipLogError( + Controller, + "Network Commissioning cluster is malformed - more than one network technology is specified (0x%" PRIX32 ")", + report.Get().features); + err = CHIP_ERROR_INTEGRITY_CHECK_FAILED; + break; + } + break; case CommissioningStage::kSendPAICertificateRequest: SetPAI(report.Get().certificate); break; diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index c2c0f04837252d..d836feb5d444a1 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -39,7 +39,7 @@ class AutoCommissioner : public CommissioningDelegate CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) override; private: - CommissioningStage GetNextCommissioningStage(CommissioningStage currentStage, CHIP_ERROR lastErr); + CommissioningStage GetNextCommissioningStage(CommissioningStage currentStage, CHIP_ERROR & lastErr); void ReleaseDAC(); void ReleasePAI(); @@ -61,6 +61,9 @@ class AutoCommissioner : public CommissioningDelegate uint8_t mSsid[CommissioningParameters::kMaxSsidLen]; uint8_t mCredentials[CommissioningParameters::kMaxCredentialsLen]; uint8_t mThreadOperationalDataset[CommissioningParameters::kMaxThreadDatasetLen]; + // TODO: if the device library adds a network commissioning device type, this will need to be 1 per endpoint. + BitFlags mNetworkTechnology; + bool mNeedsNetworkSetup = false; // TODO: Why were the nonces statically allocated, but the certs dynamically allocated? uint8_t * mDAC = nullptr; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 5ecbf30fcf20e7..b3a28b5ca048df 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1617,6 +1617,20 @@ void DeviceCommissioner::SetupCluster(ClusterBase & base, DeviceProxy * proxy, E base.SetCommandTimeout(timeout); } +void OnFeatureMapSuccess(void * context, uint32_t value) +{ + DeviceCommissioner * commissioner = static_cast(context); + CommissioningDelegate::CommissioningReport report; + report.Set(value); + commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); +} + +void AttributeReadFailure(void * context, CHIP_ERROR status) +{ + DeviceCommissioner * commissioner = static_cast(context); + commissioner->CommissioningStageComplete(status); +} + void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, CommissioningStage step, CommissioningParameters & params, CommissioningDelegate * delegate, EndpointId endpoint, Optional timeout) @@ -1644,6 +1658,15 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio genCom.ArmFailSafe(mSuccess.Cancel(), mFailure.Cancel(), params.GetFailsafeTimerSeconds(), breadcrumb, kCommandTimeoutMs); } break; + case CommissioningStage::kGetNetworkTechnology: { + ChipLogProgress(Controller, "Sending request for network cluster feature map"); + NetworkCommissioningCluster netCom; + // TODO: swap to given endpoint once that PR is merged + netCom.Associate(proxy, 0); + netCom.ReadAttribute(this, OnFeatureMapSuccess, + AttributeReadFailure); + } + break; case CommissioningStage::kConfigRegulatory: { // To set during config phase: // UTC time diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 5730ac4d328717..b89a66ade8b0f8 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -601,6 +601,12 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, const ByteSpan & attestationNonce, const ByteSpan & pai, const ByteSpan & dac, DeviceProxy * proxy); + /** + * @brief + * Sends CommissioningStepComplete report to the commissioning delegate. Function will fill in current step. + * @params[in] err error from the current step + * @params[in] report report to send. Current step will be filled in automatically + */ void CommissioningStageComplete(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report = CommissioningDelegate::CommissioningReport()); diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 12bda4f6fb6ab9..d52eef56977d3d 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -32,6 +32,7 @@ enum CommissioningStage : uint8_t // kConfigTime, // NOT YET IMPLEMENTED // kConfigTimeZone, // NOT YET IMPLEMENTED // kConfigDST, // NOT YET IMPLEMENTED + kGetNetworkTechnology, kConfigRegulatory, kSendPAICertificateRequest, kSendDACCertificateRequest, @@ -232,12 +233,18 @@ struct OperationalNodeFoundData OperationalNodeFoundData(OperationalDeviceProxy * proxy) : operationalProxy(proxy) {} OperationalDeviceProxy * operationalProxy; }; +struct FeatureMap +{ + FeatureMap(uint32_t featureBitmap) : features(featureBitmap) {} + uint32_t features; +}; + class CommissioningDelegate { public: virtual ~CommissioningDelegate(){}; - struct CommissioningReport : Variant + struct CommissioningReport : Variant { CommissioningReport() : stageCompleted(CommissioningStage::kError) {} CommissioningStage stageCompleted;