Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )
Change subject: [WIP] Add delegate admin ...................................................................... Patch Set 2: (6 comments) > Patch Set 1: > > (6 comments) > > Overall, I'm confused by the approach because an ADMIN privilege and Ranger's > delegate admin checkbox seem to be very different things. I was under the > impression that the latter was an equivalent to "WITH GRANT", while it seems > like we're additionally implementing the former to be an equivalent to ALL. Yea I believe that's what we agreed on: "As delegated admins seems to be the equivalent of SQL’s ALL WITH GRANT OPTION, it makes sense to copy SQL’s behavior and grant all permissions on the scope to the user with delegated admin permissions, without having to list them all or select ALL." https://docs.google.com/document/d/1V2yvNNUD_fcTLXLE9Ntoy3fybHNFPhnrwp7uhH9irwI/edit?usp=sharing It doesn't really make sense to separate the two as we already have an ALL that is SQL's ALL, and delegate admin without an ALL could still grant themselves ALL permissions, so it makes more sense to treat it as SQL's ALL WITH GRANT. Also, there's no such permission in SQL as "WITH GRANT" without the ALL, it's more like a superset. > > It's probably worth restating our goals with these changes. Here's my best > summary so far. We'd like to: > - support table ownership > - support the changing of table ownership > - define an authorization policy for who can change table ownership > > Based on some discussion in a design doc and on the next patch, it seems like > a constraint we have is that ownership is not stored by Ranger -- Ranger > relies on Kudu to supply whether or not the request is sent by the owner. So > in transferring ownership in Kudu, we are also transferring the special > "owner" policy that is associated with the table. Because of this, it seemed > like a good idea to rely on Ranger's delegate admin checkbox to determine > whether we can transfer ownership, since the act of transferring ownership > inherently transfers policies from one user to another. I suppose we were > optimistic that defining delegate admins would be straightforward, but that > seems less true than we'd hoped. Even if it's less straight-forward to implement it than we previously thought, I think it still makes sense to do it this way for consistency's sake and to avoid. > > Completely separate from the discussion of ownership, CREATE TABLE with an > owner that isn't the user making the request requires ALL WITH GRANT in > Sentry. I suppose we ought to reconcile that with Ranger if we can. > Yea that's the plan, part of it is implemented in the next part: https://gerrit.cloudera.org/c/16072/1/src/kudu/master/ranger_authz_provider.cc#101 The rest might land in the previous one, it's still WIP. > I don't fully understand Impala's privilege evaluation, but this patch's > approach seems quite different from their implementation of ALL WITH GRANT, > where they receive the entire policy relevant to a specific request and > iterate through the policy items to determine whether the delegate admin > property is set: > https://github.com/apache/impala/blob/9ea8b1454fabf5872459cd8ae6af60e8d89f62a5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java#L287 Based on this I think their whole policy evaluation is different from ours, not only the admin part. > > We also somewhat discussed that for changing ownership of a table, Impala > simply checks that it is the current owner that is making the ChangeOwner > request. For us, that would mean circumventing Ranger altogether. There was > some concern about how this allows user to shoot themselves in the foot, > though I wasn't sold on that being a reason not to implement it, given its > simplicity and understandability. I suppose there's always the caveat that we > expect most Impala traffic to Kudu to be done as the 'impala' trusted user, > but I don't think that matters much, since presumably we rely on Impala to > authorize its requests against the owner stored in the HMS, which is synced > with the owner stored in Kudu. Seems weird to circumvent Ranger in this case, especially as it's easy to replicate this behavior with Ranger config, I think we should doc it instead: "Trusted users and delegated admins can change ownership of a table, current owner of the table can not. ADMIN access type is redundant and won’t be implemented. If the admins decide they’d like to grant permissions to the owner of the table to change ownership and grant others permissions (equivalent to ALL WITH GRANT OPTION in SQL), they can simply delegate admin to {OWNER} on all resources." By not implementing shortcuts like this it would most likely be easier to add auditing later on too. http://gerrit.cloudera.org:8080/#/c/16071/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java: http://gerrit.cloudera.org:8080/#/c/16071/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@35 PS1, Line 35: Ra > nit: prefer to avoid wildcard imports Done http://gerrit.cloudera.org:8080/#/c/16071/1/src/kudu/master/ranger_authz_provider.cc File src/kudu/master/ranger_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/16071/1/src/kudu/master/ranger_authz_provider.cc@94 PS1, Line 94: // Table creation requires 'CREATE ON DATABASE' or 'ADMIN' privilege. > Based on the Sentry policy, it seems like we only want there to be differen We agreed that ADMIN should imply ALL privileges, but unfortunately Ranger doesn't treat it that way by default, so we need to allow access to ADMIN explicitly. This is treated as OR, so either of these privileges work (i.e. CREATE, ALL (which implies CREATE) and ADMIN). http://gerrit.cloudera.org:8080/#/c/16071/1/src/kudu/master/ranger_authz_provider.cc@94 PS1, Line 94: // Table creation requires 'CREATE ON DATABASE' or 'ADMIN' privilege. : unordered_set<ActionPB, ActionHash> actions = { : ActionPB::CREATE, : ActionPB::ADMIN : }; > Consider typedefing and using a FixedBitSet<ActionPB, ActionPB_Type_Type_AR I'll look into this. http://gerrit.cloudera.org:8080/#/c/16071/1/src/kudu/master/ranger_authz_provider.cc@120 PS1, Line 120: unordered_set<ActionPB, ActionHash> actions = { : ActionPB::DROP, : ActionPB::ADMIN : }; > I thought we're only focusing on doing what it takes to change ownership. W Same as above, ADMIN needs to be able to DROP tables as well. http://gerrit.cloudera.org:8080/#/c/16071/1/src/kudu/ranger/mini_ranger.cc File src/kudu/ranger/mini_ranger.cc: http://gerrit.cloudera.org:8080/#/c/16071/1/src/kudu/ranger/mini_ranger.cc@316 PS1, Line 316: if (action == ActionPB::ADMIN) { : item.Set("delegateAdmin", true); > At first glance, this seems a bit strange because we are conflating ActionP Well, yeah, I thought it would make sense to treat this the same way in Kudu as "normal" privileges. Do you think it doesn't? Do you have other suggestions? http://gerrit.cloudera.org:8080/#/c/16071/1/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: http://gerrit.cloudera.org:8080/#/c/16071/1/src/kudu/ranger/ranger.proto@51 PS1, Line 51: ADMIN = 9; > I seem to recall discussion whether we wanted to have an explicit ADMIN pri This is not a "real" ADMIN privilege, as in this maps to Ranger's built-in delegate admin. Kudu service def in Ranger still doesn't have a new admin privilege. -- To view, visit http://gerrit.cloudera.org:8080/16071 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c Gerrit-Change-Number: 16071 Gerrit-PatchSet: 2 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 15 Jun 2020 17:24:39 +0000 Gerrit-HasComments: Yes
