Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16524 )
Change subject: IMPALA-10192: Filter out redundant AuthzAuditEvent's for column masking ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/16524/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16524/1//COMMIT_MSG@9 PS1, Line 9: 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 Do we consider this as a Ranger bug? This probably doesn't cause issues for most components that use Ranger, but also doesn't sound logical. http://gerrit.cloudera.org:8080/#/c/16524/1//COMMIT_MSG@30 PS1, Line 30: Furthermore, we also revise all the checks : for the generated AuthzAuditEvent's due to the evaluation of column : masking policies so that a failed check would not fail the query but : only result in an entry in the log file. I disagree with the removal of the checks - I think that we should return an error and fail the query if the audit system works in an unexpected way, as this can lead to catching real bugs in Ranger. http://gerrit.cloudera.org:8080/#/c/16524/1/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/16524/1/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@338 PS1, Line 338: StringUtils.equalsIgnoreCase((accessResult.getMaskType()), "MASK_NONE") || : !accessResult.isMaskEnabled()) This seems redundant to me, as isMaskEnabled() is false if mask type is MASK_NONE, see https://github.com/apache/ranger/blob/master/agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerAccessResult.java#L296 http://gerrit.cloudera.org:8080/#/c/16524/1/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@346 PS1, Line 346: auditHandler.getAuthzEvents() We could call remove on auditEvents instead of auditHandler.getAuthzEvents(). http://gerrit.cloudera.org:8080/#/c/16524/1/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@347 PS1, Line 347: if (auditEvents.size() - numAuthzAuditEventsBefore > 1) { : LOG.error("More than one AuthzAuditEvent is added to 'auditHandler'."); This actually means more than 2, right, as we have just removed the last element of auditEvents? -- To view, visit http://gerrit.cloudera.org:8080/16524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dbf65874003523b5176680e42f26fa2114c229b Gerrit-Change-Number: 16524 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Mon, 05 Oct 2020 20:50:34 +0000 Gerrit-HasComments: Yes
