ActiveDirectoryIdentityProvider should support nested group searches with a maximum depth
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 739
- Forks
- 85
- Avg merge
- 5d 5h
- Merged PRs (30d)
- 1
Description
Is your feature request related to a problem? Please describe.
Many AD users use nested group membership. For example, user pinny could be a member of the group seals, and the group seals is a member of the group marine mammals which is a member of the group animals. But searching through all of the groups that your groups are members of can be a costly recursive operation for domains with a lot of users and deeply nested groups.
Describe the solution you'd like
Rather than always searching all nested groups that a user belongs to, pinniped users should be able to specify how many levels to recurse when searching for nested groups.
To get all nested groups, you can use active directory's special LDAP_MATCHING_RULE_IN_CHAIN matching rule.
However, that syntax doesn't allow for specifying how deeply to search in order to say that you want a user's direct groups, and the groups that those groups are a part of, but not the groups that the groups' groups are a part of.
We would have to write our own recursing logic instead of relying on the matching rule.
For the API, I propose we add an optional integer field to the ActiveDirectoryIdentityProvider's spec.groupSearch called maxNestedDepth or something like it. To say that you don't want nesting at all, the maxNestedDepth should be 1 (or maybe 0? What's a reasonable way to index this?)
apiVersion: idp.supervisor.pinniped.dev/v1alpha1
kind: ActiveDirectoryIdentityProvider
...
spec:
...
groupSearch:
base: "ou=Groups,DC=activedirectory,DC=example,DC=com"
filter: "&(objectClass=group)(member={})"
# only search 5 levels deep
maxNestedDepth: 5
# Specify the name of the Kubernetes Secret that contains your Active Directory
# bind account credentials. This service account will be used by the
# Supervisor to perform LDAP user and group searches.
bind:
secretName: "active-directory-bind-account"
Default Values
Because we want most AD values to be totally optional with good defaults, there are 4 different ways to configure the filter and maxNestedDepth.
-
Leave both blank
By default we could set the same behavior as we have now: don't limit how deeply we recurse. This is the easiest behavior that should work great on small AD instances and avoids the problem of users wondering why they aren't getting all their groups. Implementation wise we could use the same matching rule or we could use our own recursion logic. -
Specify maxNestedDepth, leave filter blank
It doesn't really make sense to specifymember:1.2.840.113556.1.4.1941:={})anymore if you only want some, so the search filter should be(&(objectClass=group)(member={})) -
Specify filter, leave maxNestedSearch blank
Don't limit how deeply to recurse. -
Specify both
There's no defaulting we need to do. Just search as many levels as specified.
Questions
- Is it possible to end up infinitely recursing?
- How similar would this be if we wanted to port it over to generic LDAP? How useful would it be?
- How can we make it backwards compatible to users who already specified a filter that disabled nested group search by removing the matching rule (if this feature lands in a different release than the main Active Directory).
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reviewing the ActiveDirectoryIdentityProvider spec.groupSearch behavior and the current nested-group search implementation. Resolve the depth indexing, default behavior, recursion termination, and backward-compatibility questions before defining the API and tests needed to verify bounded and unbounded searches.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- api, authentication
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100