zookeeper-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [zookeeper] ztzg commented on a change in pull request #1054: ZOOKEEPER-1112: Add support for C client for SASL authentication
Date Thu, 15 Aug 2019 08:01:38 GMT
ztzg commented on a change in pull request #1054: ZOOKEEPER-1112: Add support for C client
for SASL authentication
URL: https://github.com/apache/zookeeper/pull/1054#discussion_r314209362

 File path: zookeeper-server/src/main/java/org/apache/zookeeper/util/SecurityUtils.java
 @@ -153,6 +155,11 @@ public SaslClient run() throws SaslException {
     public static SaslServer createSaslServer(final Subject subject,
             final String protocol, final String serverName,
             final CallbackHandler callbackHandler, final Logger LOG) {
+        // required by c client api - Sasl.QOP="auth" is not set
+        // by default although stated in javadoc (Sun JRE 1.6.0_26-b03)
+        HashMap<String, Object> props = new HashMap<String, Object>();
 Review comment:
   That change was part of the original patches, which I have tried to "rebase" with a minimum
amount of changes.
   I don't have a good answer to this question.  I don't know if @kloni is still around, and
if he could comment on it.
   Looking closer, it looks like putting anything else than `auth` in there might not be compliant—so
I am planning to revert this part of the patch in an upcoming respin, and am including the
preliminary commit message below.
   (Note that I am not against squashing some of the patches if it makes the series easier
to ingest; just let me know if you prefer sequential changes or mixed authorship.)
   ZOOKEEPER-1112: Do not specify QOP for the SASL server
   The explicit QOP setting had been added with a comment specifying that `Sasl.QOP="auth"`
was not set by the 1.6 JRE.
   More recent JREs, such as 1.8, properly set QOP to `auth` by default, and both Cyrus SASL
and Perl's `Authen::SASL` have been verified to be okay with it.
   The patch also included `auth-conf` and `auth-int` in the preferences list; the reason
for that is unclear.  It also seems incorrect, as the wire protocol does not provide checksums
nor encryption.  (The plan is to carry everything over TLS anyway; perhaps QOP should be set
to that triple when TLS is active?)

This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:

With regards,
Apache Git Services

View raw message