[
https://issues.apache.org/jira/browse/HBASE-3025?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13115868#comment-13115868
]
jirapos...@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/AccessController.java,
line 98
bq. > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line98>
bq. >
bq. > Can we make this 1?
sure
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
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. On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq. >
security/src/main/java/org/apache/hadoop/hbase/security/rbac/AccessController.java,
line 200
bq. > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line200>
bq. >
bq. > Should be INFO or TRACE level? TRACE makes more sense to me.
Sure, can use trace for all audit log decisions.
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 208
bq. > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line208>
bq. >
bq. > Debug logging should go to LOG not AUDITLOG
This is an authorization decision since we're returning true below. We can
make this trace level, and improve the format, but I think AUDITLOG (if
enabled) should contain a single message per request on why the request was
allowed or denied.
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 274
bq. > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line274>
bq. >
bq. > Should be INFO or TRACE level? TRACE makes more sense to me.
will change to trace.
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 354
bq. > <https://reviews.apache.org/r/2041/diff/1/?file=45404#file45404line354>
bq. >
bq. > Should something go to AUDITLOG here?
Failure should already have been recorded in AUDITLOG via logDenied(). Agree
that moving AUDITLOG messages up here with consistent format would be clearer,
but will require some restructuring of return value from permissionGranted() so
that some context specific reason can be pulled back up for logging.
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.
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().
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.
Agree, should go to AUDITLOG at trace.
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?
sure
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.
Yes, agree.
bq. On 2011-09-27 16:58:47, Andrew Purtell wrote:
bq. >
security/src/main/java/org/apache/hadoop/hbase/security/rbac/Permission.java,
line 174
bq. > <https://reviews.apache.org/r/2041/diff/1/?file=45406#file45406line174>
bq. >
bq. > 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.
Yeah, seems safer to throw an exception here than to ignore invalid input.
What about throwing an IOException (to tie in to existing error handling)?
We could potentially trap the VersionMismatchException from VersionedWritable
to allow skip and continue when reading newer versions of Permission with
potentially added Action codes. Would need to think about what kind of errors
that would expose us to.
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 47
bq. > <https://reviews.apache.org/r/2041/diff/1/?file=45407#file45407line47>
bq. >
bq. > 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.
Yeah "rbac" here and in package name is a misnomer. How about using "access"
instead? "auth" seems ambiguous to me as it could mean "authentication" or
"authorization". JDK uses "auth" in javax.security.auth and claims it's for
both, but seems like that and sub-packages are more "authentication" related to
me. Hadoop uses "authorize" for a similar package to this.
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?
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. 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.
My thinking has been to handle all secure ZooKeeper changes separately. So I'd
prefer to handle any check here as part of that.
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).
We could complain loudly to give an indication that you have a security hole
though.
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.
Yes, shouldn't just be swallowing this.
- 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
