Issue 22318 - commissioner attestation delegate should be able to ove…

…rride success (#22321) * Issue 22318 - commissioner attestation delegate should be able to override success * restyled * hook up darwin delegate when commissioning * restyled * header doc and nullability fix for darwin MTRDeviceAttestationDelegate * NULL check before memcpy Co-authored-by: Boris Zbarsky <[email protected]> * Declare const member as const in MTRDeviceAttestationDelegateBridge Co-authored-by: Boris Zbarsky <[email protected]> * Address review comments Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Sep 7, 2022 · 89f5c8a · 89f5c8a
1 parent af37272
commit 89f5c8a
Show file tree

Hide file tree

Showing 15 changed files with 278 additions and 45 deletions.
diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -795,8 +795,8 @@ CHIP_ERROR DeviceCommissioner::Commission(NodeId remoteDeviceId)
 }
 
 CHIP_ERROR
-DeviceCommissioner::ContinueCommissioningAfterDeviceAttestationFailure(DeviceProxy * device,
-                                                                       Credentials::AttestationVerificationResult attestationResult)
+DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * device,
+                                                                Credentials::AttestationVerificationResult attestationResult)
 {
     MATTER_TRACE_EVENT_SCOPE("continueCommissioningDevice", "DeviceCommissioner");
     if (device == nullptr || device != mDeviceBeingCommissioned)
@@ -992,7 +992,8 @@ void DeviceCommissioner::OnAttestationResponse(void * context,
     commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report);
 }
 
-void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * context, AttestationVerificationResult result)
+void DeviceCommissioner::OnDeviceAttestationInformationVerification(
+    void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info, AttestationVerificationResult result)
 {
     MATTER_TRACE_EVENT_SCOPE("OnDeviceAttestationInformationVerification", "DeviceCommissioner");
     DeviceCommissioner * commissioner = reinterpret_cast<DeviceCommissioner *>(context);
@@ -1003,6 +1004,9 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * conte
         return;
     }
 
+    auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters();
+    Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate();
+
     if (result != AttestationVerificationResult::kSuccess)
     {
         CommissioningDelegate::CommissioningReport report;
@@ -1023,14 +1027,11 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * conte
         // Go look at AttestationVerificationResult enum in src/credentials/attestation_verifier/DeviceAttestationVerifier.h to
         // understand the errors.
 
-        auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters();
-        Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate();
-
         // If a device attestation status delegate is installed, delegate handling of failure to the client and let them
         // decide on whether to proceed further or not.
         if (deviceAttestationDelegate)
         {
-            commissioner->ExtendArmFailSafeForFailedDeviceAttestation(result);
+            commissioner->ExtendArmFailSafeForDeviceAttestation(info, result);
         }
         else
         {
@@ -1039,15 +1040,22 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * conte
     }
     else
     {
-        ChipLogProgress(Controller, "Successfully validated 'Attestation Information' command received from the device.");
-        commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
+        if (deviceAttestationDelegate && deviceAttestationDelegate->ShouldWaitAfterDeviceAttestation())
+        {
+            commissioner->ExtendArmFailSafeForDeviceAttestation(info, result);
+        }
+        else
+        {
+            ChipLogProgress(Controller, "Successfully validated 'Attestation Information' command received from the device.");
+            commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
+        }
     }
 }
 
-void DeviceCommissioner::OnArmFailSafeExtendedForFailedDeviceAttestation(
+void DeviceCommissioner::OnArmFailSafeExtendedForDeviceAttestation(
     void * context, const GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data)
 {
-    // If this function starts using "data", need to fix ExtendArmFailSafeForFailedDeviceAttestation accordingly.
+    // If this function starts using "data", need to fix ExtendArmFailSafeForDeviceAttestation accordingly.
     DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
 
     if (!commissioner->mDeviceBeingCommissioned)
@@ -1059,9 +1067,10 @@ void DeviceCommissioner::OnArmFailSafeExtendedForFailedDeviceAttestation(
     Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate();
     if (deviceAttestationDelegate)
     {
-        ChipLogProgress(Controller, "Device attestation failed, delegating error handling to client");
-        deviceAttestationDelegate->OnDeviceAttestationFailed(commissioner, commissioner->mDeviceBeingCommissioned,
-                                                             commissioner->mAttestationResult);
+        ChipLogProgress(Controller, "Device attestation completed, delegating continuation to client");
+        deviceAttestationDelegate->OnDeviceAttestationCompleted(commissioner, commissioner->mDeviceBeingCommissioned,
+                                                                *commissioner->mAttestationDeviceInfo,
+                                                                commissioner->mAttestationResult);
     }
     else
     {
@@ -1072,7 +1081,7 @@ void DeviceCommissioner::OnArmFailSafeExtendedForFailedDeviceAttestation(
     }
 }
 
-void DeviceCommissioner::OnFailedToExtendedArmFailSafeFailedDeviceAttestation(void * context, CHIP_ERROR error)
+void DeviceCommissioner::OnFailedToExtendedArmFailSafeDeviceAttestation(void * context, CHIP_ERROR error)
 {
     ChipLogProgress(Controller, "Failed to extend fail-safe timer to handle attestation failure %s", chip::ErrorStr(error));
     DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
@@ -1082,13 +1091,17 @@ void DeviceCommissioner::OnFailedToExtendedArmFailSafeFailedDeviceAttestation(vo
     commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
 }
 
-void DeviceCommissioner::ExtendArmFailSafeForFailedDeviceAttestation(AttestationVerificationResult result)
+void DeviceCommissioner::ExtendArmFailSafeForDeviceAttestation(const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
+                                                               Credentials::AttestationVerificationResult result)
 {
     mAttestationResult = result;
 
     auto & params                                                      = mDefaultCommissioner->GetCommissioningParameters();
     Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate();
-    auto expiryLengthSeconds                                           = deviceAttestationDelegate->FailSafeExpiryTimeoutSecs();
+
+    mAttestationDeviceInfo = Platform::MakeUnique<Credentials::DeviceAttestationVerifier::AttestationDeviceInfo>(info);
+
+    auto expiryLengthSeconds = deviceAttestationDelegate->FailSafeExpiryTimeoutSecs();
     if (expiryLengthSeconds.HasValue())
     {
         GeneralCommissioning::Commands::ArmFailSafe::Type request;
@@ -1097,16 +1110,16 @@ void DeviceCommissioner::ExtendArmFailSafeForFailedDeviceAttestation(Attestation
         ChipLogProgress(Controller, "Changing fail-safe timer to %u seconds to handle DA failure", request.expiryLengthSeconds);
         // Per spec, anything we do with the fail-safe armed must not time out
         // in less than kMinimumCommissioningStepTimeout.
-        SendCommand<GeneralCommissioningCluster>(mDeviceBeingCommissioned, request, OnArmFailSafeExtendedForFailedDeviceAttestation,
-                                                 OnFailedToExtendedArmFailSafeFailedDeviceAttestation,
+        SendCommand<GeneralCommissioningCluster>(mDeviceBeingCommissioned, request, OnArmFailSafeExtendedForDeviceAttestation,
+                                                 OnFailedToExtendedArmFailSafeDeviceAttestation,
                                                  MakeOptional(kMinimumCommissioningStepTimeout));
     }
     else
     {
         ChipLogProgress(Controller, "Proceeding without changing fail-safe timer value as delegate has not set it");
         // Callee does not use data argument.
         const GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType data;
-        OnArmFailSafeExtendedForFailedDeviceAttestation(this, data);
+        OnArmFailSafeExtendedForDeviceAttestation(this, data);
     }
 }
 
@@ -1473,6 +1486,7 @@ void OnBasicFailure(void * context, CHIP_ERROR error)
 void DeviceCommissioner::CleanupCommissioning(DeviceProxy * proxy, NodeId nodeId, const CompletionStatus & completionStatus)
 {
     commissioningCompletionStatus = completionStatus;
+
     if (completionStatus.err == CHIP_NO_ERROR)
     {
 

diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h
@@ -487,15 +487,14 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     /**
      * @brief
      *   This function instructs the commissioner to proceed to the next stage of commissioning after
-     *   attestation failure is reported to an installed attestation delegate.
+     *   attestation is reported to an installed attestation delegate.
      *
      * @param[in] device                The device being commissioned.
      * @param[in] attestationResult     The attestation result to use instead of whatever the device
      *                                  attestation verifier came up with. May be a success or an error result.
      */
     CHIP_ERROR
-    ContinueCommissioningAfterDeviceAttestationFailure(DeviceProxy * device,
-                                                       Credentials::AttestationVerificationResult attestationResult);
+    ContinueCommissioningAfterDeviceAttestation(DeviceProxy * device, Credentials::AttestationVerificationResult attestationResult);
 
     CHIP_ERROR GetDeviceBeingCommissioned(NodeId deviceId, CommissioneeDeviceProxy ** device);
 
@@ -715,7 +714,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     /* Callback when the previously sent CSR request results in failure */
     static void OnCSRFailureResponse(void * context, CHIP_ERROR error);
 
-    void ExtendArmFailSafeForFailedDeviceAttestation(Credentials::AttestationVerificationResult result);
+    void ExtendArmFailSafeForDeviceAttestation(const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
+                                               Credentials::AttestationVerificationResult result);
     static void OnCertificateChainFailureResponse(void * context, CHIP_ERROR error);
     static void OnCertificateChainResponse(
         void * context, const app::Clusters::OperationalCredentials::Commands::CertificateChainResponse::DecodableType & response);
@@ -754,7 +754,9 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     static void OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle);
     static void OnDeviceConnectionFailureFn(void * context, const ScopedNodeId & peerId, CHIP_ERROR error);
 
-    static void OnDeviceAttestationInformationVerification(void * context, Credentials::AttestationVerificationResult result);
+    static void OnDeviceAttestationInformationVerification(void * context,
+                                                           const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
+                                                           Credentials::AttestationVerificationResult result);
 
     static void OnDeviceNOCChainGeneration(void * context, CHIP_ERROR status, const ByteSpan & noc, const ByteSpan & icac,
                                            const ByteSpan & rcac, Optional<AesCcm128KeySpan> ipk, Optional<NodeId> adminSubject);
@@ -779,9 +781,9 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
                                  const app::Clusters::GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data);
     static void OnDisarmFailsafeFailure(void * context, CHIP_ERROR error);
     void DisarmDone();
-    static void OnArmFailSafeExtendedForFailedDeviceAttestation(
+    static void OnArmFailSafeExtendedForDeviceAttestation(
         void * context, const chip::app::Clusters::GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data);
-    static void OnFailedToExtendedArmFailSafeFailedDeviceAttestation(void * context, CHIP_ERROR error);
+    static void OnFailedToExtendedArmFailSafeDeviceAttestation(void * context, CHIP_ERROR error);
 
     /**
      * @brief
@@ -860,7 +862,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     chip::Callback::Callback<OnDeviceConnected> mOnDeviceConnectedCallback;
     chip::Callback::Callback<OnDeviceConnectionFailure> mOnDeviceConnectionFailureCallback;
 
-    chip::Callback::Callback<Credentials::OnAttestationInformationVerification> mDeviceAttestationInformationVerificationCallback;
+    chip::Callback::Callback<Credentials::DeviceAttestationVerifier::OnAttestationInformationVerification>
+        mDeviceAttestationInformationVerificationCallback;
 
     chip::Callback::Callback<OnNOCChainGeneration> mDeviceNOCChainCallback;
     SetUpCodePairer mSetUpCodePairer;
@@ -874,6 +877,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     Platform::UniquePtr<app::ClusterStateCache> mAttributeCache;
     Platform::UniquePtr<app::ReadClient> mReadClient;
     Credentials::AttestationVerificationResult mAttestationResult;
+    Platform::UniquePtr<Credentials::DeviceAttestationVerifier::AttestationDeviceInfo> mAttestationDeviceInfo;
     Credentials::DeviceAttestationVerifier * mDeviceAttestationVerifier = nullptr;
 };
 

diff --git a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp
@@ -148,7 +148,7 @@ void PartialDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer
     }
 
 exit:
-    onCompletion->mCall(onCompletion->mContext, attestationError);
+    onCompletion->mCall(onCompletion->mContext, info, attestationError);
 }
 
 } // namespace Credentials

diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp
@@ -277,7 +277,7 @@ void DefaultDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer
     }
 
 exit:
-    onCompletion->mCall(onCompletion->mContext, attestationError);
+    onCompletion->mCall(onCompletion->mContext, info, attestationError);
 }
 
 AttestationVerificationResult DefaultDACVerifier::ValidateCertificationDeclarationSignature(const ByteSpan & cmsEnvelopeBuffer,

diff --git a/src/credentials/attestation_verifier/DeviceAttestationDelegate.h b/src/credentials/attestation_verifier/DeviceAttestationDelegate.h
@@ -48,14 +48,26 @@ class DeviceAttestationDelegate
     /**
      * @brief
      *   This method is invoked when device attestation fails for a device that is being commissioned. The client
-     *   handling the failure has the option to continue commissionning or fail the operation.
+     *   handling the failure has the option to continue commissioning or fail the operation.
+     *
+     *   Optionally, when ShouldWaitAfterDeviceAttestation is overridden to return true, this method is also
+     *   invoked when device attestation succeeds.
      *
      *   @param deviceCommissioner The commissioner object that is commissioning the device
      *   @param device The proxy represent the device being commissioned
+     *   @param info The structure holding device info for additional verification by the application
      *   @param attestationResult The failure code for the device attestation validation operation
      */
-    virtual void OnDeviceAttestationFailed(Controller::DeviceCommissioner * deviceCommissioner, DeviceProxy * device,
-                                           AttestationVerificationResult attestationResult) = 0;
+    virtual void OnDeviceAttestationCompleted(Controller::DeviceCommissioner * deviceCommissioner, DeviceProxy * device,
+                                              const DeviceAttestationVerifier::AttestationDeviceInfo & info,
+                                              AttestationVerificationResult attestationResult) = 0;
+
+    /**
+     * @brief
+     *   Override this method to return whether the attestation delegate wants the commissioner to wait for a
+     *   ContinueCommissioningAfterDeviceAttestation call in the case attestationResult is successful.
+     */
+    virtual bool ShouldWaitAfterDeviceAttestation() { return false; }
 };
 
 } // namespace Credentials

diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp
@@ -16,7 +16,9 @@
  */
 #include "DeviceAttestationVerifier.h"
 
+#include <credentials/DeviceAttestationConstructor.h>
 #include <crypto/CHIPCryptoPAL.h>
+#include <lib/support/CHIPMem.h>
 
 using namespace chip::Crypto;
 
@@ -111,5 +113,32 @@ void SetDeviceAttestationVerifier(DeviceAttestationVerifier * verifier)
     gDacVerifier = verifier;
 }
 
+static inline Platform::ScopedMemoryBufferWithSize<uint8_t> CopyByteSpanHelper(const ByteSpan & span_to_copy)
+{
+    Platform::ScopedMemoryBufferWithSize<uint8_t> bufferCopy;
+    if (bufferCopy.Alloc(span_to_copy.size()))
+    {
+        memcpy(bufferCopy.Get(), span_to_copy.data(), span_to_copy.size());
+    }
+    return bufferCopy;
+}
+
+DeviceAttestationVerifier::AttestationDeviceInfo::AttestationDeviceInfo(const AttestationInfo & attestationInfo) :
+    mPaiDerBuffer(CopyByteSpanHelper(attestationInfo.paiDerBuffer)), mDacDerBuffer(CopyByteSpanHelper(attestationInfo.dacDerBuffer))
+{
+    ByteSpan certificationDeclarationSpan;
+    ByteSpan attestationNonceSpan;
+    uint32_t timestampDeconstructed;
+    ByteSpan firmwareInfoSpan;
+    DeviceAttestationVendorReservedDeconstructor vendorReserved;
+
+    if (DeconstructAttestationElements(attestationInfo.attestationElementsBuffer, certificationDeclarationSpan,
+                                       attestationNonceSpan, timestampDeconstructed, firmwareInfoSpan,
+                                       vendorReserved) == CHIP_NO_ERROR)
+    {
+        mCdBuffer = CopyByteSpanHelper(certificationDeclarationSpan);
+    }
+}
+
 } // namespace Credentials
 } // namespace chip
diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h
@@ -20,6 +20,7 @@
 #include <lib/core/CHIPCallback.h>
 #include <lib/core/CHIPError.h>
 #include <lib/core/CHIPVendorIdentifiers.hpp>
+#include <lib/support/ScopedBuffer.h>
 #include <lib/support/Span.h>
 
 namespace chip {
@@ -108,8 +109,6 @@ struct DeviceInfoForAttestation
     uint8_t paaSKID[Crypto::kSubjectKeyIdentifierLength] = { 0 };
 };
 
-typedef void (*OnAttestationInformationVerification)(void * context, AttestationVerificationResult result);
-
 /**
  * @brief Helper utility to model a basic trust store usable for device attestation verifiers.
  *
@@ -222,6 +221,43 @@ class DeviceAttestationVerifier
         uint16_t productId;
     };
 
+    // Copies the bytes passed to it, and holds the PAI, DAC, and CD for additional verification step
+    class AttestationDeviceInfo
+    {
+    public:
+        AttestationDeviceInfo(const AttestationInfo & attestationInfo);
+        AttestationDeviceInfo(const ByteSpan & attestationElementsBuffer, const ByteSpan paiDerBuffer, const ByteSpan dacDerBuffer);
+
+        ~AttestationDeviceInfo() = default;
+
+        // Returns buffer containing the PAI certificate from device in DER format.
+        const ByteSpan paiDerBuffer() const { return ByteSpan(mPaiDerBuffer.Get(), mPaiDerBuffer.AllocatedSize()); }
+
+        // Returns buffer containing the DAC certificate from device in DER format.
+        const ByteSpan dacDerBuffer() const { return ByteSpan(mDacDerBuffer.Get(), mDacDerBuffer.AllocatedSize()); }
+
+        // Returns optional buffer containing the certificate declaration from device.
+        const Optional<ByteSpan> cdBuffer() const
+        {
+            if (mCdBuffer.Get())
+            {
+                return MakeOptional(ByteSpan(mDacDerBuffer.Get(), mDacDerBuffer.AllocatedSize()));
+            }
+            else
+            {
+                return Optional<ByteSpan>();
+            }
+        }
+
+    private:
+        Platform::ScopedMemoryBufferWithSize<uint8_t> mPaiDerBuffer;
+        Platform::ScopedMemoryBufferWithSize<uint8_t> mDacDerBuffer;
+        Platform::ScopedMemoryBufferWithSize<uint8_t> mCdBuffer;
+    };
+
+    typedef void (*OnAttestationInformationVerification)(void * context, const AttestationInfo & info,
+                                                         AttestationVerificationResult result);
+
     /**
      * @brief Verify an attestation information payload against a DAC/PAI chain.
      *
-Original file line number
+Diff line change
@@ Expand Up @@
         }
     exit:
-        onCompletion->mCall(onCompletion->mContext, attestationError);
+        onCompletion->mCall(onCompletion->mContext, info, attestationError);
     }
     } // namespace Credentials
@@ Expand Down @@