Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enforce access control #14454

Closed
mlepage-google opened this issue Jan 27, 2022 · 1 comment
Closed

Enforce access control #14454

mlepage-google opened this issue Jan 27, 2022 · 1 comment
Assignees
@mlepage-google
Labels
acl Access Control feature V1.0

Comments

@mlepage-google
Copy link
Contributor

mlepage-google commented Jan 27, 2022 •
edited
Loading

Access control is present but not enforced.

AccessControl::Check is being called but the result is being ignored, and all access is being allowed and not denied where appropriate.

We're just about at the point where it can be "turned on" so access control is being enforced. Probably it's wise to turn it on in a "warning in the logs" mode first, and give a grace period for clients (including integration tests and so on) to ensure they are adding all the ACLs they will need to, so access isn't denied when it's turned on in "enforce and deny for real" mode.

"Turning on" means going into the places where AccessControl::Check is called (mostly in ember-compatibility-functions.cpp and CommandHandler.cpp) and removing a subsequent "override" of its return value to CHIP_NO_ERROR, so it's not discarded.
@mlepage-google mlepage-google added acl Access Control feature V1.0 labels Jan 27, 2022
@mlepage-google mlepage-google mentioned this issue Jan 27, 2022
Closed
mlepage-google added a commit to mlepage-google/connectedhomeip that referenced this issue Jan 28, 2022
@mlepage-google
Add access control logging
4b838dc 
Add a log statement in AccessControl::Check so we can see what
params are being used to do the check.

This looks like:
AccessControl: checking f=1 a=c s=0xbc5c01 t= c=0x30 e=0 p=o

With some CATs:
AccessControl: checking f=1 a=c s=0xbc5c01 t=0xaaaa0001,0xbbbb0002 c=0x30 e=0 p=o

Where AccessControl::Check is called, if there's an error,
override it for now (grace period) but log it as an error:

This looks like:
AccessControl: overriding DENY (for now)

Progress toward issue project-chip#14454
@mlepage-google mlepage-google mentioned this issue Jan 28, 2022
Merged
@andy31415 andy31415 added v1_triage_split_8 V1.0 acl Access Control feature and removed V1.0 acl Access Control feature labels Jan 30, 2022
mlepage-google added a commit that referenced this issue Feb 1, 2022
@mlepage-google
Add access control logging (#14540)
c25c90d 
Add a log statement in AccessControl::Check so we can see what
params are being used to do the check.

This looks like:
AccessControl: checking f=1 a=c s=0x0000000000BC5C01 t= c=0x0000_0030 e=0 p=o
AccessControl: checking f=1 a=c s=0x0000000000BC5C01 t=0xAAAA0001,0xBBBB0002 c=0x0000_0030 e=0 p=o

Where AccessControl::Check is called, if there's an error,
override it for now (grace period) but log it as an error.

This looks like:
AccessControl: overriding DENY (for now)

Progress toward issue #14454
mlepage-google added a commit to mlepage-google/connectedhomeip that referenced this issue Feb 9, 2022
@mlepage-google
Enforce access control
a6f811e 
Issue project-chip#14454
This was referenced Feb 9, 2022
Merged
Closed
andy31415 pushed a commit that referenced this issue Feb 17, 2022
@mlepage-google
Enforce access control (#14921)
ec939d9 
* 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
@mlepage-google
Copy link
Contributor Author

This is done now via PR #14921 which does:

  • change some of the logging from "detail" to "progress" (at least for short term)
  • enforce AccessControl::Check results (the point of this PR)
  • but provide a TemporaryCheckOverride in the AccessControl::Delegate (false by default)
  • and provide Examples::PermissiveAccessControlDelegate (with override set to true)
  • which is set/configured in AppTestContext so the IM unit tests pass
  • build files and dependencies updated accordingly

There remains one more place to integrate access control, for events, but that is being tracked separately by issue #14326.

jamesluo11 pushed a commit to jamesluo11/connectedhomeip that referenced this issue Apr 26, 2022
@mlepage-google @fuxingguo16
Enforce access control (project-chip#14921)
dba7c8e 
* Enforce access control

Issue project-chip#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mlepage-google mlepage-google

Labels
acl Access Control feature V1.0
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@andy31415 @cecille @mlepage-google