> On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/Learner.java, line 247 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388773#file1388773line247> > > > > 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.
I've made the interface QuorumAuthClient more generic and throws IOException. Please see latest patch. Thanks! > On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java, > > line 250 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388775#file1388775line250> > > > > Should we use LOG.error here? Done > On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java, > > line 334 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388775#file1388775line334> > > > > Should we use LOG.error here? Done > On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line > > 173 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388774#file1388774line173> > > > > 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. Done > On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 375 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388776#file1388776line375> > > > > Nit: Trailing whitespaces could be removed. Missing full stop. Done > On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 1420 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388776#file1388776line1420> > > > > Not sure if we really need this log, since the value of > > enableClientAuth is also captured in the following Log.info. > > Rakesh R wrote: > Yes, I agree. Here I have added WARN log message to highlight the > insecure client quorum peer communication channel. I will remove this if this > is a duplicate info, should I remove? Done > On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 1425 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388776#file1388776line1425> > > > > 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. Done > On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuth.java, line > > 49 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388781#file1388781line49> > > > > I am not sure if I follow this comment - is this comment completed? Done > On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuth.java, line > > 70 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388781#file1388781line70> > > > > 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. Done > On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthClient.java, > > line 34 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388782#file1388782line34> > > > > 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. > > Rakesh R wrote: > QuorumAuthServer and QuorumAuthClient are interfaces. I have defined the > interfaces and written javadoc in a generic way. I think, I could have added > javadoc for the implementation classes(SaslQuorumAuthServer, > NullQuorumAuthServer, SaslQuorumAuthClient, NullQuorumAuthClient) detailing > the specific cases. I will add javadocs for the implementations. Whats your > opinion? > > Michael Han wrote: > Yeah that should work. I think my main concern was that the interface doc > should mention that a throw of SaslException also means authentication > failure, because when I was reading the code it was not clear to me what > should I rely on to decide authentication sucess or fail, until I read the > implementation. Modified the interface "public void authenticate(Socket sock) throws IOException;". Please take a look at it. thanks! > On May 24, 2016, 12:03 a.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthServer.java, > > line 39 > > <https://reviews.apache.org/r/47354/diff/3/?file=1388783#file1388783line39> > > > > 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. Modified the interface "public void authenticate(Socket sock, DataInputStream din)". Please take a look at it. thanks! - Rakesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47354/#review134426 ----------------------------------------------------------- On June 5, 2016, 7:56 a.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47354/ > ----------------------------------------------------------- > > (Updated June 5, 2016, 7:56 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 aaa220c > 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 > >
