You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2020/06/12 14:40:21 UTC

[kudu-CR] [WIP] Add delegate admin 

Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16071


Change subject: [WIP] Add delegate admin
......................................................................

[WIP] Add delegate admin

Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
---
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/ts_authz-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/default_authz_provider.h
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/master/ranger_authz_provider.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/ranger.proto
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
M src/kudu/tools/tool_action_hms.cc
13 files changed, 301 insertions(+), 88 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/16071/1
-- 
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: newchange
Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
Gerrit-Change-Number: 16071
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>

[kudu-CR] [WIP] Add delegate admin

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/16071

to look at the new patch set (#2).

Change subject: [WIP] Add delegate admin
......................................................................

[WIP] Add delegate admin

Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
---
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/ts_authz-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/default_authz_provider.h
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/master/ranger_authz_provider.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/ranger.proto
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
M src/kudu/tools/tool_action_hms.cc
13 files changed, 300 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/16071/2
-- 
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: newpatchset
Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
Gerrit-Change-Number: 16071
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/16071

to look at the new patch set (#6).

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................

KUDU-3090 Add delegate admin privilege

Database systems usually require ALL WITH GRANT OPTION privilege to
change ownership of a table. Apache Ranger supports a special
quasi-access type called "delegate admin" which grants the user
permissions to grant and revoke permissions which is similar to GRANT
OPTION in other systems.

This patch adds support for this delegate option flag which is required
in addition to "ALL" to create a table with a different owner. It also
refactors the Ranger subprocess to allow evaluating ALL and delegate
admin in a single RangerRequestPB.

Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
---
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/RangerProtocolHandler.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/ranger/TestRangerSubprocess.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/ranger/authorization/TestRangerKuduAuthorizer.java
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/ts_authz-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger.proto
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
15 files changed, 256 insertions(+), 135 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/16071/6
-- 
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: newpatchset
Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
Gerrit-Change-Number: 16071
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 6:

(1 comment)

> Patch Set 4:
> 
> (7 comments)
> 
> > Patch Set 4:
> > 
> > (8 comments)
> > 
> > We can probably break this out of the relation chain so it doesn't depend on the owner patch. The behavior we're adding hinges on ownership in CreateTable RPCs (which already exists), rather than storing ownership under the hood.
> 
> I guess I could break it out, but it does require KuduTableCreator::set_owner() which I just moved to the owner patch. Do you think it's worth moving it here or to a separate patch and add tests?

Ah I see -- I saw 'owner' defined in the protobuf and assumed the client supported it. Yeah I'd be happy with separating that into its own patch with some tests. Should be pretty small compared to the big ownership patch, and that way we can merge this patch without blocking on all the work listed in the big patch.

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger.proto
File src/kudu/ranger/ranger.proto:

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger.proto@71
PS4, Line 71: delegate admin privilege i
> Given the vast number of admins that appear to exist in Ranger[1], let's be
Done



-- 
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: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Jun 2020 16:59:32 +0000
Gerrit-HasComments: Yes

[kudu-CR] [WIP] Add delegate admin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 12 Jun 2020 19:38:41 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 4:

(8 comments)

We can probably break this out of the relation chain so it doesn't depend on the owner patch. The behavior we're adding hinges on ownership in CreateTable RPCs (which already exists), rather than storing ownership under the hood.

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@414
PS4, Line 414: GrantAllWithGrantPrivilege
Let's follow suit with naming and call this GrantAllWithGrantDatabasePrivilege


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@419
PS4, Line 419: AuthorizationPolicy tbl_policy;
             :     tbl_policy.databases.emplace_back(p.db_name);
             :     tbl_policy.tables.emplace_back("*");
             :     tbl_policy.items.emplace_back(PolicyItem({kTestUser}, {ActionPB::ALL}, true));
             :     RETURN_NOT_OK(ranger_->AddPolicy(move(tbl_policy)));
Why do we need this too?


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@660
PS4, Line 660: TEST_P(MasterAuthzITest, TestCreateTableDifferentOwner) {
Consider adding some or all of the following test cases:
- what if the user is a delegate admin for the table?
- what if the user also has ALL ON TABLE (but not on database)
- what if the user has ALL ON DATABASE but isn't a delegate admin?
- what if the user is a trusted user?


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/master/ranger_authz_provider.cc@95
PS4, Line 95: admin
nit: same here -- this naming makes it sound like we should be passing the name of an admin around. Perhaps consider something like 'requires_admin' or 'requires_delegate_admin'


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger.proto
File src/kudu/ranger/ranger.proto:

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger.proto@71
PS4, Line 71: adminisitrative privileges
Given the vast number of admins that appear to exist in Ranger[1], let's be specific and refer to this in comments and in variable name as "delegate admin".

[1] https://cwiki.apache.org/confluence/display/RANGER/Introduction+of+Security+Zones+in+Ranger

Also I mentioned something similar in another review -- IMO it's a confusing practice to refer to booleans as things that sound like they should be strings (e.g. admin/owner instead of requires_admin/is_owner). consider naming this requires_delegate_admin or somesuch


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc
File src/kudu/ranger/ranger_client-test.cc:

PS4: 
Not totally related to this patch, but we should really consider migrating this to use MiniRanger. The fact that we can plug in arbitrary privileges and allowances seems odd to me.


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc@182
PS4, Line 182: false
nit: comment annotations here too?


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc@398
PS4, Line 398: true
Why do any of these need to be true?



-- 
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: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 25 Jun 2020 16:33:04 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 6: Code-Review+2

(1 comment)

> Patch Set 6:
> 
> > Patch Set 6:
> > 
> > (1 comment)
> > 
> > > Patch Set 4:
> > > 
> > > (7 comments)
> > > 
> > > > Patch Set 4:
> > > > 
> > > > (8 comments)
> > > > 
> > > > We can probably break this out of the relation chain so it doesn't depend on the owner patch. The behavior we're adding hinges on ownership in CreateTable RPCs (which already exists), rather than storing ownership under the hood.
> > > 
> > > I guess I could break it out, but it does require KuduTableCreator::set_owner() which I just moved to the owner patch. Do you think it's worth moving it here or to a separate patch and add tests?
> > 
> > Ah I see -- I saw 'owner' defined in the protobuf and assumed the client supported it. Yeah I'd be happy with separating that into its own patch with some tests. Should be pretty small compared to the big ownership patch, and that way we can merge this patch without blocking on all the work listed in the big patch.
> 
> Hm as I still have to work on Grant's patch anyway, and it shouldn't take too long either, I think it's actually better to leave it in that patch as I feel it fits there naturally. We can wait with merging these patches until that work is finished.

K SGTM.

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc
File src/kudu/ranger/ranger_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc@398
PS4, Line 398: 
> They don't, it doesn't matter. Changed to false.
Mind changing the rest to 'false' too? The fact that they're different draws questions as a reader of this code.



-- 
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: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 29 Jun 2020 17:00:35 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 14: Code-Review+2


-- 
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: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 14:07:19 +0000
Gerrit-HasComments: No

[kudu-CR] [WIP] Add delegate admin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 15 Jun 2020 19:55:31 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 10:

(1 comment)

> Patch Set 6: Code-Review+2
> 
> (1 comment)
> 
> > Patch Set 6:
> > 
> > > Patch Set 6:
> > > 
> > > (1 comment)
> > > 
> > > > Patch Set 4:
> > > > 
> > > > (7 comments)
> > > > 
> > > > > Patch Set 4:
> > > > > 
> > > > > (8 comments)
> > > > > 
> > > > > We can probably break this out of the relation chain so it doesn't depend on the owner patch. The behavior we're adding hinges on ownership in CreateTable RPCs (which already exists), rather than storing ownership under the hood.
> > > > 
> > > > I guess I could break it out, but it does require KuduTableCreator::set_owner() which I just moved to the owner patch. Do you think it's worth moving it here or to a separate patch and add tests?
> > > 
> > > Ah I see -- I saw 'owner' defined in the protobuf and assumed the client supported it. Yeah I'd be happy with separating that into its own patch with some tests. Should be pretty small compared to the big ownership patch, and that way we can merge this patch without blocking on all the work listed in the big patch.
> > 
> > Hm as I still have to work on Grant's patch anyway, and it shouldn't take too long either, I think it's actually better to leave it in that patch as I feel it fits there naturally. We can wait with merging these patches until that work is finished.
> 
> K SGTM.

ye

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc
File src/kudu/ranger/ranger_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc@398
PS4, Line 398: 
> Mind changing the rest to 'false' too? The fact that they're different draw
yep sorry, thought I did



-- 
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: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Jul 2020 15:24:27 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 4:

(7 comments)

> Patch Set 4:
> 
> (8 comments)
> 
> We can probably break this out of the relation chain so it doesn't depend on the owner patch. The behavior we're adding hinges on ownership in CreateTable RPCs (which already exists), rather than storing ownership under the hood.

I guess I could break it out, but it does require KuduTableCreator::set_owner() which I just moved to the owner patch. Do you think it's worth moving it here or to a separate patch and add tests?

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@414
PS4, Line 414: GrantAllWithGrantPrivilege
> Let's follow suit with naming and call this GrantAllWithGrantDatabasePrivil
Done


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@419
PS4, Line 419: AuthorizationPolicy tbl_policy;
             :     tbl_policy.databases.emplace_back(p.db_name);
             :     tbl_policy.tables.emplace_back("*");
             :     tbl_policy.items.emplace_back(PolicyItem({kTestUser}, {ActionPB::ALL}, true));
             :     RETURN_NOT_OK(ranger_->AddPolicy(move(tbl_policy)));
> Why do we need this too?
In Ranger a permission on a database doesn't imply permissions on tables and columns, and IsCreateTableDone requires METADATA on *, I thought it makes sense to do it this way. In real clusters policies are usually created for each scope too, for example in CDP.


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@660
PS4, Line 660: TEST_P(MasterAuthzITest, TestCreateTableDifferentOwner) {
> Consider adding some or all of the following test cases:
Done


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:

http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/master/ranger_authz_provider.cc@95
PS4, Line 95: admin
> nit: same here -- this naming makes it sound like we should be passing the 
Done


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc
File src/kudu/ranger/ranger_client-test.cc:

PS4: 
> Not totally related to this patch, but we should really consider migrating 
I think it makes sense to test the client itself on its own without depending on Ranger as it can help identify where the issue is if something breaks, but I don't feel strongly about it and I agree that it's a bit of a pain to maintain (although I don't expect many changes after KUDU-3090 land).


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc@182
PS4, Line 182: false
> nit: comment annotations here too?
Done


http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc@398
PS4, Line 398: true
> Why do any of these need to be true?
They don't, it doesn't matter. Changed to false.



-- 
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: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Jun 2020 09:28:58 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/16071

to look at the new patch set (#4).

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................

KUDU-3090 Add delegate admin privilege

Database systems usually require ALL WITH GRANT OPTION privilege to
change ownership of a table. Apache Ranger supports a special
quasi-access type called "delegate admin" which grants the user
permissions to grant and revoke permissions which is similar to GRANT
OPTION in other systems.

This patch adds support for this delegate option flag which is required
in addition to "ALL" to create a table with a different owner. It also
refactors the Ranger subprocess to allow evaluating ALL and delegate
admin in a single RangerRequestPB.

Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
---
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/RangerProtocolHandler.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/ranger/TestRangerSubprocess.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/ranger/authorization/TestRangerKuduAuthorizer.java
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/ts_authz-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger.proto
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
14 files changed, 160 insertions(+), 117 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/16071/4
-- 
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: newpatchset
Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
Gerrit-Change-Number: 16071
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 4:

> Patch Set 3:
> 
> (3 comments)
> 
> Perhaps consider separating the delegate admin part of this patch from the ownership part. E.g. we could implement the delegate admin part of this, adding back the CreateTable privileges check (i.e. ALL WITH GRANT / ALL && ADMIN 
>  ALL + delegate admin), and add unit tests for the RangerAuthzProvider. Then, once the rest of ownership lands, we can add more end-to-end tests with the master.
> 
> That way we can get the more controversial "admin" bits squared away separately from the native ownership changes.

Sounds good, moved it to the next patch.


-- 
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: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 25 Jun 2020 15:15:46 +0000
Gerrit-HasComments: No

[kudu-CR] [WIP] Add all with grant and changing ownership

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/16071

to look at the new patch set (#3).

Change subject: [WIP] Add all with grant and changing ownership
......................................................................

[WIP] Add all with grant and changing ownership

Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
---
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/default_authz_provider.h
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/master/ranger_authz_provider.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/ranger.proto
M src/kudu/tools/tool_action_hms.cc
10 files changed, 186 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/16071/3
-- 
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: newpatchset
Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
Gerrit-Change-Number: 16071
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] [WIP] Add all with grant and changing ownership

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: [WIP] Add all with grant and changing ownership
......................................................................


Patch Set 3:

(3 comments)

Perhaps consider separating the delegate admin part of this patch from the ownership part. E.g. we could implement the delegate admin part of this, adding back the CreateTable privileges check (i.e. ALL WITH GRANT / ALL && ADMIN 
 ALL + delegate admin), and add unit tests for the RangerAuthzProvider. Then, once the rest of ownership lands, we can add more end-to-end tests with the master.

That way we can get the more controversial "admin" bits squared away separately from the native ownership changes.

http://gerrit.cloudera.org:8080/#/c/16071/3/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/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@165
PS3, Line 165: action
Another reason to use a boolean, it seems we should be auditing this as the action string, rather than "ADMIN", though it's probably worth confirming that with what other systems do.


http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:

http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/master/ranger_authz_provider.cc@95
PS3, Line 95:   // Table creation requires 'CREATE ON DATABASE' privilege.
Can we plug the owner in here and require ALL and the delegate admin option here if it's a different owner than the user? A la the documented required Sentry privileges?


http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/ranger/ranger.proto
File src/kudu/ranger/ranger.proto:

http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/ranger/ranger.proto@51
PS3, Line 51:   ADMIN = 9;
I would much rather have this be decoupled from ActionPB entirely, e.g. through a bool. IMO it'd be confusing for other developers who are at all familiar with Ranger because this is not a first-class action in the Kudu-Ranger policy, while the others are.



-- 
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: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 22 Jun 2020 18:29:38 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
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: deleteVote
Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
Gerrit-Change-Number: 16071
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/16071/3/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/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@165
PS3, Line 165: = requ
> Another reason to use a boolean, it seems we should be auditing this as the
It will still be two separate requests (_ADMIN and ALL). On the other hand I think it makes sense for Ranger to audit the actual permission checks and once we have an audit feature/integration, audit the actual operations there.


http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/master/catalog_manager.cc@1827
PS3, Line 1827:   auto authz_func = [&] (const string& username, const string& table_name) {
> warning: parameter 'owner' is unused [misc-unused-parameters]
Done


http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:

http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/master/ranger_authz_provider.cc@95
PS3, Line 95:   bool admin = user != owner;
> Can we plug the owner in here and require ALL and the delegate admin option
Done


http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/ranger/ranger.proto
File src/kudu/ranger/ranger.proto:

http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/ranger/ranger.proto@51
PS3, Line 51: }
> I would much rather have this be decoupled from ActionPB entirely, e.g. thr
Done



-- 
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: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 25 Jun 2020 15:15:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 10: Verified+1

unrelated flaky test


-- 
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: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Jul 2020 16:43:59 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 6:

> Patch Set 6:
> 
> (1 comment)
> 
> > Patch Set 4:
> > 
> > (7 comments)
> > 
> > > Patch Set 4:
> > > 
> > > (8 comments)
> > > 
> > > We can probably break this out of the relation chain so it doesn't depend on the owner patch. The behavior we're adding hinges on ownership in CreateTable RPCs (which already exists), rather than storing ownership under the hood.
> > 
> > I guess I could break it out, but it does require KuduTableCreator::set_owner() which I just moved to the owner patch. Do you think it's worth moving it here or to a separate patch and add tests?
> 
> Ah I see -- I saw 'owner' defined in the protobuf and assumed the client supported it. Yeah I'd be happy with separating that into its own patch with some tests. Should be pretty small compared to the big ownership patch, and that way we can merge this patch without blocking on all the work listed in the big patch.

Hm as I still have to work on Grant's patch anyway, and it shouldn't take too long either, I think it's actually better to leave it in that patch as I feel it fits there naturally. We can wait with merging these patches until that work is finished.


-- 
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: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 29 Jun 2020 10:49:54 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/16071

to look at the new patch set (#10).

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................

KUDU-3090 Add delegate admin privilege

Database systems usually require ALL WITH GRANT OPTION privilege to
change ownership of a table. Apache Ranger supports a special
quasi-access type called "delegate admin" which grants the user
permissions to grant and revoke permissions which is similar to GRANT
OPTION in other systems.

This patch adds support for this delegate option flag which is required
in addition to "ALL" to create a table with a different owner. It also
refactors the Ranger subprocess to allow evaluating ALL and delegate
admin in a single RangerRequestPB.

Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
---
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/RangerProtocolHandler.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/ranger/TestRangerSubprocess.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/ranger/authorization/TestRangerKuduAuthorizer.java
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/ts_authz-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger.proto
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
14 files changed, 254 insertions(+), 133 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/16071/10
-- 
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: newpatchset
Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
Gerrit-Change-Number: 16071
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] [WIP] Add delegate admin

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
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 <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 15 Jun 2020 17:24:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................


Patch Set 13: Code-Review+2


-- 
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: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 02:19:10 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16071 )

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................

KUDU-3090 Add delegate admin privilege

Database systems usually require ALL WITH GRANT OPTION privilege to
change ownership of a table. Apache Ranger supports a special
quasi-access type called "delegate admin" which grants the user
permissions to grant and revoke permissions which is similar to GRANT
OPTION in other systems.

This patch adds support for this delegate option flag which is required
in addition to "ALL" to create a table with a different owner. It also
refactors the Ranger subprocess to allow evaluating ALL and delegate
admin in a single RangerRequestPB.

Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
Reviewed-on: http://gerrit.cloudera.org:8080/16071
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
---
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/RangerProtocolHandler.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/ranger/TestRangerSubprocess.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/ranger/authorization/TestRangerKuduAuthorizer.java
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/ts_authz-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger.proto
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
14 files changed, 254 insertions(+), 133 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Grant Henke: Looks good to me, approved

-- 
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: merged
Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
Gerrit-Change-Number: 16071
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Add delegate admin privilege

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/16071

to look at the new patch set (#5).

Change subject: KUDU-3090 Add delegate admin privilege
......................................................................

KUDU-3090 Add delegate admin privilege

Database systems usually require ALL WITH GRANT OPTION privilege to
change ownership of a table. Apache Ranger supports a special
quasi-access type called "delegate admin" which grants the user
permissions to grant and revoke permissions which is similar to GRANT
OPTION in other systems.

This patch adds support for this delegate option flag which is required
in addition to "ALL" to create a table with a different owner. It also
refactors the Ranger subprocess to allow evaluating ALL and delegate
admin in a single RangerRequestPB.

Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
---
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/RangerProtocolHandler.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/ranger/TestRangerSubprocess.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/ranger/authorization/TestRangerKuduAuthorizer.java
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/ts_authz-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger.proto
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
15 files changed, 254 insertions(+), 130 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/16071/5
-- 
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: newpatchset
Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c
Gerrit-Change-Number: 16071
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)