Skip to content

Commit 4b3fff6

Browse files
andy31415restyled-commitsbzbarsky-appleandreilitvin
authored
Handle ACL and readability in reporting/Engine.cpp (project-chip#36488)
* 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>
1 parent 29f008e commit 4b3fff6

File tree

5 files changed

+110
-90
lines changed

5 files changed

+110
-90
lines changed

src/app/BUILD.gn

+4-1
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,10 @@ static_library("interaction-model") {
201201
"reporting/reporting.h",
202202
]
203203

204-
deps = [ "${chip_root}/src/app:events" ]
204+
deps = [
205+
"${chip_root}/src/app:events",
206+
"${chip_root}/src/app:global-attributes",
207+
]
205208

206209
# Temporary dependency: codegen data provider instance should be provided
207210
# by the application

src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp

-27
Original file line numberDiff line numberDiff line change
@@ -104,33 +104,6 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const Data
104104
ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId),
105105
request.path.mExpanded);
106106

107-
// ACL check for non-internal requests
108-
if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal))
109-
{
110-
VerifyOrReturnError(request.subjectDescriptor != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
111-
112-
Access::RequestPath requestPath{ .cluster = request.path.mClusterId,
113-
.endpoint = request.path.mEndpointId,
114-
.requestType = Access::RequestType::kAttributeReadRequest,
115-
.entityId = request.path.mAttributeId };
116-
CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath,
117-
RequiredPrivilege::ForReadAttribute(request.path));
118-
if (err != CHIP_NO_ERROR)
119-
{
120-
VerifyOrReturnError((err == CHIP_ERROR_ACCESS_DENIED) || (err == CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err);
121-
122-
// Implementation of 8.4.3.2 of the spec for path expansion
123-
if (request.path.mExpanded)
124-
{
125-
return CHIP_NO_ERROR;
126-
}
127-
128-
// access denied and access restricted have specific codes for IM
129-
return err == CHIP_ERROR_ACCESS_DENIED ? CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)
130-
: CHIP_IM_GLOBAL_STATUS(AccessRestricted);
131-
}
132-
}
133-
134107
auto metadata = Ember::FindAttributeMetadata(request.path);
135108

136109
// Explicit failure in finding a suitable metadata

src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp

-32
Original file line numberDiff line numberDiff line change
@@ -1321,20 +1321,6 @@ TEST(TestCodegenModelViaMocks, CommandHandlerInterfaceAcceptedCommands)
13211321
EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 33)).has_value());
13221322
}
13231323

1324-
TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny)
1325-
{
1326-
UseMockNodeConfig config(gTestNodeConfig);
1327-
CodegenDataModelProviderWithContext model;
1328-
ScopedMockAccessControl accessControl;
1329-
1330-
ReadOperation testRequest(kMockEndpoint1, MockClusterId(1), MockAttributeId(10));
1331-
testRequest.SetSubjectDescriptor(kDenySubjectDescriptor);
1332-
1333-
std::unique_ptr<AttributeValueEncoder> encoder = testRequest.StartEncoding();
1334-
1335-
ASSERT_EQ(model.ReadAttribute(testRequest.GetRequest(), *encoder), Status::UnsupportedAccess);
1336-
}
1337-
13381324
TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath)
13391325
{
13401326
UseMockNodeConfig config(gTestNodeConfig);
@@ -1392,24 +1378,6 @@ TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead)
13921378
}
13931379
}
13941380

1395-
TEST(TestCodegenModelViaMocks, EmberAttributePathExpansionAccessDeniedRead)
1396-
{
1397-
UseMockNodeConfig config(gTestNodeConfig);
1398-
CodegenDataModelProviderWithContext model;
1399-
ScopedMockAccessControl accessControl;
1400-
1401-
ReadOperation testRequest(kMockEndpoint1, MockClusterId(1), MockAttributeId(10));
1402-
testRequest.SetSubjectDescriptor(kDenySubjectDescriptor);
1403-
testRequest.SetPathExpanded(true);
1404-
1405-
std::unique_ptr<AttributeValueEncoder> encoder = testRequest.StartEncoding();
1406-
1407-
// For expanded paths, access control failures succeed without encoding anything
1408-
// This is temporary until ACL checks are moved inside the IM/ReportEngine
1409-
ASSERT_EQ(model.ReadAttribute(testRequest.GetRequest(), *encoder), CHIP_NO_ERROR);
1410-
ASSERT_FALSE(encoder->TriedEncode());
1411-
}
1412-
14131381
TEST(TestCodegenModelViaMocks, AccessInterfaceUnsupportedRead)
14141382
{
14151383
UseMockNodeConfig config(gTestNodeConfig);

src/app/data-model-provider/Provider.h

+1-9
Original file line numberDiff line numberDiff line change
@@ -60,15 +60,7 @@ class Provider : public ProviderMetadataTree
6060
virtual InteractionModelContext CurrentContext() const { return mContext; }
6161

6262
/// TEMPORARY/TRANSITIONAL requirement for transitioning from ember-specific code
63-
/// ReadAttribute is REQUIRED to perform:
64-
/// - ACL validation (see notes on OperationFlags::kInternal)
65-
/// - Validation of readability/writability (also controlled by OperationFlags::kInternal)
66-
/// - use request.path.mExpanded to skip encoding replies for data according
67-
/// to 8.4.3.2 of the spec:
68-
/// > If the path indicates attribute data that is not readable, then the path SHALL
69-
/// be discarded.
70-
/// > Else if reading from the attribute in the path requires a privilege that is not
71-
/// granted to access the cluster in the path, then the path SHALL be discarded.
63+
/// ReadAttribute is REQUIRED to respond to GlobalAttribute read requests
7264
///
7365
/// Return value notes:
7466
/// ActionReturnStatus::IsOutOfSpaceEncodingResponse

src/app/reporting/Engine.cpp

+105-21
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,24 @@
1616
* limitations under the License.
1717
*/
1818

19+
#include <access/AccessRestrictionProvider.h>
20+
#include <access/Privilege.h>
1921
#include <app/AppConfig.h>
2022
#include <app/ConcreteEventPath.h>
23+
#include <app/GlobalAttributes.h>
2124
#include <app/InteractionModelEngine.h>
2225
#include <app/RequiredPrivilege.h>
2326
#include <app/data-model-provider/ActionReturnStatus.h>
27+
#include <app/data-model-provider/MetadataTypes.h>
2428
#include <app/data-model-provider/Provider.h>
2529
#include <app/icd/server/ICDServerConfig.h>
2630
#include <app/reporting/Engine.h>
2731
#include <app/reporting/reporting.h>
2832
#include <app/util/MatterCallbacks.h>
33+
#include <lib/core/CHIPError.h>
2934
#include <lib/core/DataModelTypes.h>
35+
#include <lib/support/CodeUtils.h>
36+
#include <optional>
3037
#include <protocols/interaction_model/StatusCode.h>
3138

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

55-
DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel,
56-
const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered,
57-
AttributeReportIBs::Builder & reportBuilder,
62+
/// Returns the status of ACL validation.
63+
/// If the return value has a status set, that means the ACL check failed,
64+
/// the read must not be performed, and the returned status (which may
65+
/// be success, when dealing with non-concrete paths) should be used
66+
/// as the status for the read.
67+
///
68+
/// If the returned value is std::nullopt, that means the ACL check passed and the
69+
/// read should proceed.
70+
std::optional<CHIP_ERROR> ValidateReadAttributeACL(DataModel::Provider * dataModel, const SubjectDescriptor & subjectDescriptor,
71+
const ConcreteReadAttributePath & path)
72+
{
73+
74+
RequestPath requestPath{ .cluster = path.mClusterId,
75+
.endpoint = path.mEndpointId,
76+
.requestType = RequestType::kAttributeReadRequest,
77+
.entityId = path.mAttributeId };
78+
79+
std::optional<DataModel::AttributeInfo> info = dataModel->GetAttributeInfo(path);
80+
81+
// If the attribute exists, we know whether it is readable (readPrivilege has value)
82+
// and what the required access privilege is. However for attributes missing from the metatada
83+
// (e.g. global attributes) or completely missing attributes we do not actually know of a required
84+
// privilege and default to kView (this is correct for global attributes and a reasonable check
85+
// for others)
86+
Privilege requiredPrivilege = Privilege::kView;
87+
if (info.has_value() && info->readPrivilege.has_value())
88+
{
89+
// attribute exists and is readable, set the correct read privilege
90+
requiredPrivilege = *info->readPrivilege;
91+
}
92+
93+
CHIP_ERROR err = GetAccessControl().Check(subjectDescriptor, requestPath, requiredPrivilege);
94+
if (err == CHIP_NO_ERROR)
95+
{
96+
if (IsSupportedGlobalAttributeNotInMetadata(path.mAttributeId))
97+
{
98+
// Global attributes passing a kView check is ok
99+
return std::nullopt;
100+
}
101+
102+
// We want to return "success" (i.e. nulopt) IF AND ONLY IF the attribute exists and is readable (has read privilege).
103+
// Since the Access control check above may have passed with kView, we do another check here:
104+
// - Attribute exists (info has value)
105+
// - Attribute is readable (readProvilege has value) and not "write only"
106+
// If the attribute exists and is not readable, we will return UnsupportedRead (spec 8.4.3.2: "Else if the path indicates
107+
// attribute data that is not readable, an AttributeStatusIB SHALL be generated with the UNSUPPORTED_READ Status Code.")
108+
//
109+
// TODO:: https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/9024 requires interleaved ordering that
110+
// is NOT implemented here. Spec requires:
111+
// - check cluster access check (done here as kView at least)
112+
// - unsupported endpoint/cluster/attribute check (NOT done here) when the attribute is missing.
113+
// this SHOULD be done here when info does not have a value. This was not done as a first pass to
114+
// minimize amount of delta in the initial PR.
115+
// - "write-only" attributes should return UNSUPPORTED_READ (this is done here)
116+
if (info.has_value() && !info->readPrivilege.has_value())
117+
{
118+
return CHIP_IM_GLOBAL_STATUS(UnsupportedRead);
119+
}
120+
121+
return std::nullopt;
122+
}
123+
VerifyOrReturnError((err == CHIP_ERROR_ACCESS_DENIED) || (err == CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err);
124+
125+
// Implementation of 8.4.3.2 of the spec for path expansion
126+
if (path.mExpanded)
127+
{
128+
return CHIP_NO_ERROR;
129+
}
130+
131+
// access denied and access restricted have specific codes for IM
132+
return err == CHIP_ERROR_ACCESS_DENIED ? CHIP_IM_GLOBAL_STATUS(UnsupportedAccess) : CHIP_IM_GLOBAL_STATUS(AccessRestricted);
133+
}
134+
135+
DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const SubjectDescriptor & subjectDescriptor,
136+
bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder,
58137
const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState)
59138
{
60139
ChipLogDetail(DataManagement, "<RE:Run> Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", path.mClusterId,
@@ -64,10 +143,7 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode
64143

65144
DataModel::ReadAttributeRequest readRequest;
66145

67-
if (isFabricFiltered)
68-
{
69-
readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered);
70-
}
146+
readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered, isFabricFiltered);
71147
readRequest.subjectDescriptor = &subjectDescriptor;
72148
readRequest.path = path;
73149

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

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

89-
DataModel::ActionReturnStatus status = dataModel->ReadAttribute(readRequest, attributeValueEncoder);
166+
if (auto access_status = ValidateReadAttributeACL(dataModel, subjectDescriptor, path); access_status.has_value())
167+
{
168+
status = *access_status;
169+
}
170+
else
171+
{
172+
status = dataModel->ReadAttribute(readRequest, attributeValueEncoder);
173+
}
90174

91175
if (status.IsSuccess())
92176
{
@@ -435,13 +519,13 @@ CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool &
435519
aHasEncodedData = true;
436520
}
437521

438-
Access::RequestPath requestPath{ .cluster = current->mValue.mClusterId,
439-
.endpoint = current->mValue.mEndpointId,
440-
.requestType = RequestType::kEventReadRequest,
441-
.entityId = current->mValue.mEventId };
442-
Access::Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path);
522+
RequestPath requestPath{ .cluster = current->mValue.mClusterId,
523+
.endpoint = current->mValue.mEndpointId,
524+
.requestType = RequestType::kEventReadRequest,
525+
.entityId = current->mValue.mEventId };
526+
Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path);
443527

444-
err = Access::GetAccessControl().Check(apReadHandler->GetSubjectDescriptor(), requestPath, requestPrivilege);
528+
err = GetAccessControl().Check(apReadHandler->GetSubjectDescriptor(), requestPath, requestPrivilege);
445529
if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL))
446530
{
447531
ReturnErrorOnFailure(err);
@@ -580,13 +664,13 @@ CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder
580664
CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
581665
{
582666
CHIP_ERROR err = CHIP_NO_ERROR;
583-
chip::System::PacketBufferTLVWriter reportDataWriter;
667+
System::PacketBufferTLVWriter reportDataWriter;
584668
ReportDataMessage::Builder reportDataBuilder;
585-
chip::System::PacketBufferHandle bufHandle = nullptr;
586-
uint16_t reservedSize = 0;
587-
bool hasMoreChunks = false;
588-
bool needCloseReadHandler = false;
589-
size_t reportBufferMaxSize = 0;
669+
System::PacketBufferHandle bufHandle = nullptr;
670+
uint16_t reservedSize = 0;
671+
bool hasMoreChunks = false;
672+
bool needCloseReadHandler = false;
673+
size_t reportBufferMaxSize = 0;
590674

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

628712
// Create a report data.
629713
err = reportDataBuilder.Init(&reportDataWriter);

0 commit comments

Comments
 (0)