Address testing feedback from opcert callbacks, and re-commissioning (#…

…22369)

* Draft: address testing feedback from new opcert callbacks

* temporary workaround to isolate a memory corruption

* add check for existing fabric during commissioning, handle correctly on content app platform

* address comments

* address comments

* address comments

* Restyled by clang-format (#22524)

Co-authored-by: Restyled.io <[email protected]>

* address comments

Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <[email protected]>

examples/platform/linux/AppMain.cpp

@@ -323,7 +323,8 @@ void ChipLinuxAppMainLoop()
 #if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
     ChipLogProgress(AppServer, "Starting commissioner");
     VerifyOrReturn(InitCommissioner(LinuxDeviceOptions::GetInstance().securedCommissionerPort + 10,
-                                    LinuxDeviceOptions::GetInstance().unsecuredCommissionerPort) == CHIP_NO_ERROR);
+                                    LinuxDeviceOptions::GetInstance().unsecuredCommissionerPort,
+                                    LinuxDeviceOptions::GetInstance().commissionerFabricId) == CHIP_NO_ERROR);
     ChipLogProgress(AppServer, "Started commissioner");
 #if defined(ENABLE_CHIP_SHELL)
     Shell::RegisterControllerCommands();

examples/platform/linux/CommissionerMain.cpp

@@ -30,7 +30,10 @@
 #include <lib/core/NodeId.h>
 #include <lib/support/logging/CHIPLogging.h>
 
+#include <credentials/DeviceAttestationConstructor.h>
+#include <credentials/DeviceAttestationVendorReserved.h>
 #include <credentials/GroupDataProviderImpl.h>
+#include <credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h>
 #include <credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h>
 #include <credentials/attestation_verifier/DeviceAttestationVerifier.h>
 #include <credentials/examples/DeviceAttestationCredsExample.h>
@@ -110,16 +113,17 @@ class MyCommissionerCallback : public CommissionerCallback
     }
 };
 
+AutoCommissioner gAutoCommissioner;
+
 DeviceCommissioner gCommissioner;
 CommissionerDiscoveryController gCommissionerDiscoveryController;
 MyCommissionerCallback gCommissionerCallback;
 MyServerStorageDelegate gServerStorage;
 ExampleOperationalCredentialsIssuer gOpCredsIssuer;
 NodeId gLocalId = kMaxOperationalNodeId;
 Credentials::GroupDataProviderImpl gGroupDataProvider;
-AutoCommissioner gAutoCommissioner;
 
-CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort)
+CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort, FabricId fabricId)
 {
     Controller::FactoryInitParams factoryParams;
     Controller::SetupParams params;
@@ -140,6 +144,10 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort)
     params.controllerVendorId = static_cast<VendorId>(vendorId);
 
     ReturnErrorOnFailure(gOpCredsIssuer.Initialize(gServerStorage));
+    if (fabricId != kUndefinedFabricId)
+    {
+        gOpCredsIssuer.SetFabricIdForNextNOCRequest(fabricId);
+    }
 
     // No need to explicitly set the UDC port since we will use default
     ChipLogProgress(Support, " ----- UDC listening on port %d", udcListenPort);
@@ -175,6 +183,11 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort)
 
     params.defaultCommissioner = &gAutoCommissioner;
 
+    // assign prefered feature settings
+    CommissioningParameters commissioningParams = gAutoCommissioner.GetCommissioningParameters();
+    commissioningParams.SetCheckForMatchingFabric(true);
+    gAutoCommissioner.SetCommissioningParameters(commissioningParams);
+
     auto & factory = Controller::DeviceControllerFactory::GetInstance();
     ReturnErrorOnFailure(factory.Init(factoryParams));
     ReturnErrorOnFailure(factory.SetupCommissioner(params, gCommissioner));
@@ -201,8 +214,9 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort)
     // advertise operational since we are an admin
     app::DnssdServer::Instance().AdvertiseOperational();
 
-    ChipLogProgress(Support, "InitCommissioner nodeId=0x" ChipLogFormatX64 " fabricIndex=0x%x",
-                    ChipLogValueX64(gCommissioner.GetNodeId()), static_cast<unsigned>(fabricIndex));
+    ChipLogProgress(Support,
+                    "InitCommissioner nodeId=0x" ChipLogFormatX64 " fabric.fabricId=0x" ChipLogFormatX64 " fabricIndex=0x%x",
+                    ChipLogValueX64(gCommissioner.GetNodeId()), ChipLogValueX64(fabricId), static_cast<unsigned>(fabricIndex));
 
     return CHIP_NO_ERROR;
 }
@@ -231,6 +245,10 @@ class PairingCommand : public Controller::DevicePairingDelegate
     void OnPairingDeleted(CHIP_ERROR error) override;
     void OnCommissioningComplete(NodeId deviceId, CHIP_ERROR error) override;
 
+    void OnCommissioningStatusUpdate(PeerId peerId, CommissioningStage stageCompleted, CHIP_ERROR error) override;
+
+    void OnReadCommissioningInfo(const ReadCommissioningInfo & info) override;
+
 private:
 #if CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED
     static void OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr,
@@ -292,7 +310,8 @@ void PairingCommand::OnCommissioningComplete(NodeId nodeId, CHIP_ERROR err)
 #if CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED
         ChipLogProgress(AppServer, "Device commissioning completed with success - getting OperationalDeviceProxy");
 
-        gCommissioner.GetConnectedDevice(nodeId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback);
+        gCommissioner.GetConnectedDevice(gAutoCommissioner.GetCommissioningParameters().GetRemoteNodeId().ValueOr(nodeId),
+                                         &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback);
 #else  // CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED
         ChipLogProgress(AppServer, "Device commissioning completed with success");
 #endif // CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED
@@ -310,6 +329,32 @@ void PairingCommand::OnCommissioningComplete(NodeId nodeId, CHIP_ERROR err)
     }
 }
 
+void PairingCommand::OnCommissioningStatusUpdate(PeerId peerId, CommissioningStage stageCompleted, CHIP_ERROR error)
+{
+    ChipLogProgress(AppServer, "OnCommissioningStatusUpdate - stageCompleted='%s' error='%s'", StageToString(stageCompleted),
+                    ErrorStr(error));
+
+    // if we have successfully finished attestation AND this device already has a NodeId on our fabric
+    // then stop commissioning and attempt to connect to it.
+    if (stageCompleted == CommissioningStage::kAttestationVerification && error == CHIP_NO_ERROR &&
+        gAutoCommissioner.GetCommissioningParameters().GetRemoteNodeId().HasValue())
+    {
+        gAutoCommissioner.StopCommissioning();
+    }
+}
+
+void PairingCommand::OnReadCommissioningInfo(const ReadCommissioningInfo & info)
+{
+    ChipLogProgress(AppServer, "OnReadCommissioningInfo - vendorId=0x%04X productId=0x%04X", info.basic.vendorId,
+                    info.basic.productId);
+
+    if (info.nodeId != kUndefinedNodeId)
+    {
+        ChipLogProgress(AppServer, "ALREADY ON FABRIC WITH nodeId=0x" ChipLogFormatX64, ChipLogValueX64(info.nodeId));
+        // wait until attestation verification before cancelling so we can validate vid/pid
+    }
+}
+
 #if CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED
 
 void PairingCommand::OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle)

examples/platform/linux/CommissionerMain.h

@@ -34,7 +34,7 @@ using chip::Transport::PeerAddress;
 CHIP_ERROR CommissionerPairOnNetwork(uint32_t pincode, uint16_t disc, PeerAddress address);
 CHIP_ERROR CommissionerPairUDC(uint32_t pincode, size_t index);
 
-CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort);
+CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort, chip::FabricId fabricId);
 void ShutdownCommissioner();
 
 DeviceCommissioner * GetDeviceCommissioner();

examples/platform/linux/Options.cpp

@@ -72,6 +72,7 @@ enum
     kOptionCSRResponseAttestationSignatureInvalid       = 0x101d,
     kOptionCSRResponseCSRExistingKeyPair                = 0x101e,
     kDeviceOption_TestEventTriggerEnableKey             = 0x101f,
+    kCommissionerOption_FabricID                        = 0x1020,
 };
 
 constexpr unsigned kAppUsageLength = 64;
@@ -117,6 +118,7 @@ OptionDef sDeviceOptionDefs[] = {
     { "cert_error_attestation_signature_incorrect_type", kNoArgument, kOptionCSRResponseAttestationSignatureIncorrectType },
     { "cert_error_attestation_signature_invalid", kNoArgument, kOptionCSRResponseAttestationSignatureInvalid },
     { "enable-key", kArgumentRequired, kDeviceOption_TestEventTriggerEnableKey },
+    { "commissioner-fabric-id", kArgumentRequired, kCommissionerOption_FabricID },
     {}
 };
 
@@ -182,6 +184,9 @@ const char * sDeviceOptionHelp =
     "  --unsecured-commissioner-port <port>\n"
     "       A 16-bit unsigned integer specifying the port to use for unsecured commissioner messages (default is 5550).\n"
     "\n"
+    "  --commissioner-fabric-id <fabricid>\n"
+    "       The fabric ID to be used when this device is a commissioner (default in code is 1).\n"
+    "\n"
     "  --command <command-name>\n"
     "       A name for a command to execute during startup.\n"
     "\n"
@@ -460,6 +465,11 @@ bool HandleOption(const char * aProgram, OptionSet * aOptions, int aIdentifier,
 
         break;
     }
+    case kCommissionerOption_FabricID: {
+        char * eptr;
+        LinuxDeviceOptions::GetInstance().commissionerFabricId = (chip::FabricId) strtoull(aValue, &eptr, 0);
+        break;
+    }
 
     default:
         PrintArgError("%s: INTERNAL ERROR: Unhandled option: %s\n", aProgram, aName);

examples/platform/linux/Options.h

@@ -60,6 +60,7 @@ struct LinuxDeviceOptions
     chip::Credentials::DeviceAttestationCredentialsProvider * dacProvider = nullptr;
     chip::CSRResponseOptions mCSRResponseOptions;
     uint8_t testEventTriggerEnableKey[16] = { 0 };
+    chip::FabricId commissionerFabricId   = chip::kUndefinedFabricId;
 
     static LinuxDeviceOptions & GetInstance();
 };

src/app/app-platform/ContentAppPlatform.cpp

@@ -455,6 +455,61 @@ uint32_t ContentAppPlatform::GetPincodeFromContentApp(uint16_t vendorId, uint16_
     return (uint32_t) strtol(pinString.c_str(), &eptr, 10);
 }
 
+// Returns ACL entry with match subject or CHIP_ERROR_NOT_FOUND if no match is found
+CHIP_ERROR ContentAppPlatform::GetACLEntryIndex(size_t * foundIndex, FabricIndex fabricIndex, NodeId subjectNodeId)
+{
+    size_t index = 0;
+    if (Access::GetAccessControl().GetEntryCount(fabricIndex, index) == CHIP_NO_ERROR)
+    {
+        while (index)
+        {
+            Access::AccessControl::Entry entry;
+            CHIP_ERROR err = Access::GetAccessControl().ReadEntry(fabricIndex, --index, entry);
+            if (err != CHIP_NO_ERROR)
+            {
+                ChipLogDetail(DeviceLayer, "ContentAppPlatform::GetACLEntryIndex error reading entry %d err %s",
+                              static_cast<int>(index), ErrorStr(err));
+            }
+            else
+            {
+                size_t count;
+                err = entry.GetSubjectCount(count);
+                if (err != CHIP_NO_ERROR)
+                {
+                    ChipLogDetail(DeviceLayer,
+                                  "ContentAppPlatform::GetACLEntryIndex error reading subject count for entry %d err %s",
+                                  static_cast<int>(index), ErrorStr(err));
+                    continue;
+                }
+                if (count)
+                {
+                    ChipLogDetail(DeviceLayer, "subjects: %u", static_cast<unsigned>(count));
+                    for (size_t i = 0; i < count; ++i)
+                    {
+                        NodeId subject;
+                        err = entry.GetSubject(i, subject);
+                        if (err != CHIP_NO_ERROR)
+                        {
+                            ChipLogDetail(DeviceLayer,
+                                          "ContentAppPlatform::GetACLEntryIndex error reading subject %i for entry %d err %s",
+                                          static_cast<int>(i), static_cast<int>(index), ErrorStr(err));
+                            continue;
+                        }
+                        if (subject == subjectNodeId)
+                        {
+                            ChipLogDetail(DeviceLayer, "ContentAppPlatform::GetACLEntryIndex found matching subject at index %d",
+                                          static_cast<int>(index));
+                            *foundIndex = index;
+                            return CHIP_NO_ERROR;
+                        }
+                    }
+                }
+            }
+        }
+    }
+    return CHIP_ERROR_NOT_FOUND;
+}
+
 constexpr EndpointId kTargetBindingClusterEndpointId = 0;
 constexpr EndpointId kLocalVideoPlayerEndpointId     = 1;
 constexpr EndpointId kLocalSpeakerEndpointId         = 2;
@@ -472,6 +527,8 @@ constexpr ClusterId kClusterIdAudioOutput     = 0x050b;
 // constexpr ClusterId kClusterIdApplicationLauncher = 0x050c;
 // constexpr ClusterId kClusterIdAccountLogin        = 0x050e;
 
+// Add ACLs on this device for the given client,
+// and create bindings on the given client so that it knows what it has access to.
 CHIP_ERROR ContentAppPlatform::ManageClientAccess(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle,
                                                   uint16_t targetVendorId, NodeId localNodeId,
                                                   Controller::WriteResponseSuccessCallback successCb,
@@ -482,12 +539,30 @@ CHIP_ERROR ContentAppPlatform::ManageClientAccess(Messaging::ExchangeManager & e
 
     Access::Privilege vendorPrivilege = mContentAppFactory->GetVendorPrivilege(targetVendorId);
 
+    NodeId subjectNodeId    = sessionHandle->GetPeer().GetNodeId();
+    FabricIndex fabricIndex = sessionHandle->GetFabricIndex();
+
+    // first, delete existing ACLs for this nodeId
+    {
+        size_t index;
+        CHIP_ERROR err;
+        while (CHIP_NO_ERROR == (err = GetACLEntryIndex(&index, fabricIndex, subjectNodeId)))
+        {
+            err = Access::GetAccessControl().DeleteEntry(nullptr, fabricIndex, index);
+            if (err != CHIP_NO_ERROR)
+            {
+                ChipLogDetail(DeviceLayer, "ContentAppPlatform::ManageClientAccess error entry %d err %s", static_cast<int>(index),
+                              ErrorStr(err));
+            }
+        }
+    }
+
     Access::AccessControl::Entry entry;
     ReturnErrorOnFailure(GetAccessControl().PrepareEntry(entry));
     ReturnErrorOnFailure(entry.SetAuthMode(Access::AuthMode::kCase));
-    entry.SetFabricIndex(sessionHandle->GetFabricIndex());
+    entry.SetFabricIndex(fabricIndex);
     ReturnErrorOnFailure(entry.SetPrivilege(vendorPrivilege));
-    ReturnErrorOnFailure(entry.AddSubject(nullptr, sessionHandle->GetPeer().GetNodeId()));
+    ReturnErrorOnFailure(entry.AddSubject(nullptr, subjectNodeId));
 
     std::vector<Binding::Structs::TargetStruct::Type> bindings;
 
@@ -502,6 +577,13 @@ CHIP_ERROR ContentAppPlatform::ManageClientAccess(Messaging::ExchangeManager & e
      * We could have organized things differently, for example,
      * - a single ACL for (a) and (b) which is shared by many subjects
      * - a single ACL entry per subject for (c)
+     *
+     * We are also creating the following set of bindings on the remote device:
+     * a) Video Player endpoint
+     * b) Speaker endpoint
+     * c) selection of content app endpoints (0 to many)
+     * The purpose of the bindings is to inform the client of its access to
+     * nodeId and endpoints on the app platform.
      */
 
     ChipLogProgress(Controller, "Create video player endpoint ACL and binding");

src/app/app-platform/ContentAppPlatform.h

@@ -138,6 +138,11 @@ class DLL_EXPORT ContentAppPlatform
      *   Add ACLs on this device for the given client,
      *   and create bindings on the given client so that it knows what it has access to.
      *
+     * The default implementation follows the device library Video Player Architecture spec
+     * for a typical video player given assumptions like video player endpoint id is 1 and
+     * speaker endpoint id is 2. Some devices may need to override this implementation when
+     * these assumptions are not correct.
+     *
      * @param[in] exchangeMgr     Exchange manager to be used to get an exchange context.
      * @param[in] sessionHandle   Reference to an established session.
      * @param[in] targetVendorId  Vendor ID for the target device.
@@ -155,6 +160,7 @@ class DLL_EXPORT ContentAppPlatform
     // requires vendorApp to be in the catalog of the platform
     ContentApp * LoadContentAppInternal(const CatalogVendorApp & vendorApp);
     ContentApp * GetContentAppInternal(const CatalogVendorApp & vendorApp);
+    CHIP_ERROR GetACLEntryIndex(size_t * foundIndex, FabricIndex fabricIndex, NodeId subjectNodeId);
 
     static const int kNoCurrentEndpointId = 0;
     EndpointId mCurrentAppEndpointId      = kNoCurrentEndpointId;

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

@@ -328,9 +328,6 @@ void OnPlatformEventHandler(const chip::DeviceLayer::ChipDeviceEvent * event, in
 
 } // anonymous namespace
 
-// As per specifications section 11.22.5.1. Constant RESP_MAX
-constexpr size_t kMaxRspLen = 900;
-
 class OpCredsFabricTableDelegate : public chip::FabricTable::Delegate
 {
 public:
@@ -961,7 +958,7 @@ bool emberAfOperationalCredentialsClusterAttestationRequestCallback(app::Command
     attestationElementsSpan = MutableByteSpan{ attestationElements.Get(), attestationElementsLen };
     err = Credentials::ConstructAttestationElements(certDeclSpan, attestationNonce, timestamp, kEmptyFirmwareInfo,
                                                     emptyVendorReserved, attestationElementsSpan);
-    VerifyOrExit((err == CHIP_NO_ERROR) && (attestationElementsSpan.size() <= kMaxRspLen), finalStatus = Status::Failure);
+    VerifyOrExit(err == CHIP_NO_ERROR, finalStatus = Status::Failure);
 
     // Append attestation challenge in the back of the reserved space for the signature
     memcpy(attestationElements.Get() + attestationElementsSpan.size(), attestationChallenge.data(), attestationChallenge.size());
@@ -1092,7 +1089,7 @@ bool emberAfOperationalCredentialsClusterCSRRequestCallback(app::CommandHandler
 
         err = Credentials::ConstructNOCSRElements(ByteSpan{ csrSpan.data(), csrSpan.size() }, CSRNonce, kNoVendorReserved,
                                                   kNoVendorReserved, kNoVendorReserved, nocsrElementsSpan);
-        VerifyOrExit((err == CHIP_NO_ERROR) && (nocsrElementsSpan.size() <= kMaxRspLen), finalStatus = Status::Failure);
+        VerifyOrExit(err == CHIP_NO_ERROR, finalStatus = Status::Failure);
 
         // Append attestation challenge in the back of the reserved space for the signature
         memcpy(nocsrElements.Get() + nocsrElementsSpan.size(), attestationChallenge.data(), attestationChallenge.size());