This is an automated email from the ASF dual-hosted git repository. stigahuang pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 62e20d1ba842a3f27395251c57dea9850f462fc9 Author: Fang-Yu Rao <[email protected]> AuthorDate: Thu Aug 11 16:37:23 2022 -0700 IMPALA-11494: Don't always produce Ranger audit log for authorized query Before this patch, when Impala could not resolve a given table '<db_name>.<tbl_name>' during the query analysis, Impala would still attempt to register 2 privilege requests. One was for the table '<tbl_name>' under the database '<db_name>' and the other was for the table '<db_name>' under the database 'default'. The first one should be registered since Impala had to determine whether such an access should be allowed (even though in fact the table did not exist), whereas the second one was incorrect in that 'default.<db_name>' definitely was not '<db_name>.<tbl_name>' in general. Furthermore, Impala always sent audit log entries to the Ranger server for an authorized query against non-existing table(s). The 2 facts described above resulted in Impala producing Ranger audit log entries for the tables '<db_name>.<tbl_name>' and 'default.<db_name>' when a requesting user granted sufficient privileges on the databases of '<db_name>' and 'default' submitted a query against a non-existing table '<db_name>.<tbl_name>'. None of the audit log entries should be generated because i) the privilege request for '<db_name>.<tbl_name>' was allowed and '<db_name>.<tbl_name>' did not exist, and ii) 'default.<db_name>' did not correspond to a table. This patch fixes the 2 issues mentioned above so that Impala will not generate any Ranger audit log entry for an authorized query against a non-existing table. Testing: - Added a frontend test case to verify no Ranger audit log entry will be produced for an authorized query against a non-existing table. Change-Id: I701652e457d3118f43249e83be933713b17ce48f Reviewed-on: http://gerrit.cloudera.org:8080/18850 Reviewed-by: Csaba Ringhofer <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- .../apache/impala/analysis/AnalysisContext.java | 3 +- .../java/org/apache/impala/analysis/Analyzer.java | 18 ++++++------ .../impala/authorization/AuthorizationChecker.java | 2 +- .../authorization/BaseAuthorizationChecker.java | 3 +- .../ranger/RangerAuthorizationChecker.java | 18 ++++++++++-- .../authorization/AuthorizationTestBase.java | 33 ++++++++++++++++++++-- .../authorization/ranger/RangerAuditLogTest.java | 19 +++++++++++-- 7 files changed, 75 insertions(+), 21 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java b/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java index 9304ffbe5..bb21809a7 100644 --- a/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java +++ b/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java @@ -487,7 +487,8 @@ public class AnalysisContext { } catch (AuthorizationException e) { authException = e; } finally { - authzChecker.postAuthorize(authzCtx, authException == null); + authzChecker.postAuthorize(authzCtx, authException == null, + analysisException == null); } } diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java index d0c4ac131..b2742e269 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java +++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java @@ -892,16 +892,16 @@ public class Analyzer { } return builder.build(); }); + } else { + registerPrivReq(builder -> { + builder.onTableUnknownOwner( + getDefaultDb(), tableRawPath.get(0)).allOf(tableRef.getPrivilege()); + if (tableRef.requireGrantOption()) { + builder.grantOption(); + } + return builder.build(); + }); } - - registerPrivReq(builder -> { - builder.onTableUnknownOwner( - getDefaultDb(), tableRawPath.get(0)).allOf(tableRef.getPrivilege()); - if (tableRef.requireGrantOption()) { - builder.grantOption(); - } - return builder.build(); - }); } /** diff --git a/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java b/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java index 17749b561..10d698b2c 100644 --- a/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java +++ b/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java @@ -70,7 +70,7 @@ public interface AuthorizationChecker { /** * This method is to be executed after an authorization check has occurred. */ - void postAuthorize(AuthorizationContext authzCtx, boolean authzOk) + void postAuthorize(AuthorizationContext authzCtx, boolean authzOk, boolean analysisOk) throws AuthorizationException, InternalException; /** 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 698332972..1ab05cf09 100644 --- a/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java +++ b/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java @@ -104,7 +104,8 @@ public abstract class BaseAuthorizationChecker implements AuthorizationChecker { * Override this method to add custom post-authorization check. */ @Override - public void postAuthorize(AuthorizationContext authzCtx, boolean authzOk) { + public void postAuthorize(AuthorizationContext authzCtx, boolean authzOk, + boolean analysisOk) { if (authzCtx.getTimeline().isPresent()) { EventSequence timeline = authzCtx.getTimeline().get(); long durationMs = timeline.markEvent(String.format("Authorization finished (%s)", 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 03fe16ded..668d904a1 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 @@ -177,9 +177,10 @@ public class RangerAuthorizationChecker extends BaseAuthorizationChecker { } @Override - public void postAuthorize(AuthorizationContext authzCtx, boolean authzOk) { + public void postAuthorize(AuthorizationContext authzCtx, boolean authzOk, + boolean analysisOk) { Preconditions.checkArgument(authzCtx instanceof RangerAuthorizationContext); - super.postAuthorize(authzCtx, authzOk); + super.postAuthorize(authzCtx, authzOk, analysisOk); // Consolidate the audit log entries and apply the deduplicated column masking events // to update the List of all AuthzAuditEvent's only if the authorization is // successful. @@ -194,7 +195,18 @@ public class RangerAuthorizationChecker extends BaseAuthorizationChecker { } RangerBufferAuditHandler auditHandler = ((RangerAuthorizationContext) authzCtx).getAuditHandler(); - auditHandler.flush(); + if (authzOk && !analysisOk) { + // When the query was authorized, we do not send any audit log entry to the Ranger + // server if there was an AnalysisException during query analysis. + // We still have to call clear() to remove audit log entries in this case because + // the current test framework checks the contents in auditHandler.getAuthzEvents() + // to determine whether the correct audit events are collected. + auditHandler.getAuthzEvents().clear(); + } else { + // We send audit log entries to the Ranger server only if authorization failed or + // analysis succeeded. + auditHandler.flush(); + } } @Override diff --git a/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java b/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java index 082f35c46..bd65f9b8b 100644 --- a/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java +++ b/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java @@ -30,6 +30,7 @@ import org.apache.impala.authorization.ranger.RangerImpalaResourceBuilder; import org.apache.impala.catalog.Role; import org.apache.impala.catalog.ScalarFunction; import org.apache.impala.catalog.Type; +import org.apache.impala.common.AnalysisException; import org.apache.impala.common.FrontendTestBase; import org.apache.impala.common.ImpalaException; import org.apache.impala.service.Frontend; @@ -319,13 +320,22 @@ public abstract class AuthorizationTestBase extends FrontendTestBase { */ public AuthzTest ok(TPrivilege[]... privileges) throws ImpalaException { + ok(/* expectAnalysisOk */ true, privileges); + return this; + } + + /** + * This method runs with the specified privileges. + */ + public AuthzTest ok(boolean expectAnalysisOk, TPrivilege[]... privileges) + throws ImpalaException { for (WithPrincipal withPrincipal: buildWithPrincipals()) { try { withPrincipal.init(privileges); if (context_ != null) { - authzOk(context_, stmt_, withPrincipal); + authzOk(context_, stmt_, withPrincipal, expectAnalysisOk); } else { - authzOk(stmt_, withPrincipal); + authzOk(stmt_, withPrincipal, expectAnalysisOk); } } finally { withPrincipal.cleanUp(); @@ -482,11 +492,21 @@ public abstract class AuthorizationTestBase extends FrontendTestBase { } private void authzOk(String stmt, WithPrincipal withPrincipal) throws ImpalaException { - authzOk(authzCtx_, stmt, withPrincipal); + authzOk(authzCtx_, stmt, withPrincipal, /* expectAnalysisOk */ true); + } + + private void authzOk(String stmt, WithPrincipal withPrincipal, + boolean expectAnalysisOk) throws ImpalaException { + authzOk(authzCtx_, stmt, withPrincipal, expectAnalysisOk); } private void authzOk(AnalysisContext context, String stmt, WithPrincipal withPrincipal) throws ImpalaException { + authzOk(context, stmt, withPrincipal, /* expectAnalysisOk */ true); + } + + private void authzOk(AnalysisContext context, String stmt, WithPrincipal withPrincipal, + boolean expectAnalysisOk) throws ImpalaException { try { LOG.info("Testing authzOk for {}", stmt); parseAndAnalyze(stmt, context, authzFrontend_); @@ -496,6 +516,13 @@ public abstract class AuthorizationTestBase extends FrontendTestBase { throw new AuthorizationException(String.format( "\nPrincipal: %s\nStatement: %s\nError: %s", withPrincipal.getName(), stmt, e.getMessage(), e)); + } catch (AnalysisException e) { + // We throw an AnalysisException only if we did not expect query analysis to fail. + if (expectAnalysisOk) { + throw new AnalysisException(String.format( + "\nPrincipal: %s\nStatement: %s\nError: %s", withPrincipal.getName(), + stmt, e.getMessage(), e)); + } } } 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 6f4e7b4ca..8c7e60cc5 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 @@ -53,8 +53,9 @@ public class RangerAuditLogTest extends AuthorizationTestBase { } @Override - public void postAuthorize(AuthorizationContext authzCtx, boolean authzOk) { - super.postAuthorize(authzCtx, authzOk); + public void postAuthorize(AuthorizationContext authzCtx, boolean authzOk, + boolean analysisOk) { + super.postAuthorize(authzCtx, authzOk, analysisOk); authzCtx_ = authzCtx; } } @@ -278,6 +279,13 @@ public class RangerAuditLogTest extends AuthorizationTestBase { }, "select min(id) from functional.alltypes union all " + "select max(id) from functional.alltypes", onTable("functional", "alltypes", TPrivilegeLevel.SELECT)); + + // No audit log entry should be produced for an authorized query against a + // non-existing table. + authzOk(events -> { + assertEquals(0, events.size()); + }, "select * from functional.non_existing_tbl", + /* expectAnalysisOk */ false, onDatabase("functional", TPrivilegeLevel.SELECT)); } @Test @@ -802,7 +810,12 @@ public class RangerAuditLogTest extends AuthorizationTestBase { private void authzOk(Consumer<List<AuthzAuditEvent>> resultChecker, String stmt, TPrivilege[]... privileges) throws ImpalaException { - authorize(stmt).ok(privileges); + authzOk(resultChecker, stmt, /* expectAnalysisOk */ true, privileges); + } + + private void authzOk(Consumer<List<AuthzAuditEvent>> resultChecker, String stmt, + boolean expectAnalysisOk, TPrivilege[]... privileges) throws ImpalaException { + authorize(stmt).ok(expectAnalysisOk, privileges); RangerAuthorizationContext rangerCtx = (RangerAuthorizationContext) authzChecker_.authzCtx_; Preconditions.checkNotNull(rangerCtx);
