hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-3025) Coprocessor based simple access control
Date Tue, 01 Nov 2011 00:19:33 GMT

    [ https://issues.apache.org/jira/browse/HBASE-3025?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13140719#comment-13140719
] 

jiraposter@reviews.apache.org commented on HBASE-3025:
------------------------------------------------------



bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/TableAuthManager.java,
line 84
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45407#file45407line84>
bq.  >
bq.  >     Isn't this an error?
bq.  
bq.  Gary Helmling wrote:
bq.      Yes, and in this context a pretty bad one, as it probably means region server initiated
RPCs won't work or will be denied.  We should probably let the IOE escape here...
bq.  
bq.  Andrew Purtell wrote:
bq.      Agree.

Letting the IOException from getCurrent() escape and throwing an exception if the returned
user is null.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/TableAuthManager.java,
line 113
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45407#file45407line113>
bq.  >
bq.  >     Should be at DEBUG level

Done.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessControlFilter.java,
line 37
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45402#file45402line37>
bq.  >
bq.  >     Could be stated better.

Fixed.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessControlLists.java,
line 98
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45403#file45403line98>
bq.  >
bq.  >     Comment needs updating.
bq.  >

Fixed.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java,
line 98
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line98>
bq.  >
bq.  >     Can we make this 1?
bq.  
bq.  Gary Helmling wrote:
bq.      sure

Done.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java,
line 192
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line192>
bq.  >
bq.  >     Debug logging should go to LOG not AUDITLOG
bq.  
bq.  Gary Helmling wrote:
bq.      The idea was that all authorization decisions should be separated into audit log.
 Here we're allowing access, so AUDITLOG seemed to make sense.  I agree that this still needs
to be cleaned up a lot.  Maybe all audit logging should be done up in requirePermission()
with authorization result?  At the very least we need a consistent format and consistent logging
levels for messages (trace, right?).
bq.  
bq.  Andrew Purtell wrote:
bq.      > Maybe all audit logging should be done up in requirePermission() with authorization
result?
bq.      
bq.      Sounds good.
bq.      
bq.      > At the very least we need a consistent format and consistent logging levels
for messages (trace, right?).
bq.      
bq.      I'd argue for TRACE

Reworked the audit logging to happen in requirePermission(), so we get a single log message
per auth check indicating success or failure, with a more consistent format.  Result is logged
to AUDITLOG at trace level.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java,
line 366
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line366>
bq.  >
bq.  >     Should hasFamilyQualifierPermission log to AUDITLOG? It is used in places to
make decisions -- an exception is thrown directly or not.
bq.  
bq.  Gary Helmling wrote:
bq.      Yes, agree, we should either log to AUDITLOG at decision points here or consistently
move the AUDITLOG logging up a level out of permissionGranted() and hasFamilyQualifierPermission().

With moving the audit logging up to requirePermission(), logging to AUDITLOG here would be
redundant.  Removing the existing log message.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java,
line 375
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line375>
bq.  >
bq.  >     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.
bq.  
bq.  Gary Helmling wrote:
bq.      Agree, should go to AUDITLOG at trace.

Removing as redundant with the checking in permissionGranted() and AUDITLOG logging performed
in requirePermission().


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java,
line 497
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line497>
bq.  >
bq.  >     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.
bq.  >     
bq.  >     In that case, CREATE permission and an ownership check could follow the test
for ADMIN permission.

Noted


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java,
line 590
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line590>
bq.  >
bq.  >     Should be logged with ERROR?
bq.  
bq.  Gary Helmling wrote:
bq.      sure

Done.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java,
line 832
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line832>
bq.  >
bq.  >     Should this go to AUDITLOG? At INFO or TRACE level? My preference is TRACE.

Done.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java,
line 856
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line856>
bq.  >
bq.  >     Should this go to AUDITLOG? At INFO or TRACE level? My preference is TRACE.
bq.  
bq.  Gary Helmling wrote:
bq.      Yes, agree.

Done.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/ZKPermissionWatcher.java,
line 77
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45410#file45410line77>
bq.  >
bq.  >     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.
bq.  >     
bq.  >     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.
bq.  
bq.  Gary Helmling wrote:
bq.      Yes, shouldn't just be swallowing this.

ZooKeeperListener defined methods don't throw any exceptions, so logging and aborting on KeeperExceptions
here instead.


bq.  On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/rbac/ZKPermissionWatcher.java,
line 59
bq.  > <https://reviews.apache.org/r/2041/diff/1/?file=45410#file45410line59>
bq.  >
bq.  >     I wonder if there is some way we can check if a secure variant of ZooKeeper
is running, and refuse to initialize if not.
bq.  
bq.  Gary Helmling wrote:
bq.      My thinking has been to handle all secure ZooKeeper changes separately.  So I'd prefer
to handle any check here as part of that.
bq.      
bq.      I do think it's reasonable to run AccessController with only SIMPLE auth and no secure
ZooKeeper.  It's not secure but could still be useful (we currently use this setup for tests).
bq.      
bq.      We could complain loudly to give an indication that you have a security hole though.
bq.  
bq.  Andrew Purtell wrote:
bq.      > I do think it's reasonable to run AccessController with only SIMPLE auth and
no secure ZooKeeper.
bq.      
bq.      I'd argue only for test cases, and we can make provisions for tests to add an undocumented
configuration property to that effect.
bq.      
bq.      > We could complain loudly to give an indication that you have a security hole
though.
bq.      
bq.      Complaining loudly is good to do in any case except when unit tests want to do something.

I will add this into a separate patch handling ZooKeeper authentication.


- Gary


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2041/#review2077
-----------------------------------------------------------


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

        

Mime
View raw message