[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
Fredy Wijaya has abandoned this change. ( http://gerrit.cloudera.org:8080/10966 ) Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege .. Abandoned Abandon for now. Will work on it again later. -- To view, visit http://gerrit.cloudera.org:8080/10966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 Gerrit-Change-Number: 10966 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10966 ) Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege .. Patch Set 4: > Patch Set 4: > > (4 comments) > > My comments are mainly related to the DELETE t1 FROM t1, t2 WHERE ... case. > t1 should need ALL privileges in this case and must be a Kudu table, while t2 > should only need SELECT on the referred columns and can be any kind of table. Thanks for the review! Your comment makes sense. I agree that only the target table needs to have ALL privilege and not other tables in the from clause. This problem will be more apparent once we have a DELETE privilege since the existing code will require both DELETE and SELECT privileges on the target table because the existing code always requires SELECT on all tables in the from clause. I'll need to spend more time on this. -- To view, visit http://gerrit.cloudera.org:8080/10966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 Gerrit-Change-Number: 10966 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 31 Jul 2018 18:34:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/10966 )
Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
..
Patch Set 4:
(4 comments)
My comments are mainly related to the DELETE t1 FROM t1, t2 WHERE ... case. t1
should need ALL privileges in this case and must be a Kudu table, while t2
should only need SELECT on the referred columns and can be any kind of table.
http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
File fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java:
http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@128
PS4, Line 128: fromClause_.analyze(analyzer, Privilege.ALL);
I think that only the target table needs Privilege.ALL, other tables are ok
with Privilege.SELECT (even select privilege for the referred columns should be
enough).
http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@154
PS4, Line 154: targetTableRef_
The ALL privilege for the target table could be added here.
http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java:
http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@423
PS4, Line 423: new TAccessEvent("functional_kudu.alltypes",
TCatalogObjectType.TABLE, "ALL"),
Do we really need ALL privileges for functional_kudu.alltypes? This table is
only used in the from + where clause, but it will not be modified the delete.
http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:
http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1924
PS4, Line 1924: .error(accessError("functional_kudu.alltypes"),
onDatabase("functional_kudu",
: allExcept(TPrivilegeLevel.ALL)))
: .error(accessError("functional_kudu.alltypes"),
onTable(
: "functional_kudu", "alltypes",
allExcept(TPrivilegeLevel.ALL)));
nit: I think that similar tests could be a bit more readable by trying to break
lines at specific points, for example before a new onDatabase/onTable calls.
--
To view, visit http://gerrit.cloudera.org:8080/10966
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739
Gerrit-Change-Number: 10966
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Tue, 31 Jul 2018 12:37:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10966 ) Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/57/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/10966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 Gerrit-Change-Number: 10966 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 25 Jul 2018 19:01:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
Fredy Wijaya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10966 ) Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege .. IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege UPDATE and DELETE statements require ALL privilege on the target table. In the prior code, UPDATE and DELETE statements use the default FROM clause which requires SELECT privilege on the target table. This causes an issue where if a user executes an UPDATE/DELETE statement with only a SELECT privilege on SERVER or DATABASE, an AnalysisException may be thrown instead of an AuthorizationException, which may reveal potentially sensitive information. This patch fixes the issue by requiring the FROM clause to also require ALL privilege on the target table to be consistent with the UPDATE/DELETE authorization privilege requirement. Testing: - Updated authorization tests - Ran all FE tests Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/FromClause.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/TableRef.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java 6 files changed, 74 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/10966/4 -- To view, visit http://gerrit.cloudera.org:8080/10966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 Gerrit-Change-Number: 10966 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10966 ) Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege .. Patch Set 4: Code-Review+1 Rebased and carry Adam's +1 -- To view, visit http://gerrit.cloudera.org:8080/10966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 Gerrit-Change-Number: 10966 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 25 Jul 2018 18:26:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10966 ) Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege .. Patch Set 4: Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/57/ Running initial code review checks. This is experimental - please report any issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317 -- To view, visit http://gerrit.cloudera.org:8080/10966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 Gerrit-Change-Number: 10966 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 25 Jul 2018 18:26:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10966 ) Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/10966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 Gerrit-Change-Number: 10966 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 19 Jul 2018 19:42:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10966 Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege .. IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege UPDATE and DELETE statements require ALL privilege on the target table. In the prior code, UPDATE and DELETE statements use the default FROM clause which requires SELECT privilege on the target table. This causes an issue where if a user executes an UPDATE/DELETE statement with only a SELECT privilege on SERVER or DATABASE, an AnalysisException may be thrown instead of an AuthorizationException, which may reveal potentially sensitive information. This patch fixes the issue by requiring the FROM clause to also require ALL privilege on the target table to be consistent with the UPDATE/DELETE authorization privilege requirement. Testing: - Updated authorization tests - Ran all FE tests Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/FromClause.java M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java M fe/src/main/java/org/apache/impala/analysis/TableRef.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java 6 files changed, 74 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/10966/3 -- To view, visit http://gerrit.cloudera.org:8080/10966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I69d451f727a7df6c41166a15cf1ed6f5334dc739 Gerrit-Change-Number: 10966 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya
