[MINOR] Apply proper RFC 4515 / RFC 4514 escaping in LDAP realms#5226
Open
jongyoul wants to merge 2 commits intoapache:masterfrom
Open
[MINOR] Apply proper RFC 4515 / RFC 4514 escaping in LDAP realms#5226jongyoul wants to merge 2 commits intoapache:masterfrom
jongyoul wants to merge 2 commits intoapache:masterfrom
Conversation
LDAP search filter and DN substitutions in LdapRealm and
ActiveDirectoryGroupRealm previously used either no escaping or RFC 4514
(Distinguished Name) escaping where RFC 4515 (Search Filter) escaping is
required. The metacharacters '(', ')' and '*' are filter syntax characters
under RFC 4515 but pass through RFC 4514 unchanged, so applying the wrong
escape leaves them untouched in filter context.
Changes:
- Add LdapFilterEncoder.escapeFilterValue implementing RFC 4515 section 3
escaping for the metacharacters '\\', '(', ')', '*' and NUL.
- Route user-controlled values in LdapRealm filter substitution sites
(groupSearchFilter, userSearchFilter, userSearchAttributeTemplate)
through a new expandFilterTemplate helper that escapes before
substituting into the filter template.
- Add expandDnTemplate helper that uses the existing escapeAttributeValue
(RFC 4514) for DN substitutions (userDnTemplate, userSearchBase) so the
two escape contexts are clearly separated.
- Apply LdapFilterEncoder.escapeFilterValue at the two String.format
filter construction sites in ActiveDirectoryGroupRealm
(searchForUserName and getRoleNamesForUser).
- Wrap admin-configured object class / attribute name values through the
same escape utility for defense in depth.
Tests:
- LdapFilterEncoderTest (14): unit coverage of the escape utility and the
RFC 4514 vs 4515 character set distinction.
- LdapFilterEncoderFuzzTest (1005): 1000-iteration deterministic fuzz
with random ASCII / metacharacter / Unicode payloads, plus edge cases.
- LdapRealmFilterInjectionTest (9): expandFilterTemplate end-to-end
rendering with realistic templates.
- LdapRealmDnInjectionTest (22): DN substitution path including PoC-style
inputs and Korean username regression.
- ActiveDirectoryGroupRealmFilterInjectionTest (11): mocked LdapContext
capturing the actual filter strings sent to the search call.
- LdapRealmTest: trailing-space expected value adjusted to reflect the
pre-existing RFC 4514 escape behaviour (no functional change).
All 1068 tests in the LDAP realm test set pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens Zeppelin’s LDAP realms against injection by separating escaping rules for LDAP search filters (RFC 4515) vs Distinguished Names (RFC 4514), and routing each substitution site through the appropriate encoder.
Changes:
- Introduces RFC 4515 escaping for filter assertion values and applies it to user-controlled interpolations in
LdapRealmandActiveDirectoryGroupRealm. - Splits template expansion into filter-safe vs DN-safe helpers (
expandFilterTemplatevsexpandDnTemplate) and updates call sites accordingly. - Adds focused unit tests plus injection and deterministic fuzz tests to validate escaping and prevent regressions.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| zeppelin-server/src/main/java/org/apache/zeppelin/realm/LdapFilterEncoder.java | Adds RFC 4515 filter-value escaping utility used across realms. |
| zeppelin-server/src/main/java/org/apache/zeppelin/realm/LdapRealm.java | Routes filter/DN template expansions through context-appropriate escaping helpers. |
| zeppelin-server/src/main/java/org/apache/zeppelin/realm/ActiveDirectoryGroupRealm.java | Escapes interpolated filter components at both String.format filter construction sites. |
| zeppelin-server/src/test/java/org/apache/zeppelin/realm/LdapRealmTest.java | Updates expected getUserDn behavior for RFC 4514 escaping (e.g., trailing space). |
| zeppelin-server/src/test/java/org/apache/zeppelin/realm/LdapFilterEncoderTest.java | Unit tests for RFC 4515 escaping behavior and known PoC payloads. |
| zeppelin-server/src/test/java/org/apache/zeppelin/realm/LdapFilterEncoderFuzzTest.java | Deterministic fuzz/round-trip validation for filter escaping. |
| zeppelin-server/src/test/java/org/apache/zeppelin/realm/LdapRealmFilterInjectionTest.java | Realm-level test ensuring expandFilterTemplate prevents metachar injection. |
| zeppelin-server/src/test/java/org/apache/zeppelin/realm/ActiveDirectoryGroupRealmFilterInjectionTest.java | Mockito-based tests asserting escaped filters are passed to LdapContext#search. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+385
to
+390
| String searchFilter = String.format("(objectclass=%1$s)", | ||
| LdapFilterEncoder.escapeFilterValue(groupObjectClass)); | ||
|
|
||
| // If group search filter is defined in Shiro config, then use it | ||
| if (groupSearchFilter != null) { | ||
| searchFilter = expandTemplate(groupSearchFilter, userName); | ||
| searchFilter = expandFilterTemplate(groupSearchFilter, userName); |
| if ((userSearchBase == null || userSearchBase.isEmpty()) || (userSearchAttributeName == null | ||
| && userSearchFilter == null && !"object".equalsIgnoreCase(userSearchScope))) { | ||
| userDn = expandTemplate(userDnTemplate, matchedPrincipal); | ||
| userDn = expandDnTemplate(userDnTemplate, matchedPrincipal); |
Comment on lines
+1048
to
+1049
| * {@code ,}, {@code =}, {@code +} or leading {@code #} cannot break out of | ||
| * their RDN or inject additional RDNs. |
| package org.apache.zeppelin.realm; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; |
| package org.apache.zeppelin.realm; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; |
Comment on lines
+84
to
+108
| // Drive getRoleNamesForUser indirectly via searchForUserName parameters | ||
| // since getRoleNamesForUser is package-private; this exercises the same | ||
| // String.format filter site at L304 with userPrincipalName. | ||
| LdapContext ctx = mock(LdapContext.class); | ||
| NamingEnumeration<SearchResult> empty = mock(NamingEnumeration.class); | ||
| when(empty.hasMoreElements()).thenReturn(false); | ||
| when(ctx.search(anyString(), anyString(), any(), any(SearchControls.class))) | ||
| .thenReturn(empty); | ||
|
|
||
| ActiveDirectoryGroupRealm realm = new ActiveDirectoryGroupRealm() { | ||
| // Expose the package-private method through a test-only hook. | ||
| java.util.Set<String> testGetRoleNamesForUser(String username, LdapContext c) | ||
| throws javax.naming.NamingException { | ||
| // Mirror the call chain in queryForAuthorizationInfo | ||
| return new java.util.LinkedHashSet<>(); | ||
| } | ||
| }; | ||
| realm.setSearchBase("dc=example,dc=com"); | ||
|
|
||
| // Use searchForUserName as proxy — the same escape utility is applied at | ||
| // both filter sites and the assertion holds for either one. | ||
| realm.searchForUserName(payload, ctx, 100); | ||
|
|
||
| ArgumentCaptor<String> filterCap = ArgumentCaptor.forClass(String.class); | ||
| verify(ctx).search(anyString(), filterCap.capture(), any(), |
- Apply RFC 4515 escaping to the matching-rule-in-chain filter site
(groupObjectClass, memberAttribute, userDn) — previously this branch
used String.format with no escape on values that may include parts of
the principal once group membership is resolved through the chain.
- Preserve the bare-placeholder DN template behaviour: when
userDnTemplate is exactly "{0}", treat the principal as a full DN
supplied verbatim rather than running RFC 4514 escape on it. Escaping
collapsed comma-separated DNs into a single RDN value and broke binds
for deployments that rely on full-DN principals.
- Stop applying RFC 4514 escape inside setUserSearchFilter and
setGroupSearchFilter. The values are operator-supplied filter
templates; the placeholder substitution itself is the only place that
needs escaping, and that already happens at expandFilterTemplate time.
- Update LdapRealmTest expectations to reflect the verbatim template
storage and the bare-placeholder passthrough.
- Switch the second-call-site coverage in
ActiveDirectoryGroupRealmFilterInjectionTest to actually invoke
getRoleNamesForUser via reflection rather than re-running
searchForUserName.
- Drop the unused assertFalse imports flagged by Checkstyle and replace
the raw NUL byte in LdapRealmDnInjectionTest with the Java escape
sequence to keep the source file plain text.
- Tighten the inline comment on the bare-placeholder branch and remove
'=' from the expandDnTemplate Javadoc since RFC 4514 does not require
escaping '=' inside an attribute value.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR for?
LdapRealmandActiveDirectoryGroupRealmbuild LDAP search filters and DNs by interpolating user-controlled values into format strings. The existing escape utility inLdapRealmimplements RFC 4514 (Distinguished Name) escaping, which leaves the filter metacharacters(,)and*unchanged.ActiveDirectoryGroupRealmusesString.formatwith no escape at all. The two contexts need different rules: filter values follow RFC 4515 § 3 and DN values follow RFC 4514.This PR separates the two escape contexts so each substitution site uses the right one.
What type of PR is it?
Improvement
Todos
LdapFilterEncoder.escapeFilterValueimplementing RFC 4515 § 3 (\\,(,),*, NUL).expandFilterTemplatehelper inLdapRealmthat escapes values before substituting into a filter template.expandDnTemplatehelper that uses the existingescapeAttributeValue(RFC 4514) for DN substitutions.LdapRealmfilter substitution sites throughexpandFilterTemplate.LdapRealmDN substitution sites throughexpandDnTemplate.LdapFilterEncoder.escapeFilterValueat the twoString.formatfilter sites inActiveDirectoryGroupRealm.LdapFilterEncoder(14).LdapContext(20 tests).What is the Jira issue?
N/A —
[MINOR]change.How should this be tested?
All 1068 tests pass locally.
Questions