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

Allow including access lists in other access lists #38738

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

lxea
Copy link
Contributor

@lxea lxea commented Feb 28, 2024

Allows recursive inclusion of Access Lists in other Access Lists.

For example, with the hierarchy below, user A would inherit traits and roles from acl C, acl B, and acl A, while user B would traits and roles from acl C and acl B:

flowchart TD
    A[acl A] --has member-->B(user A)
    C[acl B] --has member-->A
    C --has member--> D(user B)
    E[acl C] --has member-->C
Loading

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.

@mdwn mdwn self-requested a review February 28, 2024 18:28
@lxea lxea force-pushed the lxea/recursive-accesslists branch 2 times, most recently from fa58f2a to 7e449e3 Compare April 4, 2024 12:48
api/proto/teleport/accesslist/v1/accesslist.proto Outdated Show resolved Hide resolved
api/types/accesslist/accesslist.go Outdated Show resolved Hide resolved
@lxea lxea force-pushed the lxea/recursive-accesslists branch 5 times, most recently from c9bda8d to 91b8d4e Compare April 18, 2024 13:54
@lxea lxea marked this pull request as ready for review April 18, 2024 13:56
@github-actions github-actions bot added size/md tctl tctl - Teleport admin tool labels Apr 18, 2024
@gravitational gravitational deleted a comment from github-actions bot Apr 18, 2024
@lxea lxea force-pushed the lxea/recursive-accesslists branch from 91b8d4e to 95d5adb Compare April 18, 2024 14:09
@fspmarshall
Copy link
Contributor

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.

@lxea
Copy link
Contributor Author

lxea commented Apr 19, 2024

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

@justinas
Copy link
Contributor

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

@lxea
Copy link
Contributor Author

lxea commented Apr 19, 2024

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

Copy link
Collaborator

@r0mant r0mant left a 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".

Copy link
Contributor

@codingllama codingllama left a 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.

api/proto/teleport/accesslist/v1/accesslist.proto Outdated Show resolved Hide resolved
api/proto/teleport/accesslist/v1/accesslist.proto Outdated Show resolved Hide resolved
api/proto/teleport/accesslist/v1/accesslist.proto Outdated Show resolved Hide resolved
@lxea lxea force-pushed the lxea/recursive-accesslists branch from 95d5adb to b4271ad Compare May 10, 2024 13:38
@lxea lxea force-pushed the lxea/recursive-accesslists branch 3 times, most recently from 46692ee to e62c03c Compare June 4, 2024 15:25
@r0mant
Copy link
Collaborator

r0mant commented Jun 6, 2024

@lxea Is this implementation ready for re-review now that the RFD is approved, have you applied all changes discussed in the RFD?

@lxea lxea force-pushed the lxea/recursive-accesslists branch from e62c03c to 1d50e98 Compare June 7, 2024 14:00

This comment was marked as outdated.

Copy link

🤖 Vercel preview here: https://docs-awm8nb9xb-goteleport.vercel.app/docs/ver/preview

@kiosion kiosion force-pushed the lxea/recursive-accesslists branch 2 times, most recently from 7ae81a9 to 69f66f6 Compare October 31, 2024 00:14
@kiosion kiosion added this pull request to the merge queue Nov 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 1, 2024
@kiosion kiosion added this pull request to the merge queue Nov 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 1, 2024
@kiosion kiosion added this pull request to the merge queue Nov 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 1, 2024
@kiosion kiosion added this pull request to the merge queue Nov 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 1, 2024
@kiosion kiosion added this pull request to the merge queue Nov 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 1, 2024
@kiosion kiosion added this pull request to the merge queue Nov 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 1, 2024
- 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
@kiosion kiosion added this pull request to the merge queue Nov 1, 2024
Merged via the queue into master with commit fa97b8f Nov 1, 2024
44 checks passed
@kiosion kiosion deleted the lxea/recursive-accesslists branch November 1, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/md tctl tctl - Teleport admin tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants