RFD: Introduce nested access list membership

[lxea]

related pr #38738

[r0mant]

Let's call out in the RFD explicitly that we don't support cyclic memberships: #38738 (review).

[r0mant]

I think it's also a bit inaccurate to call this "recursive" access lists. Let's call it "nested" instead. Same comment for the terminology used throughout the doc.

[r0mant]

Overall makes sense but this only covers how user permissions will be calculated. We also need to think through the impact of this feature on other access list related features like access requests and access reviews.

Also, can you add a section covering IaC - will it require any work to support this, or will it work out of the box? Check with @hugoShaka if any changes to Terraform provider of Kube operator will be needed.

[r0mant]

How does this affect the access request flow, will similar traversal happen there when calculating suggested access lists? For example, say there is an access list "Dev Team" that gives access to server S1. Then, say "Dev Team" contains another list "Core Team" that gives access to server S2.

Now Alice requests access to server S1. Will the request reviewer (if they're an owner of both lists) be able to promote her to either one or just the "Dev Team"? Can you add a section to the RFD that clarifies this.

[lxea]

I'm not super experienced in how the access requests and access lists interact

In this case because alice is requesting server S1 which Dev team owners can give access to, is she actually requesting to be on the list or requesting the resource s1?

The way its implemented now, user permissions are calculated in IsAccessList(Member/Owner) and I have implemented it to just look up the tree in there, so its fairly transparent to access requests, as to whether a user is a member/owner via nesting or direct membership. I think this should also apply to calculating the suggested reviewers below

[r0mant]

Similar question to the one I left below - how does this affect "suggested reviewers" functionality? Right now we detect if there's an access list that grants resources you're requesting and puts that list owners as suggested reviewers.

With nested lists, will we take owners from the list that grants actual resource, plus owners from nested lists?

[jakule]

Maybe this is a basic question, but why do we need member and owners list separate? I'd assume that an access list has "child" access lists and the owners and members are inherited without modifications.

[justinas]

For ownership of ALs imported from Entra, it is a plan to have something simple for now, e.g. a static list of owners ["alice", "bob"] for all imported access lists, so for Entra the owner_access_lists is not particularly relevant.

[lxea]

I think it makes sens to only have the one way to include access lists 👍

[r0mant]

@lxea We need to keep separate access lists for owners and members like you had originally. Otherwise, how do I set IT-Team access list as an owner, and Dev-Team as a member?

[smallinsky]

Themember_access_lists and owner_access_lists approuch looks reasonable by wonder about extensibility.

Right now the member_access_lists is static property that can refer only to access list members.

So we have:

member_access_lists
 - "access_list_id"
owner_access_lists
- "access_list_id"

Does it make sense to build a flexible solution allowing to nested access list like below where members and owners properties are crafted for extensibility so in the future we can add additional types:

dynamic_members: 
 // A user becomes a access list members if is member of access list Y
 - access_list_members
    - Y
 // A user becomes a access list member if is owner of access list Z
 - access_list_owners:
     - Z
 // A user becomes a access list member if has role editor
 - roles:
    - editor
 - user_traits:
     - name: "org"
        value: "dev"

but for now we can implement

dynamic_members: 
 - access_list_members:
    - access_list_id

and

dynamic_owners: 
 - access_list_owners:
    - access_list_id

WDYT @lxea @r0mant ?

[lxea]

That looks like a good system to me 👍

[r0mant]

Forgot to reply here earlier but Marek's proposed structure looks good to me too.

[jakule]

I'm not sure if I understand this point. What looping is? I'd assume that an access list would fetch all parents, merge them into one set of rules an run all checks against it.
Also, are we going to support cycles? If no, how are we going to prevent them?

[justinas]

I'm reading this "looping" as a synonym for cycles.

[jakule]

I don't think that looping has anything to do with cycles. for(i = 0; i < 10; i++) and while(true) are both loops where the first one will end and the second will loop forever.
All graph algorithms terminate on DAG, but a cycle make cause an algorithm to never terminate.
@justinas AFAIR you mentioned that Entra allows cycles in the setup? If this true, then we won't be able to import all roles.

[justinas]

@justinas AFAIR you mentioned that Entra allows cycles in the setup? If this true, then we won't be able to import all roles.

That's true, Entra does not limit you from making cycles A -> B -> A from groups. IMO it should not be a common pattern, so I'd be fine with not supporting importing such cycles, but that would technically mean we are not producing an accurate view of the directory.

[lxea]

I meant looping in terms of a cycle in a graph, just misnamed it here. We just want to avoid it I think to avoid introducing overly complicated configuration options, with the hope that they're not too common, if they are required by somone, it shouldnt be too difficult to implement it

[justinas]

In general, looks good to me in relation with Entra groups.

[smallinsky]

Themember_access_lists and owner_access_lists approuch looks reasonable by wonder about extensibility.

Right now the member_access_lists is static property that can refer only to access list members.

So we have:

member_access_lists
 - "access_list_id"
owner_access_lists
- "access_list_id"

Does it make sense to build a flexible solution allowing to nested access list like below where members and owners properties are crafted for extensibility so in the future we can add additional types:

dynamic_members: 
 // A user becomes a access list members if is member of access list Y
 - access_list_members
    - Y
 // A user becomes a access list member if is owner of access list Z
 - access_list_owners:
     - Z
 // A user becomes a access list member if has role editor
 - roles:
    - editor
 - user_traits:
     - name: "org"
        value: "dev"

but for now we can implement

dynamic_members: 
 - access_list_members:
    - access_list_id

and

dynamic_owners: 
 - access_list_owners:
    - access_list_id

WDYT @lxea @r0mant ?

[smallinsky]

Does members and owners from nested access list will be verified against MembershipRequires and OwnershipRequires ?

[r0mant]

lgtm, let's get your WIP PR wrapped up and updated with the point discussed in this RFD and

[r0mant]

Don't forget to assign the number, in the file name as well.

[smallinsky]

@lxea Could you align the RFD and switch to dynamic_members instead of member_access_lists field ?

Otherwise LGTM.

[lxea]

@r0mant @smallinsky Members of lists have the reason they were added/who they were added by in the datastore, should we encode this information in the dynamic access list section or should it be stored elsewhere, or not included at all?

[r0mant]

@lxea Hmm yeah you're right, they also have TTL just like other regular members right? I wonder if we need to revisit the data model a bit and instead of encoding them directly into access list resource store them separately as also a "AccessListMember" resource (or whatever it's called) with all the same metadata as individual user members.