[impala] 01/03: IMPALA-10192: Filter out redundant AuthzAuditEvent's for column masking

[st...@apache.org]

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

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

commit cd51d031188ddd805042b8df72eb8ef59496a546
Author: Fang-Yu Rao <fa...@cloudera.com>
AuthorDate: Mon Sep 28 09:08:24 2020 -0700

    IMPALA-10192: Filter out redundant AuthzAuditEvent's for column masking
    
    We found that Ranger would generate an AuthzAuditEvent as long as
    there exists a column masking policy corresponding to the column
    even though the policy does not apply to the requesting user. This
    resulted in an IllegalStateException if a user "A" submits a SELECT
    query against a table that has a column specified in a column masking
    policy when the policy does not apply to "A", i.e., the field of
    'Select User' for this policy in the Ranger web UI does not contain "A".
    For such an AuthzAuditEvent, its field of 'accessType' will not be one
    of the supported mask types since its corresponding
    accessResult.isMaskEnabled() would evaluates to false, indicating that
    there is no matching column masking policy associated with the user "A"
    and thus the AuthzAuditEvent will not be post-processed by Impala in
    RangerAuthorizationCheker#createColumnMask(). But since we did not
    filter out such an AuthzAuditEvent when it was generated and returned
    from RangerBasePlugin#evalDataMaskPolicies(), we failed the check that
    requires every AuthzAuditEvent be column masking-related in
    RangerAuthorizationContext#stashAuditEvents().
    
    To address this issue, in this patch we filter out such an
    AuthzAuditEvent after each call to
    RangerBasePlugin#evalDataMaskPolicies() so that no redundant
    AuthzAuditEvent is generated.
    
    Testing:
     - Added a new column masking policy associated with a non-matching user
       in RangerAuditLogTest#testAuditsForColumnMasking() to verify that
       the redundant AuthzAuditEvent is removed.
     - Verified that the patch passes the exhaustive tests in the DEBUG
       build.
    
    Change-Id: I1dbf65874003523b5176680e42f26fa2114c229b
    Reviewed-on: http://gerrit.cloudera.org:8080/16524
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 .../ranger/RangerAuthorizationChecker.java         | 27 ++++++++++++++++++----
 .../authorization/ranger/RangerAuditLogTest.java   | 15 +++++++++---
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java b/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
index b4812ec..d3bf9b6 100644
--- a/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
+++ b/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
@@ -305,6 +305,7 @@ public class RangerAuthorizationChecker extends BaseAuthorizationChecker {
       String columnName, AuthorizationContext rangerCtx) throws InternalException {
     RangerBufferAuditHandler auditHandler =
         ((RangerAuthorizationContext) rangerCtx).getAuditHandler();
+    int numAuthzAuditEventsBefore = auditHandler.getAuthzEvents().size();
     RangerAccessResult accessResult = evalColumnMask(user, dbName, tableName, columnName,
         auditHandler);
     // When a user adds an "Unmasked" policy for 'columnName' that retains the original
@@ -318,15 +319,30 @@ public class RangerAuthorizationChecker extends BaseAuthorizationChecker {
     // whereas this method will be called when there are policies of other types. If we
     // do not remove the event of the "Unmasked" policy, we will have one more logged
     // event in the latter case where there are policies other than "Unmasked".
+
+    // Moreover, notice that there will be an AuthzAuditEvent added to
+    // 'auditHandler.getAuthzEvents()' as long as there exists a column masking policy
+    // associated with 'dbName', 'tableName', and 'columnName', even though the policy
+    // does NOT apply to 'user'. In this case, accessResult.isMaskEnabled() would be
+    // false, and accessResult.getPolicyId() would be -1. In addition, the field of
+    // 'accessResult' would be 0 for that AuthzAuditEvent. An AuthzAuditEvent with
+    // 'accessResult' equal to 0 will be considered as an indication of a failed access
+    // request in RangerBufferAuditHandler#flush() even though there is no failed access
+    // request at all for the current query and thus we need to filter out this
+    // AuthzAuditEvent. If we did not filter it out, other AuthzAuditEvent's
+    // corresponding to successful/allowed access requests in the same query will not be
+    // logged since only the first AuthzAuditEvent with 'accessResult' equal to 0 is
+    // logged in RangerBufferAuditHandler#flush().
     List<AuthzAuditEvent> auditEvents = auditHandler.getAuthzEvents();
-    if (StringUtils.equalsIgnoreCase((accessResult.getMaskType()), "MASK_NONE")) {
+    Preconditions.checkState(auditEvents.size() - numAuthzAuditEventsBefore <= 1);
+    if (auditEvents.size() > numAuthzAuditEventsBefore &&
+        !accessResult.isMaskEnabled()) {
       // Recall that the same instance of RangerAuthorizationContext is passed to
       // createColumnMask() every time this method is called. Thus the same instance of
       // RangerBufferAuditHandler is provided for evalColumnMask() to log the event.
       // Since there will be exactly one event added to 'auditEvents' when there is a
-      // corresponding column masking policy, i.e., accessResult.isMaskEnabled() evaluates
-      // to true, we only need to process the last event on 'auditEvents'.
-      Preconditions.checkState(!auditEvents.isEmpty());
+      // corresponding column masking policy, i.e., accessResult.isMaskEnabled()
+      // evaluates to true, we only need to process the last event on 'auditEvents'.
       auditHandler.getAuthzEvents().remove(auditEvents.size() - 1);
     }
     // No column masking policies, return the original column.
@@ -376,6 +392,9 @@ public class RangerAuthorizationChecker extends BaseAuthorizationChecker {
   /**
    * Evaluate column masking policies on the given column and returns the result.
    * A RangerAccessResult contains the matched policy details and the masked column.
+   * Note that Ranger will add an AuthzAuditEvent to auditHandler.getAuthzEvents() as
+   * long as there exists a policy for the given column even though the policy does not
+   * apply to 'user'.
    */
   private RangerAccessResult evalColumnMask(User user, String dbName,
       String tableName, String columnName, RangerBufferAuditHandler auditHandler)
diff --git a/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java b/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java
index 2b4775c..a23a0c4 100644
--- a/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java
+++ b/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java
@@ -220,8 +220,11 @@ public class RangerAuditLogTest extends AuthorizationTestBase {
   public void testAuditsForColumnMasking() throws ImpalaException {
     String databaseName = "functional";
     String tableName = "alltypestiny";
-    String policyNames[] = {"col_mask_custom", "col_mask_null", "col_mask_none"};
-    String columnNames[] = {"string_col", "date_string_col", "id"};
+    String policyNames[] = {"col_mask_custom", "col_mask_null", "col_mask_none",
+        "col_mask_redact"};
+    String columnNames[] = {"string_col", "date_string_col", "id", "year"};
+    String users[] = {user_.getShortName(), user_.getShortName(), user_.getShortName(),
+        "non_owner_2"};
     String masks[] = {
         "  {\n" +
         "    \"dataMaskType\": \"CUSTOM\",\n" +
@@ -234,6 +237,12 @@ public class RangerAuditLogTest extends AuthorizationTestBase {
         // the Ranger UI to verify the respective AuthzAuditEvent is removed.
         "  {\n" +
         "    \"dataMaskType\": \"MASK_NONE\"\n" +
+        "  }\n",
+        // We add a mask of type "MASK" that corresponds to a "Redact" policy in the
+        // Ranger UI for the user "non_owner_2" to verify the respective AuthzAuditEvent
+        // is removed.
+        "  {\n" +
+        "    \"dataMaskType\": \"MASK\"\n" +
         "  }\n"
     };
 
@@ -275,7 +284,7 @@ public class RangerAuditLogTest extends AuthorizationTestBase {
           "    }\n" +
           "  ]\n" +
           "}", policyNames[i], RANGER_SERVICE_TYPE, RANGER_SERVICE_NAME, databaseName,
-          tableName, columnNames[i], user_.getShortName(), masks[i]);
+          tableName, columnNames[i], users[i], masks[i]);
       policies.add(json);
     }