From 4800642114ac8e2ddbd5977d39814cdea1f01882 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 23 Jun 2023 06:44:44 -0400 Subject: [PATCH] Ensure that auto commissioner gets the attribute values it expects. (#27301) * 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. * Address review comments. --- src/app/ClusterStateCache.h | 11 ++ src/controller/CHIPDeviceController.cpp | 139 +++++++++++------------- 2 files changed, 76 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 e095201d37767c..83a77b7cdeb741 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1891,82 +1891,70 @@ 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 + { + ChipLogError(Controller, "Failed to read BasicCommissioningInfo: %" CHIP_ERROR_FORMAT, err.Format()); + 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); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Failed to read RegulatoryConfig: %" CHIP_ERROR_FORMAT, err.Format()); + 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); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Failed to read LocationCapability: %" CHIP_ERROR_FORMAT, err.Format()); + 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); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Failed to read Breadcrumb: %" CHIP_ERROR_FORMAT, err.Format()); + 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 +2002,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; + 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(NetworkCommissioning::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(NetworkCommissioning::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(NetworkCommissioning::Feature::kEthernetNetworkInterface)) { ChipLogProgress(Controller, "----- NetworkCommissioning Features: has Ethernet. endpointid = %u", path.mEndpointId); @@ -2066,14 +2058,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;