Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )
Change subject: [WIP] Add delegate admin ...................................................................... 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. 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. 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. 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 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. 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: *; nit: prefer to avoid wildcard imports 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 different semantics if 'owner' is specified to be a different user than the one making the call, no? https://github.com/apache/kudu/blob/e70d806d1d410b3f1b623e21465061ebe0f62682/docs/security.adoc 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_ARRAYSIZE> from util/bitset.h Same elsewhere. Though maybe that's worth doing in a separate patch. 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. Why is drop table being updated too? 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 ActionPB, which corresponds 1:1 with the actions defined in Kudu's Ranger service definition[1] and "delegateAdmin" which appears to be something else entirely? [1] https://github.com/apache/ranger/blob/master/agents-common/src/main/resources/service-defs/ranger-servicedef-kudu.json 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 privilege, and I thought the consensus was eventually that we didn't. What changed? -- 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: 1 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: Fri, 12 Jun 2020 19:38:41 +0000 Gerrit-HasComments: Yes
