This is an automated email from the ASF dual-hosted git repository.
asf-gitbox-commits pushed a commit to branch branch-3.9
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
The following commit(s) were added to refs/heads/branch-3.9 by this push:
new 58ab7b1d94 ZOOKEEPER-4947: Stop the send loop after SASL
authentication fails
58ab7b1d94 is described below
commit 58ab7b1d9463ef63045335e320cbd75207a3d450
Author: Stefan Wang <[email protected]>
AuthorDate: Tue Sep 29 09:53:26 2026 -0700
ZOOKEEPER-4947: Stop the send loop after SASL authentication fails
Reviewers: kezhuw, anmolnar
Author: 1fanwang
Closes #2443 from 1fanwang/fix-zookeeper-4947-auth-failed-close
(cherry picked from commit 92f104032d5a2f3938d6514ee46cccc07f9d522b)
Signed-off-by: Andor Molnar <[email protected]>
---
.../main/java/org/apache/zookeeper/ClientCnxn.java | 1 +
.../main/java/org/apache/zookeeper/ZooKeeper.java | 2 +-
.../org/apache/zookeeper/TestableZooKeeper.java | 5 ++++
.../apache/zookeeper/test/SaslAuthFailTest.java | 21 ++++++++++++--
.../test/SaslAuthRequiredMultiClientTest.java | 32 ++++++++++++++--------
5 files changed, 46 insertions(+), 15 deletions(-)
diff --git
a/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
index 9e79f346de..b59d9d0a51 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
@@ -1233,6 +1233,7 @@ public void run() {
eventThread.queueEvent(new
WatchedEvent(Watcher.Event.EventType.None, authState, null));
if (state == States.AUTH_FAILED) {
eventThread.queueEventOfDeath();
+ break;
}
}
}
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
index 1108833a27..18b94d97c9 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
@@ -1214,7 +1214,7 @@ public synchronized void register(Watcher watcher) {
* @throws InterruptedException
*/
public synchronized void close() throws InterruptedException {
- if (!cnxn.getState().isAlive()) {
+ if (cnxn.getState() == States.CLOSED) {
LOG.debug("Close called on already closed client");
return;
}
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java
index 7f9e41e338..da9e5a7cc2 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java
@@ -85,6 +85,11 @@ public void run() {
}
}
+ @Override
+ public boolean testableWaitForShutdown(int wait) throws
InterruptedException {
+ return super.testableWaitForShutdown(wait);
+ }
+
public SocketAddress testableLocalSocketAddress() {
return super.testableLocalSocketAddress();
}
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java
index 2384cd612e..0dfd2cc955 100644
---
a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java
+++
b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java
@@ -18,12 +18,18 @@
package org.apache.zookeeper.test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.TestableZooKeeper;
import org.apache.zookeeper.WatchedEvent;
import org.apache.zookeeper.Watcher.Event.KeeperState;
import org.apache.zookeeper.ZooDefs.Ids;
@@ -77,7 +83,7 @@ public synchronized void process(WatchedEvent event) {
@Test
public void testAuthFail() {
- try (ZooKeeper zk = createClient()) {
+ try (ZooKeeper zk = new ZooKeeper(hostPort, CONNECTION_TIMEOUT, new
CountdownWatcher())) {
zk.create("/path1", null, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT);
fail("Should have gotten exception.");
} catch (Exception e) {
@@ -88,9 +94,18 @@ public void testAuthFail() {
@Test
public void testBadSaslAuthNotifiesWatch() throws Exception {
- try (ZooKeeper ignored = createClient(new MyWatcher(), hostPort)) {
+ try (TestableZooKeeper zk = new TestableZooKeeper(hostPort,
CONNECTION_TIMEOUT, new MyWatcher())) {
// wait for authFailed event from client's EventThread.
- authFailed.await();
+ assertTrue(authFailed.await(CONNECTION_TIMEOUT,
TimeUnit.MILLISECONDS));
+ boolean threadsStopped = zk.testableWaitForShutdown(1000);
+ LOG.info("SASL failure shutdown without close: threadsStopped={},
state={}",
+ threadsStopped, zk.getState());
+ assertTrue(threadsStopped, "Client threads should stop after SASL
authentication fails");
+ assertEquals(ZooKeeper.States.AUTH_FAILED, zk.getState());
+ assertThrows(KeeperException.AuthFailedException.class, () ->
zk.exists("/", false));
+ zk.close();
+ assertEquals(ZooKeeper.States.CLOSED, zk.getState());
+ assertTrue(zk.close(CONNECTION_TIMEOUT));
}
}
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthRequiredMultiClientTest.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthRequiredMultiClientTest.java
index f21d634558..cd082f5145 100644
---
a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthRequiredMultiClientTest.java
+++
b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthRequiredMultiClientTest.java
@@ -19,10 +19,15 @@
package org.apache.zookeeper.test;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
import javax.security.auth.login.Configuration;
import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.Watcher.Event.KeeperState;
import org.apache.zookeeper.ZooDefs.Ids;
import org.apache.zookeeper.ZooKeeper;
import org.junit.jupiter.api.AfterAll;
@@ -55,12 +60,7 @@ public void
testClientOpWithInvalidSASLUserAuthAfterSuccessLogin() throws Except
}
resetJaasConfiguration("jaas.conf", "super_wrong", "test");
- try (ZooKeeper wrongUserZk = createClient()) {
- wrongUserZk.create("/bar", null, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT);
- fail("Client with wrong SASL config should not pass SASL
authentication.");
- } catch (KeeperException e) {
- assertEquals(KeeperException.Code.AUTHFAILED, e.code());
- }
+ assertClientAuthFailed();
}
@Test
@@ -73,11 +73,21 @@ public void
testClientOpWithInvalidSASLPasswordAuthAfterSuccessLogin() throws Ex
}
resetJaasConfiguration("jaas.conf", "super", "test_wrongong");
- try (ZooKeeper wrongPasswordZk = createClient()) {
- wrongPasswordZk.create("/bar", null, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT);
- fail("Client with wrong SASL config should not pass SASL
authentication.");
- } catch (KeeperException e) {
- assertEquals(KeeperException.Code.AUTHFAILED, e.code());
+ assertClientAuthFailed();
+ }
+
+ private void assertClientAuthFailed() throws Exception {
+ CountDownLatch authFailed = new CountDownLatch(1);
+ // A rejected connection may disappear before createClient's JMX check.
+ try (ZooKeeper zk = new ZooKeeper(hostPort, CONNECTION_TIMEOUT, event
-> {
+ if (event.getState() == KeeperState.AuthFailed) {
+ authFailed.countDown();
+ }
+ })) {
+ assertTrue(authFailed.await(CONNECTION_TIMEOUT,
TimeUnit.MILLISECONDS));
+ assertEquals(ZooKeeper.States.AUTH_FAILED, zk.getState());
+ assertThrows(KeeperException.AuthFailedException.class,
+ () -> zk.create("/bar", null, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT));
}
}
