----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1059/#review1019 -----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxn.java <https://reviews.apache.org/r/1059/#comment2094> we don't want to make the state visible. we don't want applications to have to deal with new states, especially ones that depend on the authentication type. src/java/main/org/apache/zookeeper/ClientCnxn.java <https://reviews.apache.org/r/1059/#comment2095> these don't seem like equivalent states. CONNECTEDREADONLY also needs SASL src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java <https://reviews.apache.org/r/1059/#comment2096> this should be done in the session verification of at the ServerCnxn not here. src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java <https://reviews.apache.org/r/1059/#comment2097> this should also be done much closer to the server connection. probably in ZooKeeperServer.processPacket(). that is where the auth processing is being done anyway. src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java <https://reviews.apache.org/r/1059/#comment2098> the requireClientAuthScheme is general functionality that belongs here, but otherwise there is too much SASL code here. this logic should be moved into another class, perhaps SASLServerCallbackHandler, so that we are just hooking into ServerCnxnFactory. src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java <https://reviews.apache.org/r/1059/#comment2099> i don't think we should be passing the SASL stuff through ServerCnxnFactory so non-opaquely. the configuration will set zookeeper.xxxx system properties from the config file and the SASL handler can grab them there. src/java/main/org/apache/zookeeper/server/ServerConfig.java <https://reviews.apache.org/r/1059/#comment2100> these are properties used by a specific auth provider, we shouldn't have them exposed in a general class. src/java/main/org/apache/zookeeper/server/ServerConfig.java <https://reviews.apache.org/r/1059/#comment2101> sasl should be able to figure out the renewtime based on the token since the token comes with an expiration. right? src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java <https://reviews.apache.org/r/1059/#comment2093> is there really no checking that we can do here? src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/1059/#comment2102> this property is too specific to be exposed here. - Benjamin On 2011-07-09 19:51:29, Eugene Koontz wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1059/ > ----------------------------------------------------------- > > (Updated 2011-07-09 19:51:29) > > > Review request for zookeeper. > > > Summary > ------- > > Support Kerberos authentication of clients. > > > This addresses bug ZOOKEEPER-938. > https://issues.apache.org/jira/browse/ZOOKEEPER-938 > > > Diffs > ----- > > src/java/main/org/apache/zookeeper/ClientCnxn.java 87477df > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java d0e74fa > src/java/main/org/apache/zookeeper/LoginThread.java PRE-CREATION > src/java/main/org/apache/zookeeper/Watcher.java e72105c > src/java/main/org/apache/zookeeper/ZooDefs.java f77ac20 > src/java/main/org/apache/zookeeper/ZooKeeper.java f2ab4a6 > src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java > b690817 > src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java bab8998 > src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java e7e1846 > src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java c6ab5dd > src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java > deb1e7a > src/java/main/org/apache/zookeeper/server/Request.java 80d2b99 > src/java/main/org/apache/zookeeper/server/SaslServerCallbackHandler.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6d69073 > src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 72af158 > src/java/main/org/apache/zookeeper/server/ServerConfig.java ec710cd > src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 52d3820 > src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > c6d9c09 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java > 969a482 > src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java PRE-CREATION > src/java/test/org/apache/zookeeper/test/SaslAuthTest.java PRE-CREATION > src/zookeeper.jute 34eac78 > > Diff: https://reviews.apache.org/r/1059/diff > > > Testing > ------- > > Passes all unit tests on my local machine. > > Also tested in a kerberos-enabled 3-node quorum in an HBase cluster which was > then loaded with data using the Faulkner load test > (https://github.com/ekoontz/hbase-ec2/blob/4a3a1a11810ed6acd96593e2818cbd7d2ffc8950/faulkner/faulkner.rb) > > > Thanks, > > Eugene > >
