[impala] 01/03: IMPALA-9341: Set delegateAdmin to false for REVOKE without GRANT OPTION

[ta...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 3ba6fcb47909af7c4f22493a94cd08e9b9689a88
Author: Fang-Yu Rao <fa...@cloudera.com>
AuthorDate: Sat Jun 6 15:08:28 2020 -0700

    IMPALA-9341: Set delegateAdmin to false for REVOKE without GRANT OPTION
    
    When executing a GRANT or REVOKE statement with Ranger being the
    authorization provider, Impala has to prepare a GrantRevokeRequest to
    allow Ranger to add/delete the corresponding RangerPolicy or modify the
    existing RangerPolicyItem's in the related RangerPolicy. One of the
    fields that has to be set in a GrantRevokeRequest is delegateAdmin,
    which dictates whether the grantee is allowed to transfer the privilege
    on the resource to other principals. Specifically, the field
    of delegateAdmin in the updated RangerPolicyItem corresponding to the
    grantee would be set to the value of delegateAdmin in the
    GrantRevokeRequest prepared by Impala.
    
    Before this patch, when executing a REVOKE statement without the GRANT
    OPTION, Impala would set delegateAdmin in the GrantRevokeRequest to
    true. This is fine if the privilege to be revoked is the only privilege
    that was previously granted to the grantee. However, in the case when
    the privilege to be revoked was not granted and there is a
    RangerPolicyItem with respect to the other privilege on the same
    resource, the grantee actually obtains the permission to transfer the
    non-matching privilege afterwards. The root cause of this issue is that
    the privileges on the same resource share the same field of
    delegateAdmin in the corresponding RangerPolicyItem, a current
    limitation of Ranger. In this regard, as a workaround, we set
    delegateAdmin in the GrantRevokeRequest to false for a REVOKE statement
    without the GRANT OPTION. As a result, delegateAdmin would be false in
    the GrantRevokeRequest whether or not the query has the GRANT OPTION.
    
    We would like to point out that there is a limitation of this
    workaround. More precisely, in the case when the grantee was permitted
    to transfer the non-matching privilege, setting delegateAdmin to false
    in the GrantRevokeRequest would deprive the grantee of the permission
    that should not have been revoked, making it a bit inconvenient for both
    the administrator and the grantee since the permission to transfer the
    non-matching privilege should be restored afterwards if necessary.
    
    An alternative approach is for Impala to always check the current
    delegateAdmin value when performing a REVOKE statement without the GRANT
    OPTION. Specifically, we could resolve this problem by 1) checking
    whether or not there exists a RangerPolicyItem with respect to the same
    resource and the grantee such that the delegateAdmin field is set to
    true and 2) setting up the delegateAdmin field in the GrantRevokeRequest
    accordingly. This alternative, however, suffers from the drawback that
    additional logic has to be added to iterate over the RangerPolicy's
    for the resource in the query, slowing down the query execution.
    Therefore, we decide to choose the approach proposed in this patch over
    the alternative that is less efficient, which could be found at
    https://gerrit.cloudera.org/c/16013/.
    
    Testing:
    - Revised a test case in test_ranger.py to reflect the behavior change
      of Impala when a REVOKE statement without the GRANT OPTION is
      executed.
    - Verified that this patch passed the exhaustive tests in the DEBUG
      build.
    
    Change-Id: I19ff45a5a30293e9c6cf35b22ea4aa5cb10355c9
    Reviewed-on: http://gerrit.cloudera.org:8080/16046
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 .../authorization/ranger/RangerCatalogdAuthorizationManager.java  | 2 +-
 tests/authorization/test_ranger.py                                | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java b/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
index 9229272..6f741ba 100644
--- a/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
+++ b/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
@@ -285,7 +285,7 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager
     request.setGrantor(grantor);
     if (user != null) request.getUsers().add(user);
     if (!groups.isEmpty()) request.getGroups().addAll(groups);
-    request.setDelegateAdmin(isGrant == withGrantOpt);
+    request.setDelegateAdmin(isGrant && withGrantOpt);
     request.setEnableAudit(Boolean.TRUE);
     request.setReplaceExistingPermissions(Boolean.FALSE);
     request.setClusterName(clusterName);
diff --git a/tests/authorization/test_ranger.py b/tests/authorization/test_ranger.py
index ac4d821..42c04d3 100644
--- a/tests/authorization/test_ranger.py
+++ b/tests/authorization/test_ranger.py
@@ -156,9 +156,13 @@ class TestRanger(CustomClusterTestSuite):
                                         .format(unique_database, user1), user=ADMIN)
       result = self.execute_query("show grant user {0} on database {1}"
                                   .format(user1, unique_database))
+      # Revoking the select privilege also deprives the grantee of the permission to
+      # transfer other privilege(s) on the same resource to other principals. This is a
+      # current limitation of Ranger since privileges on the same resource share the same
+      # delegateAdmin field in the corresponding RangerPolicyItem.
       TestRanger._check_privileges(result, [
-          ["USER", user1, unique_database, "", "", "", "*", "insert", "true"],
-          ["USER", user1, unique_database, "*", "*", "", "", "insert", "true"]])
+          ["USER", user1, unique_database, "", "", "", "*", "insert", "false"],
+          ["USER", user1, unique_database, "*", "*", "", "", "insert", "false"]])
 
       # Revoke privilege granting from user 1
       self.execute_query_expect_success(admin_client, "revoke grant option for insert "