[GitHub] zookeeper pull request #457: ZOOKEEPER-1534: ZookeeperServer now returns Aut...

[eolivelli]

Github user eolivelli commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/457#discussion_r168304302
  
--- Diff: src/java/test/org/apache/zookeeper/test/WatcherAuthTest.java ---
@@ -0,0 +1,84 @@
+package org.apache.zookeeper.test;
+
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.client.ZKClientConfig;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.util.concurrent.LinkedBlockingQueue;
+
+import static org.apache.zookeeper.test.ClientBase.createTmpDir;
+
+public class WatcherAuthTest {
--- End diff --

I am sorry, I can't find in this test where ate we starting the server


---

[GitHub] zookeeper pull request #457: ZOOKEEPER-1534: ZookeeperServer now returns Aut...

[eolivelli]

Github user eolivelli commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/457#discussion_r168303288
  
--- Diff: src/java/test/org/apache/zookeeper/test/WatcherAuthTest.java ---
@@ -0,0 +1,84 @@
+package org.apache.zookeeper.test;
+
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.client.ZKClientConfig;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.util.concurrent.LinkedBlockingQueue;
+
+import static org.apache.zookeeper.test.ClientBase.createTmpDir;
+
+public class WatcherAuthTest {
+
+protected static final Logger LOG = 
LoggerFactory.getLogger(WatcherTest.class);
+
+private class MyWatcher extends ClientBase.CountdownWatcher {
+LinkedBlockingQueue events =
+new LinkedBlockingQueue();
+
+@Override
+public void process(WatchedEvent event) {
+super.process(event);
+if (event.getState() == Event.KeeperState.AuthFailed) {
+try {
+events.put(event);
+} catch (InterruptedException e) {
+LOG.warn("ignoring interrupt during event.put");
+}
+}
+}
+}
+
+@Before
+public void setUp() throws Exception {
+// Reset to default value since some test cases set this to true.
+// Needed for JDK7 since unit test can run is random order
+System.setProperty(ZKClientConfig.DISABLE_AUTO_WATCH_RESET, 
"false");
+}
+
+// Note: This test only works with a real ZKServer, running with 
TestServer masks the bug
+@Ignore
+@Test
+public void testWatcherCanGetAuthFailedEvents() throws Exception {
+MyWatcher myWatcher = new MyWatcher();
+
System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.SASLAuthenticationProvider");
+try {
+File tmpDir = createTmpDir();
+File saslConfFile = new File(tmpDir, "jaas.conf");
+FileWriter fwriter = new FileWriter(saslConfFile);
+
+fwriter.write("" +
+"Server {\n" +
+"  
org.apache.zookeeper.server.auth.DigestLoginModule required\n" +
+"  user_super=\"test\";\n" +
+"};\n" +
+"Client {\n" +
+"   
org.apache.zookeeper.server.auth.DigestLoginModule required\n" +
+"   username=\"super\"\n" +
+"   password=\"test1\";\n" + // NOTE: wrong 
password ('test' != 'test1') : this is to test SASL authentication failure.
+"};" + "\n");
+fwriter.close();
+
System.setProperty("java.security.auth.login.config",saslConfFile.getAbsolutePath());
+}
+catch (IOException e) {
+// could not create tmp directory to hold JAAS conf file.
+}
+
+// Specify your ZK Server endpoints here
+ZooKeeper zk = new ZooKeeper("127.0.0.1:2281", 2, myWatcher);
--- End diff --

We should close this handle


---

[GitHub] zookeeper pull request #457: ZOOKEEPER-1534: ZookeeperServer now returns Aut...

[eolivelli]

Github user eolivelli commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/457#discussion_r168302735
  
--- Diff: 
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java ---
@@ -310,6 +313,9 @@ public void respondToServer(byte[] serverToken, 
ClientCnxn cnxn) {
 // TODO: introspect about runtime environment (such as 
jaas.conf)
 saslState = SaslState.FAILED;
 throw new SaslException("Error in authenticating with a 
Zookeeper Quorum member: the quorum member's saslToken is null.");
+} else if (new String(saslToken).equals(AUTHENTICATION_FAILED)) {
--- End diff --

Not setting an explicit charset is a code smell, in this case we are using 
only chars so it won't be a gread deal, but it is better ti have clean code 


---

[GitHub] zookeeper pull request #457: ZOOKEEPER-1534: ZookeeperServer now returns Aut...

[eolivelli]

Github user eolivelli commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/457#discussion_r168302873
  
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -1101,8 +1102,16 @@ public void processPacket(ServerCnxn cnxn, 
ByteBuffer incomingBuffer) throws IOE
 } else {
 if (h.getType() == OpCode.sasl) {
 Record rsp = processSasl(incomingBuffer,cnxn);
-ReplyHeader rh = new ReplyHeader(h.getXid(), 0, 
KeeperException.Code.OK.intValue());
-cnxn.sendResponse(rh,rsp, "response"); // not sure about 
3rd arg..what is it?
+if (rsp == null) {
+ReplyHeader rh = new ReplyHeader(h.getXid(), 0, 
Code.AUTHFAILED.intValue());
+cnxn.sendResponse(rh, new 
SetSASLResponse(AUTHENTICATION_FAILED.getBytes()), "response"); // not sure 
about 3rd arg..what is it?
+LOG.warn("Closing client connection due to SASL 
authentication failure.");
--- End diff --

Can we log at least the ip address of the client?


---

[GitHub] zookeeper pull request #457: ZOOKEEPER-1534: ZookeeperServer now returns Aut...

[craz186]

GitHub user craz186 opened a pull request:

https://github.com/apache/zookeeper/pull/457

ZOOKEEPER-1534: ZookeeperServer now returns AuthFailed events for SASL cred 
failures

ZookeeperServer previously closed client connections instead of returning 
AuthFailed events for SASL authentication failures.
This PR changes the Zookeeper Server to return an AuthFailed event and then 
afterwards closes the connection. 
I am unsure of the standard for SetSaslResponses and would appreciate any 
feedback as to how to represent a failed Authentication through SetSaslResponse 
objects. Currently I am just returning a string.

Note: The unit test I've supplied will only work with a real ZKServer, it 
seems that the testing server hides this bug and I've been unable to reproduce 
with the Testing Server. 

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/craz186/zookeeper ZOOKEEPER-1534

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/457.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #457


commit add6963b8e62f3ccdaf80f1a02428544c3a105d8
Author: sean.gibbons 
Date:   2018-02-05T16:09:59Z

ZOOKEEPER-1534: ZookeeperServer now returns AuthFailed events instead of 
closing client connection when SASL authentication uses invalid credentials, 
added unit test to demonstrate




---