HBase
  1. HBase
  2. HBASE-10619

Don't allow non super users to do DDL ops on system tables

    Details

    • Type: Bug Bug
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Don't allow non super users to do DDL ops on system tables
      For META and NS tables, we should allow them to be disabled even by super users. With out these tables online the cluster will be non operational.

      1. HBASE-10619_V6.patch
        33 kB
        Anoop Sam John
      2. HBASE-10619_V5.patch
        42 kB
        Anoop Sam John
      3. HBASE-10619_V4.patch
        35 kB
        Anoop Sam John
      4. HBASE-10619_V3.patch
        12 kB
        Anoop Sam John
      5. HBASE-10619_V2.patch
        7 kB
        Anoop Sam John
      6. HBASE-10619.patch
        2 kB
        Anoop Sam John

        Activity

        Hide
        Anoop Sam John added a comment -

        Why not parse the superuser information out of Configuration and cache it in one place? In User. Then have the AC and VC and other users call User methods rather than keep local copies of these lists.

        That would be ideal and User can be the best place. Only thing is we have to cache it as a static member there. That is why I was a bit hesitant.
        We can add isSuperUser(Configuration) API to User and the 1st call will get the super users/groups from Conf and cache at User level.

        Show
        Anoop Sam John added a comment - Why not parse the superuser information out of Configuration and cache it in one place? In User. Then have the AC and VC and other users call User methods rather than keep local copies of these lists. That would be ideal and User can be the best place. Only thing is we have to cache it as a static member there. That is why I was a bit hesitant. We can add isSuperUser(Configuration) API to User and the 1st call will get the super users/groups from Conf and cache at User level.
        Hide
        Stephen Yuan Jiang added a comment -

        -1 as DisableTableHandler is deprecated (in master branch as of April 10th, see HBASE-13211) - please update the DisableTableProcedure.java instead.

        Anoop Sam John], are you planning to port this to branch-1?

        Show
        Stephen Yuan Jiang added a comment - -1 as DisableTableHandler is deprecated (in master branch as of April 10th, see HBASE-13211 ) - please update the DisableTableProcedure.java instead. Anoop Sam John ], are you planning to port this to branch-1?
        Hide
        Andrew Purtell added a comment -

        -1 in current form. API is not good I think.

        Do we need a new UserUtil? There's already User.

        public static Pair<List<String>, List<String>> getSuperUsers(Configuration conf)
        

        Returning a pair of lists is weird. It's not like entries in one are directly related to another. One is a list of users. One is a list of groups. getSuperUsers should return the list of user principals. A new getSuperGroups should return the list of group principals.

        public static boolean isSuperUser(User user, List<String> superUsers, List<String> superGroups)
        

        This is kind of useless? You have to pass in the list of users and groups to check. Those lists could be anything. How does this check if the User is actually a super user?

        Why not parse the superuser information out of Configuration and cache it in one place? In User. Then have the AC and VC and other users call User methods rather than keep local copies of these lists.

        Show
        Andrew Purtell added a comment - -1 in current form. API is not good I think. Do we need a new UserUtil? There's already User. public static Pair<List<String>, List<String>> getSuperUsers(Configuration conf) Returning a pair of lists is weird. It's not like entries in one are directly related to another. One is a list of users. One is a list of groups. getSuperUsers should return the list of user principals. A new getSuperGroups should return the list of group principals. public static boolean isSuperUser(User user, List<String> superUsers, List<String> superGroups) This is kind of useless? You have to pass in the list of users and groups to check. Those lists could be anything. How does this check if the User is actually a super user? Why not parse the superuser information out of Configuration and cache it in one place? In User. Then have the AC and VC and other users call User methods rather than keep local copies of these lists.
        Hide
        Anoop Sam John added a comment -

        Sure I will correct on commit.
        Ping Enis Soztutar for branch-1 and branch-1.0
        Andrew Purtell 0.98?

        Show
        Anoop Sam John added a comment - Sure I will correct on commit. Ping Enis Soztutar for branch-1 and branch-1.0 Andrew Purtell 0.98?
        Hide
        Srikanth Srungarapu added a comment -

        Yeah, just for the sake of readability. It's upto you though.

        Show
        Srikanth Srungarapu added a comment - Yeah, just for the sake of readability. It's upto you though.
        Hide
        Anoop Sam John added a comment -

        Thanks for the reviews Jerry and Srikanth.
        Regarding if else comment. As the if conditions are breaking the path of execution in the method (it throws Exception and come out) I thought we no need to have else block as such.. You said it wrt better readability with if else? If so I tend to agree.. Better readability would be with if else.. If so I can change on commit.

        Show
        Anoop Sam John added a comment - Thanks for the reviews Jerry and Srikanth. Regarding if else comment. As the if conditions are breaking the path of execution in the method (it throws Exception and come out) I thought we no need to have else block as such.. You said it wrt better readability with if else? If so I tend to agree.. Better readability would be with if else.. If so I can change on commit.
        Hide
        Srikanth Srungarapu added a comment -

        Nice refactoring of helper methods. Just a minor thing

        +    if (tableName.equals(TableName.META_TABLE_NAME)) {
        +      throw new ConstraintException("Cannot disable " + TableName.META_TABLE_NAME + " table");
        +    }
        +    if (tableName.equals(TableName.NAMESPACE_TABLE_NAME)) {
        +      throw new ConstraintException("Cannot disable " + TableName.NAMESPACE_TABLE_NAME + " table");
        +    }
        +    if (tableName.isSystemTable()) {
        +      User user = UserUtil.getActiveUser();
        +      if (!UserUtil.isSuperUser(user, this.superUsers, this.superGroups)) {
        +        throw new ConstraintException("User : " + user.getShortName()
        +            + " can't disable tables in '" + NamespaceDescriptor.SYSTEM_NAMESPACE_NAME_STR
        +            + "' namespace");
        +      }
        +    }
        

        How about using else-ifs here?

        Show
        Srikanth Srungarapu added a comment - Nice refactoring of helper methods. Just a minor thing + if (tableName.equals(TableName.META_TABLE_NAME)) { + throw new ConstraintException( "Cannot disable " + TableName.META_TABLE_NAME + " table" ); + } + if (tableName.equals(TableName.NAMESPACE_TABLE_NAME)) { + throw new ConstraintException( "Cannot disable " + TableName.NAMESPACE_TABLE_NAME + " table" ); + } + if (tableName.isSystemTable()) { + User user = UserUtil.getActiveUser(); + if (!UserUtil.isSuperUser(user, this .superUsers, this .superGroups)) { + throw new ConstraintException( "User : " + user.getShortName() + + " can't disable tables in '" + NamespaceDescriptor.SYSTEM_NAMESPACE_NAME_STR + + "' namespace" ); + } + } How about using else-ifs here?
        Hide
        Jerry He added a comment -

        Looks good. The TODO is a pre-existing problem that can be dealt with separately.
        +1

        Show
        Jerry He added a comment - Looks good. The TODO is a pre-existing problem that can be dealt with separately. +1
        Hide
        Anoop Sam John added a comment -

        Can I get +1s? Ping Andrew Purtell, Jerry He
        Checkstyle one issue in UserUtil - I can correct on commit.

        Show
        Anoop Sam John added a comment - Can I get +1s? Ping Andrew Purtell , Jerry He Checkstyle one issue in UserUtil - I can correct on commit.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12723868/HBASE-10619_V6.patch
        against master branch at commit b6756b39c2ff5675f96a6e82dc4d68ec437f01c4.
        ATTACHMENT ID: 12723868

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        -1 checkstyle. The applied patch generated 1922 checkstyle errors (more than the master's current 1921 errors).

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        -1 core zombie tests. There are 1 zombie test(s):

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13618//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13618//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13618//artifact/patchprocess/checkstyle-aggregate.html

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13618//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12723868/HBASE-10619_V6.patch against master branch at commit b6756b39c2ff5675f96a6e82dc4d68ec437f01c4. ATTACHMENT ID: 12723868 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 checkstyle . The applied patch generated 1922 checkstyle errors (more than the master's current 1921 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . -1 core zombie tests . There are 1 zombie test(s): Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13618//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13618//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13618//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13618//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12723839/HBASE-10619_V5.patch
        against master branch at commit b6756b39c2ff5675f96a6e82dc4d68ec437f01c4.
        ATTACHMENT ID: 12723839

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 9 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithCustomVisLabService

        -1 core zombie tests. There are 1 zombie test(s): at org.apache.sling.discovery.impl.cluster.ClusterLoadTest.testFramework(ClusterLoadTest.java:66)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13615//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13615//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13615//artifact/patchprocess/checkstyle-aggregate.html

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13615//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12723839/HBASE-10619_V5.patch against master branch at commit b6756b39c2ff5675f96a6e82dc4d68ec437f01c4. ATTACHMENT ID: 12723839 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithCustomVisLabService -1 core zombie tests . There are 1 zombie test(s): at org.apache.sling.discovery.impl.cluster.ClusterLoadTest.testFramework(ClusterLoadTest.java:66) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13615//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13615//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13615//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13615//console This message is automatically generated.
        Anoop Sam John made changes -
        Description We should treat NS table just like META table, which is having checks so that user can not disable/drop.

        Don't allow non super users to do DDL ops on system tables
        For META and NS tables, we should allow them to be disabled even by super users. With out these tables online the cluster will be non operational.

        Anoop Sam John made changes -
        Summary Don't allow non super users to disable/drop system tables Don't allow non super users to do DDL ops on system tables
        Anoop Sam John made changes -
        Attachment HBASE-10619_V6.patch [ 12723868 ]
        Hide
        Anoop Sam John added a comment -

        Leaving the changes in VC and AC from V5 patch.. We have HBASE-13336 and rules to be followed is added there.. So better handle it in that issue.

        Show
        Anoop Sam John added a comment - Leaving the changes in VC and AC from V5 patch.. We have HBASE-13336 and rules to be followed is added there.. So better handle it in that issue.
        Anoop Sam John made changes -
        Attachment HBASE-10619_V5.patch [ 12723839 ]
        Hide
        Anoop Sam John added a comment -

        Agree with Andy.. It looks a security hole to allow non super users to create table in system NS. So I will continue to disallow as how it is done in this patch now.
        Yes the test failures are related. I will correct them.
        Thanks Jerry. Sure I will have to update the Jira title and description.

        Do you want to use the TableBName.isSystemTable?

        Sure. That will be better any way.

        Show
        Anoop Sam John added a comment - Agree with Andy.. It looks a security hole to allow non super users to create table in system NS. So I will continue to disallow as how it is done in this patch now. Yes the test failures are related. I will correct them. Thanks Jerry. Sure I will have to update the Jira title and description. Do you want to use the TableBName.isSystemTable? Sure. That will be better any way.
        Hide
        Jerry He added a comment -

        Patch looks good overall. Good work doing the refactoring in UserUtils.
        Do you want to use the TableBName.isSystemTable?
        Since you covered the checkTableModifiable as well, do you mind updating the JIRA title and description?

        Show
        Jerry He added a comment - Patch looks good overall. Good work doing the refactoring in UserUtils. Do you want to use the TableBName.isSystemTable? Since you covered the checkTableModifiable as well, do you mind updating the JIRA title and description?
        Hide
        Andrew Purtell added a comment -

        Please note that for disable table, there is an extra check for META table. Even for super user this op is not allowed.

        This is fine as a special case IMO, if the META table is disabled then everything immediately falls over. I think we need the same protection on the namespace table too. The superuser should be able to otherwise create, modify, enable/disable, and delete tables in the system namespace.

        Show
        Andrew Purtell added a comment - Please note that for disable table, there is an extra check for META table. Even for super user this op is not allowed. This is fine as a special case IMO, if the META table is disabled then everything immediately falls over. I think we need the same protection on the namespace table too. The superuser should be able to otherwise create, modify, enable/disable, and delete tables in the system namespace.
        Hide
        Andrew Purtell added a comment -

        These test failures look like they are related to the patch:

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.security.access.TestAccessController
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDefaultVisLabelService
        -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hbase.security.visibility.TestVisibilityLabels.testMutateRow(TestVisibilityLabels.java:780)

        Show
        Andrew Purtell added a comment - These test failures look like they are related to the patch: -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.security.access.TestAccessController org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDefaultVisLabelService -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hbase.security.visibility.TestVisibilityLabels.testMutateRow(TestVisibilityLabels.java:780)
        Hide
        Andrew Purtell added a comment -

        Preventing creation of user tables in system namespace would be a BC issue

        I think it's more of a security or operational concern, so it would be fine with me if we declared the ability to create a user table in a system namespace as a bug, not a backwards compatibility concern.

        Show
        Andrew Purtell added a comment - Preventing creation of user tables in system namespace would be a BC issue I think it's more of a security or operational concern, so it would be fine with me if we declared the ability to create a user table in a system namespace as a bug, not a backwards compatibility concern.
        Hide
        Anoop Sam John added a comment -

        We might have to handle the cases of table alter APIs (system ns table's) to consider the online schema change cases. Will work on that. Let me see what others think on the create table limitation. (BC break is ok or not)

        Show
        Anoop Sam John added a comment - We might have to handle the cases of table alter APIs (system ns table's) to consider the online schema change cases. Will work on that. Let me see what others think on the create table limitation. (BC break is ok or not)
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12723320/HBASE-10619_V4.patch
        against master branch at commit 057499474c346b28ad5ac3ab7da420814eba547d.
        ATTACHMENT ID: 12723320

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 protoc. The applied patch does not increase the total number of protoc compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.security.access.TestAccessController
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDefaultVisLabelService

        -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hbase.security.visibility.TestVisibilityLabels.testMutateRow(TestVisibilityLabels.java:780)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13577//testReport/
        Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13577//artifact/patchprocess/newFindbugsWarnings.html
        Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13577//artifact/patchprocess/checkstyle-aggregate.html

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13577//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12723320/HBASE-10619_V4.patch against master branch at commit 057499474c346b28ad5ac3ab7da420814eba547d. ATTACHMENT ID: 12723320 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.security.access.TestAccessController org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDefaultVisLabelService -1 core zombie tests . There are 1 zombie test(s): at org.apache.hadoop.hbase.security.visibility.TestVisibilityLabels.testMutateRow(TestVisibilityLabels.java:780) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13577//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13577//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13577//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13577//console This message is automatically generated.
        Hide
        Anoop Sam John added a comment -

        Preventing creation of user tables in system namespace would be a BC issue!! If we allow normal users to create table in System NS, we should not deny him option to disable this table. Ping Andrew Purtell, stack

        Show
        Anoop Sam John added a comment - Preventing creation of user tables in system namespace would be a BC issue!! If we allow normal users to create table in System NS, we should not deny him option to disable this table. Ping Andrew Purtell , stack
        Hide
        Anoop Sam John added a comment -

        Sorry for the delay in rework on the patch. I some how missed this completely.
        As per the comment from Andy, added the super user check. The super user can create table in system namespace as well as disable tables in namespace. For non super users these are not allowed.
        Please note that for disable table, there is an extra check for META table. Even for super user this op is not allowed. (This is what we were doing till now). This is because for a disable op to complete, the META table should be available. On disable request, we call the async op of DisableTableHandler. The calles checks for all regions of that table to be in disabled state in META. But for that META has to be up. Means we can not allow disable of META even for super users.

        Show
        Anoop Sam John added a comment - Sorry for the delay in rework on the patch. I some how missed this completely. As per the comment from Andy, added the super user check. The super user can create table in system namespace as well as disable tables in namespace. For non super users these are not allowed. Please note that for disable table, there is an extra check for META table. Even for super user this op is not allowed. (This is what we were doing till now). This is because for a disable op to complete, the META table should be available. On disable request, we call the async op of DisableTableHandler. The calles checks for all regions of that table to be in disabled state in META. But for that META has to be up. Means we can not allow disable of META even for super users.
        Anoop Sam John made changes -
        Summary Don't allow user to disable/drop NS table Don't allow non super users to disable/drop system tables
        Anoop Sam John made changes -
        Attachment HBASE-10619_V4.patch [ 12723320 ]
        Hide
        Andrew Purtell added a comment -

        So the super user should be able to even disable/drop/modify even the META or namespace table also

        By definition. Just like on UNIX, root can "rm -rf /"

        Show
        Andrew Purtell added a comment - So the super user should be able to even disable/drop/modify even the META or namespace table also By definition. Just like on UNIX, root can "rm -rf /"
        Hide
        Anoop Sam John added a comment -

        So the super user should be able to even disable/drop/modify even the META or namespace table also? I think no one should be able to deal with these tables at least.

        Show
        Anoop Sam John added a comment - So the super user should be able to even disable/drop/modify even the META or namespace table also? I think no one should be able to deal with these tables at least.
        Hide
        Andrew Purtell added a comment -

        The v3 patch isn't quite right. We should not universally reject requests. The superuser is allowed to do anything everywhere else. The tests should be isSuperUser(User.getCurrentUser()) || <condition>.

        Since we have this concept of superuser, i.e. "root", it has to be consistent. An almost-superuser is difficult to reason about.

        Show
        Andrew Purtell added a comment - The v3 patch isn't quite right. We should not universally reject requests. The superuser is allowed to do anything everywhere else. The tests should be isSuperUser(User.getCurrentUser()) || <condition>. Since we have this concept of superuser, i.e. "root", it has to be consistent. An almost-superuser is difficult to reason about.
        Hide
        Enis Soztutar added a comment -

        +1. I was a bit puzzled for how we were able to create meta/acl, etc tables with this patch, but it seems the check happens only on the rpc side, and they are calling the internal HMaster method.

        Show
        Enis Soztutar added a comment - +1. I was a bit puzzled for how we were able to create meta/acl, etc tables with this patch, but it seems the check happens only on the rpc side, and they are calling the internal HMaster method.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12631762/HBASE-10619_V3.patch
        against trunk revision .
        ATTACHMENT ID: 12631762

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 6 new or modified tests.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12631762/HBASE-10619_V3.patch against trunk revision . ATTACHMENT ID: 12631762 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 6 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8853//console This message is automatically generated.
        Anoop Sam John made changes -
        Attachment HBASE-10619_V3.patch [ 12631762 ]
        Hide
        Anoop Sam John added a comment -

        TestAccessController#createTableInSystemNamespace()
        The test creates a table in system NS and asserts successful creation. Is it fine to change this behaviour now?

        Show
        Anoop Sam John added a comment - TestAccessController#createTableInSystemNamespace() The test creates a table in system NS and asserts successful creation. Is it fine to change this behaviour now?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12631703/HBASE-10619_V2.patch
        against trunk revision .
        ATTACHMENT ID: 12631703

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithACL
        org.apache.hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles
        org.apache.hadoop.hbase.security.access.TestTablePermissions
        org.apache.hadoop.hbase.security.access.TestAccessControlFilter
        org.apache.hadoop.hbase.client.TestAdmin
        org.apache.hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFilesSplitRecovery
        org.apache.hadoop.hbase.TestNamespace
        org.apache.hadoop.hbase.security.access.TestAccessController
        org.apache.hadoop.hbase.snapshot.TestSecureExportSnapshot

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12631703/HBASE-10619_V2.patch against trunk revision . ATTACHMENT ID: 12631703 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithACL org.apache.hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles org.apache.hadoop.hbase.security.access.TestTablePermissions org.apache.hadoop.hbase.security.access.TestAccessControlFilter org.apache.hadoop.hbase.client.TestAdmin org.apache.hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFilesSplitRecovery org.apache.hadoop.hbase.TestNamespace org.apache.hadoop.hbase.security.access.TestAccessController org.apache.hadoop.hbase.snapshot.TestSecureExportSnapshot Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8848//console This message is automatically generated.
        Anoop Sam John made changes -
        Attachment HBASE-10619_V2.patch [ 12631703 ]
        Hide
        Anoop Sam John added a comment -

        Don't allow users to create table in system ns. And also don't disable/delete/modify tables in system ns.

        Show
        Anoop Sam John added a comment - Don't allow users to create table in system ns. And also don't disable/delete/modify tables in system ns.
        Hide
        Enis Soztutar added a comment -

        Now we allow user to create any tables in hbase namespace. So in such a case user will be able to create some table in hbase NS but can not do any changes to that or drop that.

        Good point. We should also not allow users to create tables there.

        Show
        Enis Soztutar added a comment - Now we allow user to create any tables in hbase namespace. So in such a case user will be able to create some table in hbase NS but can not do any changes to that or drop that. Good point. We should also not allow users to create tables there.
        Hide
        Anoop Sam John added a comment -

        Should the check be more generic? For any table in the system namespace?

        I was abt to ask this same here. I am +1 for this.

        Now we allow user to create any tables in hbase namespace. So in such a case user will be able to create some table in hbase NS but can not do any changes to that or drop that.

        As of now we have handled, protecting acl and visibility labels tables in AccessController/VisibilityController. Making a generic check for system NS can avoid this code also.

        Show
        Anoop Sam John added a comment - Should the check be more generic? For any table in the system namespace? I was abt to ask this same here. I am +1 for this. Now we allow user to create any tables in hbase namespace. So in such a case user will be able to create some table in hbase NS but can not do any changes to that or drop that. As of now we have handled, protecting acl and visibility labels tables in AccessController/VisibilityController. Making a generic check for system NS can avoid this code also.
        Hide
        Enis Soztutar added a comment -

        +1 on making it generic to all tables in hbase: namespace.

        Show
        Enis Soztutar added a comment - +1 on making it generic to all tables in hbase: namespace.
        Hide
        Andrew Purtell added a comment -

        +1 for 0.98

        Should the check be more generic? For any table in the system namespace?

        I agree, we should get away from the special casing by name now that we have a whole namespace for system tables.

        Show
        Andrew Purtell added a comment - +1 for 0.98 Should the check be more generic? For any table in the system namespace? I agree, we should get away from the special casing by name now that we have a whole namespace for system tables.
        Hide
        stack added a comment -

        +1 for 0.96.

        Should the check be more generic? For any table in the system namespace?

        Show
        stack added a comment - +1 for 0.96. Should the check be more generic? For any table in the system namespace?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12631554/HBASE-10619.patch
        against trunk revision .
        ATTACHMENT ID: 12631554

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12631554/HBASE-10619.patch against trunk revision . ATTACHMENT ID: 12631554 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8837//console This message is automatically generated.
        Anoop Sam John made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Anoop Sam John made changes -
        Field Original Value New Value
        Attachment HBASE-10619.patch [ 12631554 ]
        Hide
        Andrew Purtell added a comment -

        Ok

        Show
        Andrew Purtell added a comment - Ok
        Hide
        Anoop Sam John added a comment -

        Yes Andy. But I am saying case where AccessController is not installed. Tested in such an env. We do special handling for META. NS table is also like META which we should protect from user DDL ops

        Show
        Anoop Sam John added a comment - Yes Andy. But I am saying case where AccessController is not installed. Tested in such an env. We do special handling for META. NS table is also like META which we should protect from user DDL ops
        Hide
        Andrew Purtell added a comment -

        Seems reasonable, though permissions on the NS table should only allow GLOBAL ADMIN to drop it, right?

        Show
        Andrew Purtell added a comment - Seems reasonable, though permissions on the NS table should only allow GLOBAL ADMIN to drop it, right?
        Anoop Sam John created issue -

          People

          • Assignee:
            Anoop Sam John
            Reporter:
            Anoop Sam John
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:

              Development