HBase
  1. HBase
  2. HBASE-11434

[AccessController] Disallow inbound cells with reserved tags

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.99.0, 0.98.4
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The AccessController allows users to store cells with ACL tags encoded by the client. This isn't a security issue currently, because in order to store the cell the user must have a relevant WRITE grant, and the user is allowed to specify whatever ACL for the cell they'd like. However it could become a correctness problem in the future, if we introduce format sanity checking or the like, so let's disallow inbound mutations containing cells with reserved tags like the VisibilityController does.

      The check is skipped if the active user is a superuser. First, superusers are allowed to do anything. Second, replication (as superuser) must be able to store incoming cells with ACL tags.

      1. HBASE-11434.patch
        11 kB
        Andrew Purtell
      2. HBASE-11434.patch
        11 kB
        Andrew Purtell
      3. HBASE-11434.patch
        9 kB
        Andrew Purtell
      4. HBASE-11434.patch
        9 kB
        Andrew Purtell

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Patch Available Patch Available Open Open
        4d 21m 5 Andrew Purtell 02/Jul/14 20:02
        Open Open Patch Available Patch Available
        31m 23s 6 Andrew Purtell 02/Jul/14 20:03
        Patch Available Patch Available Resolved Resolved
        1d 5h 39m 1 Andrew Purtell 04/Jul/14 01:42
        Resolved Resolved Closed Closed
        232d 22h 50m 1 Enis Soztutar 21/Feb/15 23:32
        Enis Soztutar made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Enis Soztutar added a comment -

        Closing this issue after 0.99.0 release.

        Show
        Enis Soztutar added a comment - Closing this issue after 0.99.0 release.
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-0.98-on-Hadoop-1.1 #354 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/354/)
        HBASE-11434 [AccessController] Disallow inbound cells with reserved tags (apurtell: rev 54c186b42c3c77248783d2c3d9181c12e7b06802)

        • hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-0.98-on-Hadoop-1.1 #354 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/354/ ) HBASE-11434 [AccessController] Disallow inbound cells with reserved tags (apurtell: rev 54c186b42c3c77248783d2c3d9181c12e7b06802) hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-0.98 #374 (See https://builds.apache.org/job/HBase-0.98/374/)
        HBASE-11434 [AccessController] Disallow inbound cells with reserved tags (apurtell: rev 54c186b42c3c77248783d2c3d9181c12e7b06802)

        • hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-0.98 #374 (See https://builds.apache.org/job/HBase-0.98/374/ ) HBASE-11434 [AccessController] Disallow inbound cells with reserved tags (apurtell: rev 54c186b42c3c77248783d2c3d9181c12e7b06802) hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-TRUNK #5270 (See https://builds.apache.org/job/HBase-TRUNK/5270/)
        HBASE-11434 [AccessController] Disallow inbound cells with reserved tags (apurtell: rev 80f1271ee5eb8957ff67040e95b2880257bda822)

        • hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-TRUNK #5270 (See https://builds.apache.org/job/HBase-TRUNK/5270/ ) HBASE-11434 [AccessController] Disallow inbound cells with reserved tags (apurtell: rev 80f1271ee5eb8957ff67040e95b2880257bda822) hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-1.0 #10 (See https://builds.apache.org/job/HBase-1.0/10/)
        HBASE-11434 [AccessController] Disallow inbound cells with reserved tags (apurtell: rev a8cbf2988b9153ade4d4ebf8b5eefe5420c5b41a)

        • hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java
        • hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-1.0 #10 (See https://builds.apache.org/job/HBase-1.0/10/ ) HBASE-11434 [AccessController] Disallow inbound cells with reserved tags (apurtell: rev a8cbf2988b9153ade4d4ebf8b5eefe5420c5b41a) hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Andrew Purtell made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags Reviewed [ 10343 ]
        Resolution Fixed [ 1 ]
        Hide
        Andrew Purtell added a comment -

        Committed to 0.98+

        Show
        Andrew Purtell added a comment - Committed to 0.98+
        Hide
        Andrew Purtell added a comment -

        Looks like this is good to go, committing soon.

        Show
        Andrew Purtell added a comment - Looks like this is good to go, committing soon.
        Hide
        Enis Soztutar added a comment -

        +1 for branch-1.

        Show
        Enis Soztutar added a comment - +1 for branch-1.
        Andrew Purtell made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Andrew Purtell made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Fix Version/s 1.0.0 [ 12325852 ]
        Andrew Purtell made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Andrew Purtell made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Andrew Purtell added a comment -

        Replication tests in precommit were broken due to HBASE-11440. Resubmitting for QA anyhow.

        Show
        Andrew Purtell added a comment - Replication tests in precommit were broken due to HBASE-11440 . Resubmitting for QA anyhow.
        Hide
        Andrew Purtell added a comment -

        Ping Enis Soztutar for branch-1

        Show
        Andrew Purtell added a comment - Ping Enis Soztutar for branch-1
        Hide
        Anoop Sam John added a comment -

        +1

        Show
        Anoop Sam John added a comment - +1
        Hide
        Hadoop QA added a comment -

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

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

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

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

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

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

        -1 findbugs. The patch appears to introduce 13 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.replication.TestReplicationDisableInactivePeer
        org.apache.hadoop.hbase.replication.TestMasterReplication

        -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hbase.client.TestReplicaWithCluster.testChangeTable(TestReplicaWithCluster.java:217)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//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/12653463/HBASE-11434.patch against trunk revision . ATTACHMENT ID: 12653463 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 findbugs . The patch appears to introduce 13 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.replication.TestReplicationDisableInactivePeer org.apache.hadoop.hbase.replication.TestMasterReplication -1 core zombie tests . There are 1 zombie test(s): at org.apache.hadoop.hbase.client.TestReplicaWithCluster.testChangeTable(TestReplicaWithCluster.java:217) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9927//console This message is automatically generated.
        Andrew Purtell made changes -
        Fix Version/s 1.0.0 [ 12325852 ]
        Andrew Purtell made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Andrew Purtell made changes -
        Attachment HBASE-11434.patch [ 12653463 ]
        Hide
        Andrew Purtell added a comment -

        Rebase patch on master, looks like a whitespace difference, maybe CRLF

        Show
        Andrew Purtell added a comment - Rebase patch on master, looks like a whitespace difference, maybe CRLF
        Andrew Purtell made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Hadoop QA added a comment -

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

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9925//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/12653456/HBASE-11434.patch against trunk revision . ATTACHMENT ID: 12653456 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9925//console This message is automatically generated.
        Andrew Purtell made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Andrew Purtell made changes -
        Attachment HBASE-11434.patch [ 12653456 ]
        Hide
        Andrew Purtell added a comment -

        Yes. New patch, with unit test.

        Show
        Andrew Purtell added a comment - Yes. New patch, with unit test.
        Andrew Purtell made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Anoop Sam John added a comment -
        +    // set up the list of users with superuser privilege
        +    superusers = Lists.asList(userProvider.getCurrentUserName(),
        +      conf.getStrings(AccessControlLists.SUPERUSER_CONF_KEY, new String[0]));
        

        And UserProvider#getCurrentUserName() returns user.getName().
        We should use user.getShortName()

        And previously this List of users were creating by

        -    String currentUser = user.getShortName();
        -    List<String> superusers = Lists.asList(currentUser, conf.getStrings(
        -      AccessControlLists.SUPERUSER_CONF_KEY, new String[0]));
        

        So we used getShortName().

        Show
        Anoop Sam John added a comment - + // set up the list of users with superuser privilege + superusers = Lists.asList(userProvider.getCurrentUserName(), + conf.getStrings(AccessControlLists.SUPERUSER_CONF_KEY, new String [0])); And UserProvider#getCurrentUserName() returns user.getName(). We should use user.getShortName() And previously this List of users were creating by - String currentUser = user.getShortName(); - List< String > superusers = Lists.asList(currentUser, conf.getStrings( - AccessControlLists.SUPERUSER_CONF_KEY, new String [0])); So we used getShortName().
        Andrew Purtell made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Andrew Purtell added a comment -

        Resubmit for QA

        Show
        Andrew Purtell added a comment - Resubmit for QA
        Andrew Purtell made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Andrew Purtell made changes -
        Attachment HBASE-11434.patch [ 12653037 ]
        Hide
        Andrew Purtell added a comment -

        Thanks Anoop. Paste-o. New patch

        Show
        Andrew Purtell added a comment - Thanks Anoop. Paste-o. New patch
        Hide
        Anoop Sam John added a comment -
        +    // Superusers are allowed to store cells unconditionally
        +    if (!(superusers.contains(user.getShortName()))) {
        +      return;
        +    }
        

        Should be superusers.containc() check not the ! check right Andy?

        Show
        Anoop Sam John added a comment - + // Superusers are allowed to store cells unconditionally + if (!(superusers.contains(user.getShortName()))) { + return ; + } Should be superusers.containc() check not the ! check right Andy?
        Andrew Purtell made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Andrew Purtell made changes -
        Attachment HBASE-11434.patch [ 12653017 ]
        Hide
        Andrew Purtell added a comment -

        Attached patch passes security unit tests locally.

        Show
        Andrew Purtell added a comment - Attached patch passes security unit tests locally.
        Andrew Purtell made changes -
        Field Original Value New Value
        Description Currently the AccessController allows users to store cells with ACL tags encoded by the client. This isn't a security issue currently, because in order to store the cell the user must have a relevant WRITE grant, and the user is allowed to specify whatever ACL for the cell they'd like. However it could become a correctness problem in the future, if we introduce format sanity checking or the like, so let's disallow inbound mutations containing cells with reserved tags like the VisibilityController does.

        The check is skipped if the active user is a superuser. First, superusers are allowed to do anything. Second, replication (as superuser) must be able to store incoming cells with ACL tags.
        The AccessController allows users to store cells with ACL tags encoded by the client. This isn't a security issue currently, because in order to store the cell the user must have a relevant WRITE grant, and the user is allowed to specify whatever ACL for the cell they'd like. However it could become a correctness problem in the future, if we introduce format sanity checking or the like, so let's disallow inbound mutations containing cells with reserved tags like the VisibilityController does.

        The check is skipped if the active user is a superuser. First, superusers are allowed to do anything. Second, replication (as superuser) must be able to store incoming cells with ACL tags.
        Andrew Purtell created issue -

          People

          • Assignee:
            Andrew Purtell
            Reporter:
            Andrew Purtell
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development