-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Allow including access lists in other access lists #38738
Conversation
fa58f2a
to
7e449e3
Compare
c9bda8d
to
91b8d4e
Compare
91b8d4e
to
95d5adb
Compare
Is there an RFD or other design document for this feature somewhere? My gut reaction to the idea of access list recursion (or cross-dependency of any kind) is that it seems risky and hard to reason about. In general, I'm of the opinion that composition of access-control primitives should have a well-defined acyclic hierarchy wherever possible. One of the key design goals of a good access-control system should be to mitigate/prevent human error, both on the part of the programmer and the end user. Keeping the compositional structure as simple as possible to trace, reason about, and model is an important part of that. |
There isn't an RFD but I'm happy to write one up. Keeping the hierarchy acyclic seems like it would be ideal, however I'm not sure if the services being integrated with (Azure Entra groups @justinas) support cycles in their group hierarchies |
Entra (sadly) seems to support cycles. Generally we'd like to represent the imported data as faithfully as possible. Maybe we could assume that cycles are uncommon in real-life scenarios and refuse to process such data, but that decision would not be up to me I think 🙂 /cc @jakule |
Maybe having it require --force flag in the terminal/a warning in the ui, if the data being imported or acl being modified introduces a cycle would be useful? or maybe that would just be unnecessary overhead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lxea @justinas @fspmarshall @jakule We do need to support nested access lists (both EntraID and Github can have nested groups/teams) but I agree that cycles ("A" is member of "B", "B" is a member of "C", "C" is a member of "A") add too much complexity in the system so let's prevent them and see if anyone complains. I can't come up with a reasonable real-world scenario where such group membership could be useful.
Let's also not call them "recursive". Let's call them "nested".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drive-by review, as this is related to other stuff I was just looking at. Added a couple questions.
95d5adb
to
b4271ad
Compare
46692ee
to
e62c03c
Compare
@lxea Is this implementation ready for re-review now that the RFD is approved, have you applied all changes discussed in the RFD? |
e62c03c
to
1d50e98
Compare
This comment was marked as outdated.
This comment was marked as outdated.
2750b21
to
cb04fa3
Compare
🤖 Vercel preview here: https://docs-awm8nb9xb-goteleport.vercel.app/docs/ver/preview |
7ae81a9
to
69f66f6
Compare
69f66f6
to
79a725f
Compare
- Recursively check for accesslist membership - Allow adding/removing/listing included access lists in acl commands - Add a recursive test - Use dynamic access lists structure from RFD - Resolve proto changes - Exclude 'list' members from Access List memberCount - Calc Access List member count with members of type 'list' excluded, return seperately to front end - Update examples/integrations - Update crd docs - Update tf docs - Perform calculation of inherited roles/traits to AccessList service in order to utilize cache and minimize number of requests. - Grant Okta integration RO for Access Lists - Update AccessListMember-* events - Include count for inherited grants - Include MembershipKind of affected member(s) - Emit inherited grants / members' MembershipKind for AccessListMember-* events - Update notified owners for Access Requests - Ensure dynamic owners are notified for Access Requests - Ensure dynamic owners are notified via Slack integration - Optionally pass an AbortSignal to `fetchAccessLists` in Web UI - Replace usages of `services.IsAccessListOwner/IsAccessListMember` with equivelant funcs from `Hierarchy` - Remove final references to AccessListMembershipChecker - Don't allow ACL deletion when member/owner in other lists - Guard Access List deletion behind membership/ownership checks for List - Expose Hierarchy func to recursively get all members - Tidy UserLoginStateGenerator logic involving ACL Membership/Ownership
79a725f
to
1b665aa
Compare
Allows recursive inclusion of Access Lists in other Access Lists.
For example, with the hierarchy below,
user A
would inherit traits and roles fromacl C
,acl B
, andacl A
, whileuser B
would traits and roles fromacl C
andacl B
:changelog: Allow nested inclusion of Access Lists as Members and Owners in other Access Lists
RFD: https://github.com/gravitational/teleport/blob/master/rfd/0170-nested-accesslists.md
E: https://github.com/gravitational/teleport.e/pull/4549
Depends on #47828.