----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review67955 -----------------------------------------------------------
I think there's room for improvement in the ClientContext/AccumuloServerContext construction of the SaslConnectionParams from the AccumuloConfiguration. It's nothing that would prevent this from being committed. However, I think some of it might naturally be polished, as a result of my comments below on the SystemCredentials.java changes. core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java <https://reviews.apache.org/r/29386/#comment112093> Wouldn't instanceof satisfy this check more robustly? core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java <https://reviews.apache.org/r/29386/#comment112094> shouldn't saslPrincipal be part of the saslParams? core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java <https://reviews.apache.org/r/29386/#comment112099> Should this be an IOException? IllegalArgumentException? pom.xml <https://reviews.apache.org/r/29386/#comment112028> Why bump this? Do we want 2.3.0 to be the default build? Are we dropping support for 2.2.x and require 2.3.0 and later? pom.xml <https://reviews.apache.org/r/29386/#comment112081> If we're dropping Hadoop 1 support, that should be done in a separate commit. server/base/src/main/java/org/apache/accumulo/server/AccumuloServerContext.java <https://reviews.apache.org/r/29386/#comment112120> I don't understand this comment. server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java <https://reviews.apache.org/r/29386/#comment112125> more readable as: `isKerberos = (condition);` server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java <https://reviews.apache.org/r/29386/#comment112128> Doesn't seem like this needed to move. server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java <https://reviews.apache.org/r/29386/#comment112127> unused method? server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java <https://reviews.apache.org/r/29386/#comment112138> I think some of these changes are unnecessary. We technically have a single system user. What a lot of this appears to be doing is creating multiple identities for that user. Really, though, all we need is multiple SaslConnectionParams for the transport layer. That can/should be done in ClientContext/AccumuloServerContext, not here. Essentially, it seems we're using the token to get the SASL params, and the SASL params to get the identity of the user. That works for regular users, but because each server has a different SASL params (Kerberos identity), but they all represent the same (special) Accumulo identity, we can easily add a special case. We already have to have a special case; but I think some ways of adding that special case are better than others. Apologies that this is so vague. I'm finding it difficult to word my perspective clearly. So, I'm leaving this comment so we can follow-up. test/src/test/java/org/apache/accumulo/server/security/SystemCredentialsIT.java <https://reviews.apache.org/r/29386/#comment112140> I think my previous comment about coverage here still holds. This still results in failure, but for a different reason than what was originally being tested for. It now fails because there's no user with a password "fake". Before, it was testing that a failure would occur if instance.* configuration details were different. That case is now absent. - Christopher Tubbs On Jan. 9, 2015, 5:29 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29386/ > ----------------------------------------------------------- > > (Updated Jan. 9, 2015, 5:29 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2815 > https://issues.apache.org/jira/browse/ACCUMULO-2815 > > > Repository: accumulo > > > Description > ------- > > ACCUMULO-2815 Initial support for Kerberos client authentication. > > Leverage SASL transport provided by Thrift which can speak GSSAPI, which > Kerberos implements. Introduced... > > * An Accumulo KerberosToken which is an AuthenticationToken to validate users. > * Custom thrift processor and invocation handler to ensure server RPCs have a > valid KRB identity and Accumulo authentication. > * A KerberosAuthenticator which extends ZKAuthenticator to support Kerberos > identities seamlessly. > * New ClientConf variables to use SASL transport and pass Kerberos server > principal > * Updated ClientOpts and Shell opts to transparently use a KerberosToken when > SASL is enabled (no extra client work). > > I believe this is the "bare minimum" for Kerberos support. They are also > grossly lacking in unit and integration tests. I believe that I might have > somehow broken the client address string in the server (I saw log messages > with client: null, but I'm not sure if it's due to these changes or not). A > necessary limitation in the Thrift server used is that, like the SSL > transport, the SASL transport cannot presently be used with the > TFramedTransport, which means none of the [half]async thrift servers will > function with this -- we're stuck with the TThreadPoolServer. > > Performed some contrived benchmarks on my laptop (while still using it > myself) to get at big-picture view of the performance impact against "normal" > operation and Kerberos alone. Each "run" was the duration to ingest 100M > records using continuous-ingest, timed with `time`, using 'real'. > > THsHaServer (our default), 6 runs: > > Avg: 10m7.273s (607.273s) > Min: 9m43.395s > Max: 10m52.715s > > TThreadPoolServer (no SASL), 5 runs: > > Avg: 11m16.254s (676.254s) > Min: 10m30.987s > Max: 12m24.192s > > TThreadPoolServer+SASL/GSSAPI (these changes), 6 runs: > > Avg: 13m17.187s (797.187s) > Min: 10m52.997s > Max: 16m0.975s > > The general takeway is that there's about 15% performance degredation in its > initial state which is in the realm of what I expected (~10%). > > > Diffs > ----- > > README ad6f2bf > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java eb020eb > core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java > df53645 > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java > e75bec6 > core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java > f481cc3 > core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java > fcbf9f9 > > core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java > 6dc846f > > core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java > 5da803b > > core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/conf/Property.java ce5de85 > core/src/main/java/org/apache/accumulo/core/rpc/FilterTransport.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/rpc/SaslConnectionParams.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java 09bd6c4 > core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransport.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransportFactory.java > PRE-CREATION > core/src/test/java/org/apache/accumulo/core/cli/TestClientOpts.java ff49bc0 > > core/src/test/java/org/apache/accumulo/core/client/ClientConfigurationTest.java > PRE-CREATION > > core/src/test/java/org/apache/accumulo/core/client/impl/ThriftTransportKeyTest.java > PRE-CREATION > > core/src/test/java/org/apache/accumulo/core/conf/ClientConfigurationTest.java > 40be70f > > core/src/test/java/org/apache/accumulo/core/rpc/SaslConnectionParamsTest.java > PRE-CREATION > docs/src/main/asciidoc/accumulo_user_manual.asciidoc ec8e538 > docs/src/main/asciidoc/chapters/clients.txt 64f0e55 > docs/src/main/asciidoc/chapters/kerberos.txt PRE-CREATION > > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java > 27d6b19 > > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java > 26c23ed > pom.xml 203acdc > proxy/src/main/java/org/apache/accumulo/proxy/Proxy.java 7eb4fbf > > server/base/src/main/java/org/apache/accumulo/server/AccumuloServerContext.java > 09ae4f4 > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java > ed2189d > > server/base/src/main/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingInvocationHandler.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingWrapper.java > PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java > 641c0bf > > server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java > 5fe57b7 > > server/base/src/main/java/org/apache/accumulo/server/security/SecurityUtil.java > 42d1313 > > server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java > 79201b1 > > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthorizor.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosPermissionHandler.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/thrift/UGIAssumingProcessor.java > PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/util/Admin.java > 7d247f7 > server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java > ef182f1 > > server/base/src/test/java/org/apache/accumulo/server/AccumuloServerContextTest.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingInvocationHandlerTest.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/rpc/ThriftServerTypeTest.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/security/SystemCredentialsTest.java > 01ff9ac > server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java > db37c8b > > server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogsTest.java > f98721f > > server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java > 99558b8 > > server/gc/src/test/java/org/apache/accumulo/gc/replication/CloseWriteAheadLogReferencesTest.java > cad1e01 > server/master/src/main/java/org/apache/accumulo/master/Master.java 12195fa > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/Basic.java > 2d98fed > server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java > 3063cdc > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > 2bfa5a0 > shell/src/main/java/org/apache/accumulo/shell/Shell.java 9697a85 > shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java 875367d > shell/src/test/java/org/apache/accumulo/shell/ShellOptionsJCTest.java > PRE-CREATION > test/pom.xml b0a926f > test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java > 3bb44ff > > test/src/main/java/org/apache/accumulo/test/performance/thrift/NullTserver.java > 0afa243 > test/src/test/java/org/apache/accumulo/harness/AccumuloClusterIT.java > 8f7e1b7 > test/src/test/java/org/apache/accumulo/harness/MiniClusterHarness.java > abdb627 > test/src/test/java/org/apache/accumulo/harness/SharedMiniClusterIT.java > 2380f66 > test/src/test/java/org/apache/accumulo/harness/TestingKdc.java PRE-CREATION > > test/src/test/java/org/apache/accumulo/harness/conf/AccumuloMiniClusterConfiguration.java > 11b7530 > > test/src/test/java/org/apache/accumulo/server/security/SystemCredentialsIT.java > abbe5e6 > test/src/test/java/org/apache/accumulo/test/ArbitraryTablePropertiesIT.java > aa5c164 > test/src/test/java/org/apache/accumulo/test/CleanWalIT.java 1fcd5a4 > > test/src/test/java/org/apache/accumulo/test/functional/BatchScanSplitIT.java > 221889b > test/src/test/java/org/apache/accumulo/test/functional/KerberosIT.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/test/security/KerberosTokenTest.java > PRE-CREATION > test/src/test/resources/log4j.properties cb35840 > > Diff: https://reviews.apache.org/r/29386/diff/ > > > Testing > ------- > > Ensure existing unit tests still function. Accumulo is functional and ran > continuous ingest multiple times using a client with only a Kerberos identity > (no user/password provided). Used MIT Kerberos with Apache Hadoop 2.6.0 and > Apache ZooKeeper 3.4.5. > > > Thanks, > > Josh Elser > >
