IMPALA-10505: Avoid creating misleading audit logs
Before this patch, if a requesting user granted the privilege on a view
does not have the privilege on the table(s) on which the view is based,
only an audit log entry indicating a failed authorization with respect
to an underlying table will be produced, whereas the requesting user is
actually able to fetch the data from the view since the user is granted
the privilege to do so. Such an audit log entry, however, is misleading
and thus should not be produced at all. Moreover, the audit log entry
corresponding to the successful authorization with respect to the view
should also be created.
Recall that to authorize a query involving a view, Impala performs
privilege checks for both the view as well as the underlying table(s).
Thus, for a user granted the privilege on the view but not the
underlying tables, the privilege check for the view would succeed but
those for the underlying table(s) would fail. Each privilege check
results in an audit log entry produced by Ranger. These audit log
entries will be collected by Impala and will be sent back to Ranger
after the query authorization. In the case where there is at least one
AuthzAuditEvent indicating a failed privilege check, only the
AuthzAuditEvent corresponding to the first failed check will be sent
back to Ranger. Refer to RangerBufferAuditHandler#flush() for further
details. Impala performs checks for both the view as well as the
underlying table(s) so that it is able to disallow the requesting user
from accessing the runtime profile or execution summary when the
requesting user is not granted the privilege on the underlying table(s).
Note that allowing the requesting user the access to the runtime profile
would reveal the existence of the underlying tables.
This patch resolves the issue by specifying whether or not we should
retain the audit log entries when calling
BaseAuthorizationChecker#authorizePrivilegeRequest() so that Impala will
not collect the audit log entries resulting from the privilege checks
for the underlying table(s) of a view.
Testing:
- Added new FE tests to verify that the correct audit log entry is
produced after this patch.
- Added a new E2E test to verify that a user not granted the privilege
on the underlying table(s) of a view is still not able to access the
runtime profile or execution summary even though the user is granted
the privilege on the view.
- Verified that the patch passes the core tests in the DEBUG build.
Change-Id: I02f40eb96d6ed863cd2cd88d717c354dc351a64c
Reviewed-on: http://gerrit.cloudera.org:8080/17078
Reviewed-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Tested-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
diff --git a/fe/src/main/java/org/apache/impala/authorization/AuthorizationContext.java b/fe/src/main/java/org/apache/impala/authorization/AuthorizationContext.java
index 1930dc2..5972a9a 100644
--- a/fe/src/main/java/org/apache/impala/authorization/AuthorizationContext.java
+++ b/fe/src/main/java/org/apache/impala/authorization/AuthorizationContext.java
@@ -27,6 +27,8 @@
public class AuthorizationContext {
private final Optional<EventSequence> timeline_;
+ private boolean retainAudits_ = true;
+
public AuthorizationContext(Optional<EventSequence> timeline) {
this.timeline_ = timeline;
}
@@ -36,4 +38,7 @@
*/
public Optional<EventSequence> getTimeline() { return timeline_; }
+ public void setRetainAudits(boolean retainAudits) { retainAudits_ = retainAudits; }
+
+ public boolean getRetainAudits() { return retainAudits_; }
}
diff --git a/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java b/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
index 42781d8..09bb7804 100644
--- a/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
+++ b/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
@@ -179,6 +179,7 @@
// 'user_has_profile_access' flag in queryCtx_.
for (Pair<PrivilegeRequest, String> maskedReq : analyzer.getMaskedPrivilegeReqs()) {
try {
+ authzCtx.setRetainAudits(false);
authorizePrivilegeRequest(authzCtx, analysisResult, catalog, maskedReq.first);
} catch (AuthorizationException e) {
analysisResult.setUserHasProfileAccess(false);
@@ -186,6 +187,8 @@
throw new AuthorizationException(maskedReq.second);
}
break;
+ } finally {
+ authzCtx.setRetainAudits(true);
}
}
}
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 d3bf9b6..eb62599 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
@@ -455,7 +455,10 @@
// Use a temporary audit handler instead of the original audit handler
// so that we know all the audit events generated by the temporary audit
// handler are contained to the given resource.
- RangerBufferAuditHandler tmpAuditHandler = originalAuditHandler == null ?
+ // Recall that in some case, e.g., the DESCRIBE statement, 'originalAuditHandler'
+ // could be null, resulting in 'tmpAuditHandler' being null as well.
+ RangerBufferAuditHandler tmpAuditHandler =
+ (originalAuditHandler == null || !authzCtx.getRetainAudits()) ?
null : new RangerBufferAuditHandler(originalAuditHandler);
for (Privilege impliedPrivilege: privilege.getImpliedPrivileges()) {
if (authorizeResource(authzCtx, resource, user, impliedPrivilege,
@@ -464,7 +467,9 @@
break;
}
}
- if (originalAuditHandler != null) {
+ // 'tmpAuditHandler' could be null if 'originalAuditHandler' is null or
+ // authzCtx.getRetainAudits() is false.
+ if (originalAuditHandler != null && tmpAuditHandler != null) {
updateAuditEvents(tmpAuditHandler, originalAuditHandler, true /*any*/,
privilege);
}
@@ -479,7 +484,13 @@
// Use a temporary audit handler instead of the original audit handler
// so that we know all the audit events generated by the temporary audit
// handler are contained to the given resource.
- RangerBufferAuditHandler tmpAuditHandler = originalAuditHandler == null ?
+ // Recall that if 'min_privilege_set_for_show_stmts' is specified when the impalad's
+ // are started, this method will be called with the RangerBufferAuditHandler in
+ // 'authzCtx' being null for the SHOW DATABASES and SHOW TABLES statements. Refer to
+ // BaseAuthorizationChecker#hasAccess(User user, PrivilegeRequest request) for
+ // further details.
+ RangerBufferAuditHandler tmpAuditHandler =
+ (originalAuditHandler == null || !authzCtx.getRetainAudits()) ?
null : new RangerBufferAuditHandler(originalAuditHandler);
for (Privilege impliedPrivilege: privilege.getImpliedPrivileges()) {
if (!authorizeResource(authzCtx, resource, user, impliedPrivilege,
@@ -488,7 +499,9 @@
break;
}
}
- if (originalAuditHandler != null) {
+ // 'tmpAuditHandler' could be null if 'originalAuditHandler' is null or
+ // authzCtx.getRetainAudits() is false.
+ if (originalAuditHandler != null && tmpAuditHandler != null) {
updateAuditEvents(tmpAuditHandler, originalAuditHandler, false /*not any*/,
privilege);
}
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 a23a0c4..06bba79 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
@@ -160,6 +160,45 @@
}, "compute stats FUNCTIONAL.ALLTYPES",
onTable("functional", "alltypes", TPrivilegeLevel.ALTER,
TPrivilegeLevel.SELECT));
+
+ // Recall that the view 'functional.complex_view' is created based on two underlying
+ // tables, i.e., functional.alltypesagg and functional.alltypestiny. The following 3
+ // test cases make sure the same audit log entry indicating a successful
+ // authorization event is produced whether or not the requesting user is granted the
+ // privileges on any of the underlying tables.
+ authzOk(events -> {
+ assertEquals(1, events.size());
+ assertEventEquals("@table", "select", "functional/complex_view", 1,
+ events.get(0));
+ assertEquals("select count(*) from functional.complex_view",
+ events.get(0).getRequestData());
+ }, "select count(*) from functional.complex_view",
+ onTable("functional", "complex_view", TPrivilegeLevel.SELECT));
+
+ // The same audit log entry is produced if the requesting user is granted the
+ // privilege on one of the underlying tables.
+ authzOk(events -> {
+ assertEquals(1, events.size());
+ assertEventEquals("@table", "select", "functional/complex_view", 1,
+ events.get(0));
+ assertEquals("select count(*) from functional.complex_view",
+ events.get(0).getRequestData());
+ }, "select count(*) from functional.complex_view",
+ onTable("functional", "complex_view", TPrivilegeLevel.SELECT),
+ onTable("functional", "alltypesagg", TPrivilegeLevel.SELECT));
+
+ // The same audit log entry is produced if the requesting user is granted the
+ // privileges on all of the underlying tables.
+ authzOk(events -> {
+ assertEquals(1, events.size());
+ assertEventEquals("@table", "select", "functional/complex_view", 1,
+ events.get(0));
+ assertEquals("select count(*) from functional.complex_view",
+ events.get(0).getRequestData());
+ }, "select count(*) from functional.complex_view",
+ onTable("functional", "complex_view", TPrivilegeLevel.SELECT),
+ onTable("functional", "alltypesagg", TPrivilegeLevel.SELECT),
+ onTable("functional", "alltypestiny", TPrivilegeLevel.SELECT));
}
@Test
diff --git a/tests/authorization/test_ranger.py b/tests/authorization/test_ranger.py
index 74940e6..791755d 100644
--- a/tests/authorization/test_ranger.py
+++ b/tests/authorization/test_ranger.py
@@ -1069,6 +1069,69 @@
@pytest.mark.execute_serially
@CustomClusterTestSuite.with_args(
+ impalad_args=IMPALAD_ARGS, catalogd_args=CATALOGD_ARGS)
+ def test_profile_protection(self, vector):
+ """Test that a requesting user is able to access the runtime profile or execution
+ summary of a query involving a view only if the user is granted the privileges on all
+ the underlying tables of the view. Recall that the view functional.complex_view we
+ use here is created based on the tables functional.alltypesagg and
+ functional.alltypestiny."""
+ admin_client = self.create_impala_client()
+ non_owner_client = self.create_impala_client()
+ test_db = "functional"
+ test_view = "complex_view"
+ grantee_user = "non_owner"
+ try:
+ admin_client.execute(
+ "grant select on table {0}.{1} to user {2}"
+ .format(test_db, test_view, grantee_user), user=ADMIN)
+
+ admin_client.execute("refresh authorization")
+ # Recall that in a successful execution, result.exec_summary and
+ # result.runtime_profile store the execution summary and runtime profile,
+ # respectively. But when the requesting user does not have the privileges
+ # on the underlying tables, an exception will be thrown from
+ # ImpalaBeeswaxClient.get_runtime_profile().
+ result = self.execute_query_expect_failure(
+ non_owner_client, "select count(*) from {0}.{1}".format(test_db, test_view),
+ user=grantee_user)
+ assert "User {0} is not authorized to access the runtime profile or " \
+ "execution summary".format(grantee_user) in str(result)
+
+ admin_client.execute(
+ "grant select on table {0}.alltypesagg to user {1}"
+ .format(test_db, grantee_user), user=ADMIN)
+
+ admin_client.execute("refresh authorization")
+ self.execute_query_expect_failure(
+ non_owner_client, "select count(*) from {0}.{1}".format(test_db, test_view),
+ user=grantee_user)
+ assert "User {0} is not authorized to access the runtime profile or " \
+ "execution summary".format(grantee_user) in str(result)
+
+ admin_client.execute(
+ "grant select on table {0}.alltypestiny to user {1}"
+ .format(test_db, grantee_user), user=ADMIN)
+
+ admin_client.execute("refresh authorization")
+ self.execute_query_expect_success(
+ non_owner_client, "select count(*) from {0}.{1}".format(test_db, test_view),
+ user=grantee_user)
+ finally:
+ cleanup_statements = [
+ "revoke select on table {0}.{1} from user {2}"
+ .format(test_db, test_view, grantee_user),
+ "revoke select on table {0}.alltypesagg from user {1}"
+ .format(test_db, grantee_user),
+ "revoke select on table {0}.alltypestiny from user {1}"
+ .format(test_db, grantee_user)
+ ]
+
+ for statement in cleanup_statements:
+ admin_client.execute(statement, user=ADMIN)
+
+ @pytest.mark.execute_serially
+ @CustomClusterTestSuite.with_args(
# We additionally provide impalad and catalogd with the customized user-to-groups
# mapper since some test cases in grant_revoke.test require Impala to retrieve the
# groups a given user belongs to and such users might not exist in the underlying
