[ICD] ICD registration in AutoCommissioner (#30260)

* [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]>

Author: 3 people

examples/chip-tool/commands/pairing/PairingCommand.cpp

@@ -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;
 }
 
@@ -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

examples/chip-tool/commands/pairing/PairingCommand.h

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

examples/darwin-framework-tool/templates/tests/ciTests.json

@@ -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"
     ]
 }

scripts/tests/chiptest/__init__.py

@@ -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

src/app/tests/suites/TestIcdManagementCluster.yaml

@@ -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"
@@ -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"

src/controller/AutoCommissioner.cpp

@@ -18,6 +18,8 @@
 
 #include <controller/AutoCommissioner.h>
 
+#include <cstring>
+
 #include <app/InteractionModelTimeout.h>
 #include <controller/CHIPDeviceController.h>
 #include <credentials/CHIPCert.h>
@@ -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
@@ -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));
 
@@ -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;
 }
 
@@ -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.
@@ -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;
@@ -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;

src/controller/AutoCommissioner.h

@@ -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.
@@ -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;
@@ -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

src/controller/CHIPDeviceController.cpp

@@ -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)
@@ -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)
 {
@@ -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());