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, 15 Nov 2011 19:51:53 GMT

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

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



bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java,
line 238
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55451#file55451line238>
bq.  >
bq.  >     These are expensive calls now that tableinfo has been removed from HRI (IIRC);
I don't think there caching going on.

I think the HRegionInfo version is the expensive one (doing the HDFS read).  For HRegion,
this is just returning the HTD instance, so seems like it should be okay...


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java,
line 344
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55451#file55451line344>
bq.  >
bq.  >     Is this on every 'action'?  Will this become annoying?  Loads of spew in logs?

Yeah, that's going to get pretty noisy.  Since we have the audit logging, I think we can just
remove this.


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java,
line 498
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55451#file55451line498>
bq.  >
bq.  >     BaseRegionObserver doesn't noop in this case?  Do we need this empty method?

This is from MasterObserver -- need to implement these since java doesn't give us multiple
inheritance.


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java,
line 482
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55451#file55451line482>
bq.  >
bq.  >     So, requirePermission will  not find a controlling user -- here we are assigning
current user as controlling (controlling user has to make the table?)

"getControllingUser" is a poor name -- it's really more the "active user": the remote user
in an rpc request, otherwise the currently logged in user for the process.  I'll change the
name.

We could use the same method here (I think this code just predates that).  I'll update.


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java,
line 676
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55451#file55451line676>
bq.  >
bq.  >     Oh, so these are independent?  You could NOT have perms on family but JUST on
an individual cf+qualifier combo?

Yes.


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java,
line 798
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55451#file55451line798>
bq.  >
bq.  >     Only controlling user can scan?  The person who created the table (or the group
I suppose?)

Anyone with read access can scan (getControllingUser is just poorly named -- see above).


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java,
line 865
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55451#file55451line865>
bq.  >
bq.  >     This code is duplicated.  Could be a private method?

Done.


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/AccessControllerProtocol.java,
line 37
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55452#file55452line37>
bq.  >
bq.  >     Since it throws exception if fails -- and I guess you want to do this because
the IOE will have lots of info on the why -- then this method might as well be void rather
than boolean return?

Updated grant and revoke to both have void return type (depends on HBASE-4784).


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java,
line 116
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55453#file55453line116>
bq.  >
bq.  >     Why we have TablePermission pollution in this base Permission class?

Incomplete refactoring.  Fixed.


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/AccessControllerProtocol.java,
line 43
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55452#file55452line43>
bq.  >
bq.  >     Fix this first sentence... reads wrong?

Done.


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java,
line 651
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55451#file55451line651>
bq.  >
bq.  >     This is not a good name for a boolean.   Shouldn't it be aclRegion?  isAclRegion
is the name of the method that checks the boolean is true.

Changed to aclRegion.


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java,
line 41
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55453#file55453line41>
bq.  >
bq.  >     static?

Nested enums are implicitly static


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java,
line 94
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55453#file55453line94>
bq.  >
bq.  >     You need this?  Else its a read-only class.  Just create new instance if want
to change Actions?

Yeah, good point.  This was only used in tests, which were easily fixed, so I've removed it.
 The class is now immutable (except for writable).


bq.  On 2011-11-13 19:47:04, Michael Stack wrote:
bq.  > security/src/main/java/org/apache/hadoop/hbase/security/access/UserPermission.java,
line 95
bq.  > <https://reviews.apache.org/r/2041/diff/3/?file=55456#file55456line95>
bq.  >
bq.  >     Do we need a hash method?  Same for other Permission objects?

Added hashCode() for Permission, TablePermission, and UserPermission


- Gary


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


On 2011-11-01 21:18:27, 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-11-01 21:18:27)
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/access/AccessControlFilter.java
PRE-CREATION 
bq.    security/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java
PRE-CREATION 
bq.    security/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
PRE-CREATION 
bq.    security/src/main/java/org/apache/hadoop/hbase/security/access/AccessControllerProtocol.java
PRE-CREATION 
bq.    security/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java PRE-CREATION

bq.    security/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java
PRE-CREATION 
bq.    security/src/main/java/org/apache/hadoop/hbase/security/access/TablePermission.java
PRE-CREATION 
bq.    security/src/main/java/org/apache/hadoop/hbase/security/access/UserPermission.java
PRE-CREATION 
bq.    security/src/main/java/org/apache/hadoop/hbase/security/access/ZKPermissionWatcher.java
PRE-CREATION 
bq.    security/src/test/java/org/apache/hadoop/hbase/security/access/SecureTestUtil.java
PRE-CREATION 
bq.    security/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControlFilter.java
PRE-CREATION 
bq.    security/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java
PRE-CREATION 
bq.    security/src/test/java/org/apache/hadoop/hbase/security/access/TestTablePermissions.java
PRE-CREATION 
bq.    security/src/test/java/org/apache/hadoop/hbase/security/access/TestZKPermissionsWatcher.java
PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java 99875b8 
bq.    src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 8a40762 
bq.    src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java bb67e53 
bq.    src/main/resources/hbase-default.xml 3785533 
bq.    src/main/ruby/hbase.rb 4d27191 
bq.    src/main/ruby/hbase/admin.rb 61e04d8 
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/user_permission.rb PRE-CREATION 
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
>            Priority: Critical
>             Fix For: 0.92.0
>
>         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