KNOX-3340: Add Control for LDAPRolesLookupInterceptor by handavid · Pull Request #1258 · apache/knox

handavid · 2026-06-11T04:57:38Z

KNOX-3340 - Add Control to LDAPRolesLookupInterceptor

What changes were proposed in this pull request?

This commit adds a RolesLookupBypassControl for use with the LDAPRolesLookupInterceptor. The LDAPRolesLookupInterceptor will skip role mapping if this control is present and true in the request. This lets the client decide whether they will receive users' groups or roles.

How was this patch tested?

Unit tests were added to cover the new code.

Manual testing was performed. The LDAP Proxy was configured with the RolesLookup interceptor and the following ldapsearch commands were run.

# add control by OID with value "true"
ldapsearch -v -x -H ldap://localhost:3890 -b 'ou=people,DC=proxy,DC=com' -e "1.3.6.1.4.1.18060.2.1379319520.35362.17433.40846.265936912329953=AQP/" '(uid=sam*)' '*'

# add control by OID with value "false"
ldapsearch -v -x -H ldap://localhost:3890 -b 'ou=people,DC=proxy,DC=com' -e "1.3.6.1.4.1.18060.2.1379319520.35362.17433.40846.265936912329953=AQMA" '(uid=sam*)' '*'

# don't add control
ldapsearch -v -x -H ldap://localhost:3890 -b 'ou=people,DC=proxy,DC=com' '(uid=sam*)' '*'

Integration Tests

no integration tests added

UI changes

no UI changes

This commit adds a RolesLookupBypassControl for use with the LDAPRolesLookupInterceptor. The LDAPRolesLookupInterceptor will skip role mapping if this control is present and true in the request.

handavid · 2026-06-11T04:57:57Z

@smolnar82

github-actions · 2026-06-11T05:20:55Z

Test Results

22 tests 22 ✅ 2s ⏱️
1 suites 0 💤
1 files 0 ❌

Results for commit 1efba9a.

♻️ This comment has been updated with latest results.

smolnar82

A BER-encoded Boolean consists of

Tag: 1 byte (0x01)
Length: 1 byte (The size of the value)
Value: 1 byte (0x00 or 0xFF)
Total: 3 bytes.

The error in the PR is specifically the content of the middle byte (the "Length" field of the TLV).

In BER, the "Length" byte should only represent the size of the Value portion. For a Boolean, the value is only 1 byte long.

Current PR encoding: 0x01 (Tag) | 0x03 (Length) | Value (1 byte) -> This says "expect 3 more bytes" but only provides 1.
Correct BER encoding: 0x01 (Tag) | 0x01 (Length) | Value (1 byte).

Setting the length byte to 0x03 while only providing 1 byte of data might cause standard LDAP clients or strict decoders to fail.

Once this change is done, you may also have to change the documentation.

Great work on the implementation and the tests otherwise!

handavid · 2026-06-11T20:30:14Z

@smolnar82 I've updated the PR for your requested change and also to move the OID into configuration until we get an official OID.

smolnar82 · 2026-06-12T07:43:55Z

+    public void setBypassRolesLookup() {
+        rolesLookupBypassControlDecorator.setBypassRolesLookup(true);
+        assertEquals("isBypassRolesLookup should match the value from the decorated Impl", rolesLookupBypassControl.isBypassRolesLookup(), rolesLookupBypassControlDecorator.isBypassRolesLookup());
+        rolesLookupBypassControlDecorator.setBypassRolesLookup(false);
+        assertEquals("isBypassRolesLookup should match the value from the decorated Impl", rolesLookupBypassControl.isBypassRolesLookup(), rolesLookupBypassControlDecorator.isBypassRolesLookup());
+    }


This is exactly the same test as the previous one. You might want to remove this one and rename the above to testBypassLookup?

minor difference between the tests.
One checks that the decorator.isBypassRolesLookup matches the decorated instance values by setting the values of the decorated instance.
The other checks that the decorator can set the values on the decorated instance.
I'll tweak the tests to make this more obvious.

Thanks, David!

smolnar82 · 2026-06-12T07:46:18Z

+        rolesLookupBypassControl.setBypassRolesLookup(true);
+        byte[] expectedBytes = new byte[]{0x01, 0x01, (byte) 0xff};
+
+        ByteBuffer byteBuffer = ByteBuffer.allocate(3);
+        ByteBuffer encodedBuffer = rolesLookupBypassControlDecorator.encode(byteBuffer);
+        // transition from write mode to read mode
+        encodedBuffer.flip();
+        byte[] encodedBytes = new byte[encodedBuffer.remaining()];
+        encodedBuffer.get(encodedBytes);
+        assertArrayEquals(expectedBytes, encodedBytes);


Maybe creating a new private method (testEncode(boolean encodeTrue)) and call that one from here and from encodeFalseValue? The only difference is the content of expectedBytes, but we'd save some lines of duplication.

smolnar82 · 2026-06-12T07:50:12Z

+        String rolesLookupBypassControlOid = gatewayConfig.getLdapRolesLookupBypassControlOid();
+        if (StringUtils.isNotBlank(rolesLookupBypassControlOid)) {
+            if (!Oid.isOid(rolesLookupBypassControlOid)) {
+                rolesLookupBypassControlOid = null;


I see you are throwing throw new ConfigurationException("Roles Lookup Bypass Control OID is not valid"); below in KnoxLDAPServerManager. However, I think that exception belongs here, when the factory wants to create an instance of LDAPRolesLookupInterceptor.
The creation of this interceptor is also happening in KnoxLDAPServerManager.init() by calling createInterceptors, so it'd fail fast in case someone provided an invalid OID.
What do you think?

I can get rid of this change since we have an OID.

outdated comment because I removed the oid configuration

smolnar82 · 2026-06-12T07:55:53Z

+        Asn1Buffer asn1Buffer = new Asn1Buffer();
+        RolesLookupBypassControl control = new RolesLookupBypassControlImpl(ROLES_LOOKUP_BYPASS_CONTROL_OID);
+        control.setBypassRolesLookup(true);
+
+        rolesLookupBypassControlFactory.encodeValue(asn1Buffer, control);
+
+        // expectedBytes in reverse because Asn1Buffer stores bytes in reverse order
+        byte[] expectedBytes = new byte[]{(byte) 0xff, 0x01, 0x01};
+        System.out.println(asn1Buffer.toString());
+        ByteBuffer encodedBuffer = asn1Buffer.getBytes();
+        byte[] encodedBytes = new byte[encodedBuffer.remaining()];
+        encodedBuffer.get(encodedBytes);
+        assertArrayEquals(expectedBytes, encodedBytes);


I've the same observation here as above wrt. encode/decode testing (one private method would eliminate the majority of duplicated lines here).

smolnar82 · 2026-06-12T08:07:24Z

+
+| Property | Default Value | Description |
+| :--- | :--- | :--- |
+| `gateway.ldap.roles.lookup.bypass.control.oid` | N/A | The OID to use for the bypass control. The control will not be registered if this value is not provided. The Knox LDAP Service will fail to initialize if the value provided is on an OID. |


typo: if the value provided is on an OID. -> if the value provided is not an OID.

removed since OID will not be configurable

smolnar82 · 2026-06-12T13:46:06Z

Hi @handavid !

In addition to the above comments, I see that we have unit test failures now:

Error:  Tests run: 16, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.378 s <<< FAILURE! -- in org.apache.knox.gateway.services.ldap.KnoxLDAPServerManagerTest
Error:  org.apache.knox.gateway.services.ldap.KnoxLDAPServerManagerTest.testStartDontRegistersRolesLookupBypassControl -- Time elapsed: 0.166 s <<< FAILURE!
java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertFalse(Assert.java:65)
	at org.junit.Assert.assertFalse(Assert.java:75)
	at org.apache.knox.gateway.services.ldap.KnoxLDAPServerManagerTest.testStartDontRegistersRolesLookupBypassControl(KnoxLDAPServerManagerTest.java:415)

The good news is that this test may go away, because we now have an official Knox sub-arc under ASF's OID base. See KNOX-3346 for details.
Given the new KNOX OID base (1.3.6.1.4.1.18060.18), we can drop this temporary configuration and add our own as part of this PR. What do you think?

This removes the configurability of the control OID, updates the documentation, and adds a SchemaConstants class to gateway-spi for holding the official OID. Tests were clarified and refactored into helper methods to reduce code duplication.

KNOX-3340: Add Control for LDAPRolesLookupInterceptor

00b7219

This commit adds a RolesLookupBypassControl for use with the LDAPRolesLookupInterceptor. The LDAPRolesLookupInterceptor will skip role mapping if this control is present and true in the request.

handavid commented Jun 11, 2026

View reviewed changes

Comment thread ...er/src/main/java/org/apache/knox/gateway/services/ldap/control/RolesLookupBypassControl.java Outdated

smolnar82 requested changes Jun 11, 2026

View reviewed changes

Comment thread ...main/java/org/apache/knox/gateway/services/ldap/control/RolesLookupBypassControlFactory.java Outdated

handavid added 2 commits June 11, 2026 09:25

fix BER so that length reflects length of value only

b3d0b1f

move OID to configuration

523ff62

minor fix

86a29ff

smolnar82 self-requested a review June 12, 2026 07:28

smolnar82 assigned handavid Jun 12, 2026

smolnar82 added the ldap label Jun 12, 2026

smolnar82 requested changes Jun 12, 2026

View reviewed changes

Remove configurable OID

1efba9a

This removes the configurability of the control OID, updates the documentation, and adds a SchemaConstants class to gateway-spi for holding the official OID. Tests were clarified and refactored into helper methods to reduce code duplication.

handavid force-pushed the knox-3340-role-lookup-groups branch from 412243a to 1efba9a Compare June 12, 2026 21:53

Conversation

handavid commented Jun 11, 2026

What changes were proposed in this pull request?

How was this patch tested?

Integration Tests

UI changes

Uh oh!

handavid commented Jun 11, 2026

Uh oh!

Uh oh!

github-actions Bot commented Jun 11, 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!

Uh oh!

handavid commented Jun 11, 2026

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smolnar82 commented Jun 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Jun 11, 2026 •

edited

Loading