Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============8236267092254442134=="
MIME-Version: 1.0
Subject: Re: Review Request 29386: ACCUMULO-2815 Client authentication via
 Kerberos
From: "Mike Drob" <mdrob@mdrob.com>
To: "Mike Drob" <mdrob@mdrob.com>, "accumulo" <dev@accumulo.apache.org>,
 "Josh Elser" <josh.elser@gmail.com>
Date: Mon, 29 Dec 2014 16:17:58 -0000
Message-ID: <20141229161758.31225.54784@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Mike Drob" <noreply@reviews.apache.org>
References: <20141229155942.31226.16531@reviews.apache.org>
In-Reply-To: <20141229155942.31226.16531@reviews.apache.org>
Reply-To: "Mike Drob" <mdrob@mdrob.com>

--===============8236267092254442134==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On Dec. 29, 2014, 3:59 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java, line 251
> > <https://reviews.apache.org/r/29386/diff/1/?file=799919#file799919line251>
> >
> >     Will this conditional ever be called?
> 
> Josh Elser wrote:
>     In the Kerberos case, no. For the normal case, yes. This isn't something that I changed.

I'm not convinced. It looks like any time that this is true, the exception on line 242 gets thrown first.


> On Dec. 29, 2014, 3:59 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java, line 369
> > <https://reviews.apache.org/r/29386/diff/1/?file=799930#file799930line369>
> >
> >     Could this occur during transient conditions? How noisy will it appear in the logs and monitor page when a tserver goes down.
> 
> Josh Elser wrote:
>     It sure can and does. If the client gets the address for a server to communicate with and the server dies before the client opens the transport, it'll get the error. However, it'll then transparently retry the connection. This is something that also hasn't changed.

This isn't something that you changed, but you did add error level logging, which I'm not sure is necessary. If it's transient and retries automatically, then that's a WARN at best. I'm of the opinion that if it can wait until morning then it shouldn't be an ERROR.


- Mike


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


On Dec. 29, 2014, 4:13 p.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29386/
> -----------------------------------------------------------
> 
> (Updated Dec. 29, 2014, 4:13 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 
>   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/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/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 
>   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
> 
>


--===============8236267092254442134==--