You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2018/07/17 23:59:06 UTC

[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 <fw...@cloudera.com>

[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 19:01:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
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 <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jul 2018 19:42:00 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 12:37:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 18:26:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10966 )

Change subject: IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/main/java/org/apache/impala/analysis/FromClause.java
File fe/src/main/java/org/apache/impala/analysis/FromClause.java:

http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/main/java/org/apache/impala/analysis/FromClause.java@70
PS4, Line 70: registers
clearer: Analyze this statement after registering the specified privileges.

follow-up question here: would a map of table-refs to privs be useful for cases such as the delete test? the map can be used as an override, e.g., 'privilege' is applied unless there's an override.

however, given the usage in ModifyStmt, perhaps targeting the specific table in the operation as suggested seems easier/clearer.


http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/main/java/org/apache/impala/analysis/TableRef.java
File fe/src/main/java/org/apache/impala/analysis/TableRef.java:

http://gerrit.cloudera.org:8080/#/c/10966/4/fe/src/main/java/org/apache/impala/analysis/TableRef.java@629
PS4, Line 629:   public void setPrivilege(Privilege privilege) { this.priv_ = privilege; }
allowed to be null?



-- 
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 <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Aug 2018 17:34:32 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 18:34:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 tarmstrong@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 <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 18:26:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7217: Incorrect UPDATE/DELETE authorization privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>