From 0247d3258898f3cc1ee3148bf864122b3478c845 Mon Sep 17 00:00:00 2001 From: chrisdecenzo Date: Fri, 2 Sep 2022 08:58:36 -0700 Subject: [PATCH 1/8] Draft: address testing feedback from new opcert callbacks --- src/controller/AutoCommissioner.cpp | 38 +++++++++++++++++-- src/controller/AutoCommissioner.h | 6 +++ .../AndroidOperationalCredentialsIssuer.cpp | 4 +- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 6cc54e5ee913ca..e5c964d0f9cb6a 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -502,9 +502,41 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio SetDAC(report.Get().certificate); break; case CommissioningStage::kSendAttestationRequest: - // These don't need to be deep copied to local memory because they are used in this one step then never again. - mParams.SetAttestationElements(report.Get().attestationElements) - .SetAttestationSignature(report.Get().signature); + if (report.Get().attestationElements.size() > kAttestationElementsLength) + { + ChipLogError(Controller, "AutoCommissioner attestationElements buffer size %d larger than cache size %d", + static_cast(report.Get().attestationElements.size()), + static_cast(kAttestationElementsLength)); + mParams.SetAttestationElements(report.Get().attestationElements); + } + else + { + memcpy(mAttestationElements, report.Get().attestationElements.data(), + report.Get().attestationElements.size()); + mParams.SetAttestationElements( + ByteSpan(mAttestationElements, report.Get().attestationElements.size())); + } + + if (report.Get().signature.size() > kAttestationSignatureLength) + { + ChipLogError(Controller, + "AutoCommissioner attestationSignature buffer size %d larger than " + "cache size %d", + static_cast(report.Get().signature.size()), + static_cast(kAttestationSignatureLength)); + mParams.SetAttestationSignature(report.Get().signature); + } + else + { + memcpy(mAttestationSignature, report.Get().signature.data(), + report.Get().signature.size()); + mParams.SetAttestationSignature( + ByteSpan(mAttestationSignature, report.Get().signature.size())); + } + // ? These don't need to be deep copied to local memory because they are used in this one step then never again. + // mParams.SetAttestationElements(report.Get().attestationElements) + // .SetAttestationSignature(report.Get().signature); + // TODO: Does this need to be done at runtime? Seems like this could be done earlier and we wouldn't need to hold a // reference to the operational credential delegate here if (mOperationalCredentialsDelegate != nullptr) diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 4dbfa4332841bf..891b39452a458e 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -23,6 +23,9 @@ namespace chip { namespace Controller { +constexpr size_t kAttestationElementsLength = 900; +constexpr size_t kAttestationSignatureLength = 64; + class DeviceCommissioner; class AutoCommissioner : public CommissioningDelegate @@ -86,6 +89,9 @@ class AutoCommissioner : public CommissioningDelegate uint8_t mCSRNonce[kCSRNonceLength]; uint8_t mNOCertBuffer[Credentials::kMaxCHIPCertLength]; uint8_t mICACertBuffer[Credentials::kMaxCHIPCertLength]; + + uint8_t mAttestationElements[kAttestationElementsLength]; + uint8_t mAttestationSignature[kAttestationSignatureLength]; }; } // namespace Controller } // namespace chip diff --git a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp index e9808409da5d3c..a7d87a62b53b6c 100644 --- a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp +++ b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp @@ -159,7 +159,9 @@ CHIP_ERROR AndroidOperationalCredentialsIssuer::CallbackGenerateNOCChain(const B jmethodID method; CHIP_ERROR err = CHIP_NO_ERROR; JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread(); - err = JniReferences::GetInstance().FindMethod(env, mJavaObjectRef, "onNOCChainGenerationNeeded", "([B[B[B[B[B[B[B[B[B)V", + + err = JniReferences::GetInstance().FindMethod(env, mJavaObjectRef, "onNOCChainGenerationNeeded", + "(Lchip/devicecontroller/CSRInfo;Lchip/devicecontroller/AttestationInfo;)V", &method); if (err != CHIP_NO_ERROR) { From 6a2ff0aa2ff3f666ca591422c317886e701ad650 Mon Sep 17 00:00:00 2001 From: chrisdecenzo Date: Sat, 3 Sep 2022 17:41:16 -0700 Subject: [PATCH 2/8] temporary workaround to isolate a memory corruption --- src/controller/AutoCommissioner.cpp | 7 ++++++- src/controller/AutoCommissioner.h | 6 ++++++ .../java/AndroidOperationalCredentialsIssuer.cpp | 10 ++++++---- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index e5c964d0f9cb6a..fc535fead4a7ee 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -513,8 +513,12 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio { memcpy(mAttestationElements, report.Get().attestationElements.data(), report.Get().attestationElements.size()); + mAttestationElementsLen = static_cast(report.Get().attestationElements.size()); mParams.SetAttestationElements( ByteSpan(mAttestationElements, report.Get().attestationElements.size())); + ChipLogError(Controller, "AutoCommissioner setting attestationElements buffer size %d/%d", + static_cast(report.Get().attestationElements.size()), + static_cast(mParams.GetAttestationElements().Value().size())); } if (report.Get().signature.size() > kAttestationSignatureLength) @@ -530,12 +534,13 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio { memcpy(mAttestationSignature, report.Get().signature.data(), report.Get().signature.size()); + mAttestationSignatureLen = static_cast(report.Get().signature.size()); mParams.SetAttestationSignature( ByteSpan(mAttestationSignature, report.Get().signature.size())); } // ? These don't need to be deep copied to local memory because they are used in this one step then never again. // mParams.SetAttestationElements(report.Get().attestationElements) - // .SetAttestationSignature(report.Get().signature); + // .SetAttestationSignature(report.Get().signature); // TODO: Does this need to be done at runtime? Seems like this could be done earlier and we wouldn't need to hold a // reference to the operational credential delegate here diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 891b39452a458e..d04c10e21cb7f4 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -41,6 +41,10 @@ class AutoCommissioner : public CommissioningDelegate CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) override; + ByteSpan GetAttestationElements() const { return ByteSpan(mAttestationElements, mAttestationElementsLen); } + ByteSpan GetAttestationSignature() const { return ByteSpan(mAttestationSignature, mAttestationSignatureLen); } + ByteSpan GetAttestationNonce() const { return ByteSpan(mAttestationNonce, sizeof(mAttestationNonce)); } + protected: CommissioningStage GetNextCommissioningStage(CommissioningStage currentStage, CHIP_ERROR & lastErr); DeviceCommissioner * GetCommissioner() { return mCommissioner; } @@ -90,7 +94,9 @@ class AutoCommissioner : public CommissioningDelegate uint8_t mNOCertBuffer[Credentials::kMaxCHIPCertLength]; uint8_t mICACertBuffer[Credentials::kMaxCHIPCertLength]; + uint16_t mAttestationElementsLen = 0; uint8_t mAttestationElements[kAttestationElementsLength]; + uint16_t mAttestationSignatureLen = 0; uint8_t mAttestationSignature[kAttestationSignatureLength]; }; } // namespace Controller diff --git a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp index a7d87a62b53b6c..c552b3c53c5d64 100644 --- a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp +++ b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp @@ -221,17 +221,19 @@ CHIP_ERROR AndroidOperationalCredentialsIssuer::CallbackGenerateNOCChain(const B JniReferences::GetInstance().N2J_ByteArray(env, attestationChallenge.data(), attestationChallenge.size(), javaAttestationChallenge); - const ByteSpan & attestationElements = mAutoCommissioner->GetCommissioningParameters().GetAttestationElements().Value(); + const ByteSpan & attestationElements = mAutoCommissioner->GetAttestationElements(); jbyteArray javaAttestationElements; JniReferences::GetInstance().N2J_ByteArray(env, attestationElements.data(), attestationElements.size(), javaAttestationElements); + // new log message below + ChipLogError(Controller, "AndroidOpCredsIssuer attestationElements size %d/%d", static_cast(attestationElements.size()), + static_cast(env->GetArrayLength(javaAttestationElements))); - const ByteSpan & attestationNonce = mAutoCommissioner->GetCommissioningParameters().GetAttestationNonce().Value(); + const ByteSpan & attestationNonce = mAutoCommissioner->GetAttestationNonce(); jbyteArray javaAttestationNonce; JniReferences::GetInstance().N2J_ByteArray(env, attestationNonce.data(), attestationNonce.size(), javaAttestationNonce); - const ByteSpan & attestationElementsSignature = - mAutoCommissioner->GetCommissioningParameters().GetAttestationSignature().Value(); + const ByteSpan & attestationElementsSignature = mAutoCommissioner->GetAttestationSignature(); jbyteArray javaAttestationElementsSignature; JniReferences::GetInstance().N2J_ByteArray(env, attestationElementsSignature.data(), attestationElementsSignature.size(), javaAttestationElementsSignature); From 26fbecbb5bcacdd3a4924f4178f8b50aa15b99e3 Mon Sep 17 00:00:00 2001 From: chrisdecenzo Date: Sun, 4 Sep 2022 13:52:18 -0700 Subject: [PATCH 3/8] add check for existing fabric during commissioning, handle correctly on content app platform --- examples/platform/linux/CommissionerMain.cpp | 85 ++++++++++++++++++-- src/app/app-platform/ContentAppPlatform.cpp | 77 +++++++++++++++++- src/app/app-platform/ContentAppPlatform.h | 1 + src/controller/AutoCommissioner.cpp | 12 ++- src/controller/AutoCommissioner.h | 3 + src/controller/CHIPDeviceController.cpp | 47 ++++++++++- src/controller/CommissioningDelegate.h | 13 +++ 7 files changed, 226 insertions(+), 12 deletions(-) diff --git a/examples/platform/linux/CommissionerMain.cpp b/examples/platform/linux/CommissionerMain.cpp index 7e8bdbc7a39b5c..2a01d3ba71d803 100644 --- a/examples/platform/linux/CommissionerMain.cpp +++ b/examples/platform/linux/CommissionerMain.cpp @@ -30,7 +30,10 @@ #include #include +#include +#include #include +#include #include #include #include @@ -110,14 +113,50 @@ class MyCommissionerCallback : public CommissionerCallback } }; +AutoCommissioner gAutoCommissioner; + +class MyCredsIssuer : public ExampleOperationalCredentialsIssuer +{ + CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & csrNonce, const ByteSpan & attestationSignature, + const ByteSpan & attestationChallenge, const ByteSpan & DAC, const ByteSpan & PAI, + Callback::Callback * onCompletion) override + { + ChipLogError(Controller, "---------------------------MyCredsIssuer "); + // add parsing here + const ByteSpan & attestationElements = gAutoCommissioner.GetCommissioningParameters().GetAttestationElements().Value(); + ChipLogError(Controller, "MyCredsIssuer attestationElements size %d", static_cast(attestationElements.size())); + + ByteSpan certificationDeclarationSpan; + ByteSpan attestationNonceSpan; + uint32_t timestampDeconstructed; + ByteSpan firmwareInfoSpan; + DeviceAttestationVendorReservedDeconstructor vendorReserved; + + CHIP_ERROR err = DeconstructAttestationElements(attestationElements, certificationDeclarationSpan, attestationNonceSpan, + timestampDeconstructed, firmwareInfoSpan, vendorReserved); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Failed to parse attestation elements"); + } + else + { + ChipLogError(Controller, "Parse attestation elements success"); + } + + return ExampleOperationalCredentialsIssuer::GenerateNOCChain(csrElements, csrNonce, attestationSignature, + attestationChallenge, DAC, PAI, onCompletion); + } +}; + DeviceCommissioner gCommissioner; CommissionerDiscoveryController gCommissionerDiscoveryController; MyCommissionerCallback gCommissionerCallback; MyServerStorageDelegate gServerStorage; -ExampleOperationalCredentialsIssuer gOpCredsIssuer; +MyCredsIssuer gOpCredsIssuer; NodeId gLocalId = kMaxOperationalNodeId; Credentials::GroupDataProviderImpl gGroupDataProvider; -AutoCommissioner gAutoCommissioner; +Credentials::PartialDACVerifier gPartialDACVerifier; +FabricId gFabricId = 1; CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort) { @@ -140,6 +179,7 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort) params.controllerVendorId = static_cast(vendorId); ReturnErrorOnFailure(gOpCredsIssuer.Initialize(gServerStorage)); + gOpCredsIssuer.SetFabricIdForNextNOCRequest(gFabricId); // No need to explicitly set the UDC port since we will use default ChipLogProgress(Support, " ----- UDC listening on port %d", udcListenPort); @@ -150,6 +190,9 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort) const Credentials::AttestationTrustStore * testingRootStore = Credentials::GetTestAttestationTrustStore(); SetDeviceAttestationVerifier(GetDefaultDACVerifier(testingRootStore)); + // Uncomment the following line to perform DAC verification during NOC chain generation + // SetDeviceAttestationVerifier(&gPartialDACVerifier); + Platform::ScopedMemoryBuffer noc; VerifyOrReturnError(noc.Alloc(Controller::kMaxCHIPDERCertLength), CHIP_ERROR_NO_MEMORY); MutableByteSpan nocSpan(noc.Get(), Controller::kMaxCHIPDERCertLength); @@ -201,8 +244,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(fabricIndex)); + ChipLogProgress(Support, + "InitCommissioner nodeId=0x" ChipLogFormatX64 " fabric.fabricId=0x" ChipLogFormatX64 " fabricIndex=0x%x", + ChipLogValueX64(gCommissioner.GetNodeId()), ChipLogValueX64(gFabricId), static_cast(fabricIndex)); return CHIP_NO_ERROR; } @@ -231,6 +275,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 +340,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 +359,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) diff --git a/src/app/app-platform/ContentAppPlatform.cpp b/src/app/app-platform/ContentAppPlatform.cpp index 7865c87c71d0a3..540bfe2b77c77e 100644 --- a/src/app/app-platform/ContentAppPlatform.cpp +++ b/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(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(index), ErrorStr(err)); + continue; + } + if (count) + { + ChipLogDetail(DeviceLayer, "subjects: %u", static_cast(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(i), static_cast(index), ErrorStr(err)); + continue; + } + if (subject == subjectNodeId) + { + ChipLogDetail(DeviceLayer, "ContentAppPlatform::GetACLEntryIndex found matching subject at index %d", + static_cast(index)); + *foundIndex = index; + return CHIP_NO_ERROR; + } + } + } + } + } + } + return CHIP_ERROR_NOT_FOUND; +} + constexpr EndpointId kTargetBindingClusterEndpointId = 0; constexpr EndpointId kLocalVideoPlayerEndpointId = 1; constexpr EndpointId kLocalSpeakerEndpointId = 2; @@ -482,12 +537,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(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 bindings; diff --git a/src/app/app-platform/ContentAppPlatform.h b/src/app/app-platform/ContentAppPlatform.h index 3a39365e95c754..4fce0f668da36a 100644 --- a/src/app/app-platform/ContentAppPlatform.h +++ b/src/app/app-platform/ContentAppPlatform.h @@ -155,6 +155,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; diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index fc535fead4a7ee..278d5a12cc27a3 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -171,6 +171,10 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(CommissioningStage currentStage, CHIP_ERROR & lastErr) { + if (mStopCommissioning) + { + return CommissioningStage::kCleanup; + } if (lastErr != CHIP_NO_ERROR) { return CommissioningStage::kCleanup; @@ -345,6 +349,7 @@ CHIP_ERROR AutoCommissioner::StartCommissioning(DeviceCommissioner * commissione ChipLogError(Controller, "Device proxy secure session error"); return CHIP_ERROR_INVALID_ARGUMENT; } + mStopCommissioning = false; mCommissioner = commissioner; mCommissioneeDeviceProxy = proxy; mNeedsNetworkSetup = @@ -494,6 +499,10 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio .SetRemoteProductId(mDeviceCommissioningInfo.basic.productId) .SetDefaultRegulatoryLocation(mDeviceCommissioningInfo.general.currentRegulatoryLocation) .SetLocationCapability(mDeviceCommissioningInfo.general.locationCapability); + if (mDeviceCommissioningInfo.nodeId != kUndefinedNodeId) + { + mParams.SetRemoteNodeId(mDeviceCommissioningInfo.nodeId); + } break; case CommissioningStage::kSendPAICertificateRequest: SetPAI(report.Get().certificate); @@ -538,9 +547,6 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio mParams.SetAttestationSignature( ByteSpan(mAttestationSignature, report.Get().signature.size())); } - // ? These don't need to be deep copied to local memory because they are used in this one step then never again. - // mParams.SetAttestationElements(report.Get().attestationElements) - // .SetAttestationSignature(report.Get().signature); // TODO: Does this need to be done at runtime? Seems like this could be done earlier and we wouldn't need to hold a // reference to the operational credential delegate here diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index d04c10e21cb7f4..8c320d0be21a1f 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -38,6 +38,7 @@ class AutoCommissioner : public CommissioningDelegate void SetOperationalCredentialsDelegate(OperationalCredentialsDelegate * operationalCredentialsDelegate) override; CHIP_ERROR StartCommissioning(DeviceCommissioner * commissioner, CommissioneeDeviceProxy * proxy) override; + void StopCommissioning() { mStopCommissioning = true; }; CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) override; @@ -70,6 +71,8 @@ class AutoCommissioner : public CommissioningDelegate EndpointId GetEndpoint(const CommissioningStage & stage) const; CommissioningStage GetNextCommissioningStageInternal(CommissioningStage currentStage, CHIP_ERROR & lastErr); + bool mStopCommissioning = false; + DeviceCommissioner * mCommissioner = nullptr; CommissioneeDeviceProxy * mCommissioneeDeviceProxy = nullptr; OperationalCredentialsDelegate * mOperationalCredentialsDelegate = nullptr; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 7e72bb2b6f2a17..6ebefc4b5dffc9 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1733,6 +1733,45 @@ void DeviceCommissioner::OnDone(app::ReadClient *) return CHIP_NO_ERROR; } }); + + err = mAttributeCache->ForEachAttribute( + app::Clusters::OperationalCredentials::Id, [this, &info](const app::ConcreteAttributePath & path) { + if (path.mAttributeId != app::Clusters::OperationalCredentials::Attributes::Fabrics::Id) + { + // Continue on + return CHIP_NO_ERROR; + } + + switch (path.mAttributeId) + { + case app::Clusters::OperationalCredentials::Attributes::Fabrics::Id: { + app::Clusters::OperationalCredentials::Attributes::Fabrics::TypeInfo::DecodableType fabrics; + ReturnErrorOnFailure( + this->mAttributeCache->Get(path, + fabrics)); + auto iter = fabrics.begin(); + while (iter.Next()) + { + auto & fabricDescriptor = iter.GetValue(); + ChipLogProgress(AppServer, + "DeviceCommissioner::OnDone - fabric.vendorId=0x%04X fabric.fabricId=0x" ChipLogFormatX64 + " fabric.nodeId=0x" ChipLogFormatX64, + fabricDescriptor.vendorId, ChipLogValueX64(fabricDescriptor.fabricId), + ChipLogValueX64(fabricDescriptor.nodeId)); + if (GetFabricId() == fabricDescriptor.fabricId) + { + ChipLogProgress(AppServer, "DeviceCommissioner::OnDone - found a matching fabric"); + info.nodeId = fabricDescriptor.nodeId; + } + } + + return CHIP_NO_ERROR; + } + default: + return CHIP_NO_ERROR; + } + }); + // Try to parse as much as we can here before returning, even if this is an error. return_err = err == CHIP_NO_ERROR ? return_err : err; @@ -1997,7 +2036,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance(); app::ReadPrepareParams readParams(proxy->GetSecureSession().Value()); - app::AttributePathParams readPaths[8]; + app::AttributePathParams readPaths[9]; // Read all the feature maps for all the networking clusters on any endpoint to determine what is supported readPaths[0] = app::AttributePathParams(app::Clusters::NetworkCommissioning::Id, app::Clusters::NetworkCommissioning::Attributes::FeatureMap::Id); @@ -2018,9 +2057,13 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio // Read the requested minimum connection times from all network commissioning clusters readPaths[7] = app::AttributePathParams(app::Clusters::NetworkCommissioning::Id, app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::Id); + // Read the current fabrics + readPaths[8] = app::AttributePathParams(app::Clusters::OperationalCredentials::Id, + app::Clusters::OperationalCredentials::Attributes::Fabrics::Id); readParams.mpAttributePathParamsList = readPaths; - readParams.mAttributePathParamsListSize = 8; + readParams.mAttributePathParamsListSize = 9; + readParams.mIsFabricFiltered = false; if (timeout.HasValue()) { readParams.mTimeout = timeout.Value(); diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 1e3fdef1276e44..40fa72f05d3a70 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -177,11 +177,13 @@ class CommissioningParameters // Attestation elements from the node. These are obtained from node in response to the AttestationRequest command. In the // AutoCommissioner, this is automatically set from the report from the kSendAttestationRequest stage. // This must be set before calling PerformCommissioningStep for the kAttestationVerification step. + // Warning: data is not deep copied to local memory and can only be read during PerformCommissioningStep. const Optional GetAttestationElements() const { return mAttestationElements; } // Attestation signature from the node. This is obtained from node in response to the AttestationRequest command. In the // AutoCommissioner, this is automatically set from the report from the kSendAttestationRequest stage. // This must be set before calling PerformCommissioningStep for the kAttestationVerification step. + // Warning: data is not deep copied to local memory and can only be read during PerformCommissioningStep. const Optional GetAttestationSignature() const { return mAttestationSignature; } // Product attestation intermediate certificate from the node. This is obtained from the node in response to the @@ -196,6 +198,10 @@ class CommissioningParameters // This must be set before calling PerformCommissioningStep for the kAttestationVerification step. const Optional GetDAC() const { return mDAC; } + // Node ID when a matching fabric is found in the Node Operational Credentials cluster. + // In the AutoCommissioner, this is set from kReadCommissioningInfo stage. + const Optional GetRemoteNodeId() const { return mRemoteNodeId; } + // Node vendor ID from the basic information cluster. In the AutoCommissioner, this is automatically set from report from the // kReadCommissioningInfo stage. // This must be set before calling PerformCommissioningStep for the kAttestationVerification step. @@ -328,6 +334,11 @@ class CommissioningParameters mDAC = MakeOptional(dac); return *this; } + CommissioningParameters & SetRemoteNodeId(NodeId id) + { + mRemoteNodeId = MakeOptional(id); + return *this; + } CommissioningParameters & SetRemoteVendorId(VendorId id) { mRemoteVendorId = MakeOptional(id); @@ -407,6 +418,7 @@ class CommissioningParameters Optional mAttestationSignature; Optional mPAI; Optional mDAC; + Optional mRemoteNodeId; Optional mRemoteVendorId; Optional mRemoteProductId; Optional mDefaultRegulatoryLocation; @@ -491,6 +503,7 @@ struct ReadCommissioningInfo NetworkClusters network; BasicClusterInfo basic; GeneralCommissioningInfo general; + NodeId nodeId = kUndefinedNodeId; }; struct AttestationErrorInfo From 0c5629838601d687372af81de7c8828c2f722260 Mon Sep 17 00:00:00 2001 From: chrisdecenzo Date: Tue, 6 Sep 2022 17:54:28 -0700 Subject: [PATCH 4/8] address comments --- examples/platform/linux/CommissionerMain.cpp | 15 +++-- .../operational-credentials-server.cpp | 9 ++- src/controller/AutoCommissioner.cpp | 55 ++++++++----------- src/controller/AutoCommissioner.h | 8 +-- src/controller/CHIPDeviceController.cpp | 6 ++ src/controller/CommissioningDelegate.h | 2 - .../AndroidOperationalCredentialsIssuer.cpp | 5 +- .../DeviceAttestationConstructor.h | 3 + 8 files changed, 48 insertions(+), 55 deletions(-) diff --git a/examples/platform/linux/CommissionerMain.cpp b/examples/platform/linux/CommissionerMain.cpp index 2a01d3ba71d803..ba0a05a02385c8 100644 --- a/examples/platform/linux/CommissionerMain.cpp +++ b/examples/platform/linux/CommissionerMain.cpp @@ -76,7 +76,7 @@ using namespace chip::app::Clusters; using namespace ::chip::Messaging; using namespace ::chip::Controller; -class MyServerStorageDelegate : public PersistentStorageDelegate +class CustomServerStorageDelegate : public PersistentStorageDelegate { CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override { @@ -105,7 +105,7 @@ class MyServerStorageDelegate : public PersistentStorageDelegate } }; -class MyCommissionerCallback : public CommissionerCallback +class CustomCommissionerCallback : public CommissionerCallback { void ReadyForCommissioning(uint32_t pincode, uint16_t longDiscriminator, PeerAddress peerAddress) override { @@ -115,16 +115,15 @@ class MyCommissionerCallback : public CommissionerCallback AutoCommissioner gAutoCommissioner; -class MyCredsIssuer : public ExampleOperationalCredentialsIssuer +class CustomCredsIssuer : public ExampleOperationalCredentialsIssuer { CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & csrNonce, const ByteSpan & attestationSignature, const ByteSpan & attestationChallenge, const ByteSpan & DAC, const ByteSpan & PAI, Callback::Callback * onCompletion) override { - ChipLogError(Controller, "---------------------------MyCredsIssuer "); // add parsing here const ByteSpan & attestationElements = gAutoCommissioner.GetCommissioningParameters().GetAttestationElements().Value(); - ChipLogError(Controller, "MyCredsIssuer attestationElements size %d", static_cast(attestationElements.size())); + ChipLogError(Controller, "CustomCredsIssuer attestationElements size %d", static_cast(attestationElements.size())); ByteSpan certificationDeclarationSpan; ByteSpan attestationNonceSpan; @@ -150,9 +149,9 @@ class MyCredsIssuer : public ExampleOperationalCredentialsIssuer DeviceCommissioner gCommissioner; CommissionerDiscoveryController gCommissionerDiscoveryController; -MyCommissionerCallback gCommissionerCallback; -MyServerStorageDelegate gServerStorage; -MyCredsIssuer gOpCredsIssuer; +CustomCommissionerCallback gCommissionerCallback; +CustomServerStorageDelegate gServerStorage; +CustomCredsIssuer gOpCredsIssuer; NodeId gLocalId = kMaxOperationalNodeId; Credentials::GroupDataProviderImpl gGroupDataProvider; Credentials::PartialDACVerifier gPartialDACVerifier; diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index baf6a97877df81..610277e2ba0c75 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/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,8 @@ 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) && (attestationElementsSpan.size() <= Credentials::kMaxRspLen), + 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 +1090,8 @@ 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) && (nocsrElementsSpan.size() <= Credentials::kMaxRspLen), + 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()); diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 278d5a12cc27a3..943f1687a0b32c 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -510,43 +510,33 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio case CommissioningStage::kSendDACCertificateRequest: SetDAC(report.Get().certificate); break; - case CommissioningStage::kSendAttestationRequest: - if (report.Get().attestationElements.size() > kAttestationElementsLength) + case CommissioningStage::kSendAttestationRequest: { + auto & elements = report.Get().attestationElements; + auto & signature = report.Get().signature; + if (elements.size() > sizeof(mAttestationElements)) { - ChipLogError(Controller, "AutoCommissioner attestationElements buffer size %d larger than cache size %d", - static_cast(report.Get().attestationElements.size()), - static_cast(kAttestationElementsLength)); - mParams.SetAttestationElements(report.Get().attestationElements); + ChipLogError(Controller, "AutoCommissioner attestationElements buffer size %u larger than cache size %u", + static_cast(elements.size()), static_cast(sizeof(mAttestationElements))); + return CHIP_ERROR_MESSAGE_TOO_LONG; } - else - { - memcpy(mAttestationElements, report.Get().attestationElements.data(), - report.Get().attestationElements.size()); - mAttestationElementsLen = static_cast(report.Get().attestationElements.size()); - mParams.SetAttestationElements( - ByteSpan(mAttestationElements, report.Get().attestationElements.size())); - ChipLogError(Controller, "AutoCommissioner setting attestationElements buffer size %d/%d", - static_cast(report.Get().attestationElements.size()), - static_cast(mParams.GetAttestationElements().Value().size())); - } - - if (report.Get().signature.size() > kAttestationSignatureLength) + memcpy(mAttestationElements, elements.data(), elements.size()); + mAttestationElementsLen = static_cast(elements.size()); + mParams.SetAttestationElements(ByteSpan(mAttestationElements, elements.size())); + ChipLogError(Controller, "AutoCommissioner setting attestationElements buffer size %u/%u", + static_cast(elements.size()), + static_cast(mParams.GetAttestationElements().Value().size())); + + if (signature.size() > sizeof(mAttestationSignature)) { ChipLogError(Controller, - "AutoCommissioner attestationSignature buffer size %d larger than " - "cache size %d", - static_cast(report.Get().signature.size()), - static_cast(kAttestationSignatureLength)); - mParams.SetAttestationSignature(report.Get().signature); - } - else - { - memcpy(mAttestationSignature, report.Get().signature.data(), - report.Get().signature.size()); - mAttestationSignatureLen = static_cast(report.Get().signature.size()); - mParams.SetAttestationSignature( - ByteSpan(mAttestationSignature, report.Get().signature.size())); + "AutoCommissioner attestationSignature buffer size %u larger than " + "cache size %u", + static_cast(signature.size()), static_cast(sizeof(mAttestationSignature))); + return CHIP_ERROR_MESSAGE_TOO_LONG; } + memcpy(mAttestationSignature, signature.data(), signature.size()); + mAttestationSignatureLen = static_cast(signature.size()); + mParams.SetAttestationSignature(ByteSpan(mAttestationSignature, signature.size())); // TODO: Does this need to be done at runtime? Seems like this could be done earlier and we wouldn't need to hold a // reference to the operational credential delegate here @@ -557,6 +547,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio mParams.SetCSRNonce(ByteSpan(mCSRNonce, sizeof(mCSRNonce))); } break; + } case CommissioningStage::kSendOpCertSigningRequest: { NOCChainGenerationParameters nocParams; nocParams.nocsrElements = report.Get().nocsrElements; diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 8c320d0be21a1f..d014af0b82af8d 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -18,14 +18,12 @@ #pragma once #include #include +#include #include namespace chip { namespace Controller { -constexpr size_t kAttestationElementsLength = 900; -constexpr size_t kAttestationSignatureLength = 64; - class DeviceCommissioner; class AutoCommissioner : public CommissioningDelegate @@ -98,9 +96,9 @@ class AutoCommissioner : public CommissioningDelegate uint8_t mICACertBuffer[Credentials::kMaxCHIPCertLength]; uint16_t mAttestationElementsLen = 0; - uint8_t mAttestationElements[kAttestationElementsLength]; + uint8_t mAttestationElements[Credentials::kMaxRspLen]; uint16_t mAttestationSignatureLen = 0; - uint8_t mAttestationSignature[kAttestationSignatureLength]; + uint8_t mAttestationSignature[Crypto::kMax_ECDSA_Signature_Length]; }; } // namespace Controller } // namespace chip diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 6ebefc4b5dffc9..1c0a80d43bf4ee 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1734,6 +1734,9 @@ void DeviceCommissioner::OnDone(app::ReadClient *) } }); + // Try to parse as much as we can here before returning, even if this is an error. + return_err = err == CHIP_NO_ERROR ? return_err : err; + err = mAttributeCache->ForEachAttribute( app::Clusters::OperationalCredentials::Id, [this, &info](const app::ConcreteAttributePath & path) { if (path.mAttributeId != app::Clusters::OperationalCredentials::Attributes::Fabrics::Id) @@ -1742,6 +1745,9 @@ void DeviceCommissioner::OnDone(app::ReadClient *) return CHIP_NO_ERROR; } + // this code is checking if the device is already on the commissioner's fabric. + // if a matching fabric is found, then remember the nodeId so that the commissioner + // can cancel commissioning (before it fails in AddNoc) and know it's nodeId. switch (path.mAttributeId) { case app::Clusters::OperationalCredentials::Attributes::Fabrics::Id: { diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 40fa72f05d3a70..c1e23c544a8fc5 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -177,13 +177,11 @@ class CommissioningParameters // Attestation elements from the node. These are obtained from node in response to the AttestationRequest command. In the // AutoCommissioner, this is automatically set from the report from the kSendAttestationRequest stage. // This must be set before calling PerformCommissioningStep for the kAttestationVerification step. - // Warning: data is not deep copied to local memory and can only be read during PerformCommissioningStep. const Optional GetAttestationElements() const { return mAttestationElements; } // Attestation signature from the node. This is obtained from node in response to the AttestationRequest command. In the // AutoCommissioner, this is automatically set from the report from the kSendAttestationRequest stage. // This must be set before calling PerformCommissioningStep for the kAttestationVerification step. - // Warning: data is not deep copied to local memory and can only be read during PerformCommissioningStep. const Optional GetAttestationSignature() const { return mAttestationSignature; } // Product attestation intermediate certificate from the node. This is obtained from the node in response to the diff --git a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp index c552b3c53c5d64..73c0a3f93182e8 100644 --- a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp +++ b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp @@ -225,9 +225,8 @@ CHIP_ERROR AndroidOperationalCredentialsIssuer::CallbackGenerateNOCChain(const B jbyteArray javaAttestationElements; JniReferences::GetInstance().N2J_ByteArray(env, attestationElements.data(), attestationElements.size(), javaAttestationElements); - // new log message below - ChipLogError(Controller, "AndroidOpCredsIssuer attestationElements size %d/%d", static_cast(attestationElements.size()), - static_cast(env->GetArrayLength(javaAttestationElements))); + ChipLogProgress(Controller, "AndroidOpCredsIssuer attestationElements size %d/%d", static_cast(attestationElements.size()), + static_cast(env->GetArrayLength(javaAttestationElements))); const ByteSpan & attestationNonce = mAutoCommissioner->GetAttestationNonce(); jbyteArray javaAttestationNonce; diff --git a/src/credentials/DeviceAttestationConstructor.h b/src/credentials/DeviceAttestationConstructor.h index 87f1095792c0f9..b8404ef57577cc 100644 --- a/src/credentials/DeviceAttestationConstructor.h +++ b/src/credentials/DeviceAttestationConstructor.h @@ -23,6 +23,9 @@ namespace chip { namespace Credentials { +// As per specifications section 11.22.5.1. Constant RESP_MAX +constexpr size_t kMaxRspLen = 900; + // CSRNonce and AttestationNonce need to be this size constexpr size_t kExpectedAttestationNonceSize = 32; From abe2d30316afb4edd8c7bf9d750415f63845716f Mon Sep 17 00:00:00 2001 From: chrisdecenzo Date: Wed, 7 Sep 2022 09:52:21 -0700 Subject: [PATCH 5/8] address comments --- examples/platform/linux/AppMain.cpp | 3 +- examples/platform/linux/CommissionerMain.cpp | 56 ++++--------------- examples/platform/linux/CommissionerMain.h | 2 +- examples/platform/linux/Options.cpp | 10 ++++ examples/platform/linux/Options.h | 1 + src/app/app-platform/ContentAppPlatform.cpp | 9 +++ src/app/app-platform/ContentAppPlatform.h | 5 ++ .../operational-credentials-server.cpp | 6 +- src/controller/CHIPDeviceController.cpp | 2 +- .../AndroidOperationalCredentialsIssuer.cpp | 2 - .../DeviceAttestationConstructor.cpp | 4 ++ 11 files changed, 46 insertions(+), 54 deletions(-) diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp index 35f85dc3f016ca..303caf17c362ac 100644 --- a/examples/platform/linux/AppMain.cpp +++ b/examples/platform/linux/AppMain.cpp @@ -334,7 +334,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(); diff --git a/examples/platform/linux/CommissionerMain.cpp b/examples/platform/linux/CommissionerMain.cpp index ba0a05a02385c8..0816c5fcabc211 100644 --- a/examples/platform/linux/CommissionerMain.cpp +++ b/examples/platform/linux/CommissionerMain.cpp @@ -76,7 +76,7 @@ using namespace chip::app::Clusters; using namespace ::chip::Messaging; using namespace ::chip::Controller; -class CustomServerStorageDelegate : public PersistentStorageDelegate +class MyServerStorageDelegate : public PersistentStorageDelegate { CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override { @@ -105,7 +105,7 @@ class CustomServerStorageDelegate : public PersistentStorageDelegate } }; -class CustomCommissionerCallback : public CommissionerCallback +class MyCommissionerCallback : public CommissionerCallback { void ReadyForCommissioning(uint32_t pincode, uint16_t longDiscriminator, PeerAddress peerAddress) override { @@ -115,49 +115,15 @@ class CustomCommissionerCallback : public CommissionerCallback AutoCommissioner gAutoCommissioner; -class CustomCredsIssuer : public ExampleOperationalCredentialsIssuer -{ - CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & csrNonce, const ByteSpan & attestationSignature, - const ByteSpan & attestationChallenge, const ByteSpan & DAC, const ByteSpan & PAI, - Callback::Callback * onCompletion) override - { - // add parsing here - const ByteSpan & attestationElements = gAutoCommissioner.GetCommissioningParameters().GetAttestationElements().Value(); - ChipLogError(Controller, "CustomCredsIssuer attestationElements size %d", static_cast(attestationElements.size())); - - ByteSpan certificationDeclarationSpan; - ByteSpan attestationNonceSpan; - uint32_t timestampDeconstructed; - ByteSpan firmwareInfoSpan; - DeviceAttestationVendorReservedDeconstructor vendorReserved; - - CHIP_ERROR err = DeconstructAttestationElements(attestationElements, certificationDeclarationSpan, attestationNonceSpan, - timestampDeconstructed, firmwareInfoSpan, vendorReserved); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Controller, "Failed to parse attestation elements"); - } - else - { - ChipLogError(Controller, "Parse attestation elements success"); - } - - return ExampleOperationalCredentialsIssuer::GenerateNOCChain(csrElements, csrNonce, attestationSignature, - attestationChallenge, DAC, PAI, onCompletion); - } -}; - DeviceCommissioner gCommissioner; CommissionerDiscoveryController gCommissionerDiscoveryController; -CustomCommissionerCallback gCommissionerCallback; -CustomServerStorageDelegate gServerStorage; -CustomCredsIssuer gOpCredsIssuer; +MyCommissionerCallback gCommissionerCallback; +MyServerStorageDelegate gServerStorage; +ExampleOperationalCredentialsIssuer gOpCredsIssuer; NodeId gLocalId = kMaxOperationalNodeId; Credentials::GroupDataProviderImpl gGroupDataProvider; -Credentials::PartialDACVerifier gPartialDACVerifier; -FabricId gFabricId = 1; -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; @@ -178,7 +144,10 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort) params.controllerVendorId = static_cast(vendorId); ReturnErrorOnFailure(gOpCredsIssuer.Initialize(gServerStorage)); - gOpCredsIssuer.SetFabricIdForNextNOCRequest(gFabricId); + 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); @@ -189,9 +158,6 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort) const Credentials::AttestationTrustStore * testingRootStore = Credentials::GetTestAttestationTrustStore(); SetDeviceAttestationVerifier(GetDefaultDACVerifier(testingRootStore)); - // Uncomment the following line to perform DAC verification during NOC chain generation - // SetDeviceAttestationVerifier(&gPartialDACVerifier); - Platform::ScopedMemoryBuffer noc; VerifyOrReturnError(noc.Alloc(Controller::kMaxCHIPDERCertLength), CHIP_ERROR_NO_MEMORY); MutableByteSpan nocSpan(noc.Get(), Controller::kMaxCHIPDERCertLength); @@ -245,7 +211,7 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort) ChipLogProgress(Support, "InitCommissioner nodeId=0x" ChipLogFormatX64 " fabric.fabricId=0x" ChipLogFormatX64 " fabricIndex=0x%x", - ChipLogValueX64(gCommissioner.GetNodeId()), ChipLogValueX64(gFabricId), static_cast(fabricIndex)); + ChipLogValueX64(gCommissioner.GetNodeId()), ChipLogValueX64(fabricId), static_cast(fabricIndex)); return CHIP_NO_ERROR; } diff --git a/examples/platform/linux/CommissionerMain.h b/examples/platform/linux/CommissionerMain.h index 679fe7c847fec6..75ddf5debe852e 100644 --- a/examples/platform/linux/CommissionerMain.h +++ b/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(); diff --git a/examples/platform/linux/Options.cpp b/examples/platform/linux/Options.cpp index 73fe3568bdcda0..f7ca12dd981698 100644 --- a/examples/platform/linux/Options.cpp +++ b/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 \n" " A 16-bit unsigned integer specifying the port to use for unsecured commissioner messages (default is 5550).\n" "\n" + " --commissioner-fabric-id \n" + " The fabric ID to be used when this device is a commissioner (default in code is 1).\n" + "\n" " --command \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); diff --git a/examples/platform/linux/Options.h b/examples/platform/linux/Options.h index 00817f95a16378..7f2a5f258d2d69 100644 --- a/examples/platform/linux/Options.h +++ b/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(); }; diff --git a/src/app/app-platform/ContentAppPlatform.cpp b/src/app/app-platform/ContentAppPlatform.cpp index 540bfe2b77c77e..014dec90fc7492 100644 --- a/src/app/app-platform/ContentAppPlatform.cpp +++ b/src/app/app-platform/ContentAppPlatform.cpp @@ -527,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, @@ -575,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"); diff --git a/src/app/app-platform/ContentAppPlatform.h b/src/app/app-platform/ContentAppPlatform.h index 4fce0f668da36a..3744a0fa0edf1d 100644 --- a/src/app/app-platform/ContentAppPlatform.h +++ b/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. diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 610277e2ba0c75..c012ed42d89d32 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -958,8 +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() <= Credentials::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()); @@ -1090,8 +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() <= Credentials::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()); diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 1c0a80d43bf4ee..6d887c7f2db0d6 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1735,7 +1735,7 @@ void DeviceCommissioner::OnDone(app::ReadClient *) }); // Try to parse as much as we can here before returning, even if this is an error. - return_err = err == CHIP_NO_ERROR ? return_err : err; + return_err = (err == CHIP_NO_ERROR) ? return_err : err; err = mAttributeCache->ForEachAttribute( app::Clusters::OperationalCredentials::Id, [this, &info](const app::ConcreteAttributePath & path) { diff --git a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp index 73c0a3f93182e8..76d5dcb1895d77 100644 --- a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp +++ b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp @@ -225,8 +225,6 @@ CHIP_ERROR AndroidOperationalCredentialsIssuer::CallbackGenerateNOCChain(const B jbyteArray javaAttestationElements; JniReferences::GetInstance().N2J_ByteArray(env, attestationElements.data(), attestationElements.size(), javaAttestationElements); - ChipLogProgress(Controller, "AndroidOpCredsIssuer attestationElements size %d/%d", static_cast(attestationElements.size()), - static_cast(env->GetArrayLength(javaAttestationElements))); const ByteSpan & attestationNonce = mAutoCommissioner->GetAttestationNonce(); jbyteArray javaAttestationNonce; diff --git a/src/credentials/DeviceAttestationConstructor.cpp b/src/credentials/DeviceAttestationConstructor.cpp index 8a13419bf5f699..6056ed1a0f9fb7 100644 --- a/src/credentials/DeviceAttestationConstructor.cpp +++ b/src/credentials/DeviceAttestationConstructor.cpp @@ -183,6 +183,8 @@ CHIP_ERROR ConstructAttestationElements(const ByteSpan & certificationDeclaratio ReturnErrorOnFailure(tlvWriter.Finalize()); attestationElements = attestationElements.SubSpan(0, tlvWriter.GetLengthWritten()); + VerifyOrReturnError(attestationElements.size() <= Credentials::kMaxRspLen, CHIP_ERROR_MESSAGE_TOO_LONG); + return CHIP_NO_ERROR; } @@ -218,6 +220,8 @@ CHIP_ERROR ConstructNOCSRElements(const ByteSpan & csr, const ByteSpan & csrNonc ReturnErrorOnFailure(tlvWriter.Finalize()); nocsrElements = nocsrElements.SubSpan(0, tlvWriter.GetLengthWritten()); + VerifyOrReturnError(nocsrElements.size() <= Credentials::kMaxRspLen, CHIP_ERROR_MESSAGE_TOO_LONG); + return CHIP_NO_ERROR; } From 31f2dc8b7876889d5e739617f2d5e2a452382b8d Mon Sep 17 00:00:00 2001 From: chrisdecenzo Date: Fri, 9 Sep 2022 12:57:38 -0700 Subject: [PATCH 6/8] address comments --- examples/platform/linux/CommissionerMain.cpp | 5 +++ src/controller/AutoCommissioner.cpp | 6 +-- src/controller/AutoCommissioner.h | 2 +- src/controller/CHIPDeviceController.cpp | 47 ++++++++++++-------- src/controller/CommissioningDelegate.h | 11 +++++ 5 files changed, 49 insertions(+), 22 deletions(-) diff --git a/examples/platform/linux/CommissionerMain.cpp b/examples/platform/linux/CommissionerMain.cpp index 0816c5fcabc211..2d5cad2564f757 100644 --- a/examples/platform/linux/CommissionerMain.cpp +++ b/examples/platform/linux/CommissionerMain.cpp @@ -183,6 +183,11 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort, F 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)); diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 943f1687a0b32c..a51f3573f85786 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -522,9 +522,9 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio memcpy(mAttestationElements, elements.data(), elements.size()); mAttestationElementsLen = static_cast(elements.size()); mParams.SetAttestationElements(ByteSpan(mAttestationElements, elements.size())); - ChipLogError(Controller, "AutoCommissioner setting attestationElements buffer size %u/%u", - static_cast(elements.size()), - static_cast(mParams.GetAttestationElements().Value().size())); + ChipLogDetail(Controller, "AutoCommissioner setting attestationElements buffer size %u/%u", + static_cast(elements.size()), + static_cast(mParams.GetAttestationElements().Value().size())); if (signature.size() > sizeof(mAttestationSignature)) { diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index d014af0b82af8d..c7d16f15e64130 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -42,7 +42,7 @@ class AutoCommissioner : public CommissioningDelegate ByteSpan GetAttestationElements() const { return ByteSpan(mAttestationElements, mAttestationElementsLen); } ByteSpan GetAttestationSignature() const { return ByteSpan(mAttestationSignature, mAttestationSignatureLen); } - ByteSpan GetAttestationNonce() const { return ByteSpan(mAttestationNonce, sizeof(mAttestationNonce)); } + ByteSpan GetAttestationNonce() const { return ByteSpan(mAttestationNonce); } protected: CommissioningStage GetNextCommissioningStage(CommissioningStage currentStage, CHIP_ERROR & lastErr); diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 6d887c7f2db0d6..f7739dca742dd4 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1737,24 +1737,18 @@ void DeviceCommissioner::OnDone(app::ReadClient *) // Try to parse as much as we can here before returning, even if this is an error. return_err = (err == CHIP_NO_ERROR) ? return_err : err; - err = mAttributeCache->ForEachAttribute( - app::Clusters::OperationalCredentials::Id, [this, &info](const app::ConcreteAttributePath & path) { - if (path.mAttributeId != app::Clusters::OperationalCredentials::Attributes::Fabrics::Id) - { - // Continue on - return CHIP_NO_ERROR; - } - + err = + mAttributeCache->ForEachAttribute(OperationalCredentials::Id, [this, &info](const app::ConcreteAttributePath & path) { // this code is checking if the device is already on the commissioner's fabric. // if a matching fabric is found, then remember the nodeId so that the commissioner - // can cancel commissioning (before it fails in AddNoc) and know it's nodeId. + // can, if it decides to, cancel commissioning (before it fails in AddNoc) and know + // the device's nodeId on its fabric. switch (path.mAttributeId) { - case app::Clusters::OperationalCredentials::Attributes::Fabrics::Id: { - app::Clusters::OperationalCredentials::Attributes::Fabrics::TypeInfo::DecodableType fabrics; + case OperationalCredentials::Attributes::Fabrics::Id: { + OperationalCredentials::Attributes::Fabrics::TypeInfo::DecodableType fabrics; ReturnErrorOnFailure( - this->mAttributeCache->Get(path, - fabrics)); + this->mAttributeCache->Get(path, fabrics)); auto iter = fabrics.begin(); while (iter.Next()) { @@ -1764,10 +1758,24 @@ void DeviceCommissioner::OnDone(app::ReadClient *) " fabric.nodeId=0x" ChipLogFormatX64, fabricDescriptor.vendorId, ChipLogValueX64(fabricDescriptor.fabricId), ChipLogValueX64(fabricDescriptor.nodeId)); + // need to add check for root public key if (GetFabricId() == fabricDescriptor.fabricId) { - ChipLogProgress(AppServer, "DeviceCommissioner::OnDone - found a matching fabric"); - info.nodeId = fabricDescriptor.nodeId; + ChipLogProgress(AppServer, "DeviceCommissioner::OnDone - found a matching fabric id"); + chip::ByteSpan rootKeySpan = fabricDescriptor.rootPublicKey; + P256PublicKeySpan rootPubKeySpan(rootKeySpan.data()); + Crypto::P256PublicKey deviceRootPublicKey(rootPubKeySpan); + + Crypto::P256PublicKey commissionerRootPublicKey; + if (CHIP_NO_ERROR != GetRootPublicKey(commissionerRootPublicKey)) + { + ChipLogError(AppServer, "DeviceCommissioner::OnDone - error reading commissioner root public key"); + } + else if (commissionerRootPublicKey.Matches(deviceRootPublicKey)) + { + ChipLogProgress(AppServer, "DeviceCommissioner::OnDone - fabric root keys match"); + info.nodeId = fabricDescriptor.nodeId; + } } } @@ -2042,6 +2050,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance(); app::ReadPrepareParams readParams(proxy->GetSecureSession().Value()); + // NOTE: this array cannot have more than 9 entries, since 9 is what the spec requires as a minimum on servers app::AttributePathParams readPaths[9]; // Read all the feature maps for all the networking clusters on any endpoint to determine what is supported readPaths[0] = app::AttributePathParams(app::Clusters::NetworkCommissioning::Id, @@ -2064,11 +2073,13 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio readPaths[7] = app::AttributePathParams(app::Clusters::NetworkCommissioning::Id, app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::Id); // Read the current fabrics - readPaths[8] = app::AttributePathParams(app::Clusters::OperationalCredentials::Id, - app::Clusters::OperationalCredentials::Attributes::Fabrics::Id); + if (params.GetCheckForMatchingFabric().ValueOr(false)) + { + readPaths[8] = app::AttributePathParams(OperationalCredentials::Id, OperationalCredentials::Attributes::Fabrics::Id); + } readParams.mpAttributePathParamsList = readPaths; - readParams.mAttributePathParamsListSize = 9; + readParams.mAttributePathParamsListSize = 8 + (params.GetCheckForMatchingFabric().ValueOr(false) ? 1 : 0); readParams.mIsFabricFiltered = false; if (timeout.HasValue()) { diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index c1e23c544a8fc5..adf9ee978551cd 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -396,6 +396,16 @@ class CommissioningParameters return *this; } + // Check for matching fabric on target device by reading fabric list and looking for a + // fabricId and RootCert match. If a match is detected, then use GetNodeId() to + // access the nodeId for the device on the matching fabric. + Optional GetCheckForMatchingFabric() const { return mCheckForMatchingFabric; } + CommissioningParameters & SetCheckForMatchingFabric(bool checkForMatchingFabric) + { + mCheckForMatchingFabric = MakeOptional(checkForMatchingFabric); + return *this; + } + private: // Items that can be set by the commissioner Optional mFailsafeTimerSeconds; @@ -427,6 +437,7 @@ class CommissioningParameters Optional mAttemptWiFiNetworkScan; Optional mAttemptThreadNetworkScan; // This automatically gets set to false when a ThreadOperationalDataset is set Optional mSkipCommissioningComplete; + Optional mCheckForMatchingFabric; }; struct RequestedCertificate From 6a625add8733bea6971043c36040a377bcb0ffc3 Mon Sep 17 00:00:00 2001 From: "restyled-io[bot]" <32688539+restyled-io[bot]@users.noreply.github.com> Date: Sat, 10 Sep 2022 15:19:35 -0700 Subject: [PATCH 7/8] Restyled by clang-format (#22524) Co-authored-by: Restyled.io --- src/controller/CHIPDeviceController.cpp | 82 ++++++++++++------------- 1 file changed, 40 insertions(+), 42 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index f7739dca742dd4..f1c600772e4c34 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1737,54 +1737,52 @@ void DeviceCommissioner::OnDone(app::ReadClient *) // Try to parse as much as we can here before returning, even if this is an error. return_err = (err == CHIP_NO_ERROR) ? return_err : err; - err = - mAttributeCache->ForEachAttribute(OperationalCredentials::Id, [this, &info](const app::ConcreteAttributePath & path) { - // this code is checking if the device is already on the commissioner's fabric. - // if a matching fabric is found, then remember the nodeId so that the commissioner - // can, if it decides to, cancel commissioning (before it fails in AddNoc) and know - // the device's nodeId on its fabric. - switch (path.mAttributeId) + err = mAttributeCache->ForEachAttribute(OperationalCredentials::Id, [this, &info](const app::ConcreteAttributePath & path) { + // this code is checking if the device is already on the commissioner's fabric. + // if a matching fabric is found, then remember the nodeId so that the commissioner + // can, if it decides to, cancel commissioning (before it fails in AddNoc) and know + // the device's nodeId on its fabric. + switch (path.mAttributeId) + { + case OperationalCredentials::Attributes::Fabrics::Id: { + OperationalCredentials::Attributes::Fabrics::TypeInfo::DecodableType fabrics; + ReturnErrorOnFailure(this->mAttributeCache->Get(path, fabrics)); + auto iter = fabrics.begin(); + while (iter.Next()) { - case OperationalCredentials::Attributes::Fabrics::Id: { - OperationalCredentials::Attributes::Fabrics::TypeInfo::DecodableType fabrics; - ReturnErrorOnFailure( - this->mAttributeCache->Get(path, fabrics)); - auto iter = fabrics.begin(); - while (iter.Next()) + auto & fabricDescriptor = iter.GetValue(); + ChipLogProgress(AppServer, + "DeviceCommissioner::OnDone - fabric.vendorId=0x%04X fabric.fabricId=0x" ChipLogFormatX64 + " fabric.nodeId=0x" ChipLogFormatX64, + fabricDescriptor.vendorId, ChipLogValueX64(fabricDescriptor.fabricId), + ChipLogValueX64(fabricDescriptor.nodeId)); + // need to add check for root public key + if (GetFabricId() == fabricDescriptor.fabricId) { - auto & fabricDescriptor = iter.GetValue(); - ChipLogProgress(AppServer, - "DeviceCommissioner::OnDone - fabric.vendorId=0x%04X fabric.fabricId=0x" ChipLogFormatX64 - " fabric.nodeId=0x" ChipLogFormatX64, - fabricDescriptor.vendorId, ChipLogValueX64(fabricDescriptor.fabricId), - ChipLogValueX64(fabricDescriptor.nodeId)); - // need to add check for root public key - if (GetFabricId() == fabricDescriptor.fabricId) - { - ChipLogProgress(AppServer, "DeviceCommissioner::OnDone - found a matching fabric id"); - chip::ByteSpan rootKeySpan = fabricDescriptor.rootPublicKey; - P256PublicKeySpan rootPubKeySpan(rootKeySpan.data()); - Crypto::P256PublicKey deviceRootPublicKey(rootPubKeySpan); + ChipLogProgress(AppServer, "DeviceCommissioner::OnDone - found a matching fabric id"); + chip::ByteSpan rootKeySpan = fabricDescriptor.rootPublicKey; + P256PublicKeySpan rootPubKeySpan(rootKeySpan.data()); + Crypto::P256PublicKey deviceRootPublicKey(rootPubKeySpan); - Crypto::P256PublicKey commissionerRootPublicKey; - if (CHIP_NO_ERROR != GetRootPublicKey(commissionerRootPublicKey)) - { - ChipLogError(AppServer, "DeviceCommissioner::OnDone - error reading commissioner root public key"); - } - else if (commissionerRootPublicKey.Matches(deviceRootPublicKey)) - { - ChipLogProgress(AppServer, "DeviceCommissioner::OnDone - fabric root keys match"); - info.nodeId = fabricDescriptor.nodeId; - } + Crypto::P256PublicKey commissionerRootPublicKey; + if (CHIP_NO_ERROR != GetRootPublicKey(commissionerRootPublicKey)) + { + ChipLogError(AppServer, "DeviceCommissioner::OnDone - error reading commissioner root public key"); + } + else if (commissionerRootPublicKey.Matches(deviceRootPublicKey)) + { + ChipLogProgress(AppServer, "DeviceCommissioner::OnDone - fabric root keys match"); + info.nodeId = fabricDescriptor.nodeId; } } - - return CHIP_NO_ERROR; - } - default: - return CHIP_NO_ERROR; } - }); + + return CHIP_NO_ERROR; + } + default: + return CHIP_NO_ERROR; + } + }); // Try to parse as much as we can here before returning, even if this is an error. return_err = err == CHIP_NO_ERROR ? return_err : err; From d55de3e3ade4109aa87f30cc78115e4e5a0adaec Mon Sep 17 00:00:00 2001 From: chrisdecenzo Date: Wed, 14 Sep 2022 08:33:24 -0700 Subject: [PATCH 8/8] address comments --- src/controller/CHIPDeviceController.cpp | 26 +++++++++++++++++-------- src/controller/CommissioningDelegate.h | 6 +++--- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index f7739dca742dd4..0f7580c2531801 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1749,31 +1749,38 @@ void DeviceCommissioner::OnDone(app::ReadClient *) OperationalCredentials::Attributes::Fabrics::TypeInfo::DecodableType fabrics; ReturnErrorOnFailure( this->mAttributeCache->Get(path, fabrics)); + // this is a best effort attempt to find a matching fabric, so no error checking on iter auto iter = fabrics.begin(); while (iter.Next()) { auto & fabricDescriptor = iter.GetValue(); - ChipLogProgress(AppServer, + ChipLogProgress(Controller, "DeviceCommissioner::OnDone - fabric.vendorId=0x%04X fabric.fabricId=0x" ChipLogFormatX64 " fabric.nodeId=0x" ChipLogFormatX64, fabricDescriptor.vendorId, ChipLogValueX64(fabricDescriptor.fabricId), ChipLogValueX64(fabricDescriptor.nodeId)); - // need to add check for root public key if (GetFabricId() == fabricDescriptor.fabricId) { - ChipLogProgress(AppServer, "DeviceCommissioner::OnDone - found a matching fabric id"); + ChipLogProgress(Controller, "DeviceCommissioner::OnDone - found a matching fabric id"); chip::ByteSpan rootKeySpan = fabricDescriptor.rootPublicKey; + if (rootKeySpan.size() != Crypto::kP256_PublicKey_Length) + { + ChipLogError(Controller, "DeviceCommissioner::OnDone - fabric root key size mismatch %u != %u", + static_cast(rootKeySpan.size()), + static_cast(Crypto::kP256_PublicKey_Length)); + continue; + } P256PublicKeySpan rootPubKeySpan(rootKeySpan.data()); Crypto::P256PublicKey deviceRootPublicKey(rootPubKeySpan); Crypto::P256PublicKey commissionerRootPublicKey; if (CHIP_NO_ERROR != GetRootPublicKey(commissionerRootPublicKey)) { - ChipLogError(AppServer, "DeviceCommissioner::OnDone - error reading commissioner root public key"); + ChipLogError(Controller, "DeviceCommissioner::OnDone - error reading commissioner root public key"); } else if (commissionerRootPublicKey.Matches(deviceRootPublicKey)) { - ChipLogProgress(AppServer, "DeviceCommissioner::OnDone - fabric root keys match"); + ChipLogProgress(Controller, "DeviceCommissioner::OnDone - fabric root keys match"); info.nodeId = fabricDescriptor.nodeId; } } @@ -2072,14 +2079,17 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio // Read the requested minimum connection times from all network commissioning clusters readPaths[7] = app::AttributePathParams(app::Clusters::NetworkCommissioning::Id, app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::Id); + + readParams.mpAttributePathParamsList = readPaths; + readParams.mAttributePathParamsListSize = 8; + // Read the current fabrics - if (params.GetCheckForMatchingFabric().ValueOr(false)) + if (params.GetCheckForMatchingFabric()) { + readParams.mAttributePathParamsListSize = 9; readPaths[8] = app::AttributePathParams(OperationalCredentials::Id, OperationalCredentials::Attributes::Fabrics::Id); } - readParams.mpAttributePathParamsList = readPaths; - readParams.mAttributePathParamsListSize = 8 + (params.GetCheckForMatchingFabric().ValueOr(false) ? 1 : 0); readParams.mIsFabricFiltered = false; if (timeout.HasValue()) { diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index adf9ee978551cd..355d740a0c31df 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -399,10 +399,10 @@ class CommissioningParameters // Check for matching fabric on target device by reading fabric list and looking for a // fabricId and RootCert match. If a match is detected, then use GetNodeId() to // access the nodeId for the device on the matching fabric. - Optional GetCheckForMatchingFabric() const { return mCheckForMatchingFabric; } + bool GetCheckForMatchingFabric() const { return mCheckForMatchingFabric; } CommissioningParameters & SetCheckForMatchingFabric(bool checkForMatchingFabric) { - mCheckForMatchingFabric = MakeOptional(checkForMatchingFabric); + mCheckForMatchingFabric = checkForMatchingFabric; return *this; } @@ -437,7 +437,7 @@ class CommissioningParameters Optional mAttemptWiFiNetworkScan; Optional mAttemptThreadNetworkScan; // This automatically gets set to false when a ThreadOperationalDataset is set Optional mSkipCommissioningComplete; - Optional mCheckForMatchingFabric; + bool mCheckForMatchingFabric = false; }; struct RequestedCertificate