KNOX-3341: LDAP proxy general search — review fixes and hardening#1284
Open
smolnar82 wants to merge 10 commits into
Open
KNOX-3341: LDAP proxy general search — review fixes and hardening#1284smolnar82 wants to merge 10 commits into
smolnar82 wants to merge 10 commits into
Conversation
extractUser() returns null when the filter targets no recognized user identifier (uid/cn/sAMAccountName), e.g. (objectclass=inetOrgPerson). Since FileBackend.search() now delegates every filter to searchUsers(), such general searches threw a NullPointerException on toLowerCase(). Guard the null and return no matches, consistent with the pre-existing behavior of this user-only backend for filters it cannot satisfy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
getUserGroups() with useMemberOf=true and recursiveGroupResolution=true returned an incomplete group list: populateParentCacheViaMemberOf looked up group entries requesting only the "memberOf" attribute, so the cached entries carried no "cn", and getCnsFromEntries silently dropped them. Request "cn" alongside "memberOf" in the lookup, and harden getCnsFromEntries to fall back to the CN in the entry's DN so resolved groups are never dropped when the cn attribute is absent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g entries convertRemoteEntryToProxyEntry copied every attribute from the remote entry verbatim, including credential-bearing attributes (e.g. userPassword), and ran the remote->proxy DN rewrite on every value, which could mangle non-DN values (mail, description) that happen to contain a base DN substring. Skip a denylist of sensitive attributes when building the proxy entry, and only apply DN rewriting to known DN-valued attributes (member, uniqueMember, memberOf, manager, owner, seeAlso). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
convertRemoteDnToProxyDn and convertProxyDnToRemoteDn used the base DNs directly as regex patterns and the targets as replacement strings, so a base DN containing regex metacharacters (or a replacement containing $ or \) would break or corrupt the conversion. Introduce a replaceIgnoreCase helper that treats both operands as literals via Pattern.quote / Matcher.quoteReplacement while keeping the case-insensitive, most-specific-first replacement behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Minor correctness/readability cleanups with no behavior change:
- DisabledUserInterceptor.search: remove the dead catch(Exception){throw e;}
- LdapProxyBackend: drop stale "tODO" comment; log search failures in
search() via ldapSearchFailed instead of the misleading
ldapAttributeCopyError
- RemoteSchemaConverter: null-check the objectclass attribute before use,
and narrow convertProxyFilterToRemoteFilter from throws Exception to
throws ParseException
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
UserSearchInterceptor.search forwarded every search to the backend after the PR, including ApacheDS internal/operational searches (ou=schema, cn=config, root-DSE) that should never be proxied to a remote LDAP server. Add isUnderBackendBaseDn() and skip the backend.search() call when the search base is not a suffix of the backend's base DN. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erage
Two bugs in ldap-recursive-test.ldif:
- memberOflevel2 had a mismatched cn value ("memberOflevel2level2")
- its member attribute referenced the wrong OU (recursiveGroups instead
of recursiveMemberOfGroups), so the chain was broken
Also fix a null-return from LdapConnection.lookup() in
populateParentCacheViaMemberOf: the API returns null (not LdapException)
when an entry does not exist, which caused an NPE on the returned entry.
Fall back to a skeleton entry in both the exception and null cases.
Add testGetUserGroupsUseMemberOfRecursive and
testGetUserGroupsUseMemberOfRecursiveDepth2 to cover the
useMemberOf+recursiveGroupResolution path through getUserGroups(),
which was previously untested.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ptorType values service_ldap_server.md had a duplicated proxy.poolMaxActive table row introduced in the PR. Also the interceptorType summary property row only listed "backend" and "duplicateuserfilter", omitting the newly-added "disableduserfilter" and the existing "rolesLookup" types. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The existing integration coverage only exercised bind and recursive group resolution (Shiro authenticates via a userDnTemplate bind, not a search), so the new LdapProxyBackend.search() path had no end-to-end coverage. Add test_knox_ldap_proxy_search.py: it binds to the embedded Knox LDAP service (which proxies to the demo LDAP backend) and searches by objectClass, cn wildcard and uid, validating that general searches are proxied and converted correctly. Adds ldap3 to the test requirements. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Cc. @handavid |
smolnar82
changed the title
Test Results32 tests 32 ✅ 3s ⏱️ Results for commit aa086a4. |
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
1 participant
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.
KNOX-3341 - LDAP proxy general search: review follow-ups and hardening
What changes were proposed in this pull request?
This is a follow-up to #1274 (KNOX-3341, "LDAP proxy backend handles general searches"). A post-merge review surfaced a few correctness gaps and some hardening/cleanup opportunities in the new proxy/search code.
Correctness fixes
FileBackend.searchUsersNPE on non-user filters.extractUser()returnsnullwhen a filter targets no recognized user identifier (e.g.(objectclass=inetOrgPerson)). SinceFileBackend.search()now delegates every filter tosearchUsers(), such general searches threw an NPE ontoLowerCase(). The null is now guarded (the user-only backend matches nothing for filters it cannot satisfy).memberOfresolution.getUserGroups()withuseMemberOf=trueandrecursiveGroupResolution=truereturned an incomplete list: parent groups were looked up requesting only thememberOfattribute, so the cached entries carried nocnandgetCnsFromEntriessilently dropped them. The lookup now requestscnas well, andgetCnsFromEntriesfalls back to the CN in the entry's DN. Also fixes an NPE whenLdapConnection.lookup()returnsnull(rather than throwing) for a missing entry.Hardening
convertRemoteEntryToProxyEntrycopied every remote attribute verbatim (including credential-bearing ones such asuserPassword) and ran the remote→proxy DN rewrite on every value, which could mangle non-DN values (mail,description) containing a base-DN substring. Sensitive attributes are now skipped, and DN rewriting is applied only to known DN-valued attributes (member,uniqueMember,memberOf,manager,owner,seeAlso).convertRemoteDnToProxyDn/convertProxyDnToRemoteDnused the base DNs directly as regex patterns and the targets as replacement strings. They now usePattern.quote/Matcher.quoteReplacement(case-insensitive, most-specific-first) so a DN with regex metacharacters can't break the conversion.UserSearchInterceptor.searchforwarded every search to the backend, including ApacheDS internal/operational searches (ou=schema,cn=config, root-DSE). It now skips the backend call when the search base is not under the backend's base DN.Cleanups
catch (Exception e) { throw e; }inDisabledUserInterceptortODOcommentobjectclassattribute before useconvertProxyFilterToRemoteFilterfromthrows Exceptiontothrows ParseExceptionldapAttributeCopyErrorlog insearch()toldapSearchFailed.Docs
knox-site/docs/service_ldap_server.md: removed a duplicatedproxy.poolMaxActivetable row and completed theinterceptorTypevalue list (disableduserfilter,rolesLookup).How was this patch tested?
gateway-server): the fullorg.apache.knox.gateway.services.ldap.**suite passes.Added
testGetUserGroupsUseMemberOfRecursiveandtestGetUserGroupsUseMemberOfRecursiveDepth2to cover the previously-untesteduse MemberOf+ recursivegetUserGroups()path, and fixed two data bugs inldap-recursive-test.ldif(a mismatchedcnand a wrong-OUmemberreference) that had masked the dropped-CN bug.32 passed), including the new LDAP search test, andpylintrates the test sources10.00/10.Integration Tests
Added
.github/workflows/tests/test_knox_ldap_proxy_search.py(andldap3torequirements.txt).The existing integration coverage only exercised bind and recursive group resolution (Shiro authenticates via a
userDnTemplatebind, not a search) so the newLdapProxyBackend.search()path had no end-to-end coverage.The new test binds to the embedded Knox LDAP service (which proxies to the demo LDAP backend) and searches by
objectClass,cnwildcard, anduid, validating that general searches are proxied and converted correctly.UI changes
N/A