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 Tue, 30 Dec 2014 22:46:02 GMT

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


Overall, I really like this feature. It's a great idea. I have 3 big concerns, along with
a bunch of smaller points. The big ones are:
1) the changes to the SystemToken
2) the changes to init of the root user
3) the coupling with ZKAuthenticator


core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java
<https://reviews.apache.org/r/29386/#comment109843>

    Are these mutually exclusive?



core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java
<https://reviews.apache.org/r/29386/#comment109892>

    This link tag is bad. It should be UserGroupInformation, not ugi.



core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java
<https://reviews.apache.org/r/29386/#comment109846>

    The no-arg constructor is used for Writable, and is used to serialize credentials to a
file. This is useful for MapReduce.
    
    As is, this *should* work, as long as the user wishes to use the UserGroupInformation.getLoginUser().
However, that basically forces the MapReduce task to use the MapReduce task's own credentials,
to talk to Accumulo, and not the user's. This could be a cumbersome pitfall.
    
    Is there a way to serialize the user's ugi, so that it can be de-serialized, falling back
on the UserGroupInformation.getLoginUser() if the user's ugi cannot be deserialized?
    
    At the very least, the serialization should store *something* (magic bytes which refer
to "nothing here"), so we can distinguish between that and future serialized options, if we
add any later.



core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java
<https://reviews.apache.org/r/29386/#comment109893>

    Javadoc tag needs description or removal.



core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java
<https://reviews.apache.org/r/29386/#comment109845>

    Why is this unsupported?



core/src/main/java/org/apache/accumulo/core/conf/Property.java
<https://reviews.apache.org/r/29386/#comment109848>

    Descriptions should explain if these are mutually exclusive. Or, at least, that auth-conf
obviates the need for ssl.



core/src/main/java/org/apache/accumulo/core/conf/Property.java
<https://reviews.apache.org/r/29386/#comment109850>

    I don't think we want to expose this. It's high risk. What is the reason and how is it
related to this change?



core/src/main/java/org/apache/accumulo/core/rpc/SaslConnectionParams.java
<https://reviews.apache.org/r/29386/#comment109889>

    These are malformed javadocs. These tags are supposed to have descriptions. Please put
javadoc descriptions or leave these off.
    
    Of the choices, in this case, I think the AccumuloConfiguration should really be documented.
We have a bad habit of passing around configuration in an "AccumuloConfiguration" object,
with no context of where that configuration came from.
    
    This is really just a utility method, so maybe it doesn't matter, but if it's only used
internally, it might be better to use a ClientContext object instead of the configuration,
to prevent abuse (like instantiating additional instances of AccumuloConfiguration to satisfy
the method signature). If it uses the configuration from the ClientContext, it's more clear
which configuration it is using (the one used to create the connector or, in the case of the
server, the one used to launch the server).
    
    At the very least, the javadocs should be fixed.



core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransport.java
<https://reviews.apache.org/r/29386/#comment109853>

    This is what AssertionError is for.



core/src/main/java/org/apache/accumulo/core/security/Credentials.java
<https://reviews.apache.org/r/29386/#comment109851>

    Why add it if it's not used, only to suppress it's lack of use?



proxy/src/main/java/org/apache/accumulo/proxy/Proxy.java
<https://reviews.apache.org/r/29386/#comment109890>

    This warnings suppression is unnecessary if proxyProcClass was of type Class<? extends
TProcessor> and the forName was followed by .asSubclass(TProcessor.class).
    
    I realize this wasn't part of this changeset, but it's something I just noticed that could
be cleaned up.



server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java
<https://reviews.apache.org/r/29386/#comment109864>

    It was discussed in the context of ACCUMULO-259 that the root user would be a built-in,
password-based user, even if other auth mechanisms were available (like Linux root user).
Some proposals were made to make this easier to work with and more intuitive for users, but
the initialization of the root user should really just be the 
    
    See ACCUMULO-1300 and related issues, for more details.
    
    What are the risks of making the root user rely on Kerberos?



server/base/src/main/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingWrapper.java
<https://reviews.apache.org/r/29386/#comment109894>

    originalClass should be Class<? extends T> to ensure type safety.



server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
<https://reviews.apache.org/r/29386/#comment109914>

    Intentional what? (Should clarify that it's the case statement fall-through which is intentional).



server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java
<https://reviews.apache.org/r/29386/#comment109915>

    What is the reason for making SystemCredentials able to use Kerberos? SystemCredentials
are how the system identifies itself to itself. It's all internal. It shouldn't rely on external
components, in my opinion.



server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java
<https://reviews.apache.org/r/29386/#comment109916>

    The SystemToken should be a separate type. it is not a password token. It is a token type
that is exclusive to the system (it's even final, so it cannot be sub-class'd). It should
not be swapped out with Kerberos tokens, or any other kind of token.
    
    It should also not be considered a password token. That just happens to be the closest
analogy to the current implementation. It's really it's own special type.
    
    However, there is a proposal to make individual components authenticate, but I don't think
this achieves that: ACCUMULO-1165



server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java
<https://reviews.apache.org/r/29386/#comment109935>

    This should not be here. Using Kerberos for the SystemToken removes an important check
against rogue servers from other instances communicating with the current instance.
    
    It also removes all the other mandatory checks in the SystemToken which protect against
incompatible variations in the "instance.*" properties, which are encapsulated in the SystemToken's
hash.



server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java
<https://reviews.apache.org/r/29386/#comment109918>

    I don't think this should be so tightly coupled with the ZKAuthenticator. It doesn't seem
that this coupling is necessary to provide authentication functionality.
    
    Have a different implementation of an Authenticator that hijacks the data storage of another
implementation, could be quite confusing to administrators, especially if they need to swap
out the implementation.
    
    See ACCUMULO-1300 and related issues for other options.



server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java
<https://reviews.apache.org/r/29386/#comment109917>

    Why is this seed fixed?



shell/src/main/java/org/apache/accumulo/shell/Shell.java
<https://reviews.apache.org/r/29386/#comment109922>

    Do we even need this principal field, if we can just ask the connector?



shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java
<https://reviews.apache.org/r/29386/#comment109924>

    Descriptions are not consistent with capitalization. ("Use" vs. "use")



shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java
<https://reviews.apache.org/r/29386/#comment109926>

    Why is username the "short" user name? Is that unique in Kerberos? If not, the long version
should be used everywhere instead. Otherwise, one user can appear to be another in logs, etc.
    
    If "getShortUserName" is not unique, it should avoided everywhere.



test/src/test/java/org/apache/accumulo/server/security/SystemCredentialsIT.java
<https://reviews.apache.org/r/29386/#comment109933>

    This change reduces the coverage and does not behave the way that was being tested.
    
    Previously, we were able to verify that the test properly failed if the information used
to generate the unique system password for that instance, was incorrect (e.g. a system user
from another instance).
    
    Now, it seems to be testing that a regular user named "!SYSTEM" with a "fake" password
doesn't exist. This is not correct.


- Christopher Tubbs


On Dec. 30, 2014, 5:31 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, 5:31 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