Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review66393 --- core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java https://reviews.apache.org/r/29386/#comment109875 It seems like the expectation is that `getPrincipal()` would be called now? If so can this be made private? Can't remember if jcommander is ok w/ that. core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java https://reviews.apache.org/r/29386/#comment109876 Should this also be private? core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java https://reviews.apache.org/r/29386/#comment109877 Should this be private? core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java https://reviews.apache.org/r/29386/#comment109878 should these have javadoc w/ since tags? core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java https://reviews.apache.org/r/29386/#comment109879 needs @since tag core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java https://reviews.apache.org/r/29386/#comment109880 needs @since tag core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java https://reviews.apache.org/r/29386/#comment109886 javadoc explaining what serverKerberosPrincipal arg is would be nice. core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java https://reviews.apache.org/r/29386/#comment109881 needs @sice tag core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java https://reviews.apache.org/r/29386/#comment109884 This is adding UserGroupInformation to Accumulo API. Is that a stable hadoop API? Does it have to be in API, why not just principal? core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java https://reviews.apache.org/r/29386/#comment109885 why is this a noop? server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java https://reviews.apache.org/r/29386/#comment109883 why did this method need to be added? server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java https://reviews.apache.org/r/29386/#comment109882 Could offer a command line option to set root username for non-interactive cases. - kturner On Dec. 30, 2014, 4:01 a.m., Josh Elser wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/ --- (Updated Dec. 30, 2014, 4:01 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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review66394 --- core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java https://reviews.apache.org/r/29386/#comment109887 Not sure this is correct. Should probably just be a no-op, from looking at the javadoc on Destroyable. core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java https://reviews.apache.org/r/29386/#comment109888 Narrow the return value to KerberosToken. core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java https://reviews.apache.org/r/29386/#comment109891 UGI.getShortName() is not guaranteed to be non-null, so we should be more defensive either here or in the constructor. core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransport.java https://reviews.apache.org/r/29386/#comment109895 Does Java 7 allows for multi-catch blocks, or was that in 8? core/src/main/java/org/apache/accumulo/core/security/Credentials.java https://reviews.apache.org/r/29386/#comment109896 Why have this? - Mike Drob On Dec. 30, 2014, 4:01 a.m., Josh Elser wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/ --- (Updated Dec. 30, 2014, 4:01 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/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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/ --- (Updated Dec. 30, 2014, 9:17 p.m.) Review request for accumulo. Changes --- Added some unit tests for the changes so far. 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 (updated) - 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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
On Dec. 30, 2014, 7:39 p.m., kturner wrote: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java, line 109 https://reviews.apache.org/r/29386/diff/4/?file=803139#file803139line109 It seems like the expectation is that `getPrincipal()` would be called now? If so can this be made private? Can't remember if jcommander is ok w/ that. I'm not sure if JCommander supports that (my gut reaction is no), but there might also be public API implications of that... On Dec. 30, 2014, 7:39 p.m., kturner wrote: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java, line 118 https://reviews.apache.org/r/29386/diff/4/?file=803139#file803139line118 Should this also be private? Same comments as the principal. On Dec. 30, 2014, 7:39 p.m., kturner wrote: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java, line 201 https://reviews.apache.org/r/29386/diff/4/?file=803139#file803139line201 Should this be private? Probably protected or package-protected (with a note about not intended for public use), but I made it public just because startDebugLogging and startTracing were also public. If we make it private, it would make testing it harder. 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? 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. On Dec. 30, 2014, 7:39 p.m., kturner wrote: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java, line 63 https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line63 why is this a noop? The SASL transport in thrift is actually doing the work of sending the kerberos credentials over the wire for our RPC use. We have a TProcessor and a wrapper around the thrift interface implementation which passes this information from the thrift layer back into our Credentials objects which alleviates us from having to do (nearly) anything in the KerberosToken itself. I'm not sure if there is a better way to do it, but I'm open to suggestions. On Dec. 30, 2014, 7:39 p.m., kturner wrote: server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java, line 540 https://reviews.apache.org/r/29386/diff/4/?file=803157#file803157line540 why did this method need to be added? Client authentication with Kerberos implies that any user with valid Kerberos credentials can be authenticated with the system transparently. As such, the notion of a root super-user goes out the window -- it doesn't make sense to have a root user because that would require a new kerberos identity to be created just to delegate permissions to an actual user. By changing `accumulo init` to accept a root user (and granting the necessary system permissions to that user), we remove the need to have some extra user created only for the purposes of bootstraping the Accumulo permissions. Alternatively, we could also require that the user running `accumulo init` has Kerberos credentials and give that user system permissions. I could see this work either way. On Dec. 30, 2014, 7:39 p.m., kturner wrote: server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java, line 550 https://reviews.apache.org/r/29386/diff/4/?file=803157#file803157line550 Could offer a command line option to set root username for non-interactive cases. Will do. - Josh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review66393 --- On Dec. 30, 2014, 9:17 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, 9:17 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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
On Dec. 30, 2014, 8:44 p.m., Mike Drob wrote: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java, line 73 https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line73 Not sure this is correct. Should probably just be a no-op, from looking at the javadoc on Destroyable. Ok, I wasn't really sure how this was actually supposed to work. I wasn't sure if there's something we need to do to the UGI if/when destroy() is called -- of course, I haven't a clue when destroy() is even called... On Dec. 30, 2014, 8:44 p.m., Mike Drob wrote: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java, line 98 https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line98 UGI.getShortName() is not guaranteed to be non-null, so we should be more defensive either here or in the constructor. Hrm, I'm not sure I understand how the subject would not have a primary to extract as the shortName (if it's possible to actually make getShortName() return us null), but it's an easy check to make. On Dec. 30, 2014, 8:44 p.m., Mike Drob wrote: core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransport.java, lines 61-64 https://reviews.apache.org/r/29386/diff/4/?file=803151#file803151line61 Does Java 7 allows for multi-catch blocks, or was that in 8? Yup, that was Java7. I need to clean this up anyways -- thanks for pointing it out. On Dec. 30, 2014, 8:44 p.m., Mike Drob wrote: core/src/main/java/org/apache/accumulo/core/security/Credentials.java, lines 43-44 https://reviews.apache.org/r/29386/diff/4/?file=803153#file803153line43 Why have this? I had something in here at some point in time, but then removed it. Rather than just re-deleting the logger (to assumable re-add it again some day, I just left it in place). I can remove it if it bothers you. - Josh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review66394 --- On Dec. 30, 2014, 9:17 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, 9:17 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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/ --- (Updated Dec. 30, 2014, 10:31 p.m.) Review request for accumulo. Changes --- Thanks again Mike and Keith for the review. Javadoc additions, use of AtomicReferences instead catch/throw/catch/unwrap/throw block, better return type on clone(), make destory() a noop, `-u` option on initialize, and no prompt for password for root user w/ SASL enabled on init. 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 (updated) - 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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
--- 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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
On Dec. 30, 2014, 3:44 p.m., Mike Drob wrote: core/src/main/java/org/apache/accumulo/core/security/Credentials.java, lines 43-44 https://reviews.apache.org/r/29386/diff/4/?file=803153#file803153line43 Why have this? Josh Elser wrote: I had something in here at some point in time, but then removed it. Rather than just re-deleting the logger (to assumable re-add it again some day, I just left it in place). I can remove it if it bothers you. I don't like warning suppression. We can add it later. I also noted this issue in my review, below. (I'll close mine, to de-dupe). - Christopher --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review66394 --- 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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
On Dec. 30, 2014, 5:46 p.m., Christopher Tubbs wrote: core/src/main/java/org/apache/accumulo/core/security/Credentials.java, lines 43-44 https://reviews.apache.org/r/29386/diff/4/?file=803153#file803153line43 Why add it if it's not used, only to suppress it's lack of use? Duplicate of Mike's comment. - Christopher --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review66382 --- 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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
On Dec. 30, 2014, 2:39 p.m., kturner wrote: server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java, line 550 https://reviews.apache.org/r/29386/diff/4/?file=803157#file803157line550 Could offer a command line option to set root username for non-interactive cases. Josh Elser wrote: Will do. It should be noted that the idea of adding a configurable root user name was rejected outright in the reviews of ACCUMULO-259, in favor of ACCUMULO-1300 (and related issues) as the way forward. Authenticators should really be independent, and manage their own users / permissions / etc., rather than make init do it. This was discussed at length prior to the 1.5 release, for ACCUMULO-259. - Christopher --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review66393 --- 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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/ --- (Updated Dec. 30, 2014, 11:16 p.m.) Review request for accumulo. Changes --- Avoid the re-use of GENERAL_KERBEROS_PRINCIPAL in client configuration as it implies that the client has to be aware of the instance component of the principal that servers use when they only need the primary and realm. Automatically handle the conversion for servers when they're acting as clients. I've been waffling over this for a while, but being specific in the client properties will be better long term than introducing some property we half use. 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 (updated) - 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
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
On Dec. 30, 2014, 10:46 p.m., Christopher Tubbs wrote: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java, lines 272-277 https://reviews.apache.org/r/29386/diff/4/?file=803139#file803139line272 Are these mutually exclusive? Yes, as far as I understand it. Hypothetically, I don't think they have to be, but the thrift implementations won't properly work. We have numerous checks on the server side to ensure exclusivity (see above discussion on the subject), but I'm not sure if one of them will also cover client side. Will investigate. On Dec. 30, 2014, 10:46 p.m., Christopher Tubbs wrote: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java, line 41 https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line41 This link tag is bad. It should be UserGroupInformation, not ugi. Meant to use `codeugi/code` to refer to the parameter, not a link. On Dec. 30, 2014, 10:46 p.m., Christopher Tubbs wrote: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java, lines 52-69 https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line52 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. MapReduce is something I need to investigate and test. I have no idea how token delegation works with MapReduce (if it's anything more than use a keytab). If it's that, this approach should be fine. Otherwise, it'll be problematic. I've also switched this from loginUser to currentUser (I think in the most recent patch) as I think loginUser was incorrect. I'll add some magic bytes to the serialization. On Dec. 30, 2014, 10:46 p.m., Christopher Tubbs wrote: core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java, lines 71-79 https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line71 Why is this unsupported? Again, see earlier discussion. I have no idea what this is actually supposed to destroy (we have zero documentation on the matter that I found) and it's already been changed to be a noop. I am just as confident that a noop is correct as an unsupported operation is correct (I have no confidence). As far as destroying a UGI, I haven't found any parallel to the Destroyable interface. I'm not sure if something exists that we should be calling. On Dec. 30, 2014, 10:46 p.m., Christopher Tubbs wrote: core/src/main/java/org/apache/accumulo/core/conf/Property.java, lines 150-153 https://reviews.apache.org/r/29386/diff/4/?file=803146#file803146line150 Descriptions should explain if these are mutually exclusive. Or, at least, that auth-conf obviates the need for ssl. Will add a note about SSL exclusivity. I don't know enough to compare the encryption of SSL compared to auth-conf so I don't feel comfortable documenting anything on the subject (yet). On Dec. 30, 2014, 10:46 p.m., Christopher Tubbs wrote: core/src/main/java/org/apache/accumulo/core/conf/Property.java, lines 165-167 https://reviews.apache.org/r/29386/diff/4/?file=803146#file803146line165 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? Again, see the previous discussion on this subject. This is necessary to actually perform an apples to apples comparison of the impact of Kerberos on Accumulo. Comparing KRB/SASL with TThreadPoolServer against no KRB/SASL and THsHaServer is an invalid benchmark to determine the overhead of KRB. Like I said earlier, `Experimental` is not really the appropriate annotation here (Internal use would be more accurate), but it's what we have. Do you have a recommendation on some other change in wording/annotation to convey the intent? I'd like to not have to change the codebase any time I want to verify no performance regressions in future versions (having to change code would preclude any non-devs from running their own benchmarks too). On Dec. 30, 2014, 10:46 p.m., Christopher Tubbs wrote: core/src/main/java/org/apache/accumulo/core/rpc/SaslConnectionParams.java, lines 80-81 https://reviews.apache.org/r/29386/diff/4/?file=803148#file803148line80
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
On Dec. 30, 2014, 10:46 p.m., Christopher Tubbs wrote: proxy/src/main/java/org/apache/accumulo/proxy/Proxy.java, lines 133-136 https://reviews.apache.org/r/29386/diff/4/?file=803155#file803155line133 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. Josh Elser wrote: This code is already very problematic (in that it doesn't use TServerUtils). I forget if I made a ticket for this yet, but I will make sure it gets documented in an isue. Created ACCUMULO-3459 - Josh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review66382 --- On Dec. 30, 2014, 11:16 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, 11:16 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
Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29502/ --- (Updated Dec. 31, 2014, 4:05 a.m.) Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner. Changes --- Added accumulo group to review. Repository: accumulo Description --- ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them. Diffs - core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION Diff: https://reviews.apache.org/r/29502/diff/ Testing --- Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment Thanks, Corey Nolet
Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29502/#review66439 --- Neat stuff, Corey! core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java https://reviews.apache.org/r/29502/#comment109983 Can't you pull this from the Scanner? server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java https://reviews.apache.org/r/29502/#comment109984 It looks like TabletIteratorEnvironment is used for minor compactions. Isn't always setting `Authorizations.EMPTY` a little misleading? Is there something more representative of having all auths we could do here? Maybe extra documentation is enough? Could also throw UnsupportedOperationException or similar when the IteratorScope is something that isn't SCAN? test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java https://reviews.apache.org/r/29502/#comment109986 Please create a user, assign it the auths you need, and then remove the user after the test. If this test is run against a standalone instance, it should try to leave the system in the same state the test started in. test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java https://reviews.apache.org/r/29502/#comment109985 Close the batchwriter. - Josh Elser On Dec. 31, 2014, 4:05 a.m., Corey Nolet wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29502/ --- (Updated Dec. 31, 2014, 4:05 a.m.) Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner. Repository: accumulo Description --- ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them. Diffs - core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION Diff: https://reviews.apache.org/r/29502/diff/ Testing --- Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment Thanks, Corey Nolet
Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/ --- (Updated Dec. 31, 2014, 4:36 a.m.) Review request for accumulo. Changes --- Review recommendations from Christopher. 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 (updated) - 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
