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/26 09:45:39 UTC

[kudu-CR] KUDU-3090 Restrict changing ownership of a table 

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


Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 76 insertions(+), 5 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/1
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

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

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

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 123 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/20
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 20
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

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

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

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 124 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/19
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 19
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

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

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

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 111 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/17
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................


Patch Set 16:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/16113/14/src/kudu/integration-tests/master_authz-itest.cc@1099
PS14, Line 1099:       kTableName,
               :       ""
               :     },
               :     {
               :       {
               :         &MasterAuthzITestBase::GetTabletLocations,
               :         &MasterAuthzITestBase::GrantGetMetadataTablePrivilege,
               :         "GetTabletLocations",
               :       
> We should have test cases for the following behaviors too:
The 'user is the owner' case is tested by this test case actually, as it tries to authorize changing ownership before granting permissions AllWithGrant to {OWNER}, but kTestUser is the owner in all of these cases.

Created a test for the other two scenarios, but they have to be their own test as these authz combinations work in a way that there are no permissions granted at all and the action fails, then a permission is granted and the same action succeed.



-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 14 Jul 2020 16:30:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

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

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

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 107 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/16
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................


Patch Set 3:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/16113/2/src/kudu/integration-tests/master_authz-itest.cc@1044
PS2, Line 1044: GrantAllWithGrantTablePrivilege,
> But don't we only need ALL WITH GRANT on table?
You're right.


http://gerrit.cloudera.org:8080/#/c/16113/2/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:

http://gerrit.cloudera.org:8080/#/c/16113/2/src/kudu/master/authz_provider.h@117
PS2, Line 117:   // 'is_owner' indicates whether 'user' is the current owner of t
> nit: maybe reword so it's more pertinent to this interface, e.g. "'is_owner
Done


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

http://gerrit.cloudera.org:8080/#/c/16113/2/src/kudu/master/catalog_manager.cc@2573
PS2, Line 2573:     const string new_table = req.has_new_table_name() ?
              :         NormalizeTableName(req.new_table_name()) : table_name;
              :     if (req.has_new_table_owner()) {
> We shouldn't have to do this if we're changing owners, right?
Yep you're right.



-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
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: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 30 Jun 2020 14:42:10 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/16113/2/src/kudu/integration-tests/master_authz-itest.cc@1044
PS2, Line 1044: GrantAllWithGrantDatabasePrivilege
But don't we only need ALL WITH GRANT on table?


http://gerrit.cloudera.org:8080/#/c/16113/2/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:

http://gerrit.cloudera.org:8080/#/c/16113/2/src/kudu/master/authz_provider.h@117
PS2, Line 117:   // The owner is the current owner, the new owner doesn't matter.
nit: maybe reword so it's more pertinent to this interface, e.g. "'is_owner' indicates whether 'user' is the current owner of the table."


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

http://gerrit.cloudera.org:8080/#/c/16113/2/src/kudu/master/catalog_manager.cc@2573
PS2, Line 2573:     RETURN_NOT_OK(SetupError(authz_provider_->AuthorizeAlterTable(table_name, new_table, username,
              :                                                                   username == owner),
              :                              resp, MasterErrorPB::NOT_AUTHORIZED));
We shouldn't have to do this if we're changing owners, right?



-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 26 Jun 2020 17:45:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................


Patch Set 19:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/16113/18/src/kudu/master/catalog_manager.cc@2615
PS18, Line 2615:     // Change owner requires higher level of privilege (ALL WITH GRANT OPTION,
               :     // or ALL + delegate admin) than other types of alter operations, so if a
               :     // single alter contains an owner change as well as other changes, it's
               :     // sufficient to authorize only the owner change.
> Thanks for adding this! It's probably also worth adding a test for this -- 
Makes sense, added a test for it. Thanks for the suggestion.



-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 19
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 15 Jul 2020 22:17:04 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/16113/10/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:

http://gerrit.cloudera.org:8080/#/c/16113/10/src/kudu/master/authz_provider.h@32
PS10, Line 32: lass TablePrivilegePB;
             : 
> nit: drop the extra line?
Done


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

http://gerrit.cloudera.org:8080/#/c/16113/10/src/kudu/master/catalog_manager.cc@2598
PS10, Line 2598:     // Change owner requires higher level of privilege (ALL WITH GRANT OPTION,
               :     // or ALL + delegate admin) than other types of alter operations, so if a
               :     // single alter contains an owner change as well as other changes, it's
               :     // sufficient to authorize only the owner change.
               :     i
> It seems easy to misconstrue this with a privilege escalation, since we're 
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 12:17:11 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

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

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

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 71 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/3
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................


Patch Set 20: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 20
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 15 Jul 2020 23:08:54 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, 

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

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

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 75 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/2
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

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

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

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 110 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/18
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................


Patch Set 10: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/16113/10/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:

http://gerrit.cloudera.org:8080/#/c/16113/10/src/kudu/master/authz_provider.h@32
PS10, Line 32: lass TablePrivilegePB;
             : 
nit: drop the extra line?


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

http://gerrit.cloudera.org:8080/#/c/16113/10/src/kudu/master/catalog_manager.cc@2598
PS10, Line 2598:     if (req.has_new_table_owner()) {
               :       return SetupError(authz_provider_->AuthorizeChangeOwner(table_name, username,
               :                                                               username == owner),
               :                         resp, MasterErrorPB::NOT_AUTHORIZED);
               :     }
It seems easy to misconstrue this with a privilege escalation, since we're returning having only authorized the user for changing ownership.

It's probably safe to do so because changing ownership requires ALL, and so it implies ALTER. That said, it'd be nice to add a comment addressing it here.



-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 02:52:03 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Reviewed-on: http://gerrit.cloudera.org:8080/16113
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Kudu Jenkins
---
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
6 files changed, 123 insertions(+), 3 deletions(-)

Approvals:
  Andrew Wong: Looks good to me, approved
  Kudu Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 21
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................


Patch Set 6:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/16113/3/src/kudu/master/catalog_manager.cc@2579
PS3, Line 2579:       return SetupError(authz_provider_->AuthorizeChangeOwner(table_name, username,
> warning: do not use 'else' after 'return' [readability-else-after-return]
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Jul 2020 16:43:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................


Patch Set 14:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/16113/14/src/kudu/integration-tests/master_authz-itest.cc@1099
PS14, Line 1099:     {
               :       {
               :         &MasterAuthzITestBase::ChangeOwner,
               :         &MasterAuthzITestBase::GrantAllWithGrantTablePrivilege,
               :         "ChangeOwner",
               :       },
               :       kDatabaseName,
               :       kTableName,
               :     },
We should have test cases for the following behaviors too:
- the user has ALL but isn't a delegate admin
- the user has METADATA (or some other non-ALL privilege) and is a delegate admin
- the user is the owner



-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 10 Jul 2020 18:21:44 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

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

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

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 74 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/11
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

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

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

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 71 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/5
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

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

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

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................

KUDU-3090 Restrict changing ownership of a table

Before this patch changing the ownership required ALTER permissions on a
table. This patch adds a new method to authorization provider to
authorize ownership changes and makes catalog manager call it if the
owner is changed on an alter.

Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
---
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
6 files changed, 71 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/6
-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3090 Restrict changing ownership of a table

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

Change subject: KUDU-3090 Restrict changing ownership of a table
......................................................................


Patch Set 18: Code-Review+1

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/16113/18/src/kudu/master/catalog_manager.cc@2615
PS18, Line 2615:     // Change owner requires higher level of privilege (ALL WITH GRANT OPTION,
               :     // or ALL + delegate admin) than other types of alter operations, so if a
               :     // single alter contains an owner change as well as other changes, it's
               :     // sufficient to authorize only the owner change.
Thanks for adding this! It's probably also worth adding a test for this -- that if we alter the table and change ownership at the same time, we can't do so unless we have ALL assigned.



-- 
To view, visit http://gerrit.cloudera.org:8080/16113
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1
Gerrit-Change-Number: 16113
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 15 Jul 2020 18:56:43 +0000
Gerrit-HasComments: Yes