vmware / vmware/pinniped

ActiveDirectoryIdentityProvider should support nested group searches with a maximum depth

Open
#759 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

priority/undecided
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.

  1. 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.

  2. Specify maxNestedDepth, leave filter blank
    It doesn't really make sense to specify member:1.2.840.113556.1.4.1941:={}) anymore if you only want some, so the search filter should be (&(objectClass=group)(member={}))

  3. Specify filter, leave maxNestedSearch blank
    Don't limit how deeply to recurse.

  4. Specify both
    There's no defaulting we need to do. Just search as many levels as specified.

Questions

  1. Is it possible to end up infinitely recursing?
  2. How similar would this be if we wanted to port it over to generic LDAP? How useful would it be?
  3. 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.