Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12927 )
Change subject: IMPALA-8363: Deny access when column masking or row filtering is enabled in Ranger ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@106 PS4, Line 106: StatementBase consider adding @Nullable for this parameter http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@120 PS4, Line 120: * Performs an authorization for a given user. can you add to the javadoc indicating what 'statement' is used for http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@123 PS4, Line 123: statement and this one http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java File fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java: http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java@47 PS4, Line 47: NoopAuthorizationFactory if it's not too much of a pain, is it possible to separate out these unrelated changes into a different commit? makes it hard to review when there is refactoring/renaming going on at the same time as a functional change http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@160 PS4, Line 160: case COLUMN: I'm a bit confused by the implementation here... It seems like in AnalysisContext.authorize() we're already doing the work to collect the tables and columns that need access, and calling down to here once per table/column. Now, for each of those calls, you're duplicating the work of collecting the accessed tables/columns and checking them. In some cases this will end up doing a bunch of repeated work, and it also duplicates a bunch of code. The one barrier to reusing the existing code in AnalysisContext looks like it sees SELECT access on the table, and at that point short circuits the column-level checks by setting 'hasTableSelectPriv = true'. In the case that one column is masked, I guess we could either have this return 'false' for table-level select access and then 'true' for all of the non-masked columns, or we could make the code return an enum like 'HAS_MASKED_COLUMNS' for the table-level check, so that it then falls through to checking each referenced column? http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@206 PS4, Line 206: return false; Curious whether it would be feasible to give a different error message indicating that the access is denied due to lack of filtering/masking support. That way, a user hitting this "access denied" can realize that they could use Hive to do their work if they need access to this table. Maybe add a TODO and come back to it since it probably requires a lot of plumbing so that this method would return an enum instead of a boolean. http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@250 PS4, Line 250: } I'm worried that this pattern will be very hard to miss when adding a new type of statement, and we'll end up not doing appropriate authorization Perhaps we can add a 'visitExprs' method to StatementBase or something and ensure that all statement subclasses implement it appropriately? (this may be unnecessary if we rely on the existing analysis code to collect referenced columns) -- To view, visit http://gerrit.cloudera.org:8080/12927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If46b4bf24d916e4a4ea8a36ff4acfd95d5f45c8e Gerrit-Change-Number: 12927 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 05 Apr 2019 17:08:24 +0000 Gerrit-HasComments: Yes
