Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============8493551628951363511=="
MIME-Version: 1.0
Subject: Re: Review Request 29386: ACCUMULO-2815 Client authentication via
 Kerberos
From: "Josh Elser" <josh.elser@gmail.com>
To: "Christopher Tubbs" <ctubbsii-rb@apache.org>,
 "accumulo" <dev@accumulo.apache.org>, "Josh Elser" <josh.elser@gmail.com>,
 keith@deenlo.com
Date: Wed, 07 Jan 2015 01:28:41 -0000
Message-ID: <20150107012841.26560.56511@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Josh Elser" <noreply@reviews.apache.org>
References: <20141230193911.31225.36742@reviews.apache.org>
In-Reply-To: <20141230193911.31225.36742@reviews.apache.org>
Reply-To: "Josh Elser" <josh.elser@gmail.com>

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


> 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?
> 
> Josh Elser wrote:
>     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.
> 
> kturner wrote:
>     > I have no qualms against including it into our API
>     
>     Why do you think its ok to ignore the API marking?  Shouldnt we honor whats marked on the assumption that someone in Hadoop land will think they are free to make breaking changes based on whats makred?
>     
>     > but could see an argument for "transparently" using it behind the scenes if you think that would be better.
>     
>     From an implementation perspective, I don't know whats best.  I don't know what the benefits of using UGI are.  What are the problems with not using it?  Would not using it require a complex facade class?  Would not using it cause interoperability problems with others using Kerberos in the hadoop ecosystem?  
>     
>     If this object is really useful to users of Hadoop and we want to use it in our API, I would advocate for opening a hadoop issue to change the stability guarantees.  I am uncertain if we should use it before this change is made, but thats because I don't fully understand the details.
> 
> Josh Elser wrote:
>     bq. Why do you think its ok to ignore the API marking? Shouldnt we honor whats marked on the assumption that someone in Hadoop land will think they are free to make breaking changes based on whats makred?
>     
>     The only difference between Stable and Evolving is that breaking changes could happen at minor releases instead of just major releases (akin to semver). So, just to make a point, even using only Stable classes will result in us having to worry about breaking changes.
>     
>     It's possible that we could use a very thin shim around the most often used methods of UGI, but I'm worried that would be insufficient. For example, most people use `UserGroupInformation.loginUserFromKeytab` or they are have a cached ticket (from `kinit`'ing on the command line before invoking the application) which we can get access to via `UserGroupInformation.getCurrentUser`. The other approach is to use `UserGroupInformation.createProxyUser` which accepts a principal and "proxies" another user on top of another UGI -- this is very useful for us as it eliminates the need to do Authorization intersection as some "server-user" to maintain application security. The server is running an Accumulo query for the user; however Accumulo "sees" the user, not the server.
>     
>     Back to the point though: this actual assertion is a quick fail to make sure that the user trying to use Kerberos is actually logged in (catch it and fail before we actually try to send an RPC). We could push this farther down into the codebase -- inside ConnectorImpl maybe? -- which would even moreso make this a "shell" AuthenticationToken. To clarify, your biggest worry is the UGI argument in the constructor and not using it in the implementation, right?
> 
> kturner wrote:
>     > To clarify, your biggest worry is the UGI argument in the constructor and not using it in the implementation, right?
>     
>     Correct.  This patch adds KerberosToken to the public API w/ a public method using UGI.  I would be much less concerned if UGI were only in implementation of KerberosToken as that gives us a lot more wiggle room.
> 
> kturner wrote:
>     > The only difference between Stable and Evolving is that breaking changes could happen at minor releases instead of just major releases (akin to semver). 
>     
>     Will Hadoop deprecate before changing in a minor release? Trying to think through case where a minor hadoop release moves UGI.  If Hadoop does not deprecate and just moves it, we would be up the creek w/o a paddle.  W/o deprecation we would be forced to deprecated and introduce a facade anyway because we can depend on two hadoop releases simultaneously.
>     
>      1. Accumulo 1.7.0 release API method w/ UGI in signature, lets call this method foo1(UGI_A).
>      2. Hadoop release 2.9.0 which moves UGI_A to UGI_B AND leaves UGI_A as deprecated.
>      3. Accumulo must release 1.8.0 w/ foo1(UGI_A) deprecated and introduce foo1(UGI_B).

To bring some closure to this (in addition to something odd I noticed today). UGI (the class) is marked as limited private, while the static methods to login as marked as public. I've already removed UGI from the signatures itself which means that clients are expected to call those (public) Hadoop methods to log in before trying to connect to Accumulo. I think this is the best mix of adhering to Hadoop stability markings and not polluting our API with unnecessary args. The String arg constructor on KerberosToken can be used as a sanity check for clients to verify that they're logged in as who they think they are.


- Josh


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


On Jan. 6, 2015, 11:14 p.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29386/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 11:14 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/MasterClient.java a9ad8a1 
>   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/AbstractKerberosToken.java PRE-CREATION 
>   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/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 
>   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 ae188a0 
>   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/pom.xml b0a926f 
>   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/harness/AccumuloClusterIT.java 8f7e1b7 
>   test/src/test/java/org/apache/accumulo/harness/MiniClusterHarness.java abdb627 
>   test/src/test/java/org/apache/accumulo/harness/MiniClusterKdc.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/harness/SharedMiniClusterIT.java 2380f66 
>   test/src/test/java/org/apache/accumulo/harness/conf/AccumuloMiniClusterConfiguration.java 11b7530 
>   test/src/test/java/org/apache/accumulo/server/security/SystemCredentialsIT.java fb71f5f 
>   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/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
> 
>


--===============8493551628951363511==--