HBase
  1. HBase
  2. HBASE-10883

Restrict the universe of labels and authorizations

    Details

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

      Description

      Currently we allow any string as visibility label or request authorization. However as seen on HBASE-10878, we accept for authorizations strings that would not work if provided as labels in visibility expressions. We should throw an exception at least in cases where someone tries to define or use a label or authorization including visibility expression operators '&', '|', '!', '(', ')'.

      1. HBASE-10883_7.patch
        9 kB
        ramkrishna.s.vasudevan
      2. HBASE-10883_6.patch
        10 kB
        ramkrishna.s.vasudevan
      3. HBASE-10883_5.patch
        9 kB
        ramkrishna.s.vasudevan
      4. HBASE-10883_4.patch
        10 kB
        ramkrishna.s.vasudevan
      5. HBASE-10883_3.patch
        10 kB
        ramkrishna.s.vasudevan
      6. HBASE-10883_2.patch
        9 kB
        ramkrishna.s.vasudevan
      7. HBASE-10883_1.patch
        5 kB
        ramkrishna.s.vasudevan
      8. HBASE-10883.patch
        3 kB
        ramkrishna.s.vasudevan

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        13h 27m 7 ramkrishna.s.vasudevan 07/Apr/14 12:40
        Patch Available Patch Available Open Open
        7d 15h 18m 7 ramkrishna.s.vasudevan 09/Apr/14 12:59
        Open Open Resolved Resolved
        21d 13h 21m 1 Andrew Purtell 01/May/14 02:20
        Resolved Resolved Closed Closed
        296d 22h 14m 1 Enis Soztutar 21/Feb/15 23:35
        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.
        Andrew Purtell made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Hadoop Flags Reviewed [ 10343 ]
        Resolution Fixed [ 1 ]
        Hide
        Andrew Purtell added a comment -

        This was committed.

        Show
        Andrew Purtell added a comment - This was committed.
        Andrew Purtell made changes -
        Fix Version/s 0.98.2 [ 12326505 ]
        Fix Version/s 0.98.3 [ 12326765 ]
        Andrew Purtell made changes -
        Fix Version/s 0.98.3 [ 12326765 ]
        Fix Version/s 0.98.2 [ 12326505 ]
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #254 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/254/)
        HBASE-10883-Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585947)

        • /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java
        • /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java
        • /hbase/branches/0.98/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #254 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/254/ ) HBASE-10883 -Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585947) /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java /hbase/branches/0.98/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #5073 (See https://builds.apache.org/job/HBase-TRUNK/5073/)
        HBASE-10883-Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585946)

        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java
        • /hbase/trunk/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #5073 (See https://builds.apache.org/job/HBase-TRUNK/5073/ ) HBASE-10883 -Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585946) /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java /hbase/trunk/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-0.98 #270 (See https://builds.apache.org/job/HBase-0.98/270/)
        HBASE-10883-Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585947)

        • /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java
        • /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java
        • /hbase/branches/0.98/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-0.98 #270 (See https://builds.apache.org/job/HBase-0.98/270/ ) HBASE-10883 -Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585947) /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java /hbase/branches/0.98/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        ramkrishna.s.vasudevan made changes -
        Attachment HBASE-10883_7.patch [ 12639386 ]
        Hide
        ramkrishna.s.vasudevan added a comment -

        This is what was committed to trunk and 0.98.
        Thanks for the reviews everyone.

        Show
        ramkrishna.s.vasudevan added a comment - This is what was committed to trunk and 0.98. Thanks for the reviews everyone.
        ramkrishna.s.vasudevan made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Anoop Sam John added a comment -

        VisibilityLabelsValidator#isValidLabel(List<String> labels) - This is not used anywhere now I guess. Pls remove on commit.
        +1

        Show
        Anoop Sam John added a comment - VisibilityLabelsValidator#isValidLabel(List<String> labels) - This is not used anywhere now I guess. Pls remove on commit. +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/12638965/HBASE-10883_6.patch
        against trunk revision .
        ATTACHMENT ID: 12638965

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

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

        +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 does not introduce any 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/9213//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//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/12638965/HBASE-10883_6.patch against trunk revision . ATTACHMENT ID: 12638965 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +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 does not introduce any 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/9213//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//console This message is automatically generated.
        ramkrishna.s.vasudevan made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        ramkrishna.s.vasudevan made changes -
        Attachment HBASE-10883_6.patch [ 12638965 ]
        ramkrishna.s.vasudevan made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Anoop Sam John added a comment -

        Authorizations(List<String> labels)
        Validation from here can be for label one after other so that in the Exception msg, u can clearly say which Auth label is invalid. Same applicable to VisibilityController#createVisibilityLabelFilter
        We can just use VisibilityLabelsValidator#isValidLabel(byte[] label) which is already there and used by put ?

        throw new IllegalArgumentException("Authorizations cannot contain '(', ')' ,'&' ,'|', '!'"
        +          + " and cannot be empty :"+label);
        

        Error message can be bettter I think. This is invalid Auth label

        Show
        Anoop Sam John added a comment - Authorizations(List<String> labels) Validation from here can be for label one after other so that in the Exception msg, u can clearly say which Auth label is invalid. Same applicable to VisibilityController#createVisibilityLabelFilter We can just use VisibilityLabelsValidator#isValidLabel(byte[] label) which is already there and used by put ? throw new IllegalArgumentException( "Authorizations cannot contain '(', ')' ,'&' ,'|', '!'" + + " and cannot be empty :" +label); Error message can be bettter I think. This is invalid Auth label
        Hide
        ramkrishna.s.vasudevan added a comment -

        Ping to commit?

        Show
        ramkrishna.s.vasudevan added a comment - Ping to 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/12638532/HBASE-10883_5.patch
        against trunk revision .
        ATTACHMENT ID: 12638532

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

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

        +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 does not introduce any 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.procedure.TestZKProcedure

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//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/12638532/HBASE-10883_5.patch against trunk revision . ATTACHMENT ID: 12638532 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +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 does not introduce any 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.procedure.TestZKProcedure Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//console This message is automatically generated.
        ramkrishna.s.vasudevan made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        ramkrishna.s.vasudevan made changes -
        Attachment HBASE-10883_5.patch [ 12638532 ]
        Hide
        ramkrishna.s.vasudevan added a comment -

        Patch got stale with latest code.

        Show
        ramkrishna.s.vasudevan added a comment - Patch got stale with latest code.
        ramkrishna.s.vasudevan made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Assignee ramkrishna.s.vasudevan [ ram_krish ]
        Hide
        Hadoop QA added a comment -

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

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

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

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

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

        Sorry for the mistake. This patch works fine.

        Show
        ramkrishna.s.vasudevan added a comment - Sorry for the mistake. This patch works fine.
        ramkrishna.s.vasudevan 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/12638458/HBASE-10883_3.patch
        against trunk revision .
        ATTACHMENT ID: 12638458

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

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

        +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 does not introduce any 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.TestVisibilityLabels
        org.apache.hadoop.hbase.rest.TestScannersWithLabels
        org.apache.hadoop.hbase.mapreduce.TestImportTSVWithVisibilityLabels
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithACL
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsOpWithDifferentUsersNoACL
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDistributedLogReplay

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//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/12638458/HBASE-10883_3.patch against trunk revision . ATTACHMENT ID: 12638458 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +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 does not introduce any 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.TestVisibilityLabels org.apache.hadoop.hbase.rest.TestScannersWithLabels org.apache.hadoop.hbase.mapreduce.TestImportTSVWithVisibilityLabels org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithACL org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsOpWithDifferentUsersNoACL org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDistributedLogReplay Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//console This message is automatically generated.
        ramkrishna.s.vasudevan made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        ramkrishna.s.vasudevan made changes -
        Attachment HBASE-10883_3.patch [ 12638458 ]
        Hide
        ramkrishna.s.vasudevan added a comment -

        Updated patch.

        Show
        ramkrishna.s.vasudevan added a comment - Updated patch.
        ramkrishna.s.vasudevan made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Ted Yu added a comment -
        +  public static final boolean isValidLabel(List<String> labels) {
        

        The method can be package-private, right ?

           public Authorizations(List<String> labels) {
        +    if (!VisibilityLabelsValidator.isValidLabel(labels)) {
        

        Better include the label which is invalid in exception message.

        Show
        Ted Yu added a comment - + public static final boolean isValidLabel(List< String > labels) { The method can be package-private, right ? public Authorizations(List< String > labels) { + if (!VisibilityLabelsValidator.isValidLabel(labels)) { Better include the label which is invalid in exception message.
        ramkrishna.s.vasudevan made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        ramkrishna.s.vasudevan made changes -
        Attachment HBASE-10883_2.patch [ 12638196 ]
        Hide
        ramkrishna.s.vasudevan added a comment -

        Checks both on client and server side. The matcher code is moved to VisbililtyLabelsValidator.

        Show
        ramkrishna.s.vasudevan added a comment - Checks both on client and server side. The matcher code is moved to VisbililtyLabelsValidator.
        ramkrishna.s.vasudevan made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        ramkrishna.s.vasudevan added a comment -

        We better keep the validation at server side?

        I thought HConstants would help in client side validation. I thought of unifying but decided not to as I thought better to be client side, just on creation of Authorization object.

        Show
        ramkrishna.s.vasudevan added a comment - We better keep the validation at server side? I thought HConstants would help in client side validation. I thought of unifying but decided not to as I thought better to be client side, just on creation of Authorization object.
        Hide
        Andrew Purtell added a comment -

        Why not re-use the code for validating labels? Checking on the client is fine but the check has to happen on the server side as well since we can't guarantee a validating client (REST? Thrift?)

        Show
        Andrew Purtell added a comment - Why not re-use the code for validating labels? Checking on the client is fine but the check has to happen on the server side as well since we can't guarantee a validating client (REST? Thrift?)
        Hide
        Anoop Sam John added a comment -

        Why to add pattern and regex to HConstants?
        We better keep the validation at server side?
        And move it to VisibilityLabelsValidator? VisibilityLabelsValidator#isValidLabel(String)

        Show
        Anoop Sam John added a comment - Why to add pattern and regex to HConstants? We better keep the validation at server side? And move it to VisibilityLabelsValidator? VisibilityLabelsValidator#isValidLabel(String)
        Hide
        Hadoop QA added a comment -

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

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

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

        +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 does not introduce any 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/9157//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//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/12638051/HBASE-10883_1.patch against trunk revision . ATTACHMENT ID: 12638051 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +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 does not introduce any 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/9157//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//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/12638028/HBASE-10883.patch
        against trunk revision .
        ATTACHMENT ID: 12638028

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

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

        +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 1 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/9155//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//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/12638028/HBASE-10883.patch against trunk revision . ATTACHMENT ID: 12638028 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +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 1 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/9155//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//console This message is automatically generated.
        Hide
        Ashish Singhi added a comment -

        Minor nit.

         } catch (IllegalArgumentException e) {
        +      fail("Should have failed for -B");
        +    }
        

        Message should be something like "should not fail".

        Show
        Ashish Singhi added a comment - Minor nit. } catch (IllegalArgumentException e) { + fail( "Should have failed for -B" ); + } Message should be something like "should not fail".
        ramkrishna.s.vasudevan made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        ramkrishna.s.vasudevan made changes -
        Attachment HBASE-10883_1.patch [ 12638051 ]
        Hide
        ramkrishna.s.vasudevan added a comment -

        Now validation is sync with VisibilityLabelsValidator. Also checks for empty string or white space String.

        Show
        ramkrishna.s.vasudevan added a comment - Now validation is sync with VisibilityLabelsValidator. Also checks for empty string or white space String.
        ramkrishna.s.vasudevan made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Andrew Purtell added a comment -

        Well, you do have to change the current regexp somehow because it will match the empty string and that's not valid.

        Show
        Andrew Purtell added a comment - Well, you do have to change the current regexp somehow because it will match the empty string and that's not valid.
        Hide
        Andrew Purtell added a comment -
        +  private final String regex = "[A-Za-z_0-9]*";
        

        I think this should be

        +  private final String regex = "[A-Za-z\\_][A-Za-z0-9\\_\\-]*";
        

        but this is a minor thing, if you want the other then no problem.

        The regex should be precompiled since it may be applied often.

        Like Anoop says, we need to validate authorizations and labels the same way now.

        Show
        Andrew Purtell added a comment - + private final String regex = "[A-Za-z_0-9]*" ; I think this should be + private final String regex = "[A-Za-z\\_][A-Za-z0-9\\_\\-]*" ; but this is a minor thing, if you want the other then no problem. The regex should be precompiled since it may be applied often. Like Anoop says, we need to validate authorizations and labels the same way now.
        Hide
        Anoop Sam John added a comment -

        Referring VisibilityLabelsValidator we allow some special characters to be part of labels. So this check should contain those also.

        Show
        Anoop Sam John added a comment - Referring VisibilityLabelsValidator we allow some special characters to be part of labels. So this check should contain those also.
        ramkrishna.s.vasudevan made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        ramkrishna.s.vasudevan made changes -
        Attachment HBASE-10883.patch [ 12638028 ]
        Hide
        ramkrishna.s.vasudevan added a comment -

        Patch for trunk. Should apply on 0.98 also.
        Tested with shell also. Throws exception if any special characters are used.

        Show
        ramkrishna.s.vasudevan added a comment - Patch for trunk. Should apply on 0.98 also. Tested with shell also. Throws exception if any special characters are used.
        Andrew Purtell made changes -
        Field Original Value New Value
        Summary Restrict the universe of labels and authorizations to [A-Za-z0-9\_][A-Za-z0-9\_\-]* Restrict the universe of labels and authorizations
        Hide
        Andrew Purtell added a comment -

        I think we should use the common pattern for identifiers in various programming languages, [A-Za-z\_][A-Za-z0-9\_\-]*

        Show
        Andrew Purtell added a comment - I think we should use the common pattern for identifiers in various programming languages, [A-Za-z\_] [A-Za-z0-9\_\-] *
        Andrew Purtell created issue -

          People

          • Assignee:
            ramkrishna.s.vasudevan
            Reporter:
            Andrew Purtell
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development