This is an automated email from the ASF dual-hosted git repository. smolnar82 pushed a commit to branch knox_idf in repository https://gitbox.apache.org/repos/asf/knox.git
commit de9d8ddb310db5ad50b729032a3a4afd739d610e Author: Sandor Molnar <[email protected]> AuthorDate: Wed Jul 1 09:21:26 2026 +0200 KNOX-3341: LDAP proxy general search - review fixes and hardening (#1284) (cherry picked from commit 1e257783be8e8b2186bf05dad6cb65c573d63031) --- .github/workflows/tests/requirements.txt | 3 +- .../workflows/tests/test_knox_ldap_proxy_search.py | 95 ++++++++++++++++++++++ .../gateway/services/ldap/backend/FileBackend.java | 8 +- .../services/ldap/backend/LdapProxyBackend.java | 18 ++-- .../ldap/backend/RemoteSchemaConverter.java | 91 +++++++++++++++------ .../ldap/interceptor/DisabledUserInterceptor.java | 2 - .../ldap/interceptor/UserSearchInterceptor.java | 23 +++++- .../ldap/backend/LdapProxyBackendTest.java | 27 ++++++ .../src/test/resources/ldap-recursive-test.ldif | 4 +- knox-site/docs/service_ldap_server.md | 3 +- 10 files changed, 232 insertions(+), 42 deletions(-) diff --git a/.github/workflows/tests/requirements.txt b/.github/workflows/tests/requirements.txt index 9f223fca6..7d2d5b5fd 100644 --- a/.github/workflows/tests/requirements.txt +++ b/.github/workflows/tests/requirements.txt @@ -1,3 +1,4 @@ requests==2.32.4 pytest==8.3.4 -pylint==4.0.5 \ No newline at end of file +pylint==4.0.5 +ldap3==2.9.1 \ No newline at end of file diff --git a/.github/workflows/tests/test_knox_ldap_proxy_search.py b/.github/workflows/tests/test_knox_ldap_proxy_search.py new file mode 100644 index 000000000..9d015551e --- /dev/null +++ b/.github/workflows/tests/test_knox_ldap_proxy_search.py @@ -0,0 +1,95 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to you under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Integration tests for general LDAP search through the embedded Knox LDAP proxy. + +These exercise the LdapProxyBackend.search() path (KNOX-3341): clients can query +the embedded Knox LDAP service - which proxies to the demo LDAP backend - by +objectClass, cn and uid (including wildcards), not just by a single uid lookup. +""" + +from __future__ import annotations + +import os +import unittest +from urllib.parse import urlparse + +import ldap3 + +# The embedded Knox LDAP service port (see gateway-site.xml: gateway.ldap.port). +KNOX_LDAP_PORT = 33390 + +BASE_DN = "dc=hadoop,dc=apache,dc=org" +PEOPLE_BASE = f"ou=people,{BASE_DN}" +GROUPS_BASE = f"ou=groups,{BASE_DN}" + +# A valid backend user used to bind to the proxy before searching. +BIND_DN = f"uid=guest,{PEOPLE_BASE}" +BIND_PASSWORD = "guest-password" + + +def knox_host() -> str: + """Derive the Knox host from KNOX_GATEWAY_URL (defaults to localhost).""" + url = os.environ.get("KNOX_GATEWAY_URL", "https://localhost:8443/";) + return urlparse(url).hostname or "localhost" + + +class TestKnoxLdapProxySearch(unittest.TestCase): + """Verify general search requests are proxied to the demo LDAP backend.""" + + def setUp(self) -> None: + server = ldap3.Server(knox_host(), port=KNOX_LDAP_PORT, get_info=ldap3.NONE) + self.connection = ldap3.Connection( + server, user=BIND_DN, password=BIND_PASSWORD, auto_bind=True + ) + + def tearDown(self) -> None: + self.connection.unbind() + + def rdn_values(self, base: str, ldap_filter: str) -> list[str]: + """Run a subtree search and return the leading RDN value of each entry.""" + self.connection.search(base, ldap_filter, search_scope=ldap3.SUBTREE) + values = [] + for entry in self.connection.entries: + # entry_dn looks like "uid=guest,ou=people,..." or "cn=level1,ou=groups,..." + first_rdn = entry.entry_dn.split(",", 1)[0] + values.append(first_rdn.split("=", 1)[1]) + return values + + def test_search_all_users_by_objectclass(self) -> None: + """All inetOrgPerson entries under ou=people are returned.""" + users = self.rdn_values(PEOPLE_BASE, "(objectClass=inetOrgPerson)") + for expected in ("guest", "admin", "sam", "tom", "recursiveUser"): + self.assertIn(expected, users) + + def test_search_all_groups_by_objectclass(self) -> None: + """All groupOfNames entries under ou=groups are returned.""" + groups = self.rdn_values(GROUPS_BASE, "(objectClass=groupOfNames)") + for expected in ("analyst", "scientist", "admin", "level1", "level2", "level3"): + self.assertIn(expected, groups) + + def test_search_groups_by_cn_wildcard(self) -> None: + """A cn wildcard filter returns only the matching groups.""" + groups = self.rdn_values(GROUPS_BASE, "(cn=level*)") + self.assertEqual({"level1", "level2", "level3"}, set(groups)) + + def test_search_user_by_uid(self) -> None: + """A single user can still be looked up by uid.""" + users = self.rdn_values(PEOPLE_BASE, "(uid=sam)") + self.assertIn("sam", users) + + +if __name__ == "__main__": + unittest.main() diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/FileBackend.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/FileBackend.java index 9568f2471..872d1c764 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/FileBackend.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/FileBackend.java @@ -153,7 +153,13 @@ public class FileBackend implements LdapBackend { public List<Entry> searchUsers(String filter, SchemaManager schemaManager) throws Exception { List<Entry> results = new ArrayList<>(); - String userFilter = extractUser(filter).toLowerCase(Locale.ROOT); + final String extractedUser = extractUser(filter); + if (extractedUser == null) { + // The filter does not target a recognized user identifier (uid/cn/sAMAccountName), + // so there is nothing for this user-only backend to match. + return results; + } + String userFilter = extractedUser.toLowerCase(Locale.ROOT); // Simple filter matching - just check if username matches for (String username : users.keySet()) { diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java index 2c8be212e..300acff39 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java @@ -399,7 +399,7 @@ public class LdapProxyBackend implements LdapBackend { results.add(remoteSchemaConverter.convertRemoteEntryToProxyEntry(entry, schemaManager)); } } catch (LdapException e) { - LOG.ldapAttributeCopyError(e); + LOG.ldapSearchFailed(remoteSearchBase, remoteFilter, e); } return results; } finally { @@ -502,14 +502,19 @@ public class LdapProxyBackend implements LdapBackend { groupEntry = entryCache.get((groupDn)); } else { try { - groupEntry = connection.lookup(groupDn, "memberOf"); + // Request "cn" alongside "memberOf" so the cached entry can be resolved to a + // group name later (e.g. by getUserGroups); a memberOf-only lookup omits it. + groupEntry = connection.lookup(groupDn, "cn", "memberOf"); } catch (LdapException e) { - // assume group doesn't exist and has no parent groups + groupEntry = null; + } + if (groupEntry == null) { + // Entry not found or lookup failed — synthesise a skeleton so cn is still known. groupEntry = createSkeletonGroupEntry(groupDn); } entryCache.put(groupDn, groupEntry); } - Attribute memberOf = groupEntry.get("memberOf"); + Attribute memberOf = groupEntry == null ? null : groupEntry.get("memberOf"); if (memberOf != null) { for (Value value : memberOf) { parents.add(value.getNormalized()); @@ -683,7 +688,6 @@ public class LdapProxyBackend implements LdapBackend { return null; } - // tODO reuse this method in recursive calls private List<Entry> getUserGroupsInternal(LdapConnection connection, Dn... dns) throws LdapException, CursorException, IOException { List<Entry> groups = new ArrayList<>(); if (dns.length == 0) { @@ -736,6 +740,10 @@ public class LdapProxyBackend implements LdapBackend { Attribute cnAttr = entry.get("cn"); if (cnAttr != null) { cns.add(cnAttr.getString()); + } else if (entry.getDn() != null && entry.getDn().getRdn() != null) { + // Fall back to the CN carried in the DN when the entry was fetched without the + // cn attribute, so resolved groups are not silently dropped from the result. + cns.add(entry.getDn().getRdn().getValue()); } } return cns; diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/RemoteSchemaConverter.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/RemoteSchemaConverter.java index bd55f58ba..a4a5f07f6 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/RemoteSchemaConverter.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/RemoteSchemaConverter.java @@ -28,9 +28,24 @@ import org.apache.directory.api.ldap.model.schema.SchemaManager; import org.apache.knox.gateway.i18n.messages.MessagesFactory; import org.apache.knox.gateway.services.ldap.LdapMessages; +import java.text.ParseException; +import java.util.Locale; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + public class RemoteSchemaConverter { private static final LdapMessages LOG = MessagesFactory.get(LdapMessages.class); + // Credential-bearing attributes that must never be surfaced through the proxy entry. + private static final Set<String> SENSITIVE_ATTRIBUTES = Set.of( + "userpassword", "unicodepwd", "userpkcs12"); + + // Attributes whose values are distinguished names and therefore need remote->proxy + // DN rewriting. Other attribute values (mail, description, ...) are copied verbatim. + private static final Set<String> DN_VALUED_ATTRIBUTES = Set.of( + "member", "uniquemember", "memberof", "manager", "owner", "seealso"); + // Proxy configuration private final String proxyBaseDn; // Base DN for proxy entries (e.g., dc=proxy,dc=com) private final String proxyUserSearchBase; @@ -80,8 +95,12 @@ public class RemoteSchemaConverter { Entry entry = new DefaultEntry(schemaManager); entry.setDn(sourceEntry.getDn()); - // Copy all known AttributeTypes as-is from backend response + // Copy attributes from the backend response, skipping credential-bearing ones so + // they are never exposed through the proxy. for (Attribute attribute : sourceEntry.getAttributes()) { + if (SENSITIVE_ATTRIBUTES.contains(attribute.getId().toLowerCase(Locale.ROOT))) { + continue; + } copyAttribute(sourceEntry, entry, attribute.getId()); } @@ -95,13 +114,15 @@ public class RemoteSchemaConverter { // replace userObjectClass and groupObjectClass object classes Attribute objectClassAttribute = sourceEntry.get("objectclass"); - if (objectClassAttribute.contains(remoteGroupObjectClass)) { - entry.remove("objectclass", remoteGroupObjectClass); - entry.add("objectclass", "groupofnames"); - } - if (objectClassAttribute.contains(remoteUserObjectClass)) { - entry.remove("objectclass", remoteUserObjectClass); - entry.add("objectclass", "inetOrgPerson"); + if (objectClassAttribute != null) { + if (objectClassAttribute.contains(remoteGroupObjectClass)) { + entry.remove("objectclass", remoteGroupObjectClass); + entry.add("objectclass", "groupofnames"); + } + if (objectClassAttribute.contains(remoteUserObjectClass)) { + entry.remove("objectclass", remoteUserObjectClass); + entry.add("objectclass", "inetOrgPerson"); + } } return entry; @@ -112,9 +133,9 @@ public class RemoteSchemaConverter { * @param filter the filter * @param schemaManager the schema manager * @return the converted filter - * @throws Exception if the filter cannot be parsed + * @throws ParseException if the filter cannot be parsed */ - public String convertProxyFilterToRemoteFilter(String filter, SchemaManager schemaManager) throws Exception { + public String convertProxyFilterToRemoteFilter(String filter, SchemaManager schemaManager) throws ParseException { FilterMappingVisitor filterMappingVisitor = new FilterMappingVisitor(remoteUserIdentifierAttribute, remoteUserObjectClass, remoteGroupObjectClass, schemaManager); // Filter likely has already been annotated by other interceptors. @@ -135,9 +156,12 @@ public class RemoteSchemaConverter { public void copyAttribute(Entry source, Entry target, String attributeName) { final Attribute attribute = source.get(attributeName); if (attribute != null) { + // Only rewrite DNs for DN-valued attributes; other values (e.g. mail, description) + // are copied verbatim so they are not corrupted if they happen to contain a base DN. + final boolean dnValued = DN_VALUED_ATTRIBUTES.contains(attributeName.toLowerCase(Locale.ROOT)); // Copy all values of the attribute (important for multi-valued attributes like objectClass) for (Value value : attribute) { - String valueString = convertRemoteDnToProxyDn(value.toString()); + String valueString = dnValued ? convertRemoteDnToProxyDn(value.toString()) : value.toString(); if (!target.contains(attributeName, valueString)) { try { target.add(attributeName, valueString); @@ -151,27 +175,44 @@ public class RemoteSchemaConverter { /** * Converts a remote dn string to a proxy dn string. This also works for search base strings. - * @param string the remote dn or search base + * @param attributeValue the remote dn or search base * @return the proxy search base */ - public String convertRemoteDnToProxyDn(String string) { - return string == null ? - null : - string.replaceAll("(?i)" + remoteGroupSearchBase, proxyGroupSearchBase) - .replaceAll("(?i)" + remoteUserSearchBase, proxyUserSearchBase) - .replaceAll("(?i)" + remoteBaseDn, proxyBaseDn); + private String convertRemoteDnToProxyDn(String attributeValue) { + if (attributeValue == null) { + return null; + } + // Replace the most specific bases first; they contain the base DN as a suffix. + String result = replaceIgnoreCase(attributeValue, remoteGroupSearchBase, proxyGroupSearchBase); + result = replaceIgnoreCase(result, remoteUserSearchBase, proxyUserSearchBase); + result = replaceIgnoreCase(result, remoteBaseDn, proxyBaseDn); + return result; } /** * Converts a proxy dn string to a remote dn string. This also works for search base strings. - * @param string the proxy dn or search base + * @param attributeValue the proxy dn or search base * @return the remote search base */ - public String convertProxyDnToRemoteDn(String string) { - return string == null ? - null : - string.replaceAll("(?i)" + proxyGroupSearchBase, remoteGroupSearchBase) - .replaceAll("(?i)" + proxyUserSearchBase, remoteUserSearchBase) - .replaceAll("(?i)" + proxyBaseDn, remoteBaseDn); + public String convertProxyDnToRemoteDn(String attributeValue) { + if (attributeValue == null) { + return null; + } + // Replace the most specific bases first; they contain the base DN as a suffix. + String result = replaceIgnoreCase(attributeValue, proxyGroupSearchBase, remoteGroupSearchBase); + result = replaceIgnoreCase(result, proxyUserSearchBase, remoteUserSearchBase); + result = replaceIgnoreCase(result, proxyBaseDn, remoteBaseDn); + return result; + } + + /** + * Case-insensitive literal replacement of all occurrences of {@code search} with + * {@code replacement}. Both operands are treated as literals (not regular expressions), + * so DNs containing regex metacharacters are handled safely. + */ + private String replaceIgnoreCase(String input, String search, String replacement) { + return Pattern.compile(Pattern.quote(search), Pattern.CASE_INSENSITIVE) + .matcher(input) + .replaceAll(Matcher.quoteReplacement(replacement)); } } diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/DisabledUserInterceptor.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/DisabledUserInterceptor.java index 2f05ad4df..c4ffc92f5 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/DisabledUserInterceptor.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/DisabledUserInterceptor.java @@ -71,8 +71,6 @@ public class DisabledUserInterceptor extends BaseInterceptor { } catch (IOException e) { // IOException would only occur after finishing iterating over results // we can ignore this exception and return the filtered entries - } catch (Exception e) { - throw e; } return new EntryFilteringCursorImpl(new ListCursor<>(filteredEntries), ctx, schemaManager); } diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/UserSearchInterceptor.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/UserSearchInterceptor.java index 014cd2bb8..1c9edb988 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/UserSearchInterceptor.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/UserSearchInterceptor.java @@ -39,6 +39,8 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import static java.util.Locale.ROOT; + /** * Interceptor for LDAP operations to proxy user searches to backends when not found locally */ @@ -95,16 +97,29 @@ public class UserSearchInterceptor extends BaseInterceptor { } catch (Exception e) { // If we get an error or no results, try the backends } - try { - entries.addAll(backend.search(baseDn, ctx.getScope(), filter, schemaManager)); - } catch (Exception e) { - LOG.ldapSearchFailed(baseDn, filter, e); + + // Only forward to the backend when the search base is under the backend's namespace. + // System/operational searches (ou=schema, cn=config, root-DSE) must not be forwarded. + if (isUnderBackendBaseDn(baseDn)) { + try { + entries.addAll(backend.search(baseDn, ctx.getScope(), filter, schemaManager)); + } catch (Exception e) { + LOG.ldapSearchFailed(baseDn, filter, e); + } } // Return cursor with our results - use a simple approach return new EntryFilteringCursorImpl(new ListCursor<>(entries), ctx, schemaManager); } + private boolean isUnderBackendBaseDn(String searchBase) { + final String backendBase = backend.getBaseDn(); + if (searchBase == null || searchBase.isEmpty() || backendBase == null || backendBase.isEmpty()) { + return false; + } + return searchBase.toLowerCase(ROOT).endsWith(backendBase.toLowerCase(ROOT)); + } + @Override public void bind(BindOperationContext ctx) throws LdapException { LOG.ldapBind(ctx.getDn() != null ? ctx.getDn().toString() : "anonymous"); diff --git a/gateway-server/src/test/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackendTest.java b/gateway-server/src/test/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackendTest.java index acf8a786d..e814735f4 100644 --- a/gateway-server/src/test/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackendTest.java +++ b/gateway-server/src/test/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackendTest.java @@ -293,6 +293,33 @@ public class LdapProxyBackendTest { assertTrue(userGroups.isEmpty()); } + @Test + public void testGetUserGroupsUseMemberOfRecursive() throws Exception { + Map<String, String> config = createRecursiveConfigForMemberOf(10); + config.put("useMemberOf", "true"); + ldapProxyBackend = new LdapProxyBackend("testbackend", config); + + List<String> userGroups = ldapProxyBackend.getUserGroups("memberOfUser", schemaManager); + assertTrue(userGroups.contains("memberOflevel1")); + assertTrue(userGroups.contains("memberOflevel2")); + assertTrue(userGroups.contains("memberOflevel3")); + assertTrue(userGroups.contains("memberOflevel4")); + assertTrue(userGroups.contains("memberOfCycleA")); + assertTrue(userGroups.contains("memberOfCycleB")); + } + + @Test + public void testGetUserGroupsUseMemberOfRecursiveDepth2() throws Exception { + Map<String, String> config = createRecursiveConfigForMemberOf(2); + config.put("useMemberOf", "true"); + ldapProxyBackend = new LdapProxyBackend("testbackend", config); + + List<String> userGroups = ldapProxyBackend.getUserGroups("memberOfUser", schemaManager); + assertTrue(userGroups.contains("memberOflevel1")); + assertTrue(userGroups.contains("memberOflevel2")); + assertFalse("Level 3 should not appear at max depth 2", userGroups.contains("memberOflevel3")); + } + @Test public void testSearchUsers() throws Exception { ldapProxyBackend = new LdapProxyBackend("testbackend", ldapBackendConfig); diff --git a/gateway-server/src/test/resources/ldap-recursive-test.ldif b/gateway-server/src/test/resources/ldap-recursive-test.ldif index da8732759..c1ea55fd2 100644 --- a/gateway-server/src/test/resources/ldap-recursive-test.ldif +++ b/gateway-server/src/test/resources/ldap-recursive-test.ldif @@ -143,10 +143,10 @@ member: uid=memberOfUser2,ou=recursiveMemberOfPeople,dc=hadoop,dc=apache,dc=org dn: cn=memberOflevel2,ou=recursiveMemberOfGroups,dc=hadoop,dc=apache,dc=org objectclass:top objectclass: groupofnames -cn: memberOflevel2level2 +cn: memberOflevel2 memberOf: cn=memberOflevel3,ou=recursiveMemberOfGroups,dc=hadoop,dc=apache,dc=org memberOf: cn=memberOfCycleA,ou=recursiveMemberOfGroups,dc=hadoop,dc=apache,dc=org -member: cn=memberOflevel1,ou=recursiveGroups,dc=hadoop,dc=apache,dc=org +member: cn=memberOflevel1,ou=recursiveMemberOfGroups,dc=hadoop,dc=apache,dc=org # memberOf Level 3 Group (Member is Level 2) dn: cn=memberOflevel3,ou=recursiveMemberOfGroups,dc=hadoop,dc=apache,dc=org diff --git a/knox-site/docs/service_ldap_server.md b/knox-site/docs/service_ldap_server.md index 4dedf9468..63495195c 100644 --- a/knox-site/docs/service_ldap_server.md +++ b/knox-site/docs/service_ldap_server.md @@ -108,7 +108,7 @@ remains available with its default credentials. | Property | Default Value | Description | | :--- | :--- | :--- | -| `gateway.ldap.interceptor.<name>.interceptorType` | N/A | The type of interceptor to use (`backend` or `duplicateuserfilter`). | +| `gateway.ldap.interceptor.<name>.interceptorType` | N/A | The type of interceptor to use (`backend`, `duplicateuserfilter`, `disableduserfilter`, or `rolesLookup`). | #### User Search Interceptor (`backend`) @@ -198,7 +198,6 @@ The proxy backend delegates lookups to a remote LDAP or Active Directory server. | `gateway.ldap.interceptor.<name>.groupMemberAttribute` | `memberUid` | Attribute used for group membership (e.g., `member` for AD). | | `gateway.ldap.interceptor.<name>.useMemberOf` | `false` | If `true`, use the `memberOf` attribute for efficient group lookups. | | `gateway.ldap.interceptor.<name>.proxy.poolMaxActive` | `8` | Maximum number of active connections in the pool. | -| `gateway.ldap.interceptor.<name>.proxy.poolMaxActive` | `8` | Maximum number of active connections in the pool. | ## Active Directory (AD) Integration
