Add the ability to read extra attributes during commissioning (project-chip#36867)

* Add support for ExtraReadPaths to CommissioningParameters

* CHIP_CONFIG_ENABLE_READ_CLIENT strikes again

* Check for alloc failure

* Address further review comments

Author: ksperling-apple

src/app/AttributePathParams.h

@@ -30,27 +30,27 @@ struct AttributePathParams
 {
     AttributePathParams() = default;
 
-    explicit AttributePathParams(EndpointId aEndpointId) :
+    constexpr explicit AttributePathParams(EndpointId aEndpointId) :
         AttributePathParams(aEndpointId, kInvalidClusterId, kInvalidAttributeId, kInvalidListIndex)
     {}
 
     //
     // TODO: (Issue #10596) Need to ensure that we do not encode the NodeId over the wire
     // if it is either not 'set', or is set to a value that matches accessing fabric
     // on which the interaction is undertaken.
-    AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId) :
+    constexpr AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId) :
         AttributePathParams(aEndpointId, aClusterId, kInvalidAttributeId, kInvalidListIndex)
     {}
 
-    AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId, AttributeId aAttributeId) :
+    constexpr AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId, AttributeId aAttributeId) :
         AttributePathParams(aEndpointId, aClusterId, aAttributeId, kInvalidListIndex)
     {}
 
-    AttributePathParams(ClusterId aClusterId, AttributeId aAttributeId) :
+    constexpr AttributePathParams(ClusterId aClusterId, AttributeId aAttributeId) :
         AttributePathParams(kInvalidEndpointId, aClusterId, aAttributeId, kInvalidListIndex)
     {}
 
-    AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId, AttributeId aAttributeId, ListIndex aListIndex) :
+    constexpr AttributePathParams(EndpointId aEndpointId, ClusterId aClusterId, AttributeId aAttributeId, ListIndex aListIndex) :
         mClusterId(aClusterId), mAttributeId(aAttributeId), mEndpointId(aEndpointId), mListIndex(aListIndex)
     {}
 

src/controller/AutoCommissioner.cpp

@@ -18,13 +18,14 @@
 
 #include <controller/AutoCommissioner.h>
 
-#include <cstring>
-
 #include <app/InteractionModelTimeout.h>
 #include <controller/CHIPDeviceController.h>
 #include <credentials/CHIPCert.h>
 #include <lib/support/SafeInt.h>
 
+#include <cstring>
+#include <type_traits>
+
 namespace chip {
 namespace Controller {
 
@@ -225,6 +226,32 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
         mParams.SetICDClientType(params.GetICDClientType().Value());
     }
 
+    auto extraReadPaths = params.GetExtraReadPaths();
+    if (extraReadPaths.size() > 0)
+    {
+        using ReadPath = std::remove_pointer_t<decltype(extraReadPaths.data())>;
+        static_assert(std::is_trivially_copyable_v<ReadPath>, "can't use memmove / memcpy, not trivially copyable");
+
+        if (mExtraReadPaths.AllocatedSize() == extraReadPaths.size())
+        {
+            memmove(mExtraReadPaths.Get(), extraReadPaths.data(), extraReadPaths.size() * sizeof(ReadPath));
+        }
+        else
+        {
+            // We can't reallocate mExtraReadPaths yet as this would free the old buffer,
+            // and the caller might be passing a sub-span of the old paths.
+            decltype(mExtraReadPaths) oldReadPaths(std::move(mExtraReadPaths));
+            VerifyOrReturnError(mExtraReadPaths.Alloc(extraReadPaths.size()), CHIP_ERROR_NO_MEMORY);
+            memcpy(mExtraReadPaths.Get(), extraReadPaths.data(), extraReadPaths.size() * sizeof(ReadPath));
+        }
+
+        mParams.SetExtraReadPaths(mExtraReadPaths.Span());
+    }
+    else
+    {
+        mExtraReadPaths.Free();
+    }
+
     return CHIP_NO_ERROR;
 }
 

src/controller/AutoCommissioner.h

@@ -16,10 +16,13 @@
  *    limitations under the License.
  */
 #pragma once
+
+#include <app/AttributePathParams.h>
 #include <controller/CommissioneeDeviceProxy.h>
 #include <controller/CommissioningDelegate.h>
 #include <credentials/DeviceAttestationConstructor.h>
 #include <crypto/CHIPCryptoPAL.h>
+#include <lib/support/ScopedBuffer.h>
 #include <protocols/secure_channel/RendezvousParameters.h>
 
 namespace chip {
@@ -116,7 +119,9 @@ class AutoCommissioner : public CommissioningDelegate
     CommissioneeDeviceProxy * mCommissioneeDeviceProxy               = nullptr;
     OperationalCredentialsDelegate * mOperationalCredentialsDelegate = nullptr;
     OperationalDeviceProxy mOperationalDeviceProxy;
-    // Memory space for the commisisoning parameters that come in as ByteSpans - the caller is not guaranteed to retain this memory
+
+    // BEGIN memory space for commissioning parameters that are Spans or similar pointers:
+    // The caller is not guaranteed to retain the memory these parameters point to.
     uint8_t mSsid[CommissioningParameters::kMaxSsidLen];
     uint8_t mCredentials[CommissioningParameters::kMaxCredentialsLen];
     uint8_t mThreadOperationalDataset[CommissioningParameters::kMaxThreadDatasetLen];
@@ -136,6 +141,11 @@ class AutoCommissioner : public CommissioningDelegate
     static constexpr size_t kMaxDefaultNtpSize = 128;
     char mDefaultNtp[kMaxDefaultNtpSize];
 
+    uint8_t mICDSymmetricKey[Crypto::kAES_CCM128_Key_Length];
+    Platform::ScopedMemoryBufferWithSize<app::AttributePathParams> mExtraReadPaths;
+
+    // END memory space for commisisoning parameters
+
     bool mNeedsNetworkSetup = false;
     ReadCommissioningInfo mDeviceCommissioningInfo;
     bool mNeedsDST = false;
@@ -155,8 +165,6 @@ 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

@@ -2329,6 +2329,12 @@ void DeviceCommissioner::ContinueReadingCommissioningInfo(const CommissioningPar
                                                 Clusters::IcdManagement::Attributes::ActiveModeDuration::Id));
         VerifyOrReturn(builder.AddAttributePath(kRootEndpointId, Clusters::IcdManagement::Id,
                                                 Clusters::IcdManagement::Attributes::ActiveModeThreshold::Id));
+
+        // Extra paths requested via CommissioningParameters
+        for (auto const & path : params.GetExtraReadPaths())
+        {
+            VerifyOrReturn(builder.AddAttributePath(path));
+        }
     }();
 
     VerifyOrDie(builder.size() > 0); // our logic is broken if there is nothing to read
@@ -2363,6 +2369,7 @@ void DeviceCommissioner::FinishReadingCommissioningInfo()
     // up returning an error (e.g. because some mandatory information was missing).
     CHIP_ERROR err = CHIP_NO_ERROR;
     ReadCommissioningInfo info;
+    info.attributes = mAttributeCache.get();
     AccumulateErrors(err, ParseGeneralCommissioningInfo(info));
     AccumulateErrors(err, ParseBasicInformation(info));
     AccumulateErrors(err, ParseNetworkCommissioningInfo(info));

src/controller/CHIPDeviceController.h

@@ -816,6 +816,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
 
     Optional<CommissioningParameters> GetCommissioningParameters()
     {
+        // TODO: Return a non-optional const & to avoid a copy, mDefaultCommissioner is never null
         return mDefaultCommissioner == nullptr ? NullOptional : MakeOptional(mDefaultCommissioner->GetCommissioningParameters());
     }
 

src/controller/CommissioningDelegate.h

@@ -18,11 +18,14 @@
 
 #pragma once
 #include <app-common/zap-generated/cluster-objects.h>
+#include <app/AttributePathParams.h>
+#include <app/ClusterStateCache.h>
 #include <app/OperationalSessionSetup.h>
 #include <controller/CommissioneeDeviceProxy.h>
 #include <credentials/attestation_verifier/DeviceAttestationDelegate.h>
 #include <credentials/attestation_verifier/DeviceAttestationVerifier.h>
 #include <crypto/CHIPCryptoPAL.h>
+#include <lib/support/Span.h>
 #include <lib/support/Variant.h>
 #include <matter/tracing/build_config.h>
 #include <system/SystemClock.h>
@@ -591,6 +594,18 @@ class CommissioningParameters
     }
     void ClearICDStayActiveDurationMsec() { mICDStayActiveDurationMsec.ClearValue(); }
 
+    Span<const app::AttributePathParams> GetExtraReadPaths() const { return mExtraReadPaths; }
+
+    // Additional attribute paths to read as part of the kReadCommissioningInfo stage.
+    // These values read from the device will be available in ReadCommissioningInfo.attributes.
+    // Clients should avoid requesting paths that are already read internally by the commissioner
+    // as no consolidation of internally read and extra paths provided here will be performed.
+    CommissioningParameters & SetExtraReadPaths(Span<const app::AttributePathParams> paths)
+    {
+        mExtraReadPaths = paths;
+        return *this;
+    }
+
     // Clear all members that depend on some sort of external buffer.  Can be
     // used to make sure that we are not holding any dangling pointers.
     void ClearExternalBufferDependentValues()
@@ -613,6 +628,7 @@ class CommissioningParameters
         mDSTOffsets.ClearValue();
         mDefaultNTP.ClearValue();
         mICDSymmetricKey.ClearValue();
+        mExtraReadPaths = decltype(mExtraReadPaths)();
     }
 
 private:
@@ -662,6 +678,7 @@ class CommissioningParameters
     Optional<uint32_t> mICDStayActiveDurationMsec;
     ICDRegistrationStrategy mICDRegistrationStrategy = ICDRegistrationStrategy::kIgnore;
     bool mCheckForMatchingFabric                     = false;
+    Span<const app::AttributePathParams> mExtraReadPaths;
 };
 
 struct RequestedCertificate
@@ -762,6 +779,9 @@ struct ICDManagementClusterInfo
 
 struct ReadCommissioningInfo
 {
+#if CHIP_CONFIG_ENABLE_READ_CLIENT
+    app::ClusterStateCache const * attributes = nullptr;
+#endif
     NetworkClusters network;
     BasicClusterInfo basic;
     GeneralCommissioningInfo general;

src/lib/support/ScopedBuffer.h

@@ -26,6 +26,7 @@
 
 #include <lib/support/CHIPMem.h>
 #include <lib/support/CodeUtils.h>
+#include <lib/support/Span.h>
 
 #include <type_traits>
 #include <utility>
@@ -218,11 +219,12 @@ class ScopedMemoryBufferWithSize : public ScopedMemoryBuffer<T>
         return *this;
     }
 
-    ~ScopedMemoryBufferWithSize() { mCount = 0; }
-
     // return the size as count of elements
     inline size_t AllocatedSize() const { return mCount; }
 
+    chip::Span<T> Span() { return chip::Span<T>(this->Get(), AllocatedSize()); }
+    chip::Span<const T> Span() const { return chip::Span<T>(this->Get(), AllocatedSize()); }
+
     void Free()
     {
         mCount = 0;