Allow accesslist cycles pt.1

[hugoShaka]

This PR:

• adds a cycle-proof walk function that can be used to traverse the accesslist graph
• use walk in IsAccessListMember to make it cycle-proof
• uncomments the cycle tests in hierarchy_test.go

[smallinsky]

Could you rise a PR with the alignment of design change in terms of access list cycles functionality to the https://github.com/gravitational/teleport/blob/ff91dc086596669fb9e1f1e972b1b13def2f0c9c/rfd/0170-nested-accesslists.md#cycles ?

[smallinsky]

looks reasonable but want to have a final look before approving with fresh head on Monday.

[kopiczko]

Round 2

[kopiczko]

I'm still not a fan of this visitor struct because we pass the context as a field. I understand this is just a fancy way to pass parameters and avoid single indentation level. I guess you can reduce indentation by extracting visitMember/processMember.

[hugoShaka]

I was planning to reuse the visitor and add another function to collect owners, hence the dedicated struct. I can get rid of this if this is a blocker.

[kopiczko]

If it's supposed to be expanded I'm OK with keeping but, but let's not store the context inside the struct, because it creates a potential for it to escape the function call stack.

[hugoShaka]

I updated the PR based on your feedback:

• the IsAccessListMember function signature now differentiate between "an error happened" and "no valid access path to user, here are some suspects" to make sure we don't misinterpret the error. Every call site was updated.
• IsAccessListMembernow indicates potential issues (expired memberships, unmet requirements) so we can explain and test access decisions. Some parts of the graph are still filtered out, but we will be able to say "this graph part was ignored because user doesn't meet requirements for list A"
• the visitor now validates nested access list membership expiry, this is also covered in tests

Things that I will do that are non-blocking for the review:

• as suggested by @kopiczko: upstream nested expired membership and unmet requirements tests separately, in a PR that will be merged before
• send a PR to update the nested AL RFD to mention that we will be allowing cycles
• send 2 other PRs:
  • one for IsAccessListOwner so it uses the same iterator
  • one for GetMembersOf so it uses the iterate with the "allow all" filter function

Note: this PR changes the IsAccessListMember signature and will need some e counterpart.

[smallinsky]

Can we do https://github.com/gravitational/teleport/pull/60034/files#r2419292154

I don't see this as a blocker but for sure it will help the reviewer to understand the behavior change.

@kopiczko @kiosion could you also take a look.

[hugoShaka]

Can we do https://github.com/gravitational/teleport/pull/60034/files#r2419292154

Tests and signature change are already in their own PR waiting your review:

• Add nested AL tests + change IsAccessListMember behaviour #60271
• https://github.com/gravitational/teleport.e/pull/7423

Once those are merged I will rebase the current PR so the test changes will not show up in the diff.

[r0mant]

First pass, nothing major from my side so far.

[kopiczko]

I'm thinking the rage function is not a good fit here and the flow suffers because we introduce some patters that are not typical in Go stdlib. But I'm approving regardless because I don't want to get you stuck and visitor is private which means we can rework it without a big penalty.

Having said that, thank you for being patient with the explanations and addressing the comments.

[kopiczko]

Some more minor comments about the visitor internals

[hugoShaka]

@kopiczko I sent 2 more commits addressing your remaining feedback:

• storing state in the walk function state instead of the struct
• moving to walk-like semantics

Can you take another look?

[kopiczko]

Thanks for taking the time to rewrite it. To me it looks way easier to follow now.

I left a bunch of comments, but they mostly circle around the same thing.