[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.
- 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>
ParkGyeongTae
approved these changes
May 9, 2026
Member
ParkGyeongTae
left a comment
ParkGyeongTae
left a comment
There was a problem hiding this comment.
Thanks for this fix. The separation between RFC 4515 (filter) and RFC 4514 (DN) escaping contexts is exactly right, and LdapFilterEncoder.escapeFilterValue() covers all five metacharacters required by the spec. The injection points in both LdapRealm and ActiveDirectoryGroupRealm are consistently patched, and wrapping admin-configured values like objectClass and memberAttribute is a nice defense-in-depth touch.
The test coverage is solid — the fuzz test in particular gives good confidence across a wide input space. Verified locally: all 1,066 tests pass.
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