From 44df9067698fdc5aace88ab90da4626f93e4b39d Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 16 Jun 2023 17:08:02 -0400 Subject: [PATCH] Ensure that auto commissioner gets the attribute values it expects. If a device failed to, for example, send us an actual value for the VendorID attribute, we would silently end up using some random value for it during commissioning, probably right up until device attestation failed. Instead of relying on that, we should just explicitly error out if the attributes that must be present (VendorID, ProductID, BasicCommissioningInfo, etc) don't exist. --- src/app/ClusterStateCache.h | 11 +++ src/controller/CHIPDeviceController.cpp | 126 ++++++++++-------------- 2 files changed, 63 insertions(+), 74 deletions(-) diff --git a/src/app/ClusterStateCache.h b/src/app/ClusterStateCache.h index b6fb2029c85b49..e773dcb4e55ea6 100644 --- a/src/app/ClusterStateCache.h +++ b/src/app/ClusterStateCache.h @@ -152,6 +152,17 @@ class ClusterStateCache : protected ReadClient::Callback return DataModel::Decode(reader, value); } + /** + * Get the value of a particular attribute for the given endpoint. See the + * documentation for Get() with a ConcreteAttributePath above. + */ + template + CHIP_ERROR Get(EndpointId endpoint, typename AttributeObjectTypeT::DecodableType & value) const + { + ConcreteAttributePath path(endpoint, AttributeObjectTypeT::GetClusterId(), AttributeObjectTypeT::GetAttributeId()); + return Get(path, value); + } + /* * Retrieve the StatusIB for a given attribute if one exists currently in the cache. * diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index acb524769ed37a..7511409c84eb67 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1891,82 +1891,57 @@ void DeviceCommissioner::OnDone(app::ReadClient *) CHIP_ERROR return_err = CHIP_NO_ERROR; ReadCommissioningInfo info; - // Using ForEachAttribute because this attribute can be queried on any endpoint. - err = mAttributeCache->ForEachAttribute( - app::Clusters::GeneralCommissioning::Id, [this, &info](const app::ConcreteAttributePath & path) { - switch (path.mAttributeId) - { - case app::Clusters::GeneralCommissioning::Attributes::BasicCommissioningInfo::Id: { - app::Clusters::GeneralCommissioning::Attributes::BasicCommissioningInfo::TypeInfo::DecodableType basicInfo; - ReturnErrorOnFailure( - this->mAttributeCache->Get( - path, basicInfo)); - info.general.recommendedFailsafe = basicInfo.failSafeExpiryLengthSeconds; - } - break; - case app::Clusters::GeneralCommissioning::Attributes::RegulatoryConfig::Id: { - ReturnErrorOnFailure( - this->mAttributeCache->Get( - path, info.general.currentRegulatoryLocation)); - } - break; - case app::Clusters::GeneralCommissioning::Attributes::LocationCapability::Id: { - ReturnErrorOnFailure( - this->mAttributeCache->Get( - path, info.general.locationCapability)); - } - break; - case app::Clusters::GeneralCommissioning::Attributes::Breadcrumb::Id: { - ReturnErrorOnFailure( - this->mAttributeCache->Get( - path, info.general.breadcrumb)); - } - break; - default: - return CHIP_NO_ERROR; - } + // Try to parse as much as we can here before returning, even if attributes + // are missing or cannot be decoded. + { + using namespace chip::app::Clusters::GeneralCommissioning; + using namespace chip::app::Clusters::GeneralCommissioning::Attributes; - return CHIP_NO_ERROR; - }); + BasicCommissioningInfo::TypeInfo::DecodableType basicInfo; + err = mAttributeCache->Get(kRootEndpointId, basicInfo); + if (err == CHIP_NO_ERROR) + { + info.general.recommendedFailsafe = basicInfo.failSafeExpiryLengthSeconds; + } + else + { + return_err = err; + } - // 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->Get(kRootEndpointId, info.general.currentRegulatoryLocation); + return_err = err == CHIP_NO_ERROR ? return_err : err; - err = mAttributeCache->ForEachAttribute( - app::Clusters::BasicInformation::Id, [this, &info](const app::ConcreteAttributePath & path) { - if (path.mAttributeId != app::Clusters::BasicInformation::Attributes::VendorID::Id && - path.mAttributeId != app::Clusters::BasicInformation::Attributes::ProductID::Id) - { - // Continue on - return CHIP_NO_ERROR; - } + err = mAttributeCache->Get(kRootEndpointId, info.general.locationCapability); + return_err = err == CHIP_NO_ERROR ? return_err : err; - switch (path.mAttributeId) - { - case app::Clusters::BasicInformation::Attributes::VendorID::Id: - return this->mAttributeCache->Get( - path, info.basic.vendorId); - case app::Clusters::BasicInformation::Attributes::ProductID::Id: - return this->mAttributeCache->Get( - path, info.basic.productId); - default: - return CHIP_NO_ERROR; - } - }); + err = mAttributeCache->Get(kRootEndpointId, info.general.breadcrumb); + return_err = err == CHIP_NO_ERROR ? return_err : err; + } - // 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; + { + using namespace chip::app::Clusters::BasicInformation; + using namespace chip::app::Clusters::BasicInformation::Attributes; + + err = mAttributeCache->Get(kRootEndpointId, info.basic.vendorId); + return_err = err == CHIP_NO_ERROR ? return_err : err; + + err = mAttributeCache->Get(kRootEndpointId, info.basic.productId); + return_err = err == CHIP_NO_ERROR ? return_err : err; + } + // We might not have requested a Fabrics attribute at all, so not having a + // value for it is not an error. err = mAttributeCache->ForEachAttribute(OperationalCredentials::Id, [this, &info](const app::ConcreteAttributePath & path) { + using namespace chip::app::Clusters::OperationalCredentials::Attributes; // 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)); + case Fabrics::Id: { + 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()) @@ -2014,32 +1989,36 @@ 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; - // Set the network cluster endpoints first so we can match up the connection times. + // Set the network cluster endpoints first so we can match up the connection + // times. Note that here we don't know what endpoints the network + // commissioning clusters might be on. err = mAttributeCache->ForEachAttribute( app::Clusters::NetworkCommissioning::Id, [this, &info](const app::ConcreteAttributePath & path) { - if (path.mAttributeId != app::Clusters::NetworkCommissioning::Attributes::FeatureMap::Id) + using namespace chip::app::Clusters::NetworkCommissioning; + using namespace chip::app::Clusters::NetworkCommissioning::Attributes; + if (path.mAttributeId != FeatureMap::Id) { return CHIP_NO_ERROR; } TLV::TLVReader reader; if (this->mAttributeCache->Get(path, reader) == CHIP_NO_ERROR) { - BitFlags features; + BitFlags features; if (app::DataModel::Decode(reader, features) == CHIP_NO_ERROR) { - if (features.Has(app::Clusters::NetworkCommissioning::Feature::kWiFiNetworkInterface)) + if (features.Has(Feature::kWiFiNetworkInterface)) { ChipLogProgress(Controller, "----- NetworkCommissioning Features: has WiFi. endpointid = %u", path.mEndpointId); info.network.wifi.endpoint = path.mEndpointId; } - else if (features.Has(app::Clusters::NetworkCommissioning::Feature::kThreadNetworkInterface)) + else if (features.Has(Feature::kThreadNetworkInterface)) { ChipLogProgress(Controller, "----- NetworkCommissioning Features: has Thread. endpointid = %u", path.mEndpointId); info.network.thread.endpoint = path.mEndpointId; } - else if (features.Has(app::Clusters::NetworkCommissioning::Feature::kEthernetNetworkInterface)) + else if (features.Has(Feature::kEthernetNetworkInterface)) { ChipLogProgress(Controller, "----- NetworkCommissioning Features: has Ethernet. endpointid = %u", path.mEndpointId); @@ -2066,14 +2045,13 @@ void DeviceCommissioner::OnDone(app::ReadClient *) err = mAttributeCache->ForEachAttribute( app::Clusters::NetworkCommissioning::Id, [this, &info](const app::ConcreteAttributePath & path) { - if (path.mAttributeId != app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::Id) + using namespace chip::app::Clusters::NetworkCommissioning::Attributes; + if (path.mAttributeId != ConnectMaxTimeSeconds::Id) { return CHIP_NO_ERROR; } - app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::TypeInfo::DecodableArgType time; - ReturnErrorOnFailure( - this->mAttributeCache->Get(path, - time)); + ConnectMaxTimeSeconds::TypeInfo::DecodableArgType time; + ReturnErrorOnFailure(this->mAttributeCache->Get(path, time)); if (path.mEndpointId == info.network.wifi.endpoint) { info.network.wifi.minConnectionTime = time;