----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47354/#review134426 -----------------------------------------------------------
ivy.xml (line 94) <https://reviews.apache.org/r/47354/#comment199269> Nit: Trailing whitespaces could be removed. ivy.xml (line 115) <https://reviews.apache.org/r/47354/#comment199270> Nit: Trailing whitespaces could be removed. src/java/main/org/apache/zookeeper/server/quorum/Learner.java (line 245) <https://reviews.apache.org/r/47354/#comment199248> This will throw SaslException when authentication failed. As a result, we should probably update the exception description of connectToLeader method: @throws SaslException - if Sasl authentication failed. src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java (line 173) <https://reviews.apache.org/r/47354/#comment199240> Should we use LOG.error instead of LOG.warn here? The presence of a SaslException indicates an authentication failure, and in existing ZK codebase, such auth failures were logged as errors, instead of warnings. src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java (line 244) <https://reviews.apache.org/r/47354/#comment199258> Should we use LOG.error here? src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java (line 326) <https://reviews.apache.org/r/47354/#comment199260> Should we use LOG.error here? src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java (line 375) <https://reviews.apache.org/r/47354/#comment199265> Nit: Trailing whitespaces could be removed. Missing full stop. src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java (line 1418) <https://reviews.apache.org/r/47354/#comment199296> Not sure if we really need this log, since the value of enableClientAuth is also captured in the following Log.info. src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java (line 1423) <https://reviews.apache.org/r/47354/#comment199297> Maybe move this line at the start of the method, to be consistent with the rest of setQuorum methods where logs are after assignment statement. src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuth.java (line 32) <https://reviews.apache.org/r/47354/#comment199309> I am tempted to rename this variable to QUORUM_SERVER_SASL_REQUIRED so it is consistent just like other configuration variables where the value of the variable and its name literally match. There are also other places in code where we could replace 'auth' with 'sasl', for example: quorumServerAuthRequired -> quorumServerSaslRequired Not sure what others think about this. src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuth.java (line 49) <https://reviews.apache.org/r/47354/#comment199311> I am not sure if I follow this comment - is this comment completed? src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuth.java (line 70) <https://reviews.apache.org/r/47354/#comment199312> Should we return ERROR here for unknown status? The follow up question is under what use cases would we get unknown status in a QuorumAuthPacket - if there is never such case then probably assert(false) is better here. src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthClient.java (line 34) <https://reviews.apache.org/r/47354/#comment199236> Similar to the comment on QuorumAuthServer::authenticate interface, we could provide some clarifications on the return value and the exception throwed for a user of this interface. Also the QuorumAuthClient::authenticate will never return false under current implementation. src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthServer.java (line 39) <https://reviews.apache.org/r/47354/#comment199226> The current implementation of this method in SaslQuorumAuthServer will never return false. Also, there is one case that this will return true even if authentication fail: that is, when sasl is not required. So it appears to me that first, the semantic of 'authentication success' is not very clearly defined, and a clarification might be worth it; and secondly, it might be better to clarify what the caller should use to decide an authentication is a success or not. Since NullQuorumAuthServer will return false, a caller of the API has to check both the return code and catch the SaslException to decide an authentication is failed. So I am thinking updating the documentation with something like: @return true in these cases: 1. after successfully authenticating the quorum client when sasl is required; or 2. when sasl is not required. Will return false if authentication failed. @throws SaslException When authentication failed. Caller should catch this exception which declares failing of an authentication. Alternatively, we could enforce that the interface authenticate will return true in all cases when authentication succeeds, and never return false. A failed authentication is materialized in the format of a SaslException, rather than a return code. src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthClient.java (line 101) <https://reviews.apache.org/r/47354/#comment199313> Nit: missing full stop, comment start with upper case letter. Please let me know if we care about such stylish issue or not - if not then I will stop picking :) src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthClient.java (line 140) <https://reviews.apache.org/r/47354/#comment199314> Nit: missing full stop, comment start with upper case letter. src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthServer.java (line 84) <https://reviews.apache.org/r/47354/#comment199315> Nit: missing full stop, comment start with upper case letter. src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthServer.java (line 111) <https://reviews.apache.org/r/47354/#comment199318> Nit: missing full stop. src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthServer.java (line 120) <https://reviews.apache.org/r/47354/#comment199317> Nit: missing full stop, comment start with upper case letter. src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthServer.java (line 137) <https://reviews.apache.org/r/47354/#comment199319> Nit: missing full stop, comment start with upper case letter. - Michael Han On May 20, 2016, 3:10 a.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47354/ > ----------------------------------------------------------- > > (Updated May 20, 2016, 3:10 a.m.) > > > Review request for zookeeper, fpj, Ivan Kelly, Patrick Hunt, and Raul > Gutierrez Segales. > > > Bugs: ZOOKEEPER-1045 > https://issues.apache.org/jira/browse/ZOOKEEPER-1045 > > > Repository: zookeeper-git > > > Description > ------- > > Quorum mutual authentication using SASL mechanism - Digest/Kerberos > > > Diffs > ----- > > build.xml ab254b2 > ivy.xml 95b0e5a > src/java/main/org/apache/zookeeper/Login.java a214c9c > src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 21ef0fa > src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java 71870ce > > src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java > 2fbd6ed > src/java/main/org/apache/zookeeper/server/quorum/Leader.java 40c6748 > src/java/main/org/apache/zookeeper/server/quorum/Learner.java c73a8ee > src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java > 8a748c7 > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java > 20e5f16 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 2f0f21b > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > 8ae820d > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java > e9c8007 > > src/java/main/org/apache/zookeeper/server/quorum/auth/NullQuorumAuthClient.java > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/NullQuorumAuthServer.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuth.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthClient.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthServer.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/README.md > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthClient.java > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthServer.java > PRE-CREATION > src/java/main/org/apache/zookeeper/util/SecurityUtils.java PRE-CREATION > src/java/test/data/kerberos/minikdc-krb5.conf PRE-CREATION > src/java/test/data/kerberos/minikdc.ldiff PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/CnxManagerTest.java > 831d3ed > > src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java > c1259d1 > src/java/test/org/apache/zookeeper/server/quorum/FLECompatibilityTest.java > 72e4fc9 > src/java/test/org/apache/zookeeper/server/quorum/FLEDontCareTest.java > a4c0cb0 > src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java > 39a53ca > src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 2ae57ce > src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java > ef552db > src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java ab8ce42 > > src/java/test/org/apache/zookeeper/server/quorum/auth/KerberosSecurityTestcase.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/KerberosTestUtils.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/auth/MiniKdc.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/auth/MiniKdcTest.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumAuthTestBase.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumAuthUpgradeTest.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumDigestAuthTest.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumKerberosAuthTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 8088505 > src/zookeeper.jute 6521e54 > > Diff: https://reviews.apache.org/r/47354/diff/ > > > Testing > ------- > > Added unit test cases to verify the changes. > > > Thanks, > > Rakesh R > >
