> On Dec. 30, 2014, 7:39 p.m., kturner wrote: > > core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java, > > line 46 > > <https://reviews.apache.org/r/29386/diff/4/?file=803145#file803145line46> > > > > This is adding UserGroupInformation to Accumulo API. Is that a stable > > hadoop API? Does it have to be in API, why not just principal? > > Josh Elser wrote: > UGI is marked as LimitedPrivate and Evolving, but I have no qualms > against including it into our API. The problem with accepting a principal > alone, we would then have to duplicate the login capabilities that UGI > already provides (e.g performing a login via password or keytab). By > accepting a UGI directly, we don't have to "own" this logic. It's my opinion > that it's easiest to just use UGI directly, but could see an argument for > "transparently" using it behind the scenes if you think that would be better. > > kturner wrote: > > I have no qualms against including it into our API > > Why do you think its ok to ignore the API marking? Shouldnt we honor > whats marked on the assumption that someone in Hadoop land will think they > are free to make breaking changes based on whats makred? > > > but could see an argument for "transparently" using it behind the > scenes if you think that would be better. > > From an implementation perspective, I don't know whats best. I don't > know what the benefits of using UGI are. What are the problems with not > using it? Would not using it require a complex facade class? Would not > using it cause interoperability problems with others using Kerberos in the > hadoop ecosystem? > > If this object is really useful to users of Hadoop and we want to use it > in our API, I would advocate for opening a hadoop issue to change the > stability guarantees. I am uncertain if we should use it before this change > is made, but thats because I don't fully understand the details. > > Josh Elser wrote: > bq. Why do you think its ok to ignore the API marking? Shouldnt we honor > whats marked on the assumption that someone in Hadoop land will think they > are free to make breaking changes based on whats makred? > > The only difference between Stable and Evolving is that breaking changes > could happen at minor releases instead of just major releases (akin to > semver). So, just to make a point, even using only Stable classes will result > in us having to worry about breaking changes. > > It's possible that we could use a very thin shim around the most often > used methods of UGI, but I'm worried that would be insufficient. For example, > most people use `UserGroupInformation.loginUserFromKeytab` or they are have a > cached ticket (from `kinit`'ing on the command line before invoking the > application) which we can get access to via > `UserGroupInformation.getCurrentUser`. The other approach is to use > `UserGroupInformation.createProxyUser` which accepts a principal and > "proxies" another user on top of another UGI -- this is very useful for us as > it eliminates the need to do Authorization intersection as some "server-user" > to maintain application security. The server is running an Accumulo query for > the user; however Accumulo "sees" the user, not the server. > > Back to the point though: this actual assertion is a quick fail to make > sure that the user trying to use Kerberos is actually logged in (catch it and > fail before we actually try to send an RPC). We could push this farther down > into the codebase -- inside ConnectorImpl maybe? -- which would even moreso > make this a "shell" AuthenticationToken. To clarify, your biggest worry is > the UGI argument in the constructor and not using it in the implementation, > right? > > kturner wrote: > > To clarify, your biggest worry is the UGI argument in the constructor > and not using it in the implementation, right? > > Correct. This patch adds KerberosToken to the public API w/ a public > method using UGI. I would be much less concerned if UGI were only in > implementation of KerberosToken as that gives us a lot more wiggle room. > > kturner wrote: > > The only difference between Stable and Evolving is that breaking > changes could happen at minor releases instead of just major releases (akin > to semver). > > Will Hadoop deprecate before changing in a minor release? Trying to think > through case where a minor hadoop release moves UGI. If Hadoop does not > deprecate and just moves it, we would be up the creek w/o a paddle. W/o > deprecation we would be forced to deprecated and introduce a facade anyway > because we can depend on two hadoop releases simultaneously. > > 1. Accumulo 1.7.0 release API method w/ UGI in signature, lets call this > method foo1(UGI_A). > 2. Hadoop release 2.9.0 which moves UGI_A to UGI_B AND leaves UGI_A as > deprecated. > 3. Accumulo must release 1.8.0 w/ foo1(UGI_A) deprecated and introduce > foo1(UGI_B). > > Josh Elser wrote: > To bring some closure to this (in addition to something odd I noticed > today). UGI (the class) is marked as limited private, while the static > methods to login as marked as public. I've already removed UGI from the > signatures itself which means that clients are expected to call those > (public) Hadoop methods to log in before trying to connect to Accumulo. I > think this is the best mix of adhering to Hadoop stability markings and not > polluting our API with unnecessary args. The String arg constructor on > KerberosToken can be used as a sanity check for clients to verify that > they're logged in as who they think they are. > > Christopher Tubbs wrote: > Another thing to consider here is that AuthenticationTokens in Accumulo > have a "login" concept, also. You can provide properties to be utilized by a > token to "log in" (passphrase to Kerberos account?), and the resulting > credentials (keytab? not sure I'm getting terminology correct) could be > stored in the token. Doing this would make it slightly more functional, > especially with the proxy, and it would also give more clarity on how > "destroy" should be used.
UGI doesn't expose anything that will let us prompt for password (we would have to make a local ticket cache and do that ourselves), but we could consider adding a keytab option to the KerberosToken. That might make a nice experience for headless applications that leverage ClientOpts. - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review66393 ----------------------------------------------------------- On Jan. 8, 2015, 11:52 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29386/ > ----------------------------------------------------------- > > (Updated Jan. 8, 2015, 11:52 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 > ----- > > README ad6f2bf > 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 > docs/src/main/asciidoc/accumulo_user_manual.asciidoc ec8e538 > docs/src/main/asciidoc/chapters/clients.txt 64f0e55 > docs/src/main/asciidoc/chapters/kerberos.txt 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/security/handler/KerberosAuthorizor.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosPermissionHandler.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/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/Basic.java > 2d98fed > server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java > 7e33300 > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > d5c1d2f > > server/tserver/src/main/java/org/apache/accumulo/tserver/replication/ReplicationWorker.java > 1d20e2b > 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/SharedMiniClusterIT.java > 2380f66 > test/src/test/java/org/apache/accumulo/harness/TestingKdc.java PRE-CREATION > > 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/java/org/apache/accumulo/test/security/KerberosTokenTest.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 > >
