ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1185

Send AuthFailed event to client if SASL authentication fails

    Details

    • Release Note:
      Hide
      This patch fixes SaslAuthFailTest.testBadSaslAuthNotifiesWatch() to test for the AuthFailed event : previously, the test was incorrectly not testing for this event.

      It also removes the testBadSaslAuthNotifiesWatch() method from the SaslAuthTest class : this method belongs in SaslAuthFailTest, not SaslAuthTest. The former tests unsuccessful SASL authentication; the latter, successful SASL authentication.
      Show
      This patch fixes SaslAuthFailTest.testBadSaslAuthNotifiesWatch() to test for the AuthFailed event : previously, the test was incorrectly not testing for this event. It also removes the testBadSaslAuthNotifiesWatch() method from the SaslAuthTest class : this method belongs in SaslAuthFailTest, not SaslAuthTest. The former tests unsuccessful SASL authentication; the latter, successful SASL authentication.

      Description

      There are 3 places where ClientCnxn should queue a AuthFailed event if client fails to authenticate. Without sending this event, clients may be stuck watching for a SaslAuthenticated event that will never come (since the client failed to authenticate).

      1. ZOOKEEPER-1185.patch
        5 kB
        Eugene Koontz
      2. ZOOKEEPER-1185.patch
        5 kB
        Mahadev konar

        Issue Links

          Activity

          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12494547/ZOOKEEPER-1185.patch
          against trunk revision 1170886.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/543//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/543//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/543//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12494547/ZOOKEEPER-1185.patch against trunk revision 1170886. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/543//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/543//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/543//console This message is automatically generated.
          Hide
          Thomas Koch added a comment -

          Could you please upload the patch to https://reviews.apache.org/r/new/ for review?

          Show
          Thomas Koch added a comment - Could you please upload the patch to https://reviews.apache.org/r/new/ for review?
          Hide
          Eugene Koontz added a comment -

          Hi Thomas,
          Thanks for your interest; please see https://reviews.apache.org/r/1959/

          -Eugene

          Show
          Eugene Koontz added a comment - Hi Thomas, Thanks for your interest; please see https://reviews.apache.org/r/1959/ -Eugene
          Hide
          Eugene Koontz added a comment -

          Hi Patrick,

          As with ZOOKEEPER-1181, I would like to lobby for this to be in 3.4.0. This fix addresses a significant functional bug in SASL authentication.

          -Eugene

          Show
          Eugene Koontz added a comment - Hi Patrick, As with ZOOKEEPER-1181 , I would like to lobby for this to be in 3.4.0. This fix addresses a significant functional bug in SASL authentication. -Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1959/
          -----------------------------------------------------------

          (Updated 2011-09-22 18:33:42.805934)

          Review request for zookeeper.

          Changes
          -------

          add link to JIRA.

          Summary
          -------

          There are 3 places where ClientCnxn should queue a AuthFailed event if client fails to authenticate. Without sending this event, clients may be stuck watching for a SaslAuthenticated event that will never come (since the client failed to authenticate).

          This patch fixes SaslAuthFailTest.testBadSaslAuthNotifiesWatch() to test for the AuthFailed event : previously, the test was incorrectly not testing for this event.

          It also removes the testBadSaslAuthNotifiesWatch() method from the SaslAuthTest class : this method belongs in SaslAuthFailTest, not SaslAuthTest. The former tests unsuccessful SASL authentication; the latter, successful SASL authentication.

          This addresses bug ZOOKEEPER-1185.
          https://issues.apache.org/jira/browse/ZOOKEEPER-1185

          Diffs


          src/java/main/org/apache/zookeeper/ClientCnxn.java db15348
          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 43382c8
          src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java 8de7c2a
          src/java/test/org/apache/zookeeper/test/SaslAuthTest.java fd20346

          Diff: https://reviews.apache.org/r/1959/diff

          Testing
          -------

          All unit tests pass. Also tested with an HBase cluster with an hbase shell running as an unauthenticated Zookeeper client. As expected, hbase shell could not access cluster, but, as expected, did not hang.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1959/ ----------------------------------------------------------- (Updated 2011-09-22 18:33:42.805934) Review request for zookeeper. Changes ------- add link to JIRA. Summary ------- There are 3 places where ClientCnxn should queue a AuthFailed event if client fails to authenticate. Without sending this event, clients may be stuck watching for a SaslAuthenticated event that will never come (since the client failed to authenticate). This patch fixes SaslAuthFailTest.testBadSaslAuthNotifiesWatch() to test for the AuthFailed event : previously, the test was incorrectly not testing for this event. It also removes the testBadSaslAuthNotifiesWatch() method from the SaslAuthTest class : this method belongs in SaslAuthFailTest, not SaslAuthTest. The former tests unsuccessful SASL authentication; the latter, successful SASL authentication. This addresses bug ZOOKEEPER-1185 . https://issues.apache.org/jira/browse/ZOOKEEPER-1185 Diffs src/java/main/org/apache/zookeeper/ClientCnxn.java db15348 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 43382c8 src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java 8de7c2a src/java/test/org/apache/zookeeper/test/SaslAuthTest.java fd20346 Diff: https://reviews.apache.org/r/1959/diff Testing ------- All unit tests pass. Also tested with an HBase cluster with an hbase shell running as an unauthenticated Zookeeper client. As expected, hbase shell could not access cluster, but, as expected, did not hang. Thanks, Eugene
          Hide
          Mahadev konar added a comment -

          Eugene,
          Just a minor nit, commented on the review board. Other than that it looks good. Ill go ahead and commit it to 3.4 and 3.5 as soon as its updated.

          thanks!

          Show
          Mahadev konar added a comment - Eugene, Just a minor nit, commented on the review board. Other than that it looks good. Ill go ahead and commit it to 3.4 and 3.5 as soon as its updated. thanks!
          Hide
          Eugene Koontz added a comment -

          Hi Mahadev, I didn't see your comment on https://reviews.apache.org/r/1959/ - did you not publish it perhaps?
          -Eugene

          Show
          Eugene Koontz added a comment - Hi Mahadev, I didn't see your comment on https://reviews.apache.org/r/1959/ - did you not publish it perhaps? -Eugene
          Hide
          Mahadev konar added a comment -

          patch that fixes the following statement in ClientCnxn.java from:

          if ((p.ctx == null) || (clientCnxn.zooKeeperSaslClient == null) ||
                                        (clientCnxn.zooKeeperSaslClient.getSaslState() == ZooKeeperSaslClient.SaslState.FAILED))
          

          to

          if ((clientCnxn == null) || (clientCnxn.zooKeeperSaslClient == null) ||
                                        (clientCnxn.zooKeeperSaslClient.getSaslState() == ZooKeeperSaslClient.SaslState.FAILED))
          
          Show
          Mahadev konar added a comment - patch that fixes the following statement in ClientCnxn.java from: if ((p.ctx == null ) || (clientCnxn.zooKeeperSaslClient == null ) || (clientCnxn.zooKeeperSaslClient.getSaslState() == ZooKeeperSaslClient.SaslState.FAILED)) to if ((clientCnxn == null ) || (clientCnxn.zooKeeperSaslClient == null ) || (clientCnxn.zooKeeperSaslClient.getSaslState() == ZooKeeperSaslClient.SaslState.FAILED))
          Hide
          Mahadev konar added a comment -

          Just committed this. Thanks Eugene. I ran tests manually to make sure my changes were fine.

          Show
          Mahadev konar added a comment - Just committed this. Thanks Eugene. I ran tests manually to make sure my changes were fine.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12496613/ZOOKEEPER-1185.patch
          against trunk revision 1176144.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/590//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/590//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/590//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12496613/ZOOKEEPER-1185.patch against trunk revision 1176144. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/590//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/590//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/590//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          Excellent, thank you Mahadev.

          Show
          Eugene Koontz added a comment - Excellent, thank you Mahadev.
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #1316 (See https://builds.apache.org/job/ZooKeeper-trunk/1316/)
          ZOOKEEPER-1185. Send AuthFailed event to client if SASL authentication fails. (Eugene Kuntz via mahadev)

          mahadev : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1176159
          Files :

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthTest.java
          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #1316 (See https://builds.apache.org/job/ZooKeeper-trunk/1316/ ) ZOOKEEPER-1185 . Send AuthFailed event to client if SASL authentication fails. (Eugene Kuntz via mahadev) mahadev : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1176159 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthTest.java

            People

            • Assignee:
              Eugene Koontz
              Reporter:
              Eugene Koontz
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development