Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============6835108343795834204=="
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>
Date: Wed, 07 Jan 2015 17:39:26 -0000
Message-ID: <20150107173926.26561.77196@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Josh Elser" <noreply@reviews.apache.org>
References: <20141230224602.31225.80952@reviews.apache.org>
In-Reply-To: <20141230224602.31225.80952@reviews.apache.org>
Reply-To: "Josh Elser" <josh.elser@gmail.com>

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


> On Dec. 30, 2014, 10:46 p.m., Christopher Tubbs wrote:
> > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java, line 41
> > <https://reviews.apache.org/r/29386/diff/4/?file=803164#file803164line41>
> >
> >     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.
> 
> Josh Elser wrote:
>     I understand the intentions for ACCUMULO-1300; however, that's not implemented. Being able to leverage the existing ZKAuthorizor was wonderful so I really don't want to move away from how this fundamentally works.
>     
>     Shouldn't the Authenticator and Authorizor be fixed per instance (allowing reset via `accumulo init --reset-security` or w/e)? What do you actually want changed aside from documentation to make it clear to administrators (I haven't written any documentation yet).
> 
> Christopher Tubbs wrote:
>     So, I pointed out in the conversation around ACCUMULO-259, that I would prefer a pluggable security module that integrated all the security-related functions, because they are so dependent on each other. However, they were implemented as 3 separately configurable pluggable components. I would still like to see them merged (which doesn't preclude making one from modular components themselves).
>     
>     For now, I'm not really sure what it means for somebody to "create a user" when the authenticator already validates them. That's why it's confusing. Are we essentially making ZKAuthenticator Kerberos-aware, or are we providing a separate Kerberos Authenticator? That's really what it boils down to for me. It seems like we're doing a hybrid... but I think we should solidify on one or the other.
> 
> Josh Elser wrote:
>     I view it as a Kerberos Authenticator. The implementation details of that authenticator is that is happens to persist information into ZK to handle things like permissions and authorizations, but a user doesn't have to know that to use it. We could, hypothetically, swap out ZK for any other persistent store for that information and it would continue to work.
>     
>     What would help alleviate your confusion? More javadocs? A different class name?
> 
> Christopher Tubbs wrote:
>     Well, part is documentation. Part is behavior.
>     
>     If it's a separate authenticator, we should make sure that it does not use the same storage area in such a tightly coupled way. It should not, for instance, be setting things that look like passwords, and "create local user", "list local users", and "delete local users" should be unsupported client side operations, just as you've done with "change password" (since users are managed within Kerberos). We already share storage between the ZKAuthenticator and the corresponding permissions handler and authorizer, so it's probably okay to share the directory identified with the principal. However, it can seamlessly create this directory as needed.
>     
>     Mainly, I'm thinking the more we can separate the "local user" management functions from the KerberosAuthenticator, the better, since users are managed in Kerberos. Some of my concerns are with the current state of things, but I'm hoping not to exacerbate the problems I see with the current implementations. I don't want to make it harder to make improvements later. I'd rather move towards a better design, then simply add features to the existing ad-hoc design.
> 
> Josh Elser wrote:
>     bq. It should not, for instance, be setting things that look like passwords, and "create local user", "list local users", and "delete local users" should be unsupported client side operations, just as you've done with "change password" (since users are managed within Kerberos)
>     
>     Thanks for pointing that out. That helps me understand where your concerns are coming from. Perhaps it would be cleaner to just keep a reference to the ZkAuthenticator as a delegate (instead of extending it). This would give us a bit more obvious control over what's happening.
>     
>     bq. I'm thinking the more we can separate the "local user" management functions from the KerberosAuthenticator, the better, since users are managed in Kerberos. Some of my concerns are with the current state of things, but I'm hoping not to exacerbate the problems I see with the current implementations
>     
>     Agreed, those are very.. awkward, presently. Is making sure that we throw exceptions from KerberosAuthenticator when we call these non-sensical methods a sufficient change? Then, we can move to a better design later on like you said?
> 
> Christopher Tubbs wrote:
>     What do you actually need the ZKAuthenticator for? Just to create the directory, so the ZKPermHandler and ZKAuthorizor have a place to put stuff? It might be better to sever completely (vs. delegate or subclass), and just use a common constant for the ZK Path to create the user directories to make those other two components happy (assuming that's all that ZKAuthenticator was needed for).
>     
>     Throwing an exception from KerberosAuthenticator might work... I'm more concerned about what happens on the client side, and these may not map one-to-one. So, a closer inspection/testing will be need to ensure that's sufficient.
>     
>     The important bit about implementing additional Authenticators, is that when a user is deleted in the external system, somebody needs to call the cleanup/dropuser methods on the PermHandler/Authorizor. That will need to be documented. And, it's a bit weird, because the actual interfaces aren't public API (it's one of the reasons why I prefer a single, comprehensive pluggable security module, because it helps consolidate this kind of integration between components).
> 
> Josh Elser wrote:
>     > What do you actually need the ZKAuthenticator for? Just to create the directory, so the ZKPermHandler and ZKAuthorizor have a place to put stuff?
>     
>     Yes. It also lets us know what users currently exist (even though any new user *could* come in at any point). I'm not sure what else the ZKAuthenticator is magically handling for me behind the scenes. I'd need to re-audit it.
>     
>     > Throwing an exception from KerberosAuthenticator might work... I'm more concerned about what happens on the client side, and these may not map one-to-one. So, a closer inspection/testing will be need to ensure that's sufficient.
>     
>     Will do.
>     
>     > The important bit about implementing additional Authenticators, is that when a user is deleted in the external system, somebody needs to call the cleanup/dropuser methods on the PermHandler/Authorizor. That will need to be documented. And, it's a bit weird, because the actual interfaces aren't public API (it's one of the reasons why I prefer a single, comprehensive pluggable security module, because it helps consolidate this kind of integration between components).
>     
>     Noted and understand/agree about the security module. Will be clear as clear as possible in the docs to outline the situation.

I cleaned up the logic in KerberosAuthenticator to remove the random password bits that were required from directly using the ZKAuthenticator which did clean things up a little bit. I think leaving in dropUser, listUsers and createUser do each have merits as long as they are properly explained in documentation (as I plan to do today).

dropUser allows a simple way for all perms/auths to be reset on a user (even if they might come back because of their Kerberos auths), listUsers can show the users who have actually connected to Accumulo (known by us, not just the KDC) and createUser can allow an admin to seed permissions/authorizations without the user authenticating with the system once (this also prevents us from having to alter how SecurityOperations works in the kerberos case). I still agree that all three of these can be better handled with a more centralized approach and a less password-centric client API to implement.


- Josh


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


On Jan. 7, 2015, 6:16 a.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29386/
> -----------------------------------------------------------
> 
> (Updated Jan. 7, 2015, 6:16 a.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/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/main/java/org/apache/accumulo/server/util/Admin.java ae36f1f 
>   server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java 7fdbf13 
>   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 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
> 
>


--===============6835108343795834204==--