[ https://issues.apache.org/jira/browse/ZOOKEEPER-938?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13062658#comment-13062658 ]
jirapos...@reviews.apache.org commented on ZOOKEEPER-938: --------------------------------------------------------- ----------------------------------------------------------- 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: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/1059/ bq. ----------------------------------------------------------- bq. bq. (Updated 2011-07-09 19:51:29) bq. bq. bq. Review request for zookeeper. bq. bq. bq. Summary bq. ------- bq. bq. Support Kerberos authentication of clients. bq. bq. bq. This addresses bug ZOOKEEPER-938. bq. https://issues.apache.org/jira/browse/ZOOKEEPER-938 bq. bq. bq. Diffs bq. ----- bq. bq. src/java/main/org/apache/zookeeper/ClientCnxn.java 87477df bq. src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java d0e74fa bq. src/java/main/org/apache/zookeeper/LoginThread.java PRE-CREATION bq. src/java/main/org/apache/zookeeper/Watcher.java e72105c bq. src/java/main/org/apache/zookeeper/ZooDefs.java f77ac20 bq. src/java/main/org/apache/zookeeper/ZooKeeper.java f2ab4a6 bq. src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java PRE-CREATION bq. src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java b690817 bq. src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java bab8998 bq. src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java e7e1846 bq. src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java c6ab5dd bq. src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java deb1e7a bq. src/java/main/org/apache/zookeeper/server/Request.java 80d2b99 bq. src/java/main/org/apache/zookeeper/server/SaslServerCallbackHandler.java PRE-CREATION bq. src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6d69073 bq. src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 72af158 bq. src/java/main/org/apache/zookeeper/server/ServerConfig.java ec710cd bq. src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java PRE-CREATION bq. src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 52d3820 bq. src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java PRE-CREATION bq. src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java PRE-CREATION bq. src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java c6d9c09 bq. src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 969a482 bq. src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java PRE-CREATION bq. src/java/test/org/apache/zookeeper/test/SaslAuthTest.java PRE-CREATION bq. src/zookeeper.jute 34eac78 bq. bq. Diff: https://reviews.apache.org/r/1059/diff bq. bq. bq. Testing bq. ------- bq. bq. Passes all unit tests on my local machine. bq. bq. 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) bq. bq. bq. Thanks, bq. bq. Eugene bq. bq. > Support Kerberos authentication of clients. > ------------------------------------------- > > Key: ZOOKEEPER-938 > URL: https://issues.apache.org/jira/browse/ZOOKEEPER-938 > Project: ZooKeeper > Issue Type: New Feature > Components: java client, server > Reporter: Eugene Koontz > Assignee: Eugene Koontz > Fix For: 3.4.0, 3.5.0 > > Attachments: NIOServerCnxn.patch, ZOOKEEPER-938.patch, > ZOOKEEPER-938.patch, ZOOKEEPER-938.patch, ZOOKEEPER-938.patch, > ZOOKEEPER-938.patch, ZOOKEEPER-938.patch, ZOOKEEPER-938.patch, > ZOOKEEPER-938.patch, ZOOKEEPER-938.patch, ZOOKEEPER-938.patch, > ZOOKEEPER-938.patch, jaas.conf, sasl.patch > > > Support Kerberos authentication of clients. > The following usage would let an admin use Kerberos authentication to assign > ACLs to authenticated clients. > 1. Admin logs into zookeeper (not necessarily through Kerberos however). > 2. Admin decides that a new node called '/mynode' should be owned by the user > 'zkclient' and have full permissions on this. > 3. Admin does: zk> create /mynode content sasl:zkcli...@foofers.org:cdrwa > 4. User 'zkclient' logins to kerberos using the command line utility 'kinit'. > 5. User connects to zookeeper server using a Kerberos-enabled version of > zkClient (ZookeeperMain). > 6. Behind the scenes, the client and server exchange authentication > information. User is now authenticated as 'zkclient'. > 7. User accesses /mynode with permissions 'cdrwa'. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira