----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31963/#review76341 -----------------------------------------------------------
src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java <https://reviews.apache.org/r/31963/#comment123882> Since the connection establishment failed, can we close the connection before return the call. cnxn#close(); src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java <https://reviews.apache.org/r/31963/#comment123889> I could see handleAuthentication(cnxn, null) can return KeeperException.Code.AUTHFAILED. In that case, we should have a check and if it is successful then continue with the logic otherwise cnxn#close(). isn't it? if(KeeperException.Code.OK == authProvider.handleAuthentication(cnxn, null)){ // success }else{ // failure } src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java <https://reviews.apache.org/r/31963/#comment123883> Since the connection establishment failed, can we close the connection before return the call. cnxn#close(); src/java/main/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java <https://reviews.apache.org/r/31963/#comment123894> Please make trustManager and keyManager var as 'final' src/java/main/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java <https://reviews.apache.org/r/31963/#comment123900> One general doubt about the exception handling in the constructor: Should we continue with the server startup if there is an exception while instantiating the X509AuthenticationProvider ? Down the code I could see #handleAuthentication() is checking trustManager == null and failing all the clients KeeperException.Code.AUTHFAILED. Since no clients are allowed to establish a connection, it would good we fail the server startup rather than rejecting the client connections. What do you say? src/java/main/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java <https://reviews.apache.org/r/31963/#comment123887> Can we include cnxn#sessionId() also in the log message, this would be helpful when debugging issues. src/java/main/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java <https://reviews.apache.org/r/31963/#comment123896> Should we log an error message? src/java/main/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java <https://reviews.apache.org/r/31963/#comment123901> I couldn't see anyone is calling this. Do we need this method? src/java/main/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java <https://reviews.apache.org/r/31963/#comment123902> I couldn't see anyone is calling this. Do we need this method? src/java/test/org/apache/zookeeper/server/TestServerCnxn.java <https://reviews.apache.org/r/31963/#comment123903> Please add Apache License header. src/java/test/org/apache/zookeeper/server/TestServerCnxn.java <https://reviews.apache.org/r/31963/#comment123906> As this is for mocking the actual impl, ca we rename this to MockServerCnxn.java ? src/java/test/org/apache/zookeeper/test/X509AuthTest.java <https://reviews.apache.org/r/31963/#comment123904> Please add Apache License header. src/java/test/org/apache/zookeeper/test/X509AuthTest.java <https://reviews.apache.org/r/31963/#comment123905> Great! Good unit testcases. Can we add one end to end test case together with the ZK-2125 patch. So that will be able to see the behaviors of authProvider like, 1) when a good client(trusted) connects to the server. 2) when a bad client(untrusted) connects to the server. 3) when start a server with invalid ssl configs - Rakesh R On March 12, 2015, 12:01 a.m., Ian Dimayuga wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31963/ > ----------------------------------------------------------- > > (Updated March 12, 2015, 12:01 a.m.) > > > Review request for zookeeper, fpj, Hongchao Deng, and Rakesh R. > > > Repository: zookeeper-git > > > Description > ------- > > Patch in X509AuthenticationProvider on top of ZOOKEEPER-2125 > > > Diffs > ----- > > src/java/main/org/apache/zookeeper/common/X509Util.java PRE-CREATION > src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java > e02753f4fb926a8cc6c7a7c10af42f949c1e210c > src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java > b4bdc82f8b52f736c6ee3d67bb793a3616c1b436 > src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java > 09a794844978456fc3580adc22b6064e2a12cf77 > src/java/main/org/apache/zookeeper/server/ServerCnxn.java > a47d85662970cc0c219a46b226737a8689f8fe96 > src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java > 406015f84a51e6afcfe704b881f8494bdd687a25 > > src/java/main/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/TestServerCnxn.java PRE-CREATION > src/java/test/org/apache/zookeeper/test/X509AuthTest.java PRE-CREATION > > Diff: https://reviews.apache.org/r/31963/diff/ > > > Testing > ------- > > > Thanks, > > Ian Dimayuga > >
