[
https://issues.apache.org/jira/browse/KNOX-3487?focusedWorklogId=1047847&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1047847
]
ASF GitHub Bot logged work on KNOX-3487:
----------------------------------------
Author: ASF GitHub Bot
Created on: 16/Oct/26 13:41
Start Date: 16/Oct/26 13:41
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #1454:
URL: https://github.com/apache/knox/pull/1454#discussion_r4294989077
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -80,6 +83,8 @@ public class LdapProxyBackend implements LdapBackend {
private static final String[] ALL_ATTRIBUTES = new String[]{"*"};
private static final String[] ALL_ATTRIBUTES_AND_MEMBEROF = new
String[]{"*", "memberOf"};
+ private static final String DEFAULT_MAX_FILTER_LENGTH = "2000";
Review Comment:
- `buildMultipleGroupMemberFilters` also splits the existing
group-membership resolution, not only the new memberOf expansion. At roughly
150 characters per POSIX clause, every `useMemberOf=false` deployment makes one
extra round trip for about every 13 users.
- Real server limits are far higher (OpenLDAP 256KB anonymous / 4MB
authenticated, AD 10MB).
I'd default to `0` (unlimited) or to something much larger.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/LDAPRolesLookupInterceptor.java:
##########
@@ -86,6 +96,90 @@ public EntryFilteringCursor search(SearchOperationContext
ctx) throws LdapExcept
}
}
+ if (isAllGroupsSearch(ctx)) {
+ return handleAllGroupsSearch(ctx);
+ } else {
+ return handleSearch(ctx);
+ }
+ }
+
+ private boolean isAllGroupsSearch(SearchOperationContext ctx) {
+ Dn searchDn = ctx.getDn();
+ if (searchDn == null) {
+ return false;
+ }
+
+ if (ctx.getScope() == SearchScope.OBJECT ||
!isAllGroupsFilter(ctx.getFilter())) {
+ return false;
+ }
+ // Either the proxy groups base, or the root with an explicit group
filter
+ return searchDn.toString().equalsIgnoreCase(proxyGroupSearchBase) ||
searchDn.size() <= 2;
Review Comment:
The comment's intent ("the proxy groups base, or the root with an explicit
group filter") makes sense, but the code doesn't enforce either half:
- `searchDn.size() <= 2` only means "root" when the base DN has exactly two
RDNs. With `gateway.ldap.base.dn=o=acme`, `ou=users,o=acme` also matches, so a
user search with `(cn=*)` returns roles. With `dc=acme,dc=co,dc=uk`, the root
never matches. Please compare normalized `Dn`s against the configured base DN
and `ou=groups,<baseDn>`, which also removes the spacing problem with
`toString().equalsIgnoreCase`.
- `isAllGroupsFilter` accepts `(cn=*)` and `(objectClass=*)`, which aren't
explicit group filters. At the root, the default `ldapsearch -b
dc=proxy,dc=com` uses `(objectClass=*)` and now returns only roles, with no
users. Please accept those two filters only when the base is
`ou=groups,<baseDn>`, and require `objectClass=groupOfNames|groupOfUniqueNames`
at the root.
- Minor: a ONELEVEL search at the root returns roles, which sit one level
lower, under `ou=groups`. With `objectClass=*` accepted, this breaks tree
browsing in LDAP browsers.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/LDAPRolesLookupInterceptor.java:
##########
@@ -86,6 +96,90 @@ public EntryFilteringCursor search(SearchOperationContext
ctx) throws LdapExcept
}
}
+ if (isAllGroupsSearch(ctx)) {
+ return handleAllGroupsSearch(ctx);
+ } else {
+ return handleSearch(ctx);
+ }
+ }
+
+ private boolean isAllGroupsSearch(SearchOperationContext ctx) {
+ Dn searchDn = ctx.getDn();
+ if (searchDn == null) {
+ return false;
+ }
+
+ if (ctx.getScope() == SearchScope.OBJECT ||
!isAllGroupsFilter(ctx.getFilter())) {
+ return false;
+ }
+ // Either the proxy groups base, or the root with an explicit group
filter
+ return searchDn.toString().equalsIgnoreCase(proxyGroupSearchBase) ||
searchDn.size() <= 2;
+ }
+
+ private boolean isAllGroupsFilter(ExprNode filter) {
+ // Case 1: Simple objectClass evaluation (e.g.,
objectClass=groupOfNames)
+ if (filter instanceof EqualityNode) {
+ EqualityNode<?> eqNode = (EqualityNode<?>) filter;
+ if ("objectClass".equalsIgnoreCase(eqNode.getAttribute())) {
+ String value = eqNode.getValue().toString();
+ return "groupOfNames".equalsIgnoreCase(value) ||
+ "groupOfUniqueNames".equalsIgnoreCase(value);
+ }
+ }
+
+ // Case 2a: Wildcard match on all common names and object classes
(e.g., cn=*, objectClass=*)
+ if (filter instanceof PresenceNode) {
+ PresenceNode presNode = (PresenceNode) filter;
+ return "cn".equalsIgnoreCase(presNode.getAttribute()) ||
+ "objectClass".equalsIgnoreCase(presNode.getAttribute());
+ }
+ // Case 2b: Wildcard match on object class (e.g., objectClass=*)
+ if (filter instanceof ObjectClassNode) {
+ return true;
+ }
+
+ // Case 3: Compound filters. An AND is an all groups filter only if
every child is, an OR
+ // if any child is, e.g. (&(objectClass=groupOfNames)(cn=*)) or
(|(uid=alice)(cn=*)).
+ // A NOT is never an all groups filter: negating a condition never
positively selects groups.
+ if (filter instanceof AndNode) {
+ List<ExprNode> children = ((AndNode) filter).getChildren();
+ return !children.isEmpty() &&
children.stream().allMatch(this::isAllGroupsFilter);
+ }
+ if (filter instanceof OrNode) {
+ return ((OrNode)
filter).getChildren().stream().anyMatch(this::isAllGroupsFilter);
Review Comment:
With "any child matches", `(|(uid=alice)(cn=*))` is treated as all-groups
and `alice` is dropped. An OR filter only selects all groups if every child
does, so this should be `allMatch`.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/roles/rest/RestApiLdapRolesReverseLookup.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.
+ */
+package org.apache.knox.gateway.services.ldap.roles.rest;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.http.HttpEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.utils.URIBuilder;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.services.ldap.LdapMessages;
+import org.apache.knox.gateway.services.ldap.RoleAssignees;
+import org.apache.knox.gateway.services.ldap.RoleAssignment;
+import org.apache.knox.gateway.services.ldap.roles.LdapRolesReverseLookup;
+import org.apache.knox.gateway.services.ldap.roles.RoleException;
+
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * REST API based implementation of LdapRolesReverseLookup.
+ */
+public class RestApiLdapRolesReverseLookup implements LdapRolesReverseLookup {
+ private static final LdapMessages LOG =
MessagesFactory.get(LdapMessages.class);
+
+ private final ObjectMapper mapper = new ObjectMapper();
+ private final String listEndpoint;
+ private final String getEndpoint;
+
+ public RestApiLdapRolesReverseLookup(String listEndpoint, String
getEndpoint) {
+ this.listEndpoint = listEndpoint;
+ this.getEndpoint = getEndpoint;
+ }
+
+ @Override
+ public Collection<String> listRoles() throws RoleException {
+ final List<Role> roles = listRoles(null);
+ Set<String> result = new HashSet<>();
+ for (Role role : roles) {
+ result.add(new RoleAssignment(role.getScope(),
role.getName()).getDisplayValue());
+ }
+ return result;
+ }
+
+ @Override
+ public RoleAssignees lookupRoleAssignees(String role) throws RoleException
{
+ if (role != null) {
+ RoleAssignment roleAssignment =
RoleAssignment.createFromDisplayValue(role);
+ List<Role> rolesList = listRoles(roleAssignment);
+ String roleId = findRoleId(roleAssignment, rolesList);
+ if (roleId != null) {
+ List<String> users = new ArrayList<>();
+ List<String> groups = new ArrayList<>();
+ List<RoleMember> members = getMembers(roleId);
+ if (members != null) {
+ for (RoleMember member : members) {
+ if (member.getMemberType().equalsIgnoreCase("user")) {
+ users.add(member.getMemberId());
+ } else if
(member.getMemberType().equalsIgnoreCase("group")) {
+ groups.add(member.getMemberId());
+ } else {
+ // ignore unknown member types
+ LOG.restRolesAssigneeTypeUnknown(role,
member.getMemberType());
+ }
+ }
+ }
+ return new RoleAssignees(users, groups);
+ }
+ }
+ return new RoleAssignees(List.of(), List.of());
+ }
+
+ private List<RoleMember> getMembers(String roleId) throws RoleException {
+ String encodedRoleId = URLEncoder.encode(roleId,
StandardCharsets.UTF_8)
+ .replace("+", "%20"); // URLEncoder converts spaces to '+',
path params need '%20'
+ String getUrl = getEndpoint + "/" + encodedRoleId;
+
+ try (CloseableHttpClient httpClient = HttpClients.createDefault()) {
+ LOG.restRolesGetRequest(getUrl, roleId);
+ HttpGet getRequest = new HttpGet(getUrl);
+ getRequest.setHeader("Accept", "application/json");
+ try (CloseableHttpResponse response =
httpClient.execute(getRequest)) {
+ int statusCode = response.getStatusLine().getStatusCode();
+ HttpEntity entity = response.getEntity();
+ final String jsonResponse = entity == null ? null :
EntityUtils.toString(entity);
+ LOG.restRolesGetResponse(getUrl, statusCode, jsonResponse);
+
+ if (statusCode != 200) {
+ LOG.restRolesLookupHttpError(getUrl, statusCode,
jsonResponse);
Review Comment:
The new `restRolesGetHttpError` and `restRolesListHttpError` messages aren't
used, so get and list errors are logged as lookup errors
(`restRolesLookupHttpError`).
The same applies at line 162 (`listRoles`).
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -576,6 +586,54 @@ public List<Entry> search(String searchBase, SearchScope
searchScope, String fil
}
}
+ private String mapMemberOfExpression(String filter, SchemaManager
schemaManager) throws Exception {
+ if (useMemberOf || !filter.toLowerCase(ROOT).contains("memberof")) {
+ return filter;
+ }
+ // Convert filter if it contains "memberOf" but the backend doesn't
support "memberOf"
+ MemberOfMappingVisitor visitor = new
MemberOfMappingVisitor(schemaManager, this::lookupGroup, this::isUser);
+ ExprNode node = FilterParser.parse(schemaManager, filter);
+ // Baseline in the same form as the visitor output so an unchanged
visit exits on the first pass
+ String current = node.toString();
+ for (int i = 0; i < recursiveGroupResolutionMaxDepth; ++i) {
Review Comment:
This loop always runs up to `recursiveGroupResolutionMaxDepth` (default 3),
even when `recursiveGroupResolution` is off, which is the default. Please do a
single pass when recursion is disabled.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/LDAPRolesLookupInterceptor.java:
##########
@@ -86,6 +96,90 @@ public EntryFilteringCursor search(SearchOperationContext
ctx) throws LdapExcept
}
}
+ if (isAllGroupsSearch(ctx)) {
+ return handleAllGroupsSearch(ctx);
+ } else {
+ return handleSearch(ctx);
+ }
+ }
+
+ private boolean isAllGroupsSearch(SearchOperationContext ctx) {
+ Dn searchDn = ctx.getDn();
+ if (searchDn == null) {
+ return false;
+ }
+
+ if (ctx.getScope() == SearchScope.OBJECT ||
!isAllGroupsFilter(ctx.getFilter())) {
+ return false;
+ }
+ // Either the proxy groups base, or the root with an explicit group
filter
+ return searchDn.toString().equalsIgnoreCase(proxyGroupSearchBase) ||
searchDn.size() <= 2;
+ }
+
+ private boolean isAllGroupsFilter(ExprNode filter) {
+ // Case 1: Simple objectClass evaluation (e.g.,
objectClass=groupOfNames)
+ if (filter instanceof EqualityNode) {
+ EqualityNode<?> eqNode = (EqualityNode<?>) filter;
+ if ("objectClass".equalsIgnoreCase(eqNode.getAttribute())) {
+ String value = eqNode.getValue().toString();
+ return "groupOfNames".equalsIgnoreCase(value) ||
+ "groupOfUniqueNames".equalsIgnoreCase(value);
+ }
+ }
+
+ // Case 2a: Wildcard match on all common names and object classes
(e.g., cn=*, objectClass=*)
+ if (filter instanceof PresenceNode) {
+ PresenceNode presNode = (PresenceNode) filter;
+ return "cn".equalsIgnoreCase(presNode.getAttribute()) ||
+ "objectClass".equalsIgnoreCase(presNode.getAttribute());
+ }
+ // Case 2b: Wildcard match on object class (e.g., objectClass=*)
+ if (filter instanceof ObjectClassNode) {
+ return true;
+ }
+
+ // Case 3: Compound filters. An AND is an all groups filter only if
every child is, an OR
+ // if any child is, e.g. (&(objectClass=groupOfNames)(cn=*)) or
(|(uid=alice)(cn=*)).
+ // A NOT is never an all groups filter: negating a condition never
positively selects groups.
+ if (filter instanceof AndNode) {
+ List<ExprNode> children = ((AndNode) filter).getChildren();
+ return !children.isEmpty() &&
children.stream().allMatch(this::isAllGroupsFilter);
+ }
+ if (filter instanceof OrNode) {
+ return ((OrNode)
filter).getChildren().stream().anyMatch(this::isAllGroupsFilter);
+ }
+
+ return false;
+ }
+
+ private EntryFilteringCursor handleAllGroupsSearch(SearchOperationContext
ctx) throws LdapException {
+ // short-circuit the LDAP search to return all roles
+ Collection<String> roles = null;
+ try {
+ roles = rolesLookupService.listRoles();
+ } catch (Exception e) {
+ throw new LdapException("Failed to retrieve roles from roles
lookup service.", e);
+ }
+ List<Entry> entries = new ArrayList<>();
+ for (String role : roles) {
+ Dn roleDn = new Dn(schemaManager, createGroupDnFromName(role));
+ Entry entry = createRoleEntry(roleDn);
+ entries.add(entry);
+ }
+
+ return new EntryFilteringCursorImpl(new ListCursor<>(entries), ctx,
schemaManager);
+ }
+
+ private EntryFilteringCursor handleSearch(SearchOperationContext ctx)
throws LdapException {
+ // Replace the role dn with the member dns if the search filter
contains a "memberOf" node.
+ ExprNode filter = ctx.getFilter();
+ ReverseRoleLookupMappingVisitor reverseRoleLookupMappingVisitor =
+ new ReverseRoleLookupMappingVisitor(schemaManager,
rolesLookupService, this::createGroupDnFromName);
+ Object updatedFilter = filter.accept(reverseRoleLookupMappingVisitor);
Review Comment:
If the roles service fails, a DN can't be parsed, or
`RoleAssignment.createFromDisplayValue` throws on `a:b:c`, the visitor throws
an unchecked exception (`RuntimeException` from
`ReverseRoleLookupMappingVisitor.java:64`, or `IllegalArgumentException`). This
call isn't inside a try/catch, so the exception escapes, the client doesn't get
an `LdapException`, and `ldapRolesLookupFailed` is never logged.
Please catch the exception here and convert it to an `LdapException`. The
same applies to `MemberOfMappingVisitor.java:66` as called from
`LdapProxyBackend.mapMemberOfExpression`.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -576,6 +586,54 @@ public List<Entry> search(String searchBase, SearchScope
searchScope, String fil
}
}
+ private String mapMemberOfExpression(String filter, SchemaManager
schemaManager) throws Exception {
+ if (useMemberOf || !filter.toLowerCase(ROOT).contains("memberof")) {
+ return filter;
+ }
+ // Convert filter if it contains "memberOf" but the backend doesn't
support "memberOf"
+ MemberOfMappingVisitor visitor = new
MemberOfMappingVisitor(schemaManager, this::lookupGroup, this::isUser);
+ ExprNode node = FilterParser.parse(schemaManager, filter);
Review Comment:
This parses the raw filter before `convertProxyFilterToRemoteFilter`
(`RemoteSchemaConverter.java:151`) removes the `:[...]` annotations, and that
method's comment says those annotations occur in practice. Please apply the
same cleanup first, ideally through a shared helper. It's worth testing with a
real annotated filter.
##########
gateway-spi/src/main/java/org/apache/knox/gateway/services/ldap/RoleAssignment.java:
##########
@@ -20,6 +20,18 @@
import com.fasterxml.jackson.annotation.JsonProperty;
public class RoleAssignment {
+ public static RoleAssignment createFromDisplayValue(String displayValue) {
+ String[] tokens = displayValue.split(":");
Review Comment:
Please use `split(":", 2)` so a role name that contains `:` (for example
`platform:team:admin`) is parsed as scope `platform` and name `team:admin`.
Right now it throws `IllegalArgumentException` at line 31, and with the
unhandled-exception issue in `handleSearch` that error escapes the LDAP search
as an unchecked exception.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/roles/rest/RestApiLdapRolesReverseLookup.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.
+ */
+package org.apache.knox.gateway.services.ldap.roles.rest;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.http.HttpEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.utils.URIBuilder;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.services.ldap.LdapMessages;
+import org.apache.knox.gateway.services.ldap.RoleAssignees;
+import org.apache.knox.gateway.services.ldap.RoleAssignment;
+import org.apache.knox.gateway.services.ldap.roles.LdapRolesReverseLookup;
+import org.apache.knox.gateway.services.ldap.roles.RoleException;
+
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * REST API based implementation of LdapRolesReverseLookup.
+ */
+public class RestApiLdapRolesReverseLookup implements LdapRolesReverseLookup {
+ private static final LdapMessages LOG =
MessagesFactory.get(LdapMessages.class);
+
+ private final ObjectMapper mapper = new ObjectMapper();
+ private final String listEndpoint;
+ private final String getEndpoint;
+
+ public RestApiLdapRolesReverseLookup(String listEndpoint, String
getEndpoint) {
+ this.listEndpoint = listEndpoint;
+ this.getEndpoint = getEndpoint;
+ }
+
+ @Override
+ public Collection<String> listRoles() throws RoleException {
+ final List<Role> roles = listRoles(null);
+ Set<String> result = new HashSet<>();
+ for (Role role : roles) {
+ result.add(new RoleAssignment(role.getScope(),
role.getName()).getDisplayValue());
+ }
+ return result;
+ }
+
+ @Override
+ public RoleAssignees lookupRoleAssignees(String role) throws RoleException
{
+ if (role != null) {
+ RoleAssignment roleAssignment =
RoleAssignment.createFromDisplayValue(role);
+ List<Role> rolesList = listRoles(roleAssignment);
+ String roleId = findRoleId(roleAssignment, rolesList);
+ if (roleId != null) {
+ List<String> users = new ArrayList<>();
+ List<String> groups = new ArrayList<>();
+ List<RoleMember> members = getMembers(roleId);
+ if (members != null) {
+ for (RoleMember member : members) {
+ if (member.getMemberType().equalsIgnoreCase("user")) {
+ users.add(member.getMemberId());
+ } else if
(member.getMemberType().equalsIgnoreCase("group")) {
+ groups.add(member.getMemberId());
+ } else {
+ // ignore unknown member types
+ LOG.restRolesAssigneeTypeUnknown(role,
member.getMemberType());
+ }
+ }
+ }
+ return new RoleAssignees(users, groups);
+ }
+ }
+ return new RoleAssignees(List.of(), List.of());
+ }
+
+ private List<RoleMember> getMembers(String roleId) throws RoleException {
+ String encodedRoleId = URLEncoder.encode(roleId,
StandardCharsets.UTF_8)
+ .replace("+", "%20"); // URLEncoder converts spaces to '+',
path params need '%20'
+ String getUrl = getEndpoint + "/" + encodedRoleId;
+
+ try (CloseableHttpClient httpClient = HttpClients.createDefault()) {
+ LOG.restRolesGetRequest(getUrl, roleId);
+ HttpGet getRequest = new HttpGet(getUrl);
+ getRequest.setHeader("Accept", "application/json");
+ try (CloseableHttpResponse response =
httpClient.execute(getRequest)) {
+ int statusCode = response.getStatusLine().getStatusCode();
+ HttpEntity entity = response.getEntity();
+ final String jsonResponse = entity == null ? null :
EntityUtils.toString(entity);
+ LOG.restRolesGetResponse(getUrl, statusCode, jsonResponse);
+
+ if (statusCode != 200) {
+ LOG.restRolesLookupHttpError(getUrl, statusCode,
jsonResponse);
+ throw new RoleException("Failed to get roles: HTTP " +
statusCode);
+ }
+
+ if (jsonResponse == null) {
+ throw new RoleException("Empty response from role get
API");
+ }
+
+ return mapper.readValue(jsonResponse, Role.class).getMembers();
+ }
+ } catch (IOException e) {
+ throw new RoleException("Error while executing role get", e);
+ }
+ }
+
+ private List<Role> listRoles(RoleAssignment roleAssignment) throws
RoleException {
+ String listUrl = null;
+ int numRolesRetrieved = 0;
+ int totalExpectedRoles = 0;
+ List<Role> roles = new ArrayList<>();
+
+ try (CloseableHttpClient httpClient = HttpClients.createDefault()) {
+ do {
+ URIBuilder uriBuilder = new URIBuilder(listEndpoint);
+ if (roleAssignment != null) {
+ if (roleAssignment.getName() != null) {
+ uriBuilder.addParameter("name",
roleAssignment.getName());
+ }
+ if (roleAssignment.getScope() != null) {
+ uriBuilder.addParameter("scope",
roleAssignment.getScope());
+ }
+ }
+ uriBuilder.addParameter("offset",
Integer.toString(numRolesRetrieved));
+ URI listURI = uriBuilder.build();
+ listUrl = listURI.toString();
+ LOG.restRolesListRequest(listUrl);
+
+ HttpGet listRequest = new HttpGet(listUrl);
+ listRequest.setHeader("Accept", "application/json");
+ try (CloseableHttpResponse response =
httpClient.execute(listRequest)) {
+ int statusCode = response.getStatusLine().getStatusCode();
+ HttpEntity entity = response.getEntity();
+ final String jsonResponse = entity == null ? null :
EntityUtils.toString(entity);
+ LOG.restRolesListResponse(listUrl, statusCode,
jsonResponse);
+
+ if (statusCode != 200) {
+ LOG.restRolesLookupHttpError(listUrl, statusCode,
jsonResponse);
+ throw new RoleException("Failed to list roles: HTTP "
+ statusCode);
+ }
+
+ if (jsonResponse == null) {
+ throw new RoleException("Empty response from role list
API");
+ }
+
+ ListRolesResponse listRolesResponse =
mapper.readValue(jsonResponse, ListRolesResponse.class);
+ numRolesRetrieved += listRolesResponse.getData().size();
Review Comment:
These throw an NPE when the response has no `data`, `pagination` or
`member_type` (`getData().size()`, `getPagination().getTotal()`,
`member.getMemberType().equalsIgnoreCase(...)`). Please null-check them and
raise a `RoleException` with a clear message.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -861,13 +919,49 @@ private List<Entry> getUserGroupsInternal(LdapConnection
connection, Dn... dns)
return groups;
}
- String filter = buildMultipleGroupMemberFilter(dns);
-
- groups.addAll(performPagedSearch(connection, remoteGroupSearchBase,
filter, SearchScope.SUBTREE, "cn"));
+ groups.addAll(performSplitSearch(connection, remoteGroupSearchBase,
buildMultipleGroupMemberFilters(dns), SearchScope.SUBTREE, "cn"));
return groups;
}
+ /**
+ * Splits a filter that exceeds maxFilterLength into several filters whose
combined results are
+ * equivalent to the original filter. A filter that can't be split is
returned as is.
+ */
+ private List<String> splitFilter(String filter) throws LdapException {
+ if (maxFilterLength <= 0 || filter.length() <= maxFilterLength) {
+ return List.of(filter);
+ }
+ try {
+ // Remote attributes may be unknown to the local schema so parse
without a schema manager
+ List<ExprNode> pieces =
FilterSplitter.split(FilterParser.parse(filter), maxFilterLength);
+ LOG.ldapFilterSplit(filter.length(), maxFilterLength,
pieces.size());
+ return
pieces.stream().map(ExprNode::toString).collect(Collectors.toList());
+ } catch (java.text.ParseException e) {
+ throw new LdapException("Unable to split oversized filter: " +
e.getMessage(), e);
+ }
+ }
+
+ /**
+ * Executes each filter as a separate paged search and combines the
results, removing entries
+ * returned by more than one query. Stops early once maxResultSetSize
entries have been collected.
+ */
+ protected List<Entry> performSplitSearch(LdapConnection connection, String
baseDn, List<String> filters, SearchScope scope, String... attributes) throws
LdapException, CursorException, IOException {
+ if (filters.size() == 1) {
+ return performPagedSearch(connection, baseDn, filters.get(0),
scope, attributes);
+ }
+ Map<String, Entry> combined = new LinkedHashMap<>();
+ for (String filter : filters) {
+ for (Entry entry : performPagedSearch(connection, baseDn, filter,
scope, attributes)) {
+ combined.putIfAbsent(entry.getDn().getNormName(), entry);
+ }
+ if (maxResultSetSize != 0 && combined.size() >= maxResultSetSize) {
+ break;
+ }
+ }
+ return new ArrayList<>(combined.values());
Review Comment:
The `maxResultSetSize` check runs only after each filter piece finishes, so
the combined result can go past the limit. Please truncate it to
`maxResultSetSize`.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/roles/rest/RestApiLdapRolesReverseLookup.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.
+ */
+package org.apache.knox.gateway.services.ldap.roles.rest;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.http.HttpEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.utils.URIBuilder;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.services.ldap.LdapMessages;
+import org.apache.knox.gateway.services.ldap.RoleAssignees;
+import org.apache.knox.gateway.services.ldap.RoleAssignment;
+import org.apache.knox.gateway.services.ldap.roles.LdapRolesReverseLookup;
+import org.apache.knox.gateway.services.ldap.roles.RoleException;
+
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * REST API based implementation of LdapRolesReverseLookup.
+ */
+public class RestApiLdapRolesReverseLookup implements LdapRolesReverseLookup {
+ private static final LdapMessages LOG =
MessagesFactory.get(LdapMessages.class);
+
+ private final ObjectMapper mapper = new ObjectMapper();
+ private final String listEndpoint;
+ private final String getEndpoint;
+
+ public RestApiLdapRolesReverseLookup(String listEndpoint, String
getEndpoint) {
+ this.listEndpoint = listEndpoint;
+ this.getEndpoint = getEndpoint;
+ }
+
+ @Override
+ public Collection<String> listRoles() throws RoleException {
+ final List<Role> roles = listRoles(null);
+ Set<String> result = new HashSet<>();
+ for (Role role : roles) {
+ result.add(new RoleAssignment(role.getScope(),
role.getName()).getDisplayValue());
+ }
+ return result;
+ }
+
+ @Override
+ public RoleAssignees lookupRoleAssignees(String role) throws RoleException
{
+ if (role != null) {
+ RoleAssignment roleAssignment =
RoleAssignment.createFromDisplayValue(role);
+ List<Role> rolesList = listRoles(roleAssignment);
+ String roleId = findRoleId(roleAssignment, rolesList);
+ if (roleId != null) {
+ List<String> users = new ArrayList<>();
+ List<String> groups = new ArrayList<>();
+ List<RoleMember> members = getMembers(roleId);
+ if (members != null) {
+ for (RoleMember member : members) {
+ if (member.getMemberType().equalsIgnoreCase("user")) {
+ users.add(member.getMemberId());
+ } else if
(member.getMemberType().equalsIgnoreCase("group")) {
+ groups.add(member.getMemberId());
+ } else {
+ // ignore unknown member types
+ LOG.restRolesAssigneeTypeUnknown(role,
member.getMemberType());
+ }
+ }
+ }
+ return new RoleAssignees(users, groups);
+ }
+ }
+ return new RoleAssignees(List.of(), List.of());
+ }
+
+ private List<RoleMember> getMembers(String roleId) throws RoleException {
+ String encodedRoleId = URLEncoder.encode(roleId,
StandardCharsets.UTF_8)
+ .replace("+", "%20"); // URLEncoder converts spaces to '+',
path params need '%20'
+ String getUrl = getEndpoint + "/" + encodedRoleId;
Review Comment:
If the endpoint is configured with a trailing slash
(`http://host/auth/roles/`), this builds `//<id>`. Please strip trailing
slashes from the endpoint, or build the URL with `URIBuilder`.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -576,6 +586,54 @@ public List<Entry> search(String searchBase, SearchScope
searchScope, String fil
}
}
+ private String mapMemberOfExpression(String filter, SchemaManager
schemaManager) throws Exception {
+ if (useMemberOf || !filter.toLowerCase(ROOT).contains("memberof")) {
+ return filter;
+ }
+ // Convert filter if it contains "memberOf" but the backend doesn't
support "memberOf"
+ MemberOfMappingVisitor visitor = new
MemberOfMappingVisitor(schemaManager, this::lookupGroup, this::isUser);
+ ExprNode node = FilterParser.parse(schemaManager, filter);
+ // Baseline in the same form as the visitor output so an unchanged
visit exits on the first pass
+ String current = node.toString();
+ for (int i = 0; i < recursiveGroupResolutionMaxDepth; ++i) {
+ Object visitedNode = node.accept(visitor);
+ if (!(visitedNode instanceof ExprNode)) {
+ throw new Exception("Unable to transform memberOf expressions
in filter: " + filter);
+ }
+ node = (ExprNode) visitedNode;
+ String updated = node.toString();
+ if (updated.equals(current)) {
+ break;
+ }
+ current = updated;
+ }
+ return current;
+ }
+
+ private Entry lookupGroup(String dn, SchemaManager schemaManager) throws
Exception {
+ String remoteDn = remoteSchemaConverter.convertProxyDnToRemoteDn(dn);
+ LdapConnection connection = null;
+ try {
+ connection = getConnection();
+ Entry entry = connection.lookup(remoteDn);
+ if (entry != null) {
+ return
remoteSchemaConverter.convertRemoteEntryToProxyEntry(entry, schemaManager);
+ } else {
+ return null;
+ }
+ } finally {
+ releaseConnection(connection);
+ }
+ }
+
+ private boolean isUser(String userDn) {
+ if (userDn == null) {
+ return false;
+ }
+ String lowerCaseUserDn = userDn.toLowerCase(ROOT);
+ return lowerCaseUserDn.contains(proxyUserSearchBase) ||
lowerCaseUserDn.contains(remoteUserObjectClass);
Review Comment:
This check is unreliable:
- `contains(remoteUserObjectClass)`: an objectClass is never part of a DN.
The default `inetOrgPerson` is mixed-case and is compared against a lowercased
string, so it can never match. For AD, `user` matches substrings such as
`cn=users` or `cn=superusers`.
- `proxyUserSearchBase` isn't lowercased, so a base DN configured as
`DC=proxy,DC=com` never matches.
- With `dnMappingEnabled=false`, member DNs stay remote DNs, so they never
contain the proxy base. Every user is then treated as a group, and the filter
silently returns nothing.
Please use `Dn.isDescendantOf` against the user base that matches the
DN-mapping mode, or check the member entry's objectClass. The current tests
don't catch this because `LdapProxyBackendTest` sets `baseDn == remoteBaseDn`
and the visitor test stubs `isUser`.
Issue Time Tracking
-------------------
Worklog Id: (was: 1047847)
Time Spent: 50m (was: 40m)
> LDAP Proxy find all members of a role
> -------------------------------------
>
> Key: KNOX-3487
> URL: https://issues.apache.org/jira/browse/KNOX-3487
> Project: Apache Knox
> Issue Type: Improvement
> Components: Server
> Affects Versions: 3.1.0
> Reporter: David Han
> Assignee: David Han
> Priority: Major
> Fix For: 3.1.0
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> When role lookup is enabled on the LDAP Proxy, the LDAP Proxy should be able
> to return all users that are assigned a role. E.g.,
> {code}
> ldapsearch -b 'ou=users,DC=proxy,DC=com' -s sub
> '(memberOf=cn=rolename,ou=groups,dc=proxy,dc=com)' '*'
> {code}
> This would require calling the roles lookup service to get the mapped users
> and groups for the given role. Then modifying the search filter to replace
> the rolename with the users and groups.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
