[ 
https://issues.apache.org/jira/browse/KNOX-3048?focusedWorklogId=972019&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-972019
 ]

ASF GitHub Bot logged work on KNOX-3048:
----------------------------------------

                Author: ASF GitHub Bot
            Created on: 05/Jun/25 20:02
            Start Date: 05/Jun/25 20:02
    Worklog Time Spent: 10m 
      Work Description: pzampino commented on code in PR #1043:
URL: https://github.com/apache/knox/pull/1043#discussion_r2127436635


##########
gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java:
##########
@@ -63,285 +64,304 @@
 import org.apache.knox.gateway.util.HttpExceptionUtils;
 
 public class CommonIdentityAssertionFilter extends 
AbstractIdentityAssertionFilter {
-  private static final IdentityAsserterMessages LOG = 
MessagesFactory.get(IdentityAsserterMessages.class);
-
-  public static final String VIRTUAL_GROUP_MAPPING_PREFIX = "group.mapping.";
-  public static final String GROUP_PRINCIPAL_MAPPING = 
"group.principal.mapping";
-  public static final String PRINCIPAL_MAPPING = "principal.mapping";
-  public static final String ADVANCED_PRINCIPAL_MAPPING = 
"expression.principal.mapping";
-  private static final String PRINCIPAL_PARAM = "user.name";
-  private static final String DOAS_PRINCIPAL_PARAM = "doAs";
-  static final String IMPERSONATION_ENABLED_PARAM = 
AuthFilterUtils.PROXYUSER_PREFIX + ".impersonation.enabled";
-
-  private SimplePrincipalMapper mapper = new SimplePrincipalMapper();
-  private final Parser parser = new Parser();
-  private VirtualGroupMapper virtualGroupMapper;
-  /* List of all default and configured impersonation params */
-  protected final List<String> impersonationParamsList = new ArrayList<>();
-  protected boolean impersonationEnabled;
-  private AbstractSyntaxTree expressionPrincipalMapping;
-  private String topologyName;
-
-  @Override
-  public void init(FilterConfig filterConfig) throws ServletException {
-    topologyName = (String) 
filterConfig.getServletContext().getAttribute(GatewayServices.GATEWAY_CLUSTER_ATTRIBUTE);
-    String principalMapping = filterConfig.getInitParameter(PRINCIPAL_MAPPING);
-    if (principalMapping == null || principalMapping.isEmpty()) {
-      principalMapping = 
filterConfig.getServletContext().getInitParameter(PRINCIPAL_MAPPING);
-    }
-    String groupPrincipalMapping = 
filterConfig.getInitParameter(GROUP_PRINCIPAL_MAPPING);
-    if (groupPrincipalMapping == null || groupPrincipalMapping.isEmpty()) {
-      groupPrincipalMapping = 
filterConfig.getServletContext().getInitParameter(GROUP_PRINCIPAL_MAPPING);
+    private static final IdentityAsserterMessages LOG = 
MessagesFactory.get(IdentityAsserterMessages.class);
+
+    public static final String VIRTUAL_GROUP_MAPPING_PREFIX = "group.mapping.";
+    public static final String GROUP_PRINCIPAL_MAPPING = 
"group.principal.mapping";
+    public static final String PRINCIPAL_MAPPING = "principal.mapping";
+    public static final String ADVANCED_PRINCIPAL_MAPPING = 
"expression.principal.mapping";
+    private static final String PRINCIPAL_PARAM = "user.name";
+    private static final String DOAS_PRINCIPAL_PARAM = "doAs";
+    static final String IMPERSONATION_ENABLED_PARAM = 
AuthFilterUtils.PROXYUSER_PREFIX + ".impersonation.enabled";
+
+    private SimplePrincipalMapper mapper = new SimplePrincipalMapper();
+    private final Parser parser = new Parser();
+    private VirtualGroupMapper virtualGroupMapper;
+    /* List of all default and configured impersonation params */
+    protected final List<String> impersonationParamsList = new ArrayList<>();
+    protected boolean impersonationEnabled;
+    private AbstractSyntaxTree expressionPrincipalMapping;
+    private String topologyName;
+    private boolean hasProxyGroupParams;
+
+    @Override
+    public void init(FilterConfig filterConfig) throws ServletException {
+        topologyName = (String) 
filterConfig.getServletContext().getAttribute(GatewayServices.GATEWAY_CLUSTER_ATTRIBUTE);
+        String principalMapping = 
filterConfig.getInitParameter(PRINCIPAL_MAPPING);
+        if (principalMapping == null || principalMapping.isEmpty()) {
+            principalMapping = 
filterConfig.getServletContext().getInitParameter(PRINCIPAL_MAPPING);
+        }
+        String groupPrincipalMapping = 
filterConfig.getInitParameter(GROUP_PRINCIPAL_MAPPING);
+        if (groupPrincipalMapping == null || groupPrincipalMapping.isEmpty()) {
+            groupPrincipalMapping = 
filterConfig.getServletContext().getInitParameter(GROUP_PRINCIPAL_MAPPING);
+        }
+        if (principalMapping != null && !principalMapping.isEmpty() || 
groupPrincipalMapping != null && !groupPrincipalMapping.isEmpty()) {

Review Comment:
   Do these need to be grouped? You're checking whether there are valid 
userproxy configs or group proxy configs, correct? Something like 
((hasPrincipalMapping) || (hasGroupMapping)):
   
   `if ((principalMapping != null && !principalMapping.isEmpty()) || 
(groupPrincipalMapping != null && !groupPrincipalMapping.isEmpty()))`
   
   Maybe that is how the logical operators are evaluated by default?



##########
gateway-spi/src/main/java/org/apache/knox/gateway/util/GroupBasedImpersonationProvider.java:
##########
@@ -0,0 +1,257 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.util;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.security.authorize.AccessControlList;
+import org.apache.hadoop.security.authorize.AuthorizationException;
+import org.apache.hadoop.security.authorize.DefaultImpersonationProvider;
+import org.apache.hadoop.util.MachineList;
+import org.apache.knox.gateway.i18n.GatewaySpiMessages;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+
+import java.net.InetAddress;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+import static org.apache.knox.gateway.util.AuthFilterUtils.PROXYGROUP_PREFIX;
+import static org.apache.knox.gateway.util.AuthFilterUtils.PROXYUSER_PREFIX;
+
+/**
+ * An extension of Hadoop's DefaultImpersonationProvider that adds support for 
group-based impersonation.
+ * This provider allows users who belong to specific groups to impersonate 
other users.
+ */
+public class GroupBasedImpersonationProvider extends 
DefaultImpersonationProvider {
+    private static final GatewaySpiMessages LOG = 
MessagesFactory.get(GatewaySpiMessages.class);
+    private static final String CONF_HOSTS = ".hosts";
+    private static final String CONF_USERS = ".users";
+    private static final String CONF_GROUPS = ".groups";
+    private final Map<String, AccessControlList> proxyGroupsAcls = new 
HashMap<>();
+    private Map<String, MachineList> groupProxyHosts = new HashMap<>();
+    private String groupConfigPrefix;
+    private boolean doesProxyUserConfigExist = true;
+    static final String IMPERSONATION_ENABLED_PARAM = "impersonation.enabled";
+
+    public GroupBasedImpersonationProvider() {
+        super();
+    }
+
+    @Override
+    public Configuration getConf() {
+        return super.getConf();
+    }
+
+    @Override
+    public void setConf(Configuration conf) {
+        super.setConf(conf);
+    }
+
+    @Override
+    public void init(String configurationPrefix) {
+        super.init(configurationPrefix);
+
+        /* Check if user proxy configs are provided */
+        final Map<String, String> filteredProps = 
Optional.ofNullable(getConf().getPropsWithPrefix(PROXYUSER_PREFIX + "."))
+                .orElse(Collections.emptyMap())  // handle null map defensively
+                .entrySet()
+                .stream()
+                .filter(entry -> 
!IMPERSONATION_ENABLED_PARAM.equals(entry.getKey())) // avoid NPE by reversing 
equals
+                .collect(Collectors.toMap(Map.Entry::getKey, 
Map.Entry::getValue));
+
+        doesProxyUserConfigExist = !filteredProps.isEmpty();
+
+        initGroupBasedProvider(PROXYGROUP_PREFIX);
+    }
+
+    private void initGroupBasedProvider(String proxyGroupPrefix) {
+        groupConfigPrefix = proxyGroupPrefix +
+                (proxyGroupPrefix.endsWith(".") ? "" : ".");
+
+        // constructing regex to match the following patterns:
+        //   $configPrefix.[ANY].users
+        //   $configPrefix.[ANY].groups
+        //   $configPrefix.[ANY].hosts
+        //
+        String prefixRegEx = groupConfigPrefix.replace(".", "\\.");

Review Comment:
   Could these regex defintions be constants?



##########
gateway-spi/src/main/java/org/apache/knox/gateway/util/GroupBasedImpersonationProvider.java:
##########
@@ -0,0 +1,257 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.util;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.security.authorize.AccessControlList;
+import org.apache.hadoop.security.authorize.AuthorizationException;
+import org.apache.hadoop.security.authorize.DefaultImpersonationProvider;
+import org.apache.hadoop.util.MachineList;
+import org.apache.knox.gateway.i18n.GatewaySpiMessages;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+
+import java.net.InetAddress;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+import static org.apache.knox.gateway.util.AuthFilterUtils.PROXYGROUP_PREFIX;
+import static org.apache.knox.gateway.util.AuthFilterUtils.PROXYUSER_PREFIX;
+
+/**
+ * An extension of Hadoop's DefaultImpersonationProvider that adds support for 
group-based impersonation.
+ * This provider allows users who belong to specific groups to impersonate 
other users.
+ */
+public class GroupBasedImpersonationProvider extends 
DefaultImpersonationProvider {
+    private static final GatewaySpiMessages LOG = 
MessagesFactory.get(GatewaySpiMessages.class);
+    private static final String CONF_HOSTS = ".hosts";
+    private static final String CONF_USERS = ".users";
+    private static final String CONF_GROUPS = ".groups";
+    private final Map<String, AccessControlList> proxyGroupsAcls = new 
HashMap<>();
+    private Map<String, MachineList> groupProxyHosts = new HashMap<>();
+    private String groupConfigPrefix;
+    private boolean doesProxyUserConfigExist = true;
+    static final String IMPERSONATION_ENABLED_PARAM = "impersonation.enabled";
+
+    public GroupBasedImpersonationProvider() {
+        super();
+    }
+
+    @Override
+    public Configuration getConf() {
+        return super.getConf();
+    }
+
+    @Override
+    public void setConf(Configuration conf) {
+        super.setConf(conf);
+    }
+
+    @Override
+    public void init(String configurationPrefix) {
+        super.init(configurationPrefix);
+
+        /* Check if user proxy configs are provided */
+        final Map<String, String> filteredProps = 
Optional.ofNullable(getConf().getPropsWithPrefix(PROXYUSER_PREFIX + "."))
+                .orElse(Collections.emptyMap())  // handle null map defensively
+                .entrySet()
+                .stream()
+                .filter(entry -> 
!IMPERSONATION_ENABLED_PARAM.equals(entry.getKey())) // avoid NPE by reversing 
equals
+                .collect(Collectors.toMap(Map.Entry::getKey, 
Map.Entry::getValue));
+
+        doesProxyUserConfigExist = !filteredProps.isEmpty();
+
+        initGroupBasedProvider(PROXYGROUP_PREFIX);
+    }
+
+    private void initGroupBasedProvider(String proxyGroupPrefix) {
+        groupConfigPrefix = proxyGroupPrefix +
+                (proxyGroupPrefix.endsWith(".") ? "" : ".");
+
+        // constructing regex to match the following patterns:
+        //   $configPrefix.[ANY].users
+        //   $configPrefix.[ANY].groups
+        //   $configPrefix.[ANY].hosts
+        //
+        String prefixRegEx = groupConfigPrefix.replace(".", "\\.");
+        String usersGroupsRegEx = prefixRegEx + "[\\S]*(" +
+                Pattern.quote(CONF_USERS) + "|" + Pattern.quote(CONF_GROUPS) + 
")";
+        String hostsRegEx = prefixRegEx + "[\\S]*" + Pattern.quote(CONF_HOSTS);
+
+        // get list of users and groups per proxygroup
+        // Map of <hadoop.proxygroup.[VIRTUAL_GROUP].users|groups, 
group1,group2>
+        Map<String, String> allMatchKeys =
+                getConf().getValByRegex(usersGroupsRegEx);
+
+        for (Map.Entry<String, String> entry : allMatchKeys.entrySet()) {
+            //aclKey = hadoop.proxygroup.[VIRTUAL_GROUP]
+            String aclKey = getAclKey(entry.getKey());
+
+            if (!proxyGroupsAcls.containsKey(aclKey)) {
+                proxyGroupsAcls.put(aclKey, new AccessControlList(
+                        allMatchKeys.get(aclKey + CONF_USERS),
+                        allMatchKeys.get(aclKey + CONF_GROUPS)));
+            }
+        }
+
+        // get hosts per proxygroup
+        allMatchKeys = getConf().getValByRegex(hostsRegEx);
+        for (Map.Entry<String, String> entry : allMatchKeys.entrySet()) {
+            groupProxyHosts.put(entry.getKey(),
+                    new MachineList(entry.getValue()));
+        }
+    }
+
+    private String getAclKey(String key) {
+        int endIndex = key.lastIndexOf('.');
+        if (endIndex != -1) {
+            return key.substring(0, endIndex);
+        }
+        return key;
+    }
+
+    /**
+     * Authorization based on user and group impersonation policies.
+     *
+     * @param user the user information attempting the operation, which 
includes the real
+     *             user and the effective impersonated user.
+     * @param remoteAddress the remote address from which the user is 
connecting.
+     * @throws AuthorizationException if the user is not authorized based on 
the
+     *                                configured impersonation and group 
policies.
+     */
+    @Override
+    public void authorize(UserGroupInformation user, InetAddress 
remoteAddress) throws AuthorizationException {
+        authorize(user, remoteAddress, Collections.emptyList());
+    }
+
+    /**
+     * Authorization based on groups that are provided as a function argument
+     *
+     * @param user the user information attempting the operation, which 
includes the real
+     *             user and the effective impersonated user.
+     * @param groups the list of groups to check for authorization.
+     * @param remoteAddress the remote address from which the user is 
connecting.
+     * @throws AuthorizationException if the user is not authorized based on 
the
+     *                                configured impersonation and group 
policies.
+     */
+    public void authorize(UserGroupInformation user, InetAddress 
remoteAddress, List<String> groups) throws AuthorizationException {
+
+        /**
+         *  check for proxy user authorization only if PROXYUSER_PREFIX 
properties exist.
+         *  If proxy is configured then use those properties instead of 
group-based properties
+         *  given user based configs are more finegrained.
+         */
+        if (doesProxyUserConfigExist) {
+            super.authorize(user, remoteAddress);
+        } else{

Review Comment:
   I would expect this to be evaluated in two cases:
   
   1. There is no proxyuser config
   2. There is no matching proxyuser config for this specific user
   
   I would not expect this to be an if-else relationship.





Issue Time Tracking
-------------------

    Worklog Id:     (was: 972019)
    Time Spent: 3h 10m  (was: 3h)

> Surrogate proxy user configuration for user groups
> --------------------------------------------------
>
>                 Key: KNOX-3048
>                 URL: https://issues.apache.org/jira/browse/KNOX-3048
>             Project: Apache Knox
>          Issue Type: Improvement
>          Components: Server
>    Affects Versions: 2.0.0
>            Reporter: Philip Zampino
>            Assignee: Sandeep More
>            Priority: Major
>             Fix For: 2.1.0
>
>          Time Spent: 3h 10m
>  Remaining Estimate: 0h
>
> *Problem Statement:*
> Currently Knox has the ability for specific users (say for e.g. {{sp_user}}) 
> to impersonate other users (say for e.g.{{ot_user}}). This is driven by 
> configs defined in a topology. Currently these configs are needed for each 
> user that impersonates other users (i.e. {{sp_user}}), this can get out of 
> hand quickly and can be difficult to maintain.
> To solve this problem the proposed solution uses a group level impersonation 
> configuration. This configuration will be based on the virtual groups feature 
> that is already available in Knox. With this new configuration we can have 
> specific users who belong to a virtual group/s (based on conditions such as 
> {{(match groups 'analyst|scientist') }}) impersonate other users. This will 
> significantly cut down on the config properties and keep them readable and 
> maintainable.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Reply via email to