KNOX-3487, KNOX-3486: LDAP Proxy supports reverse role lookups and role listing by handavid · Pull Request #1454 · apache/knox

handavid · 2026-10-14T04:54:10Z

KNOX-3486 - LDAP Proxy supports role listing
KNOX-3487 - LDAP Proxy supports reverse role lookups

What changes were proposed in this pull request?

Reverse role lookup (role to members) and role listing is added to the LdapRolesLookupService. This is supported by both file and rest-based implementations.

Role listing is performed by analyzing the search to determine if it returns all groups. The overall LDAP search is then short-circuited and role entries are returned from the roles "list" endpoint instead.

Reverse role lookup is performed by expanding "memberOf" expressions in the search filter to the matching users and groups as returned from the roles "get" endpoint.

Additional improvements to memberOf handling was added to the LdapProxyBackend to expand the search filter if "useMemberOf" is set to false. This expansion uses the same recursive resolution max depth that is used for transitive membership calcuations.

These changes may increase the size of the search filter. The LdapProxyBackend will split the search filter into multiple backend searches if it becomes too large.

How was this patch tested?

Unit tests were added for the new functionality.
Role list and get functionality was tested against file and rest endpoints.

Integration Tests

(Please add or update integration tests .github/workflows/tests for the feature you are adding. If no unit test is added, please explain why. Check out .github/workflows/tests/README.md for instructions)

Workflow tests were added to test_knox_ldap_proxy_search_with_role_lookup.py

UI changes

no UI changes

handavid · 2026-10-14T04:55:34Z

@smolnar82
What do you think of the requirement to add the new endpoints? Should I worry about making that backwards compatible (e.g.,, deriving a default value from the lookup endpoint)?

smolnar82 · 2026-10-15T09:56:38Z

@smolnar82 What do you think of the requirement to add the new endpoints? Should I worry about making that backwards compatible (e.g.,, deriving a default value from the lookup endpoint)?

Thanks for raising this. Short answer: I don't think backward compatibility is needed here, and I'd avoid deriving defaults from the lookup endpoint.

Backward compatibility: Upgrades are out of scope for this change, so it's fine for the rest strategy to require all three endpoints (lookup, list, get). Failing fast at startup with a clear error when one is missing is the right behavior. It's better than starting with role listing or reverse lookup silently disabled.

Deriving defaults: I'd skip this. It means guessing how the roles service lays out its URLs, and a wrong guess sends Knox to the wrong endpoint without any obvious error. The docs example in this PR shows how easily that happens: the lookup URL moved from /auth/roles to /auth/roles/lookup, and /auth/roles became the list endpoint. Separate endpoints also make sense on their own terms, since list and get-by-id are different resources from the POST lookup.

What we do need is documentation, because anyone implementing the roles service has to know exactly what Knox calls and what it expects back. Could you add these to service_ldap_server.md?

Common Properties table: say that all three gateway.ldap.roles.*.rest.api.endpoint properties are required when gateway.ldap.roles.lookup.strategy=rest, and that the gateway fails to start if one is missing. For the get endpoint, say that Knox requests <endpoint>/<URL-encoded role id>, so it should be configured without a trailing slash.
Usage example: remove the trailing slash from the get endpoint (http://localhost:8080/auth/roles/). As written, Knox requests .../roles//<id>.
A "Roles REST API contract" subsection under the Roles Lookup Interceptor, covering all three calls:
- Lookup: POST, request {"user_id": "...", "groups": [...]}, response {"user_id": "...", "roles": [{"scope": "...", "name": "..."}]}. This one was never documented either.
- List: GET ?offset=N, plus name/scope query params when filtering. Response {"data": [{"id", "name", "scope"}], "pagination": {"limit", "offset", "total"}}. Knox keeps advancing offset until it reaches total, and it doesn't send limit.
- Get: GET /<id>, response {"id", "name", "scope", "members": [{"id", "member_type": "user"|"group", "member_id"}]}.
- Common rules: any status other than 200 fails the LDAP search, and role names match case-insensitively. A role appears in LDAP as cn=<name> when unscoped and cn=<scope>:<name> when scoped.
File strategy format a roles.json example (the CI one works). Note that listing only returns roles with at least one assignment.
Role listing and reverse lookup behavior:
- which searches count as "all groups";
- that listed role entries have no member attribute;
- how (memberOf=cn=<role>,ou=groups,<baseDn>) is rewritten: users are matched on uid, and groups are matched through memberOf;
- how this interacts with useMemberOf and recursiveGroupResolution;
- that each memberOf condition costs extra roles-service calls;
- an ldapsearch example matching the one in KNOX-3487.
maxFilterLength: the docs say the default is 4096, but the code uses 2000. Please make them match.
Troubleshooting: the DEBUG log messages for list/get requests and responses, the INFO message logged when a filter is split, and using the bypass control to compare results with and without role mapping.

…e listing Reverse role lookup (role to members) and role listing is added to the LdapRolesLookupService. This is supported by both file and rest-based implementations. Role listing is performed by analyzing the search to determine if it returns all groups. The overall LDAP search is then short-circuited and role entries are returned from the roles "list" endpoint instead. Reverse role lookup is performed by expanding "memberOf" expressions in the search filter to the matching users and groups as returned from the roles "get" endpoint. Additional improvements to memberOf handling was added to the LdapProxyBackend to expand the search filter if "useMemberOf" is set to false. This expansion uses the same recursive resolution max depth that is used for transitive membership calcuations. These changes may increase the size of the search filter. The LdapProxyBackend will split the search filter into multiple backend searches if it becomes too large. Unit tests and workflow tests were added for the new functionality. Co-Authored-By: Claude Code <noreply@anthropic.com>

github-actions · 2026-10-16T13:16:49Z

Test Results

4 files 4 suites 1m 14s ⏱️
145 tests 145 ✅ 0 💤 0 ❌
169 runs 169 ✅ 0 💤 0 ❌

Results for commit 15d545d.

♻️ This comment has been updated with latest results.

smolnar82

Side note, not for this PR: like the existing forward lookup (RestApiLdapRolesLookup), the new calls use HttpClients.createDefault() with no timeouts and a new client per call, so a hung roles service blocks LDAP threads. I'll open a follow-up JIRA to add a shared client with timeouts (and maybe a short TTL cache) for all REST role lookups.

smolnar82 · 2026-10-16T12:53:28Z

+            return false;
+        }
+        // Either the proxy groups base, or the root with an explicit group filter
+        return searchDn.toString().equalsIgnoreCase(proxyGroupSearchBase) || searchDn.size() <= 2;


The comment's intent ("the proxy groups base, or the root with an explicit group filter") makes sense, but the code doesn't enforce either half:

searchDn.size() <= 2 only means "root" when the base DN has exactly two RDNs. With gateway.ldap.base.dn=o=acme, ou=users,o=acme also matches, so a user search with (cn=*) returns roles. With dc=acme,dc=co,dc=uk, the root never matches. Please compare normalized Dns against the configured base DN and ou=groups,<baseDn>, which also removes the spacing problem with toString().equalsIgnoreCase.

isAllGroupsFilter accepts (cn=*) and (objectClass=*), which aren't explicit group filters. At the root, the default ldapsearch -b dc=proxy,dc=com uses (objectClass=*) and now returns only roles, with no users. Please accept those two filters only when the base is ou=groups,<baseDn>, and require objectClass=groupOfNames|groupOfUniqueNames at the root.

Minor: a ONELEVEL search at the root returns roles, which sit one level lower, under ou=groups. With objectClass=* accepted, this breaks tree browsing in LDAP browsers.

fixed to use separate criteria for the group search base and ancestor DNs.

smolnar82 · 2026-10-16T12:54:40Z

+            return !children.isEmpty() && children.stream().allMatch(this::isAllGroupsFilter);
+        }
+        if (filter instanceof OrNode) {
+            return ((OrNode) filter).getChildren().stream().anyMatch(this::isAllGroupsFilter);


With "any child matches", (|(uid=alice)(cn=*)) is treated as all-groups and alice is dropped. An OR filter only selects all groups if every child does, so this should be allMatch.

anyMatch is correct for searches on the proxyGroupSearchBase. This criteria is too loose for searches on the ancestors or the root. This is solved by splitting the filter examination depending on whether the search base is the complete proxyGroupSearchBase or an ancestor.

smolnar82 · 2026-10-16T13:08:23Z

+            return false;
+        }
+        String lowerCaseUserDn = userDn.toLowerCase(ROOT);
+        return lowerCaseUserDn.contains(proxyUserSearchBase) || lowerCaseUserDn.contains(remoteUserObjectClass);


This check is unreliable:

contains(remoteUserObjectClass): an objectClass is never part of a DN. The default inetOrgPerson is mixed-case and is compared against a lowercased string, so it can never match. For AD, user matches substrings such as cn=users or cn=superusers.

proxyUserSearchBase isn't lowercased, so a base DN configured as DC=proxy,DC=com never matches.

With dnMappingEnabled=false, member DNs stay remote DNs, so they never contain the proxy base. Every user is then treated as a group, and the filter silently returns nothing.

Please use Dn.isDescendantOf against the user base that matches the DN-mapping mode, or check the member entry's objectClass. The current tests don't catch this because LdapProxyBackendTest sets baseDn == remoteBaseDn and the visitor test stubs isUser.

switched string handling to DN handling and check against both proxy and backend user search base.

smolnar82 · 2026-10-16T13:09:23Z

    private static final String[] ALL_ATTRIBUTES = new String[]{"*"};
    private static final String[] ALL_ATTRIBUTES_AND_MEMBEROF = new String[]{"*", "memberOf"};

+    private static final String DEFAULT_MAX_FILTER_LENGTH = "2000";


buildMultipleGroupMemberFilters also splits the existing group-membership resolution, not only the new memberOf expansion. At roughly 150 characters per POSIX clause, every useMemberOf=false deployment makes one extra round trip for about every 13 users.

Real server limits are far higher (OpenLDAP 256KB anonymous / 4MB authenticated, AD 10MB).

I'd default to 0 (unlimited) or to something much larger.

default to unlimited

smolnar82 · 2026-10-16T13:13:50Z

+                    }
+
+                    ListRolesResponse listRolesResponse = mapper.readValue(jsonResponse, ListRolesResponse.class);
+                    numRolesRetrieved += listRolesResponse.getData().size();


These throw an NPE when the response has no data, pagination or member_type (getData().size(), getPagination().getTotal(), member.getMemberType().equalsIgnoreCase(...)). Please null-check them and raise a RoleException with a clear message.

added null-checks

smolnar82 · 2026-10-16T13:37:17Z

+        ExprNode node = FilterParser.parse(schemaManager, filter);
+        // Baseline in the same form as the visitor output so an unchanged visit exits on the first pass
+        String current = node.toString();
+        for (int i = 0; i < recursiveGroupResolutionMaxDepth; ++i) {


This loop always runs up to recursiveGroupResolutionMaxDepth (default 3), even when recursiveGroupResolution is off, which is the default. Please do a single pass when recursion is disabled.

smolnar82 · 2026-10-16T13:38:23Z


 public class RoleAssignment {
+    public static RoleAssignment createFromDisplayValue(String displayValue) {
+        String[] tokens = displayValue.split(":");


Please use split(":", 2) so a role name that contains : (for example platform:team:admin) is parsed as scope platform and name team:admin. Right now it throws IllegalArgumentException at line 31, and with the unhandled-exception issue in handleSearch that error escapes the LDAP search as an unchecked exception.

smolnar82 · 2026-10-16T13:38:40Z

+        ExprNode filter = ctx.getFilter();
+        ReverseRoleLookupMappingVisitor reverseRoleLookupMappingVisitor =
+                new ReverseRoleLookupMappingVisitor(schemaManager, rolesLookupService, this::createGroupDnFromName);
+        Object updatedFilter = filter.accept(reverseRoleLookupMappingVisitor);


If the roles service fails, a DN can't be parsed, or RoleAssignment.createFromDisplayValue throws on a:b:c, the visitor throws an unchecked exception (RuntimeException from ReverseRoleLookupMappingVisitor.java:64, or IllegalArgumentException). This call isn't inside a try/catch, so the exception escapes, the client doesn't get an LdapException, and ldapRolesLookupFailed is never logged.

Please catch the exception here and convert it to an LdapException. The same applies to MemberOfMappingVisitor.java:66 as called from LdapProxyBackend.mapMemberOfExpression.

smolnar82 · 2026-10-16T13:40:07Z

+        }
+        // Convert filter if it contains "memberOf" but the backend doesn't support "memberOf"
+        MemberOfMappingVisitor visitor = new MemberOfMappingVisitor(schemaManager, this::lookupGroup, this::isUser);
+        ExprNode node = FilterParser.parse(schemaManager, filter);


This parses the raw filter before convertProxyFilterToRemoteFilter (RemoteSchemaConverter.java:151) removes the :[...] annotations, and that method's comment says those annotations occur in practice. Please apply the same cleanup first, ideally through a shared helper. It's worth testing with a real annotated filter.

moved the filter cleanup to LdapProxyBackend to run before either filter is applied

smolnar82 · 2026-10-16T13:40:44Z

+                break;
+            }
+        }
+        return new ArrayList<>(combined.values());


The maxResultSetSize check runs only after each filter piece finishes, so the combined result can go past the limit. Please truncate it to maxResultSetSize.

truncated both the split search and paged search in case a single page is larger than the maxResultSetSize

smolnar82

Only 3 minor, but important findings this time.

smolnar82 · 2026-10-20T08:49:05Z

+            Dn searchBaseDn = new Dn(searchBase);
+            return searchBaseDn.isDescendantOf(proxyBaseDn) ||
+                    searchBaseDn.isDescendantOf(remoteBaseDn);


new Dn(searchBase) has no SchemaManager, so RDN values are compared case-sensitively (api-ldap-model 2.1.8: Ava.equals -> Value.equals -> upValue.equals(...) when there is no AttributeType). The caller (UserSearchInterceptor) passes ctx.getDn().toString(), which is the user-provided form. So -b 'ou=people,DC=Proxy,DC=com', or a remote base configured as DC=Example,DC=com and queried as dc=example,dc=com, is silently skipped and returns no backend results, even though these DNs are equal under LDAP matching rules (dc/ou/cn use caseIgnoreMatch).

Suggest a schema-aware Dn (pass the SchemaManager or the normalized ctx.getDn() from UserSearchInterceptor), or at least lowercase both sides:

Dn searchBaseDn = new Dn(searchBase.toLowerCase(Locale.ROOT)); return searchBaseDn.isDescendantOf(proxyBaseDn.toLowerCase(Locale.ROOT)) || searchBaseDn.isDescendantOf(remoteBaseDn.toLowerCase(Locale.ROOT));

A mixed-case base in testIsSupportedSearchBase would cover it.

great catch! I used toLowerCase(Locale.ROOT) since the SchemaManager may not be available in this method.

smolnar82 · 2026-10-20T08:49:53Z

+**Role listing.** A search for all groups is answered with the roles known to the Role Lookup Service instead of the backend groups. Each role is returned as a `groupOfNames` entry with the DN `cn=<scope>:<name>,ou=groups,<base dn>`. Listed Role entries do not contain the `member` attribute because the role data is returned through the role strategy without further search requests to LDAP. A search is an all-groups search when its scope is not `base` (object) and its filter is an all-groups filter, and it is issued either from `ou=groups,<base dn>` or from the root of the directory (a base DN of at most two RDNs). A filter is an all-groups filter if it is one of:
+
+- `(objectClass=groupOfNames)` or `(objectClass=groupOfUniqueNames)`
+- `(cn=*)` or `(objectClass=*)`
+- an AND filter where every child is an all-groups filter, for example `(&(objectClass=groupOfNames)(cn=*))`
+- an OR filter where at least one child is an all-groups filter


The all-groups rules in the code changed with the last fixes, but this paragraph still describes the old ones:

Line 161, "from the root of the directory (a base DN of at most two RDNs)": the code now accepts any ancestor of ou=groups,<base dn> (compared as a Dn). It no longer counts RDNs.

Line 164: (cn=*) / (objectClass=*) are only all-groups filters when the base is ou=groups,<base dn>. From an ancestor they now go to the backend.

Line 166, "an OR filter where at least one child is an all-groups filter": this only applies at ou=groups,<base dn>. From an ancestor, every OR child must be (objectClass=groupOfNames) or (objectClass=groupOfUniqueNames). An AND needs at least one such objectClass child, and every other child must be an all-groups filter.

Suggest splitting the list by search base, for example:

From ou=groups,<base dn>: (objectClass=groupOfNames), (objectClass=groupOfUniqueNames), (cn=*), (objectClass=*), an AND where every child is one of these, or an OR where at least one child is.

From an ancestor of ou=groups,<base dn> (e.g. <base dn>): (objectClass=groupOfNames), (objectClass=groupOfUniqueNames), an OR of only those, or an AND that contains at least one of them and whose other children are all-groups filters, e.g. (&(objectClass=groupOfNames)(cn=*)).

smolnar82 · 2026-10-20T08:50:55Z

+
+| Field | Description |
+| :--- | :--- |
+| `members[].member_type` | `user` or `group` (case-insensitive). Members of any other type are ignored and logged. |


"Members of any other type are ignored and logged." no longer matches the code: RestApiLdapRolesReverseLookup now throws a RoleException for an unknown member_type, which fails the LDAP search (see testLookupRoleAssigneesThrowsOnUnknownMemberType). Please update the docs to say an unknown member_type fails the lookup and the LDAP search, or switch the code to log-and-skip if that's the intended contract.

changed code to log and skip. I think that's nicer behavior (forwards compatible) for handling new member types in the future.

…pes. fix dn comparison

smolnar82 assigned handavid Oct 15, 2026

smolnar82 added the ldap label Oct 15, 2026

smolnar82 requested review from hanicz, moresandeep and smolnar82 October 15, 2026 10:02

handavid and others added 2 commits October 15, 2026 22:36

update docs

be34ae2

handavid force-pushed the knox-3487-reverse-rolelookup branch from 3ef04db to be34ae2 Compare October 16, 2026 03:38

smolnar82 requested changes Oct 16, 2026

View reviewed changes

handavid added 2 commits October 20, 2026 02:22

addressed comments

84cea4b

truncate results to maxResultSetSize

15d545d

smolnar82 requested changes Oct 20, 2026

View reviewed changes

ignore unknown in JSON classes. update docs. ignore unknown member ty…

a0bc3f3

…pes. fix dn comparison

Conversation

handavid commented Oct 14, 2026

What changes were proposed in this pull request?

How was this patch tested?

Integration Tests

UI changes

Uh oh!

handavid commented Oct 14, 2026

Uh oh!

smolnar82 commented Oct 15, 2026

Uh oh!

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

Uh oh!

Test Results

Uh oh!

smolnar82 left a comment

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!

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!

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!

smolnar82 left a comment

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!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

github-actions Bot commented Oct 16, 2026 •

edited

Loading