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 <[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: Tue, 05 Mar 2019 23:25:19 +0000 Gerrit-HasComments: Yes
