ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-938

Support Kerberos authentication of clients.

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.4.0
    • Component/s: java client, server
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      ZOOKEEPER-938 : support Kerberos authentication via SASL.

      Description

      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:zkclient@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'.

      1. jaas.conf
        0.3 kB
        Eugene Koontz
      2. NIOServerCnxn.patch
        9 kB
        Eugene Koontz
      3. sasl.patch
        42 kB
        Eugene Koontz
      4. ZOOKEEPER-938.patch
        113 kB
        Eugene Koontz
      5. ZOOKEEPER-938.patch
        113 kB
        Eugene Koontz
      6. ZOOKEEPER-938.patch
        113 kB
        Eugene Koontz
      7. ZOOKEEPER-938.patch
        82 kB
        Eugene Koontz
      8. ZOOKEEPER-938.patch
        81 kB
        Eugene Koontz
      9. ZOOKEEPER-938.patch
        81 kB
        Eugene Koontz
      10. ZOOKEEPER-938.patch
        81 kB
        Eugene Koontz
      11. ZOOKEEPER-938.patch
        81 kB
        Eugene Koontz
      12. ZOOKEEPER-938.patch
        82 kB
        Eugene Koontz
      13. ZOOKEEPER-938.patch
        83 kB
        Eugene Koontz
      14. ZOOKEEPER-938.patch
        103 kB
        Eugene Koontz
      15. ZOOKEEPER-938.patch
        105 kB
        Eugene Koontz
      16. ZOOKEEPER-938.patch
        96 kB
        Eugene Koontz
      17. ZOOKEEPER-938.patch
        95 kB
        Eugene Koontz

        Issue Links

          Activity

          Hide
          Eugene Koontz added a comment -

          ZOOKEEPER-896 relates to Kerberos support for the C-client. This bug relates to Java Server and Java client support. The fix for ZOOKEEPER-938 to servers should be compatible with ZOOKEEPER-896's Kerberos-enabled C-client.

          Show
          Eugene Koontz added a comment - ZOOKEEPER-896 relates to Kerberos support for the C-client. This bug relates to Java Server and Java client support. The fix for ZOOKEEPER-938 to servers should be compatible with ZOOKEEPER-896 's Kerberos-enabled C-client.
          Hide
          Eugene Koontz added a comment -

          Do authentication on server side. Does not include java client-side. Also does not have callback support for renewal of credentials (see discussion in ZOOKEEPER-896 about callbacks and credential renewal).

          Show
          Eugene Koontz added a comment - Do authentication on server side. Does not include java client-side. Also does not have callback support for renewal of credentials (see discussion in ZOOKEEPER-896 about callbacks and credential renewal).
          Hide
          Eugene Koontz added a comment -

          Use documentation provided at https://issues.apache.org/jira/browse/ZOOKEEPER-329 as a reference to implement Kerberos authentication within the Zookeeper authentication plugin framework.

          Show
          Eugene Koontz added a comment - Use documentation provided at https://issues.apache.org/jira/browse/ZOOKEEPER-329 as a reference to implement Kerberos authentication within the Zookeeper authentication plugin framework.
          Show
          Eugene Koontz added a comment - Zookeeper client Kerberos authentication: https://github.com/ekoontz/zookeeper/blob/kerberos/src/java/main/org/apache/zookeeper/ClientCnxn.java#L1390 Zookeeper server Kerberos authentication: https://github.com/ekoontz/zookeeper/blob/kerberos/src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java#L112
          Hide
          Eugene Koontz added a comment -

          I have a patch, which I'm attaching, that does SASL authentication between the Zookeeper server and client. It still requires at least 2 things:

          -integrate with zoo.cfg rather than my hard-wired test development configuration settings.
          -get other mechanisms working other than GSSAPI : currently this patch only works for GSSAPI; should work for other mechs such as PLAIN and DIGEST-MD5.

          Show
          Eugene Koontz added a comment - I have a patch, which I'm attaching, that does SASL authentication between the Zookeeper server and client. It still requires at least 2 things: -integrate with zoo.cfg rather than my hard-wired test development configuration settings. -get other mechanisms working other than GSSAPI : currently this patch only works for GSSAPI; should work for other mechs such as PLAIN and DIGEST-MD5.
          Hide
          Eugene Koontz added a comment -

          Not quite yet ready for "submit patch" (see comment posted right before this attachment was done)

          Show
          Eugene Koontz added a comment - Not quite yet ready for "submit patch" (see comment posted right before this attachment was done)
          Hide
          Eugene Koontz added a comment -

          See session transcript of a SASLized zkCli.sh ZK client connecting to a SASL-ized ZK server:

          https://gist.github.com/773429

          Show
          Eugene Koontz added a comment - See session transcript of a SASLized zkCli.sh ZK client connecting to a SASL-ized ZK server: https://gist.github.com/773429
          Hide
          Eugene Koontz added a comment -

          to enable SASL authentication, zoo.cfg must have:

          authProvider.1=org.apache.zookeeper.server.auth.SASLAuthenticationProvider

          Show
          Eugene Koontz added a comment - to enable SASL authentication, zoo.cfg must have: authProvider.1=org.apache.zookeeper.server.auth.SASLAuthenticationProvider
          Hide
          Mahadev konar added a comment -

          wow!!! This is just great! Nice work Eugene. Ill take a look at the patch. Ill be reviewing the other kerberos patches tonight. Hopefully they should be in soon!.

          Show
          Mahadev konar added a comment - wow!!! This is just great! Nice work Eugene. Ill take a look at the patch. Ill be reviewing the other kerberos patches tonight. Hopefully they should be in soon!.
          Hide
          Eugene Koontz added a comment -

          sample jaas.conf file used by both server and client (separate "Client" and "Server" sections in a single file). Save this in the top-level zookeeper directory (TODO: should be in conf/ directory)

          Show
          Eugene Koontz added a comment - sample jaas.conf file used by both server and client (separate "Client" and "Server" sections in a single file). Save this in the top-level zookeeper directory (TODO: should be in conf/ directory)
          Hide
          Eugene Koontz added a comment -

          As Botond mentioned in ZOOKEEPER-896, Zookeeper clients that use Kerberos must be able to handle mutual authentication with more than one Zookeeper server, and also must cope with ticket expiry. The sasl.patch of 2011-01-10 03:57 PM does not handle either of these circumstances. I'll be studying Botond's C client patch to ZOOKEEPER-896 to learn how it might apply to the Java client.

          Show
          Eugene Koontz added a comment - As Botond mentioned in ZOOKEEPER-896 , Zookeeper clients that use Kerberos must be able to handle mutual authentication with more than one Zookeeper server, and also must cope with ticket expiry. The sasl.patch of 2011-01-10 03:57 PM does not handle either of these circumstances. I'll be studying Botond's C client patch to ZOOKEEPER-896 to learn how it might apply to the Java client.
          Hide
          Eugene Koontz added a comment -

          Still don't have a complete patch worthy of review yet, but please see instructions for getting started with Kerberized Zookeeper from a blank Amazon EC2 Linux:

          https://github.com/ekoontz/zookeeper/wiki

          Show
          Eugene Koontz added a comment - Still don't have a complete patch worthy of review yet, but please see instructions for getting started with Kerberized Zookeeper from a blank Amazon EC2 Linux: https://github.com/ekoontz/zookeeper/wiki
          Hide
          Eugene Koontz added a comment -

          Needs some DIGEST-MD5 SASL unit tests still.

          Show
          Eugene Koontz added a comment - Needs some DIGEST-MD5 SASL unit tests still.
          Hide
          Eugene Koontz added a comment -

          Passes all junit.run tests; still needs units tests for DIGEST-MD5 with SASL.

          Show
          Eugene Koontz added a comment - Passes all junit.run tests; still needs units tests for DIGEST-MD5 with SASL.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12470334/ZOOKEEPER-938.patch
          against trunk revision 1062244.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/119//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/12470334/ZOOKEEPER-938.patch against trunk revision 1062244. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/119//console This message is automatically generated.
          Hide
          Patrick Hunt added a comment -

          Hi Eugene, you have to create the patch using the --no-prefix option in git in order for hudson to run properly. (it only knows svn)

          Show
          Patrick Hunt added a comment - Hi Eugene, you have to create the patch using the --no-prefix option in git in order for hudson to run properly. (it only knows svn)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12470648/ZOOKEEPER-938.patch
          against trunk revision 1068067.

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

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

          -1 javadoc. The javadoc tool appears to have generated 2 warning messages.

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

          -1 findbugs. The patch appears to introduce 10 new Findbugs (version 1.3.9) warnings.

          -1 release audit. The applied patch generated 27 release audit warnings (more than the trunk's current 24 warnings).

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

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

          Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/126//testReport/
          Release audit warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/126//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/126//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/126//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/12470648/ZOOKEEPER-938.patch against trunk revision 1068067. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 2 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 10 new Findbugs (version 1.3.9) warnings. -1 release audit. The applied patch generated 27 release audit warnings (more than the trunk's current 24 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/126//testReport/ Release audit warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/126//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/126//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/126//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          submitting new patch

          Show
          Eugene Koontz added a comment - submitting new patch
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12470866/ZOOKEEPER-938.patch
          against trunk revision 1069169.

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

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

          -1 javadoc. The javadoc tool appears to have generated 2 warning messages.

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

          -1 findbugs. The patch appears to introduce 2 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/127//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/127//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/127//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/12470866/ZOOKEEPER-938.patch against trunk revision 1069169. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 2 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 2 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/127//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/127//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/127//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          Sorry for the resubmission; trying to trigger a new Hudson build.

          Show
          Eugene Koontz added a comment - Sorry for the resubmission; trying to trigger a new Hudson build.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 11 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/129//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/129//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/129//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/12470889/ZOOKEEPER-938.patch against trunk revision 1069169. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/129//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/129//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/129//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12470890/ZOOKEEPER-938.patch
          against trunk revision 1069169.

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

          +1 tests included. The patch appears to include 11 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 failed core unit tests.

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

          Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/130//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/130//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/130//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/12470890/ZOOKEEPER-938.patch against trunk revision 1069169. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/130//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/130//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/130//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          this version of the patch adds support for:

          requireClientAuthScheme=(scheme)

          in zoo.cfg, which will disconnect clients if they do not first authenticate with the given scheme. For example,

          requireClientAuthScheme=sasl

          will disconnect clients if they do not first authenticate using SASL.

          Show
          Eugene Koontz added a comment - this version of the patch adds support for: requireClientAuthScheme=(scheme) in zoo.cfg, which will disconnect clients if they do not first authenticate with the given scheme. For example, requireClientAuthScheme=sasl will disconnect clients if they do not first authenticate using SASL.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 11 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/132//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/132//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/132//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/12471140/ZOOKEEPER-938.patch against trunk revision 1069169. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/132//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/132//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/132//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12471140/ZOOKEEPER-938.patch
          against trunk revision 1072085.

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

          +1 tests included. The patch appears to include 11 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 failed core unit tests.

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

          Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/144//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/144//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/144//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/12471140/ZOOKEEPER-938.patch against trunk revision 1072085. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/144//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/144//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/144//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12471140/ZOOKEEPER-938.patch
          against trunk revision 1072085.

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

          +1 tests included. The patch appears to include 11 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 failed core unit tests.

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

          Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/163//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/163//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/163//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/12471140/ZOOKEEPER-938.patch against trunk revision 1072085. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/163//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/163//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/163//console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          Eugene,

          Sorry I have been late in reviewing this, but I think this should definitely go into 3.4 release.

          One thing though, how does the zkclient authenticate via the client API?

          The user does kinit and then? How do we handle client credential expiry?

          Can you please layout a detailed design on this? I would really like this in 3.4 but am a novice on kerberos and would be lost without a design.

          Show
          Mahadev konar added a comment - Eugene, Sorry I have been late in reviewing this, but I think this should definitely go into 3.4 release. One thing though, how does the zkclient authenticate via the client API? The user does kinit and then? How do we handle client credential expiry? Can you please layout a detailed design on this? I would really like this in 3.4 but am a novice on kerberos and would be lost without a design.
          Hide
          Eugene Koontz added a comment -

          This bug describes the problem that's causing the "-1 overall" automated comments on this bug.

          Show
          Eugene Koontz added a comment - This bug describes the problem that's causing the "-1 overall" automated comments on this bug.
          Hide
          Eugene Koontz added a comment -
          Show
          Eugene Koontz added a comment - Sorry, "this bug" means https://issues.apache.org/jira/browse/ZOOKEEPER-1004 .
          Hide
          Eugene Koontz added a comment -

          Simple testing setup for Kerberized Zookeeper on Amazon EC-2 (or CentOS/Redhat):

          https://github.com/ekoontz/zookeeper/wiki

          Show
          Eugene Koontz added a comment - Simple testing setup for Kerberized Zookeeper on Amazon EC-2 (or CentOS/Redhat): https://github.com/ekoontz/zookeeper/wiki
          Hide
          Eugene Koontz added a comment -

          Hi Mahadev, Thanks for your questions. Also I was glad to talk to you and tproa in #zookeeper about this.

          -How zkclient authenticates

          The zookeeper client authenticates by setting the property: java.security.auth.login.config=$FILE

          where $FILE is a JAAS (Java Authentication and Authorization Service) configuration file. Please see my github wiki link (previous comment)for an example of such a configuration file.

          Internally, this property is used by the Zookeeper class to authenticate with kerberos.

          As far as client credential expiry, I believe that client expiry is the big remaining feature remaining to be done. The client would need to renew the credential in a separate thread that wakes up once a day (or some time period), just as the Zookeeper quorum member (the ZK server) must do. As tproa said on March 1 in #zookeeper, "when you acquire credentials, look into those credentials and discover how long they are valid. make your thread wake up in that time minus some fudge factor to renew".

          (and as tproa said, it's easy to test expiry by using the "modprinc" tool).

          I am working on getting the extra thread support on both client and server working now. I also need to get a diagram up about how it works - I'm assuming that the Zookeeper wiki would be a good place?

          I'm really looking forward to getting this in 3.4!

          -Eugene

          Show
          Eugene Koontz added a comment - Hi Mahadev, Thanks for your questions. Also I was glad to talk to you and tproa in #zookeeper about this. -How zkclient authenticates The zookeeper client authenticates by setting the property: java.security.auth.login.config=$FILE where $FILE is a JAAS (Java Authentication and Authorization Service) configuration file. Please see my github wiki link (previous comment)for an example of such a configuration file. Internally, this property is used by the Zookeeper class to authenticate with kerberos. As far as client credential expiry, I believe that client expiry is the big remaining feature remaining to be done. The client would need to renew the credential in a separate thread that wakes up once a day (or some time period), just as the Zookeeper quorum member (the ZK server) must do. As tproa said on March 1 in #zookeeper, "when you acquire credentials, look into those credentials and discover how long they are valid. make your thread wake up in that time minus some fudge factor to renew". (and as tproa said, it's easy to test expiry by using the "modprinc" tool). I am working on getting the extra thread support on both client and server working now. I also need to get a diagram up about how it works - I'm assuming that the Zookeeper wiki would be a good place? I'm really looking forward to getting this in 3.4! -Eugene
          Hide
          Kan Zhang added a comment -

          @Eugene,

          Congrats on getting Kerberos to work! Mahadev asked me to take a look of your patch. We did a similar thing for Hadoop you might want to take a look. You don't have a design doc attached, at the risk of misunderstanding you, here are my initial comments.

          1. Why use SASL? We use SASL in Hadoop because we want to support both Kerberos and our own tokens. If we only need to support Kerberos, we might have chosen to use Java GSSAPI directly (which, for example, allows you to specify the full Kerberos principal name for the server, whereas in SASL the REALM part is picked up from default conf). Do you have a need for username/password auth once Kerberos mode is turned on? If your connections are long-lived, Kerberos-only auth might be enough.

          2. If you do need to support username/password in addition to Kerberos, how user passwords are stored on server?

          3. I saw you have a "OpCode.addcred". Is this the operation that allows a client to install her chosen password on the server? If so, you need to make sure this operation is done over a Kerberos authenticated and preferably encrypted connection. In Hadoop, we only allow delegation tokens to be issued to Kerberos authenticated clients.

          4.

          +        } else if (cmd.equals("addcred") && args.length == 3) {
          +            String username = args[1];
          +            String password = args[2];
          +            zk.addCredentials(username,password);
          +        }
          

          It's better not to put passwords on the command line. Either read from a file or get it from user interactively.

          5. You might want to set login configurations programmatically, which is more flexible (take a look of what we did in UserGroupInformation.java). I think you'll need to support both keytab and TGT cache login.

          6. You'll have to deal with relogin issues since your connections are long-lived. See UserGroupInformation for some example code. For Hadoop RPC, we don't refresh TGT proactively. The client simply tries to set up a connection and if the TGT is expired, the setup will fail. When the client detects the failure, it does relogin and retry connection setup.

          Show
          Kan Zhang added a comment - @Eugene, Congrats on getting Kerberos to work! Mahadev asked me to take a look of your patch. We did a similar thing for Hadoop you might want to take a look. You don't have a design doc attached, at the risk of misunderstanding you, here are my initial comments. 1. Why use SASL? We use SASL in Hadoop because we want to support both Kerberos and our own tokens. If we only need to support Kerberos, we might have chosen to use Java GSSAPI directly (which, for example, allows you to specify the full Kerberos principal name for the server, whereas in SASL the REALM part is picked up from default conf). Do you have a need for username/password auth once Kerberos mode is turned on? If your connections are long-lived, Kerberos-only auth might be enough. 2. If you do need to support username/password in addition to Kerberos, how user passwords are stored on server? 3. I saw you have a "OpCode.addcred". Is this the operation that allows a client to install her chosen password on the server? If so, you need to make sure this operation is done over a Kerberos authenticated and preferably encrypted connection. In Hadoop, we only allow delegation tokens to be issued to Kerberos authenticated clients. 4. + } else if (cmd.equals( "addcred" ) && args.length == 3) { + String username = args[1]; + String password = args[2]; + zk.addCredentials(username,password); + } It's better not to put passwords on the command line. Either read from a file or get it from user interactively. 5. You might want to set login configurations programmatically, which is more flexible (take a look of what we did in UserGroupInformation.java). I think you'll need to support both keytab and TGT cache login. 6. You'll have to deal with relogin issues since your connections are long-lived. See UserGroupInformation for some example code. For Hadoop RPC, we don't refresh TGT proactively. The client simply tries to set up a connection and if the TGT is expired, the setup will fail. When the client detects the failure, it does relogin and retry connection setup.
          Hide
          Eugene Koontz added a comment -

          Hi Kan,
          Thanks very much for your feedback. I wanted to quickly deal with issue 6 since this is the major missing piece. Thanks for the explanation about Hadoop RPC. I'm going to study the Hadoop RPC code to determine how to apply this.

          Show
          Eugene Koontz added a comment - Hi Kan, Thanks very much for your feedback. I wanted to quickly deal with issue 6 since this is the major missing piece. Thanks for the explanation about Hadoop RPC. I'm going to study the Hadoop RPC code to determine how to apply this.
          Hide
          Eugene Koontz added a comment -

          Addresses Kan Zhang's comments:

          -adds credential-refreshing LoginThread class for both Zookeeper client and server.
          -removes 'addcred' command that was only useful for development and testing of DIGEST-MD5 with SASL: unnecessary and unsafe to add passwords by command-line, especially without authentication and encryption

          Show
          Eugene Koontz added a comment - Addresses Kan Zhang's comments: -adds credential-refreshing LoginThread class for both Zookeeper client and server. -removes 'addcred' command that was only useful for development and testing of DIGEST-MD5 with SASL: unnecessary and unsafe to add passwords by command-line, especially without authentication and encryption
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12474560/ZOOKEEPER-938.patch
          against trunk revision 1082362.

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

          +1 tests included. The patch appears to include 9 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 appears to introduce 3 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/198//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/198//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/198//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/12474560/ZOOKEEPER-938.patch against trunk revision 1082362. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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 appears to introduce 3 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/198//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/198//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/198//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          adding new patch that fixes FindBugs warnings

          Show
          Eugene Koontz added a comment - adding new patch that fixes FindBugs warnings
          Hide
          Eugene Koontz added a comment -

          Fixes findbugs warnings.

          Show
          Eugene Koontz added a comment - Fixes findbugs warnings.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12474661/ZOOKEEPER-938.patch
          against trunk revision 1082362.

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

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

          -1 javadoc. The javadoc tool appears to have generated 2 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/201//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/201//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/201//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/12474661/ZOOKEEPER-938.patch against trunk revision 1082362. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 2 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/201//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/201//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/201//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          fix javadoc warning about @params.

          Show
          Eugene Koontz added a comment - fix javadoc warning about @params.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12474671/ZOOKEEPER-938.patch
          against trunk revision 1082362.

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

          +1 tests included. The patch appears to include 9 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 failed core unit tests.

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

          Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/203//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/203//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/203//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/12474671/ZOOKEEPER-938.patch against trunk revision 1082362. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/203//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/203//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/203//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          move java system property check up one level for simplicity of determining whether SASL is enabled or not.

          Show
          Eugene Koontz added a comment - move java system property check up one level for simplicity of determining whether SASL is enabled or not.
          Hide
          Eugene Koontz added a comment -

          Move java system property check up one level for simplicity of determining whether SASL is enabled or not.

          Show
          Eugene Koontz added a comment - Move java system property check up one level for simplicity of determining whether SASL is enabled or not.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 9 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/208//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/208//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/208//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/12474845/ZOOKEEPER-938.patch against trunk revision 1082362. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/208//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/208//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/208//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          Please see https://cwiki.apache.org/confluence/display/ZOOKEEPER/Zookeeper+and+SASL for a design document explaining the proposed patch for this issue.

          Show
          Eugene Koontz added a comment - Please see https://cwiki.apache.org/confluence/display/ZOOKEEPER/Zookeeper+and+SASL for a design document explaining the proposed patch for this issue.
          Hide
          Kan Zhang added a comment -

          Hi Eugene,

          Thanks for addressing some of my previous comments in your new patch. Before I comment further, I'd like to understand what use cases you plan to support. In your documentation, you explained a znode can have any number of <scheme:expression,perms> pairs for specifying the authentication scheme, principal name, and permissions for that principal. Is this the only way to specify permissions? How does it scale to thousands of users for a single node? And if I have a user that needs to access a ZooKeeper tree with thousands of nodes, do I have to add this user to every node? In addition to USER, do you have a GROUP concept for specifying permissions?

          As you may already know, Hadoop doesn't use authentication specific names (we call them long-names) for specifying permissions. Instead, we convert long-names to Hadoop internal short-names using configured rules, and use short-names for specifying permissions and permission checking. This has the benefits of specifying permissions for a user regardless of which authentication scheme the user might use to access the system. It makes authorization policy independent of authentication. It can also map different external users to the same internal user.

          Show
          Kan Zhang added a comment - Hi Eugene, Thanks for addressing some of my previous comments in your new patch. Before I comment further, I'd like to understand what use cases you plan to support. In your documentation, you explained a znode can have any number of <scheme:expression,perms> pairs for specifying the authentication scheme, principal name, and permissions for that principal. Is this the only way to specify permissions? How does it scale to thousands of users for a single node? And if I have a user that needs to access a ZooKeeper tree with thousands of nodes, do I have to add this user to every node? In addition to USER, do you have a GROUP concept for specifying permissions? As you may already know, Hadoop doesn't use authentication specific names (we call them long-names) for specifying permissions. Instead, we convert long-names to Hadoop internal short-names using configured rules, and use short-names for specifying permissions and permission checking. This has the benefits of specifying permissions for a user regardless of which authentication scheme the user might use to access the system. It makes authorization policy independent of authentication. It can also map different external users to the same internal user.
          Hide
          Eugene Koontz added a comment -

          It's intended to use ZOOKEEPER-938 as a foundation for ZOOKEEPER-1045

          Show
          Eugene Koontz added a comment - It's intended to use ZOOKEEPER-938 as a foundation for ZOOKEEPER-1045
          Hide
          Eugene Koontz added a comment -

          Thanks for your review again, Kan. Addressing your points from the beginning (sorry for the delay):

          > 1. Why use SASL? We use SASL in Hadoop because we want to support both Kerberos and our own tokens. If we only need to support Kerberos, we might have > chosen to use Java GSSAPI directly (which, for example, allows you to specify the full Kerberos principal name for the server, whereas in SASL the
          > REALM part is picked up from default conf). Do you have a need for username/password auth once Kerberos mode is turned on? If your connections are
          > long-lived, Kerberos-only auth might be enough.

          I think the powerful motivation for SASL is its flexibility - for example, Mahadev is interested in possibly using SSL with Zookeeper. We could use the SASL EXTERNAL mechanism: http://tools.ietf.org/html/rfc2222#section-7.4 (thanks to Gary Helmling for pointing this out and the link).

          With regard to specifying the full Kerberos principal name for the server, do you mean that the SASL API does not allow the client to specify the full server principal name e.g. "zookeeper/my.host.name@MYDOMAIN"?

          > 2. If you do need to support username/password in addition to Kerberos, how user passwords are stored on server?

          > 3. I saw you have a "OpCode.addcred". Is this the operation that allows a client to install her chosen password on the server? If so, you need to make > sure this operation is done over a Kerberos authenticated and preferably encrypted connection. In Hadoop, we only allow delegation tokens to be
          > issued to Kerberos authenticated clients.

          > 4.

          > + } else if (cmd.equals("addcred") && args.length == 3)

          { > + String username = args[1]; > + String password = args[2]; > + zk.addCredentials(username,password); > + }

          > It's better not to put passwords on the command line. Either read from a file or get it from user interactively.

          Thanks for catching this : I've removed all of the "addcred" stuff because of your good criticism. It was only intended for testing my own DIGEST-MD5 mechanism which should only be used for testing and development, not production. See two SASL unit tests that use DIGEST-MD5 here:

          https://github.com/ekoontz/zookeeper/blob/sasl/src/java/test/org/apache/zookeeper/test/SaslAuthTest.java
          https://github.com/ekoontz/zookeeper/blob/sasl/src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java

          However these unit tests do not require the "addcred" command and so I've removed it.

          > 5. You might want to set login configurations programmatically, which is more flexible (take a look of what we did in UserGroupInformation.java). I
          > think you'll need to support both keytab and TGT cache login.

          I find it more flexible to use external text files. Also, if we use the JAAS configuration format, it's easier to integrate with other services that already use this format - see this JIRA: https://issues.apache.org/jira/browse/HADOOP-7070.

          > 6. You'll have to deal with relogin issues since your connections are long-lived. See UserGroupInformation for some example code. For Hadoop RPC, we > don't refresh TGT proactively. The client simply tries to set up a connection and if the TGT is expired, the setup will fail. When the client detects > the failure, it does relogin and retry connection setup.

          This is now addressed with the LoginThread code; see the latest patch. I chose to do a periodic refresh rather than a fail and retry, since it seemed simpler to me. I'd be happy to see how a fail and retry replacement would look, though.

          Show
          Eugene Koontz added a comment - Thanks for your review again, Kan. Addressing your points from the beginning (sorry for the delay): > 1. Why use SASL? We use SASL in Hadoop because we want to support both Kerberos and our own tokens. If we only need to support Kerberos, we might have > chosen to use Java GSSAPI directly (which, for example, allows you to specify the full Kerberos principal name for the server, whereas in SASL the > REALM part is picked up from default conf). Do you have a need for username/password auth once Kerberos mode is turned on? If your connections are > long-lived, Kerberos-only auth might be enough. I think the powerful motivation for SASL is its flexibility - for example, Mahadev is interested in possibly using SSL with Zookeeper. We could use the SASL EXTERNAL mechanism: http://tools.ietf.org/html/rfc2222#section-7.4 (thanks to Gary Helmling for pointing this out and the link). With regard to specifying the full Kerberos principal name for the server, do you mean that the SASL API does not allow the client to specify the full server principal name e.g. "zookeeper/my.host.name@MYDOMAIN"? > 2. If you do need to support username/password in addition to Kerberos, how user passwords are stored on server? > 3. I saw you have a "OpCode.addcred". Is this the operation that allows a client to install her chosen password on the server? If so, you need to make > sure this operation is done over a Kerberos authenticated and preferably encrypted connection. In Hadoop, we only allow delegation tokens to be > issued to Kerberos authenticated clients. > 4. > + } else if (cmd.equals("addcred") && args.length == 3) { > + String username = args[1]; > + String password = args[2]; > + zk.addCredentials(username,password); > + } > It's better not to put passwords on the command line. Either read from a file or get it from user interactively. Thanks for catching this : I've removed all of the "addcred" stuff because of your good criticism. It was only intended for testing my own DIGEST-MD5 mechanism which should only be used for testing and development, not production. See two SASL unit tests that use DIGEST-MD5 here: https://github.com/ekoontz/zookeeper/blob/sasl/src/java/test/org/apache/zookeeper/test/SaslAuthTest.java https://github.com/ekoontz/zookeeper/blob/sasl/src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java However these unit tests do not require the "addcred" command and so I've removed it. > 5. You might want to set login configurations programmatically, which is more flexible (take a look of what we did in UserGroupInformation.java). I > think you'll need to support both keytab and TGT cache login. I find it more flexible to use external text files. Also, if we use the JAAS configuration format, it's easier to integrate with other services that already use this format - see this JIRA: https://issues.apache.org/jira/browse/HADOOP-7070 . > 6. You'll have to deal with relogin issues since your connections are long-lived. See UserGroupInformation for some example code. For Hadoop RPC, we > don't refresh TGT proactively. The client simply tries to set up a connection and if the TGT is expired, the setup will fail. When the client detects > the failure, it does relogin and retry connection setup. This is now addressed with the LoginThread code; see the latest patch. I chose to do a periodic refresh rather than a fail and retry, since it seemed simpler to me. I'd be happy to see how a fail and retry replacement would look, though.
          Hide
          Eugene Koontz added a comment -

          > Thanks for addressing some of my previous comments in your new patch. Before I comment further, I'd like to understand what use cases you plan to
          > support. In your documentation, you explained a znode can have any number of <scheme:expression,perms> pairs for specifying the authentication scheme,
          > principal name, and permissions for that principal. Is this the only way to specify permissions? How does it scale to thousands of users for a single
          > node? And if I have a user that needs to access a ZooKeeper tree with thousands of nodes, do I have to add this user to every node? In addition to USER, > do you have a GROUP concept for specifying permissions?

          Hi Kan, I simply made the existing permission functionality useable with SASL. Adding groups and testing scalability are outside of the scope of this bug.

          > As you may already know, Hadoop doesn't use authentication specific names (we call them long-names) for specifying permissions. Instead, we convert
          > long-names to Hadoop internal short-names using configured rules, and use short-names for specifying permissions and permission checking. This has the
          > benefits of specifying permissions for a user regardless of which authentication scheme the user might use to access the system. It makes authorization
          > policy independent of authentication. It can also map different external users to the same internal user.

          That sounds interesting but again it is outside the scope of this bug, I think.

          Show
          Eugene Koontz added a comment - > Thanks for addressing some of my previous comments in your new patch. Before I comment further, I'd like to understand what use cases you plan to > support. In your documentation, you explained a znode can have any number of <scheme:expression,perms> pairs for specifying the authentication scheme, > principal name, and permissions for that principal. Is this the only way to specify permissions? How does it scale to thousands of users for a single > node? And if I have a user that needs to access a ZooKeeper tree with thousands of nodes, do I have to add this user to every node? In addition to USER, > do you have a GROUP concept for specifying permissions? Hi Kan, I simply made the existing permission functionality useable with SASL. Adding groups and testing scalability are outside of the scope of this bug. > As you may already know, Hadoop doesn't use authentication specific names (we call them long-names) for specifying permissions. Instead, we convert > long-names to Hadoop internal short-names using configured rules, and use short-names for specifying permissions and permission checking. This has the > benefits of specifying permissions for a user regardless of which authentication scheme the user might use to access the system. It makes authorization > policy independent of authentication. It can also map different external users to the same internal user. That sounds interesting but again it is outside the scope of this bug, I think.
          Hide
          Eugene Koontz added a comment -

          Hi Kan, Please don't misunderstand me; I am interested to learn more about the Hadoop name-mapping permissions model. It sounds very flexible and hopefully could be used with Zookeeper, but that would be another JIRA I would think.

          Show
          Eugene Koontz added a comment - Hi Kan, Please don't misunderstand me; I am interested to learn more about the Hadoop name-mapping permissions model. It sounds very flexible and hopefully could be used with Zookeeper, but that would be another JIRA I would think.
          Hide
          Eugene Koontz added a comment -

          Canceling patch because it needs additional work. Hope to release a more recent patch soon.

          Show
          Eugene Koontz added a comment - Canceling patch because it needs additional work. Hope to release a more recent patch soon.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12483225/ZOOKEEPER-938.patch
          against trunk revision 1136740.

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/332//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/12483225/ZOOKEEPER-938.patch against trunk revision 1136740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 13 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/332//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12483226/ZOOKEEPER-938.patch
          against trunk revision 1136740.

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

          +1 tests included. The patch appears to include 11 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/333//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/333//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/333//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/12483226/ZOOKEEPER-938.patch against trunk revision 1136740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/333//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/333//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/333//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          I don't think that the test that fails in https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/333//testReport/ (org.apache.zookeeper.test.QuorumTest.testFollowersStartAfterLeader) is due to my patch, since the same test fails for me on apache trunk in about 1 of every 3 tries.

          Show
          Eugene Koontz added a comment - I don't think that the test that fails in https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/333//testReport/ (org.apache.zookeeper.test.QuorumTest.testFollowersStartAfterLeader) is due to my patch, since the same test fails for me on apache trunk in about 1 of every 3 tries.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 11 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/337//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/337//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/337//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/12483226/ZOOKEEPER-938.patch against trunk revision 1138100. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 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/337//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/337//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/337//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 11 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/340//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/340//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/340//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/12483336/ZOOKEEPER-938.patch against trunk revision 1138100. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 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/340//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/340//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/340//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          i think this patch is pretty close i would really like to get this into 3.4.

          Show
          Benjamin Reed added a comment - i think this patch is pretty close i would really like to get this into 3.4.
          Hide
          Eugene Koontz added a comment -

          Benjamin,
          Thanks so much for taking the time to look at this. I'm working on abstracting out the SASL-specific bits from ClientCnxn now.

          Show
          Eugene Koontz added a comment - Benjamin, Thanks so much for taking the time to look at this. I'm working on abstracting out the SASL-specific bits from ClientCnxn now.
          Hide
          Eugene Koontz added a comment -

          Create new classes: ZooKeeperSaslServer and ZooKeeperSaslClient to better encapsulate SASL-related functionality (for client and server respectively).
          Thanks to Benjamin Reed for his review at the Zookeeper Dev Meeting last week.

          Show
          Eugene Koontz added a comment - Create new classes: ZooKeeperSaslServer and ZooKeeperSaslClient to better encapsulate SASL-related functionality (for client and server respectively). Thanks to Benjamin Reed for his review at the Zookeeper Dev Meeting last week.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12485455/ZOOKEEPER-938.patch
          against trunk revision 1142377.

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/366//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/12485455/ZOOKEEPER-938.patch against trunk revision 1142377. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/366//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          --no-prefix

          Show
          Eugene Koontz added a comment - --no-prefix
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12485457/ZOOKEEPER-938.patch
          against trunk revision 1142377.

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

          +1 tests included. The patch appears to include 4 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 appears to introduce 1 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/367//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/367//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/367//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/12485457/ZOOKEEPER-938.patch against trunk revision 1142377. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 appears to introduce 1 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/367//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/367//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/367//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          Fix the findbugs warning (remove unused SendThread.cnxn).

          Show
          Eugene Koontz added a comment - Fix the findbugs warning (remove unused SendThread.cnxn).
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12485475/ZOOKEEPER-938.patch
          against trunk revision 1142377.

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

          +1 tests included. The patch appears to include 4 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/369//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/369//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/369//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/12485475/ZOOKEEPER-938.patch against trunk revision 1142377. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/369//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/369//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/369//console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          Ben, can you please review this?

          Show
          Mahadev konar added a comment - Ben, can you please review this?
          Hide
          Eugene Koontz added a comment -

          Hi Mahadev and Ben,
          I'd promised Ben to submit the patch to reviews.apache.org; let me do that today to help ease the review.
          -Eugene

          Show
          Eugene Koontz added a comment - Hi Mahadev and Ben, I'd promised Ben to submit the patch to reviews.apache.org; let me do that today to help ease the review. -Eugene
          Hide
          Eugene Koontz added a comment -

          Increase similarity between ZooKeeperSaslClient and ZooKeeperSaslServer as they relate to LoginThread.

          Show
          Eugene Koontz added a comment - Increase similarity between ZooKeeperSaslClient and ZooKeeperSaslServer as they relate to LoginThread.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12485615/ZOOKEEPER-938.patch
          against trunk revision 1143688.

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

          +1 tests included. The patch appears to include 4 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 appears to introduce 2 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/375//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/375//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/375//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/12485615/ZOOKEEPER-938.patch against trunk revision 1143688. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 appears to introduce 2 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/375//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/375//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/375//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          fix 2 findbugs warnings introduced in last patch.

          Show
          Eugene Koontz added a comment - fix 2 findbugs warnings introduced in last patch.
          Hide
          Eugene Koontz added a comment -

          fix 2 findbugs warnings introduced in last patch. (retry)

          Show
          Eugene Koontz added a comment - fix 2 findbugs warnings introduced in last patch. (retry)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12485625/ZOOKEEPER-938.patch
          against trunk revision 1143688.

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

          +1 tests included. The patch appears to include 4 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 appears to introduce 2 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/377//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/377//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/377//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/12485625/ZOOKEEPER-938.patch against trunk revision 1143688. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 appears to introduce 2 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/377//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/377//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/377//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 4 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/378//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/378//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/378//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/12485628/ZOOKEEPER-938.patch against trunk revision 1143688. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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/378//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/378//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/378//console This message is automatically generated.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for zookeeper, Benjamin Reed and Mahadev Konar.

          Summary
          -------

          Support Kerberos authentication of clients.

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

          Diffs


          src/java/main/org/apache/zookeeper/ClientCnxn.java 87477df
          src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java d0e74fa
          src/java/main/org/apache/zookeeper/LoginThread.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Watcher.java e72105c
          src/java/main/org/apache/zookeeper/ZooDefs.java f77ac20
          src/java/main/org/apache/zookeeper/ZooKeeper.java f2ab4a6
          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java b690817
          src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java bab8998
          src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java e7e1846
          src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java c6ab5dd
          src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java deb1e7a
          src/java/main/org/apache/zookeeper/server/Request.java 80d2b99
          src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6d69073
          src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 72af158
          src/java/main/org/apache/zookeeper/server/ServerConfig.java ec710cd
          src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 52d3820
          src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java c6d9c09
          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 969a482
          src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/test/SaslAuthTest.java PRE-CREATION
          src/zookeeper.jute 34eac78

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

          Testing
          -------

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1049/ ----------------------------------------------------------- Review request for zookeeper, Benjamin Reed and Mahadev Konar. Summary ------- Support Kerberos authentication of clients. This addresses bug ZOOKEEPER-938 . https://issues.apache.org/jira/browse/ZOOKEEPER-938 Diffs src/java/main/org/apache/zookeeper/ClientCnxn.java 87477df src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java d0e74fa src/java/main/org/apache/zookeeper/LoginThread.java PRE-CREATION src/java/main/org/apache/zookeeper/Watcher.java e72105c src/java/main/org/apache/zookeeper/ZooDefs.java f77ac20 src/java/main/org/apache/zookeeper/ZooKeeper.java f2ab4a6 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java PRE-CREATION src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java b690817 src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java bab8998 src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java e7e1846 src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java c6ab5dd src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java deb1e7a src/java/main/org/apache/zookeeper/server/Request.java 80d2b99 src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6d69073 src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 72af158 src/java/main/org/apache/zookeeper/server/ServerConfig.java ec710cd src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java PRE-CREATION src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 52d3820 src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java PRE-CREATION src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java PRE-CREATION src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java c6d9c09 src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 969a482 src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthTest.java PRE-CREATION src/zookeeper.jute 34eac78 Diff: https://reviews.apache.org/r/1049/diff Testing ------- Thanks, Patrick
          Hide
          Eugene Koontz added a comment -

          In server-side code, create a separate ZooKeeperSaslServer for each client connection, but use the same server-wide LoginThread for all.

          Show
          Eugene Koontz added a comment - In server-side code, create a separate ZooKeeperSaslServer for each client connection, but use the same server-wide LoginThread for all.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12485860/ZOOKEEPER-938.patch
          against trunk revision 1144087.

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

          +1 tests included. The patch appears to include 4 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/388//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/388//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/388//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/12485860/ZOOKEEPER-938.patch against trunk revision 1144087. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/388//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/388//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/388//console This message is automatically generated.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for zookeeper.

          Summary
          -------

          Support Kerberos authentication of clients.

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

          Diffs


          src/java/main/org/apache/zookeeper/ClientCnxn.java 87477df
          src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java d0e74fa
          src/java/main/org/apache/zookeeper/LoginThread.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Watcher.java e72105c
          src/java/main/org/apache/zookeeper/ZooDefs.java f77ac20
          src/java/main/org/apache/zookeeper/ZooKeeper.java f2ab4a6
          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java b690817
          src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java bab8998
          src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java e7e1846
          src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java c6ab5dd
          src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java deb1e7a
          src/java/main/org/apache/zookeeper/server/Request.java 80d2b99
          src/java/main/org/apache/zookeeper/server/SaslServerCallbackHandler.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6d69073
          src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 72af158
          src/java/main/org/apache/zookeeper/server/ServerConfig.java ec710cd
          src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 52d3820
          src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java c6d9c09
          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 969a482
          src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/test/SaslAuthTest.java PRE-CREATION
          src/zookeeper.jute 34eac78

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

          Testing
          -------

          Passes all unit tests on my local machine. Also tested as part of a 3-node quorum on 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)

          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/1059/ ----------------------------------------------------------- Review request for zookeeper. Summary ------- Support Kerberos authentication of clients. This addresses bug ZOOKEEPER-938 . https://issues.apache.org/jira/browse/ZOOKEEPER-938 Diffs src/java/main/org/apache/zookeeper/ClientCnxn.java 87477df src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java d0e74fa src/java/main/org/apache/zookeeper/LoginThread.java PRE-CREATION src/java/main/org/apache/zookeeper/Watcher.java e72105c src/java/main/org/apache/zookeeper/ZooDefs.java f77ac20 src/java/main/org/apache/zookeeper/ZooKeeper.java f2ab4a6 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java PRE-CREATION src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java b690817 src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java bab8998 src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java e7e1846 src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java c6ab5dd src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java deb1e7a src/java/main/org/apache/zookeeper/server/Request.java 80d2b99 src/java/main/org/apache/zookeeper/server/SaslServerCallbackHandler.java PRE-CREATION src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6d69073 src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 72af158 src/java/main/org/apache/zookeeper/server/ServerConfig.java ec710cd src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java PRE-CREATION src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 52d3820 src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java PRE-CREATION src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java PRE-CREATION src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java c6d9c09 src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 969a482 src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthTest.java PRE-CREATION src/zookeeper.jute 34eac78 Diff: https://reviews.apache.org/r/1059/diff Testing ------- Passes all unit tests on my local machine. Also tested as part of a 3-node quorum on 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 ) Thanks, Eugene
          Hide
          Eugene Koontz added a comment -

          Hi Patrick,
          Thanks for creating a review before this one; I'm sorry to delay mine so long. Since I've updated the patch to improve the server-side code, can you kindly cancel your review and see mine instead?
          Thanks,
          Eugene

          Show
          Eugene Koontz added a comment - Hi Patrick, Thanks for creating a review before this one; I'm sorry to delay mine so long. Since I've updated the patch to improve the server-side code, can you kindly cancel your review and see mine instead? Thanks, Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2011-07-09 19:51:29.949643)

          Review request for zookeeper.

          Summary
          -------

          Support Kerberos authentication of clients.

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

          Diffs


          src/java/main/org/apache/zookeeper/ClientCnxn.java 87477df
          src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java d0e74fa
          src/java/main/org/apache/zookeeper/LoginThread.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Watcher.java e72105c
          src/java/main/org/apache/zookeeper/ZooDefs.java f77ac20
          src/java/main/org/apache/zookeeper/ZooKeeper.java f2ab4a6
          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java b690817
          src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java bab8998
          src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java e7e1846
          src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java c6ab5dd
          src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java deb1e7a
          src/java/main/org/apache/zookeeper/server/Request.java 80d2b99
          src/java/main/org/apache/zookeeper/server/SaslServerCallbackHandler.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6d69073
          src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 72af158
          src/java/main/org/apache/zookeeper/server/ServerConfig.java ec710cd
          src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 52d3820
          src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java PRE-CREATION
          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java c6d9c09
          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 969a482
          src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/test/SaslAuthTest.java PRE-CREATION
          src/zookeeper.jute 34eac78

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

          Testing (updated)
          -------

          Passes all unit tests on my local machine.

          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)

          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/1059/ ----------------------------------------------------------- (Updated 2011-07-09 19:51:29.949643) Review request for zookeeper. Summary ------- Support Kerberos authentication of clients. This addresses bug ZOOKEEPER-938 . https://issues.apache.org/jira/browse/ZOOKEEPER-938 Diffs src/java/main/org/apache/zookeeper/ClientCnxn.java 87477df src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java d0e74fa src/java/main/org/apache/zookeeper/LoginThread.java PRE-CREATION src/java/main/org/apache/zookeeper/Watcher.java e72105c src/java/main/org/apache/zookeeper/ZooDefs.java f77ac20 src/java/main/org/apache/zookeeper/ZooKeeper.java f2ab4a6 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java PRE-CREATION src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java b690817 src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java bab8998 src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java e7e1846 src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java c6ab5dd src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java deb1e7a src/java/main/org/apache/zookeeper/server/Request.java 80d2b99 src/java/main/org/apache/zookeeper/server/SaslServerCallbackHandler.java PRE-CREATION src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6d69073 src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 72af158 src/java/main/org/apache/zookeeper/server/ServerConfig.java ec710cd src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java PRE-CREATION src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 52d3820 src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java PRE-CREATION src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java PRE-CREATION src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java c6d9c09 src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 969a482 src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthTest.java PRE-CREATION src/zookeeper.jute 34eac78 Diff: https://reviews.apache.org/r/1059/diff Testing (updated) ------- Passes all unit tests on my local machine. 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 ) Thanks, Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          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:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/1059/

          -----------------------------------------------------------

          (Updated 2011-07-09 19:51:29)

          Review request for zookeeper.

          Summary

          -------

          Support Kerberos authentication of clients.

          This addresses bug ZOOKEEPER-938.

          https://issues.apache.org/jira/browse/ZOOKEEPER-938

          Diffs

          -----

          src/java/main/org/apache/zookeeper/ClientCnxn.java 87477df

          src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java d0e74fa

          src/java/main/org/apache/zookeeper/LoginThread.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Watcher.java e72105c

          src/java/main/org/apache/zookeeper/ZooDefs.java f77ac20

          src/java/main/org/apache/zookeeper/ZooKeeper.java f2ab4a6

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java PRE-CREATION

          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java b690817

          src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java bab8998

          src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java e7e1846

          src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java c6ab5dd

          src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java deb1e7a

          src/java/main/org/apache/zookeeper/server/Request.java 80d2b99

          src/java/main/org/apache/zookeeper/server/SaslServerCallbackHandler.java PRE-CREATION

          src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6d69073

          src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 72af158

          src/java/main/org/apache/zookeeper/server/ServerConfig.java ec710cd

          src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java PRE-CREATION

          src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 52d3820

          src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java PRE-CREATION

          src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java PRE-CREATION

          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java c6d9c09

          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 969a482

          src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthTest.java PRE-CREATION

          src/zookeeper.jute 34eac78

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

          Testing

          -------

          Passes all unit tests on my local machine.

          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)

          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/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: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1059/ ----------------------------------------------------------- (Updated 2011-07-09 19:51:29) Review request for zookeeper. Summary ------- Support Kerberos authentication of clients. This addresses bug ZOOKEEPER-938 . https://issues.apache.org/jira/browse/ZOOKEEPER-938 Diffs ----- src/java/main/org/apache/zookeeper/ClientCnxn.java 87477df src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java d0e74fa src/java/main/org/apache/zookeeper/LoginThread.java PRE-CREATION src/java/main/org/apache/zookeeper/Watcher.java e72105c src/java/main/org/apache/zookeeper/ZooDefs.java f77ac20 src/java/main/org/apache/zookeeper/ZooKeeper.java f2ab4a6 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java PRE-CREATION src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java b690817 src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java bab8998 src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java e7e1846 src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java c6ab5dd src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java deb1e7a src/java/main/org/apache/zookeeper/server/Request.java 80d2b99 src/java/main/org/apache/zookeeper/server/SaslServerCallbackHandler.java PRE-CREATION src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6d69073 src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 72af158 src/java/main/org/apache/zookeeper/server/ServerConfig.java ec710cd src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java PRE-CREATION src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 52d3820 src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java PRE-CREATION src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java PRE-CREATION src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java c6d9c09 src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 969a482 src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthTest.java PRE-CREATION src/zookeeper.jute 34eac78 Diff: https://reviews.apache.org/r/1059/diff Testing ------- Passes all unit tests on my local machine. 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 ) Thanks, Eugene
          Hide
          Benjamin Reed added a comment -

          are you going to move the sasl processing out of finalrp and into or close to ServerCnxn?

          Show
          Benjamin Reed added a comment - are you going to move the sasl processing out of finalrp and into or close to ServerCnxn?
          Hide
          Eugene Koontz added a comment -

          Hi Ben, yes, I am moving SASL processing out of FinalRequestProcessor. I'm planning to post a new patch soon which will be much improved, thanks to your review.

          Show
          Eugene Koontz added a comment - Hi Ben, yes, I am moving SASL processing out of FinalRequestProcessor. I'm planning to post a new patch soon which will be much improved, thanks to your review.
          Hide
          Mahadev konar added a comment -

          Eugene,
          Any update on the patch?

          Show
          Mahadev konar added a comment - Eugene, Any update on the patch?
          Hide
          Eugene Koontz added a comment -

          Hi Mahadev, I am uploading my latest patch now, but am also doing integration tests with HBase. If those turn up any problems I will refresh the patch again.

          Show
          Eugene Koontz added a comment - Hi Mahadev, I am uploading my latest patch now, but am also doing integration tests with HBase. If those turn up any problems I will refresh the patch again.
          Hide
          Eugene Koontz added a comment -

          -Server-side SASL negotiation with client moved from
          FinalRequestProcessor to ZooKeeperServer
          (Thanks to Benjamin Reed for recommending this in
          his review).

          -Removed SASL-specific Zookeeper client states:
          Back to using the existing set of client states

          {CONNECTING, ASSOCIATING, etc}

          (Thanks to Benjamin Reed for recommending this in
          his review).

          -Moved SASL-related functionality out of ServerCnxnFactory.
          (Thanks to Benjamin Reed for recommending this in
          his review).

          -Removed requireClientAuthScheme: instead using
          zookeeper.maintain_connection_despite_sasl_failure property to
          accomplish the same thing (if maintain_connection_despite_sasl_failure=false,
          then server will shut down client. Benjamin mentioned in his review
          that requireClientAuthScheme is general functionality and would be
          appropriate, but I thought that might better be a separate JIRA issue
          if so.

          -Removed extra added renewJaasLoginInterval parameter to various constructors:
          instead, using system property-checking at the appropriate points within the
          relevant SASL- and Kerberos- related classes.
          (Thanks to Benjamin Reed for recommending this in his review).

          -Kerberos ticket cache renewal fix:
          Uses the Shell class from Hadoop. The source file was copied from
          Hadoop core with minor modifications detailed in comments after the
          Apache license.

          -More robust Kerberos principal parsing:
          Uses the KerberosName class from Hadoop's security branch. The source
          file was copied from Hadoop with minor modifications detailed in
          comments after the Apache license.

          -More flexible Kerberos principal-to-Zookeeper username translation

          The following system properties are supported:

          zookeeper.kerberos.removeHostFromPrincipal
          If this is set to true, then Kerberos principals of the form:
          user/host@REALM will be converted to user@REALM.

          zookeeper.kerberos.removeRealmFromPrincipal
          If this is set to true, then Kerberos principals of the forms:
          user/host@REALM and user@REALM will be converted to
          user/host and user respectively.

          If both removeHostFromPrincipal and removeRealmFromPrincipal are true,
          then Kerberos principals of the forms: user/host@REALM and user@REALM
          will both be converted to user.

          zookeeper.superUser
          If a value is provided, and Kerberos is used, then the provided value will
          be used as the superuser. This allows one to use a Kerberos principal such as:
          "user/zookeeperadmin@REALM" as the Zookeeper super user. The default value "super"
          will be used otherwise.

          Show
          Eugene Koontz added a comment - -Server-side SASL negotiation with client moved from FinalRequestProcessor to ZooKeeperServer (Thanks to Benjamin Reed for recommending this in his review). -Removed SASL-specific Zookeeper client states: Back to using the existing set of client states {CONNECTING, ASSOCIATING, etc} (Thanks to Benjamin Reed for recommending this in his review). -Moved SASL-related functionality out of ServerCnxnFactory. (Thanks to Benjamin Reed for recommending this in his review). -Removed requireClientAuthScheme: instead using zookeeper.maintain_connection_despite_sasl_failure property to accomplish the same thing (if maintain_connection_despite_sasl_failure=false, then server will shut down client. Benjamin mentioned in his review that requireClientAuthScheme is general functionality and would be appropriate, but I thought that might better be a separate JIRA issue if so. -Removed extra added renewJaasLoginInterval parameter to various constructors: instead, using system property-checking at the appropriate points within the relevant SASL- and Kerberos- related classes. (Thanks to Benjamin Reed for recommending this in his review). -Kerberos ticket cache renewal fix: Uses the Shell class from Hadoop. The source file was copied from Hadoop core with minor modifications detailed in comments after the Apache license. -More robust Kerberos principal parsing: Uses the KerberosName class from Hadoop's security branch. The source file was copied from Hadoop with minor modifications detailed in comments after the Apache license. -More flexible Kerberos principal-to-Zookeeper username translation The following system properties are supported: zookeeper.kerberos.removeHostFromPrincipal If this is set to true, then Kerberos principals of the form: user/host@REALM will be converted to user@REALM. zookeeper.kerberos.removeRealmFromPrincipal If this is set to true, then Kerberos principals of the forms: user/host@REALM and user@REALM will be converted to user/host and user respectively. If both removeHostFromPrincipal and removeRealmFromPrincipal are true, then Kerberos principals of the forms: user/host@REALM and user@REALM will both be converted to user. zookeeper.superUser If a value is provided, and Kerberos is used, then the provided value will be used as the superuser. This allows one to use a Kerberos principal such as: "user/zookeeperadmin@REALM" as the Zookeeper super user. The default value "super" will be used otherwise.
          Hide
          Mahadev konar added a comment -

          thanks a lot Eugene!

          Show
          Mahadev konar added a comment - thanks a lot Eugene!
          Hide
          Mahadev konar added a comment -

          ben,
          can you please review this patch?

          Show
          Mahadev konar added a comment - ben, can you please review this patch?
          Hide
          Benjamin Reed added a comment -

          sorry for being off the grid. last week was not good.

          great job eugene. overall it looks good. two minor cleanup things: 1) since we aren't pushing sasl through the pipeline, we should remove it from the Request class. 2) in the code you added to ZooKeeperServer can you move that big piece of code in the if clause to a function called processSasl() or something like that?

          KerberosName and Shell use sun.* classes, which cause warnings on the build and may cause problems on non-sun jvms. is there any workarounds? or are those classes exposed through java.* or javax.* classes? we either need to fix or document.

          Show
          Benjamin Reed added a comment - sorry for being off the grid. last week was not good. great job eugene. overall it looks good. two minor cleanup things: 1) since we aren't pushing sasl through the pipeline, we should remove it from the Request class. 2) in the code you added to ZooKeeperServer can you move that big piece of code in the if clause to a function called processSasl() or something like that? KerberosName and Shell use sun.* classes, which cause warnings on the build and may cause problems on non-sun jvms. is there any workarounds? or are those classes exposed through java.* or javax.* classes? we either need to fix or document.
          Hide
          Eugene Koontz added a comment -

          Address Benjamin Reed's feedback:

          -Remove OpCode.sasl from Request.java since it's not needed there.
          -in ZooKeeperServer, move SASL-related server-side code into separate method processSasl()

          Show
          Eugene Koontz added a comment - Address Benjamin Reed's feedback: -Remove OpCode.sasl from Request.java since it's not needed there. -in ZooKeeperServer, move SASL-related server-side code into separate method processSasl()
          Hide
          Eugene Koontz added a comment -

          Hi Benjamin,
          Thanks a lot for your feedback! I addressed the two cleanup issues in a new patch.

          Regarding the sun.* classes, these are only needed to determine the default Kerberos realm from the system's Kerberos conf file (usually in /etc/krb5.conf).

          I think we can use the java.security.* functionality to replace sun.security.*:
          http://download.oracle.com/javase/1.4.2/docs/guide/security/jgss/tutorials/KerberosReq.html

          I will try this out and update the patch if this will work.

          Show
          Eugene Koontz added a comment - Hi Benjamin, Thanks a lot for your feedback! I addressed the two cleanup issues in a new patch. Regarding the sun.* classes, these are only needed to determine the default Kerberos realm from the system's Kerberos conf file (usually in /etc/krb5.conf). I think we can use the java.security.* functionality to replace sun.security.*: http://download.oracle.com/javase/1.4.2/docs/guide/security/jgss/tutorials/KerberosReq.html I will try this out and update the patch if this will work.
          Hide
          Eugene Koontz added a comment -

          Hi Benjamin,

          Unfortunately I couldn't find support for obtaining the default Kerberos realm in java.security or javax.security.

          (In fact, it seems that javax.security uses sun.security internally, for example: http://www.java2s.com/Open-Source/Java-Document/6.0-JDK-Core/security/javax/security/auth/kerberos/KerberosPrincipal.java.htm.
          (see "import sun.security.krb5.Asn1Exception;" in the source code)).

          So I don't see any way around using the sun.* classes.

          -Eugene

          Show
          Eugene Koontz added a comment - Hi Benjamin, Unfortunately I couldn't find support for obtaining the default Kerberos realm in java.security or javax.security. (In fact, it seems that javax.security uses sun.security internally, for example: http://www.java2s.com/Open-Source/Java-Document/6.0-JDK-Core/security/javax/security/auth/kerberos/KerberosPrincipal.java.htm . (see "import sun.security.krb5.Asn1Exception;" in the source code)). So I don't see any way around using the sun.* classes. -Eugene
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12489872/ZOOKEEPER-938.patch
          against trunk revision 1152141.

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

          +1 tests included. The patch appears to include 4 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 appears to introduce 1 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/446//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/446//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/446//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/12489872/ZOOKEEPER-938.patch against trunk revision 1152141. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 appears to introduce 1 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/446//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/446//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/446//console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          Eugene,
          Can you please take a look at the test failures? Looks like the SASL tests (added in the patch) are failing.

          Show
          Mahadev konar added a comment - Eugene, Can you please take a look at the test failures? Looks like the SASL tests (added in the patch) are failing.
          Hide
          Eugene Koontz added a comment -

          -fix FindBugs warning related to constructor starting a thread.
          -don't throw IllegalArgumentException if /etc/krb5.conf not found (such as on Apache's Jenkins server).

          Show
          Eugene Koontz added a comment - -fix FindBugs warning related to constructor starting a thread. -don't throw IllegalArgumentException if /etc/krb5.conf not found (such as on Apache's Jenkins server).
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 4 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/463//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/463//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/463//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/12490160/ZOOKEEPER-938.patch against trunk revision 1157698. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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/463//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/463//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/463//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          +1 looks good Eugene! thanx for sticking with this patch!

          Show
          Benjamin Reed added a comment - +1 looks good Eugene! thanx for sticking with this patch!
          Hide
          Mahadev konar added a comment -

          I just pushed this to trunk!

          Show
          Mahadev konar added a comment - I just pushed this to trunk!
          Hide
          Mahadev konar added a comment -

          Forgot to mention, Thanks a lot Eugene!

          Show
          Mahadev konar added a comment - Forgot to mention, Thanks a lot Eugene!
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #1270 (See https://builds.apache.org/job/ZooKeeper-trunk/1270/)
          ZOOKEEPER-938. Support Kerberos authentication of clients. (Eugene Koontz via mahadev)

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

          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooDefs.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthTest.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/Watcher.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
          • /zookeeper/trunk/src/zookeeper.jute
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/KerberosName.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxn.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/Shell.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java
          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java.orig
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/Login.java
          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #1270 (See https://builds.apache.org/job/ZooKeeper-trunk/1270/ ) ZOOKEEPER-938 . Support Kerberos authentication of clients. (Eugene Koontz via mahadev) mahadev : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1159432 Files : /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooDefs.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/Watcher.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/DigestLoginModule.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java /zookeeper/trunk/src/zookeeper.jute /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/KerberosName.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxn.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/Shell.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java.orig /zookeeper/trunk/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/Login.java

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development