[DarshitChanpura]

Description

Nested LDAP role resolution now skips a group DN that has no entry or that the LDAP server rejects, and the other groups still resolve. DNs that contain an escaped control character such as \0A now reach the server escaped.

• Category: Bug fix
• Why these changes are required? A group whose member or nested-group DN points to an entry that does not exist made the whole role resolution fail, so the user got no backend roles at all.
• What is the old behavior before changes and new behavior after changes?
  • Before: a DN with no entry failed with NO_SUCH_OBJECT, and a referral to a DN with no entry failed with a NullPointerException in resolveNestedRoles. Either failure removed every backend role of the user.
  • After:
    • escapeDn escapes control characters as \XX. Rdn does not escape them, so a DN such as CN=SCA-LDAP\0ACNF:... (an Active Directory conflict object) reached the server with a raw newline, and the server rejected the DN of a group that exists.
    • LdapHelper.lookup returns null for NO_SUCH_OBJECT, the same result it already gives when the search does not return one entry. Every caller of lookup already handles null. Other errors, including INVALID_DN_SYNTAX, are thrown as before.
    • resolveNestedRoles in both backends returns no roles for a DN without an entry. It does not run the role search for that DN, because a match on it can only be a stale reference, for example a group whose uniqueMember still names a deleted group.
    • resolveNestedRoles skips a DN that the server rejects with INVALID_DN_SYNTAX, with its own warning, and continues with the other groups.
    • getRoleFromEntry logs one warning per missing DN, in both the nested and the flat role path. Both backends skip a null role name, which User.withRoles rejected.

Issues Resolved

Resolves #6060

Testing

• New tests in LdapBackendTest (ldap backend) and in LdapBackendTestOldStyleConfig2 and LdapBackendTestNewStyleConfig2 (ldap2 backend), with new base.ldif fixtures: a group that points to a missing DN, a nested group that points to a missing DN, a referral to a missing DN, and a group with a stale uniqueMember reference to a missing DN. Each test asserts the exact role set, so the stale group must not appear.
• LdapHelperTest checks that escapeDn keeps \0A escaped. testResolveNestedRolesSkipsDnRejectedByServer uses a mocked connection that answers INVALID_DN_SYNTAX, because the embedded server answers NO_SUCH_OBJECT for every DN tried.
• Each new test fails with its fix reverted: the escaping test without the control-character escape, the missing-entry tests (they get staleparent) when the role search still runs, and the rejected-DN test without the skip. With all changes, the LDAP test classes pass (314 tests, 0 failures).
• spotlessCheck passes.

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit cec045f)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Behavior change in escapeDn escapeDn now also escapes control characters (c < 0x20 or 0x7F) via escapeControlCharacters. This changes the output for any DN whose RDN value contains raw control characters (e.g. a literal newline), which previously was passed through to the server as-is. The new test only confirms that a DN with an already-escaped \0A sequence is preserved, but does not cover the raw-control-character path. If any legitimate caller relied on the previous behavior (e.g. multi-line DN continuations that LdapName parses but which included real control bytes), the DN sent to the server now differs. Please confirm this change is intentional and consider adding a test for a raw control character input. // Rdn does not escape control characters, so a \0A in a value would reach the server as a raw newline. private static String escapeControlCharacters(String dn) { final StringBuilder sb = new StringBuilder(dn.length()); for (int i = 0; i < dn.length(); i++) { final char c = dn.charAt(i); if (c < 0x20 || c == 0x7f) { sb.append(String.format(Locale.ROOT, "\\%02X", (int) c)); } else { sb.append(c); } } return sb.toString(); } Possible flaky/unused test setup testResolveNestedRolesSkipsDnRejectedByServer constructs NestedRoleResolver with real LDAP host settings (localhost:${ldapPort}) but then invokes resolveNestedRoles with a mocked connection. If the superclass constructor opens or validates a connection to the embedded server during construction, the test depends on the embedded server being up for a code path that is otherwise purely mock-driven. If instead construction is lazy, the test is fine - please verify this doesn't accidentally create an unrelated dependency. public void testResolveNestedRolesSkipsDnRejectedByServer() throws Exception { final ProviderConnection providerConnection = Mockito.mock(ProviderConnection.class); Mockito.when(providerConnection.search(Mockito.any())).thenThrow(new LdapException("invalid DN", ResultCode.INVALID_DN_SYNTAX)); final Connection connection = Mockito.mock(Connection.class); Mockito.when(connection.getProviderConnection()).thenReturn(providerConnection); final Set<LdapName> nestedRoles = new NestedRoleResolver().resolve(new LdapName("CN=SCA-LDAP\\0ACNF,ou=groups,o=TEST"), connection); assertThat(nestedRoles, is(empty())); } private class NestedRoleResolver extends LDAPAuthorizationBackend { NestedRoleResolver() throws Exception { super( Settings.builder() .putList(ConfigConstants.LDAP_HOSTS, "localhost:" + ldapPort) .put(ConfigConstants.LDAP_AUTHZ_RESOLVE_NESTED_ROLES, true) .build(), null ); } Set<LdapName> resolve(final LdapName roleDn, final Connection connection) throws Exception { return resolveNestedRoles(roleDn, connection, "description", 0, true, Set.of()); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to cec045f

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle invalid-DN consistently in role lookup getRoleFromEntry also calls LdapHelper.lookup and does not catch INVALID_DN_SYNTAX, so a stale DN that the server rejects there will still bubble up as an LdapException and abort the whole authorization. For consistency with the resolveNestedRoles fix, getRoleFromEntry should also treat INVALID_DN_SYNTAX as "skip this role" rather than propagating. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [998-1007] +final LdapEntry e0; +try { + e0 = LdapHelper.lookup(ldapConnection, roleDn.toString(), this.returnAttributes, this.shouldFollowReferrals); +} catch (final LdapException e) { + if (e.getResultCode() != ResultCode.INVALID_DN_SYNTAX) { + throw e; + } + log.warn("Skipping nested roles of {} because the LDAP server rejected the DN as invalid", roleDn); + return result; +} - Suggestion importance[1-10]: 7 __ Why: Valid observation that getRoleFromEntry doesn't handle INVALID_DN_SYNTAX the same way, which could cause stale DN references to abort authorization. However, improved_code is identical to existing_code and doesn't actually show the fix in getRoleFromEntry.	Medium
	Escape control chars at RDN value level The escape loop iterates over char units, so supplementary Unicode code points encoded as surrogate pairs are inspected as individual UTF-16 surrogates (0xD800–0xDFFF), which do not satisfy the control-character predicate but can still produce malformed output if combined with the %02X branch for other reasons. More importantly, this method also re-escapes characters inside already-escaped sequences produced by Rdn (e.g., the </code> inside \0A); since </code> is 0x5C it is left alone, but any value already containing a literal control character that Rdn chose to quote rather than hex-escape will be double-processed. Consider restricting escaping to RDN values only (before building the final LdapName) rather than post-processing the serialized DN string, to avoid corrupting structural characters or already-escaped forms. src/main/java/org/opensearch/security/auth/ldap/util/LdapHelper.java [123-134] +private static String escapeControlCharacters(String dn) { + final StringBuilder sb = new StringBuilder(dn.length()); + for (int i = 0; i < dn.length(); i++) { + final char c = dn.charAt(i); + if (c < 0x20 || c == 0x7f) { + sb.append(String.format(Locale.ROOT, "\\%02X", (int) c)); + } else { + sb.append(c); + } + } + return sb.toString(); +} - Suggestion importance[1-10]: 3 __ Why: The suggestion raises a theoretical concern about escaping control characters at the serialized DN level, but the existing_code and improved_code are identical, offering no concrete fix. The concern about surrogate pairs and double-processing is speculative given the context.	Low

---

Previous suggestions

Suggestions up to commit 0b95cb5

Category	Suggestion	Impact
General	Null-safe result code comparison LdapException.getResultCode() can return null when the exception is not server-originated (e.g. a wrapped I/O failure). The current check e.getResultCode() != ResultCode.INVALID_DN_SYNTAX will correctly rethrow in that case, but relying on != with a possibly-null left side is fine; however the symmetric code in LdapHelper.lookup compares e.getResultCode() == ResultCode.NO_SUCH_OBJECT which silently swallows unrelated exceptions whose result code happens to match. Confirm both call sites tolerate a null result code and consistently rethrow for unexpected codes to avoid masking real connectivity failures. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [998-1007] final LdapEntry e0; try { e0 = LdapHelper.lookup(ldapConnection, roleDn.toString(), this.returnAttributes, this.shouldFollowReferrals); } catch (final LdapException e) { - if (e.getResultCode() != ResultCode.INVALID_DN_SYNTAX) { + if (!ResultCode.INVALID_DN_SYNTAX.equals(e.getResultCode())) { throw e; } log.warn("Skipping nested roles of {} because the LDAP server rejected the DN as invalid", roleDn); return result; } Suggestion importance[1-10]: 3 __ Why: Using ResultCode.INVALID_DN_SYNTAX.equals(e.getResultCode()) is a minor defensive improvement for null safety, but != on an enum reference with null operand also behaves correctly (rethrows), making the fix of limited impact.	Low
	Guard hex escape output width Iterating by char and escaping any code unit < 0x20 will corrupt the low surrogate of any supplemental Unicode character (whose low surrogate falls in the 0xDC00–0xDFFF range — not < 0x20, so safe), but it will also incorrectly escape bytes inside already-escaped \XX hex sequences only if raw control chars appear — however the real risk is that String.format with %02X on a char cast to int can emit more than two hex digits for values above 0xFF if the condition were ever broadened. More importantly, this method runs after LdapName has already produced the escaped form, so re-processing may re-escape backslashes-sequences inconsistently; consider restricting escaping to actual control chars only and documenting that the input must already be in LDAP string form. src/main/java/org/opensearch/security/auth/ldap/util/LdapHelper.java [125-132] for (int i = 0; i < dn.length(); i++) { final char c = dn.charAt(i); - if (c < 0x20 || c == 0x7f) { - sb.append(String.format(Locale.ROOT, "\\%02X", (int) c)); + if ((c < 0x20) || c == 0x7f) { + sb.append('\\').append(String.format(Locale.ROOT, "%02X", c & 0xff)); } else { sb.append(c); } } Suggestion importance[1-10]: 2 __ Why: The suggestion is largely speculative; %02X on a char value < 0x20 or == 0x7f always produces exactly two hex digits, so the stated concern is not a real bug. The alternative code is functionally equivalent.	Low

Suggestions up to commit eab4f89

Category	Suggestion	Impact
Possible issue	Ensure wrapped exceptions are unwrapped The search method wraps the underlying LdapException inside a RuntimeException for InvalidNameException and executes the search inside a privileged action. Verify that e.getResultCode() returns the expected code here; if the underlying ldaptive exception is wrapped (e.g., via PrivilegedActionException or RuntimeException), the ResultCode check will never match and dangling DN entries will still throw. Consider unwrapping the cause before inspecting the result code. src/main/java/org/opensearch/security/auth/ldap/util/LdapHelper.java [92-100] final List<LdapEntry> entries; try { entries = search(conn, unescapedDn, ALL, SearchScope.OBJECT, returnAttributes, shouldFollowReferrals); } catch (final LdapException e) { - if (e.getResultCode() == ResultCode.NO_SUCH_OBJECT || e.getResultCode() == ResultCode.INVALID_DN_SYNTAX) { + final ResultCode rc = e.getResultCode(); + if (rc == ResultCode.NO_SUCH_OBJECT || rc == ResultCode.INVALID_DN_SYNTAX) { return null; } throw e; } Suggestion importance[1-10]: 4 __ Why: The concern about exception wrapping is valid in principle, but the improved_code is essentially identical to the existing_code (only extracting the result code into a local variable), and the suggestion itself admits it's only a "verify" request. The PR tests appear to confirm the behavior works correctly, so the impact is marginal.	Low

Suggestions up to commit 95bcdb7

Category	Suggestion	Impact
General	Also detect wrapped LDAP exceptions search wraps the underlying LdapException inside a RuntimeException via its InvalidNameException branch and uses AccessController.doPrivileged, which may also rewrap exceptions. Catching only LdapException here can miss the wrapped NO_SUCH_OBJECT/INVALID_DN_SYNTAX cases, defeating the null-return behavior. Inspect the cause chain to robustly detect these result codes. src/main/java/org/opensearch/security/auth/ldap/util/LdapHelper.java [92-100] final List<LdapEntry> entries; try { entries = search(conn, unescapedDn, ALL, SearchScope.OBJECT, returnAttributes, shouldFollowReferrals); } catch (final LdapException e) { if (e.getResultCode() == ResultCode.NO_SUCH_OBJECT || e.getResultCode() == ResultCode.INVALID_DN_SYNTAX) { return null; } throw e; +} catch (final RuntimeException e) { + Throwable cause = e.getCause(); + while (cause != null) { + if (cause instanceof LdapException) { + ResultCode rc = ((LdapException) cause).getResultCode(); + if (rc == ResultCode.NO_SUCH_OBJECT || rc == ResultCode.INVALID_DN_SYNTAX) { + return null; + } + break; + } + cause = cause.getCause(); + } + throw e; } Suggestion importance[1-10]: 3 __ Why: The suggestion is somewhat speculative; search only wraps InvalidNameException in RuntimeException, not LdapException. The added defensive handling may be unnecessary and adds complexity without clear evidence of the actual wrapping behavior described.	Low

Suggestions up to commit e7a55d2

Category	Suggestion	Impact
Possible issue	Also handle invalid DN syntax exceptions search() wraps InvalidNameException in a RuntimeException, so a syntactically invalid DN will not surface as an LdapException with INVALID_DN_SYNTAX and will still blow up the resolution. Catch (or pre-validate) that case too so dangling/invalid DN references are consistently skipped with a null return. src/main/java/org/opensearch/security/auth/ldap/util/LdapHelper.java [92-100] final List<LdapEntry> entries; try { entries = search(conn, unescapedDn, ALL, SearchScope.OBJECT, returnAttributes, shouldFollowReferrals); } catch (final LdapException e) { if (e.getResultCode() == ResultCode.NO_SUCH_OBJECT || e.getResultCode() == ResultCode.INVALID_DN_SYNTAX) { return null; } throw e; +} catch (final RuntimeException e) { + if (e.getCause() instanceof InvalidNameException) { + return null; + } + throw e; } Suggestion importance[1-10]: 6 __ Why: Valid concern: search() wraps InvalidNameException in a RuntimeException, so invalid DNs may not be caught by the LdapException handler. Handling this consistently would make the dangling-reference logic more robust, though it's a secondary edge case.	Low
General	Return early when nested lookup entry is missing When e0 is null the method proceeds to build and return an empty result, but it still returns an empty set with no entry added for roleDn. Consider also returning early (or adding roleDn to result if that matches the intended semantics) rather than falling through to the role-search logic that follows, which may produce misleading downstream behavior if the DN has no entry at all. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [999-1003] if (e0 == null) { if (isDebugEnabled) { log.debug("Cannot resolve nested roles from the attributes of {} because no entry was found for this DN", roleDn); } + return result; } else if (e0.getAttribute(userRoleName) != null) { Suggestion importance[1-10]: 3 __ Why: The current flow already handles the null e0 case by skipping the attribute processing and proceeding to any subsequent logic; the suggested early return is a minor stylistic change whose correctness depends on code not shown in the diff.	Low

Suggestions up to commit 94a1e1d

Category	Suggestion	Impact
Possible issue	Return early when entry is null When e0 is null, the method should still return the empty result early or skip subsequent code that may dereference e0. Verify that no later code in resolveNestedRoles accesses e0 after this guard, otherwise an NPE will still occur further down. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [999-1001] if (e0 == null) { log.warn("Cannot resolve nested roles from the attributes of {} because no entry was found for this DN", roleDn); + return result; } else if (e0.getAttribute(userRoleName) != null) { Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about potential NPE if later code in resolveNestedRoles dereferences e0, but without visibility into the rest of the method it's hard to confirm this is actually necessary. Primarily a verification-style suggestion.	Low
General	Handle potentially wrapped LdapException The search method wraps LdapException inside a RuntimeException via its InvalidNameException/privileged-action handling path, which may re-wrap LdapException thrown by the underlying operation. Verify that LdapException propagates unwrapped so the ResultCode.NO_SUCH_OBJECT / INVALID_DN_SYNTAX branch is actually reachable; otherwise also handle the wrapped case. src/main/java/org/opensearch/security/auth/ldap/util/LdapHelper.java [92-100] final List<LdapEntry> entries; try { entries = search(conn, unescapedDn, ALL, SearchScope.OBJECT, returnAttributes, shouldFollowReferrals); } catch (final LdapException e) { if (e.getResultCode() == ResultCode.NO_SUCH_OBJECT || e.getResultCode() == ResultCode.INVALID_DN_SYNTAX) { return null; } throw e; +} catch (final RuntimeException e) { + if (e.getCause() instanceof LdapException) { + final LdapException cause = (LdapException) e.getCause(); + if (cause.getResultCode() == ResultCode.NO_SUCH_OBJECT || cause.getResultCode() == ResultCode.INVALID_DN_SYNTAX) { + return null; + } + } + throw e; } Suggestion importance[1-10]: 4 __ Why: The concern about LdapException being wrapped in a RuntimeException is speculative; the search method only wraps InvalidNameException, not LdapException. The suggestion mostly asks to verify behavior and the added catch block may be unnecessary.	Low

[github-actions]

Persistent review updated to latest commit e7a55d2

[codecov]

Codecov Report

❌ Patch coverage is 77.27273% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.34%. Comparing base (e344b80) to head (cec045f).

Files with missing lines	Patch %	Lines
...security/auth/ldap2/LDAPAuthorizationBackend2.java	66.66%	2 Missing and 3 partials ⚠️
...ty/auth/ldap/backend/LDAPAuthorizationBackend.java	73.33%	1 Missing and 3 partials ⚠️
...opensearch/security/auth/ldap/util/LdapHelper.java	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6697      +/-   ##
==========================================
+ Coverage   77.23%   77.34%   +0.11%     
==========================================
  Files         487      487              
  Lines       32277    32311      +34     
  Branches     4874     4883       +9     
==========================================
+ Hits        24928    24990      +62     
+ Misses       5124     5093      -31     
- Partials     2225     2228       +3     

Files with missing lines	Coverage Δ
...opensearch/security/auth/ldap/util/LdapHelper.java	86.04% <92.85%> (+2.17%)	⬆️
...ty/auth/ldap/backend/LDAPAuthorizationBackend.java	68.31% <73.33%> (+0.67%)	⬆️
...security/auth/ldap2/LDAPAuthorizationBackend2.java	58.05% <66.66%> (+8.83%)	⬆️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Persistent review updated to latest commit 95bcdb7

[github-actions]

Persistent review updated to latest commit eab4f89

[github-actions]

Persistent review updated to latest commit 0b95cb5

[github-actions]

Persistent review updated to latest commit cec045f