Skip to content

Commit

Permalink
Enforce access control (#14921)
Browse files Browse the repository at this point in the history
* Enforce access control

Issue #14454

* Forgot to save this file

* Zap regen

* Small logging changes in access control

May not keep this for release but need it for the next few weeks.

* Add another log

* Add temp logs to debug nRF test failures on Zephyr

* Temporarily change ChipLogDetail to ChipLogError

* Change access control logs from detail to progress

* Remove temporary logs

* Add PermissiveAccessControlDelegate for tests

This new delegate does not error and overrides the access control
check to always allow. It is installed in the unit test context
so that unit tests (e.g. of interaction model, which has access
control baked in) can test units other than access control
without failing due to access control, and without running with
real access control and having to install real ACLs (that's for
integration tests to do).

* Fix logging

I meant to do this in the last commit.

* Add test helper dependency on access control
  • Loading branch information
mlepage-google authored and pull[bot] committed Sep 5, 2023
1 parent 85422fa commit dd13dbe
Show file tree
Hide file tree
Showing 11 changed files with 166 additions and 50 deletions.
47 changes: 29 additions & 18 deletions src/access/AccessControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ constexpr bool IsValidGroupNodeId(NodeId aNodeId)
return chip::IsGroupId(aNodeId) && chip::IsValidGroupId(chip::GroupIdFromNodeId(aNodeId));
}

#if CHIP_DETAIL_LOGGING
#if CHIP_PROGRESS_LOGGING

char GetAuthModeStringForLogging(AuthMode authMode)
{
Expand Down Expand Up @@ -152,7 +152,7 @@ char GetPrivilegeStringForLogging(Privilege privilege)
return 'u';
}

#endif // CHIP_DETAIL_LOGGING
#endif // CHIP_PROGRESS_LOGGING

} // namespace

Expand All @@ -165,35 +165,44 @@ AccessControl::Delegate AccessControl::mDefaultDelegate;

CHIP_ERROR AccessControl::Init()
{
ChipLogDetail(DataManagement, "AccessControl: initializing");
ChipLogProgress(DataManagement, "AccessControl: initializing");
return mDelegate.Init();
}

CHIP_ERROR AccessControl::Finish()
{
ChipLogDetail(DataManagement, "AccessControl: finishing");
ChipLogProgress(DataManagement, "AccessControl: finishing");
return mDelegate.Finish();
}

CHIP_ERROR AccessControl::Check(const SubjectDescriptor & subjectDescriptor, const RequestPath & requestPath,
Privilege requestPrivilege)
{
// Don't check if using default delegate (e.g. test code that isn't testing access control)
ReturnErrorCodeIf(&mDelegate == &mDefaultDelegate, CHIP_NO_ERROR);

#if CHIP_DETAIL_LOGGING
#if CHIP_PROGRESS_LOGGING
{
char buf[6 * kCharsPerCatForLogging];
ChipLogDetail(DataManagement,
"AccessControl: checking f=%u a=%c s=0x" ChipLogFormatX64 " t=%s c=" ChipLogFormatMEI " e=%" PRIu16 " p=%c",
subjectDescriptor.fabricIndex, GetAuthModeStringForLogging(subjectDescriptor.authMode),
ChipLogValueX64(subjectDescriptor.subject), GetCatStringForLogging(buf, sizeof(buf), subjectDescriptor.cats),
ChipLogValueMEI(requestPath.cluster), requestPath.endpoint, GetPrivilegeStringForLogging(requestPrivilege));
ChipLogProgress(DataManagement,
"AccessControl: checking f=%u a=%c s=0x" ChipLogFormatX64 " t=%s c=" ChipLogFormatMEI " e=%" PRIu16 " p=%c",
subjectDescriptor.fabricIndex, GetAuthModeStringForLogging(subjectDescriptor.authMode),
ChipLogValueX64(subjectDescriptor.subject),
GetCatStringForLogging(buf, sizeof(buf), subjectDescriptor.cats), ChipLogValueMEI(requestPath.cluster),
requestPath.endpoint, GetPrivilegeStringForLogging(requestPrivilege));
}
#endif

// TODO(#13867): this will go away
if (mDelegate.TemporaryCheckOverride())
{
ChipLogProgress(DataManagement, "AccessControl: temporary check override (this will go away)");
return CHIP_NO_ERROR;
}

// Operational PASE not supported for v1.0, so PASE implies commissioning, which has highest privilege.
ReturnErrorCodeIf(subjectDescriptor.authMode == AuthMode::kPase, CHIP_NO_ERROR);
if (subjectDescriptor.authMode == AuthMode::kPase)
{
ChipLogProgress(DataManagement, "AccessControl: implicit admin (PASE)");
return CHIP_NO_ERROR;
}

EntryIterator iterator;
ReturnErrorOnFailure(Entries(iterator, &subjectDescriptor.fabricIndex));
Expand Down Expand Up @@ -297,6 +306,7 @@ CHIP_ERROR AccessControl::Check(const SubjectDescriptor & subjectDescriptor, con
}

// No entry was found which passed all checks: access is denied.
ChipLogProgress(DataManagement, "AccessControl: denied");
return CHIP_ERROR_ACCESS_DENIED;
}

Expand All @@ -317,9 +327,9 @@ bool AccessControl::IsValid(const Entry & entry)
SuccessOrExit(entry.GetSubjectCount(subjectCount));
SuccessOrExit(entry.GetTargetCount(targetCount));

ChipLogDetail(DataManagement, "AccessControl: validating f=%u p=%c a=%c s=%d t=%d", fabricIndex,
GetPrivilegeStringForLogging(privilege), GetAuthModeStringForLogging(authMode), static_cast<int>(subjectCount),
static_cast<int>(targetCount));
ChipLogProgress(DataManagement, "AccessControl: validating f=%u p=%c a=%c s=%d t=%d", fabricIndex,
GetPrivilegeStringForLogging(privilege), GetAuthModeStringForLogging(authMode), static_cast<int>(subjectCount),
static_cast<int>(targetCount));

// Fabric index must be defined.
VerifyOrExit(fabricIndex != kUndefinedFabricIndex, log = "invalid fabric index");
Expand All @@ -342,7 +352,7 @@ bool AccessControl::IsValid(const Entry & entry)
SuccessOrExit(entry.GetSubject(i, subject));
const bool kIsCase = authMode == AuthMode::kCase;
const bool kIsGroup = authMode == AuthMode::kGroup;
ChipLogDetail(DataManagement, " validating subject 0x" ChipLogFormatX64, ChipLogValueX64(subject));
ChipLogProgress(DataManagement, " validating subject 0x" ChipLogFormatX64, ChipLogValueX64(subject));
VerifyOrExit((kIsCase && IsValidCaseNodeId(subject)) || (kIsGroup && IsValidGroupNodeId(subject)), log = "invalid subject");
}

Expand Down Expand Up @@ -376,6 +386,7 @@ AccessControl & GetAccessControl()

void SetAccessControl(AccessControl & accessControl)
{
ChipLogProgress(DataManagement, "AccessControl: setting");
globalAccessControl = &accessControl;
}

Expand Down
3 changes: 3 additions & 0 deletions src/access/AccessControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,9 @@ class AccessControl
virtual void SetListener(Listener & listener) { mListener = &listener; }
virtual void ClearListener() { mListener = nullptr; }

// TODO(#13867): this will go away
virtual bool TemporaryCheckOverride() const { return false; }

private:
Listener * mListener = nullptr;
};
Expand Down
2 changes: 2 additions & 0 deletions src/access/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ static_library("access") {
"SubjectDescriptor.h",
"examples/ExampleAccessControlDelegate.cpp",
"examples/ExampleAccessControlDelegate.h",
"examples/PermissiveAccessControlDelegate.cpp",
"examples/PermissiveAccessControlDelegate.h",
]

cflags = [ "-Wconversion" ]
Expand Down
12 changes: 6 additions & 6 deletions src/access/examples/ExampleAccessControlDelegate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1065,11 +1065,11 @@ class AccessControlDelegate : public AccessControl::Delegate
public:
CHIP_ERROR Init() override
{
ChipLogDetail(DataManagement, "Examples::AccessControlDelegate::Init");
ChipLogProgress(DataManagement, "Examples::AccessControlDelegate::Init");
CHIP_ERROR err = LoadFromFlash();
if (err != CHIP_NO_ERROR)
{
ChipLogDetail(DataManagement, "AccessControl: unable to load stored ACL entries; using empty list instead");
ChipLogProgress(DataManagement, "AccessControl: unable to load stored ACL entries; using empty list instead");
for (auto & storage : EntryStorage::acl)
{
storage.Clear();
Expand All @@ -1080,7 +1080,7 @@ class AccessControlDelegate : public AccessControl::Delegate

CHIP_ERROR Finish() override
{
ChipLogDetail(DataManagement, "Examples::AccessControlDelegate::Finish");
ChipLogProgress(DataManagement, "Examples::AccessControlDelegate::Finish");
return SaveToFlash();
}

Expand Down Expand Up @@ -1140,7 +1140,7 @@ class AccessControlDelegate : public AccessControl::Delegate
CHIP_ERROR saveError = SaveToFlash();
if (saveError != CHIP_NO_ERROR && saveError != CHIP_ERROR_INCORRECT_STATE)
{
ChipLogDetail(DataManagement, "CreateEntry failed to save to flash");
ChipLogProgress(DataManagement, "CreateEntry failed to save to flash");
}
}
return err;
Expand Down Expand Up @@ -1172,7 +1172,7 @@ class AccessControlDelegate : public AccessControl::Delegate
CHIP_ERROR saveError = SaveToFlash();
if (saveError != CHIP_NO_ERROR && saveError != CHIP_ERROR_INCORRECT_STATE)
{
ChipLogDetail(DataManagement, "UpdateEntry failed to save to flash");
ChipLogProgress(DataManagement, "UpdateEntry failed to save to flash");
}
}
return err;
Expand Down Expand Up @@ -1215,7 +1215,7 @@ class AccessControlDelegate : public AccessControl::Delegate
CHIP_ERROR saveError = SaveToFlash();
if (saveError != CHIP_NO_ERROR && saveError != CHIP_ERROR_INCORRECT_STATE)
{
ChipLogDetail(DataManagement, "DeleteEntry failed to save to flash");
ChipLogProgress(DataManagement, "DeleteEntry failed to save to flash");
}
return CHIP_NO_ERROR;
}
Expand Down
80 changes: 80 additions & 0 deletions src/access/examples/PermissiveAccessControlDelegate.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
*
* Copyright (c) 2022 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "PermissiveAccessControlDelegate.h"

namespace {

using namespace chip;
using namespace chip::Access;

using Entry = chip::Access::AccessControl::Entry;
using EntryIterator = chip::Access::AccessControl::EntryIterator;
using Target = Entry::Target;

class AccessControlDelegate : public AccessControl::Delegate
{
public:
CHIP_ERROR Init() override { return CHIP_NO_ERROR; }
CHIP_ERROR Finish() override { return CHIP_NO_ERROR; }

// Capabilities
CHIP_ERROR GetMaxEntryCount(size_t & value) const override
{
value = 0;
return CHIP_NO_ERROR;
}

// Actualities
CHIP_ERROR GetEntryCount(size_t & value) const override
{
value = 0;
return CHIP_NO_ERROR;
}

// Preparation
CHIP_ERROR PrepareEntry(Entry & entry) override { return CHIP_NO_ERROR; }

// CRUD
CHIP_ERROR CreateEntry(size_t * index, const Entry & entry, FabricIndex * fabricIndex) override { return CHIP_NO_ERROR; }
CHIP_ERROR ReadEntry(size_t index, Entry & entry, const FabricIndex * fabricIndex) const override { return CHIP_NO_ERROR; }
CHIP_ERROR UpdateEntry(size_t index, const Entry & entry, const FabricIndex * fabricIndex) override { return CHIP_NO_ERROR; }
CHIP_ERROR DeleteEntry(size_t index, const FabricIndex * fabricIndex) override { return CHIP_NO_ERROR; }

// Iteration
CHIP_ERROR Entries(EntryIterator & iterator, const FabricIndex * fabricIndex) const override { return CHIP_NO_ERROR; }

// TODO(#13867): this will go away
bool TemporaryCheckOverride() const override { return true; }
};

} // namespace

namespace chip {
namespace Access {
namespace Examples {

AccessControl::Delegate & GetPermissiveAccessControlDelegate()
{
static AccessControlDelegate accessControlDelegate;
return accessControlDelegate;
}

} // namespace Examples
} // namespace Access
} // namespace chip
29 changes: 29 additions & 0 deletions src/access/examples/PermissiveAccessControlDelegate.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
*
* Copyright (c) 2022 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#pragma once

#include "access/AccessControl.h"

namespace chip {
namespace Access {
namespace Examples {

AccessControl::Delegate & GetPermissiveAccessControlDelegate();

} // namespace Examples
} // namespace Access
} // namespace chip
12 changes: 0 additions & 12 deletions src/app/CommandHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -274,12 +274,6 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath);
err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege);
if (err != CHIP_NO_ERROR)
{
// Grace period until ACLs are in place
ChipLogError(DataManagement, "AccessControl: overriding DENY (for now)");
err = CHIP_NO_ERROR;
}
if (err != CHIP_NO_ERROR)
{
if (err != CHIP_ERROR_ACCESS_DENIED)
{
Expand Down Expand Up @@ -408,12 +402,6 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath);
err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege);
if (err != CHIP_NO_ERROR)
{
// Grace period until ACLs are in place
ChipLogError(DataManagement, "AccessControl: overriding DENY (for now)");
err = CHIP_NO_ERROR;
}
if (err != CHIP_NO_ERROR)
{
// TODO: handle errors that aren't CHIP_ERROR_ACCESS_DENIED, etc.
continue;
Expand Down
13 changes: 13 additions & 0 deletions src/app/tests/AppTestContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,18 @@

#include <app/tests/AppTestContext.h>

#include <access/AccessControl.h>
#include <access/examples/PermissiveAccessControlDelegate.h>
#include <app/InteractionModelEngine.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/ErrorStr.h>

namespace {

chip::Access::AccessControl permissiveAccessControl(chip::Access::Examples::GetPermissiveAccessControlDelegate());

} // namespace

namespace chip {
namespace Test {

Expand All @@ -28,11 +36,16 @@ CHIP_ERROR AppContext::Init()
ReturnErrorOnFailure(Super::Init());
ReturnErrorOnFailure(chip::app::InteractionModelEngine::GetInstance()->Init(&GetExchangeManager()));

Access::SetAccessControl(permissiveAccessControl);
ReturnErrorOnFailure(Access::GetAccessControl().Init());

return CHIP_NO_ERROR;
}

CHIP_ERROR AppContext::Shutdown()
{
ReturnErrorOnFailure(Access::GetAccessControl().Finish());

chip::app::InteractionModelEngine::GetInstance()->Shutdown();
ReturnErrorOnFailure(Super::Shutdown());

Expand Down
1 change: 1 addition & 0 deletions src/app/tests/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ static_library("helpers") {
cflags = [ "-Wconversion" ]

deps = [
"${chip_root}/src/access",
"${chip_root}/src/lib/support",
"${chip_root}/src/messaging/tests:helpers",
"${chip_root}/src/transport/raw/tests:helpers",
Expand Down
5 changes: 3 additions & 2 deletions src/app/tests/TestCommandInteraction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,8 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(n
chip::isCommandDispatched = false;
GenerateInvokeRequest(apSuite, apContext, commandDatabuf, false /*aNeedCommandData*/, messageIsTimed);
err = commandHandler.ProcessInvokeRequest(std::move(commandDatabuf), transactionIsTimed);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR && chip::isCommandDispatched == (messageIsTimed == transactionIsTimed));
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(apSuite, chip::isCommandDispatched == (messageIsTimed == transactionIsTimed));
}
}
}
Expand Down Expand Up @@ -767,7 +768,7 @@ const nlTest sTests[] =
// clang-format off
nlTestSuite sSuite =
{
"TestReadInteraction",
"TestCommandInteraction",
&sTests[0],
TestContext::Initialize,
TestContext::Finalize
Expand Down
Loading

0 comments on commit dd13dbe

Please sign in to comment.