KNOX-3330: Refactor LDAP Proxy configuration to support multiple backends by handavid · Pull Request #1240 · apache/knox

handavid · 2026-05-26T17:30:22Z

KNOX-3330 - Refactor Knox LDAP Proxy configuration and implementation to allow multiple backends to be simultaneously configured

What changes were proposed in this pull request?

Gateway server configurations are updated to use 'gateway.ldap.interceptor.' instead of 'gateway.ldap.backend.' to allow specifying multiple types of interceptors as well as multiple backends to the LDAP proxy.

BackendFactory has been modified to use the java ServiceLoader to load a factory for a backend class instead of a backend instance directly. This allows multiple backends of the same class to be configured. InterceptorFactory has been implemented following the same pattern.

GroupLookupInterceptor is renamed to UserSearchInterceptor to more accurately describe what it does. Multiple UserSearchInterceptors can be configured with each forwarding the search to its backend and appending the results.

A DuplicateUserFilteringInterceptor has been implemented that will filter out search Entries with the same UID that are returned from different backends.

How was this patch tested?

Unit tests were updated.

KnoxLDAPServerManagerTest.java modified to configure interceptors instead of backends
KnoxLDAPServerManagerTest.java modified to configure multiple backends simultaneously

Changes were manually tested against the test ldap server and an AD that I have access to.
The following configuration was added to the gateway-site.xml

    <!-- LDAP Proxy Service Configuration -->
    <property>
        <name>gateway.ldap.enabled</name>
        <value>true</value>
        <description>Enable the embedded LDAP service for user and group lookups. Set to true to enable.</description>
    </property>
    <property>
        <name>gateway.ldap.port</name>
        <value>3890</value>
        <description>Port for the LDAP service to listen on. Default is 3890.</description>
    </property>
    <property>
        <name>gateway.ldap.base.dn</name>
        <value>dc=proxy,dc=com</value>
        <description>Base DN for LDAP entries in the proxy server. Default is dc=proxy,dc=com.</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.names</name>
        <value>localldap,testad,duplicatefilter</value>
        <description>Interceptor names for LDAP service.</description>
    </property>

    <!-- Local LDAP Server -->
    <property>
        <name>gateway.ldap.interceptor.localldap.interceptorType</name>
        <value>backend</value>
        <description>Type of interceptor. Currently supported: backend, duplicateuserfilter</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.localldap.backendType</name>
        <value>ldap</value>
        <description>Type of backend. Currently supported: file, ldap. Future: jdbc, knox.</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.localldap.url</name>
        <value>ldap://localhost:33389</value>
        <description>LDAP server URL for proxy backend</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.localldap.remoteBaseDn</name>
        <value>dc=hadoop,dc=apache,dc=org</value>
        <description>Base DN of the remote LDAP server</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.localldap.systemUsername</name>
        <value>uid=guest,ou=people,dc=hadoop,dc=apache,dc=org</value>
        <description>LDAP bind DN for proxy backend authentication</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.localldap.systemPassword</name>
        <value>guest-password</value>
        <description>LDAP bind password for proxy backend authentication</description>
    </property>
    
    <!-- Test AD -->
    <property>
        <name>gateway.ldap.interceptor.testad.interceptorType</name>
        <value>backend</value>
        <description>Type of interceptor. Currently supported: backend, duplicateuserfilter</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.testad.backendType</name>
        <value>ldap</value>
        <description>Type of backend. Currently supported: file, ldap. Future: jdbc, knox.</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.testad.url</name>
        <value>ldap://test-ad.example.com:389</value>
        <description>LDAP server URL for proxy backend</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.testad.remoteBaseDn</name>
        <value>dc=test-ad,dc=example,dc=com</value>
        <description>Base DN of the remote LDAP server</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.testad.systemUsername</name>
        <value>bind user to AD</value>
        <description>LDAP bind DN for proxy backend authentication</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.testad.systemPassword</name>
        <value>password to AD</value>
        <description>LDAP bind password for proxy backend authentication</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.testad.userIdentifierAttribute</name>
        <value>sAMAccountName</value>
        <description>Attribute used for identifying users</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.testad.userSearchBase</name>
        <value>cn=users,dc=test-ad,dc=example,dc=com</value>
        <description>Search base for users</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.testad.groupSearchBase</name>
        <value>ou=groups,dc=test-ad,dc=example,dc=com</value>
        <description>Search base for groups</description>
    </property>
    <property>
        <name>gateway.ldap.interceptor.testad.useMemberOf</name>
        <value>true</value>
        <description>Whether to use the memberOf attribute for efficiency when retrieving group memberships</description>
    </property>

    <!-- Duplicate Filter Interceptor -->
    <property>
        <name>gateway.ldap.interceptor.duplicatefilter.interceptorType</name>
        <value>duplicateuserfilter</value>
        <description>Type of interceptor. Currently supported: backend, duplicateuserfilter</description>
    </property>

    <!-- END LDAP Proxy Service Configuration -->

Integration Tests

No integration test changes. PR can be updated after #1236 is merged

UI changes

no UI changes

handavid · 2026-05-26T17:31:08Z

@smolnar82 @lmccay

github-actions · 2026-05-27T14:48:56Z

Test Results

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

Results for commit 33fe1fe.

…ends Gateway server configurations are updated to use 'gateway.ldap.interceptor.*' instead of 'gateway.ldap.backend.*' to allow specifying multiple types of interceptors as well as multiple backends to the LDAP proxy. BackendFactory has been modified to use the java ServiceLoader to load a factory for a backend class instead of a backend instance directly. This allows multiple backends of the same class to be configured. InterceptorFactory has been implemented following the same pattern. GroupLookupInterceptor is renamed to UserSearchInterceptor to more accurately describe what it does. Multiple UserSearchInterceptors can be configured with each forwarding the search to its backend and appending the results. A DuplicateUserFilteringInterceptor has been implemented that will filter out search Entries with the same UID that are returned from different backends.

handavid · 2026-06-02T05:54:25Z

rebased and fixed conflicts

smolnar82

I'll keep the review later today.

smolnar82 · 2026-06-02T13:36:34Z

Please remove this file from the commit (and the other .jceks files too).

smolnar82 · 2026-06-02T13:41:21Z

+    String[] namesArray = names.split(",");
+    List<String> namesList = new ArrayList<>();
+    for (String name : namesArray) {
+      if (!Strings.isNullOrEmpty(name)) {


We use org.apache.commons.lang3.StringUtils#isNotBlank in the project for the same purpose.
This is already imported -> the new import will be eliminated too.
Even better, we already have a method to get comma separated config values as list: org.apache.knox.gateway.config.impl.GatewayConfigImpl#splitConfigValueToList

smolnar82 · 2026-06-02T13:55:27Z

+import java.util.List;
+import java.util.Set;
+
+public class DuplicateUserFilteringInterceptor extends BaseInterceptor {


Please add unit test coverage for this class.

smolnar82 · 2026-06-02T14:00:42Z

+            while (originalResults.next()) {
+                originalEntries.add(originalResults.get());
+            }
+            originalResults.close();


Please ensure that originalResults.close() is always invoked to avoid cursor leaks (using try-with-resources is a good option here).
E.g.

try (EntryFilteringCursor originalResults = next(ctx)) { ... }

smolnar82 · 2026-06-02T14:05:19Z

+public class DuplicateUserFilteringInterceptorFactory implements KnoxLdapInterceptorFactory {
+    @Override
+    public Interceptor create(String name, Map<String, String> config) {
+        // NOTE: no further configuration expected for this Interceptor


nit: I think this JavaDoc comment is redundant and can be obsolete quickly.

smolnar82 · 2026-06-02T14:08:27Z

+
+    @Override
+    public String getType() {
+        return "duplicateuserfilter";


nit: using a constant would be better

smolnar82 · 2026-06-02T14:15:01Z

+                        // if no results, perform single-user search
+                        if (entries.isEmpty()) {
+                            // Specific user lookup
+                            LOG.ldapUserLoaded(username);


This log line should be under the actual getUser(username); line

smolnar82

Another batch of comments.
I'll review the tests tomorrow.

smolnar82 · 2026-06-02T19:31:51Z

+        List<Entry> entries = new ArrayList<>();
+        entries.addAll(backend.searchUsers(userSearchString, schemaManager));
+        return entries;


Why not simply: return backend.searchUsers(userSearchString, schemaManager)?
In fact, if this is a simple inline backend invocation, why we have this private method at all? No any particular check, logic, etc...in here.

smolnar82 · 2026-06-02T19:32:30Z

+    }
+
+    private Entry getUser(String username) throws Exception {
+        return backend.getUser(username, schemaManager);


The same as above: I don't think this private method is necessary.

smolnar82 · 2026-06-02T19:41:24Z

+    DirectoryService directoryService;
    private LdapServer ldapServer;
-    private LdapBackend backend;
+    private List<Interceptor> interceptors = new ArrayList<>();


This is redundant, as you initialize interceptors anyway (i.e. not simply add entries into it).
See new line 80: this.interceptors = createInterceptors(config)

smolnar82 · 2026-06-02T19:45:40Z

+
+    @Override
+    public String getType() {
+        return "backend";


nit: Should be a constant (we could reuse it in KnoxLdapManager, see my comment about not using the instanceof operator below).

smolnar82 · 2026-06-02T19:46:38Z

+        // Add our configured interceptors
+        SchemaManager schemaManager = directoryService.getSchemaManager();
+        for (Interceptor interceptor : interceptors) {
+            if (interceptor instanceof UserSearchInterceptor) {


I personally am not a big fan of the instanceof operator, so this feel free to ignore this comment :)

It might be an option to store the interceptors above in a Map<String, Interceptor>, where the key is the interceptor type. If that was the case, we could simply check if the Map.Entry.key.equals("backend") (this is where my comment about creating the backend interceptor type constant would be useful).

smolnar82 · 2026-06-02T19:48:34Z

+                if (!baseDns.contains(remoteBaseDn)) {
+                    //create partition
+                    String id = backend.getName().replaceAll("\\s+", "");
+                    JdbmPartition remotePartition = new JdbmPartition(schemaManager, directoryService.getDnFactory());
+                    remotePartition.setId(id);
+                    remotePartition.setSuffixDn(new Dn(schemaManager, remoteBaseDn));
+                    remotePartition.setPartitionPath(new File(workDir, id).toURI());
+                    directoryService.addPartition(remotePartition);
+                    baseDns.add(remoteBaseDn);
+                }


nit: a new private method (addRemotePartition, for instance) would increase the readability of this part of the code.

smolnar82 · 2026-06-02T19:57:00Z

    <property>
-        <name>gateway.ldap.backend.file.dataFile</name>
+        <name>gateway.ldap.interceptor.ldapfile.backendType</name>
+        <value>ldap</value>


Should be file.

smolnar82 · 2026-06-02T19:59:13Z

+    <!-- LDAP proxy backend configuration (gateway.ldap.interceptor.<interceptorName>.backendType=ldap) -->
    <!-- This backend proxies to an external LDAP server (e.g., demo LDAP) -->
    <!--
    Example 1: Using Knox demo LDAP server (default port 33389)


I'm not sure I like the idea of putting all these samples in the gateway-site.xml here.
We should rather create the new section in our user guide (see the knox-site module), and add all these sample configs there.

handavid force-pushed the knox-3330-improve-interceptors branch from 33fe1fe to dd9de8d Compare June 2, 2026 05:53

fix KnoxLDAPServerManagerTest to find and use open port for tests

39c89d8

smolnar82 requested changes Jun 2, 2026

View reviewed changes

addressed comments

0bfb637

Conversation

handavid commented May 26, 2026

What changes were proposed in this pull request?

How was this patch tested?

Integration Tests

UI changes

Uh oh!

handavid commented May 26, 2026

Uh oh!

github-actions Bot commented May 27, 2026

Test Results

Uh oh!

handavid commented Jun 2, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants