Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )
Change subject: [WIP] Add delegate admin ...................................................................... Patch Set 2: (2 comments) > 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. >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. I don't see why that matters though -- if a delegateAdmin changes their permissions, that's fine. That is the intended usage in Ranger. IIUC, what matters from Kudu's perspective are: - how do you create a table with a different owner than the requestor? - how do you change ownership? The first question is answered in Sentry by having ALL and also having the GRANT OPTION -- what harm is there to doing the same with Ranger's delegateAdmin? While delegateAdmin does seem to be more flexible than Ranger's GRANT OPTION, why should Kudu care? Why does that necessitate Kudu automatically escalating delegateAdmin to ALL? For the second question, I don't think it is weird to circumvent Ranger, and I don't see how it affects auditing. Ranger doesn't care who the owner is -- it only cares that a request is being made by the owner. And presumably, we'd need to eventually support some ChangeOwner RPC -- why shouldn't that show up in an audit when that lands in Kudu? 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. > We agreed that ADMIN should imply ALL privileges, but unfortunately Ranger Hrm, it seems the conclusion stemmed from the assumption that delegate admin should be equivalent to ALL WITH GRANT. I don't know that that is true -- it seems like at best, delegate admin is the "WITH GRANT" part, but even that isn't really clear to me. Regardless, I don't think we need to couple delegate admin with the ALL privilege, since that does not appear to be what Ranger advertises delegate admins are far. 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); > Well, yeah, I thought it would make sense to treat this the same way in Kud Yeah I don't think we should, because I don't think delegateAdmin is the same as a normal privilege. If we were to building out a full administrative privilege, maybe, but I don't think delegateAdmin is the right means to do that, based on the few Ranger docs I've seen. We may want to consider extending the RangerRequestPB to have a requiresDelegateAdmin field or somesuch that can be used in the same places we might need ALL WITH GRANT, if we think delegateAdmin is the correct means to get WITH GRANT behavior. -- 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 19:55:31 +0000 Gerrit-HasComments: Yes
