HBase
  1. HBase
  2. HBASE-10173

Need HFile version check in security coprocessors

    Details

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

      Description

      Cell level visibility labels are stored as cell tags. So HFile V3 is the minimum version which can support this feature. Better to have a version check in VisibilityController. Some one using this CP but with any HFile version as V2, we can better throw error.

      1. HBASE-10173_partial.patch
        2 kB
        Anoop Sam John
      2. HBASE-10173.patch
        4 kB
        Anoop Sam John
      3. 10173.patch
        14 kB
        Andrew Purtell
      4. 10173.patch
        14 kB
        Andrew Purtell
      5. HBASE-10173_Addendum.patch
        2 kB
        Anoop Sam John

        Activity

        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 #25 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/25/)
        HBASE-10173 Need HFile version check in security coprocessors (anoopsamjohn: rev 1552727)

        • /hbase/branches/0.98/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 #25 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/25/ ) HBASE-10173 Need HFile version check in security coprocessors (anoopsamjohn: rev 1552727) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-0.98 #28 (See https://builds.apache.org/job/HBase-0.98/28/)
        HBASE-10173 Need HFile version check in security coprocessors (anoopsamjohn: rev 1552727)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-0.98 #28 (See https://builds.apache.org/job/HBase-0.98/28/ ) HBASE-10173 Need HFile version check in security coprocessors (anoopsamjohn: rev 1552727) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #14 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/14/)
        HBASE-10173 Need HFile version check in security coprocessors (anoopsamjohn: rev 1552728)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
          HBASE-10173. Need HFile version check in security coprocessors (apurtell: rev 1552503)
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/rest/TestScannersWithLabels.java
        • /hbase/trunk/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandlerWithLabels.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #14 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/14/ ) HBASE-10173 Need HFile version check in security coprocessors (anoopsamjohn: rev 1552728) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java HBASE-10173 . Need HFile version check in security coprocessors (apurtell: rev 1552503) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/rest/TestScannersWithLabels.java /hbase/trunk/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandlerWithLabels.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #4742 (See https://builds.apache.org/job/HBase-TRUNK/4742/)
        HBASE-10173 Need HFile version check in security coprocessors (anoopsamjohn: rev 1552728)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #4742 (See https://builds.apache.org/job/HBase-TRUNK/4742/ ) HBASE-10173 Need HFile version check in security coprocessors (anoopsamjohn: rev 1552728) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        Hide
        Anoop Sam John added a comment -

        Not exactly Ram...
        initialize() will be called on the CP object created for the acl region. In case of other regions this is not called and the boolean is always false
        So moving this to start() will make sure that this is done for all regions

        Show
        Anoop Sam John added a comment - Not exactly Ram... initialize() will be called on the CP object created for the acl region. In case of other regions this is not called and the boolean is always false So moving this to start() will make sure that this is done for all regions
        Hide
        Anoop Sam John added a comment -

        Committed the addendum to 0.98 and trunk.

        Show
        Anoop Sam John added a comment - Committed the addendum to 0.98 and trunk.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Sorry. Took some time to understand. So the ACL region came in after some other region had come first. So we are moving the check to start(). Correct?

        Show
        ramkrishna.s.vasudevan added a comment - Sorry. Took some time to understand. So the ACL region came in after some other region had come first. So we are moving the check to start(). Correct?
        Hide
        Andrew Purtell added a comment -

        +1 on addendum

        Show
        Andrew Purtell added a comment - +1 on addendum
        Hide
        Andrew Purtell added a comment -

        Yep, annoying this didn't show up locally.

        Show
        Andrew Purtell added a comment - Yep, annoying this didn't show up locally.
        Hide
        Anoop Sam John added a comment -

        Addendum to fix test failure .
        Andrew Purtell , ramkrishna.s.vasudevan what do you guys say?

        Show
        Anoop Sam John added a comment - Addendum to fix test failure . Andrew Purtell , ramkrishna.s.vasudevan what do you guys say?
        Show
        Anoop Sam John added a comment - https://builds.apache.org/job/PreCommit-HBASE-Build/8241//testReport/org.apache.hadoop.hbase.security.access/TestAccessController/testCellPermissions/ Failure is related to commit.. I can give a small addendum here.
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #23 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/23/)
        HBASE-10173. Need HFile version check in security coprocessors (apurtell: rev 1552504)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/rest/TestScannersWithLabels.java
        • /hbase/branches/0.98/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandlerWithLabels.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #23 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/23/ ) HBASE-10173 . Need HFile version check in security coprocessors (apurtell: rev 1552504) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/rest/TestScannersWithLabels.java /hbase/branches/0.98/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandlerWithLabels.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-TRUNK #4741 (See https://builds.apache.org/job/HBase-TRUNK/4741/)
        HBASE-10173. Need HFile version check in security coprocessors (apurtell: rev 1552503)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/rest/TestScannersWithLabels.java
        • /hbase/trunk/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandlerWithLabels.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-TRUNK #4741 (See https://builds.apache.org/job/HBase-TRUNK/4741/ ) HBASE-10173 . Need HFile version check in security coprocessors (apurtell: rev 1552503) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/rest/TestScannersWithLabels.java /hbase/trunk/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandlerWithLabels.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.98 #26 (See https://builds.apache.org/job/HBase-0.98/26/)
        HBASE-10173. Need HFile version check in security coprocessors (apurtell: rev 1552504)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/rest/TestScannersWithLabels.java
        • /hbase/branches/0.98/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandlerWithLabels.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.98 #26 (See https://builds.apache.org/job/HBase-0.98/26/ ) HBASE-10173 . Need HFile version check in security coprocessors (apurtell: rev 1552504) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/rest/TestScannersWithLabels.java /hbase/branches/0.98/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift2/TestThriftHBaseServiceHandlerWithLabels.java
        Hide
        Andrew Purtell added a comment -

        Thanks anyway Anoop.

        Show
        Andrew Purtell added a comment - Thanks anyway Anoop.
        Hide
        Anoop Sam John added a comment -

        Oh I am late.. Still +1 Thanks Andy..

        Show
        Anoop Sam John added a comment - Oh I am late.. Still +1 Thanks Andy..
        Hide
        Andrew Purtell added a comment -

        Committed to trunk and 0.98

        Show
        Andrew Purtell added a comment - Committed to trunk and 0.98
        Hide
        Andrew Purtell added a comment -

        Hi ramkrishna.s.vasudevan, thanks for taking a look!

        Show
        Andrew Purtell added a comment - Hi ramkrishna.s.vasudevan , thanks for taking a look!
        Hide
        ramkrishna.s.vasudevan added a comment -

        Patch looks good to me. +1.

        Show
        ramkrishna.s.vasudevan added a comment - Patch looks good to me. +1.
        Hide
        Andrew Purtell added a comment -

        Can't seem to get a HadoopQA run. Test suite passes locally. What do you think Anoop Sam John?

        Show
        Andrew Purtell added a comment - Can't seem to get a HadoopQA run. Test suite passes locally. What do you think Anoop Sam John ?
        Hide
        Andrew Purtell added a comment -

        Thinking about it, we should throw DNRIOEs if the user sends an op with cell ACLs attached rather than simply warn in the server logs. Retesting locally first.

        Show
        Andrew Purtell added a comment - Thinking about it, we should throw DNRIOEs if the user sends an op with cell ACLs attached rather than simply warn in the server logs. Retesting locally first.
        Hide
        Andrew Purtell added a comment -

        Still working on this. This has been good for catching the unit tests that fail to set hfile.format.version to 3 where required. Almost ready.

        Show
        Andrew Purtell added a comment - Still working on this. This has been good for catching the unit tests that fail to set hfile.format.version to 3 where required. Almost ready.
        Hide
        Andrew Purtell added a comment -

        Ok, I'll do this one tomorrow. Thanks Anoop.

        Show
        Andrew Purtell added a comment - Ok, I'll do this one tomorrow. Thanks Anoop.
        Hide
        Anoop Sam John added a comment -

        But using V3 is not mandatory for old style table/CF perm checking.

        Same I was also thinking.. That is how we should make it.. This patch doing the simple check in AccessController which makes it need HFile V3
        Andy, Pls feel free to take the issue with the fix you would like to have in AccessController.

        Show
        Anoop Sam John added a comment - But using V3 is not mandatory for old style table/CF perm checking. Same I was also thinking.. That is how we should make it.. This patch doing the simple check in AccessController which makes it need HFile V3 Andy, Pls feel free to take the issue with the fix you would like to have in AccessController.
        Hide
        Andrew Purtell added a comment -

        In 0.98 HFile V2 is the default version. If used want to use old way of AccessControl (Not cell level) still he has to make the version to V3

        For cell level, yes V3 is needed. Otherwise it won't work. But using V3 is not mandatory for old style table/CF perm checking.

        Show
        Andrew Purtell added a comment - In 0.98 HFile V2 is the default version. If used want to use old way of AccessControl (Not cell level) still he has to make the version to V3 For cell level, yes V3 is needed. Otherwise it won't work. But using V3 is not mandatory for old style table/CF perm checking.
        Hide
        Anoop Sam John added a comment -

        Same change in AccessController.. Just copy paste Attaching that also here Andy... Pls feel free to take the issue if you wish to do it in some other way (mainly in AccessController)

        Show
        Anoop Sam John added a comment - Same change in AccessController.. Just copy paste Attaching that also here Andy... Pls feel free to take the issue if you wish to do it in some other way (mainly in AccessController)
        Hide
        Anoop Sam John added a comment -

        I can take this Anoop, let me know.

        NP.. Pls go ahead Andy.

        I am just attaching the patch I had initially which checks only in VisibilityController.

        We need this in the AC too after HBASE-7662

        In 0.98 HFile V2 is the default version. If used want to use old way of AccessControl (Not cell level) still he has to make the version to V3? I think the changes happened with AccessController demands us to do so. We can document this change then, some where in 0.98 release contents.

        Show
        Anoop Sam John added a comment - I can take this Anoop, let me know. NP.. Pls go ahead Andy. I am just attaching the patch I had initially which checks only in VisibilityController. We need this in the AC too after HBASE-7662 In 0.98 HFile V2 is the default version. If used want to use old way of AccessControl (Not cell level) still he has to make the version to V3? I think the changes happened with AccessController demands us to do so. We can document this change then, some where in 0.98 release contents.
        Hide
        Andrew Purtell added a comment -

        Raising to critical so it gets in to the RC. I can take this Anoop, let me know.

        Show
        Andrew Purtell added a comment - Raising to critical so it gets in to the RC. I can take this Anoop, let me know.
        Hide
        Andrew Purtell added a comment -

        Agree it can help avoid user confusion and reduce troubleshooting time.

        We need this in the AC too after HBASE-7662

        Show
        Andrew Purtell added a comment - Agree it can help avoid user confusion and reduce troubleshooting time. We need this in the AC too after HBASE-7662

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development