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 <[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 19:40:30 +0000 Gerrit-HasComments: Yes
