[ https://issues.apache.org/jira/browse/HBASE-3025?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13115708#comment-13115708 ]
jirapos...@reviews.apache.org commented on HBASE-3025: ------------------------------------------------------ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2041/#review2077 ----------------------------------------------------------- Looks good. The majority of my comments have to do with inconsistent logging practice. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessControlFilter.java <https://reviews.apache.org/r/2041/#comment4718> Could be stated better. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessControlLists.java <https://reviews.apache.org/r/2041/#comment4719> No. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessControlLists.java <https://reviews.apache.org/r/2041/#comment4720> Comment needs updating. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4721> Can we make this 1? security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4775> Debug logging should go to LOG not AUDITLOG security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4782> Should be INFO or TRACE level? TRACE makes more sense to me. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4776> Debug logging should go to LOG not AUDITLOG security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4779> Should be INFO or TRACE level? TRACE makes more sense to me. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4795> Should something go to AUDITLOG here? security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4797> Should hasFamilyQualifierPermission log to AUDITLOG? It is used in places to make decisions -- an exception is thrown directly or not. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4783> Another one of these was sent to AUDITLOG above. Do the same here? Should be INFO or TRACE level? TRACE makes more sense to me. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4786> Ultimately users should be allowed to enable or disable their own tables, but only after such operations don't carry as much systemic risk as they do currently. In that case, CREATE permission and an ownership check could follow the test for ADMIN permission. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4787> As above security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4791> Should be logged with ERROR? security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4799> Would it be clearer then to call permissionGranted() something like hasColumnsPermission() ? Just a random thought. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4803> Should this go to AUDITLOG? At INFO or TRACE level? My preference is TRACE. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java <https://reviews.apache.org/r/2041/#comment4804> Should this go to AUDITLOG? At INFO or TRACE level? My preference is TRACE. security/src/main/java/org/apache/hadoop/hbase/security/rbac/Permission.java <https://reviews.apache.org/r/2041/#comment4807> What if instead we check for version 0 and throw an IllegalArgumentException if so? Technically, it is an invalid request if it contains an unrecognizable action code. Skipping this check if version > 0 would be a way to handle new perms while not accepting incorrect input otherwise. security/src/main/java/org/apache/hadoop/hbase/security/rbac/TableAuthManager.java <https://reviews.apache.org/r/2041/#comment4813> Maybe we can call this ".auth."? We don't really have an RBAC implementation yet. Likewise for the package name for all of this stuff? Just a random thought. security/src/main/java/org/apache/hadoop/hbase/security/rbac/TableAuthManager.java <https://reviews.apache.org/r/2041/#comment4815> Isn't this an error? security/src/main/java/org/apache/hadoop/hbase/security/rbac/TableAuthManager.java <https://reviews.apache.org/r/2041/#comment4816> Should be at DEBUG level security/src/main/java/org/apache/hadoop/hbase/security/rbac/ZKPermissionWatcher.java <https://reviews.apache.org/r/2041/#comment4814> I wonder if there is some way we can check if a secure variant of ZooKeeper is running, and refuse to initialize if not. security/src/main/java/org/apache/hadoop/hbase/security/rbac/ZKPermissionWatcher.java <https://reviews.apache.org/r/2041/#comment4823> Shouldn't we propagate ZK exceptions upward? or at least convert them to IOE and throw those? Otherwise the permission cache is silently at risk of being out of sync with the ACL table. The safest thing to do is force a region close by bubbling up an exception from the coprocessor. This assumes that the coprocessor framework or regionserver will trigger a region close if it receives an unhandled exception from coprocessor code, and that this won't down the whole regionserver. security/src/main/java/org/apache/hadoop/hbase/security/rbac/ZKPermissionWatcher.java <https://reviews.apache.org/r/2041/#comment4825> As above security/src/main/java/org/apache/hadoop/hbase/security/rbac/ZKPermissionWatcher.java <https://reviews.apache.org/r/2041/#comment4826> As above security/src/main/java/org/apache/hadoop/hbase/security/rbac/ZKPermissionWatcher.java <https://reviews.apache.org/r/2041/#comment4827> As above src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java <https://reviews.apache.org/r/2041/#comment4829> Yes I agree this makes sense here, for convenience in setting ownership through the existing alter functionality. - Andrew On 2011-09-23 19:14:20, Gary Helmling wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/2041/ bq. ----------------------------------------------------------- bq. bq. (Updated 2011-09-23 19:14:20) bq. bq. bq. Review request for hbase. bq. bq. bq. Summary bq. ------- bq. bq. This patch implements access control list based authorization of HBase operations. The patch depends on the currently posted patch for HBASE-2742 (secure RPC engine). bq. bq. Key parts of the implementation are: bq. bq. * AccessControlLists - encapsulates storage of permission grants in a metadata table ("_acl_"). This differs from previous implementation where the ".META." table was used to store permissions. bq. bq. * AccessController - bq. - implements MasterObserver and RegionObserver, performing authorization checks in each of the preXXX() hooks. If authorization fails, an AccessDeniedException is thrown. bq. - implements AccessControllerProtocol as a coprocessor endpoint to provide RPC methods for granting, revoking and listing permissions. bq. bq. * ZKPermissionWatcher (and TableAuthManager) - synchronizes ACL entries and updates throughout the cluster nodes using ZK. ACL entries are stored in per-table znodes as /hbase/acl/tablename. bq. bq. * Additional ruby shell scripts providing the "grant", "revoke" and "user_permission" commands bq. bq. * Support for a new OWNER attribute in HTableDescriptor. I could separate out this change into a new JIRA for discussion, but I don't see it as currently useful outside of security. Alternately, I could handle the OWNER attribute completely in AccessController without changing HTD, but that would make interaction via hbase shell a bit uglier. bq. bq. bq. This addresses bug HBASE-3025. bq. https://issues.apache.org/jira/browse/HBASE-3025 bq. bq. bq. Diffs bq. ----- bq. bq. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessControlFilter.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessControlLists.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessControllerProtocol.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/rbac/Permission.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/rbac/TableAuthManager.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/rbac/TablePermission.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/rbac/UserPermission.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/rbac/ZKPermissionWatcher.java PRE-CREATION bq. security/src/test/java/org/apache/hadoop/hbase/security/rbac/SecureTestUtil.java PRE-CREATION bq. security/src/test/java/org/apache/hadoop/hbase/security/rbac/TestAccessControlFilter.java PRE-CREATION bq. security/src/test/java/org/apache/hadoop/hbase/security/rbac/TestAccessController.java PRE-CREATION bq. security/src/test/java/org/apache/hadoop/hbase/security/rbac/TestTablePermissions.java PRE-CREATION bq. security/src/test/java/org/apache/hadoop/hbase/security/rbac/TestZKPermissionsWatcher.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java 46a1a3d bq. src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 699a5f5 bq. src/main/resources/hbase-default.xml 2c8f44b bq. src/main/ruby/hbase.rb 4d27191 bq. src/main/ruby/hbase/admin.rb b244ffe bq. src/main/ruby/hbase/hbase.rb beb2450 bq. src/main/ruby/hbase/security.rb PRE-CREATION bq. src/main/ruby/shell.rb 9a47600 bq. src/main/ruby/shell/commands.rb a352c2e bq. src/main/ruby/shell/commands/grant.rb PRE-CREATION bq. src/main/ruby/shell/commands/revoke.rb PRE-CREATION bq. src/main/ruby/shell/commands/table_permission.rb PRE-CREATION bq. src/main/ruby/shell/commands/user_permission.rb PRE-CREATION bq. src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 4d7ee22 bq. bq. Diff: https://reviews.apache.org/r/2041/diff bq. bq. bq. Testing bq. ------- bq. bq. bq. Thanks, bq. bq. Gary bq. bq. > Coprocessor based simple access control > --------------------------------------- > > Key: HBASE-3025 > URL: https://issues.apache.org/jira/browse/HBASE-3025 > Project: HBase > Issue Type: Sub-task > Components: coprocessors > Reporter: Andrew Purtell > Attachments: HBASE-3025.1.patch, HBASE-3025.2011-02-01.patch > > > Thanks for the clarification Jeff which reminds me to edit this issue. > Goals of this issue > # Client access to HBase is authenticated > # User data is private unless access has been granted > # Access to data can be granted at a table or per column family basis. > Non-Goals of this issue > The following items will be left out of the initial implementation for > simplicity: > # Row-level or per value (cell) This would require broader changes for > storing the ACLs inline with rows. It's still a future goal, but would slow > down the initial implementation considerably. > # Push down of file ownership to HDFS While table ownership seems like a > useful construct to start with (at least to lay the groundwork for future > changes), making HBase act as table owners when interacting with HDFS would > require more changes. In additional, while HDFS file ownership would make > applying quotas easy, and possibly make bulk imports more straightforward, > it's not clean it would offer a more secure setup. We'll leave this to > evaluate in a later phase. > # HBase managed "roles" as collections of permissions We will not model > "roles" internally in HBase to begin with. We will instead allow group names > to be granted permissions, which will allow some external modeling of roles > via group memberships. Groups will be created and manipulated externally to > HBase. > While the assignment of permissions to roles and roles to users (or other > roles) allows a great deal of flexibility in security policy, it would add > complexity to the initial implementation. > After the initial implementation, which will appear on this issue, we will > evaluate the addition of role definitions internal to HBase in a new JIRA. In > this scheme, administrators could assign permissions specifying HDFS groups, > and additionally HBase roles. HBase roles would be created and manipulated > internally to HBase, and would appear distinct from HDFS groups via some > syntactic sugar. HBase role definitions will be allowed to reference other > HBase role definitions. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira