[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87 PS11, Line 87: sentry::SentryActionsSet granted_privileges; > Your point is valid: granted_actions suggests that actions may be "granted" Thank you for the follow-up comment! Yep, I think so as well. After clarifying on the terminology and coming to better understanding of each other and the matter behind this bike-shedding exercise, we eventually converged on 'allowed_actions' as the name for this field: 'granted_privileges' --> 'allowed_actions'. -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 23:26:08 + Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87 PS11, Line 87: sentry::SentryActionsSet granted_privileges; > This seems to be the most bike-shadeable point in this changelist :) Your point is valid: granted_actions suggests that actions may be "granted", which, as you pointed out, isn't something that makes sense within the Sentry authz model. Agreed that 'actions' would be clearer (or at least less confusing). But, I don't feel strongly about it; I know Andrew preferred to keep the current name, so please do that if it's your preference. I just wanted to make sure we had a mutual understanding of the appropriate terminology, and I think we do. -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 16:53:10 + Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 11:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@51
PS11, Line 51: AuthorizablePrivileges(sentry::SentryAuthorizableScope::Scope
scope,
> My eye is immediately drawn to the lack of a 'server' field; a comment expl
Done
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87
PS11, Line 87: sentry::SentryActionsSet granted_privileges;
> I don't feel strongly, but I also don't understand your response.
This seems to be the most bike-shadeable point in this changelist :)
Thank you for the the explanation. Yep, I think it's very important to make
sure we understand each other.
Frankly, the Sentry terminology is confusing to me. I'm more used to
Postgresql concept of privileges, and there CREATE, ALTER, etc. are privileges:
https://www.postgresql.org/docs/9.1/ddl-priv.html
https://www.postgresql.org/docs/9.0/sql-grant.html
Probably, I should get used to the Sentry terminology and move on.
However, how is it possible to grant an action? What is that? My point is
that the semantics of 'action' doesn't imply it can be granted (but a privilege
or a permission can). If so, what 'granted_actions' would mean? At least I
could not find a notion of granting an action in the user-facing documentation:
https://www.cloudera.com/documentation/enterprise/latest/topics/cm_sg_sentry_service.html
Should we then rename 'granted_privileges' --> 'actions' and be fine?
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@102
PS11, Line 102: the
: // given table.
> Yeah, as I commented in the other review, I think _encapsulating_ the enfor
Yep, that code is now in SentryPrivilegesBranch::Init(...)
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@431
PS11, Line 431: !boost::iequals(privilege.tableName,
requested_authorizable.table))) {
> > I didn't understand the 'kServer' part of your comment.
Ah, I see. I fixed the misprint: SentryAuthorizableScope::kSever -->
SentryAuthorizableScope::kServer
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 10 Apr 2019 06:47:37 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87
PS11, Line 87: sentry::SentryActionsSet granted_privileges;
> I'm not sure why privilege should imply authorizable, but action should not
I don't feel strongly, but I also don't understand your response.
My understanding of the Sentry authorization model is this:
- There are several statically defined action types (e.g. ALL, METADATA, READ,
CREATE, etc.). There's a mild hierarchy here in that ALL implies a bunch of
other actions and METADATA is implied by other actions.
- An "action" is entirely derived from an action type. This is obvious so why
bother stating it? Because I'm trying to draw a distinction with how
authorizables behave; see below.
- There are several statically defined authorizable types (e.g. SERVER,
DATABASE, TABLE, etc.). There's a hierarchy here as well.
- Unlike an action, an "authorizable" is derived from both an authorizable type
_and_ a string that names the authorizable. For example, to describe an
authorizable with type DATABASE we need a string that both names the database
as well as its parent SERVER.
- A privilege is comprised of a pair of action and authorizable.
- Privileges may be granted to users. Or roles or groups (can't quite remember
if this last part is true, but it's not really material to the argument I'm
making here).
I called out this variable name because I felt that it was conflating the nouns
of "privilege" and "action". To me, the fact that it's a set of actions
suggests that its name ought to reflect that. But ignoring that for a minute,
where did I suggest that a privilege implies an authorizable but not an action?
If anything, I said that a privilege implies the _combination_ of an action and
an authorizable.
So, to reiterate, I don't feel strongly about the name, but I do feel strongly
about making sure we're understanding one another.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@102
PS11, Line 102: the
: // given table.
> As far as I see this is enforced in SentryAuthzProvider::GetSentryPrivilege
Yeah, as I commented in the other review, I think _encapsulating_ the
enforcement into SentryPrivilegesBranch (as a constructor or something) would
further clarify the invariant.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@431
PS11, Line 431: !boost::iequals(privilege.tableName,
requested_authorizable.table))) {
> I didn't understand the 'kServer' part of your comment.
Ugh, I un-typoed the typo. SentryAuthorizableScope defines 'kSever' (note the
missing 'r').
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 09 Apr 2019 19:20:58 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 11:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
PS11:
> With all the new tests, could you double check the NUM_SHARDS value? Does i
That has been addressed to have more shards:
https://gerrit.cloudera.org/#/c/12833/10/src/kudu/master/CMakeLists.txt
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider-test.cc@439
PS11, Line 439: // Parameterized by whether Kerberos should be enabled.
> sentry_authz_provider-test extensively parameterizes with Kerberization, bu
It seems this piece hasn't been changed a lot, it's just an artifact of
diff-ing.
As for the Kerberization, I don't think it's related to the internals of Kudu
authz, but rather to the ways of Kudu trusted user (i.e. the OS user under
which kudu-master runs) to authenticate with Sentry. By my understanding, the
Kerberized parameterisation is a common pattern for the majority of test
scenarios in this file.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87
PS11, Line 87: sentry::SentryActionsSet granted_privileges;
> Might want to name this granted_actions; 'privileges' makes me think of the
I'm not sure why privilege should imply authorizable, but action should not.
If you don't feel strongly against this, I'm going to keep it as is for now.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@102
PS11, Line 102: the
: // given table.
> This was a bit confusing as there's no table argument. My takeaway (without
As far as I see this is enforced in SentryAuthzProvider::GetSentryPrivileges()
where an instance of SentryPrivilegesBranch is populated.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@371
PS11, Line 371: DCHECK_EQ(FLAGS_server_name, requested_authorizable.server);
> IIUC, invariant violations on requested_authorizable trigger DCHECKs, while
Done
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@424
PS11, Line 424: if (!privilege.__isset.columnName ||
privilege.columnName.empty()) {
> This is also just defensive.
I think having '__isset' set with an empty value in corresponding field is a
sign of messed-up TSentryPrivilege for particular context, so I think it makes
sense to validate that unless we have some guarantees from Thrift (I'm not sure
we have such guarantees from the protocol side: at least I cannot see it from
the sources where TSentryPrivilege class is defined).
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@431
PS11, Line 431: !boost::iequals(privilege.tableName,
requested_authorizable.table))) {
> Sentry authorizable names are case-insensitive? Is there a definitive refer
By my understanding Sentry authorizables are case-insensitive (databases,
tables, columns): that comes from the properties of the Kudu-HMS integration.
I added a comment on that into the code published at
https://gerrit.cloudera.org/#/c/12833/
I didn't understand the 'kServer' part of your comment.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@472
PS11, Line 472: DCHECK(!db.empty() && !table.empty());
> Conjunct DCHECKs are hard to debug when they trigger; it's never clear whic
Done
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@501
PS11, Line 501: unordered_map privileges_map;
> I think the logic from here on down might be better suited as an "initializ
I moved this into SentryPrivilegesBranch::Init() method.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@524
PS11, Line 524: privilege.all_with_grant = true;
> It should be fine to have a grant option on any action; but for Kudu we onl
I added VLOG(1) message for that.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/sentry/sentry_action.h@64
PS11, Line 64: static const size_t kMaxAction = Action::OWNER + 1;
> You might find the templates and macros in "Enumeration Casts and Tests" (g
Ack.
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-M
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@83
PS11, Line 83: // The scope of the authorizable being granted privileges.
: const sentry::SentryAuthorizableScope::Scope scope;
> Can't we figure out the scope by checking which of our strings is empty? Th
Defensive programming and avoiding some excessive branching at the cost of a
stored enum, yeah.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@424
PS11, Line 424: if (!privilege.__isset.columnName ||
privilege.columnName.empty()) {
> Why do we need to validate __isset as well as the strings themselves?
This is also just defensive.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@524
PS11, Line 524: privilege.all_with_grant = true;
> What should we do with privileges whose grant option was enabled but whose
It should be fine to have a grant option on any action; but for Kudu we only
really use it for "all with grant" or "owner with grant". In all other cases,
the grant option can be ignored, but the privilege is still valid.
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 06 Apr 2019 17:53:58 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 11: > (14 comments) > > Sorry I'm late to the party; hope this is still useful. Thank you for review. I'll try to address those comments in http://gerrit.cloudera.org:8080/12833, which is re-based atop of this patch. -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 06 Apr 2019 00:37:33 + Gerrit-HasComments: No
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@176
PS5, Line 176: for (const auto& granted_action : privilege.granted_privileges) {
: if (SentryAction(granted_action).Implies(action)) {
: return true;
: }
: }
> Honestly, using bitwise for bulk Implies seems pretty tricky, and I don't t
Ok, makes sense. We can revisit it later if it shows up.
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 05 Apr 2019 04:25:06 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 11:
(14 comments)
Sorry I'm late to the party; hope this is still useful.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/gutil/map-util.h@833
PS11, Line 833: void EmplaceValuesFromMap(MapContainer&& map_container,
Didn't know that you needed to pass a container as an rvalue reference in order
to iterate it with universal references. TIL.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
PS11:
With all the new tests, could you double check the NUM_SHARDS value? Does it
still make sense?
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider-test.cc@439
PS11, Line 439: // Parameterized by whether Kerberos should be enabled.
sentry_authz_provider-test extensively parameterizes with Kerberization, but
why is that an interesting axis across which to test? Is there any overlap
between Kerberization (or not) and how we do Sentry-based authz?
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@51
PS11, Line 51: AuthorizablePrivileges(sentry::SentryAuthorizableScope::Scope
scope,
My eye is immediately drawn to the lack of a 'server' field; a comment
explaining why that is would be helpful here.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@83
PS11, Line 83: // The scope of the authorizable being granted privileges.
: const sentry::SentryAuthorizableScope::Scope scope;
Can't we figure out the scope by checking which of our strings is empty? That
is, if we have db_name="default", table_name="foo", column_name="", we're
talking about a table scope, right?
As best I can tell, your goal here was defensive programming via sanity
checking: receive _more_ information than is absolutely necessary at
construction time, and use it for cross-validation purposes. Is that right?
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87
PS11, Line 87: sentry::SentryActionsSet granted_privileges;
Might want to name this granted_actions; 'privileges' makes me think of the
combination of action and authorizable, but this is only a set of actions.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@102
PS11, Line 102: the
: // given table.
This was a bit confusing as there's no table argument. My takeaway (without
having looked at the implementation yet) is that everything in 'privileges'
matches a single table. That is, 'privileges' includes entries like
default.foo.col1, default.foo.col2, default.foo, or default, but not entries
like non_default, non_default.bar, non_default.bar.col1, default.another_table,
or default.another_table.colx.
Is this enforced anywhere? Perhaps it should be enforced in
SentryPrivilegesBranch's constructor?
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@371
PS11, Line 371: DCHECK_EQ(FLAGS_server_name, requested_authorizable.server);
IIUC, invariant violations on requested_authorizable trigger DCHECKs, while
violations on privilege just return false. That makes sense: if we sent the
wrong thing that's on us (and a programmer error), but if we got the wrong
response we need to handle that gracefully.
Might be worth a comment though, since there's a lot of sanity checking going
on here.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@424
PS11, Line 424: if (!privilege.__isset.columnName ||
privilege.columnName.empty()) {
Why do we need to validate __isset as well as the strings themselves?
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@431
PS11, Line 431: !boost::iequals(privilege.tableName,
requested_authorizable.table))) {
Sentry authorizable names are case-insensitive? Is there a definitive reference
for that that we can link here? I guess the same is true for SentryAction and
SentryAuthorizableScope, both of which do case-insensitive comparisons.
BTW, SentryAuthorizableScope defines something called kServer; should be
kServer.
http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@472
PS11, Line 472: DCHECK(!db.empty() && !table.empty());
Conjunct DCHECKs are hard to debug when they tri
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 11: Thanks a lot for the patch - I'm going to re-base a 12833 on top of this one. -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 05 Apr 2019 01:34:27 + Gerrit-HasComments: No
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 05 Apr 2019 01:33:33 + Gerrit-HasComments: No
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. sentry: sanitize and parse privileges from Sentry Currently, we pass around the Thrift privileges received from Sentry, which can be both expensive memory-wise and cumbersome to use. This patch: - sanitizes the responses from Sentry, only keeping those that are well-formed and potentially Kudu-related, - stores them in a more ergonomic form, e.g. keeping around enums rather than strings for SentryActions, etc. This form may be updated in the future to facilitate privilege evaluation -- for now, my goal is just to make it easier to work with Sentry privileges, - encapsulates the above in an abstracted version of a Sentry response that corresponds to the hierarchy tree for a given table, with the hope that it will make changing the in-memory format more painless, - switches the SentryAuthorizableScope and SentryAction enum classes to enums, to avoid having to use the extra enum class typename everywhere (e.g. now SentryAuthorizableScope::SERVER instead of SentryAuthorizableScope::Scope::SERVER will suffice), - tests that the sanitization does what it purports to do, - tests authorizing "create tables" with OWNER privileges, due to an issue caught in review. Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Reviewed-on: http://gerrit.cloudera.org:8080/12919 Tested-by: Kudu Jenkins Reviewed-by: Hao Hao Reviewed-by: Alexey Serbin --- M src/kudu/gutil/map-util.h 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/sentry_action.h M src/kudu/sentry/sentry_authorizable_scope.h 6 files changed, 724 insertions(+), 94 deletions(-) Approvals: Kudu Jenkins: Verified Hao Hao: Looks good to me, but someone else must approve Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@176
PS5, Line 176: for (const auto& granted_action : privilege.granted_privileges) {
: if (SentryAction(granted_action).Implies(action)) {
: return true;
: }
: }
> I agree it should be a separate patch.
Honestly, using bitwise for bulk Implies seems pretty tricky, and I don't think
a series of bitwise checks will be very noticeable, so frankly, I'd prefer not
to. I don't think dwelling on this is worth the time, though if we notice it
showing up on some perf profile, or if you feel very strongly about this, then
sure.
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 05 Apr 2019 01:13:47 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 10: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@176
PS5, Line 176: for (const auto& granted_action : privilege.granted_privileges) {
: if (SentryAction(granted_action).Implies(action)) {
: return true;
: }
: }
> Maybe, but not as a part of this patch. I doubt this will be the bottleneck
I agree it should be a separate patch.
Though it can be useful when user has privileges on the same authorizable with
N kinds of fine-grained action, it will slow down the Implies() process N times
per SentryPrivilegesBranch, compare to if we can do a bit wise verification. If
you think it makes sense, can you add a TODO here?
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@513
PS5, Line 513:
> Interesting, seems like OWNER could use some more test coverage in general.
In general, the test coverage for equivalency between OWNER and ALL are covered
in sentry_action-test.cc. However, as now we have a 'hack' which link action
'ALL' with grant option, so yes, we need more test coverage where grant options
are used. Though I think it is only in CreateTable.
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 05 Apr 2019 00:58:53 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 10:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h@42
PS5, Line 42:
> nit: use
No, this is correct. I.e. "this is preferred instead of using..." or "this is
preferred over using..."
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h@73
PS5, Line 73: LOG(FATAL) << "not reachable";
: }
: #endif
> I am not sure why grant option has to work with privilege 'ALL'? I believe
Done
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h@92
PS5, Line 92: string column_na
> It would be more clear to name it as SentryPrivilegesInSameBranch or someth
A bit verbose, I went with SentryPrivilegesBranch, since it's the privileges of
a branch of the Sentry privilege tree.
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@176
PS5, Line 176: for (const auto& granted_action : privilege.granted_privileges) {
: if (SentryAction(granted_action).Implies(action)) {
: return true;
: }
: }
> As we are using FixedBitSet to store all actions granted to an authorizable
Maybe, but not as a part of this patch. I doubt this will be the bottleneck;
maybe if we begin to see a lot of this similar code around, or if we see that
this is a slow point, then sure. I'm doubtful that will be the case though.
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@362
PS5, Line 362: }
: return kColumnFields;
: }
> Should these be moved to L488 to avoid duplicated checks?
No, I want these checks to happen for every function that may use this function
(e.g. in tests when calling it directly). It wont show up in release build
anyway.
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@406
PS5, Line 406:
> nit: Granted?
Done
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@513
PS5, Line 513:
> Since OWNER is considered as equivalent to ALL, we may need to add OWNER as
Interesting, seems like OWNER could use some more test coverage in general.
http://gerrit.cloudera.org:8080/#/c/12919/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/7/src/kudu/master/sentry_authz_provider.cc@319
PS7, Line 319: static const AuthorizableScopesSet kDbFields{
SentryAuthorizableScope::TABLE,
:
SentryAuthorizableScope::COLUMN };
: static const AuthorizableScopesSet kTableFields
> This is going to be created every time this function called, no?
Done
http://gerrit.cloudera.org:8080/#/c/12919/7/src/kudu/master/sentry_authz_provider.cc@335
PS7, Line 335: return kColumnField
> Why not to return const reference here?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 04 Apr 2019 22:53:26 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12919 to look at the new patch set (#10). Change subject: sentry: sanitize and parse privileges from Sentry .. sentry: sanitize and parse privileges from Sentry Currently, we pass around the Thrift privileges received from Sentry, which can be both expensive memory-wise and cumbersome to use. This patch: - sanitizes the responses from Sentry, only keeping those that are well-formed and potentially Kudu-related, - stores them in a more ergonomic form, e.g. keeping around enums rather than strings for SentryActions, etc. This form may be updated in the future to facilitate privilege evaluation -- for now, my goal is just to make it easier to work with Sentry privileges, - encapsulates the above in an abstracted version of a Sentry response that corresponds to the hierarchy tree for a given table, with the hope that it will make changing the in-memory format more painless, - switches the SentryAuthorizableScope and SentryAction enum classes to enums, to avoid having to use the extra enum class typename everywhere (e.g. now SentryAuthorizableScope::SERVER instead of SentryAuthorizableScope::Scope::SERVER will suffice), - tests that the sanitization does what it purports to do, - tests authorizing "create tables" with OWNER privileges, due to an issue caught in review. Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b --- M src/kudu/gutil/map-util.h 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/sentry_action.h M src/kudu/sentry/sentry_authorizable_scope.h 6 files changed, 724 insertions(+), 94 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/12919/10 -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12919 to look at the new patch set (#9). Change subject: sentry: sanitize and parse privileges from Sentry .. sentry: sanitize and parse privileges from Sentry Currently, we pass around the Thrift privileges received from Sentry, which can be both expensive memory-wise and cumbersome to use. This patch: - sanitizes the responses from Sentry, only keeping those that are well-formed and potentially Kudu-related, - stores them in a more ergonomic form, e.g. keeping around enums rather than strings for SentryActions, etc. This form may be updated in the future to facilitate privilege evaluation -- for now, my goal is just to make it easier to work with Sentry privileges, - encapsulates the above in an abstracted version of a Sentry response that corresponds to the hierarchy tree for a given table, with the hope that it will make changing the in-memory format more painless, - switches the SentryAuthorizableScope and SentryAction enum classes to enums, to avoid having to use the extra enum class typename everywhere (e.g. now SentryAuthorizableScope::SERVER instead of SentryAuthorizableScope::Scope::SERVER will suffice), - tests that the sanitization does what it purports to do, - tests authorizing "create tables" with OWNER privileges, due to an issue caught in review. Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b --- M src/kudu/gutil/map-util.h 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/sentry_action.h M src/kudu/sentry/sentry_authorizable_scope.h 6 files changed, 715 insertions(+), 94 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/12919/9 -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12919 to look at the new patch set (#8). Change subject: sentry: sanitize and parse privileges from Sentry .. sentry: sanitize and parse privileges from Sentry Currently, we pass around the Thrift privileges received from Sentry, which can be both expensive memory-wise and cumbersome to use. This patch: - sanitizes the responses from Sentry, only keeping those that are well-formed and potentially Kudu-related, - stores them in a more ergonomic form, e.g. keeping around enums rather than strings for SentryActions, etc. This form may be updated in the future to facilitate privilege evaluation -- for now, my goal is just to make it easier to work with Sentry privileges, - encapsulates the above in an abstracted version of a Sentry response that corresponds to the hierarchy tree for a given table, with the hope that it will make changing the in-memory format more painless, - switches the SentryAuthorizableScope and SentryAction enum classes to enums, to avoid having to use the extra enum class typename everywhere (e.g. now SentryAuthorizableScope::SERVER instead of SentryAuthorizableScope::Scope::SERVER will suffice), - tests that the sanitization does what it purports to do, - tests authorizing "create tables" with OWNER privileges, due to an issue caught in review. Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b --- M src/kudu/gutil/map-util.h 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/sentry_action.h M src/kudu/sentry/sentry_authorizable_scope.h 6 files changed, 716 insertions(+), 94 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/12919/8 -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12919/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/7/src/kudu/master/sentry_authz_provider.cc@319
PS7, Line 319: return { SentryAuthorizableScope::DATABASE,
:SentryAuthorizableScope::TABLE,
:SentryAuthorizableScope::COLUMN };
This is going to be created every time this function called, no?
Why not to use 'static const ...' for the instances of the containers and
return const references to those? I'm not sure in C++11 it's possible to get
benefits of constexpr functions that don't have there body in a single line,
and this method isn't not even marked with constexpr.
http://gerrit.cloudera.org:8080/#/c/12919/7/src/kudu/master/sentry_authz_provider.cc@335
PS7, Line 335: AuthorizableScopesSet
Why not to return const reference here?
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 04 Apr 2019 22:00:26 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h@42
PS5, Line 42:
nit: use
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h@73
PS5, Line 73: LOG(FATAL) << "not reachable";
: }
: #endif
I am not sure why grant option has to work with privilege 'ALL'? I believe in
Sentry grant option can work with any actions. It looks like you are doing this
because it is hard to wire grant option with each action, and we only requires
grant option for CreateTable which requires 'ALL on Database'? If it is the
case, would you mind add a comment here. And we may need to revisit this if
there is some operation requires action other than 'ALL' with grant.
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h@92
PS5, Line 92:
It would be more clear to name it as SentryPrivilegesInSameBranch or something
alike? To clarify this is Sentry privileges from the same hierarchy branch.
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@176
PS5, Line 176: (SentryAuthorizableScope(privilege.scope).Implies(scope)) {
: for (const auto& granted_action :
privilege.granted_privileges) {
: if (SentryAction(granted_action).Implies(action)) {
: return true;
:
As we are using FixedBitSet to store all actions granted to an authorizable, do
you think it makes sense to have a bulk version of 'Implies' method, where
check if a SentryActionsSet can imply a single action (using the util of
FixedBitSet, e.g 'contains') without iteration through each action? The
motivation of this is to have a faster lookup.
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@362
PS5, Line 362: SentryAuthorizableScope::Scope* scope,
: SentryAction::Action* action) {
: DCHECK_EQ(FLAGS_server_name, requested_authori
Should these be moved to L488 to avoid duplicated checks?
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@406
PS5, Line 406:
nit: Granted?
http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@513
PS5, Line 513: cop
Since OWNER is considered as equivalent to ALL, we may need to add OWNER as
well.
Would you mind adding test such case in
TestTableAuthorization.TestAuthorizeCreateTable?
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 04 Apr 2019 21:27:54 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12919 to look at the new patch set (#7). Change subject: sentry: sanitize and parse privileges from Sentry .. sentry: sanitize and parse privileges from Sentry Currently, we pass around the Thrift privileges received from Sentry, which can be both expensive memory-wise and cumbersome to use. This patch: - sanitizes the responses from Sentry, only keeping those that are well-formed and potentially Kudu-related, - stores them in a more ergonomic form, e.g. keeping around enums rather than strings for SentryActions, etc. This form may be updated in the future to facilitate privilege evaluation -- for now, my goal is just to make it easier to work with Sentry privileges, - encapsulates the above in an abstracted version of a Sentry response that corresponds to the hierarchy tree for a given table, with the hope that it will make changing the in-memory format more painless, - switches the SentryAuthorizableScope and SentryAction enum classes to enums, to avoid having to use the extra enum class typename everywhere (e.g. now SentryAuthorizableScope::SERVER instead of SentryAuthorizableScope::Scope::SERVER will suffice). Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b --- M src/kudu/gutil/map-util.h 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/sentry_action.h M src/kudu/sentry/sentry_authorizable_scope.h 6 files changed, 687 insertions(+), 84 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/12919/7 -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12919 to look at the new patch set (#6). Change subject: sentry: sanitize and parse privileges from Sentry .. sentry: sanitize and parse privileges from Sentry Currently, we pass around the Thrift privileges received from Sentry, which can be both expensive memory-wise and cumbersome to use. This patch: - sanitizes the responses from Sentry, only keeping those that are well-formed and potentially Kudu-related, - stores them in a more ergonomic form, e.g. keeping around enums rather than strings for SentryActions, etc. This form may be updated in the future to facilitate privilege evaluation -- for now, my goal is just to make it easier to work with Sentry privileges, - encapsulates the above in an abstracted version of a Sentry response that corresponds to the hierarchy tree for a given table, with the hope that it will make changing the in-memory format more painless, - switches the SentryAuthorizableScope and SentryAction enum classes to enums, to avoid having to use the extra enum class typename everywhere (e.g. now SentryAuthorizableScope::SERVER instead of SentryAuthorizableScope::Scope::SERVER will suffice). Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b --- M src/kudu/gutil/map-util.h 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/sentry_action.h M src/kudu/sentry/sentry_authorizable_scope.h 6 files changed, 682 insertions(+), 84 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/12919/6 -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@91
PS3, Line 91: AuthorizableScopesSet
> Still not sure I understand; different calls to this function shouldn't sha
We discussed it offline, but I'm adding this for extra reference.
The idea is to have 4 static instances of AuthorizableScopesSet, each
pre-populated with initializer list. Each might reside within the
corresponding 'case' scope, so it's something like the following for the
'SERVER' scope:
...
case SentryAuthorizableScope::SERVER: {
static const AuthorizableScopesSet kServerScopes{
SentryAuthorizableScope::DATABASE,
SentryAuthorizableScope::TABLE,
SentryAuthorizableScope::COLUMN, };
return kServerScopes;
}
...
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 04 Apr 2019 19:53:35 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h File src/kudu/sentry/sentry_authorizable_scope.h: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@91 PS3, Line 91: > Ah, I didn't explain the idea clear enough, sorry. Still not sure I understand; different calls to this function shouldn't share the same AuthorizableScopesSet. Isn't that what defining one static instance would do? https://www.studytonight.com/cpp/static-keyword.php -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 04 Apr 2019 18:57:36 + Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12919 to look at the new patch set (#5). Change subject: sentry: sanitize and parse privileges from Sentry .. sentry: sanitize and parse privileges from Sentry Currently, we pass around the Thrift privileges received from Sentry, which can be both expensive memory-wise and cumbersome to use. This patch: - sanitizes the responses from Sentry, only keeping those that are well-formed and potentially Kudu-related, - stores them in a more ergonomic form, e.g. keeping around enums rather than strings for SentryActions, etc. This form may be updated in the future to facilitate privilege evaluation -- for now, my goal is just to make it easier to work with Sentry privileges, - encapsulates the above in an abstracted version of a Sentry response that corresponds to the hierarchy tree for a given table, with the hope that it will make changing the in-memory format more painless, - switches the SentryAuthorizableScope and SentryAction enum classes to enums, to avoid having to use the extra enum class typename everywhere (e.g. now SentryAuthorizableScope::SERVER instead of SentryAuthorizableScope::Scope::SERVER will suffice). Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b --- M src/kudu/gutil/map-util.h 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/sentry_action.h M src/kudu/sentry/sentry_authorizable_scope.h 6 files changed, 681 insertions(+), 84 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/12919/5 -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 4: Code-Review+1 (3 comments) Looks good to me, but it seems one test is failing? http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@325 PS3, Line 325: InsertOrDie(&expected_empt > It sounds more like an English sentence as is, i.e. if (SentryPrivilegeIsWe Ah, OK. SGTM. http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h File src/kudu/sentry/sentry_action.h: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h@53 PS3, Line 53:ALL, > I somewhat see the point of this, but don't feel strongly enough about it t That was just a nit, yep. Most likely I just missed this nit when sentry_action.h just appeared, and I agree there are multiple criteria on the ordering :) Keeping it as is sounds good to me. http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h File src/kudu/sentry/sentry_authorizable_scope.h: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@91 PS3, Line 91: > >Is it possible to create the result AuthorizableScopesSet once and the jus Ah, I didn't explain the idea clear enough, sorry. The idea was to have static instances of AuthorizableScopesSet per scope in body of this function, and just return const reference to the corresponding instance. -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 04 Apr 2019 17:12:28 + Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 4:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@56
PS3, Line 56: using sentry::TSentryPrivilege;
> warning: using decl 'set' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@132
PS3, Line 132: requested_authorizable, /*scope=*/nullptr, /*action=*/nullptr));
: }
> Yes, I understand the reasoning here: the responses are from real Sentry in
Done
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@147
PS3, Line 147: const char* const kTestUser = "test-user";
> warning: 'kAllActions' is a static definition in anonymous namespace; stati
Done
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@171
PS3, Line 171: }
> style nit: it seems the indent should be four spaces; a space after the col
Done
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@191
PS3, Line 191: // The scope string is set to something other than the
expected scope.
> warning: parameter 'user' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@264
PS3, Line 264:
: // Select a scope at which we'll mess up the privilege
request's field.
: AuthorizableScopesSet nonempty_fields =
:
SentryAuthzProvider::ExpectedNonEmptyFields(scope.scope());
: if (invalid_privilege == InvalidPrivilege::FLIPPED_FIELD) {
:
> syntax sugar addict's nit: could it be just
Done
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@47
PS3, Line 47: std::string db,
> warning: the parameter 'server' is copied for each invocation but only used
Done
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@77
PS3, Line 77:
> I understand that it's directly related to Sentry's terminology, but I'm no
Done
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@87
PS3, Line 87: uthorizable hierarchy
> Whoops, it should have been '... the user is granted privileges on ...'
Done
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@90
PS3, Line 90:
> Is it going to be bound to the table only?
No, clarified this.
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@95
PS3, Line 95: e.
> I'm not sure I understand why this parameter is necessary given the descrip
Yeah, I'll just remove it.
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@325
PS3, Line 325: InsertOrDie(&expected_empt
> Why not
It sounds more like an English sentence as is, i.e. if
(SentryPrivilegeIsWellFormed) reads as, "if the Sentry privilege is well formed
..."
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@463
PS3, Line 463: DCHECK(!d
> Do you think it's worth logging a message in that case? Maybe LOG(INFO) or
Done
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h@53
PS3, Line 53:ALL,
> Not from this particular changelist, but while you are at it: maybe, move A
I somewhat see the point of this, but don't feel strongly enough about it to
change it here and everywhere else where this order is held. I'm going to leave
it for now unless you feel strongly about it.
Alternative orderings would be alphabetical, or sorted by breadth. AFAICT there
isn't currently an order to this, but i might be wrong.
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@91
PS3, Line 91:
> Is it possible to create the result AuthorizableScopesSet once and the just
>Is it possible to create the result AuthorizableScopesSet once and the just
>return references to those? E.g., maybe declare those as static inside the
>function and populate those just once.
Maybe I'm missing
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12919 to look at the new patch set (#4). Change subject: sentry: sanitize and parse privileges from Sentry .. sentry: sanitize and parse privileges from Sentry Currently, we pass around the Thrift privileges received from Sentry, which can be both expensive memory-wise and cumbersome to use. This patch: - sanitizes the responses from Sentry, only keeping those that are well-formed and potentially Kudu-related, - stores them in a more ergonomic form, e.g. keeping around enums rather than strings for SentryActions, etc. This form may be updated in the future to facilitate privilege evaluation -- for now, my goal is just to make it easier to work with Sentry privileges, - encapsulates the above in an abstracted version of a Sentry response that corresponds to the hierarchy tree for a given table, with the hope that it will make changing the in-memory format more painless, - switches the SentryAuthorizableScope and SentryAction enum classes to enums, to avoid having to use the extra enum class typename everywhere (e.g. now SentryAuthorizableScope::SERVER instead of SentryAuthorizableScope::Scope::SERVER will suffice). Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b --- M src/kudu/gutil/map-util.h 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/sentry_action.h M src/kudu/sentry/sentry_authorizable_scope.h 6 files changed, 676 insertions(+), 84 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/12919/4 -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 3: > Overall looks good to me structurally, some nits and maybe add a > unit test for IsSentryPrivilegeWellFormed() if it makes sense. I think the latter (the unit test for IsSentryPrivilegeWellFormed()) might be added in a separate patfch. -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 04 Apr 2019 06:29:54 + Gerrit-HasComments: No
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@132
PS3, Line 132: Why just the server? This guarantees that a request for any
: // authorizable scope will ignore such invalid privileges.
Yes, I understand the reasoning here: the responses are from real Sentry
instance, and that's about end-to-end verification.
What do you think about adding sort of unit test for
SentryPrivilegeIsWellFormed() (currently file-scope-private of
sentry_authz_provider.cc) where the fields of TSentryPrivilege are being messed
up mercilessly?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@171
PS3, Line 171: :
style nit: it seems the indent should be four spaces; a space after the column
is needed
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@264
PS3, Line 264: static const vector kInvalidPrivileges = {
: InvalidPrivilege::INCORRECT_ACTION,
: InvalidPrivilege::INCORRECT_SCOPE,
: InvalidPrivilege::INCORRECT_SERVER,
: InvalidPrivilege::FLIPPED_FIELD,
: };
syntax sugar addict's nit: could it be just
static constexpr InvalidPrivilege kInvalidPrivileges[] = {
...
};
?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@87
PS3, Line 87: the privileges apply.
> I'm not sure I understand what 'apply' means in this context. So, maybe ch
Whoops, it should have been '... the user is granted privileges on ...'
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@325
PS3, Line 325: SentryPrivilegeIsWellFormed
Why not
IsSentryPrivilegeWellFormed(...) ?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@463
PS3, Line 463: continue;
Do you think it's worth logging a message in that case? Maybe LOG(INFO) or at
least VLOG(1) ?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h@53
PS3, Line 53:ALL,
Not from this particular changelist, but while you are at it: maybe, move ALL
closer to OWNER (e.g. insert it between 'DROP' and 'OWNER')?
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 04 Apr 2019 02:18:11 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 3: Overall looks good to me structurally, some nits and maybe add a unit test for IsSentryPrivilegeWellFormed() if it makes sense. -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 04 Apr 2019 02:19:58 + Gerrit-HasComments: No
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 3:
(6 comments)
a few comments, more are coming
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@77
PS3, Line 77: authorized_actions
I understand that it's directly related to Sentry's terminology, but I'm not
sure we want to continue confusing ourselves and others who might be reading
this code...
I would expect those to be named somehow that reflects the fact that those are
granted privileges, not authorized actions. By my understanding, the
AuthzProvider is the authority to authorize actions based on the set of granted
privileges.
What do you think?
https://en.wikipedia.org/wiki/Privilege_(computing)
https://www.postgresql.org/docs/9.0/sql-grant.html
Maybe, name it 'granted_privileges' instead?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@87
PS3, Line 87: the privileges apply.
I'm not sure I understand what 'apply' means in this context. So, maybe change
into '... the user is granted privileges at ...'.
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@90
PS3, Line 90: apply to a single table for a single user.
Is it going to be bound to the table only?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@95
PS3, Line 95: const std::string& table_ident,
I'm not sure I understand why this parameter is necessary given the description
of this method. So, given some table 'A' and privileges that apply to it (say,
at table level only), it's possible to use this method to deduce whether this
set of privileges implies some action at a table 'B', maybe in other database?
From the other point, if we are about to include object identifier as
'table_identifier' here to verify it matches with what is passed as a
parameter, why not to include username as well?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@91
PS3, Line 91: AuthorizableScopesSet
Is it possible to create the result AuthorizableScopesSet once and the just
return references to those? E.g., maybe declare those as static inside the
function and populate those just once.
Also, is it really important to declare this function to be inline and in this
header? Maybe, just move the definition into sentry_authz_provider.cc file
(the only place it's used now, if I'm not mistaked) and drop the 'inline'
specified to allow the compiler do the right thing?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@114
PS3, Line 114: inline AuthorizableScopesSet
ExpectedNonEmptyFields(SentryAuthorizableScope::Scope scope) {
: AuthorizableScopesSet expected_nonempty_fields;
: switch (scope) {
: case SentryAuthorizableScope::COLUMN:
: InsertOrDie(&expected_nonempty_fields,
SentryAuthorizableScope::COLUMN);
: FALLTHROUGH_INTENDED;
: case SentryAuthorizableScope::TABLE:
: InsertOrDie(&expected_nonempty_fields,
SentryAuthorizableScope::TABLE);
: FALLTHROUGH_INTENDED;
: case SentryAuthorizableScope::DATABASE:
: InsertOrDie(&expected_nonempty_fields,
SentryAuthorizableScope::DATABASE);
: FALLTHROUGH_INTENDED;
: case SentryAuthorizableScope::SERVER:
: InsertOrDie(&expected_nonempty_fields,
SentryAuthorizableScope::SERVER);
: break;
: default:
: LOG(DFATAL) << "not reachable";
: }
: return expected_nonempty_fields;
: }
ditto
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 04 Apr 2019 00:50:57 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12919 to look at the new patch set (#3). Change subject: sentry: sanitize and parse privileges from Sentry .. sentry: sanitize and parse privileges from Sentry Currently, we pass around the Thrift privileges received from Sentry, which can be both expensive memory-wise and cumbersome to use. This patch: - sanitizes the responses from Sentry, only keeping those that are well-formed and potentially Kudu-related, - stores them in a more ergonomic form, e.g. keeping around enums rather than strings for SentryActions, etc. This form may be updated in the future to facilitate privilege evaluation -- for now, my goal is just to make it easier to work with Sentry privileges, - encapsulates the above in an abstracted version of a Sentry response that corresponds to the hierarchy tree for a given table, with the hope that it will make changing the in-memory format more painless, - switches the SentryAuthorizableScope and SentryAction enum classes to enums, to avoid having to use the extra enum class typename everywhere (e.g. now SentryAuthorizableScope::SERVER instead of SentryAuthorizableScope::Scope::SERVER will suffice). Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b --- M src/kudu/gutil/map-util.h 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/sentry_action.h M src/kudu/sentry/sentry_authorizable_scope.h 6 files changed, 593 insertions(+), 84 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/12919/3 -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
