From 1250869b4a251334fde77db42e027bb33703793d Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Sun, 17 Jul 2022 23:07:30 -0400 Subject: [PATCH 1/6] Fix status on fabric-scoped commnads in PASE --- src/app/CommandHandler.cpp | 15 ++++- src/app/data-model/Nullable.h | 2 +- .../templates/app/cluster-objects-src.zapt | 63 +++++++++++++++++++ .../templates/app/cluster-objects.zapt | 1 + src/credentials/FabricTable.h | 5 +- .../zap-generated/cluster-objects.cpp | 63 +++++++++++++++++++ .../zap-generated/cluster-objects.h | 1 + 7 files changed, 147 insertions(+), 3 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index cff9ae492cd4fa..db727c15563e57 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -315,6 +315,18 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand return AddStatus(concretePath, Protocols::InteractionModel::Status::NeedsTimedInteraction); } + if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId)) + { + // Fabric-scoped commands are not allowed before a specific accessing fabric is available. + // This is mostly just during a PASE session before AddNOC. + if (GetAccessingFabricIndex() == kUndefinedFabricIndex) + { + // TODO: when wildcard invokes are supported, discard a + // wildcard-expanded path instead of returning a status. + return AddStatus(concretePath, Protocols::InteractionModel::Status::UnsupportedAccess); + } + } + err = aCommandElement.GetFields(&commandDataReader); if (CHIP_END_OF_TLV == err) { @@ -680,4 +692,5 @@ CHIP_ERROR __attribute__((weak)) MatterPreCommandReceivedCallback(const chip::ap return CHIP_NO_ERROR; } void __attribute__((weak)) MatterPostCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, - const chip::Access::SubjectDescriptor & subjectDescriptor) {} + const chip::Access::SubjectDescriptor & subjectDescriptor) +{} diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 31513d1f8a8817..4e6eaa7270b4f9 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -79,7 +79,7 @@ struct Nullable : protected Optional return true; } - // The only fabric-scoped objects in the spec are events and structs inside lists, and neither one can be nullable. + // The only fabric-scoped objects in the spec are commands, events and structs inside lists, and neither one can be nullable. static constexpr bool kIsFabricScoped = false; bool operator==(const Nullable & other) const { return Optional::operator==(other); } diff --git a/src/app/zap-templates/templates/app/cluster-objects-src.zapt b/src/app/zap-templates/templates/app/cluster-objects-src.zapt index 1d1c070e9e2652..8ae7efca79c57d 100644 --- a/src/app/zap-templates/templates/app/cluster-objects-src.zapt +++ b/src/app/zap-templates/templates/app/cluster-objects-src.zapt @@ -173,5 +173,68 @@ bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand) return false; } +// TODO(#20811): Actually generate the following based on ZAP metadata +// See https://github.com/project-chip/zap/issues/609 +bool CommandIsFabricScoped(ClusterId aCluster, CommandId aCommand) +{ + // Maybe it would be smaller code to codegen a table and walk over it? + // Not sure. + switch (aCluster) + { + case Clusters::Groups::Id: { + switch (aCommand) + { + case Clusters::Groups::Commands::AddGroup::Id: + case Clusters::Groups::Commands::ViewGroup::Id: + case Clusters::Groups::Commands::GetGroupMembership::Id: + case Clusters::Groups::Commands::RemoveGroup::Id: + case Clusters::Groups::Commands::RemoveAllGroups::Id: + return true; + default: + return false; + } + } + case Clusters::GroupKeyManagement::Id: { + switch (aCommand) + { + case Clusters::GroupKeyManagement::Commands::KeySetWrite::Id: + case Clusters::GroupKeyManagement::Commands::KeySetRead::Id: + case Clusters::GroupKeyManagement::Commands::KeySetRemove::Id: + case Clusters::GroupKeyManagement::Commands::KeySetReadAllIndices::Id: + return true; + default: + return false; + } + } + case Clusters::GeneralCommissioning::Id: { + switch (aCommand) + { + case Clusters::GeneralCommissioning::Commands::CommissioningComplete::Id: + return true; + default: + return false; + } + } + case Clusters::Scenes::Id: { + // Entire cluster is fabric-scoped. + return true; + } + case Clusters::OperationalCredentials::Id: { + switch (aCommand) + { + case Clusters::OperationalCredentials::Commands::UpdateNOC::Id: + case Clusters::OperationalCredentials::Commands::UpdateFabricLabel::Id: + return true; + default: + return false; + } + } + default: + break; + } + + return false; +} + } // namespace app } // namespace chip diff --git a/src/app/zap-templates/templates/app/cluster-objects.zapt b/src/app/zap-templates/templates/app/cluster-objects.zapt index 3ba302d78804ce..93d7637b5005be 100644 --- a/src/app/zap-templates/templates/app/cluster-objects.zapt +++ b/src/app/zap-templates/templates/app/cluster-objects.zapt @@ -215,6 +215,7 @@ public: } // namespace Clusters bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand); +bool CommandIsFabricScoped(ClusterId aCluster, CommandId aCommand); } // namespace app } // namespace chip diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index 68f9f9c8e132a1..823e4e0fff3219 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -883,12 +883,15 @@ class DLL_EXPORT FabricTable CHIP_ERROR AddNewFabricForTest(const ByteSpan & rootCert, const ByteSpan & icacCert, const ByteSpan & nocCert, const ByteSpan & opKeySpan, FabricIndex * outFabricIndex); + // TODO: Add docs :D + void AllowCollisionsOnNextFabricAdd() { mStateFlags.Set(StateFlags::kAreCollidingFabricsIgnored); } + // Same as AddNewFabricForTest, but ignore if we are colliding with same , so // that a single fabric table can have N nodes for same fabric. This usually works, but is bad form. CHIP_ERROR AddNewFabricForTestIgnoringCollisions(const ByteSpan & rootCert, const ByteSpan & icacCert, const ByteSpan & nocCert, const ByteSpan & opKeySpan, FabricIndex * outFabricIndex) { - mStateFlags.Set(StateFlags::kAreCollidingFabricsIgnored); + AllowCollisionsOnNextFabricAdd(); CHIP_ERROR err = AddNewFabricForTest(rootCert, icacCert, nocCert, opKeySpan, outFabricIndex); mStateFlags.Clear(StateFlags::kAreCollidingFabricsIgnored); return err; diff --git a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp index e99aec2beec10f..6d5c4cb5cf654e 100644 --- a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp +++ b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp @@ -20695,5 +20695,68 @@ bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand) return false; } +// TODO(#20811): Actually generate the following based on ZAP metadata +// See https://github.com/project-chip/zap/issues/609 +bool CommandIsFabricScoped(ClusterId aCluster, CommandId aCommand) +{ + // Maybe it would be smaller code to codegen a table and walk over it? + // Not sure. + switch (aCluster) + { + case Clusters::Groups::Id: { + switch (aCommand) + { + case Clusters::Groups::Commands::AddGroup::Id: + case Clusters::Groups::Commands::ViewGroup::Id: + case Clusters::Groups::Commands::GetGroupMembership::Id: + case Clusters::Groups::Commands::RemoveGroup::Id: + case Clusters::Groups::Commands::RemoveAllGroups::Id: + return true; + default: + return false; + } + } + case Clusters::GroupKeyManagement::Id: { + switch (aCommand) + { + case Clusters::GroupKeyManagement::Commands::KeySetWrite::Id: + case Clusters::GroupKeyManagement::Commands::KeySetRead::Id: + case Clusters::GroupKeyManagement::Commands::KeySetRemove::Id: + case Clusters::GroupKeyManagement::Commands::KeySetReadAllIndices::Id: + return true; + default: + return false; + } + } + case Clusters::GeneralCommissioning::Id: { + switch (aCommand) + { + case Clusters::GeneralCommissioning::Commands::CommissioningComplete::Id: + return true; + default: + return false; + } + } + case Clusters::Scenes::Id: { + // Entire cluster is fabric-scoped. + return true; + } + case Clusters::OperationalCredentials::Id: { + switch (aCommand) + { + case Clusters::OperationalCredentials::Commands::UpdateNOC::Id: + case Clusters::OperationalCredentials::Commands::UpdateFabricLabel::Id: + return true; + default: + return false; + } + } + default: + break; + } + + return false; +} + } // namespace app } // namespace chip diff --git a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h index b1d5803f3fb8dc..d6b1c9d3e85fd9 100644 --- a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h +++ b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h @@ -27783,6 +27783,7 @@ struct DecodableType } // namespace Clusters bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand); +bool CommandIsFabricScoped(ClusterId aCluster, CommandId aCommand); } // namespace app } // namespace chip From 8a0b87b2e40d1c9b3ef544dc75854eaf5209234e Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Sun, 17 Jul 2022 23:07:57 -0400 Subject: [PATCH 2/6] Add cirque test --- src/controller/python/chip/ChipDeviceCtrl.py | 4 ++-- src/controller/python/chip/clusters/Command.py | 12 +++++++++--- .../python/chip/interaction_model/__init__.py | 10 +++++----- src/controller/python/test/test_scripts/base.py | 15 +++++++++++++++ .../test/test_scripts/split_commissioning_test.py | 4 ++++ 5 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index 4679bfd72d8d2b..f8bdc005d2ad18 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -151,7 +151,7 @@ def HandleKeyExchangeComplete(err): self._ChipStack.callbackRes = self._ChipStack.ErrorToException( err) else: - print("Established CASE with Device") + print("Established secure ession with Device") if self.state != DCState.COMMISSIONING: # During Commissioning, HandleKeyExchangeComplete will also be called, # in this case the async operation should be marked as finished by @@ -939,7 +939,7 @@ def ZCLSend(self, cluster, command, nodeid, endpoint, groupid, args, blocking=Fa print(f"CommandResponse {res}") return (0, res) except InteractionModelError as ex: - return (int(ex.state), None) + return (int(ex.status), None) def ZCLReadAttribute(self, cluster, attribute, nodeid, endpoint, groupid, blocking=True): self.CheckIsActive() diff --git a/src/controller/python/chip/clusters/Command.py b/src/controller/python/chip/clusters/Command.py index 21c9b476d21000..7d53b21784ff5e 100644 --- a/src/controller/python/chip/clusters/Command.py +++ b/src/controller/python/chip/clusters/Command.py @@ -30,6 +30,11 @@ import inspect import sys import builtins +import logging + + +logger = logging.getLogger('chip.cluster.Command') +logger.setLevel(logging.ERROR) @dataclass @@ -94,7 +99,7 @@ def handleResponse(self, path: CommandPath, status: Status, response: bytes): self._event_loop.call_soon_threadsafe( self._handleResponse, path, status, response) - def _handleError(self, imError: int, chipError: int, exception: Exception): + def _handleError(self, imError: Status, chipError: int, exception: Exception): if exception: self._future.set_exception(exception) elif chipError != 0: @@ -103,8 +108,9 @@ def _handleError(self, imError: int, chipError: int, exception: Exception): else: try: self._future.set_exception( - chip.interaction_model.InteractionModelError(chip.interaction_model.Status(status.IMStatus))) - except: + chip.interaction_model.InteractionModelError(chip.interaction_model.Status(imError.IMStatus))) + except Exception as e2: + logger.exception("Failed to map interaction model status received: %s. Remapping to Failure." % imError) self._future.set_exception(chip.interaction_model.InteractionModelError( chip.interaction_model.Status.Failure)) diff --git a/src/controller/python/chip/interaction_model/__init__.py b/src/controller/python/chip/interaction_model/__init__.py index 79e94646e08b63..bc1fa35c214865 100644 --- a/src/controller/python/chip/interaction_model/__init__.py +++ b/src/controller/python/chip/interaction_model/__init__.py @@ -74,12 +74,12 @@ class Status(enum.IntEnum): class InteractionModelError(ChipStackException): - def __init__(self, state: Status): - self._state = state + def __init__(self, status: Status): + self._status = status def __str__(self): - return f"InteractionModelError: {self._state.name} (0x{self._state.value:x})" + return f"InteractionModelError: {self._status.name} (0x{self._status.value:x})" @property - def state(self) -> Status: - return self._state + def status(self) -> Status: + return self._status diff --git a/src/controller/python/test/test_scripts/base.py b/src/controller/python/test/test_scripts/base.py index afb775541f530a..9aa932e2038806 100644 --- a/src/controller/python/test/test_scripts/base.py +++ b/src/controller/python/test/test_scripts/base.py @@ -1012,3 +1012,18 @@ def TestNonControllerAPIs(self): self.logger.exception(f"Failed to finish API test: {ex}") return False return True + + def TestFabricScopedCommandDuringPase(self, nodeid: int): + '''Validates that fabric-scoped commands fail during PASE with UNSUPPORTED_ACCESS + + The nodeid is the PASE pseudo-node-ID used during PASE establishment + ''' + status = None + try: + response = asyncio.run(self.devCtrl.SendCommand( + nodeid, 0, Clusters.OperationalCredentials.Commands.UpdateFabricLabel("roboto"))) + except IM.InteractionModelError as ex: + status = ex.status + + # TODO: Should be IM.Status.UnsupportedAccess + return status == IM.Status.InvalidCommand diff --git a/src/controller/python/test/test_scripts/split_commissioning_test.py b/src/controller/python/test/test_scripts/split_commissioning_test.py index 35794b15420ce0..1652231b4ea222 100755 --- a/src/controller/python/test/test_scripts/split_commissioning_test.py +++ b/src/controller/python/test/test_scripts/split_commissioning_test.py @@ -108,6 +108,10 @@ def main(): nodeid=2), "Failed to establish PASE connection with device 2") + logger.info("Attempting to execute a fabric-scoped command during PASE before AddNOC") + FailIfNot(test.TestFabricScopedCommandDuringPase(nodeid=1), + "Did not get UNSUPPORTED_ACCESS for fabric-scoped command during PASE") + FailIfNot(test.TestCommissionOnly(nodeid=1), "Failed to commission device 1") From 6e00a5af33c3f57ad3a0da87b7c11cd714e2021c Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Sun, 17 Jul 2022 23:24:53 -0400 Subject: [PATCH 3/6] Fixed typo --- src/controller/python/chip/ChipDeviceCtrl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index f8bdc005d2ad18..edc2944afea6aa 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -151,7 +151,7 @@ def HandleKeyExchangeComplete(err): self._ChipStack.callbackRes = self._ChipStack.ErrorToException( err) else: - print("Established secure ession with Device") + print("Established secure session with Device") if self.state != DCState.COMMISSIONING: # During Commissioning, HandleKeyExchangeComplete will also be called, # in this case the async operation should be marked as finished by From 4bcd8057d8a50557143ba64458c01835fa769d6c Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Sun, 17 Jul 2022 23:27:02 -0400 Subject: [PATCH 4/6] Revert file committed by mistake --- src/credentials/FabricTable.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index 823e4e0fff3219..68f9f9c8e132a1 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -883,15 +883,12 @@ class DLL_EXPORT FabricTable CHIP_ERROR AddNewFabricForTest(const ByteSpan & rootCert, const ByteSpan & icacCert, const ByteSpan & nocCert, const ByteSpan & opKeySpan, FabricIndex * outFabricIndex); - // TODO: Add docs :D - void AllowCollisionsOnNextFabricAdd() { mStateFlags.Set(StateFlags::kAreCollidingFabricsIgnored); } - // Same as AddNewFabricForTest, but ignore if we are colliding with same , so // that a single fabric table can have N nodes for same fabric. This usually works, but is bad form. CHIP_ERROR AddNewFabricForTestIgnoringCollisions(const ByteSpan & rootCert, const ByteSpan & icacCert, const ByteSpan & nocCert, const ByteSpan & opKeySpan, FabricIndex * outFabricIndex) { - AllowCollisionsOnNextFabricAdd(); + mStateFlags.Set(StateFlags::kAreCollidingFabricsIgnored); CHIP_ERROR err = AddNewFabricForTest(rootCert, icacCert, nocCert, opKeySpan, outFabricIndex); mStateFlags.Clear(StateFlags::kAreCollidingFabricsIgnored); return err; From c3475206270565da4e8dc0cbd026b725bda2dcae Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Mon, 18 Jul 2022 10:55:32 -0400 Subject: [PATCH 5/6] Fix CI --- src/controller/python/test/test_scripts/base.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/controller/python/test/test_scripts/base.py b/src/controller/python/test/test_scripts/base.py index 9aa932e2038806..6cd812371f32c2 100644 --- a/src/controller/python/test/test_scripts/base.py +++ b/src/controller/python/test/test_scripts/base.py @@ -1025,5 +1025,4 @@ def TestFabricScopedCommandDuringPase(self, nodeid: int): except IM.InteractionModelError as ex: status = ex.status - # TODO: Should be IM.Status.UnsupportedAccess - return status == IM.Status.InvalidCommand + return status == IM.Status.UnsupportedAccess From 64f4dfe275bbef8e373899e40dfa1f3ce7267453 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Mon, 18 Jul 2022 14:11:25 -0400 Subject: [PATCH 6/6] Apply review comments from @bzbarsky-apple --- src/app/CommandHandler.cpp | 4 ++++ src/app/data-model/Nullable.h | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index db727c15563e57..2fc2dc55c4d850 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -407,6 +407,10 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo ExitNow(); } + // No check for `CommandIsFabricScoped` unlike in `ProcessCommandDataIB()` since group commands + // always have an accessing fabric, by definition. + + // Find which endpoints can process the command, and dispatch to them. iterator = groupDataProvider->IterateEndpoints(fabric); VerifyOrExit(iterator != nullptr, err = CHIP_ERROR_NO_MEMORY); diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 4e6eaa7270b4f9..c0e2a7ea8c9e58 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -79,7 +79,7 @@ struct Nullable : protected Optional return true; } - // The only fabric-scoped objects in the spec are commands, events and structs inside lists, and neither one can be nullable. + // The only fabric-scoped objects in the spec are commands, events and structs inside lists, and none of those can be nullable. static constexpr bool kIsFabricScoped = false; bool operator==(const Nullable & other) const { return Optional::operator==(other); }