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.

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

PASE support for access control #10242

Closed
mlepage-google opened this issue Oct 5, 2021 · 8 comments
Closed

PASE support for access control #10242

mlepage-google opened this issue Oct 5, 2021 · 8 comments
Assignees
Labels
acl Access Control feature V1.0

Comments

@mlepage-google
Copy link
Contributor

mlepage-google commented Oct 5, 2021

  • SubjectDescriptor basic implementation was added in initial prototype (Initial ACL prototype implementation #10236)
  • this task is to add PASE authmode support
  • passcode ID needs to be set
  • fabric index needs to be set (only 0?) (update: no not only 0)
  • does fabric index need to change during commissioning (when subject gets onto fabric, but hasn't yet set up CASE)? (update: yes)
  • commissioning flag needs to be set (or should it be obtained from another location?)
  • implicit admin for PASE commissioning subject in AccessControl::Check
@mlepage-google mlepage-google added the acl Access Control feature label Oct 5, 2021
@vijs vijs self-assigned this Nov 15, 2021
@mlepage-google
Copy link
Contributor Author

Current approach will be to store PASE verifier IDs in type NodeId in the subject descriptor, since the upper bits have discriminating ranges we can use to tell what "type" of NodeId it is.

@mlepage-google
Copy link
Contributor Author

PR #13003 provides a place to do this.

In theory it's done for the PASE session and verifier ID (as subject), but upstream code needs some fixes before it works (needs to provide the SessionHandle.mPeerNodeId with discriminating high bits set properly).

In addition, SubjectDescriptor.fabricIndex needs to be set properly, because there are some cases during commissioning where a fabric is available (e.g. to create ACL entries for that fabric), before switching to CASE. This isn't done yet, in part because it also needs to be tested.

mlepage-google added a commit to mlepage-google/connectedhomeip that referenced this issue Jan 7, 2022
Peer node ID for PASE sessions is the passcode ID with discriminant in
the top bits (to discriminate it from other kinds of node ID).

Also, passcode ID cannot be zero in non-default commissioning cases.

Progress towards project-chip#10242
andy31415 pushed a commit that referenced this issue Jan 7, 2022
Peer node ID for PASE sessions is the passcode ID with discriminant in
the top bits (to discriminate it from other kinds of node ID).

Also, passcode ID cannot be zero in non-default commissioning cases.

Progress towards #10242
mlepage-google added a commit to mlepage-google/connectedhomeip that referenced this issue Jan 19, 2022
PASE sessions start with no fabric, but during commissioning,
after OperationalCredentialsCluster::AddNOC, they should be
placed on the newly commissioned fabric so administrative
actions pertaining to the newly commissioned fabric can be
performed over the PASE session, if desired.

Work towards issue project-chip#10242
andy31415 pushed a commit that referenced this issue Jan 20, 2022
* Place PASESession on new fabric

PASE sessions start with no fabric, but during commissioning,
after OperationalCredentialsCluster::AddNOC, they should be
placed on the newly commissioned fabric so administrative
actions pertaining to the newly commissioned fabric can be
performed over the PASE session, if desired.

Work towards issue #10242

* Change SecureSession::NewFabric to alter mFabric

This makes it easier to also have it be the accessing fabric after the
change.
@mlepage-google
Copy link
Contributor Author

mlepage-google commented Jan 27, 2022

Almost everything in the list is done.

There's still some churn on migrating the PASE session to the new fabric index. (E.g. see PR #13765)

Still need to handle the "implicit PASE admin during commissioning" in AccessControl::Check, including getting a commissioning flag down there, so we can ensure the PASE session in the subject descriptor is the one doing commissioning.

But actually, there appears to be a desire to just edit the spec to disallow explicit PASE ACL entries for v1 (because PASE is only for commissioning, and multi-fabric is tricky with PASE), so if we do this spec change, and enforce that PASE is only able to connect during commissioning, we can just simply do "any PASE subject gets admin privilege" in AccessControl::Check.
https://csamembers.slack.com/archives/C01B5L0LAJC/p1643144928040000?thread_ts=1643144146.039400&cid=C01B5L0LAJC

@mlepage-google mlepage-google changed the title Add PASE support to SubjectDescriptor PASE support for access control Jan 27, 2022
@mlepage-google
Copy link
Contributor Author

Spec change for reserving explicit PASE ACL entries for future use:
https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/4878

selissia pushed a commit to selissia/connectedhomeip that referenced this issue Jan 28, 2022
* Place PASESession on new fabric

PASE sessions start with no fabric, but during commissioning,
after OperationalCredentialsCluster::AddNOC, they should be
placed on the newly commissioned fabric so administrative
actions pertaining to the newly commissioned fabric can be
performed over the PASE session, if desired.

Work towards issue project-chip#10242

* Change SecureSession::NewFabric to alter mFabric

This makes it easier to also have it be the accessing fabric after the
change.
mlepage-google added a commit to mlepage-google/connectedhomeip that referenced this issue Jan 28, 2022
We won't have explicit operational PASE ACL entries
for v1.0. We will enforce that PASE is only during
commissioning, therefore all PASE subjects will be
granted administer privilege.

Past v1.0, if/when we want operational PASE (requires
solving some tricky multi-fabric issues), we'll have
to check against PASE subjects in entries, and also
for implicite PASE administer privilege during
commissioning we'll have to verify that the incoming
PASE subject is commissioning (otherwise it should
not get that implicit privilege escalation).

Part of issue project-chip#10242
@mlepage-google
Copy link
Contributor Author

mlepage-google commented Jan 28, 2022

PR #14550 will affect the required change in AccessControl::Check, should not go in until the spec change goes in.

We'll need to double check that PASE sessions are only ever allowed during commissioning for v1.0 (and that this is enforced).

If we only ever allow them on PAKE Verifier Key ID 0, I can add a verify/check for that as well.

bzbarsky-apple pushed a commit that referenced this issue Jan 29, 2022
* Disallow operational PASE in AccessControl::Check

We won't have explicit operational PASE ACL entries
for v1.0. We will enforce that PASE is only during
commissioning, therefore all PASE subjects will be
granted administer privilege.

Past v1.0, if/when we want operational PASE (requires
solving some tricky multi-fabric issues), we'll have
to check against PASE subjects in entries, and also
for implicite PASE administer privilege during
commissioning we'll have to verify that the incoming
PASE subject is commissioning (otherwise it should
not get that implicit privilege escalation).

Part of issue #10242

* Update unit tests

* Use unused variable in test code

To avoid warning about unused variable.
@andy31415 andy31415 added v1_triage_split_2 V1.0 acl Access Control feature and removed V1.0 acl Access Control feature labels Jan 30, 2022
@mlepage-google
Copy link
Contributor Author

mlepage-google commented Jan 31, 2022

If we only ever allow them on PAKE Verifier Key ID 0, I can add a verify/check for that as well.

OK since non-zero passcode ID for PASE sessions during commissioning is required for multi-fabric-admin, we cannot have any checks to ensure passcode ID is 0 (because it isn't always 0). But, it is always commissioning. (Still needs to be enforced that it's only for commissioning, because we're going to be granting it administer privileges.)

@mlepage-google
Copy link
Contributor Author

Final piece of the puzzle is tracked in issue #14604.

@mlepage-google
Copy link
Contributor Author

All work for this in v1 is done in access control, anything dangling is tracked in other issues.

step0035 pushed a commit to hank820/connectedhomeip that referenced this issue Feb 8, 2022
Peer node ID for PASE sessions is the passcode ID with discriminant in
the top bits (to discriminate it from other kinds of node ID).

Also, passcode ID cannot be zero in non-default commissioning cases.

Progress towards project-chip#10242
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acl Access Control feature V1.0
Projects
None yet
Development

No branches or pull requests

4 participants