ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1497

Allow server-side SASL login with JAAS configuration to be programmatically set (rather than only by reading JAAS configuration file)

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.4.3, 3.5.0
    • Fix Version/s: 3.4.4, 3.5.0
    • Component/s: server
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      Currently the CnxnFactory checks for "java.security.auth.login.config" to decide whether or not enable SASL.

      • zookeeper/server/NIOServerCnxnFactory.java
      • zookeeper/server/NettyServerCnxnFactory.java
        • configure() checks for "java.security.auth.login.config"
          • If present start the new Login("Server", SaslServerCallbackHandler(conf))

      But since the SaslServerCallbackHandler does the right thing just checking if getAppConfigurationEntry() is empty, we can allow SASL with JAAS configuration to be programmatically just checking weather or not a configuration entry is present instead of "java.security.auth.login.config".
      (Something quite similar was done for the SaslClient in ZOOKEEPER-1373)

      1. ZOOKEEPER-1497-v5.patch
        21 kB
        Matteo Bertozzi
      2. ZOOKEEPER-1497-v4.patch
        20 kB
        Matteo Bertozzi
      3. ZOOKEEPER-1497-v3.patch
        15 kB
        Matteo Bertozzi
      4. ZOOKEEPER-1497-v2.patch
        14 kB
        Matteo Bertozzi
      5. ZOOKEEPER-1497-v1.patch
        8 kB
        Matteo Bertozzi

        Issue Links

          Activity

          Hide
          Matteo Bertozzi added a comment -

          Allow JAAS configuration to be programmatically set, and added ZooKeeperSaslServer.LOGIN_CONTEXT_NAME_KEY property to be able to specify a configuration entry different from Server

          Show
          Matteo Bertozzi added a comment - Allow JAAS configuration to be programmatically set, and added ZooKeeperSaslServer.LOGIN_CONTEXT_NAME_KEY property to be able to specify a configuration entry different from 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/12535122/ZOOKEEPER-1497-v1.patch
          against trunk revision 1355657.

          +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 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/1120//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1120//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1120//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/12535122/ZOOKEEPER-1497-v1.patch against trunk revision 1355657. +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 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/1120//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1120//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1120//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          Hi Matteo,
          It looks like a good design overall.

          It's good that we now have (with this patch) zookeeper.sasl.serverconfig as well as the existing zookeeper.sasl.clientconfig (from ZOOKEEPER-1373).

          I think ServerCnxnFactory.configureSaslLogin() could use some documentation about why we throw a caught exception or ignore it.

          Some logging at ERROR or WARN level where appropriate, to help ZK admins diagnose configuration problems (for example, what if zookeeper.sasl.serverconfig references a non-existent JAAS configuration section? or, what if java.security.auth.login.config points to a non-existent file, but Configuration has been set programmatically and is valid, should we ignore the non-existent file and login with the valid Configuration?)

          Some additional tests would be good as the QA bot mentioned. See SaslAuthDesignatedClientTest.java from ZOOKEEPER-1373 for how I added more configuration testing on the Client side.

          Would be nice to test programmatically-set JAAS configuration. Rather than writing a string to a temporary file and then setting java.security.auth.login.config to point to it, instead, we'd build a javax.security.auth.login.Configuration in code in the tests and then call Configuration.setConfiguration() (if I understand the API correctly).

          Show
          Eugene Koontz added a comment - Hi Matteo, It looks like a good design overall. It's good that we now have (with this patch) zookeeper.sasl.serverconfig as well as the existing zookeeper.sasl.clientconfig (from ZOOKEEPER-1373 ). I think ServerCnxnFactory.configureSaslLogin() could use some documentation about why we throw a caught exception or ignore it. Some logging at ERROR or WARN level where appropriate, to help ZK admins diagnose configuration problems (for example, what if zookeeper.sasl.serverconfig references a non-existent JAAS configuration section? or, what if java.security.auth.login.config points to a non-existent file, but Configuration has been set programmatically and is valid, should we ignore the non-existent file and login with the valid Configuration?) Some additional tests would be good as the QA bot mentioned. See SaslAuthDesignatedClientTest.java from ZOOKEEPER-1373 for how I added more configuration testing on the Client side. Would be nice to test programmatically-set JAAS configuration. Rather than writing a string to a temporary file and then setting java.security.auth.login.config to point to it, instead, we'd build a javax.security.auth.login.Configuration in code in the tests and then call Configuration.setConfiguration() (if I understand the API correctly).
          Hide
          Eugene Koontz added a comment -

          I added ZOOKEEPER-1503 which would help a bit for adding tests to ZOOKEEPER-1497.

          Show
          Eugene Koontz added a comment - I added ZOOKEEPER-1503 which would help a bit for adding tests to ZOOKEEPER-1497 .
          Hide
          Matteo Bertozzi added a comment -

          added comment and test

          Show
          Matteo Bertozzi added a comment - added comment and test
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12535265/ZOOKEEPER-1497-v2.patch
          against trunk revision 1357711.

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

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

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

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

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

          -1 release audit. The applied patch generated 25 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://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1124//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1124//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1124//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1124//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/12535265/ZOOKEEPER-1497-v2.patch against trunk revision 1357711. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. -1 release audit. The applied patch generated 25 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://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1124//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1124//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1124//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1124//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/12536098/ZOOKEEPER-1497-v3.patch
          against trunk revision 1357711.

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

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

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

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

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

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1133//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1133//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1133//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/12536098/ZOOKEEPER-1497-v3.patch against trunk revision 1357711. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1133//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1133//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1133//console This message is automatically generated.
          Hide
          Matteo Bertozzi added a comment -

          Is there something else that I should fix before this patch goes in?
          I need this one for HBASE-4791.

          Show
          Matteo Bertozzi added a comment - Is there something else that I should fix before this patch goes in? I need this one for HBASE-4791 .
          Hide
          Matteo Bertozzi added a comment -

          The patch is on Review board if someone want to add some inline comments
          https://reviews.apache.org/r/6290

          Show
          Matteo Bertozzi added a comment - The patch is on Review board if someone want to add some inline comments https://reviews.apache.org/r/6290
          Hide
          Patrick Hunt added a comment -

          Added some comments on RB. thanks Matteo.

          Show
          Patrick Hunt added a comment - Added some comments on RB. thanks Matteo.
          Hide
          Patrick Hunt added a comment -

          Committed. Thanks Matteo!

          Show
          Patrick Hunt added a comment - Committed. Thanks Matteo!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12538966/ZOOKEEPER-1497-v5.patch
          against trunk revision 1378285.

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

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

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

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

          Integrated in ZooKeeper-trunk #1663 (See https://builds.apache.org/job/ZooKeeper-trunk/1663/)
          ZOOKEEPER-1497. Allow server-side SASL login with JAAS configuration to be programmatically set (rather than only by reading JAAS configuration file) (Matteo Bertozzi via phunt) (Revision 1378285)

          Result = SUCCESS
          phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1378285
          Files :

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/Environment.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/JaasConfiguration.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ClientBase.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedServerTest.java
          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #1663 (See https://builds.apache.org/job/ZooKeeper-trunk/1663/ ) ZOOKEEPER-1497 . Allow server-side SASL login with JAAS configuration to be programmatically set (rather than only by reading JAAS configuration file) (Matteo Bertozzi via phunt) (Revision 1378285) Result = SUCCESS phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1378285 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/Environment.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/JaasConfiguration.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ClientBase.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedServerTest.java
          Hide
          Mahadev konar added a comment -

          Pat,
          Any reason this is not resolved?

          Show
          Mahadev konar added a comment - Pat, Any reason this is not resolved?
          Hide
          Patrick Hunt added a comment -

          Odd.... I thought I had resolved it. I remember doing that. However it was still listed as PA, even though my annotation on the fix version, and comment thanking Matteo both got committed. Perhaps a hiccup due to the recent jira updates?

          Anyway, seems to have resolved now. Thanks Mahadev for pointing it out.

          Show
          Patrick Hunt added a comment - Odd.... I thought I had resolved it. I remember doing that. However it was still listed as PA, even though my annotation on the fix version, and comment thanking Matteo both got committed. Perhaps a hiccup due to the recent jira updates? Anyway, seems to have resolved now. Thanks Mahadev for pointing it out.
          Hide
          Mahadev konar added a comment -

          Pat, was this committed to 3.4 branch? I dont see it. Maybe I missed it?

          Show
          Mahadev konar added a comment - Pat, was this committed to 3.4 branch? I dont see it. Maybe I missed it?
          Hide
          Mahadev konar added a comment -

          Nevermind, I see it now. Mistake on my side!

          Show
          Mahadev konar added a comment - Nevermind, I see it now. Mistake on my side!

            People

            • Assignee:
              Matteo Bertozzi
              Reporter:
              Matteo Bertozzi
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development