You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org> on 2017/06/02 19:03:08 UTC

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Dimitris Tsirogiannis has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7064

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the full runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if a full or a limited runtime profile
(or execution summary) will be returned.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has full profile access:
  Full profile is returned
- User A runs statement S, A asks for profile, A doesn't have full profile access:
  Limited profile is returned
- User A runs statement S, user B asks for profile:
  Limited profile is returned.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/ImpalaInternalService.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M tests/authorization/test_authorization.py
11 files changed, 281 insertions(+), 37 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Alex Behm, Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7064

to look at the new patch set (#12).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
17 files changed, 284 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/12
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has uploaded a new patch set (#3).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/ImpalaInternalService.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSet.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
16 files changed, 299 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 11: Code-Review+2

Fix minor issue that caused clang-tidy to complain. Keep Dan's +2 and restart gvo.

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 13:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/715/

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Hello Alex Behm,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7064

to look at the new patch set (#8).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
17 files changed, 265 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.

Lars Volker has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 2:

(2 comments)

I noticed that the clang-tidy job in the previous GVO run had failed. I added a comment at the line that tripped it.

http://gerrit.cloudera.org:8080/#/c/7064/10/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 35: #include <sstream>
is this still needed?


PS10, Line 151: const
clang-tidy seems to trip over this.


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 7:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 218:       const ClientRequestState& request_state);
why can't this be a regular method on 'this'?
also, function comment.


Line 327:   bool user_has_profile_access_;
comment what exactly this means (like you have in QueryStateRecord)


http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:

PS7, Line 355: GetSessionState(session_id, &session)
you should just pass '&session' as the second param to WithSession to get this.


PS7, Line 375: GetSessionState(session_id, &session)
you should just pass '&session' as the second param to WithSession to get this.


http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 638:       const QueryStateRecord& query_record);
same, why not regular method?
also add function comment.


http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/util/auth-util.h
File be/src/util/auth-util.h:

Line 45: Status CheckProfileAccess(const std::string& user, const std::string& effective_user,
this needs documentation, including what the parameters mean. i.e. not obvious from decl what 'user' really is. and really, aren't they both "effective users"? one associated with the current session and one with the original query?


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.

Alex Behm has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 218:       const ClientRequestState& request_state);
> The real logic is already consolidated in the common CheckProfileAccess() f
Agree. Your solution is simpler, let's do that.


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has uploaded a new patch set (#6).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/ImpalaInternalService.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
16 files changed, 272 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/6
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.

Alex Behm has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 6: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7064/6/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS6, Line 637: friend Status CheckProfileAccess(const std::string& user,
For my understanding, why do we need this?


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

Line 162:   private boolean enablePrivChecks_ = true;
> That was my initial intention but the way it is used in PartitionSet.java m
Got it, that's what I thought. We can reconsider the PartitionSet case later.


http://gerrit.cloudera.org:8080/#/c/7064/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 153:   // If set, when privilege requests are registered they will use this error
This only applies to masked priv requests right? Might want to clarify and/or even rename the var.


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 4:

Missed the comments in the test file. Sending a new patch in a while.

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Alex Behm, Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7064

to look at the new patch set (#10).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
17 files changed, 267 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/10
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 218:       const ClientRequestState& request_state);
> We need two implementations of CheckProfileAccess(), one that works against
The real logic is already consolidated in the common CheckProfileAccess() function that takes both users and bool. These friend functions just pull out  So how about we just get rid of these two friend functions and call directly? They don't seem to really add much (there are only two callsites each and from the same file) and it seems clearer anyway to see where the two user values come.

If you really want to keep the wrappers, why do they need to be 'friend'? Isn't the state they access public?

The real problem here is that we have two classes to represent the same info. Down the road we should get rid of QueryStateRecord.


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 7:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 218:       const ClientRequestState& request_state);
> Agree. Your solution is simpler, let's do that.
Done


Line 327:   bool user_has_profile_access_;
> comment what exactly this means (like you have in QueryStateRecord)
Done


http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:

PS7, Line 355: GetSessionState(session_id, &session)
> you should just pass '&session' as the second param to WithSession to get t
Done


PS7, Line 375: GetSessionState(session_id, &session)
> you should just pass '&session' as the second param to WithSession to get t
Done


http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 638:       const QueryStateRecord& query_record);
> Same comment as above.
Removed


http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/util/auth-util.h
File be/src/util/auth-util.h:

Line 45: Status CheckProfileAccess(const std::string& user, const std::string& effective_user,
> this needs documentation, including what the parameters mean. i.e. not obvi
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 12: Verified-1

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/712/

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7064/8/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

PS8, Line 323: or
> nit: and
Done


http://gerrit.cloudera.org:8080/#/c/7064/8/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS8, Line 570: .
> ... and execution summary.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 11:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/710/

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 13: Code-Review+2

Fix another clang-tidy madness...

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Hello Alex Behm, Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7064

to look at the new patch set (#9).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
17 files changed, 266 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/9
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 7:

(2 comments)

I am just answering two questions that Dan had. Let me know which approach you guys prefer.

http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 218:       const ClientRequestState& request_state);
> why can't this be a regular method on 'this'?
That was my initial implementation. Alex suggested we move all the CheckProfileAccess functions in auth-util.[h|cc].


http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 638:       const QueryStateRecord& query_record);
> same, why not regular method?
Same comment as above.


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 9: Verified-1

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/705/

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 6:

(1 comment)

Found a bug. Plz wait until I post a new patch before start reviewing it.

http://gerrit.cloudera.org:8080/#/c/7064/6/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS6, Line 637: friend Status CheckProfileAccess(const std::string& user,
> For my understanding, why do we need this?
Because QueryStateRecord is private in ImpalaServer. In order for CheckProfileAccess function to access it it needs to be his friend :)


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 9:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/705/

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has uploaded a new patch set (#4).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/ImpalaInternalService.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
16 files changed, 257 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has uploaded a new patch set (#2).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the full runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if a full or a limited runtime profile
(or execution summary) will be returned.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has full profile access:
  Full profile is returned
- User A runs statement S, A asks for profile, A doesn't have full profile access:
  Limited profile is returned
- User A runs statement S, user B asks for profile:
  Limited profile is returned.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/ImpalaInternalService.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSet.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
16 files changed, 299 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7064/3/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 151:   const bool user_has_profile_access() const {
> single line if it fits
Done


Line 222:   /// error. If base64_encoded is true, outputs the base64-encoded profile output,
> This API is confusing to me. Why does base64_encoded influence the auth beh
Removed.


http://gerrit.cloudera.org:8080/#/c/7064/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 606:       return request_state->GetRuntimeProfile(user, base64_encoded, output);
> We not have many different GetRuntimeProfile*() and GetExecSummary() functi
Added the CheckProfileAccess calls and removed the Get* functions.


http://gerrit.cloudera.org:8080/#/c/7064/3/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 560:     std::string limited_profile_str;
> Remove?
Done


http://gerrit.cloudera.org:8080/#/c/7064/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 2535:   public void registerPrivReq(PrivilegeRequest privReq) {
> As discussed, I think this logic can be further simplified and made more ex
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.

Alex Behm has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 7: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 12:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/712/

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 11: Verified-1

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/710/

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7064/1/tests/authorization/test_authorization.py
File tests/authorization/test_authorization.py:

Line 92:   def test_access_runtime_profile(self):
> Nice test!
Done


Line 135:     execute_statement_req.statement = """create view if not exists tpch.customer_view as
> Can we put this into a different db? Might be good to make sure this view i
We need a db in which we have access to the tables for the positive test. Open to recommendations if you can think of a better option.


Line 137:     execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
> also test that EXPLAIN works
Done


Line 145:     # Repeat as a deleted user
> delegated
Done


Line 281:     """Runs statement 'stmt' and retrieves the runtime profile and exec summary. If
> remove 'statement'
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has uploaded a new patch set (#5).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/ImpalaInternalService.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
16 files changed, 265 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Alex Behm, Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7064

to look at the new patch set (#11).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
17 files changed, 266 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/11
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 8: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7064/8/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

PS8, Line 323: or
nit: and


http://gerrit.cloudera.org:8080/#/c/7064/8/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS8, Line 570: .
... and execution summary.


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 1:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/7064/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

Line 66: static const string LIMITED_PROFILE_INFO_STRINGS[] = {"Session ID", "Session Type",
> Will this show the timeline?
Removed.


http://gerrit.cloudera.org:8080/#/c/7064/1/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 195:   const std::string limited_profile_str() const;
> Why const std::string?
Removed


http://gerrit.cloudera.org:8080/#/c/7064/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 386:     const std::string& GetEffectiveUser() {
> Consider changing this function to
Done


Line 480:   /// If the user asking for this profile is the same user that run the query
> .. that runs the query ...
Done


Line 489:   /// query profile. Otherwise, this function returns an empty exec summary.
> Seems better to return an auth warning string instead of an empty string.
Both this and the GetRuntimeProfileStr now return an error code if user is not authorized to access the profile.


http://gerrit.cloudera.org:8080/#/c/7064/1/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 362:   // profile.
> give an example why this may be the case
Done


http://gerrit.cloudera.org:8080/#/c/7064/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

Line 489:     // Check any masked requests. If the masked requests have an associated error message,
> Check all masked requests. If a masked request has an associated...
Done


Line 492:     // These checks don't result in an Authorization exception but set the
> AuthorizationException
Done


Line 494:     //
> extra line
Done


http://gerrit.cloudera.org:8080/#/c/7064/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 2533:     if (!enablePrivChecks_) {
> This new logic and naming could use a little cleanup. How about we rename e
Done


Line 2534:       globalState_.maskedPrivilegeReqs.add(Pair.create(privReq, ""));
> I'd prefer null instead of an empty string. Seems clearer what it means.
Done


Line 2538:     if (Strings.isNullOrEmpty(authErrorMsg_)) {
> Change this to authErrorMsg_ != null? Treating the empty string and null th
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 10: Code-Review+2

Fixed minor test issue, keep Dan's +2

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.

Alex Behm has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 3:

(10 comments)

Some ideas to discuss / think through.

http://gerrit.cloudera.org:8080/#/c/7064/3/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 151:   const bool user_has_profile_access() const {
single line if it fits


Line 222:   /// error. If base64_encoded is true, outputs the base64-encoded profile output,
This API is confusing to me. Why does base64_encoded influence the auth behavior? Also an empty user will always be authorized right?


http://gerrit.cloudera.org:8080/#/c/7064/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 606:       return request_state->GetRuntimeProfile(user, base64_encoded, output);
We not have many different GetRuntimeProfile*() and GetExecSummary() functions. I'm wondering if we can do the auth check more directly here, for example, with a function

Status CheckProfileAccess(user);

The function could be implemented in both ClientRequestState and the QueryLogRecord. Apart from that, most of the existing code would not have to change.

Another alternative along these lines is to have two CheckProfileAccess() functions in auth.h. One version would take a ClientRequestState and a user, the other would take a QueryLogRecord and the user. 

Happy to discuss the idea before you try it.


http://gerrit.cloudera.org:8080/#/c/7064/3/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 560:     std::string limited_profile_str;
Remove?


http://gerrit.cloudera.org:8080/#/c/7064/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 2535:   public void registerPrivReq(PrivilegeRequest privReq) {
As discussed, I think this logic can be further simplified and made more explicit.


http://gerrit.cloudera.org:8080/#/c/7064/1/tests/authorization/test_authorization.py
File tests/authorization/test_authorization.py:

Line 92:   def test_access_runtime_profile(self):
Nice test!


Line 135:     execute_statement_req.statement = """create view if not exists tpch.customer_view as
Can we put this into a different db? Might be good to make sure this view is dropped even if there is a failure.


Line 137:     execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
also test that EXPLAIN works


Line 145:     # Repeat as a deleted user
delegated


Line 281:     """Runs statement 'stmt' and retrieves the runtime profile and exec summary. If
remove 'statement'


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 13: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 12: Code-Review+2

Fix more clang-tidy issue. Keep Dan's +2

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

Line 1094:       << "or execution summary.";
> indentation
Done


http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 220:   Status CheckProfileAccess(const std::string& user);
> const function?
removed


http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 636: Status ImpalaServer::QueryStateRecord::CheckProfileAccess(const string& user) {
> What do you think of moving this into auth-util.h like the other function s
Done


http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 639:     Status CheckProfileAccess(const std::string& user);
> const fn
removed


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

Line 162:   private boolean enablePrivChecks_ = true;
> I was hoping we could remove this now because it should have the same effec
That was my initial intention but the way it is used in PartitionSet.java made me change my mind. There, we explicitly disable priv req registration, so it felt weird to make a call to setMaskPrivChecks.


http://gerrit.cloudera.org:8080/#/c/7064/4/shell/impala_shell.py
File shell/impala_shell.py:

Line 515:       print_to_stderr(e)
> are these changes for testing?
Without this change, we would always get a "Could not retrieve summary" error (L518) instead of an auth error.


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.

Alex Behm has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7064/7/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 218:       const ClientRequestState& request_state);
> That was my initial implementation. Alex suggested we move all the CheckPro
We need two implementations of CheckProfileAccess(), one that works against ClientRequestState and one against QueryStateRecord. Moving them to auth.h allows us to consolidate the code in one place. There are other ways of achieving the same thing, of course.


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Alex Behm, Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7064

to look at the new patch set (#13).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
17 files changed, 284 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/13
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 10:

(2 comments)

Thanks for pointing these out.

http://gerrit.cloudera.org:8080/#/c/7064/10/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 35: #include <sstream>
> is this still needed?
No, removed.


PS10, Line 151: const
> clang-tidy seems to trip over this.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Reviewed-on: http://gerrit.cloudera.org:8080/7064
Reviewed-by: Dimitris Tsirogiannis <dt...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
17 files changed, 284 insertions(+), 54 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Dimitris Tsirogiannis: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 8: Code-Review+2

Rebase and keep Dan's +2

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 10:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/707/

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 1:

The description in the JIRA asks for a redacted profile. I pinged the creator of this JIRA and Greg for clarification.

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.

Alex Behm has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 1:

(12 comments)

Still trying to understand all the different paths where the profile can be fetched. Here are a few comments to start with.

http://gerrit.cloudera.org:8080/#/c/7064/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

Line 66: static const string LIMITED_PROFILE_INFO_STRINGS[] = {"Session ID", "Session Type",
Will this show the timeline?


http://gerrit.cloudera.org:8080/#/c/7064/1/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 195:   const std::string limited_profile_str() const;
Why const std::string?


http://gerrit.cloudera.org:8080/#/c/7064/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 386:     const std::string& GetEffectiveUser() {
Consider changing this function to

std::string GetEffectiveUser(SessionState);

and move it into auth-util.h/cc

That way it's obvious whether the two implementations do the same thing.


Line 480:   /// If the user asking for this profile is the same user that run the query
.. that runs the query ...


Line 489:   /// query profile. Otherwise, this function returns an empty exec summary.
Seems better to return an auth warning string instead of an empty string.


http://gerrit.cloudera.org:8080/#/c/7064/1/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 362:   // profile.
give an example why this may be the case


http://gerrit.cloudera.org:8080/#/c/7064/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

Line 489:     // Check any masked requests. If the masked requests have an associated error message,
Check all masked requests. If a masked request has an associated...


Line 492:     // These checks don't result in an Authorization exception but set the
AuthorizationException


Line 494:     //
extra line


http://gerrit.cloudera.org:8080/#/c/7064/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 2533:     if (!enablePrivChecks_) {
This new logic and naming could use a little cleanup. How about we rename enablePrivChecks_ to maskPrivChecks_ and introduce a new Analyzer.setMaskPrivChecks(String msg) and Analyzer.unsetMaskPrivChecks() (or something similar).

That way when masking is enabled we always add a priv req to the masked list together with the user-provided 'msg' which controls whether a failure to auth that masked priv request leads to an AuthException (the 'msg' could be null of course).


Line 2534:       globalState_.maskedPrivilegeReqs.add(Pair.create(privReq, ""));
I'd prefer null instead of an empty string. Seems clearer what it means.


Line 2538:     if (Strings.isNullOrEmpty(authErrorMsg_)) {
Change this to authErrorMsg_ != null? Treating the empty string and null the same way seems error prone.


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.

Alex Behm has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 4:

(6 comments)

Thanks, looks much nicer!

http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

Line 1094:       << "or execution summary.";
indentation


http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

Line 220:   Status CheckProfileAccess(const std::string& user);
const function?


http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 636: Status ImpalaServer::QueryStateRecord::CheckProfileAccess(const string& user) {
What do you think of moving this into auth-util.h like the other function so it's obvious that both implementations do the same thing?

We could even consolidate the code into a single function CheckProfileAccess() that takes all 3 params, i.e. we have

CheckProfileAccess(user, QueryStateRecord);
CheckProfileAccess(user, ClientRequestState);
CheckProfileAccess(user, effective_user, has_access);

I don't fee too strongly, but seems nice to have the auth logic and error message only once.


http://gerrit.cloudera.org:8080/#/c/7064/4/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 639:     Status CheckProfileAccess(const std::string& user);
const fn


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

Line 162:   private boolean enablePrivChecks_ = true;
I was hoping we could remove this now because it should have the same effect as setMaskPrivChecks(null). Any reason we need to keep it?


http://gerrit.cloudera.org:8080/#/c/7064/4/shell/impala_shell.py
File shell/impala_shell.py:

Line 515:       print_to_stderr(e)
are these changes for testing?


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 9: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7064/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 153:   // If set, when privilege requests are registered they will use this error
> This only applies to masked priv requests right? Might want to clarify and/
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.

Hello Alex Behm,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7064

to look at the new patch set (#7).

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................

IMPALA-4965: Authorize access to runtime profile and exec summary

Bug:
When Sentry-based authorization is enabled, a user that isn't authorized
to EXPLAIN a statement that uses a view can still access unauthorized
information, such as view's definition, by running the statement and
asking for the query profile or the execution summary.

Fix:
During query compilation, determine if the user can access the the runtime
profile or the execution summary. Upon request for a runtime profile or
execution summary from a user, determine based on that information and
the user that is asking for the profile if the runtime profile
(or execution summary) will be returned or an authorization error.

The authorization rule enforced is the following:
- User A runs statement S, A asks for profile, A has profile access:
  Runtime profile is returned
- User A runs statement S, A asks for profile, A doesn't have profile access:
  Authorization error
- User A runs statement S, user B asks for profile:
  Authorization error.

This patch doesn't enforce access to the runtime profile or execution summary
through the Web UI.

Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_shell.py
M tests/authorization/test_authorization.py
17 files changed, 279 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/7064/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 1:

> The description in the JIRA asks for a redacted profile. I pinged
 > the creator of this JIRA and Greg for clarification.

The redacted profile looks a bit useless, maybe the jira author wasn't aware of that.

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary
......................................................................


Patch Set 1:

Alex and I just discussed this: the simplest approach would be to refuse access to the profile altogether in this situation (rather than introducing a limited profile - not clear how useful that is anyway).

Did a PM specifically ask for a limited profile in this particular case? If not, do you have any objections to switching off profile access?

-- 
To view, visit http://gerrit.cloudera.org:8080/7064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No