Skip to content

KNOX-3340: Add Control for LDAPRolesLookupInterceptor#1258

Open
handavid wants to merge 1 commit into
apache:masterfrom
handavid:knox-3340-role-lookup-groups
Open

KNOX-3340: Add Control for LDAPRolesLookupInterceptor#1258
handavid wants to merge 1 commit into
apache:masterfrom
handavid:knox-3340-role-lookup-groups

Conversation

@handavid

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

@smolnar82

// OID created from a UUID to ensure no collisions:
// Apache Root OID for core object classes 1.3.6.1.4.1.18060.2
// UUID "5236bee0-8a22-4419-9f8e-f1de43312ce1"
String OID = "1.3.6.1.4.1.18060.2.1379319520.35362.17433.40846.265936912329953";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smolnar82 do you know how to get an official OID? This OID is generated from a UUID so we shouldn't have any collisions, but it would be nice to get an official OID before this control actually enters use.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an excellent question, @handavid !

You are correct that this UUID-based approach ensures no collisions, but we should aim for a cleaner, official arc under the Apache Private Enterprise Number (PEN).

As a PMC member, I can help facilitate this. Here is the path forward:

  1. Apache Root: The ASF owns PEN 18060, so our official OID will start with 1.3.6.1.4.1.18060.
  2. Knox Sub-arc: I've checked the IANA registry and confirmed Knox doesn't have an explicit entry because we fall under the Apache root. The PMC needs to request a dedicated sub-arc (e.g., 1.3.6.1.4.1.18060.X) for Knox by opening a JIRA with ASF Infrastructure (INFRA). - I think @lmccay can help us with that.
  3. Next Steps: For this PR, it's fine to keep the UUID-based OID as a placeholder so we don't block progress. Once we secure the official Knox sub-arc from INFRA, we can do a quick follow-up task to update this to a shorter, official OID (like 1.3.6.1.4.1.18060.X.1.1).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed KNOX-3346 and assigned to @lmccay to create an official sub-arc for the Knox project under Apache.

@github-actions

Copy link
Copy Markdown

Test Results

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

Results for commit 00b7219.

@smolnar82 smolnar82 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

RolesLookupBypassControl rolesLookupBypassControl = (RolesLookupBypassControl) control;

buffer.put(BOOLEAN_TAG_BYTE);
buffer.put((byte) 3);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct length is 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants