[ https://issues.apache.org/jira/browse/HBASE-2742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13152476#comment-13152476 ]
jirapos...@reviews.apache.org commented on HBASE-2742: ------------------------------------------------------ bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > pom.xml, line 408 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=58999#file58999line408> bq. > bq. > Whats going on here? Why would we exclude the test hbase-site.xml? I thought it had all configs. for test running? Or is this in the security profile? I see you then add it back in later under the plugin section. Did we have it in wrong place? This is so that, when building with security profile, we can override the hbase-site.xml used for tests -- specifically set it to use SecureRpcEngine. It's an ugly change to the POM and kind of ugly to duplicate the test hbase-site.xml, but I didn't see an easier way of doing it. I'm open to other suggestions to improve it. bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > security/src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java, line 355 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59000#file59000line355> bq. > bq. > This is a number? Yes: SUCCESS (0), ERROR (1), FATAL (-1); bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > security/src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java, line 369 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59000#file59000line369> bq. > bq. > Could we get an enum we don't know about? bq. > bq. > If copied from hadoop code, don't worry about this comment. Yeah this is direct from Hadoop. It is a bit odd not to have a default block, which should probably behave same as fatal. Depends if we want to diverge more by adding it. bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > security/src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java, line 375 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59000#file59000line375> bq. > bq. > Man, this is kinda ugly... its a data member in a super class? Cryptic! bq. > bq. > (This is hadoop code?) This is actually from HBaseClient. Hadoop just does: } catch (IOException e) { markClosed(e); } bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java, line 143 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59003#file59003line143> bq. > bq. > Funny. Above we compare on State.SUCCESS.state. What if there is a FATAL here? Anything special we should do? In SecureClient, I think the check on State.SUCCESS.state is to avoid mapping the deserialized value back to the enum value. In the case of FATAL, the client will still see fatal status written separately in the response. For errorClass and error, FATAL is equivalent to ERROR. bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > src/assembly/all.xml, line 60 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59019#file59019line60> bq. > bq. > This will add the 'security' on the end if we are building w/ the security profile? Yes, so for say 0.92, when building with -P security, you would wind up with hbase-0.92.0-security.jar. Without the security profile, you just get hbase-0.92.0.jar. This at least saves having to fiddle with the project version for secure vs. insecure builds, and gives artifacts named as expected. But not sure if it complicates deploying to a maven repo? It might still need more tweaking to improve how it works. bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 399 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59021#file59021line399> bq. > bq. > So, in insecure hbase, is user provided? If not, this message will be all over our logs? In insecure HBase, User.getCurrent() should return the operating system user. So I don't think this should show up in normal circumstances with either insecure or secure versions. bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java, line 359 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59024#file59024line359> bq. > bq. > Say what to use instead? Ted pointed this out too. Fixing to reference RpcEngine.call(). bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 119 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59026#file59026line119> bq. > bq. > This seems like an odd one to expose to subclasses. Yeah, must have been necessary at one point, but I no longer see any usage that requires it. Changing back to private. bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 826 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59026#file59026line826> bq. > bq. > Is this 'fix' copied from hadoop? Seems sensible. Yes, copied from Hadoop. Made sense to me as well. bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java, line 44 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59027#file59027line44> bq. > bq. > Whats happening here? Do we have to do this -- security pollution of base hbase interface? bq. > Yes, these annotations are required for the connection authorization process and authentication. I copied over the annotation definitions to org.apache.hadoop.hbase.security as part of core. But actually removing them and using another means might take some work. bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 256 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59035#file59035line256> bq. > bq. > this a new call we have to make? This was moved down to after ZooKeeperWatcher setup so that AuthenticationTokenSecretManager (which needs zk) can be setup correctly in SecureServer. bq. On 2011-11-17 23:03:06, Michael Stack wrote: bq. > src/main/resources/hbase-default.xml, line 468 bq. > <https://reviews.apache.org/r/1991/diff/6/?file=59041#file59041line468> bq. > bq. > This stuff should be in here and not in an hbase-security-default.xml that comes in only when running security? bq. > bq. > Its ok this stuff is here... just wondering if could be shown only when a security context. Hmm, looks like the keytab stuff got duplicated. It's already there from previous patches for running on secure Hadoop. The hbase.auth.*, hbase.policy.file, and hbase.superuser properties are new. I guess we could split them out into a separate default file and add it to the resources in HBaseConfiguration. How important do you think it is? - Gary ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1991/#review3322 ----------------------------------------------------------- On 2011-11-17 07:27:20, Gary Helmling wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/1991/ bq. ----------------------------------------------------------- bq. bq. (Updated 2011-11-17 07:27:20) bq. bq. bq. Review request for hbase. bq. bq. bq. Summary bq. ------- bq. bq. This patch creates a new secure RPC engine for HBase, which provides Kerberos based authentication of clients, and a token-based authentication mechanism for mapreduce jobs. Primary components of the patch are: bq. bq. - a new maven profile for secure Hadoop/HBase: hadoop-0.20S bq. - Secure Hadoop dependent classes are separated under a pseudo-module in the security/ directory. These source and test directories are only including if building the secure Hadoop profile bq. - Currently the security classes get packaged with the regular HBase build artifacts. We need a way to at least override project.version, so we can append something like a "-security" suffix indicating the additional security components. bq. - The pseudo-module here is really a half-step forward. It enables the security code to be optionally included in the build for now, and sets up the structure for a security module. But we still will want to pursue full modularization (see HBASE-4336), which will allow packing the security code in a separate build artifact. bq. bq. - a new RPC engine providing kerberos and token-based authentication: org.apache.hadoop.hbase.ipc.SecureRpcEngine bq. - implementation under security/src/main/java/org/apache/hadoop/hbase/ipc/ bq. - The implementation classes extend the existing HBaseClient and HBaseServer to share as much of the RPC code as possible. The main override is of the connection classes to allow control over the SASL negotiation of secure connections bq. bq. - existing RPC changes bq. - The existing HBaseClient and HBaseServer have been modified to make subclassing possible bq. - All references to Hadoop UserGroupInformation have been replaced with org.apache.hadoop.hbase.security.User to insulate from future dependencies on specific Hadoop versions bq. bq. - a coprocessor endpoint for obtaining new authentication tokens: TokenProvider, and supporting classes for token generation and synchronization (incorporating HBASE-3615) bq. - implementation is under security/src/main/java/org/apache/hadoop/hbase/security/token/ bq. - Secret keys for token generation and verification are synchronized throughout the cluster in zookeeper, under /hbase/tokenauth/keys bq. bq. bq. To enable secure RPC, add the following configuration to hbase-site.xml: bq. bq. <property> bq. <name>hadoop.security.authorization</name> bq. <value>true</value> bq. </property> bq. <property> bq. <name>hadoop.security.authentication</name> bq. <value>kerberos</value> bq. </property> bq. <property> bq. <name>hbase.rpc.engine</name> bq. <value>org.apache.hadoop.hbase.ipc.SecureRpcEngine</value> bq. </property> bq. <property> bq. <name>hbase.coprocessor.region.classes</name> bq. <value>org.apache.hadoop.hbase.security.token.TokenProvider</value> bq. </property> bq. bq. In addition, the master and regionserver processes must be configured for kerberos authentication using the properties: bq. bq. * hbase.(master|regionserver).keytab.file bq. * hbase.(master|regionserver).kerberos.principal bq. * hbase.(master|regionserver).kerberos.https.principal bq. bq. bq. This addresses bug HBASE-2742. bq. https://issues.apache.org/jira/browse/HBASE-2742 bq. bq. bq. Diffs bq. ----- bq. bq. conf/hbase-policy.xml PRE-CREATION bq. pom.xml 2847416 bq. security/src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/ipc/SecureConnectionHeader.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/AccessDeniedException.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/token/AuthenticationKey.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/token/AuthenticationProtocol.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/token/AuthenticationTokenIdentifier.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/token/AuthenticationTokenSecretManager.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/token/AuthenticationTokenSelector.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/token/TokenProvider.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/token/TokenUtil.java PRE-CREATION bq. security/src/main/java/org/apache/hadoop/hbase/security/token/ZKSecretWatcher.java PRE-CREATION bq. security/src/test/java/org/apache/hadoop/hbase/security/token/TestTokenAuthentication.java PRE-CREATION bq. security/src/test/java/org/apache/hadoop/hbase/security/token/TestZKSecretWatcher.java PRE-CREATION bq. security/src/test/resources/hbase-site.xml PRE-CREATION bq. src/assembly/all.xml 3ad8ab3 bq. src/main/java/org/apache/hadoop/hbase/HServerAddress.java f28240c bq. src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java cba7bd1 bq. src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java 904c107 bq. src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 4086829 bq. src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 054b92b bq. src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 337da78 bq. src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java d9ba0ea bq. src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 9e0535e bq. src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java 0dc5bea bq. src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java e8b2c9e bq. src/main/java/org/apache/hadoop/hbase/ipc/RequestContext.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java 1b5629a bq. src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java c8e030c bq. src/main/java/org/apache/hadoop/hbase/mapred/TableMapReduceUtil.java db07ed1 bq. src/main/java/org/apache/hadoop/hbase/mapreduce/TableMapReduceUtil.java ad88b76 bq. src/main/java/org/apache/hadoop/hbase/master/HMaster.java 19ed5b8 bq. src/main/java/org/apache/hadoop/hbase/security/KerberosInfo.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/security/TokenInfo.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/security/User.java 00bd06d bq. src/main/java/org/apache/hadoop/hbase/zookeeper/ZKLeaderManager.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 4c5e844 bq. src/main/resources/hbase-default.xml 6f98f5d bq. src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java c0634f4 bq. src/test/java/org/apache/hadoop/hbase/PerformanceEvaluation.java bb9ea45 bq. src/test/java/org/apache/hadoop/hbase/zookeeper/TestZKLeaderManager.java PRE-CREATION bq. bq. Diff: https://reviews.apache.org/r/1991/diff bq. bq. bq. Testing bq. ------- bq. bq. A full test suite run showed failures in TestMasterFailover (timeout) and TestAvroServer. I'll look into those. bq. bq. Cluster testing (1 master + 3 slaves in ec2) with Kerberos authentication and map reduce token authentication, using YCSB, RowCounter, PerformanceEvaluation. bq. bq. bq. Thanks, bq. bq. Gary bq. bq. > Provide strong authentication with a secure RPC engine > ------------------------------------------------------ > > Key: HBASE-2742 > URL: https://issues.apache.org/jira/browse/HBASE-2742 > Project: HBase > Issue Type: Improvement > Components: ipc > Reporter: Gary Helmling > Priority: Critical > Fix For: 0.92.0 > > > The HBase RPC code (org.apache.hadoop.hbase.ipc.*) was originally forked off > of Hadoop RPC classes, with some performance tweaks added. Those > optimizations have come at a cost in keeping up with Hadoop RPC changes > however, both bug fixes and improvements/new features. > In particular, this impacts how we implement security features in HBase (see > HBASE-1697 and HBASE-2016). The secure Hadoop implementation (HADOOP-4487) > relies heavily on RPC changes to support client authentication via kerberos > and securing and mutual authentication of client/server connections via SASL. > Making use of the built-in Hadoop RPC classes will gain us these pieces for > free in a secure HBase. > So, I'm proposing that we drop the HBase forked version of RPC and convert to > direct use of Hadoop RPC, while working to contribute important fixes back > upstream to Hadoop core. Based on a review of the HBase RPC changes, the key > divergences seem to be: > HBaseClient: > - added use of TCP keepalive (HBASE-1754) > - made connection retries and sleep configurable (HBASE-1815) > - prevent NPE if socket == null due to creation failure (HBASE-2443) > HBaseRPC: > - mapping of method names <-> codes (removed in HBASE-2219) > HBaseServer: > - use of TCP keep alives (HBASE-1754) > - OOME in server does not trigger abort (HBASE-1198) > HbaseObjectWritable: > - allows List<> serialization > - includes it's own class <-> code mapping (HBASE-328) > Proposed process is: > 1. open issues with patches on Hadoop core for important fixes/adjustments > from HBase RPC (HBASE-1198, HBASE-1815, HBASE-1754, HBASE-2443, plus a > pluggable ObjectWritable implementation in RPC.Invocation to allow use of > HbaseObjectWritable). > 2. ship a Hadoop version with RPC patches applied -- ideally we should avoid > another copy-n-paste code fork, subject to ability to isolate changes from > impacting Hadoop internal RPC wire formats > 3. if all Hadoop core patches are applied we can drop back to a plain vanilla > Hadoop version > I realize there are many different opinions on how to proceed with HBase RPC, > so I'm hoping this issue will kick off a discussion on what the best approach > might be. My own motivation is maximizing re-use of the authentication and > connection security work that's already gone into Hadoop core. I'll put > together a set of patches around #1 and #2, but obviously we need some > consensus around this to move forward. If I'm missing other differences > between HBase and Hadoop RPC, please list as well. Discuss! -- 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