[GitHub] zookeeper pull request #457: ZOOKEEPER-1534: ZookeeperServer now returns Aut...
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...
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...
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...
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...
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 ---