> On Dec. 30, 2014, 8:44 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java, > > line 73 > > <https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line73> > > > > Not sure this is correct. Should probably just be a no-op, from looking > > at the javadoc on Destroyable.
Ok, I wasn't really sure how this was actually supposed to work. I wasn't sure if there's something we need to do to the UGI if/when destroy() is called -- of course, I haven't a clue when destroy() is even called... > On Dec. 30, 2014, 8:44 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java, > > line 98 > > <https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line98> > > > > UGI.getShortName() is not guaranteed to be non-null, so we should be > > more defensive either here or in the constructor. Hrm, I'm not sure I understand how the subject would not have a primary to extract as the shortName (if it's possible to actually make getShortName() return us null), but it's an easy check to make. > On Dec. 30, 2014, 8:44 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransport.java, > > lines 61-64 > > <https://reviews.apache.org/r/29386/diff/4/?file=803151#file803151line61> > > > > Does Java 7 allows for multi-catch blocks, or was that in 8? Yup, that was Java7. I need to clean this up anyways -- thanks for pointing it out. > On Dec. 30, 2014, 8:44 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/security/Credentials.java, > > lines 43-44 > > <https://reviews.apache.org/r/29386/diff/4/?file=803153#file803153line43> > > > > Why have this? I had something in here at some point in time, but then removed it. Rather than just re-deleting the logger (to assumable re-add it again some day, I just left it in place). I can remove it if it bothers you. - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review66394 ----------------------------------------------------------- On Dec. 30, 2014, 9:17 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29386/ > ----------------------------------------------------------- > > (Updated Dec. 30, 2014, 9:17 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 > ----- > > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java f6ea934 > core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java > 6fe61a5 > 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/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 e054a5f > 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/TTimeoutTransport.java > 6eace77 > 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/main/java/org/apache/accumulo/core/security/Credentials.java > 525a958 > 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/conf/ClientConfigurationTest.java > 40be70f > > core/src/test/java/org/apache/accumulo/core/rpc/SaslConnectionParamsTest.java > PRE-CREATION > proxy/src/main/java/org/apache/accumulo/proxy/Proxy.java 4b048eb > > server/base/src/main/java/org/apache/accumulo/server/AccumuloServerContext.java > 09ae4f4 > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java > 046cfb5 > > 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 > 5e81018 > > server/base/src/main/java/org/apache/accumulo/server/security/SecurityUtil.java > 29e4939 > > server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java > a59d57c > > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/thrift/UGIAssumingProcessor.java > PRE-CREATION > > 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/security/SystemCredentialsTest.java > 4202a7e > server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java > 93a9a49 > > 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/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java > 7e33300 > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > d5c1d2f > shell/src/main/java/org/apache/accumulo/shell/Shell.java 58308ff > shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java 8167ef8 > shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java 0e72c8c > shell/src/test/java/org/apache/accumulo/shell/ShellOptionsJCTest.java > PRE-CREATION > test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java > eb84533 > > test/src/main/java/org/apache/accumulo/test/performance/thrift/NullTserver.java > 2ebc2e3 > > test/src/test/java/org/apache/accumulo/server/security/SystemCredentialsIT.java > fb71f5f > > 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 > >