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