smolnar82 opened a new pull request, #1284: URL: https://github.com/apache/knox/pull/1284
[KNOX-3341](https://issues.apache.org/jira/browse/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. Each item below is a sep arate commit. **Correctness fixes** - **`FileBackend.searchUsers` NPE on non-user filters.** `extractUser()` returns `null` when a filter targets no recognized user identifier (e.g. `(objectclass=inetOrgPerson)`). Since `FileBackend.search()` now delegates every filter to `searchUsers()`, such general searches threw an NPE on `toLowerCase()`. The null is now guarded (the user-only backend matches nothing for filters it cannot satisfy). - **Group CNs dropped in recursive `memberOf` resolution.** `getUserGroups()` with `useMemberOf=true` and `recursiveGroupResolution=true` returned an incomplete list: parent groups were looked up requesting only the `memberOf` attribute, so the cached entries carried no `cn` and `getCnsFromEntries` silently dropped them. The lookup now requests `cn` as well, and `getCnsFromEntries` falls back to the CN in the entry's DN. Also fixes an NPE when `LdapConnection.lookup()` returns `null` (rather than throwing) for a missing entry. **Hardening** - **Don't expose secrets or corrupt non-DN values when proxying entries.** `convertRemoteEntryToProxyEntry` copied every remote attribute verbatim (including credential-bearing ones such as `userPassword`) 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`). - **Regex-safe DN conversion.** `convertRemoteDnToProxyDn`/`convertProxyDnToRemoteDn` used the base DNs directly as regex patterns and the targets as replacement strings. They now use `Pattern.quote`/`Matcher.quoteReplacement` (case-insensitive, most-specific-first) so a DN with regex metacharacters can't break the conversion. - **Only forward searches under the backend's namespace.** `UserSearchInterceptor.search` forwarded 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** - Removed a dead `catch (Exception e) { throw e; }` in `DisabledUserInterceptor` - dropped a stale `tODO` comment - null-checked the `objectclass` attribute before use - narrowed `convertProxyFilterToRemoteFilter` from `throws Exception` to `throws ParseException` - and switched a misleading `ldapAttributeCopyError` log in `search()` to `ldapSearchFailed`. **Docs** - Fixed `knox-site/docs/service_ldap_server.md`: removed a duplicated `proxy.poolMaxActive` table row and completed the `interceptorType` value list (`disableduserfilter`, `rolesLookup`). ## How was this patch tested? - **Unit tests** (`gateway-server`): the full `org.apache.knox.gateway.services.ldap.**` suite passes. Added `testGetUserGroupsUseMemberOfRecursive` and `testGetUserGroupsUseMemberOfRecursiveDepth2` to cover the previously-untested `use MemberOf` + recursive `getUserGroups()` path, and fixed two data bugs in `ldap-recursive-test.ldif` (a mismatched `cn` and a wrong-OU `member` reference) that had masked the dropped-CN bug. - **Docker integration tests**: the full compose suite passes (`32 passed`), including the new LDAP search test, and `pylint` rates the test sources `10.00/10`. ## Integration Tests Added `.github/workflows/tests/test_knox_ldap_proxy_search.py` (and `ldap3` to `requirements.txt`). 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. The new test 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. ## UI changes N/A -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
