KNOX-3328: Implement recursive group resolution for LDAP proxy by smolnar82 · Pull Request #1236 · apache/knox

smolnar82 · 2026-05-20T13:18:24Z

KNOX-3328 - Support recursive group resolution in LDAP Proxy Service

What changes were proposed in this pull request?

This PR introduces recursive group resolution to the LdapProxyBackend. Key changes include:

Recursive Logic: Implemented Breadth-First Search (BFS) for group resolution. When enabled, Knox will traverse the group hierarchy to find all transitive memberships.
Cycle Detection: Uses a visited Set to track processed DNs, ensuring that circular group references (e.g., Group A -> Group B -> Group A) do not cause infinite loops.
Depth Limiting: Added a new configuration gateway.ldap.group.max.depth (default 10) to limit how deep the recursion goes, protecting the Gateway from excessive LDAP round-trips.
Group Enrichment: Updated LdapProxyBackend.getUser to search both the user and group search bases. This allows group entries to be returned as "proxy entries" enriched with their own memberOf attributes.
Optimized Attribute Fetching: Implemented a hybrid lookup strategy. Initial user/entry lookups fetch all attributes (*, +) to ensure complete profile data, while recursive steps specifically request only the memberOf and operational attributes (+) to minimize network payload and processing overhead.
New Configuration Properties:
- gateway.ldap.recursive.group.resolution: Boolean to enable/disable the feature.
- gateway.ldap.group.max.depth: Integer to control recursion depth.

How was this patch tested?

The patch was tested using the LdapProxyBackendTest suite with an embedded ApacheDS server.

Unit Tests: Added 4 new test cases to LdapProxyBackendTest:
- testGetUserGroupsRecursive: Verifies 3-level deep nesting is resolved correctly.
- testGetUserGroupsRecursiveCircular: Verifies that circular references are handled without errors.
- testGetUserGroupsRecursiveMaxDepth: Verifies that the recursion stops at the configured depth.
- testGetUserGroupsRecursiveUseMemberOf: Verifies recursive resolution when useMemberOf is set to true.
Test Data: Updated ldap-proxy-backend-test.ldif to include nested groups, circular groups, and groups with memberOf attributes.

Integration Tests

Automated unit tests were added to gateway-server. These tests use an embedded LDAP server to simulate a real-world proxy scenario, which provides high-fidelity verification of the recursive logic and LDAP protocol handling. No changes were required to .github/workflows/tests as the standard test suite covers these new unit tests.

UI changes

N/A

smolnar82 · 2026-05-20T13:18:47Z

Cc. @handavid

github-actions · 2026-05-20T13:53:37Z

Test Results

21 tests 21 ✅ 1s ⏱️
1 suites 0 💤
1 files 0 ❌

Results for commit 529e44d.

♻️ This comment has been updated with latest results.

…oup resolution config

…olution

lmccay

@smolnar82 - thanks for this improvement!
Mostly looks good.
I question the default depth of 10 and am also wondering about cursor leaks.
Cursor leaks may have been an issue prior to this PR but we should be sure that things do get cleaned up properly.

lmccay · 2026-05-20T21:06:20Z

            cursor.close();
+
+            // 2. Try search in group base if not found in user base
+            String groupFilter = "(cn=" + username + ")";


Do we know for sure that this will always be common name?
Also, I think a little more description of what this groupFilter's affect on the search will be in the comment would go a long way for future readers.

I'll introduce a configurable groupIdentifierAttribute, defaulting to cn, and add descriptive comments to clarify this search.

lmccay · 2026-05-20T21:07:51Z

+            }
+            cursor.close();
+
            return null;


do we not need any logging here?
What affect does not getting an Entry for the getUser method have?

The return null is actually not new in this PR:

According to the JavaDoc of the parent class, this is perfectly ok:

On the other hand, I agree that it's good to have some DEBUG log.

handavid · 2026-05-26T14:42:48Z

@@ -83,19 +83,20 @@ public String getName() {
    @Override
    public void initialize(Map<String, String> config) throws Exception {
        // Proxy base DN is for entries created in the proxy LDAP server


nit: comment redundant. you can remove the old comment.

handavid · 2026-05-26T14:51:52Z

+                // This allows the proxy to enrich group entries with recursive 'memberOf' attributes
+                // when a client searches for a group name.
+                String groupFilter = "(" + groupIdentifierAttribute + "=" + username + ")";
+                return fetchEntry(username, schemaManager, connection, groupSearchBase, groupFilter);


I find it strange to search for a group in a method called getUser. This is a limitation of the GroupSearchInterceptor that will only call this method for users. I have a change forthcoming to the interceptors and another planned for handling general searches better. I understand that there is a problem that you can't search for groups directly using the current implementation and hope to solve that in a better way.

handavid · 2026-05-27T03:19:43Z

-            Attribute memberOfAttr = userEntry.get("memberOf");
+        if (!nextLevelDns.isEmpty()) {
+            allGroupDns.addAll(nextLevelDns);
+            resolveRecursiveGroupDnsViaMemberOf(connection, allGroupDns, depth + 1);


I think we only need to recursively resolve the nextLevelDns and not the full allGroupDns. Otherwise we are redundantly fetching the same groups repeatedly. You'll probably need to pass in both the nextLevelDns and allGroupDns if you want to filter out seen Dns in fetchNextLevelDNs

handavid · 2026-05-27T03:28:57Z

+    private Set<String> fetchNextLevelDNs(LdapConnection connection, Set<String> allGroupDns) throws IOException, LdapException, CursorException {
+        final Set<String> nextLevelDns = new HashSet<>();
+        for (String dn : new HashSet<>(allGroupDns)) {
+            try (EntryCursor cursor = connection.search(dn, "(objectClass=*)", SearchScope.OBJECT, MEMBER_OF, "+")) {


can we combine the groups into a single search instead of executing one search per group? We can try constructing a search filter like connection.search(groupSearchBase, "(|(cn=Group1)(cn=Group2)(cn=Group3))", SearchScope.SUBTREE, MEMBER_OF, "+").

handavid · 2026-05-27T03:32:44Z

        return null;
    }

    private List<Entry> getUserGroupsInternal(LdapConnection connection, String userDn, String username) throws LdapException, CursorException, IOException {


this isn't only applicable to users after your change. you should rename the userDn param to something more general

handavid · 2026-05-27T03:33:57Z

+    }
+
+    private void resolveRecursiveGroupDnsInternal(LdapConnection connection, Set<String> allGroupDns, Set<String> visited, int depth) throws LdapException, CursorException, IOException {
+        if (depth >= groupMaxDepth) {


optimization: you can convert this from recursion to iteration to shorten the call stack.

handavid · 2026-05-27T03:34:05Z

+    }
+
+    private void resolveRecursiveGroupDnsViaMemberOf(LdapConnection connection, Set<String> allGroupDns, int depth) throws LdapException, CursorException, IOException {
+        if (depth >= groupMaxDepth) {


optimization: you can convert this from recursion to iteration to shorten the call stack.

handavid · 2026-05-27T03:39:21Z

-                    String groupName = extractGroupNameFromDn(groupDn);
-                    if (groupName != null) {
-                        groups.add(groupName);
+    private Set<String> fetchNextLevelDNs(LdapConnection connection, Set<String> allGroupDns) throws IOException, LdapException, CursorException {


can we rename this to fetchNextLevelDNsViaMemberOf to preserve the usage of this method in the memberOf codepath only?

handavid · 2026-05-27T13:44:25Z

+                    Entry sourceEntry = cursor.get();
+                    Attribute idAttr = sourceEntry.get(userIdentifierAttribute);
+                    if (idAttr != null) {
+                        Entry entry = createProxyEntry(sourceEntry, idAttr.getString(), connection, schemaManager);


this can lead to many redundant calls to the backend because the groups are retrieved on a per-user basis. If we have 10 users that are members of the same group, we'll do the same recursive group lookup 10 times.

smolnar82 requested review from hanicz, lmccay and moresandeep May 20, 2026 13:18

smolnar82 self-assigned this May 20, 2026

smolnar82 added the ldap label May 20, 2026

smolnar82 added 3 commits May 20, 2026 22:40

KNOX-3328: Implement recursive group resolution for LDAP proxy

7dfce9f

KNOX-3328: Update KnoxLDAPServiceTest to account for new recursive gr…

e1fba84

…oup resolution config

KNOX-3328: Add integration test coverage for recursive LDAP group res…

db40ef9

…olution

lmccay reviewed May 20, 2026

View reviewed changes

KNOX-3328: Addressed review comments and did some cleanup

5129988

smolnar82 force-pushed the KNOX-3328 branch from 529e44d to 5129988 Compare May 21, 2026 11:40

KNOX-3328: Fix checkstyle issues

9f6d69b

smolnar82 requested a review from lmccay May 21, 2026 12:22

handavid mentioned this pull request May 26, 2026

KNOX-3330: Refactor LDAP Proxy configuration to support multiple backends #1240

Open

handavid reviewed May 27, 2026

View reviewed changes

Conversation

smolnar82 commented May 20, 2026

What changes were proposed in this pull request?

How was this patch tested?

Integration Tests

UI changes

Uh oh!

smolnar82 commented May 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

lmccay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented May 20, 2026 •

edited

Loading