accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubbsii...@apache.org>
Subject Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
Date Wed, 14 Jan 2015 02:02:26 GMT

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


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