Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.99.0, 0.98.2
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      Prior to 0.98.0 if a user was not granted access to a column family or partial access (qualifier grants), then the AccessController would immediately throw back an AccessDeniedException. This behavior was changed in 0.98.0 to allow cell level ACLs to grant exceptional access. The user will no longer see an exception; instead the scanner will return result sets only including cells which grant exceptional access. If no such cell level grants are made, then the scanner will return the empty result set.

      This change introduces a configuration setting which restores the pre-0.98.0 behavior. It can be set in the site file for global effect, or per table using HTableDescriptor#setConfiguration. This setting is AccessControlConstants.CF_ATTRIBUTE_EARLY_OUT ("hbase.security.access.early_out"), a boolean. Set to "true" for backwards compatible behavior. As a consequence if there are no grants at the CF level then an AccessDeniedException will be thrown immediately, and cell ACLs will be ignored, unless the cell-first ACL evaluation strategy is used (toggled via Query#setACLStrategy).

      This change also repairs a defect related to audit logging. In all cases, compatible behavior or not, indications of successful or failed access attempts will be logged.
      Show
      Prior to 0.98.0 if a user was not granted access to a column family or partial access (qualifier grants), then the AccessController would immediately throw back an AccessDeniedException. This behavior was changed in 0.98.0 to allow cell level ACLs to grant exceptional access. The user will no longer see an exception; instead the scanner will return result sets only including cells which grant exceptional access. If no such cell level grants are made, then the scanner will return the empty result set. This change introduces a configuration setting which restores the pre-0.98.0 behavior. It can be set in the site file for global effect, or per table using HTableDescriptor#setConfiguration. This setting is AccessControlConstants.CF_ATTRIBUTE_EARLY_OUT ("hbase.security.access.early_out"), a boolean. Set to "true" for backwards compatible behavior. As a consequence if there are no grants at the CF level then an AccessDeniedException will be thrown immediately, and cell ACLs will be ignored, unless the cell-first ACL evaluation strategy is used (toggled via Query#setACLStrategy). This change also repairs a defect related to audit logging. In all cases, compatible behavior or not, indications of successful or failed access attempts will be logged.

      Description

      See parent for the whole story.

      For 0.98, to start, just put back the early out that was removed in 0.98.0 and allow it to be overridden with a table attribute.

      1. HBASE-11077.patch
        65 kB
        Andrew Purtell
      2. HBASE-11077.patch
        65 kB
        Andrew Purtell
      3. HBASE-11077.patch
        93 kB
        Andrew Purtell
      4. HBASE-11077.patch
        92 kB
        Andrew Purtell
      5. HBASE-11077.patch
        91 kB
        Andrew Purtell
      6. HBASE-11077-0.98.patch
        92 kB
        Andrew Purtell

        Activity

        Hide
        Andrew Purtell added a comment -

        Make this critical for 0.98.2 since it's important and I have a patch ready to go modulo a couple of new unit tests.

        Show
        Andrew Purtell added a comment - Make this critical for 0.98.2 since it's important and I have a patch ready to go modulo a couple of new unit tests.
        Hide
        Andrew Purtell added a comment -

        The changes I would like to make in the short term. Cleaned up some code, added comments. Working on a couple of new tests.

        See also https://reviews.apache.org/r/20855/

        Show
        Andrew Purtell added a comment - The changes I would like to make in the short term. Cleaned up some code, added comments. Working on a couple of new tests. See also https://reviews.apache.org/r/20855/
        Hide
        Andrew Purtell added a comment -

        Latest patch includes new tests that verify the backwards compatible behavior toggle in HTableDescriptor and "cell first" ACL evaluation strategy under both regimes. All AccessController tests pass.

        Show
        Andrew Purtell added a comment - Latest patch includes new tests that verify the backwards compatible behavior toggle in HTableDescriptor and "cell first" ACL evaluation strategy under both regimes. All AccessController tests pass.
        Hide
        Andrew Purtell added a comment -

        Rolling the 0.98.2 RC0 tomorrow. Review would be very much welcomed because I think this is ready to go (modulo addressing review comments if any) and so will otherwise commit this tomorrow for the RC.

        Show
        Andrew Purtell added a comment - Rolling the 0.98.2 RC0 tomorrow. Review would be very much welcomed because I think this is ready to go (modulo addressing review comments if any) and so will otherwise commit this tomorrow for the RC.
        Hide
        stack added a comment -

        Why undo these as static finals? That is passing a Strategy instead?

        + String EXEC_PERMISSION_CHECKS_KEY = "hbase.security.exec.permission.checks";

        What is default for the above?

        You want to do this before commit?

        + // XXX: Compare in place, don't clone

        I skimmed the patch. Looks fine Andy. Nice test. Release note.

        Show
        stack added a comment - Why undo these as static finals? That is passing a Strategy instead? + String EXEC_PERMISSION_CHECKS_KEY = "hbase.security.exec.permission.checks"; What is default for the above? You want to do this before commit? + // XXX: Compare in place, don't clone I skimmed the patch. Looks fine Andy. Nice test. Release note.
        Hide
        Andrew Purtell added a comment - - edited

        Why undo these as static finals?

        As far as I know all constants in an interface are static final by default. If that is wrong I will put them back.

        + String EXEC_PERMISSION_CHECKS_KEY = "hbase.security.exec.permission.checks";
        What is default for the above?

        Defined right below it, but this was not introduced by this patch. It shows up in the diff because of the unnecessary keyword removals I made. I could leave out that minor cleanup to reduce noise. ?

        You want to do this before commit?

        // XXX: Compare in place, don't clone
        

        We don't have API for it in the AccessController. Many changes in many places will be required. I think this is in scope of HBASE-7123

        Show
        Andrew Purtell added a comment - - edited Why undo these as static finals? As far as I know all constants in an interface are static final by default. If that is wrong I will put them back. + String EXEC_PERMISSION_CHECKS_KEY = "hbase.security.exec.permission.checks"; What is default for the above? Defined right below it, but this was not introduced by this patch. It shows up in the diff because of the unnecessary keyword removals I made. I could leave out that minor cleanup to reduce noise. ? You want to do this before commit? // XXX: Compare in place, don't clone We don't have API for it in the AccessController. Many changes in many places will be required. I think this is in scope of HBASE-7123
        Hide
        Andrew Purtell added a comment -

        Anyway I went ahead and undid that bit of editorial involving interface AccessControlConstants. Latest patch is what I plan to commit. Let's see if HadoopQA will come around.

        Show
        Andrew Purtell added a comment - Anyway I went ahead and undid that bit of editorial involving interface AccessControlConstants. Latest patch is what I plan to commit. Let's see if HadoopQA will come around.
        Hide
        Andrew Purtell added a comment -

        Added release note

        Show
        Andrew Purtell added a comment - Added release note
        Hide
        Andrew Purtell added a comment -

        Ping Enis Soztutar, I know this is an area of concern for you. Any concerns with this change before it goes in and we have a RC?

        Show
        Andrew Purtell added a comment - Ping Enis Soztutar , I know this is an area of concern for you. Any concerns with this change before it goes in and we have a RC?
        Hide
        Andrew Purtell added a comment -

        Going to commit in a couple of hours

        Show
        Andrew Purtell added a comment - Going to commit in a couple of hours
        Hide
        Enis Soztutar added a comment -

        Sorry i missed this, was following the parent jira.

        This setting is AccessControlConstants.CF_ATTRIBUTE_EARLY_OUT ("hbase.security.access.early_out"), a boolean. Set to "true" for backwards compatible behavior.

        I think early_out should be set to true by default, so that it is least surprise to the admin. Otherwise, she has to know about this particular configuration option, and turn this on. In the other case, where you would not have perm to table, but some specific cells, then it is fine to require and explicit parameter. It should not be a common use case.
        Did you stop pursuing the READ_INVISIBLE priv aproach?

        Show
        Enis Soztutar added a comment - Sorry i missed this, was following the parent jira. This setting is AccessControlConstants.CF_ATTRIBUTE_EARLY_OUT ("hbase.security.access.early_out"), a boolean. Set to "true" for backwards compatible behavior. I think early_out should be set to true by default, so that it is least surprise to the admin. Otherwise, she has to know about this particular configuration option, and turn this on. In the other case, where you would not have perm to table, but some specific cells, then it is fine to require and explicit parameter. It should not be a common use case. Did you stop pursuing the READ_INVISIBLE priv aproach?
        Hide
        Andrew Purtell added a comment - - edited

        I think early_out should be set to true by default, so that it is least surprise to the admin.

        Then we break compatibility from 0.98.1 to 0.98.2, in that default behavior prior to 0.98.2 in the 0.98 release line is quite different. And, unfortunately cell ACLs would become largely useless, unless the admin research the feature and flip the attribute to "false", because when we early out at CF checks to retain pre-0.98 behavior the cell ACLs that would otherwise grant exceptional access won't be visited, unless using the cell-first strategy, which has the drawback of requiring the cell grant access.

        Did you stop pursuing the READ_INVISIBLE priv aproach?

        Check out the other subtask and let's figure out what makes sense for 0.99+.

        Edits: Clarity, sorry for the multiple changes.

        Show
        Andrew Purtell added a comment - - edited I think early_out should be set to true by default, so that it is least surprise to the admin. Then we break compatibility from 0.98.1 to 0.98.2, in that default behavior prior to 0.98.2 in the 0.98 release line is quite different. And, unfortunately cell ACLs would become largely useless, unless the admin research the feature and flip the attribute to "false", because when we early out at CF checks to retain pre-0.98 behavior the cell ACLs that would otherwise grant exceptional access won't be visited, unless using the cell-first strategy, which has the drawback of requiring the cell grant access. Did you stop pursuing the READ_INVISIBLE priv aproach? Check out the other subtask and let's figure out what makes sense for 0.99+. Edits: Clarity, sorry for the multiple changes.
        Hide
        Andrew Purtell added a comment -

        There was a reason we changed the behavior when introducing cell ACLs. The parent mentions the issues involved, and the release notes on this issue also.

        Show
        Andrew Purtell added a comment - There was a reason we changed the behavior when introducing cell ACLs. The parent mentions the issues involved, and the release notes on this issue also.
        Hide
        Enis Soztutar added a comment -

        Then we break compatibility from 0.98.1 to 0.98.2, in that default behavior prior to 0.98.2 in the 0.98 release line is quite different

        Maybe default to false on 0.98, but true on trunk.

        unfortunately cell ACLs would become largely useless, unless the admin research the feature and flip the attribute to "false", because when we early out at CF checks to retain pre-0.98 behavior the cell ACLs that would otherwise grant exceptional access won't be visited, unless using the cell-first strategy

        Surely we do not want to make the model complex, but at the same time allow both of the use cases. If we have table privs + config option + per-operation cell-first strategy it is already three dimensions. Can we reduce that to at least two? Can we get away with per-operation strategy or the config option?

        Show
        Enis Soztutar added a comment - Then we break compatibility from 0.98.1 to 0.98.2, in that default behavior prior to 0.98.2 in the 0.98 release line is quite different Maybe default to false on 0.98, but true on trunk. unfortunately cell ACLs would become largely useless, unless the admin research the feature and flip the attribute to "false", because when we early out at CF checks to retain pre-0.98 behavior the cell ACLs that would otherwise grant exceptional access won't be visited, unless using the cell-first strategy Surely we do not want to make the model complex, but at the same time allow both of the use cases. If we have table privs + config option + per-operation cell-first strategy it is already three dimensions. Can we reduce that to at least two? Can we get away with per-operation strategy or the config option?
        Hide
        Andrew Purtell added a comment -

        I added another subtask to the parent for capturing the configuration space and nuances (with use case examples) into the security section of the manual as HBASE-11103

        Show
        Andrew Purtell added a comment - I added another subtask to the parent for capturing the configuration space and nuances (with use case examples) into the security section of the manual as HBASE-11103
        Hide
        Andrew Purtell added a comment -

        Maybe default to false on 0.98, but true on trunk.

        Ok. It's a discontinuity on the one hand, but 0.98 is explicitly a laboratory for this stuff. I will do that and add a patch here for the 0.98 version unless objection.

        Can we reduce that to at least two?

        If there isn't any take up of the per-operation strategy toggle, we could drop it in 0.99+ ?

        Show
        Andrew Purtell added a comment - Maybe default to false on 0.98, but true on trunk. Ok. It's a discontinuity on the one hand, but 0.98 is explicitly a laboratory for this stuff. I will do that and add a patch here for the 0.98 version unless objection. Can we reduce that to at least two? If there isn't any take up of the per-operation strategy toggle, we could drop it in 0.99+ ?
        Hide
        Enis Soztutar added a comment -

        I will do that and add a patch here for the 0.98 version unless objection.

        Sounds good.

        If there isn't any take up of the per-operation strategy toggle, we could drop it in 0.99+ ?

        It seems to me that the strategy should be a table level property (a part of the param this patch adds) or baked into the priviledge (like the READ_INVISIBLE) we were talking about.

        Another argument for defaulting to a behavior for disallowing scans without read perm by default is that, we do not want a user to DOS the table by launching scanners against it although with no read permission.

        Show
        Enis Soztutar added a comment - I will do that and add a patch here for the 0.98 version unless objection. Sounds good. If there isn't any take up of the per-operation strategy toggle, we could drop it in 0.99+ ? It seems to me that the strategy should be a table level property (a part of the param this patch adds) or baked into the priviledge (like the READ_INVISIBLE) we were talking about. Another argument for defaulting to a behavior for disallowing scans without read perm by default is that, we do not want a user to DOS the table by launching scanners against it although with no read permission.
        Hide
        Andrew Purtell added a comment -

        What I committed to trunk and 0.98. Thanks for the reviews and feedback all!

        Show
        Andrew Purtell added a comment - What I committed to trunk and 0.98. Thanks for the reviews and feedback all!
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #5126 (See https://builds.apache.org/job/HBase-TRUNK/5126/)
        HBASE-11077 [AccessController] Restore compatible early-out access denial (apurtell: rev 1591524)

        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlConstants.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.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/access/AuthResult.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/SecureTestUtil.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControlFilter.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestScanEarlyTermination.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #5126 (See https://builds.apache.org/job/HBase-TRUNK/5126/ ) HBASE-11077 [AccessController] Restore compatible early-out access denial (apurtell: rev 1591524) /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlConstants.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.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/access/AuthResult.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/SecureTestUtil.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControlFilter.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestScanEarlyTermination.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #286 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/286/)
        HBASE-11077 [AccessController] Restore compatible early-out access denial (0.98 version) (apurtell: rev 1591525)

        • /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlConstants.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.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/access/AuthResult.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/SecureTestUtil.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControlFilter.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestScanEarlyTermination.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #286 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/286/ ) HBASE-11077 [AccessController] Restore compatible early-out access denial (0.98 version) (apurtell: rev 1591525) /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlConstants.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.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/access/AuthResult.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/SecureTestUtil.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControlFilter.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestScanEarlyTermination.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.98 #301 (See https://builds.apache.org/job/HBase-0.98/301/)
        HBASE-11077 [AccessController] Restore compatible early-out access denial (0.98 version) (apurtell: rev 1591525)

        • /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlConstants.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.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/access/AuthResult.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/SecureTestUtil.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControlFilter.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestScanEarlyTermination.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.98 #301 (See https://builds.apache.org/job/HBase-0.98/301/ ) HBASE-11077 [AccessController] Restore compatible early-out access denial (0.98 version) (apurtell: rev 1591525) /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlConstants.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.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/access/AuthResult.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/SecureTestUtil.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControlFilter.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLWithMultipleVersions.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCellACLs.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestScanEarlyTermination.java
        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.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development