From 72087470721b3af3707715beda0e7d46e5f5320d Mon Sep 17 00:00:00 2001 From: EthanDieterich <139397073+EthanDieterich@users.noreply.github.com> Date: Sat, 21 Jun 2025 05:08:11 -0500 Subject: [PATCH] Add LDAP parent groups search, Active Directory Hierarchy (#4113) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit enables universal nested group search support across a variety of LDAP server implementations. It updates the code to allow recursive group membership discovery during user authentication and provides CI tests to validate the functionality. Based on @paroque’s original https://github.com/dexidp/dex/pull/1058 PR. - Removed `Recursive` boolean flag from config and logic - Made recursion behavior dependant on presence of `RecursionGroupAttr` - Updated log messages to reflect changes and follow `slog` structured format Signed-off-by: Ethan Dieterich --- connector/ldap/ldap.go | 144 ++++++++++++++++++++-------- connector/ldap/ldap_test.go | 50 ++++++++++ connector/ldap/testdata/schema.ldif | 60 ++++++++++++ 3 files changed, 216 insertions(+), 38 deletions(-) diff --git a/connector/ldap/ldap.go b/connector/ldap/ldap.go index 856949d2..9fe386c6 100644 --- a/connector/ldap/ldap.go +++ b/connector/ldap/ldap.go @@ -62,6 +62,8 @@ import ( type UserMatcher struct { UserAttr string `json:"userAttr"` GroupAttr string `json:"groupAttr"` + // Look for parent groups + RecursionGroupAttr string `json:"recursionGroupAttr"` } // Config holds configuration options for LDAP logins. @@ -144,6 +146,8 @@ type Config struct { UserAttr string `json:"userAttr"` GroupAttr string `json:"groupAttr"` + RecursionGroupAttr string `json:"recursionGroupAttr"` + // Array of the field pairs used to match a user to a group. // See the "UserMatcher" struct for the exact field names // @@ -197,8 +201,9 @@ func userMatchers(c *Config, logger *slog.Logger) []UserMatcher { logger.Warn(`use "groupSearch.userMatchers" option instead of "userAttr/groupAttr" fields`, "deprecated", true) return []UserMatcher{ { - UserAttr: c.GroupSearch.UserAttr, - GroupAttr: c.GroupSearch.GroupAttr, + UserAttr: c.GroupSearch.UserAttr, + GroupAttr: c.GroupSearch.GroupAttr, + RecursionGroupAttr: c.GroupSearch.RecursionGroupAttr, }, } } @@ -591,57 +596,120 @@ func (c *ldapConnector) groups(ctx context.Context, user ldap.Entry) ([]string, return nil, nil } - var groups []*ldap.Entry + var groupNames []string + for _, matcher := range c.GroupSearch.UserMatchers { + // Initial Search + var groups []*ldap.Entry for _, attr := range c.getAttrs(user, matcher.UserAttr) { - filter := fmt.Sprintf("(%s=%s)", matcher.GroupAttr, ldap.EscapeFilter(attr)) - if c.GroupSearch.Filter != "" { - filter = fmt.Sprintf("(&%s%s)", c.GroupSearch.Filter, filter) + obtained, filter, err := c.queryGroups(ctx, matcher.GroupAttr, attr) + if err != nil { + return nil, err + } + gotGroups := len(obtained) != 0 + if !gotGroups { + // TODO(ericchiang): Is this going to spam the logs? + c.logger.Error("ldap: groups search returned no groups", "filter", filter) } + groups = append(groups, obtained...) + } - req := &ldap.SearchRequest{ - BaseDN: c.GroupSearch.BaseDN, - Filter: filter, - Scope: c.groupSearchScope, - Attributes: []string{c.GroupSearch.NameAttr}, + // If RecursionGroupAttr is not set, convert direct groups into names and return + if matcher.RecursionGroupAttr == "" { + for _, group := range groups { + name := c.getAttr(*group, c.GroupSearch.NameAttr) + if name == "" { + return nil, fmt.Errorf( + "ldap: group entity %q missing required attribute %q", + group.DN, c.GroupSearch.NameAttr, + ) + } + groupNames = append(groupNames, name) } + continue + } + + // Recursive Search + c.logger.Info("Recursive group search enabled", "groupAttr", matcher.GroupAttr, "recursionAttr", matcher.RecursionGroupAttr) + for { + var nextLevel []*ldap.Entry + for _, group := range groups { + name := c.getAttr(*group, c.GroupSearch.NameAttr) + if name == "" { + return nil, fmt.Errorf("ldap: group entity %q missing required attribute %q", + group.DN, c.GroupSearch.NameAttr) + } + + // Prevent duplicates and circular references. + duplicate := false + for _, existingName := range groupNames { + if name == existingName { + c.logger.Debug("Found duplicate group", "name", name) + duplicate = true + break + } + } + if duplicate { + continue + } + + groupNames = append(groupNames, name) - gotGroups := false - if err := c.do(ctx, func(conn *ldap.Conn) error { - c.logger.Info("performing ldap search", - "base_dn", req.BaseDN, "scope", scopeString(req.Scope), "filter", req.Filter) - resp, err := conn.Search(req) + // Search for parent groups using the group's DN. + parents, filter, err := c.queryGroups(ctx, matcher.RecursionGroupAttr, group.DN) if err != nil { - return fmt.Errorf("ldap: search failed: %v", err) + return nil, err + } + if len(parents) == 0 { + c.logger.Debug("No parent groups found", "filter", filter) + } else { + nextLevel = append(nextLevel, parents...) } - gotGroups = len(resp.Entries) != 0 - groups = append(groups, resp.Entries...) - return nil - }); err != nil { - return nil, err } - if !gotGroups { - // TODO(ericchiang): Is this going to spam the logs? - c.logger.Error("groups search returned no groups", "filter", filter) + if len(nextLevel) == 0 { + break } + groups = nextLevel } } + return groupNames, nil +} - groupNames := make([]string, 0, len(groups)) - for _, group := range groups { - name := c.getAttr(*group, c.GroupSearch.NameAttr) - if name == "" { - // Be obnoxious about missing attributes. If the group entry is - // missing its name attribute, that indicates a misconfiguration. - // - // In the future we can add configuration options to just log these errors. - return nil, fmt.Errorf("ldap: group entity %q missing required attribute %q", - group.DN, c.GroupSearch.NameAttr) - } +func (c *ldapConnector) queryGroups(ctx context.Context, memberAttr, dn string) ([]*ldap.Entry, string, error) { + filter := fmt.Sprintf("(%s=%s)", memberAttr, ldap.EscapeFilter(dn)) + if c.GroupSearch.Filter != "" { + filter = fmt.Sprintf("(&%s%s)", c.GroupSearch.Filter, filter) + } - groupNames = append(groupNames, name) + req := &ldap.SearchRequest{ + BaseDN: c.GroupSearch.BaseDN, + Filter: filter, + Scope: c.groupSearchScope, + Attributes: []string{c.GroupSearch.NameAttr}, + } + + var entries []*ldap.Entry + if err := c.do(ctx, func(conn *ldap.Conn) error { + c.logger.Info( + "performing ldap search", + "base_dn", req.BaseDN, + "scope", scopeString(req.Scope), + "filter", req.Filter, + ) + resp, err := conn.Search(req) + if err != nil { + if ldapErr, ok := err.(*ldap.Error); ok && ldapErr.ResultCode == ldap.LDAPResultNoSuchObject { + c.logger.Info("LDAP search returned no groups", "filter", filter) + return nil + } + return fmt.Errorf("ldap: search failed: %v", err) + } + entries = append(entries, resp.Entries...) + return nil + }); err != nil { + return nil, filter, err } - return groupNames, nil + return entries, filter, nil } func (c *ldapConnector) Prompt() string { diff --git a/connector/ldap/ldap_test.go b/connector/ldap/ldap_test.go index 7d587692..a9665e12 100644 --- a/connector/ldap/ldap_test.go +++ b/connector/ldap/ldap_test.go @@ -525,6 +525,56 @@ func TestUsernamePrompt(t *testing.T) { } } +func TestNestedGroups(t *testing.T) { + c := &Config{} + c.UserSearch.BaseDN = "ou=People,ou=TestNestedGroups,dc=example,dc=org" + c.UserSearch.NameAttr = "cn" + c.UserSearch.EmailAttr = "mail" + c.UserSearch.IDAttr = "DN" + c.UserSearch.Username = "cn" + + c.GroupSearch.BaseDN = "ou=TestNestedGroups,dc=example,dc=org" + c.GroupSearch.UserMatchers = []UserMatcher{ + { + UserAttr: "DN", + GroupAttr: "member", + // Enable Recursive Search + RecursionGroupAttr: "member", + }, + } + c.GroupSearch.NameAttr = "cn" + + tests := []subtest{ + { + name: "nestedgroups_jane", + username: "jane", + password: "foo", + groups: true, + want: connector.Identity{ + UserID: "cn=jane,ou=People,ou=TestNestedGroups,dc=example,dc=org", + Username: "jane", + Email: "janedoe@example.com", + EmailVerified: true, + Groups: []string{"childGroup", "circularGroup1", "intermediateGroup", "circularGroup2", "parentGroup"}, + }, + }, + { + name: "nestedgroups_john", + username: "john", + password: "bar", + groups: true, + want: connector.Identity{ + UserID: "cn=john,ou=People,ou=TestNestedGroups,dc=example,dc=org", + Username: "john", + Email: "johndoe@example.com", + EmailVerified: true, + Groups: []string{"circularGroup2", "intermediateGroup", "circularGroup1", "parentGroup"}, + }, + }, + } + runTests(t, connectLDAP, c, tests) +} + func getenv(key, defaultVal string) string { if val := os.Getenv(key); val != "" { return val diff --git a/connector/ldap/testdata/schema.ldif b/connector/ldap/testdata/schema.ldif index 69c7b3ff..a7f1393d 100644 --- a/connector/ldap/testdata/schema.ldif +++ b/connector/ldap/testdata/schema.ldif @@ -445,3 +445,63 @@ sn: doe cn: jane mail: janedoe@example.com userpassword: foo + +######################################################################## + +dn: ou=TestNestedGroups,dc=example,dc=org +objectClass: organizationalUnit +ou: TestNestedGroups + +dn: ou=People,ou=TestNestedGroups,dc=example,dc=org +objectClass: organizationalUnit +ou: People + +dn: cn=jane,ou=People,ou=TestNestedGroups,dc=example,dc=org +objectClass: person +objectClass: inetOrgPerson +sn: doe +cn: jane +mail: janedoe@example.com +userpassword: foo + +dn: cn=john,ou=People,ou=TestNestedGroups,dc=example,dc=org +objectClass: person +objectClass: inetOrgPerson +sn: doe +cn: john +mail: johndoe@example.com +userpassword: bar + +# Group definitions. + +dn: ou=Groups,ou=TestNestedGroups,dc=example,dc=org +objectClass: organizationalUnit +ou: Groups + +dn: cn=childGroup,ou=Groups,ou=TestNestedGroups,dc=example,dc=org +objectClass: groupOfNames +cn: childGroup +member: cn=jane,ou=People,ou=TestNestedGroups,dc=example,dc=org + +dn: cn=intermediateGroup,ou=Groups,ou=TestNestedGroups,dc=example,dc=org +objectClass: groupOfNames +cn: intermediateGroup +member: cn=childGroup,ou=Groups,ou=TestNestedGroups,dc=example,dc=org +member: cn=john,ou=People,ou=TestNestedGroups,dc=example,dc=org + +dn: cn=parentGroup,ou=Groups,ou=TestNestedGroups,dc=example,dc=org +objectClass: groupOfNames +cn: parentGroup +member: cn=intermediateGroup,ou=Groups,ou=TestNestedGroups,dc=example,dc=org + +dn: cn=circularGroup1,ou=Groups,ou=TestNestedGroups,dc=example,dc=org +objectClass: groupOfNames +cn: circularGroup1 +member: cn=circularGroup2,ou=Groups,ou=TestNestedGroups,dc=example,dc=org +member: cn=jane,ou=People,ou=TestNestedGroups,dc=example,dc=org + +dn: cn=circularGroup2,ou=Groups,ou=TestNestedGroups,dc=example,dc=org +objectClass: groupOfNames +cn: circularGroup2 +member: cn=circularGroup1,ou=Groups,ou=TestNestedGroups,dc=example,dc=org +member: cn=john,ou=People,ou=TestNestedGroups,dc=example,dc=org