From 1457704fdb141f49aff56d3f8a27b4c0fc215f63 Mon Sep 17 00:00:00 2001 From: Thomas Lea <35579828+tleacmcsa@users.noreply.github.com> Date: Fri, 9 Aug 2024 13:01:55 -0500 Subject: [PATCH] Update ACL API to support ARL use cases (#34537) * Update ACL API to support ARL use cases ARL needs to know what attribute/command/event is being accessed and with which interaction model action. This change brings this knowledge into the AccessControl classes Check method for further enhancement with ARL feature on top. * Addressed review comments * Addressed review comments * Clarified RequestType enum values and purpose * Renamed CanAccess to CanAccessEvent * Restyled by whitespace * Restyled by clang-format * Fixed some build issues - missed a spot to populate requestType and entityId - fix unused function definition for some cases * Fixed icd-management-server for ARL * fixed typo in previous fix :-( * Restyled by clang-format * Moved ARL related checks behind ARL feature flag Instead of validating requestType in AccessControl::Check, we will validate it is not unknown only if the ARL feature is enabled. * Add conditional ARL related validation Instead of removing ARL related validation from AccessControl::Check entirely, perform it only if the ARL feature is enabled. As of this commit, it is not enabled. * Restyled by clang-format --------- Co-authored-by: Restyled.io --- src/access/AccessControl.cpp | 32 +++++++++++++++-- src/access/AccessControl.h | 7 ++++ src/access/RequestPath.h | 18 ++++++++-- src/app/CommandHandlerImpl.cpp | 10 ++++-- src/app/EventManagement.cpp | 5 ++- src/app/InteractionModelEngine.cpp | 36 +++++++++++++------ .../icd-management-server.cpp | 5 ++- .../CodegenDataModelProvider_Read.cpp | 5 ++- .../CodegenDataModelProvider_Write.cpp | 5 ++- src/app/reporting/Engine.cpp | 5 ++- .../util/ember-compatibility-functions.cpp | 10 ++++-- 11 files changed, 115 insertions(+), 23 deletions(-) diff --git a/src/access/AccessControl.cpp b/src/access/AccessControl.cpp index 70921599fca84a..fcb5a43d975f8e 100644 --- a/src/access/AccessControl.cpp +++ b/src/access/AccessControl.cpp @@ -171,6 +171,23 @@ char GetPrivilegeStringForLogging(Privilege privilege) return 'u'; } +char GetRequestTypeStringForLogging(RequestType requestType) +{ + switch (requestType) + { + case RequestType::kAttributeReadRequest: + return 'r'; + case RequestType::kAttributeWriteRequest: + return 'w'; + case RequestType::kCommandInvokeRequest: + return 'i'; + case RequestType::kEventReadOrSubscribeRequest: + return 'e'; + default: + return '?'; + } +} + #endif // CHIP_PROGRESS_LOGGING && CHIP_CONFIG_ACCESS_CONTROL_POLICY_LOGGING_VERBOSITY > 1 } // namespace @@ -306,6 +323,11 @@ void AccessControl::RemoveEntryListener(EntryListener & listener) } } +bool AccessControl::IsAccessRestrictionListSupported() const +{ + return false; // not yet supported +} + CHIP_ERROR AccessControl::Check(const SubjectDescriptor & subjectDescriptor, const RequestPath & requestPath, Privilege requestPrivilege) { @@ -316,14 +338,20 @@ CHIP_ERROR AccessControl::Check(const SubjectDescriptor & subjectDescriptor, con constexpr size_t kMaxCatsToLog = 6; char catLogBuf[kMaxCatsToLog * kCharsPerCatForLogging]; ChipLogProgress(DataManagement, - "AccessControl: checking f=%u a=%c s=0x" ChipLogFormatX64 " t=%s c=" ChipLogFormatMEI " e=%u p=%c", + "AccessControl: checking f=%u a=%c s=0x" ChipLogFormatX64 " t=%s c=" ChipLogFormatMEI " e=%u p=%c r=%c", subjectDescriptor.fabricIndex, GetAuthModeStringForLogging(subjectDescriptor.authMode), ChipLogValueX64(subjectDescriptor.subject), GetCatStringForLogging(catLogBuf, sizeof(catLogBuf), subjectDescriptor.cats), - ChipLogValueMEI(requestPath.cluster), requestPath.endpoint, GetPrivilegeStringForLogging(requestPrivilege)); + ChipLogValueMEI(requestPath.cluster), requestPath.endpoint, GetPrivilegeStringForLogging(requestPrivilege), + GetRequestTypeStringForLogging(requestPath.requestType)); } #endif // CHIP_PROGRESS_LOGGING && CHIP_CONFIG_ACCESS_CONTROL_POLICY_LOGGING_VERBOSITY > 1 + if (IsAccessRestrictionListSupported()) + { + VerifyOrReturnError(requestPath.requestType != RequestType::kRequestTypeUnknown, CHIP_ERROR_INVALID_ARGUMENT); + } + { CHIP_ERROR result = mDelegate->Check(subjectDescriptor, requestPath, requestPrivilege); if (result != CHIP_ERROR_NOT_IMPLEMENTED) diff --git a/src/access/AccessControl.h b/src/access/AccessControl.h index 6186c33d44597a..a7c3472f5d99b4 100644 --- a/src/access/AccessControl.h +++ b/src/access/AccessControl.h @@ -627,6 +627,13 @@ class AccessControl // Removes a listener from the listener list, if in the list. void RemoveEntryListener(EntryListener & listener); + /** + * Check whether or not Access Restriction List is supported. + * + * @retval true if Access Restriction List is supported. + */ + bool IsAccessRestrictionListSupported() const; + /** * Check whether access (by a subject descriptor, to a request path, * requiring a privilege) should be allowed or denied. diff --git a/src/access/RequestPath.h b/src/access/RequestPath.h index 966f27afe8f716..af791d73eb151d 100644 --- a/src/access/RequestPath.h +++ b/src/access/RequestPath.h @@ -19,15 +19,29 @@ #pragma once #include +#include namespace chip { namespace Access { +enum class RequestType : uint8_t +{ + kRequestTypeUnknown, + kAttributeReadRequest, + kAttributeWriteRequest, + kCommandInvokeRequest, + kEventReadOrSubscribeRequest +}; + struct RequestPath { // NOTE: eventually this will likely also contain node, for proxying - ClusterId cluster = 0; - EndpointId endpoint = 0; + ClusterId cluster = 0; + EndpointId endpoint = 0; + RequestType requestType = RequestType::kRequestTypeUnknown; + + // entityId represents an attribute, command, or event ID, which is determined by the requestType. Wildcard if omitted. + std::optional entityId; }; } // namespace Access diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index b1843d23e8b0cd..1945e7e5e69dc3 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -402,7 +402,10 @@ Status CommandHandlerImpl::ProcessCommandDataIB(CommandDataIB::Parser & aCommand { Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor(); - Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId }; + Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, + .endpoint = concretePath.mEndpointId, + .requestType = Access::RequestType::kCommandInvokeRequest, + .entityId = concretePath.mCommandId }; Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath); err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege); if (err != CHIP_NO_ERROR) @@ -548,7 +551,10 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo { Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor(); - Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId }; + Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, + .endpoint = concretePath.mEndpointId, + .requestType = Access::RequestType::kCommandInvokeRequest, + .entityId = concretePath.mCommandId }; Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath); err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege); if (err != CHIP_NO_ERROR) diff --git a/src/app/EventManagement.cpp b/src/app/EventManagement.cpp index 8e6d53c24c9636..4c5faab3cdcda4 100644 --- a/src/app/EventManagement.cpp +++ b/src/app/EventManagement.cpp @@ -554,7 +554,10 @@ CHIP_ERROR EventManagement::CheckEventContext(EventLoadOutContext * eventLoadOut ReturnErrorOnFailure(ret); - Access::RequestPath requestPath{ .cluster = event.mClusterId, .endpoint = event.mEndpointId }; + Access::RequestPath requestPath{ .cluster = event.mClusterId, + .endpoint = event.mEndpointId, + .requestType = Access::RequestType::kEventReadOrSubscribeRequest, + .entityId = event.mEventId }; Access::Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path); CHIP_ERROR accessControlError = Access::GetAccessControl().Check(eventLoadOutContext->mSubjectDescriptor, requestPath, requestPrivilege); diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 80312a57873f43..64d30bc6c0e42f 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -494,7 +494,10 @@ CHIP_ERROR InteractionModelEngine::ParseAttributePaths(const Access::SubjectDesc // AttributePathExpandIterator. So we just need to check the ACL bits. for (; pathIterator.Get(readPath); pathIterator.Next()) { - Access::RequestPath requestPath{ .cluster = readPath.mClusterId, .endpoint = readPath.mEndpointId }; + // leave requestPath.entityId optional value unset to indicate wildcard + Access::RequestPath requestPath{ .cluster = readPath.mClusterId, + .endpoint = readPath.mEndpointId, + .requestType = Access::RequestType::kAttributeReadRequest }; err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, RequiredPrivilege::ForReadAttribute(readPath)); if (err == CHIP_NO_ERROR) @@ -510,7 +513,10 @@ CHIP_ERROR InteractionModelEngine::ParseAttributePaths(const Access::SubjectDesc paramsList.mValue.mAttributeId); if (ConcreteAttributePathExists(concretePath)) { - Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId }; + Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, + .endpoint = concretePath.mEndpointId, + .requestType = Access::RequestType::kAttributeReadRequest, + .entityId = paramsList.mValue.mAttributeId }; err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, RequiredPrivilege::ForReadAttribute(concretePath)); @@ -532,17 +538,27 @@ CHIP_ERROR InteractionModelEngine::ParseAttributePaths(const Access::SubjectDesc return err; } -static bool CanAccess(const Access::SubjectDescriptor & aSubjectDescriptor, const ConcreteClusterPath & aPath, - Access::Privilege aNeededPrivilege) +#if !CHIP_CONFIG_ENABLE_EVENTLIST_ATTRIBUTE +static bool CanAccessEvent(const Access::SubjectDescriptor & aSubjectDescriptor, const ConcreteClusterPath & aPath, + Access::Privilege aNeededPrivilege) { - Access::RequestPath requestPath{ .cluster = aPath.mClusterId, .endpoint = aPath.mEndpointId }; + Access::RequestPath requestPath{ .cluster = aPath.mClusterId, + .endpoint = aPath.mEndpointId, + .requestType = Access::RequestType::kEventReadOrSubscribeRequest }; + // leave requestPath.entityId optional value unset to indicate wildcard CHIP_ERROR err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, aNeededPrivilege); return (err == CHIP_NO_ERROR); } +#endif -static bool CanAccess(const Access::SubjectDescriptor & aSubjectDescriptor, const ConcreteEventPath & aPath) +static bool CanAccessEvent(const Access::SubjectDescriptor & aSubjectDescriptor, const ConcreteEventPath & aPath) { - return CanAccess(aSubjectDescriptor, aPath, RequiredPrivilege::ForReadEvent(aPath)); + Access::RequestPath requestPath{ .cluster = aPath.mClusterId, + .endpoint = aPath.mEndpointId, + .requestType = Access::RequestType::kEventReadOrSubscribeRequest, + .entityId = aPath.mEventId }; + CHIP_ERROR err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, RequiredPrivilege::ForReadEvent(aPath)); + return (err == CHIP_NO_ERROR); } /** @@ -559,7 +575,7 @@ static bool HasValidEventPathForEndpointAndCluster(EndpointId aEndpoint, const E { ConcreteEventPath path(aEndpoint, aCluster->clusterId, aCluster->eventList[idx]); // If we get here, the path exists. We just have to do an ACL check for it. - bool isValid = CanAccess(aSubjectDescriptor, path); + bool isValid = CanAccessEvent(aSubjectDescriptor, path); if (isValid) { return true; @@ -571,7 +587,7 @@ static bool HasValidEventPathForEndpointAndCluster(EndpointId aEndpoint, const E // We have no way to expand wildcards. Just assume that we would need // View permissions for whatever events are involved. ConcreteClusterPath clusterPath(aEndpoint, aCluster->clusterId); - return CanAccess(aSubjectDescriptor, clusterPath, Access::Privilege::kView); + return CanAccessEvent(aSubjectDescriptor, clusterPath, Access::Privilege::kView); #endif } @@ -581,7 +597,7 @@ static bool HasValidEventPathForEndpointAndCluster(EndpointId aEndpoint, const E // Not an existing event path. return false; } - return CanAccess(aSubjectDescriptor, path); + return CanAccessEvent(aSubjectDescriptor, path); } /** diff --git a/src/app/clusters/icd-management-server/icd-management-server.cpp b/src/app/clusters/icd-management-server/icd-management-server.cpp index 8445cad117c2a3..c2625b050d4661 100644 --- a/src/app/clusters/icd-management-server/icd-management-server.cpp +++ b/src/app/clusters/icd-management-server/icd-management-server.cpp @@ -240,7 +240,10 @@ CHIP_ERROR IcdManagementAttributeAccess::ReadMaximumCheckInBackOff(EndpointId en */ CHIP_ERROR CheckAdmin(CommandHandler * commandObj, const ConcreteCommandPath & commandPath, bool & isClientAdmin) { - RequestPath requestPath{ .cluster = commandPath.mClusterId, .endpoint = commandPath.mEndpointId }; + RequestPath requestPath{ .cluster = commandPath.mClusterId, + .endpoint = commandPath.mEndpointId, + .requestType = RequestType::kCommandInvokeRequest, + .entityId = commandPath.mCommandId }; CHIP_ERROR err = GetAccessControl().Check(commandObj->GetSubjectDescriptor(), requestPath, Privilege::kAdminister); if (CHIP_NO_ERROR == err) { diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp index 9f87e4c8f187fe..de17d1059be127 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp @@ -273,7 +273,10 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const Data { ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), CHIP_ERROR_INVALID_ARGUMENT); - Access::RequestPath requestPath{ .cluster = request.path.mClusterId, .endpoint = request.path.mEndpointId }; + Access::RequestPath requestPath{ .cluster = request.path.mClusterId, + .endpoint = request.path.mEndpointId, + .requestType = Access::RequestType::kAttributeReadRequest, + .entityId = request.path.mAttributeId }; CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath, RequiredPrivilege::ForReadAttribute(request.path)); if (err != CHIP_NO_ERROR) diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp index 37371043aa323f..333c9e27e4bf9f 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp @@ -278,7 +278,10 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat { ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), Status::UnsupportedAccess); - Access::RequestPath requestPath{ .cluster = request.path.mClusterId, .endpoint = request.path.mEndpointId }; + Access::RequestPath requestPath{ .cluster = request.path.mClusterId, + .endpoint = request.path.mEndpointId, + .requestType = Access::RequestType::kAttributeWriteRequest, + .entityId = request.path.mAttributeId }; CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath, RequiredPrivilege::ForWriteAttribute(request.path)); diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index b5621e94bd9f47..6d79e9e7b34af0 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -339,7 +339,10 @@ CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool & aHasEncodedData = true; } - Access::RequestPath requestPath{ .cluster = current->mValue.mClusterId, .endpoint = current->mValue.mEndpointId }; + Access::RequestPath requestPath{ .cluster = current->mValue.mClusterId, + .endpoint = current->mValue.mEndpointId, + .requestType = RequestType::kEventReadOrSubscribeRequest, + .entityId = current->mValue.mEventId }; Access::Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path); err = Access::GetAccessControl().Check(apReadHandler->GetSubjectDescriptor(), requestPath, requestPrivilege); diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index d556d8816c2145..ad0542a9ff0614 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -294,7 +294,10 @@ CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, b // depending on whether the path was expanded. { - Access::RequestPath requestPath{ .cluster = aPath.mClusterId, .endpoint = aPath.mEndpointId }; + Access::RequestPath requestPath{ .cluster = aPath.mClusterId, + .endpoint = aPath.mEndpointId, + .requestType = Access::RequestType::kAttributeReadRequest, + .entityId = aPath.mAttributeId }; Access::Privilege requestPrivilege = RequiredPrivilege::ForReadAttribute(aPath); CHIP_ERROR err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, requestPrivilege); if (err != CHIP_NO_ERROR) @@ -686,7 +689,10 @@ CHIP_ERROR WriteSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, } { - Access::RequestPath requestPath{ .cluster = aPath.mClusterId, .endpoint = aPath.mEndpointId }; + Access::RequestPath requestPath{ .cluster = aPath.mClusterId, + .endpoint = aPath.mEndpointId, + .requestType = Access::RequestType::kAttributeWriteRequest, + .entityId = aPath.mAttributeId }; Access::Privilege requestPrivilege = RequiredPrivilege::ForWriteAttribute(aPath); CHIP_ERROR err = CHIP_NO_ERROR; if (!apWriteHandler->ACLCheckCacheHit({ aPath, requestPrivilege }))