Skip to content

Commit

Permalink
Handle ACL and readability in reporting/Engine.cpp (#36488)
Browse files Browse the repository at this point in the history 
* Move ACL validation in reporting engine for reads

* Fix up logic for ACL & return codes

* Take into consideration global attributes: their ACL is ok

* Make testread pass (with hacks because the test is not sane)

* Update comment and model setting.

* Another comment update

* Fix includes

* Fix typo

* Restyled by clang-format

* Update comment

* Update ACL test logic and revert testread to original setup

* Restyle

* Address review comments

* Restyle

* Update src/app/reporting/Engine.cpp

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Update src/app/reporting/Engine.cpp

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Update src/app/reporting/Engine.cpp

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Update src/app/reporting/Engine.cpp

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Update src/app/reporting/Engine.cpp

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Restyle

* Fix extra bracket

---------

Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Andrei Litvin <andreilitvin@google.com>
  • Loading branch information
@andy31415 @restyled-commits
@bzbarsky-apple @andreilitvin
4 people authored Nov 19, 2024
1 parent 29f008e commit 4b3fff6
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 90 deletions.
5 changes: 4 additions & 1 deletion src/app/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,10 @@ static_library("interaction-model") {
"reporting/reporting.h",
]

deps = [ "${chip_root}/src/app:events" ]
deps = [
"${chip_root}/src/app:events",
"${chip_root}/src/app:global-attributes",
]

# Temporary dependency: codegen data provider instance should be provided
# by the application
Expand Down
27 changes: 0 additions & 27 deletions src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,33 +104,6 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const Data
ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId),
request.path.mExpanded);

// ACL check for non-internal requests
if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal))
{
VerifyOrReturnError(request.subjectDescriptor != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

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)
{
VerifyOrReturnError((err == CHIP_ERROR_ACCESS_DENIED) || (err == CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err);

// Implementation of 8.4.3.2 of the spec for path expansion
if (request.path.mExpanded)
{
return CHIP_NO_ERROR;
}

// access denied and access restricted have specific codes for IM
return err == CHIP_ERROR_ACCESS_DENIED ? CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)
: CHIP_IM_GLOBAL_STATUS(AccessRestricted);
}
}

auto metadata = Ember::FindAttributeMetadata(request.path);

// Explicit failure in finding a suitable metadata
Expand Down
32 changes: 0 additions & 32 deletions src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1321,20 +1321,6 @@ TEST(TestCodegenModelViaMocks, CommandHandlerInterfaceAcceptedCommands)
EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 33)).has_value());
}

TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny)
{
UseMockNodeConfig config(gTestNodeConfig);
CodegenDataModelProviderWithContext model;
ScopedMockAccessControl accessControl;

ReadOperation testRequest(kMockEndpoint1, MockClusterId(1), MockAttributeId(10));
testRequest.SetSubjectDescriptor(kDenySubjectDescriptor);

std::unique_ptr<AttributeValueEncoder> encoder = testRequest.StartEncoding();

ASSERT_EQ(model.ReadAttribute(testRequest.GetRequest(), *encoder), Status::UnsupportedAccess);
}

TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath)
{
UseMockNodeConfig config(gTestNodeConfig);
Expand Down Expand Up @@ -1392,24 +1378,6 @@ TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead)
}
}

TEST(TestCodegenModelViaMocks, EmberAttributePathExpansionAccessDeniedRead)
{
UseMockNodeConfig config(gTestNodeConfig);
CodegenDataModelProviderWithContext model;
ScopedMockAccessControl accessControl;

ReadOperation testRequest(kMockEndpoint1, MockClusterId(1), MockAttributeId(10));
testRequest.SetSubjectDescriptor(kDenySubjectDescriptor);
testRequest.SetPathExpanded(true);

std::unique_ptr<AttributeValueEncoder> encoder = testRequest.StartEncoding();

// For expanded paths, access control failures succeed without encoding anything
// This is temporary until ACL checks are moved inside the IM/ReportEngine
ASSERT_EQ(model.ReadAttribute(testRequest.GetRequest(), *encoder), CHIP_NO_ERROR);
ASSERT_FALSE(encoder->TriedEncode());
}

TEST(TestCodegenModelViaMocks, AccessInterfaceUnsupportedRead)
{
UseMockNodeConfig config(gTestNodeConfig);
Expand Down
10 changes: 1 addition & 9 deletions src/app/data-model-provider/Provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,7 @@ class Provider : public ProviderMetadataTree
virtual InteractionModelContext CurrentContext() const { return mContext; }

/// TEMPORARY/TRANSITIONAL requirement for transitioning from ember-specific code
/// ReadAttribute is REQUIRED to perform:
/// - ACL validation (see notes on OperationFlags::kInternal)
/// - Validation of readability/writability (also controlled by OperationFlags::kInternal)
/// - use request.path.mExpanded to skip encoding replies for data according
/// to 8.4.3.2 of the spec:
/// > If the path indicates attribute data that is not readable, then the path SHALL
/// be discarded.
/// > Else if reading from the attribute in the path requires a privilege that is not
/// granted to access the cluster in the path, then the path SHALL be discarded.
/// ReadAttribute is REQUIRED to respond to GlobalAttribute read requests
///
/// Return value notes:
/// ActionReturnStatus::IsOutOfSpaceEncodingResponse
Expand Down
126 changes: 105 additions & 21 deletions src/app/reporting/Engine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,24 @@
* limitations under the License.
*/

#include <access/AccessRestrictionProvider.h>
#include <access/Privilege.h>
#include <app/AppConfig.h>
#include <app/ConcreteEventPath.h>
#include <app/GlobalAttributes.h>
#include <app/InteractionModelEngine.h>
#include <app/RequiredPrivilege.h>
#include <app/data-model-provider/ActionReturnStatus.h>
#include <app/data-model-provider/MetadataTypes.h>
#include <app/data-model-provider/Provider.h>
#include <app/icd/server/ICDServerConfig.h>
#include <app/reporting/Engine.h>
#include <app/reporting/reporting.h>
#include <app/util/MatterCallbacks.h>
#include <lib/core/CHIPError.h>
#include <lib/core/DataModelTypes.h>
#include <lib/support/CodeUtils.h>
#include <optional>
#include <protocols/interaction_model/StatusCode.h>

#if CHIP_CONFIG_ENABLE_ICD_SERVER
Expand All @@ -52,9 +59,81 @@ Status EventPathValid(DataModel::Provider * model, const ConcreteEventPath & eve
return Status::Success;
}

DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel,
const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered,
AttributeReportIBs::Builder & reportBuilder,
/// Returns the status of ACL validation.
/// If the return value has a status set, that means the ACL check failed,
/// the read must not be performed, and the returned status (which may
/// be success, when dealing with non-concrete paths) should be used
/// as the status for the read.
///
/// If the returned value is std::nullopt, that means the ACL check passed and the
/// read should proceed.
std::optional<CHIP_ERROR> ValidateReadAttributeACL(DataModel::Provider * dataModel, const SubjectDescriptor & subjectDescriptor,
const ConcreteReadAttributePath & path)
{

RequestPath requestPath{ .cluster = path.mClusterId,
.endpoint = path.mEndpointId,
.requestType = RequestType::kAttributeReadRequest,
.entityId = path.mAttributeId };

std::optional<DataModel::AttributeInfo> info = dataModel->GetAttributeInfo(path);

// If the attribute exists, we know whether it is readable (readPrivilege has value)
// and what the required access privilege is. However for attributes missing from the metatada
// (e.g. global attributes) or completely missing attributes we do not actually know of a required
// privilege and default to kView (this is correct for global attributes and a reasonable check
// for others)
Privilege requiredPrivilege = Privilege::kView;
if (info.has_value() && info->readPrivilege.has_value())
{
// attribute exists and is readable, set the correct read privilege
requiredPrivilege = *info->readPrivilege;
}

CHIP_ERROR err = GetAccessControl().Check(subjectDescriptor, requestPath, requiredPrivilege);
if (err == CHIP_NO_ERROR)
{
if (IsSupportedGlobalAttributeNotInMetadata(path.mAttributeId))
{
// Global attributes passing a kView check is ok
return std::nullopt;
}

// We want to return "success" (i.e. nulopt) IF AND ONLY IF the attribute exists and is readable (has read privilege).
// Since the Access control check above may have passed with kView, we do another check here:
// - Attribute exists (info has value)
// - Attribute is readable (readProvilege has value) and not "write only"
// If the attribute exists and is not readable, we will return UnsupportedRead (spec 8.4.3.2: "Else if the path indicates
// attribute data that is not readable, an AttributeStatusIB SHALL be generated with the UNSUPPORTED_READ Status Code.")
//
// TODO:: https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/9024 requires interleaved ordering that
// is NOT implemented here. Spec requires:
// - check cluster access check (done here as kView at least)
// - unsupported endpoint/cluster/attribute check (NOT done here) when the attribute is missing.
// this SHOULD be done here when info does not have a value. This was not done as a first pass to
// minimize amount of delta in the initial PR.
// - "write-only" attributes should return UNSUPPORTED_READ (this is done here)
if (info.has_value() && !info->readPrivilege.has_value())
{
return CHIP_IM_GLOBAL_STATUS(UnsupportedRead);
}

return std::nullopt;
}
VerifyOrReturnError((err == CHIP_ERROR_ACCESS_DENIED) || (err == CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err);

// Implementation of 8.4.3.2 of the spec for path expansion
if (path.mExpanded)
{
return CHIP_NO_ERROR;
}

// access denied and access restricted have specific codes for IM
return err == CHIP_ERROR_ACCESS_DENIED ? CHIP_IM_GLOBAL_STATUS(UnsupportedAccess) : CHIP_IM_GLOBAL_STATUS(AccessRestricted);
}

DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const SubjectDescriptor & subjectDescriptor,
bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder,
const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState)
{
ChipLogDetail(DataManagement, "<RE:Run> Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", path.mClusterId,
Expand All @@ -64,10 +143,7 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode

DataModel::ReadAttributeRequest readRequest;

if (isFabricFiltered)
{
readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered);
}
readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered, isFabricFiltered);
readRequest.subjectDescriptor = &subjectDescriptor;
readRequest.path = path;

Expand All @@ -84,9 +160,17 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode
TLV::TLVWriter checkpoint;
reportBuilder.Checkpoint(checkpoint);

DataModel::ActionReturnStatus status(CHIP_NO_ERROR);
AttributeValueEncoder attributeValueEncoder(reportBuilder, subjectDescriptor, path, version, isFabricFiltered, encoderState);

DataModel::ActionReturnStatus status = dataModel->ReadAttribute(readRequest, attributeValueEncoder);
if (auto access_status = ValidateReadAttributeACL(dataModel, subjectDescriptor, path); access_status.has_value())
{
status = *access_status;
}
else
{
status = dataModel->ReadAttribute(readRequest, attributeValueEncoder);
}

if (status.IsSuccess())
{
Expand Down Expand Up @@ -435,13 +519,13 @@ CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool &
aHasEncodedData = true;
}

Access::RequestPath requestPath{ .cluster = current->mValue.mClusterId,
.endpoint = current->mValue.mEndpointId,
.requestType = RequestType::kEventReadRequest,
.entityId = current->mValue.mEventId };
Access::Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path);
RequestPath requestPath{ .cluster = current->mValue.mClusterId,
.endpoint = current->mValue.mEndpointId,
.requestType = RequestType::kEventReadRequest,
.entityId = current->mValue.mEventId };
Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path);

err = Access::GetAccessControl().Check(apReadHandler->GetSubjectDescriptor(), requestPath, requestPrivilege);
err = GetAccessControl().Check(apReadHandler->GetSubjectDescriptor(), requestPath, requestPrivilege);
if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL))
{
ReturnErrorOnFailure(err);
Expand Down Expand Up @@ -580,13 +664,13 @@ CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder
CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
{
CHIP_ERROR err = CHIP_NO_ERROR;
chip::System::PacketBufferTLVWriter reportDataWriter;
System::PacketBufferTLVWriter reportDataWriter;
ReportDataMessage::Builder reportDataBuilder;
chip::System::PacketBufferHandle bufHandle = nullptr;
uint16_t reservedSize = 0;
bool hasMoreChunks = false;
bool needCloseReadHandler = false;
size_t reportBufferMaxSize = 0;
System::PacketBufferHandle bufHandle = nullptr;
uint16_t reservedSize = 0;
bool hasMoreChunks = false;
bool needCloseReadHandler = false;
size_t reportBufferMaxSize = 0;

// Reserved size for the MoreChunks boolean flag, which takes up 1 byte for the control tag and 1 byte for the context tag.
const uint32_t kReservedSizeForMoreChunksFlag = 1 + 1;
Expand Down Expand Up @@ -623,7 +707,7 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
// Always limit the size of the generated packet to fit within the max size returned by the ReadHandler regardless
// of the available buffer capacity.
// Also, we need to reserve some extra space for the MIC field.
reportDataWriter.ReserveBuffer(static_cast<uint32_t>(reservedSize + chip::Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES));
reportDataWriter.ReserveBuffer(static_cast<uint32_t>(reservedSize + Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES));

// Create a report data.
err = reportDataBuilder.Init(&reportDataWriter);
Expand Down

0 comments on commit 4b3fff6

Please sign in to comment.