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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 22 Feb 2019 06:01:51 +0000 Gerrit-HasComments: Yes
