From e9984f820cbd38d3189b11687ece57955a792c58 Mon Sep 17 00:00:00 2001 From: Sharad Binjola <31142146+sharadb-amazon@users.noreply.github.com> Date: Thu, 1 Sep 2022 11:56:20 -0700 Subject: [PATCH] Address feedback from https://github.com/project-chip/connectedhomeip/issues/22197 (#22175) * Address feedback from https://github.com/project-chip/connectedhomeip/issues/22197 * Allocating and deallocating partialDacVerifier before assignment * Avoiding returning a BOOL from onNocChainGenerationComplete * Allocating partialDacVerifier in initWithFactory and checking if allocation failed * Handling potentially dangling *cppCommissioner, dispatching GetCommissioningParams call on chipWorkQueue * Update src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm Co-authored-by: Boris Zbarsky * Update src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm Co-authored-by: Boris Zbarsky * Update src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm Co-authored-by: Boris Zbarsky Co-authored-by: Boris Zbarsky --- .../Framework/CHIP/MTRDeviceController.mm | 14 ++++++++++++++ src/darwin/Framework/CHIP/MTRNOCChainIssuer.h | 7 ++++--- .../CHIP/MTROperationalCredentialsDelegate.h | 5 ++++- .../CHIP/MTROperationalCredentialsDelegate.mm | 17 ++++++++--------- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index d122d28f93e16f..a4474717a269c1 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -60,6 +60,7 @@ static NSString * const kErrorOperationalCredentialsInit = @"Init failure while creating operational credentials delegate"; static NSString * const kErrorOperationalKeypairInit = @"Init failure while creating operational keypair bridge"; static NSString * const kErrorPairingInit = @"Init failure while creating a pairing delegate"; +static NSString * const kErrorPartialDacVerifierInit = @"Init failure while creating a partial DAC verifier"; static NSString * const kErrorPairDevice = @"Failure while pairing the device"; static NSString * const kErrorUnpairDevice = @"Failure while unpairing the device"; static NSString * const kErrorStopPairing = @"Failure while trying to stop the pairing process"; @@ -106,6 +107,11 @@ - (instancetype)initWithFactory:(MTRControllerFactory *)factory queue:(dispatch_ return nil; } + _partialDACVerifier = new chip::Credentials::PartialDACVerifier(); + if ([self checkForInitError:(_partialDACVerifier != nullptr) logMsg:kErrorPartialDacVerifierInit]) { + return nil; + } + _operationalCredentialsDelegate = new MTROperationalCredentialsDelegate(); if ([self checkForInitError:(_operationalCredentialsDelegate != nullptr) logMsg:kErrorOperationalCredentialsInit]) { return nil; @@ -145,6 +151,9 @@ - (void)shutDownCppController _cppCommissioner->Shutdown(); delete _cppCommissioner; _cppCommissioner = nullptr; + if (_operationalCredentialsDelegate != nil) { + _operationalCredentialsDelegate->SetDeviceCommissioner(nullptr); + } } } @@ -160,6 +169,11 @@ - (void)cleanup _operationalCredentialsDelegate = nullptr; } + if (_partialDACVerifier) { + delete _partialDACVerifier; + _partialDACVerifier = nullptr; + } + if (_pairingDelegateBridge) { delete _pairingDelegateBridge; _pairingDelegateBridge = nullptr; diff --git a/src/darwin/Framework/CHIP/MTRNOCChainIssuer.h b/src/darwin/Framework/CHIP/MTRNOCChainIssuer.h index 2ef76670532118..7c1eb03e37c1b7 100644 --- a/src/darwin/Framework/CHIP/MTRNOCChainIssuer.h +++ b/src/darwin/Framework/CHIP/MTRNOCChainIssuer.h @@ -22,6 +22,9 @@ NS_ASSUME_NONNULL_BEGIN +typedef void (^MTRNOCChainGenerationCompleteHandler)(NSData * operationalCertificate, NSData * intermediateCertificate, + NSData * rootCertificate, NSData * _Nullable ipk, NSNumber * _Nullable adminSubject, NSError * __autoreleasing * error); + @protocol MTRNOCChainIssuer @required @@ -43,9 +46,7 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)onNOCChainGenerationNeeded:(CSRInfo *)csrInfo attestationInfo:(AttestationInfo *)attestationInfo - onNOCChainGenerationComplete:(void (^)(NSData * operationalCertificate, NSData * intermediateCertificate, - NSData * rootCertificate, NSData * ipk, NSNumber * adminSubject, - NSError * __autoreleasing * error))onNOCChainGenerationComplete; + onNOCChainGenerationComplete:(MTRNOCChainGenerationCompleteHandler)onNOCChainGenerationComplete; @end diff --git a/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.h b/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.h index 803e19eee30eed..b6427512f31106 100644 --- a/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.h +++ b/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.h @@ -57,7 +57,10 @@ class MTROperationalCredentialsDelegate : public chip::Controller::OperationalCr void SetDeviceID(chip::NodeId deviceId) { mDeviceBeingPaired = deviceId; } void ResetDeviceID() { mDeviceBeingPaired = chip::kUndefinedNodeId; } - void SetDeviceCommissioner(chip::Controller::DeviceCommissioner * cppCommissioner) { mCppCommissioner = cppCommissioner; } + void SetDeviceCommissioner(chip::Controller::DeviceCommissioner * _Nullable cppCommissioner) + { + mCppCommissioner = cppCommissioner; + } chip::Optional GetCommissioningParameters() { diff --git a/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm b/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm index 1113ad18505086..0703f4c065e76b 100644 --- a/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm +++ b/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm @@ -147,6 +147,7 @@ const chip::ByteSpan & DAC, const chip::ByteSpan & PAI, chip::Callback::Callback * onCompletion) { + VerifyOrReturnError(mCppCommissioner != nullptr, CHIP_ERROR_INCORRECT_STATE); mOnNOCCompletionCallback = onCompletion; TLVReader reader; @@ -178,12 +179,8 @@ chip::ByteSpan firmwareInfoSpan; chip::Credentials::DeviceAttestationVendorReservedDeconstructor vendorReserved; - __block chip::Optional commissioningParameters; - // Dereferencing mCppCommissioner as it would be set to point to a valid Cpp commissioner by now, as we are in the middle of - // commissioning - dispatch_sync(mChipWorkQueue, ^{ - commissioningParameters = mCppCommissioner->GetCommissioningParameters(); - }); + chip::Optional commissioningParameters + = mCppCommissioner->GetCommissioningParameters(); VerifyOrReturnError(commissioningParameters.HasValue(), CHIP_ERROR_INCORRECT_STATE); // Attestation Elements, nonce and signature will have a value in Commissioning Params as the CSR needs a signature or else we @@ -231,9 +228,11 @@ return; } - // use ipk and adminSubject from CommissioningParameters if not passed in. - // Dereferencing mCppCommissioner as it would be set to point to a valid Cpp commissioner by now, as we are in the middle of - // commissioning + if (mCppCommissioner == nullptr) { + setNSError(CHIP_ERROR_INCORRECT_STATE, error); + return; + } + __block chip::Optional commissioningParameters; dispatch_sync(mChipWorkQueue, ^{ commissioningParameters = mCppCommissioner->GetCommissioningParameters();