[GitHub] [zookeeper] ztzg commented on a change in pull request #1054: ZOOKEEPER-1112: Add support for C client for SASL authentication

2019-08-15 Thread GitBox 
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 props = new HashMap();
 
 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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [zookeeper] ztzg commented on a change in pull request #1054: ZOOKEEPER-1112: Add support for C client for SASL authentication

2019-08-15 Thread GitBox 
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_r314208678
 
 

 ##
 File path: bin/zkServer.sh
 ##
 @@ -212,6 +212,7 @@ stop)
 else
   $KILL $(cat "$ZOOPIDFILE")
   rm "$ZOOPIDFILE"
+  sleep 1
 
 Review comment:
   Hi Enrico,
   
   Probably for the same reason that there is a `sleep 3` in `restart`.  Here 
is what I have put in the corresponding commit message:
   
   ---
   
   ZOOKEEPER-1112: Make `zkServer.sh stop` more reliable
   
   Kill is asynchronous, and without the sleep, the server's TCP port can still 
be busy when the next server is started—causing flaky runs of the C client's 
test suite.
   
   (It would probably be better to spin a few times, probing with `ps -p`.)
   
   ---
   
   This is not hypothetical: I am observing `FAILED TO START` errors in the C 
test suite; the log consistently shows that those are caused by 
`java.net.BindException: Address already in use`.
   
   As noted above, the `sleep` is far from optimal, an adaptive mechanism would 
be better—but I did not want to make the patch too complicated.
   
   Here is what I would suggest: I will drop the patch from my upcoming respin, 
create a dedicated ticket, and carry it locally in the meantime.


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:
us...@infra.apache.org


With regards,
Apache Git Services