You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2019/02/15 20:05:55 UTC
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12500
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
[sentry] add privilege scope validation to SentryAuthzProvider
Currently, SentryAuthzProvider performs authorization based on the
following rules:
a privilege implies another when,
1. the authorizable from the former implies the authorizable from the
latter,
2. the action from the former implies the action from the latter,
3. and grant option from the former implies the grant option from the
latter.
We rely on Sentry API list_sentry_privileges_by_user_and_itsgroups to
enforce rule (1). This works fine when validating authorizables with
lower privilege scope on the hierarchy, but not for authorizables with
higher scope.
For example, user Alice is granted with 'ALTER ON TABLE default.a'. She
tries to alter table 'default.a' (this requires 'ALTER ON TABLE' privilege),
During authorization, list_sentry_privileges_by_user_and_itsgroups will
return any privileges that matches:
'server == server1 && (db == default || db == NULL) && (table == a || table == NULL)'.
This can correctly filter the applied authorizables. But when Alice tries
to create table 'default.b' (this requires 'CREATE ON DATABASE' privilege),
the sentry API will return anything that matches:
'server == server1 && (db == default || db == NULL)',
which means the table level privilege will also be returned.
This patch adds privilge scope validation to SentryAuthzProvider to
ensure only authorizable with a higher privilege scope on the hierarchy
can imply authorizables with a lower scope on the hierarchy.
Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
---
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
A src/kudu/sentry/sentry_privilege_scope-test.cc
A src/kudu/sentry/sentry_privilege_scope.cc
A src/kudu/sentry/sentry_privilege_scope.h
10 files changed, 510 insertions(+), 101 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/12500/1
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12500
to look at the new patch set (#4).
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
[sentry] add privilege scope validation to SentryAuthzProvider
Currently, SentryAuthzProvider::Authorize() performs authorization base
on the following rules,
a privilege implies another when:
1. the authorizable from the former implies the authorizable from the
latter, and
2. the action from the former implies the action from the latter, and
3. grant option from the former implies the grant option from the latter.
We fetch from Sentry the privileges for the given user that might imply
the given authorizable and action (via list_sentry_privileges_by_user_and_itsgroups
Sentry API). We then proceed to check the implication rules on the
returned actions. However, we rely on the API to enforce the implication
rules of the returned authorizables. This ignored the fact that the API
returns all privileges granted to the user that match the authorizable
of each scope in the input authorizable hierarchy (not just the implied
authorizables).
So when Alice tries to create table 'default.b' (this requires 'CREATE
ON DATABASE'), the Sentry API will return anything that matches:
server == "server1" && (db == "default" || db == NULL)
which may include 'ALL ON default.a'. Previously we would only take into
consideration the fact that 'ALL' is returned, and proceed to incorrectly
permit the create operation.
This patch adds privilege scope validation to SentryAuthzProvider to
ensure only authorizable with a higher scope on the hierarchy can imply
authorizables with a lower scope on the hierarchy.
Theoretically speaking, an alternative is to add authorizable scope filering
in Sentry server. However, list_sentry_privileges_by_user_and_itsgroups
API also needs to work with the default Sentry client policy validation
(org.apache.sentry.policy.common.CommonPrivilege), which supports wildcard
authorizable matching (e.g. authorizable 'server=server1->db=*' can imply
authorizable 'server=server1'). In such case, authorizable scope filering
is not appropriate. On the other hand, wildcard authorizable matching is
dropped in Kudu (see the comment in SentryAuthzProvider::Authorize()).
Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
---
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action.h
A src/kudu/sentry/sentry_authorizable_scope-test.cc
A src/kudu/sentry/sentry_authorizable_scope.cc
A src/kudu/sentry/sentry_authorizable_scope.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.h
11 files changed, 508 insertions(+), 107 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/12500/4
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 2:
(6 comments)
Took a quick look at this. I'm not sure I understand the essence of the Sentry scoping, it seems I need to revisit this once more.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@12
PS2, Line 12: a privilege implies another when,
: 1. the authorizable from the former implies the authorizable from the
: latter,
: 2. the action from the former implies the action from the latter,
: 3. and grant option from the former implies the grant option from the
: latter.
Is there a document about that? A link to Sentry's docs would be helpful.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@33
PS2, Line 33: sentry API will return anything that matches
> Understanding this requires an understanding of the Sentry API, what it ret
+1
So, in this case the list of returned privileges will contain all ALTER, DROP and RENAME even if we are not interested in that?
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@39
PS2, Line 39: This patch adds privilge scope validation to SentryAuthzProvider to
: ensure only authorizable with a higher privilege scope on the hierarchy
: can imply authorizables with a lower scope on the hierarchy.
What would be the problem if not having this patch? Could you elaborate on details? An example would be great.
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/master/sentry_authz_provider-test.cc@425
PS2, Line 425: AuthzOptions {
: true,
: SentryPrivilegeScope::Scope::TABLE,
: },
: AuthzOptions {
: false,
: SentryPrivilegeScope::Scope::TABLE,
: },
: AuthzOptions {
: true,
: SentryPrivilegeScope::Scope::DATABASE,
: },
: AuthzOptions {
: false,
: SentryPrivilegeScope::Scope::DATABASE,
: },
: AuthzOptions {
: true,
: SentryPrivilegeScope::Scope::SERVER,
: },
: AuthzOptions {
: false,
: SentryPrivilegeScope::Scope::SERVER,
: }
Did you want just the Cartesian product? If so, maybe use separate parameters and use ::testing::Combine(); there is an example in https://github.com/apache/kudu/blob/2be008763cda5eb28c0dfa8863e396575fce91ab/src/kudu/tools/kudu-admin-test.cc#L406
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.h
File src/kudu/sentry/sentry_privilege_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.h@58
PS2, Line 58: imply
implies
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.cc
File src/kudu/sentry/sentry_privilege_scope.cc:
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.cc@42
PS2, Line 42: return "<cannot reach here>";
maybe:
LOG(FATAL) << static_cast<uint16_t>(scope) << ": unknown scope";
return nullptr;
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 19 Feb 2019 18:24:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 7:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@394
PS7, Line 394: TSentryPrivilege column_priv = GetColumnPrivilege(db, tbl, col, action);
: TSentryPrivilege table_priv = GetTablePrivilege(db, tbl, action);
: TSentryPrivilege db_priv = GetDatabasePrivilege(db, action);
: TSentryPrivilege server_priv = GetServerPrivilege(action);
nit: add 'const' if the test isn't intended to update these
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@442
PS7, Line 442: Status s = sentry_authz_provider_->Authorize(scope, SentryAction::Action::ALL,
: Substitute("$0.$1", db, tbl), kTestUser);
With this verification, is it guaranteed that for other type of actions (e.g., Action::METADATA) are not authorized for higher scopes? If looking at this as a black box, I'd think of adding some coverage for that if it's not guaranteed; otherwise, please a comment explaining why other types of actions are guaranteed to be not authorized in this scenario.
Or this test is not supposed to cover that at all?
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@451
PS7, Line 451: // Scope::COLUMN is excluded since column scope for table
: // authorizable doesn't make sense.
I thought adding Scope::COLUMN would automatically skip that check, but that would add coverage for the higher scopes, no?
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@265
PS7, Line 265: required_grant_option
nit: requires_grant_option or grant_option_required ?
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@279
PS7, Line 279: authorizable 'server=server1->db=*' can imply authorizable 'server=server1'
> Wait, that sounds odd. Logically, that should not be the case when it's po
OK, it seems to be just some information on Sentry. And those widening implications are not supported, as described.
However, as for another example, I would not expect that ALL on all columns like 'server=S->db=D->table=T->column=*' is implying 'server=S->db=D->table=T'. For example, that would be strange that ability to do whatever is possible at column level implies the ability to rename or drop the table itself.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@313
PS7, Line 313: WARN_NOT_OK(s, Substitute("failed to construct sentry action from $0",
: privilege.action));
nit: the 'if (!s.ok()) {}' scope is present below, so maybe move this under the 'if' scope below and translate into simply LOG(WARN) << ...
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@321
PS7, Line 321: WARN_NOT_OK(s, Substitute("failed to construct sentry privilege scope from $0",
> Nit: for clarity via symmetry with L315, could you break out the s.ok() che
+1
Also, please move the 'WARN_NOT_OK' stuff under the 'if (!s.ok())', converting it into simple 'LOG(WARN) << ...'
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@335
PS7, Line 335: ERROR
Unrelated to this particular changelist, but since you are here: do you think it's justifiable to log is as ERROR? I would expect this to be INFO or at most WARN since no error has happened here.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h@58
PS7, Line 58: static Status FromString(const std::string& str, SentryAuthorizableScope* scope)
nit: add WARN_UNUSED_RESULT
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 12 Mar 2019 22:02:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12500
to look at the new patch set (#7).
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
[sentry] add privilege scope validation to SentryAuthzProvider
Currently, SentryAuthzProvider::Authorize() performs authorization based
on the rules that a privilege implies another when:
1. the authorizable from the former implies the authorizable from the
latter, and
2. the action from the former implies the action from the latter, and
3. grant option from the former implies the grant option from the latter.
We fetch from Sentry the privileges for the given user that might imply
the given authorizable and action (via list_sentry_privileges_by_user_and_itsgroups
Sentry API). We then proceed to check the implication rules on the
returned actions. However, we rely on the API to enforce the implication
rules of the returned authorizables. This ignored the fact that the API
returns all privileges granted to the user that match the authorizable
of each scope in the input authorizable hierarchy (not just the implied
authorizables).
So when Alice tries to create table 'default.b' (this requires 'CREATE
ON DATABASE'), the Sentry API will return anything that matches:
server == "server1" && (db == "default" || db == NULL)
which may include 'ALL ON default.a'. Previously we would only take into
consideration the fact that 'ALL' is returned, and proceed to incorrectly
permit the create operation.
This patch adds privilege scope validation to SentryAuthzProvider to
ensure only authorizable with a higher scope on the hierarchy can imply
authorizables with a lower scope on the hierarchy.
Theoretically speaking, an alternative is to add authorizable scope
filtering in Sentry server. However, such an API is inherently a Kudu-only
API, which by necessity would filter out authorizable wildcard matching
(that Sentry's default client policy validation supports). Moreover,
client-side filtering can also help enabling more aggressive caching.
Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
---
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
A src/kudu/sentry/sentry_authorizable_scope-test.cc
A src/kudu/sentry/sentry_authorizable_scope.cc
A src/kudu/sentry/sentry_authorizable_scope.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.h
12 files changed, 588 insertions(+), 131 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/12500/7
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 2:
(3 comments)
Like Andrew and Alexey, I'm also kind of perplexed by this, though it's probably because I don't know enough about Sentry. It's unclear from the commit message why this is a problem that needs to be solved within Kudu and not within Sentry.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@12
PS2, Line 12: a privilege implies another when,
Nit: if all of these conditions must be met, consider rephrasing as:
a privilege implies another when:
1. ..., and
2. ..., and
3. ...
That makes it more clear that ALL conditions are required rather than ANY condition.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@24
PS2, Line 24: with
Nit: drop 'with'
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@39
PS2, Line 39: privilge
privilege
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 19 Feb 2019 23:40:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30
PS7, Line 30: 'ALL ON default.a'
> In addition to "default", we also include "server1". Does this mean that ev
Ah, I think I confused myself thinking about this. Just to be sure, in this example, some potentially returned privileges if we provide the database scope are:
<action> ON server1
<action> ON server1.default
<action> ON server1.default.a
<action> ON server1.default.a.col0
<action> ON server1.default.a.col1
<action> ON server1.default.b
<action> ON server1.default.c
<action> ON server1.default.c.col0
<action> ON server1.default.c.col1
<action> ON server1.default.c.col2
<action> ON server1.default.c.col3
and had we passed to Sentry the table scope, we may have gotten privileges like:
<action> ON server1
<action> ON server1.default
<action> ON server1.default.a
<action> ON server1.default.a.col0
<action> ON server1.default.a.col1
Somewhat unrelated to this patch, but if this is the case, it seems like the lower the scope we provide, the less we get back from Sentry. If that's the case, I wonder if it makes sense to send the lowest-level scope to Sentry when evaluating a higher-level scope requirement, and since privileges of higher-level scopes are returned from Sentry, we should be able to evaluate them. That might be something to consider if we find that responses from Sentry consume a large amount of memory.
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 13 Mar 2019 07:49:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 7: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@92
PS6, Line 92: const std::string& user,
> Done
Nit: there's a subtle difference between 'required_grant_option' and 'require_grant_option': the former suggests that the "grant option" is always required, but that its value should be either true or false as dictated by the parameter, while the latter more clearly (IMHO) indicates that a grant option is either required or it is not.
Put another way, "require_grant_option" more clearly maps to the English sentence "do we require a GRANT OPTION?" while "required_grant_option" is more ambiguous, and it wouldn't be unheard of for it to map to something like "the required grant option should have a value of false".
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@176
PS7, Line 176: DCHECK(scope != SentryAuthorizableScope::Scope::COLUMN);
Use DCHECK_NE instead.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@321
PS7, Line 321: WARN_NOT_OK(s, Substitute("failed to construct sentry privilege scope from $0",
Nit: for clarity via symmetry with L315, could you break out the s.ok() check from L325 and make a separate "if not ok, continue" check here?
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 12 Mar 2019 18:05:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 8: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30
PS7, Line 30: 'ALL ON default.a'
> In terms of minimizing the number of calls to Sentry, I think the "optimal"
I think this is something we need to be open to experimenting with. Once there's a cache, how aggressively we "prefetch" from Sentry will be largely dictated by the semantics of the cache: can we use it for authz requests belonging to a different scope? Is it only for requests on matching scope? What's the cache memory footprint as compared to the amount of coverage it provides?
So whatever we do right now is largely immaterial as we'll need to verify that it makes sense in the presence of the cache.
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Mar 2019 21:20:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
[sentry] add privilege scope validation to SentryAuthzProvider
Currently, SentryAuthzProvider::Authorize() performs authorization based
on the rules that a privilege implies another when:
1. the authorizable from the former implies the authorizable from the
latter, and
2. the action from the former implies the action from the latter, and
3. grant option from the former implies the grant option from the latter.
We fetch from Sentry the privileges for the given user that might imply
the given authorizable and action (via list_sentry_privileges_by_user_and_itsgroups
Sentry API). We then proceed to check the implication rules on the
returned actions. However, we rely on the API to enforce the implication
rules of the returned authorizables. This ignored the fact that the API
returns all privileges granted to the user that match the authorizable
of each scope in the input authorizable hierarchy (not just the implied
authorizables).
So when Alice tries to create table 'default.b' (this requires 'CREATE
ON DATABASE'), the Sentry API will return anything that matches:
server == "server1" && (db == "default" || db == NULL)
which may include 'ALL ON default.a'. Previously we would only take into
consideration the fact that 'ALL' is returned, and proceed to incorrectly
permit the create operation.
This patch adds privilege scope validation to SentryAuthzProvider to
ensure only authorizable with a higher scope on the hierarchy can imply
authorizables with a lower scope on the hierarchy.
Theoretically speaking, an alternative is to add authorizable scope
filtering in Sentry server. However, such an API is inherently a Kudu-only
API, which by necessity would filter out authorizable wildcard matching
(that Sentry's default client policy validation supports). Moreover,
client-side filtering can also help enabling more aggressive caching.
Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Reviewed-on: http://gerrit.cloudera.org:8080/12500
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
A src/kudu/sentry/sentry_authorizable_scope-test.cc
A src/kudu/sentry/sentry_authorizable_scope.cc
A src/kudu/sentry/sentry_authorizable_scope.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.h
12 files changed, 653 insertions(+), 157 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, but someone else must approve
Andrew Wong: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30
PS7, Line 30: 'ALL ON default.a'
> Right, for example 'ALL on SERVER1' or 'CREATE on SERVER1' will be returned
In terms of minimizing the number of calls to Sentry, I think the "optimal" usage pattern would be to send the highest scope possible. Since we already know the server, database, and table associated with every request, if we take this idea to its extreme, we could ask for everything in server1. The complexity would then lie in how we get useful information out of those responses -- do we parse the huge response once, and separate the privileges out into some useful in-memory hierarchy? Maybe, but it seems tricky.
Another approach would be to always send requests at the table scope. AFAICT, every request authorized by the master is associated with a table anyway, so it seems like this would be a natural fit. It reduces the amount of stuff returned by Sentry, and I think it would simplify how we could think about caching as well, since _every_ request would send the same scope to Sentry (including what we need to create authz tokens). WDYT? Also curious if Alexey agrees, since he's been working on this caching stuff.
My main concern with sending over the highest possible scope (and more broadly, using the database scope and above at all) is that I'm pretty sure there are cases where Kudu tables will be in some default database. The default database could have a huge amount of irrelevant entries in the response -- that actually seems like it'd be a pretty common case.
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Mar 2019 07:10:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 8: Code-Review+2
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30
PS7, Line 30: 'ALL ON default.a'
> Yeah, I don't mean to bring this up here because I expect it to change the
I agree that we should be open to exploring different options here wrt responses of which scope we do store in the cache. It might be a configurable parameter, I think: the lowest scope we use to send request to sentry. The 'table' scope seems to be a reasonable default. However, caching information for the higher than requested levels would complicate queuing requests for the related entities, but I think it's manageable (I think we could use some sort of the 'tokens' idea that we use in thread pools elsewhere, if I'm not confusing that with something else).
As for the choice between storing just the raw Sentry responses or some sort of structures to be able to search through more effectively, I'm thinking we should opt for the latter.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@442
PS7, Line 442: Status s = sentry_authz_provider_->Authorize(scope, SentryAction::Action::ALL,
: Substitute("$0.$1", db, tbl), kTestUser);
> This test is not to cover that but for verifying authorizable scope filter
SGTM. Thanks for the clarification.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@451
PS7, Line 451: // Scope::COLUMN is excluded since column scope for table
: // authorizable doesn't make sense.
> Not quite following, if COLUMN scope is passed to Authorize(), there is a D
Ah, I missed that, whoops. Indeed.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@265
PS7, Line 265: yAuthorizableScope::S
> Adar suggests require_grant_option, so I am going to go with that.
SGTM
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@279
PS7, Line 279: 3. grant option from the former implies the grant option from the latter.
> I agree it is strange for cases all on column level implies rename or drop
All right, that's how Sentry works and we cannot do much about that. At least, we don't support that wildcard implications for this integration, so we don't need to worry about that :)
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h@49
PS7, Line 49:
> The default constructor is useful for FromString(const std::string& str, Se
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Mar 2019 23:16:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@9
PS4, Line 9: base
based
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@16
PS4, Line 16: 3. grant option from the former implies the grant option from the latter.
What are these, and how do they fit into the overall Sentry authorization model? I'm using https://docs.google.com/document/d/1SEBtgWwBFqij5CuCZwhOqDNSDVViC0WERq6RzsPCWjQ/edit#heading=h.p3aqzzmumh8m as a guide and it only talks about authorizables and actions.
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@36
PS4, Line 36: This patch adds privilege scope validation to SentryAuthzProvider to
: ensure only authorizable with a higher scope on the hierarchy can imply
: authorizables with a lower scope on the hierarchy.
I don't claim to understand this problem well, but here's my mental model of the situation.
Implication works on two axes: authorizables, and actions. There's even a third axis if you include grant options, though I don't understand them well enough to know whether they affect the authorization hierarchy. Surprisingly (to me at least), Kudu actually has two different authz use cases: DDLs, and authz token creation. They're different in that each one needs a slightly different set of privileges from Sentry:
DDLs
When authorizing a DDL, Kudu needs to know whether a user has enough privileges to perform a single logical action. Examples include "CREATE TABLE db.foo", or "ALTER TABLE db.foo RENAME TO db.bar". In every case, the DDL is permitted if there exists a Sentry privilege that directly maps to the action+authorizable. It is also permitted if there exist Sentry privileges that imply the action+authorizable. For example, if I am trying to do "CREATE TABLE db.foo" and I don't have "CREATE ON DATABASE db" but I do have "CREATE ON SERVER server1" (or I have "ALL ON DATABASE db"), then I'm allowed to create this table.
Authz token creation
When creating an authz token for a user, Kudu needs _all of the privileges that may affect the user's interaction with a table_. This is subtly different than the DDL case, because although it includes the direct mapping case as well as the "implication from higher up in the action or authorizable hierarchies" case, it also includes a third case: discrete privileges on actions/authorizables that lie _below_ in the implication hierarchy. For example, suppose I request an authz token on db.foo and the only privileges matching my name are "METADATA ON db.foo" and "SELECT ON COLUMN db.foo.col1". This is enough to build an authz token that allows me to scan "col1" out of "db.foo", but not enough to do any kind of destructive DDL on the table.
Why do these distinctions matter? Because they map to slightly different requests from Sentry:
1. In English, the DDL case maps to "tell me (yes/no) whether user U may perform action AC against authorizable AU, and please factor in any privileges that may exist in the higher levels of the various implication hierarchies to inform the decision."
2. The authz token creation case maps to "give me all of the privileges for user U that affect authorizable AU (where AU is some table). This includes privileges derived via implication from both above and below in the implication hierarchies."
It sounds like list_sentry_privileges_by_user_and_itsgroups corresponds to #2 from above, and this patch implements some client-side logic to map the results into #1. Why can't we directly implement #1 as a new query API in Sentry? You make the case against that below but I don't understand it; I don't see how wildcard authorizables factor into the discussion.
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@40
PS4, Line 40: filering
filtering
Below too
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 05 Mar 2019 20:18:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@279
PS7, Line 279: authorizable 'server=server1->db=*' can imply authorizable 'server=server1'
Wait, that sounds odd. Logically, that should not be the case when it's possible to perform some non-db-related actions on a server. For example, I would expect that actions on server might be something like 'adding a server into the cluster' or alike. That echoes the permissions we require in Impala for creating external Kudu tables, no?
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 12 Mar 2019 19:35:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12500
to look at the new patch set (#8).
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
[sentry] add privilege scope validation to SentryAuthzProvider
Currently, SentryAuthzProvider::Authorize() performs authorization based
on the rules that a privilege implies another when:
1. the authorizable from the former implies the authorizable from the
latter, and
2. the action from the former implies the action from the latter, and
3. grant option from the former implies the grant option from the latter.
We fetch from Sentry the privileges for the given user that might imply
the given authorizable and action (via list_sentry_privileges_by_user_and_itsgroups
Sentry API). We then proceed to check the implication rules on the
returned actions. However, we rely on the API to enforce the implication
rules of the returned authorizables. This ignored the fact that the API
returns all privileges granted to the user that match the authorizable
of each scope in the input authorizable hierarchy (not just the implied
authorizables).
So when Alice tries to create table 'default.b' (this requires 'CREATE
ON DATABASE'), the Sentry API will return anything that matches:
server == "server1" && (db == "default" || db == NULL)
which may include 'ALL ON default.a'. Previously we would only take into
consideration the fact that 'ALL' is returned, and proceed to incorrectly
permit the create operation.
This patch adds privilege scope validation to SentryAuthzProvider to
ensure only authorizable with a higher scope on the hierarchy can imply
authorizables with a lower scope on the hierarchy.
Theoretically speaking, an alternative is to add authorizable scope
filtering in Sentry server. However, such an API is inherently a Kudu-only
API, which by necessity would filter out authorizable wildcard matching
(that Sentry's default client policy validation supports). Moreover,
client-side filtering can also help enabling more aggressive caching.
Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
---
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
A src/kudu/sentry/sentry_authorizable_scope-test.cc
A src/kudu/sentry/sentry_authorizable_scope.cc
A src/kudu/sentry/sentry_authorizable_scope.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.h
12 files changed, 653 insertions(+), 157 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/12500/8
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 5:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@9
PS4, Line 9: base
> based
Done
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@10
PS4, Line 10: on the following rules,
:
: a privilege implies another when:
> nit: merge the lines as "based on the rules that a privilege implies anothe
Done
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@16
PS4, Line 16: 3. grant option from the former implies the grant option from the latter.
> What are these, and how do they fit into the overall Sentry authorization m
The design doc briefly touched the grant option when talking about the required privilege for 'CreateTable'. In short, grant option is allowing a user to grant the privilege to others.
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@40
PS4, Line 40: filering
> filtering
Done
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider-test.cc@349
PS4, Line 349: TestPrivilegeScope
> AuthorizableScope
Done
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider-test.cc@381
PS4, Line 381: default
> Why don't we need to add column privileges for Scope::TABLE?
Done
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.h@85
PS4, Line 85: sentry::SentryAuthorizableScope::Scope scope,
: sentry::SentryAction::Action action,
: const std::string& table_name,
: const std::string& user,
: bool grant_option = false);
> Document the fields. E.g. how is 'scope' used? What if 'scope' isn't TABLE?
Done
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/master/sentry_authz_provider.cc@284
PS3, Line 284: list
> nit: lists
Done
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.cc@175
PS4, Line 175: case SentryAuthorizableScope::Scope::COLUMN:
> The authorizable doesn't have any table info in it for a column? Can you ad
Done
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope-test.cc
File src/kudu/sentry/sentry_authorizable_scope-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope-test.cc@59
PS4, Line 59: TEST(SentryAuthorizableScopeTest, TestFromString) {
: SentryAuthorizableScope column;
: ASSERT_OK(SentryAuthorizableScope::FromString("column", &column));
> Should this test the other scopes too?
Done
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope.h@61
PS4, Line 61: a
> an
Done
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 07 Mar 2019 10:03:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12500
to look at the new patch set (#3).
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
[sentry] add privilege scope validation to SentryAuthzProvider
Currently, SentryAuthzProvider performs authorization based on the
following rules,
a privilege implies another when:
1. the authorizable from the former implies the authorizable from the
latter, and
2. the action from the former implies the action from the latter, and
3. grant option from the former implies the grant option from the latter.
We relied on Sentry API list_sentry_privileges_by_user_and_itsgroups to
enforce rule (1), which can result in privilege escalation (see the
comment in SentryAuthzProvider::Authorize()).
This patch adds privilege scope validation to SentryAuthzProvider to
ensure only authorizable with a higher privilege scope on the hierarchy
can imply authorizables with a lower scope on the hierarchy.
Theoretically speaking, an alternative is to add privilege scope filering
in Sentry server. However, list_sentry_privileges_by_user_and_itsgroups
API also needs to work with the default Sentry client policy validation
(org.apache.sentry.policy.common.CommonPrivilege), which supports wildcard
privilege matching (e.g. authorizable 'server=server1->db=*' can imply
authorizable 'server=server1'). In such case, privilege scope filering
is not appropriate. On the other hand, wildcard privilege matching is
dropped in SentryAuthzProvider (see the comment in
SentryAuthzProvider::Authorize()).
Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
---
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.h
A src/kudu/sentry/sentry_privilege_scope-test.cc
A src/kudu/sentry/sentry_privilege_scope.cc
A src/kudu/sentry/sentry_privilege_scope.h
11 files changed, 506 insertions(+), 108 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/12500/3
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@90
PS6, Line 90: const std::string& table_ident,
> AFAICT this is just a table name; could we name it table_name to be less co
Ah, I suggested this change since I recall there being a concept of a table_ident that included the table name and database name. In the context of these functions, those combined comprise the table name. From Kudu's POV table_name is more straight-forward because that's what it is; in the context of Sentry, it might help to know that table_name is expected to include the database name and whatnot. Perhaps a comment would suffice?
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@178
PS6, Line 178: return Status::InvalidArgument("column authorizable scope is not supported "
: "for table authorizable type");
This is a contract with developers, not clients -- it'd be a bug if we got here, so perhaps a CHECK is more appropriate?
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 09 Mar 2019 06:33:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 3:
(4 comments)
Took a brief pass, posting to make sure I understand the issue, and in case others find it helpful. Will continue to look more in depth.
http://gerrit.cloudera.org:8080/#/c/12500/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/3//COMMIT_MSG@18
PS3, Line 18: We relied on Sentry API list_sentry_privileges_by_user_and_itsgroups to
: enforce rule (1), which can result in privilege escalation (see the
: comment in SentryAuthzProvider::Authorize()).
I think it's worth describing the bug in the commit message a bit, and then remove some of the details of this bug from the impl. LMK if any of this is incorrect, and feel free to fix it if so:
Previously, when authorizing a request, Kudu would determine the authorizable and action required for that request. Kudu would then call SentryAuthzProvider::Authorize() to fetch from Sentry the privileges for the given user that might imply the given authorizable and action. We would then proceed to check the implication rules on the returned actions, and ignore the implication rules of the returned authorizables. This ignored the fact that Sentry's list_sentry_privileges_by_user_and_itsgroups API returns all privileges granted to the user that match the action and any authorizable of an equal or lower place in the input authorizable hierarchy (not just the input authorizable itself).
So when Alice tries to create table 'default.b' (this requires 'CREATE ON DATABASE'), the Sentry API will return anything that matches:
server == "server1" && (db == "default" || db == NULL)
which many include 'ALL ON default.a'. Previously we would only take into consideration the fact that 'ALL' is returned, and proceed to incorrectly permit the create operation.
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/master/sentry_authz_provider.cc@277
PS3, Line 277: wildcard privilege matching
We still have wildcard checking for actions though, right? Maybe specify "authorizables privilege matching".
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/master/sentry_authz_provider.cc@284
PS3, Line 284: list
nit: lists
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/sentry/sentry_privilege_scope.h
File src/kudu/sentry/sentry_privilege_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/sentry/sentry_privilege_scope.h@28
PS3, Line 28: // A carbon copy of Sentry privilege scope, which indicates the
: // hierarchy of authorizables (server → database → table → column).
: //
: // A privilege scope can imply another following rules defined in
: // Implies().
Would probably help to explain what the scope is scoping? I.e. a given privilege will have a scope that specifies the scope of authorizables that it is valid for, IIUC?
Also why isn't this named AuthorizableScope, like it was before?
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 22 Feb 2019 19:40:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@10
PS4, Line 10: on the following rules,
:
: a privilege implies another when:
nit: merge the lines as "based on the rules that a privilege implies another when:"
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider-test.cc@349
PS4, Line 349: TestPrivilegeScope
AuthorizableScope
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider-test.cc@381
PS4, Line 381: default
Why don't we need to add column privileges for Scope::TABLE?
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.h@85
PS4, Line 85: sentry::SentryAuthorizableScope::Scope scope,
: sentry::SentryAction::Action action,
: const std::string& table_name,
: const std::string& user,
: bool grant_option = false);
Document the fields. E.g. how is 'scope' used? What if 'scope' isn't TABLE? e.g. COLUMN? How is grant_option used?
After looking elsewhere, I think "table_name" might be more descriptive as "table_ident" since that implies it also has to do with the database?
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.cc@175
PS4, Line 175: case SentryAuthorizableScope::Scope::COLUMN:
The authorizable doesn't have any table info in it for a column? Can you add a comment explaining why this is a no-op?
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope-test.cc
File src/kudu/sentry/sentry_authorizable_scope-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope-test.cc@59
PS4, Line 59: TEST(SentryAuthorizableScopeTest, TestFromString) {
: SentryAuthorizableScope column;
: ASSERT_OK(SentryAuthorizableScope::FromString("column", &column));
Should this test the other scopes too?
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope.h@61
PS4, Line 61: a
an
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 06 Mar 2019 01:27:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12500
to look at the new patch set (#6).
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
[sentry] add privilege scope validation to SentryAuthzProvider
Currently, SentryAuthzProvider::Authorize() performs authorization based
on the rules that a privilege implies another when:
1. the authorizable from the former implies the authorizable from the
latter, and
2. the action from the former implies the action from the latter, and
3. grant option from the former implies the grant option from the latter.
We fetch from Sentry the privileges for the given user that might imply
the given authorizable and action (via list_sentry_privileges_by_user_and_itsgroups
Sentry API). We then proceed to check the implication rules on the
returned actions. However, we rely on the API to enforce the implication
rules of the returned authorizables. This ignored the fact that the API
returns all privileges granted to the user that match the authorizable
of each scope in the input authorizable hierarchy (not just the implied
authorizables).
So when Alice tries to create table 'default.b' (this requires 'CREATE
ON DATABASE'), the Sentry API will return anything that matches:
server == "server1" && (db == "default" || db == NULL)
which may include 'ALL ON default.a'. Previously we would only take into
consideration the fact that 'ALL' is returned, and proceed to incorrectly
permit the create operation.
This patch adds privilege scope validation to SentryAuthzProvider to
ensure only authorizable with a higher scope on the hierarchy can imply
authorizables with a lower scope on the hierarchy.
Theoretically speaking, an alternative is to add authorizable scope
filtering in Sentry server. However, such an API is inherently a Kudu-only
API, which by necessity would filter out authorizable wildcard matching
(that Sentry's default client policy validation supports). Moreover,
client-side filtering can also help enabling more aggressive caching.
Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
---
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/sentry/CMakeLists.txt
M src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action.h
A src/kudu/sentry/sentry_authorizable_scope-test.cc
A src/kudu/sentry/sentry_authorizable_scope.cc
A src/kudu/sentry/sentry_authorizable_scope.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.h
11 files changed, 537 insertions(+), 107 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/12500/6
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12500/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/1//COMMIT_MSG@29
PS1, Line 29: 'server == server1 && (db == default || db == NULL) && (table == a || table == NULL)'.
nit: this was weird to read at first. Perhaps:
server == "server1" && (db == "default" || db == NULL) && (table == "a" || table == NULL)
Same below.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@29
PS2, Line 29: 'server == server1 && (db == default || db == NULL) && (table == a || table == NULL)'.
To clarify, this is being evaluated on the Sentry server? What is this filtering statement being run on?
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@33
PS2, Line 33: sentry API will return anything that matches
Understanding this requires an understanding of the Sentry API, what it returns, to whom, and how the returned stuff is used. Could you include these details in this commit message? It's pretty difficult for me to understand.
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 19 Feb 2019 05:38:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 6:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@137
PS6, Line 137: const TSentryGrantOption::type& grant_option) {
May want to consider defaulting to ::DISABLED here (and in the other get privilege methods) since that's the case so often.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@363
PS6, Line 363: TEST_P(TestAuthzHierarchy, TestAuthorizableScope) {
Do you want this coverage for the "need grant option" case too?
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@378
PS6, Line 378: case SentryAuthorizableScope::Scope::TABLE:
Shouldn't there be an entry for COLUMN?
Oh, we can't do GetAuthorizable() for COLUMN. So why is COLUMN in the list of tested authorizable scopes at all?
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@397
PS6, Line 397: break;
Shouldn't this be FALLTHROUGH_INTENDED?
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@412
PS6, Line 412: false));
This is the default value. On L423 too.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@413
PS6, Line 413: ASSERT_OK(DropRole(kRoleName));
If you used slightly different role names (and user names I guess), you wouldn't need to drop roles and the test may be a bit faster (by eliminating a few RPCs). Maybe not worth it though.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@430
PS6, Line 430: ::testing::Values(true,
: false)
Use ::testing::Bool() here.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@90
PS6, Line 90: const std::string& table_ident,
AFAICT this is just a table name; could we name it table_name to be less confusing?
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@92
PS6, Line 92: bool grant_option = false);
This might be clearer as "require_grant_option" or "need_grant_option".
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@282
PS6, Line 282: Semantics
Nit: semantics
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@283
PS6, Line 283: is
Nit: are
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@314
PS6, Line 314: WARN_NOT_OK(s, Substitute("failed to construct sentry action from $0",
: privilege.action));
If we just WARN here (and below), won't the Implies() calls crash? Seems like we should WARN and either fail to authorize, or WARN and ignore the privilege.
Oh, I see there's an s.ok() check on L324. That'll ensure that SentryAuthorizableScope::FromString passed, but not SentryAction::FromString, whose status was since overwritten.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@334
PS6, Line 334: LOG(ERROR) << "Action <" << action << "> on authorizable <"
: << authorizable << "> with authorizable scope <"
: << scope << "> is not permitted for user <"
: << user << ">";
Nit: would be clearer using strings::Substitute.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.h@57
PS6, Line 57: a
Nit: an
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.cc
File src/kudu/sentry/sentry_authorizable_scope.cc:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.cc@36
PS6, Line 36: const char* ScopeToString(SentryAuthorizableScope::Scope scope) {
Nit: could you make this consistent w.r.t. ActionToString? I'm referring to the LOG(FATAL) and the return of nullptr here.
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 08 Mar 2019 00:49:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 8: Code-Review+2
(1 comment)
LGTM, but will leave this open in case Alexey has further comments.
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30
PS7, Line 30: 'ALL ON default.a'
> I think this is something we need to be open to experimenting with. Once th
Yeah, I don't mean to bring this up here because I expect it to change the behavior of this patch, and so the placement of this discussion isn't quite appropriate. It's more just fallout from my developing of this thought upon re-reading this commit message.
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Mar 2019 21:42:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30
PS7, Line 30: 'ALL ON default.a'
In addition to "default", we also include "server1". Does this mean that everything in "server1" that the user has privileges on will be returned?
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 12 Mar 2019 20:46:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 7:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@137
PS6, Line 137: const string& action,
> May want to consider defaulting to ::DISABLED here (and in the other get pr
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@363
PS6, Line 363: Status s = sentry_authz_provider_->AuthorizeCreateTable("DB.table", kTestUser, kTestUser);
> Do you want this coverage for the "need grant option" case too?
I don't think this test needs to cover grant option as well, which is independently validated from authorizable scope. Though it should happen in the test for table creation with a different owner than the user.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@378
PS6, Line 378: public ::testing::WithParamInterface<
> Shouldn't there be an entry for COLUMN?
Hmm, right. I was thinking to add more coverage, but on a second thought this is really not add anything.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@397
PS6, Line 397: TSentryPrivilege server_priv = GetServerPrivilege(action);
> Shouldn't this be FALLTHROUGH_INTENDED?
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@412
PS6, Line 412:
> This is the default value. On L423 too.
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@413
PS6, Line 413: switch (scope) {
> If you used slightly different role names (and user names I guess), you wou
Yeah, I thought about this, but mini sentry uses the fixed predefined user-group mapping https://github.com/apache/kudu/blob/master/src/kudu/sentry/mini_sentry.cc#L294. So yes, I don't think it worth it.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@430
PS6, Line 430: eAndAddToGroups(kRoleName, kUserGroup));
: ASSERT_OK(AlterRoleGrantPrivilege(kRoleName
> Use ::testing::Bool() here.
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@90
PS6, Line 90: sentry::SentryAction::Action action,
> AFAICT this is just a table name; could we name it table_name to be less co
Yeah, I will add a comment as Andrew suggested.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@90
PS6, Line 90: sentry::SentryAction::Action action,
> Ah, I suggested this change since I recall there being a concept of a table
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@92
PS6, Line 92: const std::string& user,
> This might be clearer as "require_grant_option" or "need_grant_option".
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@178
PS6, Line 178: case SentryAuthorizableScope::Scope::TABLE:
: RETURN_NOT_OK(ParseHiveTableIdentifier(table_name, &database,
> This is a contract with developers, not clients -- it'd be a bug if we got
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@282
PS6, Line 282: Therefor
> Nit: semantics
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@283
PS6, Line 283: in
> Nit: are
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@314
PS6, Line 314: privilege.action));
: if (!s.ok()) {
> If we just WARN here (and below), won't the Implies() calls crash? Seems li
Ah right, thanks for catching that. Added a regression test.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@334
PS6, Line 334: // e.g. 'whether table A exists'.
: LOG(ERROR) << Substitute("Action <$0> on table <$1> with authorizable scope "
: "<$2> is not permitted for user <$3>",
: s
> Nit: would be clearer using strings::Substitute.
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.h@57
PS6, Line 57: a
> Nit: an
Done
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.cc
File src/kudu/sentry/sentry_authorizable_scope.cc:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.cc@36
PS6, Line 36: const char* ScopeToString(SentryAuthorizableScope::Scope scope) {
> Nit: could you make this consistent w.r.t. ActionToString? I'm referring to
Done
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 12 Mar 2019 06:37:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 7:
(28 comments)
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30
PS7, Line 30: 'ALL ON default.a'
> In addition to "default", we also include "server1". Does this mean that ev
Right, for example 'ALL on SERVER1' or 'CREATE on SERVER1' will be returned. It will return 'all privileges granted to the user that match the authorizable of each scope in the input authorizable hierarchy'. And 'server1' is in the input authorizable hierarchy.
http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30
PS7, Line 30: 'ALL ON default.a'
> Ah, I think I confused myself thinking about this. Just to be sure, in this
That's right.
I think that depends on how the cache is used. If we are able to map a cache entry with higher-level scope to a lower-level scope request, then the higher-level scope request can be useful since it has higher cache coverage, and we don't need to cache lower-level scope request any more(which contains partial duplicated information).
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@394
PS7, Line 394: TSentryPrivilege column_priv = GetColumnPrivilege(db, tbl, col, action);
: TSentryPrivilege table_priv = GetTablePrivilege(db, tbl, action);
: TSentryPrivilege db_priv = GetDatabasePrivilege(db, action);
: TSentryPrivilege server_priv = GetServerPrivilege(action);
> nit: add 'const' if the test isn't intended to update these
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@442
PS7, Line 442: Status s = sentry_authz_provider_->Authorize(scope, SentryAction::Action::ALL,
: Substitute("$0.$1", db, tbl), kTestUser);
> With this verification, is it guaranteed that for other type of actions (e.
This test is not to cover that but for verifying authorizable scope filter is behave as expect independent of which action is used. Action verification should be a different dimension. The combination of authorizable and action verification should be tested in TestAuthorizeXXX series.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@451
PS7, Line 451: // Scope::COLUMN is excluded since column scope for table
: // authorizable doesn't make sense.
> I thought adding Scope::COLUMN would automatically skip that check, but tha
Not quite following, if COLUMN scope is passed to Authorize(), there is a DCHECK to disallow that (https://gerrit.cloudera.org/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@176), so I don't think how Scope::COLUMN will be automatically skipped.
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@92
PS6, Line 92: const std::string& user,
> Nit: there's a subtle difference between 'required_grant_option' and 'requi
Makes sense, updated.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@176
PS7, Line 176: DCHECK(scope != SentryAuthorizableScope::Scope::COLUMN);
> Use DCHECK_NE instead.
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@186
PS7, Line 186: authorizable->__set_db(database.ToString());
> What if 'database' is empty at this point? If that should not happen, add
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@192
PS7, Line 192: LOG(FATAL) << "unsupported SentryAuthorizableScope";
> nit: consider outputting the 'scope' parameter here as well -- might be use
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@211
PS7, Line 211: user == owner
> Might be not the part of this changelist, but just to clarify. Are usernam
Yes, usernames are case sensitive in Sentry APIs, so hence they are case sensitive in all APIs here.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@265
PS7, Line 265: required_grant_option
> nit: requires_grant_option or grant_option_required ?
Adar suggests require_grant_option, so I am going to go with that.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@279
PS7, Line 279: authorizable 'server=server1->db=*' can imply authorizable 'server=server1'
> OK, it seems to be just some information on Sentry. And those widening imp
I agree it is strange for cases all on column level implies rename or drop the table. However for better or for worse, the default Sentry policy evaluation indeed considers 'server=S->db=D->table=T->column=*' is implying 'server=S->db=D->table=T' (defined in org.apache.sentry.policy.common.CommonPrivilege). And this 'wildcard authorizable matching' is dropped in Kudu's implementation of policy evaluation.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@313
PS7, Line 313: WARN_NOT_OK(s, Substitute("failed to construct sentry action from $0",
: privilege.action));
> nit: the 'if (!s.ok()) {}' scope is present below, so maybe move this under
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@321
PS7, Line 321: WARN_NOT_OK(s, Substitute("failed to construct sentry privilege scope from $0",
> Nit: for clarity via symmetry with L315, could you break out the s.ok() che
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@321
PS7, Line 321: WARN_NOT_OK(s, Substitute("failed to construct sentry privilege scope from $0",
> +1
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@335
PS7, Line 335: ERROR
> Unrelated to this particular changelist, but since you are here: do you thi
Hmm, I agree we should use WARNING instead of ERROR as this is not something should be logged to standard error.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry-test-base.h
File src/kudu/sentry/sentry-test-base.h:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry-test-base.h@82
PS7, Line 82: virtual bool KerberosEnabled() = 0;
> nit: add 'const' ?
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_action.cc
File src/kudu/sentry/sentry_action.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_action.cc@20
PS7, Line 20: #include <stdint.h>
> nit: use <cstdint> instead
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc
File src/kudu/sentry/sentry_authorizable_scope-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@61
PS7, Line 61: SentryAuthorizableScope scope;
: for (const auto& valid_scope : valid_scopes) {
: ASSERT_OK(SentryAuthorizableScope::FromString(valid_scope, &scope));
> Does it make sense to check for the result scope? From the black-box testi
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@66
PS7, Line 66: throws
> returns
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@68
PS7, Line 68: "UNINITIALIZED"
> maybe, add 'tablecolumn' into the list of invalid scopes to verify SentryAu
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h@49
PS7, Line 49: SentryAuthorizableScope();
> Could you add a comment justifying the existence of this constructor? Idea
The default constructor is useful for FromString(const std::string& str, SentryAuthorizableScope* scope). And the UNINITIALIZED enum is added so a default constructor could exist without having undefined behavior. There is a similar discussion at https://gerrit.cloudera.org/#/c/11720/2/src/kudu/sentry/sentry_action.h@66
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h@58
PS7, Line 58: static Status FromString(const std::string& str, SentryAuthorizableScope* scope)
> nit: add WARN_UNUSED_RESULT
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc
File src/kudu/sentry/sentry_authorizable_scope.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@20
PS7, Line 20: #include <stdint.h>
> nit: use #include <cstdint> instead
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@39
PS7, Line 39: "SERVER"
> a tiny nit for this and other string literals that are shared between this
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@96
PS7, Line 96: }
> nit: I guess gcc would report a warning here because the function returns n
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client-test.cc@56
PS7, Line 56: bool KerberosEnabled() {
> nit: add 'const' ?
Done
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client.h
File src/kudu/sentry/sentry_client.h:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client.h@99
PS7, Line 99: // If the user is granted both 'ALL on SERVER server1' and 'SELECT on TABLE db1.a'
> Does that mean that if there is a request for a particular column C in a ta
Right, if user is granted with ALL on Server 's' and the request is for a particular column C in table 'A', it will return whole contents that match in each privilege scope on the hierarchy of 'server=s->db=db->table=a->column=u', and ALL on Server 's' privilege will be returned as well.
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Mar 2019 06:51:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 2:
(12 comments)
> (3 comments)
>
> Like Andrew and Alexey, I'm also kind of perplexed by this, though
> it's probably because I don't know enough about Sentry. It's
> unclear from the commit message why this is a problem that needs to
> be solved within Kudu and not within Sentry.
Added more information in the commit message to clarify the motivation. Hope it helps.
http://gerrit.cloudera.org:8080/#/c/12500/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/1//COMMIT_MSG@29
PS1, Line 29: 'server == server1 && (db == default || db == NULL) && (table == a || table == NULL)'.
> nit: this was weird to read at first. Perhaps:
Done
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@12
PS2, Line 12: a privilege implies another when,
> Nit: if all of these conditions must be met, consider rephrasing as:
Done
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@12
PS2, Line 12: a privilege implies another when,
: 1. the authorizable from the former implies the authorizable from the
: latter,
: 2. the action from the former implies the action from the latter,
: 3. and grant option from the former implies the grant option from the
: latter.
> Is there a document about that? A link to Sentry's docs would be helpful.
This is documented in the sentry_authz_provider.cc. I will add a reference to the Sentry java client implementation.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@24
PS2, Line 24: with
> Nit: drop 'with'
Done
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@29
PS2, Line 29: 'server == server1 && (db == default || db == NULL) && (table == a || table == NULL)'.
> To clarify, this is being evaluated on the Sentry server? What is this filt
Yeah, this is evaluated on the Sentry server and it ran against all privileges that were granted to the user.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@33
PS2, Line 33: sentry API will return anything that matches
> +1
Action is a part of privilege, so all returned privileges will include it. And yes, as long as these privileges match the filtering, they are returned. My understanding of why action is not part of the filtering is that action has its own implication rule which is hard to filter on the server side.
Note that RENAME is not an action defined in Kudu.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@33
PS2, Line 33: sentry API will return anything that matches
> Understanding this requires an understanding of the Sentry API, what it ret
I actually moved this information into the comment for SentryClient::ListPrivilegesByUser(), where the API is defined. Let me know if it is still not clear to you.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@39
PS2, Line 39: privilge
> privilege
Done
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@39
PS2, Line 39: This patch adds privilge scope validation to SentryAuthzProvider to
: ensure only authorizable with a higher privilege scope on the hierarchy
: can imply authorizables with a lower scope on the hierarchy.
> What would be the problem if not having this patch? Could you elaborate on
Hmm, I am trying to use the example above to elaborate. Will rephrase it to be more clear.
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/master/sentry_authz_provider-test.cc@425
PS2, Line 425: AuthzOptions {
: true,
: SentryPrivilegeScope::Scope::TABLE,
: },
: AuthzOptions {
: false,
: SentryPrivilegeScope::Scope::TABLE,
: },
: AuthzOptions {
: true,
: SentryPrivilegeScope::Scope::DATABASE,
: },
: AuthzOptions {
: false,
: SentryPrivilegeScope::Scope::DATABASE,
: },
: AuthzOptions {
: true,
: SentryPrivilegeScope::Scope::SERVER,
: },
: AuthzOptions {
: false,
: SentryPrivilegeScope::Scope::SERVER,
: }
> Did you want just the Cartesian product? If so, maybe use separate paramet
Done
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.h
File src/kudu/sentry/sentry_privilege_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.h@58
PS2, Line 58: imply
> implies
Done
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.cc
File src/kudu/sentry/sentry_privilege_scope.cc:
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.cc@42
PS2, Line 42: return "<cannot reach here>";
> maybe:
Done
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 22 Feb 2019 06:01:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@36
PS4, Line 36: This patch adds privilege scope validation to SentryAuthzProvider to
: ensure only authorizable with a higher scope on the hierarchy can imply
: authorizables with a lower scope on the hierarchy.
> It sounds like list_sentry_privileges_by_user_and_itsgroups corresponds to #2 from above, and this patch implements some client-side logic to map the results into #1. Why can't we directly implement #1 as a new query API in Sentry? You make the case against that below but I don't understand it; I don't see how wildcard authorizables factor into the discussion.
Hao and I discussed this offline; I'm reproducing our discussion here for posterity.
Wildcard authorizable matching is a weird beast in that it doesn't have any effect on the SQL authorization model. Meaning, for Kudu, Impala, and other SQL-like systems, "server1.db1.*" is exactly the same as "server1.db1". But, it's in the Sentry authorization model and so it needs to be accounted for when making Sentry server-side changes. Hao's point is that a new Sentry API that filters out privileges whose scope is "below" that of the requested action or authorizable is inherently a Kudu-only API, because by necessity such an API would filter out wildcards as well. That doesn't mean we shouldn't do it, just that it'd never be a general purpose API.
All that said, our discussion revealed another reason to do this work client-side rather than server-side: enabling more aggressive caching. Imagine that we're caching privileges by username (i.e. a user to vector<privilege> cache). An AlterTable DDL using the existing list_sentry_privileges_by_user_and_itsgroups API will populate the cache with "extra" privileges not pertinent to CreateTable (e.g. "SELECT on server1.db1.foo.col1"). However, those privileges will be relevant when an authz token is constructed for that user, and the extra caching could save us a Sentry call at that point. While not common, such use cases are real, and a new Sentry API that was more selective in its returned privileges would prevent us from eagerly populating the cache.
That, coupled with the fact that the new proposed Sentry API would be Kudu-only by nature, makes this client-side work more attractive in my mind.
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 05 Mar 2019 23:25:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 7:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@186
PS7, Line 186: authorizable->__set_db(database.ToString());
What if 'database' is empty at this point? If that should not happen, add corresponding DCHECK here.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@192
PS7, Line 192: LOG(FATAL) << "unsupported SentryAuthorizableScope";
nit: consider outputting the 'scope' parameter here as well -- might be useful for troubleshooting if things go awry and this FATAL is hit.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@211
PS7, Line 211: user == owner
Might be not the part of this changelist, but just to clarify. Are usernames case-sensitive in all this API?
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry-test-base.h
File src/kudu/sentry/sentry-test-base.h:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry-test-base.h@82
PS7, Line 82: virtual bool KerberosEnabled() = 0;
nit: add 'const' ?
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_action.cc
File src/kudu/sentry/sentry_action.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_action.cc@20
PS7, Line 20: #include <stdint.h>
nit: use <cstdint> instead
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc
File src/kudu/sentry/sentry_authorizable_scope-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@61
PS7, Line 61: SentryAuthorizableScope scope;
: for (const auto& valid_scope : valid_scopes) {
: ASSERT_OK(SentryAuthorizableScope::FromString(valid_scope, &scope));
Does it make sense to check for the result scope? From the black-box testing perspective nothing guarantees that "column", being a valid scope, might map into 'Scope::SERVER'.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@66
PS7, Line 66: throws
returns
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@68
PS7, Line 68: "UNINITIALIZED"
maybe, add 'tablecolumn' into the list of invalid scopes to verify SentryAuthorizableScope::FromString() behavior.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h@49
PS7, Line 49: SentryAuthorizableScope();
Could you add a comment justifying the existence of this constructor? Ideally, only the SentryAuthorizableScope(Scope scope) should be there since and it's not obvious why UNINITIALIZED even exists.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc
File src/kudu/sentry/sentry_authorizable_scope.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@20
PS7, Line 20: #include <stdint.h>
nit: use #include <cstdint> instead
Yes, IWYU is confusing with its suggestions and hopefully we can fix that with the newer version of the IWYU tool.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@39
PS7, Line 39: "SERVER"
a tiny nit for this and other string literals that are shared between this function and SentryAuthorizableScope::FromString() as well: consider declaring 'static constexpr char const kServer[] = "SERVER";' and using those constants instead.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@96
PS7, Line 96: }
nit: I guess gcc would report a warning here because the function returns nothing reaching the end of the scope. Consider adding 'return false;' as the last line of the SentryAuthorizableScope::Implies() method.
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client-test.cc@56
PS7, Line 56: bool KerberosEnabled() {
nit: add 'const' ?
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client.h
File src/kudu/sentry/sentry_client.h:
http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client.h@99
PS7, Line 99: // If the user is granted both 'ALL on SERVER server1' and 'SELECT on TABLE db1.a'
Does that mean that if there is a request for a particular column C in a table and
1) the user is granted 'GRANT ALL AT SERVER "S" TO USER "U"'
2) for some reason, somebody explicitly specified 'GRANT ALL TO COLUMN "C" OF TABLE "T"' at server "S"
then the response will always contain the _whole_ contents of the Sentry's DB up to the very top level (i.e. server "S")?
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 12 Mar 2019 19:28:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12500/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/3//COMMIT_MSG@18
PS3, Line 18: We relied on Sentry API list_sentry_privileges_by_user_and_itsgroups to
: enforce rule (1), which can result in privilege escalation (see the
: comment in SentryAuthzProvider::Authorize()).
> I think it's worth describing the bug in the commit message a bit, and then
Sounds good, I will add back the bug description based on your suggestion. The only thing that I want to point out is it is not "any authorizable of an equal or lower place in the input authorizable hierarchy". Instead it actually returns each scope/place in the authorizable hierarchy (including higher scope).
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/master/sentry_authz_provider.cc@277
PS3, Line 277: wildcard privilege matching
> We still have wildcard checking for actions though, right? Maybe specify "a
Right, will update.
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/sentry/sentry_privilege_scope.h
File src/kudu/sentry/sentry_privilege_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/sentry/sentry_privilege_scope.h@28
PS3, Line 28: // A carbon copy of Sentry privilege scope, which indicates the
: // hierarchy of authorizables (server → database → table → column).
: //
: // A privilege scope can imply another following rules defined in
: // Implies().
> Would probably help to explain what the scope is scoping? I.e. a given priv
Yeah, I think AuthorizableScope probably is more precise. I will update.
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 22 Feb 2019 20:08:25 +0000
Gerrit-HasComments: Yes