From 711527f39d94466e8836240e670da391e2a12b2f Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Wed, 31 Aug 2022 12:39:10 -0700 Subject: [PATCH 1/8] Issue 22318 - commissioner attestation delegate should be able to override success --- src/controller/CHIPDeviceController.cpp | 55 ++++++++++++------- src/controller/CHIPDeviceController.h | 21 ++++--- .../DacOnlyPartialAttestationVerifier.cpp | 2 +- .../DefaultDeviceAttestationVerifier.cpp | 2 +- .../DeviceAttestationDelegate.h | 21 ++++++- .../DeviceAttestationVerifier.cpp | 27 +++++++++ .../DeviceAttestationVerifier.h | 44 +++++++++++++-- .../TestDeviceAttestationCredentials.cpp | 5 +- .../CHIP/MTRDeviceAttestationDelegate.h | 26 +++++++++ .../CHIP/MTRDeviceAttestationDelegate.mm | 33 +++++++++++ .../CHIP/MTRDeviceAttestationDelegateBridge.h | 9 ++- .../MTRDeviceAttestationDelegateBridge.mm | 28 +++++++++- .../MTRDeviceAttestationDelegate_Internal.h | 28 ++++++++++ .../Framework/CHIP/MTRDeviceController.mm | 2 +- .../Matter.xcodeproj/project.pbxproj | 8 +++ 15 files changed, 267 insertions(+), 44 deletions(-) create mode 100644 src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.mm create mode 100644 src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate_Internal.h diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index b62ae41e823d8e..7ce136e40feec2 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -793,8 +793,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) @@ -990,7 +990,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(context); @@ -1001,6 +1002,9 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * conte return; } + auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters(); + Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate(); + if (result != AttestationVerificationResult::kSuccess) { CommissioningDelegate::CommissioningReport report; @@ -1021,14 +1025,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 { @@ -1037,15 +1038,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(context); if (!commissioner->mDeviceBeingCommissioned) @@ -1058,8 +1066,9 @@ void DeviceCommissioner::OnArmFailSafeExtendedForFailedDeviceAttestation( if (deviceAttestationDelegate) { ChipLogProgress(Controller, "Device attestation failed, delegating error handling to client"); - deviceAttestationDelegate->OnDeviceAttestationFailed(commissioner, commissioner->mDeviceBeingCommissioned, - commissioner->mAttestationResult); + deviceAttestationDelegate->OnDeviceAttestationCompleted(commissioner, commissioner->mDeviceBeingCommissioned, + *commissioner->mAttestationDeviceInfo, + commissioner->mAttestationResult); } else { @@ -1070,7 +1079,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(context); @@ -1080,13 +1089,20 @@ 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(); + + if (deviceAttestationDelegate->ShouldWaitAfterDeviceAttestation()) + { + mAttestationDeviceInfo = Platform::MakeUnique(info); + } + + auto expiryLengthSeconds = deviceAttestationDelegate->FailSafeExpiryTimeoutSecs(); if (expiryLengthSeconds.HasValue()) { GeneralCommissioning::Commands::ArmFailSafe::Type request; @@ -1095,8 +1111,8 @@ 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(mDeviceBeingCommissioned, request, OnArmFailSafeExtendedForFailedDeviceAttestation, - OnFailedToExtendedArmFailSafeFailedDeviceAttestation, + SendCommand(mDeviceBeingCommissioned, request, OnArmFailSafeExtendedForDeviceAttestation, + OnFailedToExtendedArmFailSafeDeviceAttestation, MakeOptional(kMinimumCommissioningStepTimeout)); } else @@ -1104,7 +1120,7 @@ void DeviceCommissioner::ExtendArmFailSafeForFailedDeviceAttestation(Attestation 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); } } @@ -1471,6 +1487,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 index 62f97c0c30e80d..5d8ccf4a242c1f 100644 --- 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,9 @@ 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 ClearAttestationDeviceInfo(); + 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 +755,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 ipk, Optional adminSubject); @@ -779,9 +782,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 +863,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, chip::Callback::Callback mOnDeviceConnectedCallback; chip::Callback::Callback mOnDeviceConnectionFailureCallback; - chip::Callback::Callback mDeviceAttestationInformationVerificationCallback; + chip::Callback::Callback + mDeviceAttestationInformationVerificationCallback; chip::Callback::Callback mDeviceNOCChainCallback; SetUpCodePairer mSetUpCodePairer; @@ -874,6 +878,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, Platform::UniquePtr mAttributeCache; Platform::UniquePtr mReadClient; Credentials::AttestationVerificationResult mAttestationResult; + Platform::UniquePtr mAttestationDeviceInfo; Credentials::DeviceAttestationVerifier * mDeviceAttestationVerifier = nullptr; }; diff --git a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp index 4dd6ab7d35a8f2..0d40f240fdcec4 100644 --- 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 index 07db1d3726d9b9..f60d4b2d70d325 100644 --- 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 index 41ad284932abcf..157a7ba0a6ee87 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationDelegate.h +++ b/src/credentials/attestation_verifier/DeviceAttestationDelegate.h @@ -48,14 +48,29 @@ 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. + * + * If ShouldWaitAfterDeviceAttestation returns false, then in the case attestationResult is successful, the + * commissioner would finish commissioning the device after OnDeviceAttestationCompleted returns. + * + * If ShouldWaitAfterDeviceAttestation returns true, then the commissioner will always wait for a + * ContinueCommissioningAfterDeviceAttestation call after calling OnDeviceAttestationCompleted. * * @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 index 68c69136893e19..3c2a2f72d603fc 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp @@ -16,7 +16,9 @@ */ #include "DeviceAttestationVerifier.h" +#include #include +#include using namespace chip::Crypto; @@ -111,5 +113,30 @@ void SetDeviceAttestationVerifier(DeviceAttestationVerifier * verifier) gDacVerifier = verifier; } +static inline Platform::ScopedMemoryBufferWithSize CopyByteSpanHelper(const ByteSpan & span_to_copy) +{ + Platform::ScopedMemoryBufferWithSize bufferCopy; + 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 index 756759d32ebe03..10360b2f21f88a 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h @@ -20,6 +20,7 @@ #include #include #include +#include #include 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. * @@ -125,7 +124,7 @@ class AttestationTrustStore virtual ~AttestationTrustStore() = default; // Not copyable - AttestationTrustStore(const AttestationTrustStore &) = delete; + AttestationTrustStore(const AttestationTrustStore &) = delete; AttestationTrustStore & operator=(const AttestationTrustStore &) = delete; /** @@ -198,7 +197,7 @@ class DeviceAttestationVerifier virtual ~DeviceAttestationVerifier() = default; // Not copyable - DeviceAttestationVerifier(const DeviceAttestationVerifier &) = delete; + DeviceAttestationVerifier(const DeviceAttestationVerifier &) = delete; DeviceAttestationVerifier & operator=(const DeviceAttestationVerifier &) = delete; struct AttestationInfo @@ -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 cdBuffer() const + { + if (mCdBuffer.Get()) + { + return MakeOptional(ByteSpan(mDacDerBuffer.Get(), mDacDerBuffer.AllocatedSize())); + } + else + { + return Optional(); + } + } + + private: + Platform::ScopedMemoryBufferWithSize mPaiDerBuffer; + Platform::ScopedMemoryBufferWithSize mDacDerBuffer; + Platform::ScopedMemoryBufferWithSize mCdBuffer; + }; + + typedef void (*OnAttestationInformationVerification)(void * context, const AttestationInfo & info, + AttestationVerificationResult result); + /** * @brief Verify an attestation information payload against a DAC/PAI chain. * diff --git a/src/credentials/tests/TestDeviceAttestationCredentials.cpp b/src/credentials/tests/TestDeviceAttestationCredentials.cpp index 5af5b2db5bc828..28549eb9b55d31 100644 --- a/src/credentials/tests/TestDeviceAttestationCredentials.cpp +++ b/src/credentials/tests/TestDeviceAttestationCredentials.cpp @@ -148,7 +148,8 @@ static void TestDACProvidersExample_Signature(nlTestSuite * inSuite, void * inCo NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); } -static void OnAttestationInformationVerificationCallback(void * context, AttestationVerificationResult result) +static void OnAttestationInformationVerificationCallback(void * context, const DeviceAttestationVerifier::AttestationInfo & info, + AttestationVerificationResult result) { AttestationVerificationResult * pResult = reinterpret_cast(context); *pResult = result; @@ -205,7 +206,7 @@ static void TestDACVerifierExample_AttestationInfoVerification(nlTestSuite * inS NL_TEST_ASSERT(inSuite, default_verifier == example_dac_verifier); attestationResult = AttestationVerificationResult::kNotImplemented; - Callback::Callback attestationInformationVerificationCallback( + Callback::Callback attestationInformationVerificationCallback( OnAttestationInformationVerificationCallback, &attestationResult); Credentials::DeviceAttestationVerifier::AttestationInfo info( diff --git a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h index 42ffa6212e9697..1ece1207bb8136 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h +++ b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h @@ -21,12 +21,38 @@ NS_ASSUME_NONNULL_BEGIN @class MTRDeviceController; +@interface MTRDeviceAttestationDeviceInfo : NSObject +- (instancetype)init NS_UNAVAILABLE; ++ (instancetype)new NS_UNAVAILABLE; +@property (nonatomic, readonly) NSData * dacCertificate; +@property (nonatomic, readonly) NSData * dacPAICertificate; +@property (nonatomic, readonly, nullable) NSData * certificateDeclaration; +@end + /** * The protocol definition for the MTRDeviceAttestationDelegate * * All delegate methods will be called on the callers queue. */ @protocol MTRDeviceAttestationDelegate +@optional +/** + * Notify the delegate when device attestation completed with device info for additional verification. If + * this callback is implemented, continueCommissioningDevice on MTRDeviceController is expected + * to be called if commisioning should continue. + * + * This allows the delegate to stop commissioning after examining the device info (DAC, PAI, CD). + * + * @param controller Controller corresponding to the commissioning process + * @param device Handle of device being commissioned + * @param attestationDeviceInfo Attestation information for the device + * @param error NSError representing the error code for the failure + */ +- (void)deviceAttestation:(MTRDeviceController *)controller + completedForDevice:(void *)device + attestationDeviceInfo:(MTRDeviceAttestationDeviceInfo *)attestationDeviceInfo + error:(NSError * _Nonnull)error; + /** * Notify the delegate when device attestation fails * diff --git a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.mm b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.mm new file mode 100644 index 00000000000000..83cd8d8a6288ab --- /dev/null +++ b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.mm @@ -0,0 +1,33 @@ +/** + * + * Copyright (c) 2022 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import +#import + +@implementation MTRDeviceAttestationDeviceInfo +- (instancetype)initWithDACCertificate:(NSData *)dacCertificate + dacPAICertificate:(NSData *)dacPAICertificate + certificateDeclaration:(NSData *)certificateDeclaration +{ + if (self = [super init]) { + _dacCertificate = [dacCertificate copy]; + _dacPAICertificate = [dacPAICertificate copy]; + _certificateDeclaration = [certificateDeclaration copy]; + } + return self; +} +@end diff --git a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h index cc44ef3584c91f..121b97f11b4a24 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h +++ b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h @@ -29,12 +29,13 @@ class MTRDeviceAttestationDelegateBridge : public chip::Credentials::DeviceAttes public: MTRDeviceAttestationDelegateBridge(MTRDeviceController * deviceController, id deviceAttestationDelegate, dispatch_queue_t queue, - chip::Optional expiryTimeoutSecs) + chip::Optional expiryTimeoutSecs, bool shouldWaitAfterDeviceAttestation = false) : mResult(chip::Credentials::AttestationVerificationResult::kSuccess) , mDeviceController(deviceController) , mDeviceAttestationDelegate(deviceAttestationDelegate) , mQueue(queue) , mExpiryTimeoutSecs(expiryTimeoutSecs) + , mShouldWaitAfterDeviceAttestation(shouldWaitAfterDeviceAttestation) { } @@ -42,9 +43,12 @@ class MTRDeviceAttestationDelegateBridge : public chip::Credentials::DeviceAttes chip::Optional FailSafeExpiryTimeoutSecs() const override { return mExpiryTimeoutSecs; } - void OnDeviceAttestationFailed(chip::Controller::DeviceCommissioner * deviceCommissioner, chip::DeviceProxy * device, + void OnDeviceAttestationCompleted(chip::Controller::DeviceCommissioner * deviceCommissioner, chip::DeviceProxy * device, + const chip::Credentials::DeviceAttestationVerifier::AttestationDeviceInfo & info, chip::Credentials::AttestationVerificationResult attestationResult) override; + bool ShouldWaitAfterDeviceAttestation() override { return mShouldWaitAfterDeviceAttestation; } + chip::Credentials::AttestationVerificationResult attestationVerificationResult() const { return mResult; } private: @@ -53,6 +57,7 @@ class MTRDeviceAttestationDelegateBridge : public chip::Credentials::DeviceAttes id mDeviceAttestationDelegate; dispatch_queue_t mQueue; chip::Optional mExpiryTimeoutSecs; + bool mShouldWaitAfterDeviceAttestation; }; NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm index db8f277ca8a144..911a164609cd2a 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm @@ -16,10 +16,13 @@ */ #import "MTRDeviceAttestationDelegateBridge.h" +#import "MTRDeviceAttestationDelegate_Internal.h" #import "MTRError_Internal.h" +#import "NSDataSpanConversion.h" -void MTRDeviceAttestationDelegateBridge::OnDeviceAttestationFailed(chip::Controller::DeviceCommissioner * deviceCommissioner, - chip::DeviceProxy * device, chip::Credentials::AttestationVerificationResult attestationResult) +void MTRDeviceAttestationDelegateBridge::OnDeviceAttestationCompleted(chip::Controller::DeviceCommissioner * deviceCommissioner, + chip::DeviceProxy * device, const chip::Credentials::DeviceAttestationVerifier::AttestationDeviceInfo & info, + chip::Credentials::AttestationVerificationResult attestationResult) { dispatch_async(mQueue, ^{ NSLog(@"MTRDeviceAttestationDelegateBridge::OnDeviceAttestationFailed failed with result: %hu", attestationResult); @@ -27,7 +30,26 @@ mResult = attestationResult; id strongDelegate = mDeviceAttestationDelegate; - if ([strongDelegate respondsToSelector:@selector(deviceAttestation:failedForDevice:error:)]) { + if ([strongDelegate respondsToSelector:@selector(deviceAttestation:completedForDevice:attestationDeviceInfo:error:)]) { + MTRDeviceController * strongController = mDeviceController; + if (strongController) { + NSData * dacData = AsData(info.dacDerBuffer()); + NSData * paiData = AsData(info.paiDerBuffer()); + NSData * cdData = info.cdBuffer().HasValue() ? AsData(info.cdBuffer().Value()) : nil; + MTRDeviceAttestationDeviceInfo * deviceInfo = + [[MTRDeviceAttestationDeviceInfo alloc] initWithDACCertificate:dacData + dacPAICertificate:paiData + certificateDeclaration:cdData]; + NSError * error = (attestationResult == chip::Credentials::AttestationVerificationResult::kSuccess) + ? nil + : [MTRError errorForCHIPErrorCode:CHIP_ERROR_INTEGRITY_CHECK_FAILED]; + [strongDelegate deviceAttestation:mDeviceController + completedForDevice:device + attestationDeviceInfo:deviceInfo + error:error]; + } + } else if ((attestationResult != chip::Credentials::AttestationVerificationResult::kSuccess) && + [strongDelegate respondsToSelector:@selector(deviceAttestation:failedForDevice:error:)]) { MTRDeviceController * strongController = mDeviceController; if (strongController) { diff --git a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate_Internal.h new file mode 100644 index 00000000000000..cc66473718cbfa --- /dev/null +++ b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate_Internal.h @@ -0,0 +1,28 @@ +/** + * + * Copyright (c) 2022 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import + +NS_ASSUME_NONNULL_BEGIN + +@interface MTRDeviceAttestationDeviceInfo () +- (instancetype)initWithDACCertificate:(NSData *)dacCertificate + dacPAICertificate:(NSData *)dacPAICertificate + certificateDeclaration:(NSData *)certificateDeclaration; +@end + +NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index a4474717a269c1..6057b8b27273a5 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -486,7 +486,7 @@ - (BOOL)continueCommissioningDevice:(void *)device : chip::Credentials::AttestationVerificationResult::kSuccess; auto deviceProxy = static_cast(device); - auto errorCode = self.cppCommissioner->ContinueCommissioningAfterDeviceAttestationFailure(deviceProxy, + auto errorCode = self.cppCommissioner->ContinueCommissioningAfterDeviceAttestation(deviceProxy, ignoreAttestationFailure ? chip::Credentials::AttestationVerificationResult::kSuccess : lastAttestationResult); success = ![self checkForError:errorCode logMsg:kErrorPairDevice error:error]; }); diff --git a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj index 17f34172908ec7..6e56afa28f948c 100644 --- a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj +++ b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj @@ -83,6 +83,8 @@ 5ACDDD7D27CD16D200EFD68A /* MTRAttributeCacheContainer.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5ACDDD7C27CD16D200EFD68A /* MTRAttributeCacheContainer.mm */; }; 5ACDDD7E27CD3F3A00EFD68A /* MTRAttributeCacheContainer_Internal.h in Headers */ = {isa = PBXBuildFile; fileRef = 5ACDDD7B27CD14AF00EFD68A /* MTRAttributeCacheContainer_Internal.h */; }; 5AE6D4E427A99041001F2493 /* MTRDeviceTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 5AE6D4E327A99041001F2493 /* MTRDeviceTests.m */; }; + 7534F12828BFF20300390851 /* MTRDeviceAttestationDelegate.mm in Sources */ = {isa = PBXBuildFile; fileRef = 7534F12628BFF20300390851 /* MTRDeviceAttestationDelegate.mm */; }; + 7534F12928BFF20300390851 /* MTRDeviceAttestationDelegate_Internal.h in Headers */ = {isa = PBXBuildFile; fileRef = 7534F12728BFF20300390851 /* MTRDeviceAttestationDelegate_Internal.h */; }; 754F3DF427FBB94B00E60580 /* MTREventTLVValueDecoder_Internal.h in Headers */ = {isa = PBXBuildFile; fileRef = 754F3DF327FBB94B00E60580 /* MTREventTLVValueDecoder_Internal.h */; }; 7560FD1C27FBBD3F005E85B3 /* MTREventTLVValueDecoder.mm in Sources */ = {isa = PBXBuildFile; fileRef = 7560FD1B27FBBD3F005E85B3 /* MTREventTLVValueDecoder.mm */; }; 7596A83E28751220004DAE0E /* MTRBaseClusters_internal.h in Headers */ = {isa = PBXBuildFile; fileRef = 7596A83D28751220004DAE0E /* MTRBaseClusters_internal.h */; }; @@ -221,6 +223,8 @@ 5ACDDD7B27CD14AF00EFD68A /* MTRAttributeCacheContainer_Internal.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MTRAttributeCacheContainer_Internal.h; sourceTree = ""; }; 5ACDDD7C27CD16D200EFD68A /* MTRAttributeCacheContainer.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRAttributeCacheContainer.mm; sourceTree = ""; }; 5AE6D4E327A99041001F2493 /* MTRDeviceTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = MTRDeviceTests.m; sourceTree = ""; }; + 7534F12628BFF20300390851 /* MTRDeviceAttestationDelegate.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRDeviceAttestationDelegate.mm; sourceTree = ""; }; + 7534F12728BFF20300390851 /* MTRDeviceAttestationDelegate_Internal.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceAttestationDelegate_Internal.h; sourceTree = ""; }; 754F3DF327FBB94B00E60580 /* MTREventTLVValueDecoder_Internal.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTREventTLVValueDecoder_Internal.h; sourceTree = ""; }; 7560FD1B27FBBD3F005E85B3 /* MTREventTLVValueDecoder.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = MTREventTLVValueDecoder.mm; path = "zap-generated/MTREventTLVValueDecoder.mm"; sourceTree = ""; }; 7596A83D28751220004DAE0E /* MTRBaseClusters_internal.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = MTRBaseClusters_internal.h; path = "zap-generated/MTRBaseClusters_internal.h"; sourceTree = ""; }; @@ -359,6 +363,8 @@ 27A53C1527FBC6920053F131 /* MTRAttestationTrustStoreBridge.h */, 27A53C1627FBC6920053F131 /* MTRAttestationTrustStoreBridge.mm */, 88EBF8CB27FABDD500686BC1 /* MTRDeviceAttestationDelegate.h */, + 7534F12728BFF20300390851 /* MTRDeviceAttestationDelegate_Internal.h */, + 7534F12628BFF20300390851 /* MTRDeviceAttestationDelegate.mm */, 88EBF8CD27FABDD500686BC1 /* MTRDeviceAttestationDelegateBridge.h */, 88EBF8CC27FABDD500686BC1 /* MTRDeviceAttestationDelegateBridge.mm */, 513DDB852761F69300DAA01A /* MTRAttributeTLVValueDecoder_Internal.h */, @@ -506,6 +512,7 @@ 99D466E12798936D0089A18F /* MTRCommissioningParameters.h in Headers */, 5136661528067D550025EDAE /* MTRControllerFactory_Internal.h in Headers */, 515C1C70284F9FFB00A48F0C /* MTRMemory.h in Headers */, + 7534F12928BFF20300390851 /* MTRDeviceAttestationDelegate_Internal.h in Headers */, D4772A46285AE98400383630 /* MTRClusterConstants.h in Headers */, B289D4212639C0D300D4E314 /* MTROnboardingPayloadParser.h in Headers */, 513DDB862761F69300DAA01A /* MTRAttributeTLVValueDecoder_Internal.h in Headers */, @@ -687,6 +694,7 @@ 5136661428067D550025EDAE /* MTRControllerFactory.mm in Sources */, 51B22C2A2740CB47008D5055 /* MTRCommandPayloadsObjc.mm in Sources */, AF5F90FF2878D351005503FA /* MTROTAProviderDelegateBridge.mm in Sources */, + 7534F12828BFF20300390851 /* MTRDeviceAttestationDelegate.mm in Sources */, 2C5EEEF7268A85C400CAE3D3 /* MTRDeviceConnectionBridge.mm in Sources */, 51B22C262740CB32008D5055 /* MTRStructsObjc.mm in Sources */, 2C222AD1255C620600E446B9 /* MTRBaseDevice.mm in Sources */, From 18cfcc0b0945d28450d2884c33edd2c19112a11f Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Fri, 2 Sep 2022 10:12:41 -0700 Subject: [PATCH 2/8] restyled --- .../attestation_verifier/DeviceAttestationVerifier.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h index 10360b2f21f88a..e94100f2a580f6 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h @@ -124,7 +124,7 @@ class AttestationTrustStore virtual ~AttestationTrustStore() = default; // Not copyable - AttestationTrustStore(const AttestationTrustStore &) = delete; + AttestationTrustStore(const AttestationTrustStore &) = delete; AttestationTrustStore & operator=(const AttestationTrustStore &) = delete; /** @@ -197,7 +197,7 @@ class DeviceAttestationVerifier virtual ~DeviceAttestationVerifier() = default; // Not copyable - DeviceAttestationVerifier(const DeviceAttestationVerifier &) = delete; + DeviceAttestationVerifier(const DeviceAttestationVerifier &) = delete; DeviceAttestationVerifier & operator=(const DeviceAttestationVerifier &) = delete; struct AttestationInfo From 4c1f9d6e10929c17a12e8eafdbae693529d922d5 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Sat, 3 Sep 2022 17:18:18 -0700 Subject: [PATCH 3/8] hook up darwin delegate when commissioning --- .../Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm | 2 +- src/darwin/Framework/CHIP/MTRDeviceController.mm | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm index 911a164609cd2a..528148d3e0cf8d 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm @@ -25,7 +25,7 @@ chip::Credentials::AttestationVerificationResult attestationResult) { dispatch_async(mQueue, ^{ - NSLog(@"MTRDeviceAttestationDelegateBridge::OnDeviceAttestationFailed failed with result: %hu", attestationResult); + NSLog(@"MTRDeviceAttestationDelegateBridge::OnDeviceAttestationFailed completed with result: %hu", attestationResult); mResult = attestationResult; diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 6057b8b27273a5..4732c9a189e60b 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -459,8 +459,12 @@ - (BOOL)commissionDevice:(uint64_t)deviceId timeoutSecs = chip::MakeOptional(static_cast([commissioningParams.failSafeExpiryTimeoutSecs unsignedIntValue])); } + BOOL shouldWaitAfterDeviceAttestation = NO; + if ([commissioningParams.deviceAttestationDelegate respondsToSelector:@selector(deviceAttestation:completedForDevice:attestationDeviceInfo:error:)]) { + shouldWaitAfterDeviceAttestation = YES; + } _deviceAttestationDelegateBridge = new MTRDeviceAttestationDelegateBridge( - self, commissioningParams.deviceAttestationDelegate, _chipWorkQueue, timeoutSecs); + self, commissioningParams.deviceAttestationDelegate, _chipWorkQueue, timeoutSecs, shouldWaitAfterDeviceAttestation); params.SetDeviceAttestationDelegate(_deviceAttestationDelegateBridge); } From 27ee7d2849cf2da461915d49455092eafda843ff Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Sat, 3 Sep 2022 17:19:43 -0700 Subject: [PATCH 4/8] restyled --- src/darwin/Framework/CHIP/MTRDeviceController.mm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 4732c9a189e60b..df2d47403e1be6 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -460,7 +460,8 @@ - (BOOL)commissionDevice:(uint64_t)deviceId = chip::MakeOptional(static_cast([commissioningParams.failSafeExpiryTimeoutSecs unsignedIntValue])); } BOOL shouldWaitAfterDeviceAttestation = NO; - if ([commissioningParams.deviceAttestationDelegate respondsToSelector:@selector(deviceAttestation:completedForDevice:attestationDeviceInfo:error:)]) { + if ([commissioningParams.deviceAttestationDelegate + respondsToSelector:@selector(deviceAttestation:completedForDevice:attestationDeviceInfo:error:)]) { shouldWaitAfterDeviceAttestation = YES; } _deviceAttestationDelegateBridge = new MTRDeviceAttestationDelegateBridge( From 8139d3f620a474f2da45a0903bcb916c6f0168b6 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Tue, 6 Sep 2022 08:37:42 -0700 Subject: [PATCH 5/8] header doc and nullability fix for darwin MTRDeviceAttestationDelegate --- .../DeviceAttestationDelegate.h | 7 ++----- .../Framework/CHIP/MTRDeviceAttestationDelegate.h | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/credentials/attestation_verifier/DeviceAttestationDelegate.h b/src/credentials/attestation_verifier/DeviceAttestationDelegate.h index 157a7ba0a6ee87..4d8e2ed50b5eaf 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationDelegate.h +++ b/src/credentials/attestation_verifier/DeviceAttestationDelegate.h @@ -50,11 +50,8 @@ class DeviceAttestationDelegate * 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 commissioning or fail the operation. * - * If ShouldWaitAfterDeviceAttestation returns false, then in the case attestationResult is successful, the - * commissioner would finish commissioning the device after OnDeviceAttestationCompleted returns. - * - * If ShouldWaitAfterDeviceAttestation returns true, then the commissioner will always wait for a - * ContinueCommissioningAfterDeviceAttestation call after calling OnDeviceAttestationCompleted. + * 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 diff --git a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h index 1ece1207bb8136..9f30053a945ad0 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h +++ b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegate.h @@ -36,6 +36,16 @@ NS_ASSUME_NONNULL_BEGIN */ @protocol MTRDeviceAttestationDelegate @optional +/** + * Only one of the following delegate callbacks should be implemented. + * + * If -deviceAttestation:failedForDevice:error: is implemented, then it will be called when device + * attestation fails, and the client can decide to continue or stop the commissioning. + * + * If -deviceAttestation:completedForDevice:attestationDeviceInfo:error: is implemented, then it + * will always be called when device attestation completes. + */ + /** * Notify the delegate when device attestation completed with device info for additional verification. If * this callback is implemented, continueCommissioningDevice on MTRDeviceController is expected @@ -46,12 +56,12 @@ NS_ASSUME_NONNULL_BEGIN * @param controller Controller corresponding to the commissioning process * @param device Handle of device being commissioned * @param attestationDeviceInfo Attestation information for the device - * @param error NSError representing the error code for the failure + * @param error NSError representing the error code on attestation failure. Nil if success. */ - (void)deviceAttestation:(MTRDeviceController *)controller completedForDevice:(void *)device attestationDeviceInfo:(MTRDeviceAttestationDeviceInfo *)attestationDeviceInfo - error:(NSError * _Nonnull)error; + error:(NSError * _Nullable)error; /** * Notify the delegate when device attestation fails From d6b3226985ef99fac951753dcc5be951c5c3ecba Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Tue, 6 Sep 2022 12:52:56 -0700 Subject: [PATCH 6/8] NULL check before memcpy Co-authored-by: Boris Zbarsky --- .../attestation_verifier/DeviceAttestationVerifier.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp index 3c2a2f72d603fc..ccaaa0eed88553 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp @@ -116,8 +116,10 @@ void SetDeviceAttestationVerifier(DeviceAttestationVerifier * verifier) static inline Platform::ScopedMemoryBufferWithSize CopyByteSpanHelper(const ByteSpan & span_to_copy) { Platform::ScopedMemoryBufferWithSize bufferCopy; - bufferCopy.Alloc(span_to_copy.size()); - memcpy(bufferCopy.Get(), span_to_copy.data(), span_to_copy.size()); + if (bufferCopy.Alloc(span_to_copy.size())) + { + memcpy(bufferCopy.Get(), span_to_copy.data(), span_to_copy.size()); + } return bufferCopy; } From 9a99545d2794f31984e45855f73a75492383f1b8 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Tue, 6 Sep 2022 12:54:28 -0700 Subject: [PATCH 7/8] Declare const member as const in MTRDeviceAttestationDelegateBridge Co-authored-by: Boris Zbarsky --- src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h index 121b97f11b4a24..30eff23345c7e7 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h +++ b/src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.h @@ -57,7 +57,7 @@ class MTRDeviceAttestationDelegateBridge : public chip::Credentials::DeviceAttes id mDeviceAttestationDelegate; dispatch_queue_t mQueue; chip::Optional mExpiryTimeoutSecs; - bool mShouldWaitAfterDeviceAttestation; + const bool mShouldWaitAfterDeviceAttestation; }; NS_ASSUME_NONNULL_END From dc014cde7cdd5cec194c3f22661a9c3d5157f1a0 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Tue, 6 Sep 2022 12:56:50 -0700 Subject: [PATCH 8/8] Address review comments --- src/controller/CHIPDeviceController.cpp | 7 ++----- src/controller/CHIPDeviceController.h | 1 - 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 7ce136e40feec2..b4e846c988e43e 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1065,7 +1065,7 @@ void DeviceCommissioner::OnArmFailSafeExtendedForDeviceAttestation( Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate(); if (deviceAttestationDelegate) { - ChipLogProgress(Controller, "Device attestation failed, delegating error handling to client"); + ChipLogProgress(Controller, "Device attestation completed, delegating continuation to client"); deviceAttestationDelegate->OnDeviceAttestationCompleted(commissioner, commissioner->mDeviceBeingCommissioned, *commissioner->mAttestationDeviceInfo, commissioner->mAttestationResult); @@ -1097,10 +1097,7 @@ void DeviceCommissioner::ExtendArmFailSafeForDeviceAttestation(const Credentials auto & params = mDefaultCommissioner->GetCommissioningParameters(); Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate(); - if (deviceAttestationDelegate->ShouldWaitAfterDeviceAttestation()) - { - mAttestationDeviceInfo = Platform::MakeUnique(info); - } + mAttestationDeviceInfo = Platform::MakeUnique(info); auto expiryLengthSeconds = deviceAttestationDelegate->FailSafeExpiryTimeoutSecs(); if (expiryLengthSeconds.HasValue()) diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 5d8ccf4a242c1f..9695179fed969b 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -714,7 +714,6 @@ 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 ClearAttestationDeviceInfo(); void ExtendArmFailSafeForDeviceAttestation(const Credentials::DeviceAttestationVerifier::AttestationInfo & info, Credentials::AttestationVerificationResult result); static void OnCertificateChainFailureResponse(void * context, CHIP_ERROR error);