accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser" <josh.el...@gmail.com>
Subject Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
Date Tue, 30 Dec 2014 21:38:44 GMT


> On Dec. 30, 2014, 7:39 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java, line 109
> > <https://reviews.apache.org/r/29386/diff/4/?file=803139#file803139line109>
> >
> >     It seems like the expectation is that `getPrincipal()` would be called now?
 If so can this be made private?  Can't remember if jcommander is ok w/ that.

I'm not sure if JCommander supports that (my gut reaction is no), but there might also be
"public API" implications of that...


> On Dec. 30, 2014, 7:39 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java, line 118
> > <https://reviews.apache.org/r/29386/diff/4/?file=803139#file803139line118>
> >
> >     Should this also be private?

Same comments as the principal.


> On Dec. 30, 2014, 7:39 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java, line 201
> > <https://reviews.apache.org/r/29386/diff/4/?file=803139#file803139line201>
> >
> >     Should this be private?

Probably protected or package-protected (with a note about not intended for public use), but
I made it public just because startDebugLogging and startTracing were also public. If we make
it private, it would make testing it harder.


> On Dec. 30, 2014, 7:39 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java,
line 46
> > <https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line46>
> >
> >     This is adding UserGroupInformation to Accumulo API.  Is that a stable hadoop
API?  Does it have to be in API, why not just principal?

UGI is marked as LimitedPrivate and Evolving, but I have no qualms against including it into
our API. The problem with accepting a principal alone, we would then have to duplicate the
login capabilities that UGI already provides (e.g performing a login via password or keytab).
By accepting a UGI directly, we don't have to "own" this logic. It's my opinion that it's
easiest to just use UGI directly, but could see an argument for "transparently" using it behind
the scenes if you think that would be better.


> On Dec. 30, 2014, 7:39 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java,
line 63
> > <https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line63>
> >
> >     why is this a noop?

The SASL transport in thrift is actually doing the work of sending the kerberos credentials
over the wire for our RPC use. We have a TProcessor and a wrapper around the thrift interface
implementation which passes this information from the "thrift" layer back into our Credentials
objects which alleviates us from having to do (nearly) anything in the KerberosToken itself.

I'm not sure if there is a better way to do it, but I'm open to suggestions.


> On Dec. 30, 2014, 7:39 p.m., kturner wrote:
> > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java, line
540
> > <https://reviews.apache.org/r/29386/diff/4/?file=803157#file803157line540>
> >
> >     why did this method need to be added?

Client authentication with Kerberos implies that any user with valid Kerberos credentials
can be authenticated with the system transparently. As such, the notion of a "root" super-user
goes out the window -- it doesn't make sense to have a "root" user because that would require
a new kerberos identity to be created just to delegate permissions to an "actual" user.

By changing `accumulo init` to accept a "root" user (and granting the necessary system permissions
to that user), we remove the need to have some extra user created only for the purposes of
bootstraping the Accumulo permissions.

Alternatively, we could also require that the user running `accumulo init` has Kerberos credentials
and give that user system permissions. I could see this work either way.


> On Dec. 30, 2014, 7:39 p.m., kturner wrote:
> > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java, line
550
> > <https://reviews.apache.org/r/29386/diff/4/?file=803157#file803157line550>
> >
> >     Could offer a command line option to set root username for non-interactive cases.

Will do.


- Josh


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


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message