You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zookeeper.apache.org by an...@apache.org on 2026/09/30 15:50:59 UTC
(zookeeper) branch branch-3.9 updated (58ab7b1d9 -> 509ffd01e)
This is an automated email from the ASF dual-hosted git repository.
anmolnar pushed a change to branch branch-3.9
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
from 58ab7b1d9 ZOOKEEPER-4947: Stop the send loop after SASL authentication fails
new 0afc92745 SASL authzid: add extra validation on client port to mirror quorum check
new 184738dc4 setACL race — missing acl attribute setting
new 0f7ce95f2 Quota: protect quota-stats data with format validation
new 1f169e149 Add multiRead response throttling to guard resource allocation
new 509ffd01e Check valid voting member before applying (re)config during LE
The 5 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../src/main/resources/markdown/zookeeperAdmin.md | 24 ++
.../src/main/java/org/apache/zookeeper/Quotas.java | 11 +-
.../main/java/org/apache/zookeeper/StatsTrack.java | 74 +++++-
.../java/org/apache/zookeeper/server/DataTree.java | 55 +++--
.../zookeeper/server/FinalRequestProcessor.java | 49 +++-
.../zookeeper/server/PrepRequestProcessor.java | 54 +++++
.../apache/zookeeper/server/ZooKeeperServer.java | 45 ++++
.../server/auth/SaslServerCallbackHandler.java | 36 ++-
.../server/quorum/FastLeaderElection.java | 60 +++--
.../org/apache/zookeeper/server/DataTreeTest.java | 118 ++++++++++
.../zookeeper/server/PrepRequestProcessorTest.java | 65 ++++++
.../server/auth/SaslServerCallbackHandlerTest.java | 250 +++++++++++++++++++++
.../server/quorum/FLEConfigFromNonVoterTest.java | 151 +++++++++++++
.../apache/zookeeper/test/MultiOperationTest.java | 115 ++++++++++
.../java/org/apache/zookeeper/test/QuotasTest.java | 14 ++
.../org/apache/zookeeper/test/StatsTrackTest.java | 86 +++++++
16 files changed, 1162 insertions(+), 45 deletions(-)
create mode 100644 zookeeper-server/src/test/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandlerTest.java
create mode 100644 zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/FLEConfigFromNonVoterTest.java
(zookeeper) 03/05: Quota: protect quota-stats data with format validation
Posted by an...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
anmolnar pushed a commit to branch branch-3.9
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
commit 0f7ce95f2fcd2906c9f2915506f0fc6ed972b8d8
Author: Andor Molnar <an...@cloudera.com>
AuthorDate: Mon Sep 21 12:45:22 2026 -0500
Quota: protect quota-stats data with format validation
---
.../src/main/java/org/apache/zookeeper/Quotas.java | 11 +-
.../main/java/org/apache/zookeeper/StatsTrack.java | 74 ++++++++++++-
.../java/org/apache/zookeeper/server/DataTree.java | 55 +++++++---
.../zookeeper/server/PrepRequestProcessor.java | 46 ++++++++
.../org/apache/zookeeper/server/DataTreeTest.java | 118 +++++++++++++++++++++
.../java/org/apache/zookeeper/test/QuotasTest.java | 14 +++
.../org/apache/zookeeper/test/StatsTrackTest.java | 86 +++++++++++++++
7 files changed, 387 insertions(+), 17 deletions(-)
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/Quotas.java b/zookeeper-server/src/main/java/org/apache/zookeeper/Quotas.java
index eb83cfea5..1292b5df2 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/Quotas.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/Quotas.java
@@ -78,9 +78,18 @@ public static String statPath(String path) {
* return the real path associated with this
* quotaPath.
* @param quotaPath the quotaPath which's started with /zookeeper/quota
- * @return the real path associated with this quotaPath.
+ * @return the real path associated with this quotaPath, or an empty string
+ * if the given path is not actually under /zookeeper/quota. Returning
+ * "" rather than throwing keeps a stray {@code zookeeper_limits} node
+ * elsewhere under /zookeeper (or the /zookeeper/quota root itself)
+ * from raising StringIndexOutOfBoundsException on the
+ * transaction-apply path, which would kill the SyncRequestProcessor
+ * critical thread and leave the dataDir unbootable on replay.
*/
public static String trimQuotaPath(String quotaPath) {
+ if (quotaPath == null || !quotaPath.startsWith(quotaZookeeper)) {
+ return "";
+ }
return quotaPath.substring(quotaZookeeper.length());
}
}
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/StatsTrack.java b/zookeeper-server/src/main/java/org/apache/zookeeper/StatsTrack.java
index 02fffb753..bd86422e7 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/StatsTrack.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/StatsTrack.java
@@ -26,12 +26,16 @@
import java.util.Objects;
import java.util.regex.Pattern;
import org.apache.zookeeper.common.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* a class that represents the stats associated with quotas
*/
public class StatsTrack {
+ private static final Logger LOG = LoggerFactory.getLogger(StatsTrack.class);
+
private static final String countStr = "count";
private static final String countHardLimitStr = "countHardLimit";
@@ -54,7 +58,11 @@ public StatsTrack() {
* @param stat the byte[] stat to be initialized with
*/
public StatsTrack(byte[] stat) {
- this(new String(stat, StandardCharsets.UTF_8));
+ // A null payload (jute length -1) must be tolerated here: this is on the
+ // transaction-apply/replay path, and new String(null, ...) would NPE,
+ // killing the SyncRequestProcessor critical thread and bricking the
+ // dataDir just like a malformed value. Treat null as empty/uninitialized.
+ this(stat == null ? null : new String(stat, StandardCharsets.UTF_8));
}
/**
@@ -70,9 +78,71 @@ public StatsTrack(String stat) {
}
String[] keyValuePairs = PAIRS_SEPARATOR.split(stat);
for (String keyValuePair : keyValuePairs) {
+ // NOTE: split with the default (limit 0) is intentional and must be
+ // kept: toString() emits a compatibility sentinel trailing '=' (e.g.
+ // "bytes=5=") that the default split silently drops, recovering "5".
+ // Changing the limit would break the round-trip of hard-limit values.
+ String[] kv = keyValuePair.split("=");
+ if (kv.length < 2 || StringUtils.isEmpty(kv[0])) {
+ // Malformed entry (e.g. "x", or "count=" with no value). This
+ // data is applied from an already-committed transaction, so
+ // throwing here would kill the SyncRequestProcessor critical
+ // thread and, because the transaction is durable, make the
+ // dataDir unbootable on replay. Skip the entry defensively
+ // instead (treated as uninitialized, i.e. -1); updateQuotaStat
+ // rewrites the node with a well-formed value, so it self-heals.
+ LOG.warn("Ignoring malformed quota stat entry: '{}'", keyValuePair);
+ continue;
+ }
+ String value = StringUtils.isEmpty(kv[1]) ? "-1" : kv[1];
+ try {
+ this.stats.put(kv[0], Long.parseLong(value));
+ } catch (NumberFormatException e) {
+ LOG.warn("Ignoring quota stat entry with non-numeric value: '{}'", keyValuePair);
+ }
+ }
+ }
+
+ /**
+ * Strictly validate that {@code data} is a well-formed stats/limits value,
+ * i.e. a (possibly empty) list of {@code key=long} pairs separated by
+ * {@code ,} or {@code ;}. Unlike the constructor, this performs no repair —
+ * it is used at request-preparation time to reject malformed writes to the
+ * quota {@code zookeeper_stats}/{@code zookeeper_limits} nodes before they
+ * are ever committed, so clients get a clear error and the quota bookkeeping
+ * never ingests garbage.
+ *
+ * @param data the candidate node data (UTF-8 encoded); empty is accepted
+ * (treated as uninitialized), null is rejected
+ * @return true if the data parses cleanly as StatsTrack content
+ */
+ public static boolean isValidStatsData(byte[] data) {
+ if (data == null) {
+ // null is never legitimate for a stats/limits node and NPEs the
+ // byte[] constructor on the apply path — reject it at prep time.
+ return false;
+ }
+ if (data.length == 0) {
+ return true;
+ }
+ String stat = new String(data, StandardCharsets.UTF_8);
+ String[] keyValuePairs = PAIRS_SEPARATOR.split(stat);
+ for (String keyValuePair : keyValuePairs) {
+ // Same tokenization as the constructor so the compatibility sentinel
+ // trailing '=' (e.g. "bytes=5=") is accepted, not rejected.
String[] kv = keyValuePair.split("=");
- this.stats.put(kv[0], Long.parseLong(StringUtils.isEmpty(kv[1]) ? "-1" : kv[1]));
+ if (kv.length < 2 || StringUtils.isEmpty(kv[0])) {
+ return false;
+ }
+ if (!StringUtils.isEmpty(kv[1])) {
+ try {
+ Long.parseLong(kv[1]);
+ } catch (NumberFormatException e) {
+ return false;
+ }
+ }
}
+ return true;
}
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
index aa9b7cee7..dbce6f90d 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
@@ -498,14 +498,25 @@ public void createNode(final String path, byte[] data, List<ACL> acl, long ephem
}
// now check if its one of the zookeeper node child
if (parentName.startsWith(quotaZookeeper)) {
- // now check if it's the limit node
- if (Quotas.limitNode.equals(childName)) {
- // this is the limit node
- // get the parent and add it to the trie
- pTrie.addPath(Quotas.trimQuotaPath(parentName));
- }
- if (Quotas.statNode.equals(childName)) {
- updateQuotaForPath(Quotas.trimQuotaPath(parentName));
+ boolean isLimitNode = Quotas.limitNode.equals(childName);
+ boolean isStatNode = Quotas.statNode.equals(childName);
+ if (isLimitNode || isStatNode) {
+ // The quota namespace between /zookeeper/quota and the
+ // limit/stat node must be non-empty. A limit/stat node created
+ // directly under /zookeeper/quota trims to "", and
+ // PathTrie.addPath("") throws, killing the SyncRequestProcessor
+ // critical thread on the apply path (and unbootable on replay).
+ String quotaPrefix = Quotas.trimQuotaPath(parentName);
+ if (quotaPrefix.isEmpty()) {
+ LOG.warn("Ignoring quota {} node with empty namespace under {}",
+ isLimitNode ? "limit" : "stat", parentName);
+ } else if (isLimitNode) {
+ // this is the limit node
+ // get the parent and add it to the trie
+ pTrie.addPath(quotaPrefix);
+ } else {
+ updateQuotaForPath(quotaPrefix);
+ }
}
}
@@ -588,10 +599,19 @@ public void deleteNode(String path, long zxid) throws NoNodeException {
}
}
- if (parentName.startsWith(procZookeeper) && Quotas.limitNode.equals(childName)) {
- // delete the node in the trie.
- // we need to update the trie as well
- pTrie.deletePath(Quotas.trimQuotaPath(parentName));
+ // Only limit nodes actually under /zookeeper/quota are ever registered
+ // in the path trie (createNode uses the same prefix). Mirror that prefix
+ // here — using the wider /zookeeper prefix let a stray zookeeper_limits
+ // node elsewhere under /zookeeper reach trimQuotaPath and throw
+ // StringIndexOutOfBoundsException on the apply path. The empty-namespace
+ // case (node directly under /zookeeper/quota) is skipped too, since
+ // deletePath("") would likewise throw.
+ if (parentName.startsWith(quotaZookeeper) && Quotas.limitNode.equals(childName)) {
+ String quotaPrefix = Quotas.trimQuotaPath(parentName);
+ if (!quotaPrefix.isEmpty()) {
+ // delete the node in the trie; we need to update the trie as well
+ pTrie.deletePath(quotaPrefix);
+ }
}
// also check to update the quotas for this node
@@ -1261,8 +1281,15 @@ private void traverseNode(String path) {
// get the real node and update
// the count and the bytes
String realPath = path.substring(Quotas.quotaZookeeper.length(), path.indexOf(endString));
- updateQuotaForPath(realPath);
- this.pTrie.addPath(realPath);
+ // A limit node directly under /zookeeper/quota yields an empty
+ // realPath; pTrie.addPath("") throws and aborts startup while
+ // rebuilding the trie from a snapshot. Skip such stray nodes.
+ if (!realPath.isEmpty()) {
+ updateQuotaForPath(realPath);
+ this.pTrie.addPath(realPath);
+ } else {
+ LOG.warn("Ignoring quota limit node with empty namespace: {}", path);
+ }
}
return;
}
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
index 10039afa5..569f4229a 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
@@ -41,6 +41,8 @@
import org.apache.zookeeper.KeeperException.Code;
import org.apache.zookeeper.MultiOperationRecord;
import org.apache.zookeeper.Op;
+import org.apache.zookeeper.Quotas;
+import org.apache.zookeeper.StatsTrack;
import org.apache.zookeeper.ZooDefs;
import org.apache.zookeeper.ZooDefs.OpCode;
import org.apache.zookeeper.common.PathUtils;
@@ -384,6 +386,7 @@ protected void pRequest2Txn(int type, long zxid, Request request, Record record)
validatePath(path, request.sessionId);
nodeRecord = getRecordForPath(path);
zks.checkACL(request.cnxn, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo, path, null);
+ validateQuotaData(path, setDataRequest.getData());
zks.checkQuota(path, nodeRecord.data, setDataRequest.getData(), OpCode.setData);
int newVersion = checkAndIncVersion(nodeRecord.stat.getVersion(), setDataRequest.getVersion(), path);
request.setTxn(new SetDataTxn(path, setDataRequest.getData(), newVersion));
@@ -690,6 +693,7 @@ private void pRequest2TxnCreate(int type, Request request, Record record) throws
throw new KeeperException.NoChildrenForEphemeralsException(path);
}
int newCversion = parentRecord.stat.getCversion() + 1;
+ validateQuotaData(path, data);
zks.checkQuota(path, null, data, OpCode.create);
if (type == OpCode.createContainer) {
request.setTxn(new CreateContainerTxn(path, data, listACL, newCversion));
@@ -734,6 +738,48 @@ private void validatePath(String path, long sessionId) throws BadArgumentsExcept
}
}
+ /**
+ * Reject a create/setData whose target is a quota {@code zookeeper_stats} or
+ * {@code zookeeper_limits} node when either the node is placed directly under
+ * {@code /zookeeper/quota} (an empty namespace) or its data is not well-formed
+ * StatsTrack content. These nodes live under the world-writable
+ * {@code /zookeeper/quota} subtree and are consumed on the transaction-apply
+ * path ({@link org.apache.zookeeper.server.DataTree#updateQuotaStat} and the
+ * path-trie registration); rejecting bad writes here stops them from ever
+ * being committed. The apply path is additionally hardened in
+ * {@link StatsTrack} and {@link DataTree}, so this is defense-in-depth rather
+ * than the sole guard.
+ *
+ * @param path the target node path
+ * @param data the proposed node data
+ * @throws BadArgumentsException if the path is a quota stat/limit node with
+ * an empty namespace or malformed {@code data}
+ */
+ private void validateQuotaData(String path, byte[] data) throws BadArgumentsException {
+ if (path == null || !path.startsWith(Quotas.quotaZookeeper + "/")) {
+ return;
+ }
+ boolean isStatNode = path.endsWith("/" + Quotas.statNode);
+ boolean isLimitNode = path.endsWith("/" + Quotas.limitNode);
+ if (!isStatNode && !isLimitNode) {
+ return;
+ }
+ // A stat/limit node must sit under a non-empty namespace, i.e.
+ // /zookeeper/quota/<ns>/zookeeper_{stats,limits}. Placed directly under
+ // /zookeeper/quota it trims to an empty prefix and crashes the apply
+ // path (PathTrie.addPath("")).
+ String parent = path.substring(0, path.lastIndexOf('/'));
+ if (Quotas.trimQuotaPath(parent).isEmpty()) {
+ LOG.warn("Rejecting quota {} node with empty namespace: {}",
+ isLimitNode ? "limit" : "stat", path);
+ throw new BadArgumentsException(path);
+ }
+ if (!StatsTrack.isValidStatsData(data)) {
+ LOG.warn("Rejecting malformed quota data write to {}", path);
+ throw new BadArgumentsException(path);
+ }
+ }
+
private String getParentPathAndValidate(String path) throws BadArgumentsException {
int lastSlash = path.lastIndexOf('/');
if (lastSlash == -1 || path.indexOf('\0') != -1 || zks.getZKDatabase().isSpecialPath(path)) {
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java
index fc20ed320..6ff202455 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java
@@ -45,6 +45,7 @@
import org.apache.zookeeper.KeeperException.NoNodeException;
import org.apache.zookeeper.KeeperException.NodeExistsException;
import org.apache.zookeeper.Quotas;
+import org.apache.zookeeper.StatsTrack;
import org.apache.zookeeper.ZKTestCase;
import org.apache.zookeeper.ZooDefs;
import org.apache.zookeeper.common.PathTrie;
@@ -687,4 +688,121 @@ private void testSerializeLastProcessedZxid(boolean enableForSerialize, boolean
}
}
+ /**
+ * Regression test for the quota-stats poisoning crash: malformed data
+ * committed to a {@code zookeeper_stats} node must not throw on the
+ * transaction-apply path (updateQuotaStat), which previously killed the
+ * SyncRequestProcessor critical thread and left the dataDir unbootable.
+ * The node must additionally self-heal into a well-formed value.
+ */
+ @Test
+ @Timeout(value = 60)
+ public void testUpdateQuotaStatSurvivesMalformedStatsData() throws Exception {
+ DataTree dt = new DataTree();
+
+ // register /ns as a quota namespace (limit node create adds it to the trie)
+ dt.createNode("/ns", new byte[0], null, -1, 1, 1, 1);
+ dt.createNode(Quotas.quotaPath("/ns"), null, null, -1, 1, 1, 1);
+ dt.createNode(Quotas.limitPath("/ns"),
+ new StatsTrack("count=10,bytes=1000").getStatsBytes(), null, -1, 1, 1, 1);
+ dt.createNode(Quotas.statPath("/ns"),
+ new StatsTrack().getStatsBytes(), null, -1, 1, 1, 1);
+
+ // poison: commit malformed data to the stats node (as an unauthenticated
+ // setData would). This write is not itself under the quota prefix, so it
+ // stores silently without triggering updateQuotaStat.
+ dt.setData(Quotas.statPath("/ns"), "x".getBytes(), 1, 2, 1);
+ assertEquals("x", new String(dt.getNode(Quotas.statPath("/ns")).data));
+
+ // detonating write: an ordinary create under the namespace re-parses the
+ // poisoned stats node in updateQuotaStat. Must NOT throw.
+ dt.createNode("/ns/child", new byte[3], null, -1, 2, 3, 1);
+
+ // the stats node self-healed into a well-formed StatsTrack value
+ byte[] healed = dt.getNode(Quotas.statPath("/ns")).data;
+ assertTrue(StatsTrack.isValidStatsData(healed),
+ "stats node should be well-formed after recovery, was: " + new String(healed));
+
+ // a further write still succeeds (no lingering poison)
+ dt.setData("/ns/child", new byte[5], 1, 4, 1);
+ assertTrue(StatsTrack.isValidStatsData(dt.getNode(Quotas.statPath("/ns")).data));
+ }
+
+ /**
+ * Companion to the above for a null stats payload (jute length -1): the
+ * byte[] StatsTrack constructor must not NPE on the apply/replay path.
+ */
+ @Test
+ @Timeout(value = 60)
+ public void testUpdateQuotaStatSurvivesNullStatsData() throws Exception {
+ DataTree dt = new DataTree();
+ dt.createNode("/ns", new byte[0], null, -1, 1, 1, 1);
+ dt.createNode(Quotas.quotaPath("/ns"), null, null, -1, 1, 1, 1);
+ dt.createNode(Quotas.limitPath("/ns"),
+ new StatsTrack("count=10,bytes=1000").getStatsBytes(), null, -1, 1, 1, 1);
+ dt.createNode(Quotas.statPath("/ns"),
+ new StatsTrack().getStatsBytes(), null, -1, 1, 1, 1);
+
+ // poison the stats node with a null payload
+ dt.setData(Quotas.statPath("/ns"), null, 1, 2, 1);
+ assertNull(dt.getNode(Quotas.statPath("/ns")).data);
+
+ // detonating write must not NPE in StatsTrack(byte[])
+ dt.createNode("/ns/child", new byte[3], null, -1, 2, 3, 1);
+ assertTrue(StatsTrack.isValidStatsData(dt.getNode(Quotas.statPath("/ns")).data));
+ }
+
+ /**
+ * A quota limit/stat node created directly under /zookeeper/quota trims to
+ * an empty namespace; previously PathTrie.addPath("") (and, on reload,
+ * traverseNode) threw and bricked the server. Creating them must now be a
+ * no-op registration rather than a crash.
+ */
+ @Test
+ @Timeout(value = 60)
+ public void testCreateQuotaNodeWithEmptyNamespaceDoesNotCrash() throws Exception {
+ DataTree dt = new DataTree();
+
+ // limit node directly under /zookeeper/quota (empty namespace)
+ dt.createNode(Quotas.quotaZookeeper + "/" + Quotas.limitNode,
+ new StatsTrack("count=10").getStatsBytes(), null, -1, 1, 1, 1);
+ // stat node variant (updateQuotaForPath(""))
+ dt.createNode(Quotas.quotaZookeeper + "/" + Quotas.statNode,
+ new StatsTrack().getStatsBytes(), null, -1, 1, 2, 1);
+
+ // no bogus empty prefix registered -> ordinary writes keep working
+ dt.createNode("/ok", new byte[1], null, -1, 1, 3, 1);
+ assertNotNull(dt.getNode("/ok"));
+
+ // and the stray limit node can be deleted without crashing the trie
+ dt.deleteNode(Quotas.quotaZookeeper + "/" + Quotas.limitNode, 4);
+ assertNull(dt.getNode(Quotas.quotaZookeeper + "/" + Quotas.limitNode));
+ }
+
+ /**
+ * A {@code zookeeper_limits} node created directly under /zookeeper (a
+ * sibling of /zookeeper/quota, not under it) passes the create-side quota
+ * guard silently, but deleting it previously reached
+ * Quotas.trimQuotaPath("/zookeeper") — substring(16) on a 10-char string —
+ * and threw StringIndexOutOfBoundsException on the apply path, permanently
+ * bricking the dataDir. Neither create nor delete may crash. Empty data.
+ */
+ @Test
+ @Timeout(value = 60)
+ public void testLimitNodeDirectlyUnderZookeeperDoesNotCrash() throws Exception {
+ DataTree dt = new DataTree();
+
+ String strayLimit = Quotas.procZookeeper + "/" + Quotas.limitNode; // /zookeeper/zookeeper_limits
+ dt.createNode(strayLimit, new byte[0], null, -1, 1, 1, 1);
+ assertNotNull(dt.getNode(strayLimit));
+
+ // the detonating delete must not throw
+ dt.deleteNode(strayLimit, 2);
+ assertNull(dt.getNode(strayLimit));
+
+ // server still healthy for ordinary writes
+ dt.createNode("/ok", new byte[1], null, -1, 1, 3, 1);
+ assertNotNull(dt.getNode("/ok"));
+ }
+
}
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/QuotasTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/QuotasTest.java
index b887e28fb..79111028f 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/QuotasTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/QuotasTest.java
@@ -46,5 +46,19 @@ public void testQuotaPathPath() {
public void testTrimQuotaPath() {
assertEquals("/foo", Quotas.trimQuotaPath("/zookeeper/quota/foo"));
assertEquals("/bar", Quotas.trimQuotaPath("/zookeeper/quota/bar"));
+ assertEquals("/a/b", Quotas.trimQuotaPath("/zookeeper/quota/a/b"));
+ }
+
+ @Test
+ public void testTrimQuotaPathIsBoundsSafe() {
+ // Paths not under /zookeeper/quota must not throw
+ // StringIndexOutOfBoundsException (they reach this method via the
+ // transaction-apply path and would otherwise kill the server).
+ assertEquals("", Quotas.trimQuotaPath("/zookeeper"));
+ assertEquals("", Quotas.trimQuotaPath("/z"));
+ assertEquals("", Quotas.trimQuotaPath(""));
+ assertEquals("", Quotas.trimQuotaPath("/some/other/path"));
+ // the /zookeeper/quota root itself trims to an empty namespace
+ assertEquals("", Quotas.trimQuotaPath("/zookeeper/quota"));
}
}
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/StatsTrackTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/StatsTrackTest.java
index 978e629da..5b980106d 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/StatsTrackTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/StatsTrackTest.java
@@ -132,4 +132,90 @@ public void testUpwardCompatibility() {
Assert.assertEquals(-1, st.getByteHardLimit());
Assert.assertEquals(-1, st.getCountHardLimit());
}
+
+ // ------------------------------------------------------------------
+ // Hardening against malformed quota-stats data (ZOOKEEPER quota-poison):
+ // a StatsTrack parsed on the transaction-apply path must never throw, or
+ // it kills the SyncRequestProcessor critical thread and, since the trigger
+ // txn is durable, the dataDir becomes unbootable on replay.
+ // ------------------------------------------------------------------
+
+ @Test
+ public void testMalformedValueNoDelimiterDoesNotThrow() {
+ // "x" has no '=' -> previously ArrayIndexOutOfBoundsException at kv[1]
+ StatsTrack st = new StatsTrack("x");
+ Assert.assertEquals(-1, st.getCount());
+ Assert.assertEquals(-1, st.getBytes());
+ }
+
+ @Test
+ public void testMalformedNonNumericValueDoesNotThrow() {
+ // "count=abc" -> previously NumberFormatException
+ StatsTrack st = new StatsTrack("count=abc");
+ Assert.assertEquals(-1, st.getCount());
+ }
+
+ @Test
+ public void testMalformedFromBytesDoesNotThrow() {
+ StatsTrack st = new StatsTrack("x".getBytes());
+ Assert.assertEquals(-1, st.getCount());
+ Assert.assertEquals(-1, st.getBytes());
+ }
+
+ @Test
+ public void testNullBytesDoesNotThrow() {
+ // null payload (jute length -1) must not NPE new String(null, ...)
+ StatsTrack st = new StatsTrack((byte[]) null);
+ Assert.assertEquals(-1, st.getCount());
+ Assert.assertEquals(-1, st.getBytes());
+ }
+
+ @Test
+ public void testPartiallyMalformedKeepsGoodEntries() {
+ // good entries survive, only the bad pair is skipped
+ StatsTrack st = new StatsTrack("count=5,bytes=zzz");
+ Assert.assertEquals(5, st.getCount());
+ Assert.assertEquals(-1, st.getBytes());
+ }
+
+ @Test
+ public void testEmptyAndNullDoNotThrow() {
+ Assert.assertEquals(-1, new StatsTrack("").getCount());
+ Assert.assertEquals(-1, new StatsTrack((String) null).getCount());
+ }
+
+ @Test
+ public void testHardLimitRoundTripStillWorks() {
+ // Guards the compatibility sentinel trailing '=' produced by toString():
+ // parsing must recover every field, including the hard limits.
+ StatsTrack quota = new StatsTrack();
+ quota.setCount(4);
+ quota.setCountHardLimit(4);
+ quota.setBytes(9L);
+ quota.setByteHardLimit(15L);
+ String serialized = quota.toString();
+ Assert.assertEquals("count=4,bytes=9=;byteHardLimit=15;countHardLimit=4", serialized);
+
+ StatsTrack reparsed = new StatsTrack(serialized);
+ Assert.assertEquals(4, reparsed.getCount());
+ Assert.assertEquals(9L, reparsed.getBytes());
+ Assert.assertEquals(15L, reparsed.getByteHardLimit());
+ Assert.assertEquals(4, reparsed.getCountHardLimit());
+ }
+
+ @Test
+ public void testIsValidStatsData() {
+ // well-formed, including the compatibility sentinel format
+ Assert.assertTrue(StatsTrack.isValidStatsData(new byte[0]));
+ Assert.assertTrue(StatsTrack.isValidStatsData("count=5,bytes=10".getBytes()));
+ Assert.assertTrue(StatsTrack.isValidStatsData(
+ "count=4,bytes=9=;byteHardLimit=15;countHardLimit=4".getBytes()));
+
+ // malformed -> rejected at prep time
+ Assert.assertFalse(StatsTrack.isValidStatsData(null));
+ Assert.assertFalse(StatsTrack.isValidStatsData("x".getBytes()));
+ Assert.assertFalse(StatsTrack.isValidStatsData("count=abc".getBytes()));
+ Assert.assertFalse(StatsTrack.isValidStatsData("=5".getBytes()));
+ Assert.assertFalse(StatsTrack.isValidStatsData("count=5,x".getBytes()));
+ }
}
(zookeeper) 02/05: setACL race — missing acl attribute setting
Posted by an...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
anmolnar pushed a commit to branch branch-3.9
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
commit 184738dc49d9ae70f1405f06c69a84b6686f1c7f
Author: Andor Molnar <an...@cloudera.com>
AuthorDate: Fri Sep 18 10:49:13 2026 -0500
setACL race — missing acl attribute setting
---
.../zookeeper/server/PrepRequestProcessor.java | 8 +++
.../zookeeper/server/PrepRequestProcessorTest.java | 65 ++++++++++++++++++++++
2 files changed, 73 insertions(+)
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
index 8404ed9b8..10039afa5 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
@@ -558,6 +558,14 @@ protected void pRequest2Txn(int type, long zxid, Request request, Record record)
request.setTxn(new SetACLTxn(path, listACL, newVersion));
nodeRecord = nodeRecord.duplicate(request.getHdr().getZxid());
nodeRecord.stat.setAversion(newVersion);
+ // Publish the new ACL onto the outstanding ChangeRecord. getRecordForPath()
+ // serves this record (in preference to the committed tree) to every request
+ // prepped before this setACL commits, so without this line those requests are
+ // authorized against the stale, pre-revocation ACL and their writes linearize
+ // after the revocation (TOCTOU). Mirrors the create path, which already sets
+ // the ACL on its ChangeRecord. ACL is not part of the node digest, so this
+ // does not affect digest calculation.
+ nodeRecord.acl = listACL;
nodeRecord.precalculatedDigest = precalculateDigest(
DigestOpCode.UPDATE, path, nodeRecord.data, nodeRecord.stat);
setTxnDigest(request, nodeRecord.precalculatedDigest);
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/PrepRequestProcessorTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/PrepRequestProcessorTest.java
index db84bf810..d17178dab 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/PrepRequestProcessorTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/PrepRequestProcessorTest.java
@@ -48,6 +48,7 @@
import org.apache.zookeeper.proto.CreateRequest;
import org.apache.zookeeper.proto.ReconfigRequest;
import org.apache.zookeeper.proto.RequestHeader;
+import org.apache.zookeeper.proto.SetACLRequest;
import org.apache.zookeeper.proto.SetDataRequest;
import org.apache.zookeeper.server.ZooKeeperServer.ChangeRecord;
import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
@@ -290,6 +291,70 @@ public void testInvalidPath() throws Exception {
assertEquals(outcome.getException().code(), KeeperException.Code.BADARGUMENTS);
}
+ /**
+ * A setACL that is prepped but not yet committed must publish the NEW ACL onto
+ * its outstanding ChangeRecord. getRecordForPath() serves that record to every
+ * request prepped before the setACL commits, so if it still carries the old ACL
+ * those requests are authorized against the pre-revocation ACL (TOCTOU).
+ *
+ * This is the direct regression test for the missing
+ * {@code nodeRecord.acl = listACL;} in the setACL case: before the fix the
+ * outstanding record keeps the old OPEN_ACL_UNSAFE; after the fix it carries the
+ * new read-only ACL.
+ */
+ @Test
+ public void testSetACLPublishesNewAclOnOutstandingChangeRecord() throws Exception {
+ zks.getZKDatabase().dataTree.createNode("/foo", new byte[0], Ids.OPEN_ACL_UNSAFE, 0, 0, 0, 0);
+ assertNull(zks.outstandingChangesForPath.get("/foo"));
+
+ pLatch = new CountDownLatch(1);
+ processor = new PrepRequestProcessor(zks, new MyRequestProcessor());
+ SetACLRequest setAcl = new SetACLRequest("/foo", Ids.READ_ACL_UNSAFE, -1);
+ // admin identity so the ADMIN permission check is not what is under test here
+ processor.pRequest(createRequest(setAcl, OpCode.setACL, true));
+ assertTrue(pLatch.await(5, TimeUnit.SECONDS), "request hasn't been processed in chain");
+
+ ChangeRecord cr = zks.outstandingChangesForPath.get("/foo");
+ assertNotNull(cr, "Change record wasn't set");
+ assertEquals(Ids.READ_ACL_UNSAFE, cr.acl,
+ "Outstanding ChangeRecord must carry the new ACL so later preps are checked against it");
+ }
+
+ /**
+ * End-to-end (at the prep layer) version of the above: a client's write that is
+ * prepped while an ACL revocation is still outstanding must be checked against the
+ * NEW ACL and denied. Before the fix the write is authorized against the stale ACL
+ * and would commit after the revocation.
+ */
+ @Test
+ public void testRacingWriteAfterAclRevocationIsDenied() throws Exception {
+ // Node is world-writable to start with (OPEN_ACL_UNSAFE grants ALL to world:anyone).
+ zks.getZKDatabase().dataTree.createNode("/foo", new byte[0], Ids.OPEN_ACL_UNSAFE, 0, 0, 0, 0);
+
+ processor = new PrepRequestProcessor(zks, new MyRequestProcessor());
+
+ // 1. A world client revokes write access: setACL -> read-only. This is prepped and
+ // left in outstandingChangesForPath (not yet committed). OPEN_ACL_UNSAFE grants
+ // ADMIN, so the world client is allowed to perform the setACL.
+ pLatch = new CountDownLatch(1);
+ SetACLRequest setAcl = new SetACLRequest("/foo", Ids.READ_ACL_UNSAFE, -1);
+ processor.pRequest(createRequest(setAcl, OpCode.setACL, false));
+ assertTrue(pLatch.await(5, TimeUnit.SECONDS), "setACL hasn't been processed in chain");
+ assertNull(outcome.getException(), "setACL revocation should succeed");
+
+ // 2. The same world client now tries to write while the revocation is still
+ // outstanding. It must be checked against the new read-only ACL and denied.
+ pLatch = new CountDownLatch(1);
+ SetDataRequest setData = new SetDataRequest("/foo", "evil".getBytes(), -1);
+ processor.pRequest(createRequest(setData, OpCode.setData, false));
+ assertTrue(pLatch.await(5, TimeUnit.SECONDS), "setData hasn't been processed in chain");
+
+ assertEquals(OpCode.error, outcome.getHdr().getType(),
+ "write racing an outstanding ACL revocation must fail");
+ assertEquals(KeeperException.Code.NOAUTH, outcome.getException().code(),
+ "write racing an outstanding ACL revocation must be denied against the new ACL");
+ }
+
private class MyRequestProcessor implements RequestProcessor {
@Override
(zookeeper) 05/05: Check valid voting member before applying (re)config during LE
Posted by an...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
anmolnar pushed a commit to branch branch-3.9
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
commit 509ffd01e2804c3a43a8828b5c7e2df051e97079
Author: Andor Molnar <an...@apache.org>
AuthorDate: Wed Sep 30 10:08:35 2026 -0500
Check valid voting member before applying (re)config during LE
---
.../server/quorum/FastLeaderElection.java | 60 +++++---
.../server/quorum/FLEConfigFromNonVoterTest.java | 151 +++++++++++++++++++++
2 files changed, 189 insertions(+), 22 deletions(-)
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FastLeaderElection.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FastLeaderElection.java
index cee02972e..e0a79ab53 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FastLeaderElection.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FastLeaderElection.java
@@ -301,31 +301,47 @@ public void run() {
byte[] b = new byte[configLength];
response.buffer.get(b);
- synchronized (self) {
- try {
- rqv = self.configFromString(new String(b, UTF_8));
- QuorumVerifier curQV = self.getQuorumVerifier();
- if (rqv.getVersion() > curQV.getVersion()) {
- LOG.info("{} Received version: {} my version: {}",
- self.getMyId(),
- Long.toHexString(rqv.getVersion()),
- Long.toHexString(self.getQuorumVerifier().getVersion()));
- if (self.getPeerState() == ServerState.LOOKING) {
- LOG.debug("Invoking processReconfig(), state: {}", self.getServerState());
- self.processReconfig(rqv, null, null, false);
- if (!rqv.equals(curQV)) {
- LOG.info("restarting leader election");
- self.shuttingDownLE = true;
- self.getElectionAlg().shutdown();
-
- break;
+ /*
+ * Only adopt a QuorumVerifier carried in a notification if
+ * the sender is a voting member of our current (or next)
+ * configuration. The election port accepts connections from
+ * any sid, so without this gate an unauthenticated peer could
+ * push an arbitrary config into processReconfig(), which
+ * persists it to the dynamic config file and restarts
+ * leader election. Non-voters (observers, joining servers)
+ * still get the reply notification below, which carries our
+ * own config, so they can learn the current membership.
+ */
+ if (!validVoter(response.sid)) {
+ LOG.info("Ignoring config section in notification from non-voter sid={} (config length: {})",
+ response.sid, configLength);
+ } else {
+ synchronized (self) {
+ try {
+ rqv = self.configFromString(new String(b, UTF_8));
+ QuorumVerifier curQV = self.getQuorumVerifier();
+ if (rqv.getVersion() > curQV.getVersion()) {
+ LOG.info("{} Received version: {} my version: {}",
+ self.getMyId(),
+ Long.toHexString(rqv.getVersion()),
+ Long.toHexString(self.getQuorumVerifier().getVersion()));
+ if (self.getPeerState() == ServerState.LOOKING) {
+ LOG.debug("Invoking processReconfig(), state: {}", self.getServerState());
+ self.processReconfig(rqv, null, null, false);
+ if (!rqv.equals(curQV)) {
+ LOG.info("restarting leader election");
+ self.shuttingDownLE = true;
+ self.getElectionAlg().shutdown();
+
+ break;
+ }
+ } else {
+ LOG.debug("Skip processReconfig(), state: {}", self.getServerState());
}
- } else {
- LOG.debug("Skip processReconfig(), state: {}", self.getServerState());
}
+ } catch (IOException | ConfigException e) {
+ LOG.error("Something went wrong while processing config received from {}", response.sid);
}
- } catch (IOException | ConfigException e) {
- LOG.error("Something went wrong while processing config received from {}", response.sid);
}
}
} else {
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/FLEConfigFromNonVoterTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/FLEConfigFromNonVoterTest.java
new file mode 100644
index 000000000..67ab4b083
--- /dev/null
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/FLEConfigFromNonVoterTest.java
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.io.File;
+import java.net.InetSocketAddress;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.server.quorum.QuorumPeer.QuorumServer;
+import org.apache.zookeeper.server.quorum.QuorumPeer.ServerState;
+import org.apache.zookeeper.server.quorum.flexible.QuorumVerifier;
+import org.apache.zookeeper.test.ClientBase;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A notification received on the election port may carry a QuorumVerifier
+ * (version >= 0x2). The election port accepts a connection from any sid, so
+ * the config section must only be adopted when the sender is a voting member
+ * of the receiver's current configuration. Otherwise an unauthenticated peer
+ * could inject an arbitrary quorum configuration into a LOOKING server.
+ */
+public class FLEConfigFromNonVoterTest extends ZKTestCase {
+
+ protected static final Logger LOG = LoggerFactory.getLogger(FLEConfigFromNonVoterTest.class);
+
+ private static final int COUNT = 3;
+ private static final long ROGUE_SID = 99L;
+
+ private Map<Long, QuorumServer> peers;
+ private File[] tmpdir;
+ private int[] port;
+ private QuorumPeer victim;
+ private QuorumCnxManager[] cnxManagers;
+ private boolean savedReconfigEnabled;
+
+ @BeforeEach
+ public void setUp() throws Exception {
+ savedReconfigEnabled = QuorumPeerConfig.isReconfigEnabled();
+ QuorumPeerConfig.setReconfigEnabled(true);
+
+ peers = new HashMap<>();
+ tmpdir = new File[COUNT + 1];
+ port = new int[COUNT + 1];
+ cnxManagers = new QuorumCnxManager[2];
+
+ for (int i = 0; i < COUNT; i++) {
+ int clientport = PortAssignment.unique();
+ peers.put((long) i, new QuorumServer(i,
+ new InetSocketAddress("127.0.0.1", PortAssignment.unique()),
+ new InetSocketAddress("127.0.0.1", PortAssignment.unique()),
+ new InetSocketAddress("127.0.0.1", clientport)));
+ tmpdir[i] = ClientBase.createTmpDir();
+ port[i] = clientport;
+ }
+ tmpdir[COUNT] = ClientBase.createTmpDir();
+ port[COUNT] = PortAssignment.unique();
+ }
+
+ @AfterEach
+ public void tearDown() throws Exception {
+ for (QuorumCnxManager m : cnxManagers) {
+ if (m != null) {
+ m.halt();
+ }
+ }
+ if (victim != null) {
+ victim.shutdown();
+ }
+ QuorumPeerConfig.setReconfigEnabled(savedReconfigEnabled);
+ }
+
+ @Test
+ public void testConfigFromNonVoterIsIgnored() throws Exception {
+ victim = new QuorumPeer(peers, tmpdir[0], tmpdir[0], port[0], 3, 0, 1000, 2, 2, 2);
+ victim.startLeaderElection();
+ QuorumVerifier originalQV = victim.getQuorumVerifier();
+ long originalVersion = originalQV.getVersion();
+
+ FLETestUtils.LEThread thread = new FLETestUtils.LEThread(victim, 0);
+ thread.start();
+
+ // Rogue peer: not a member of the victim's view, but knows the
+ // victim's election address. Its sid is larger than the victim's so
+ // the victim's QuorumCnxManager keeps the inbound connection.
+ Map<Long, QuorumServer> rogueView = new HashMap<>(peers);
+ rogueView.put(ROGUE_SID, new QuorumServer(ROGUE_SID,
+ new InetSocketAddress("127.0.0.1", PortAssignment.unique()),
+ new InetSocketAddress("127.0.0.1", PortAssignment.unique()),
+ new InetSocketAddress("127.0.0.1", port[COUNT])));
+ QuorumPeer rogue = new QuorumPeer(rogueView, tmpdir[COUNT], tmpdir[COUNT], port[COUNT], 3, ROGUE_SID, 1000, 2, 2, 2);
+ cnxManagers[0] = rogue.createCnxnManager();
+ cnxManagers[0].listener.start();
+
+ String injected = originalQV.toString().replaceAll("version=.*", "")
+ + "server." + ROGUE_SID + "=127.0.0.1:12345:12346:participant;12347\n"
+ + "version=" + Long.toHexString(originalVersion + 1);
+ cnxManagers[0].toSend(0L, FastLeaderElection.buildMsg(
+ ServerState.LOOKING.ordinal(), 0, 0, 1, 1, injected.getBytes(UTF_8)));
+
+ // Give the victim's WorkerReceiver time to process the message.
+ Thread.sleep(2000);
+
+ assertEquals(originalVersion, victim.getQuorumVerifier().getVersion(),
+ "config from non-voter must not be adopted");
+ assertFalse(victim.getQuorumVerifier().getAllMembers().containsKey(ROGUE_SID),
+ "non-voter must not be able to add itself to the quorum");
+ assertTrue(thread.isAlive(), "leader election must not have been restarted by a non-voter");
+
+ // Positive control: the same config from a valid voter (sid 1) is adopted,
+ // which proves the delivery path in this test actually works.
+ QuorumPeer voter = new QuorumPeer(peers, tmpdir[1], tmpdir[1], port[1], 3, 1, 1000, 2, 2, 2);
+ cnxManagers[1] = voter.createCnxnManager();
+ cnxManagers[1].listener.start();
+ cnxManagers[1].toSend(0L, FastLeaderElection.buildMsg(
+ ServerState.LOOKING.ordinal(), 0, 0, 1, 1, injected.getBytes(UTF_8)));
+
+ long deadline = System.currentTimeMillis() + 10000;
+ while (victim.getQuorumVerifier().getVersion() == originalVersion && System.currentTimeMillis() < deadline) {
+ Thread.sleep(100);
+ }
+ assertEquals(originalVersion + 1, victim.getQuorumVerifier().getVersion(),
+ "config from a valid voter should still be adopted");
+ }
+
+}
(zookeeper) 04/05: Add multiRead response throttling to guard resource allocation
Posted by an...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
anmolnar pushed a commit to branch branch-3.9
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
commit 1f169e149f78fe8fcfa31b7db0cf9b625cdb978d
Author: Andor Molnar <an...@apache.org>
AuthorDate: Wed Sep 30 10:40:32 2026 -0500
Add multiRead response throttling to guard resource allocation
---
.../src/main/resources/markdown/zookeeperAdmin.md | 24 +++++
.../zookeeper/server/FinalRequestProcessor.java | 49 ++++++++-
.../apache/zookeeper/server/ZooKeeperServer.java | 45 ++++++++
.../apache/zookeeper/test/MultiOperationTest.java | 115 +++++++++++++++++++++
4 files changed, 231 insertions(+), 2 deletions(-)
diff --git a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
index e122bde3d..c53bc0a4d 100644
--- a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
+++ b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
@@ -1156,6 +1156,30 @@ property, when available, is noted below.
**New in 3.6.0:**
The size threshold after which a request is considered a large request. If it is -1, then all requests are considered small, effectively turning off large request throttling. The default is -1.
+* *multiRead.maxOps* :
+ (Java system property: **zookeeper.multiRead.maxOps**)
+ **New in 3.9.7:**
+ The maximum number of read operations (getData / getChildren) permitted in a single
+ multiRead request. A multiRead can amplify a request that is small on the wire into a
+ very large in-memory response, because every sub-operation result is materialized and
+ held in memory before the response is serialized. This limit, together with
+ *multiRead.maxResponseBytes*, bounds that amplification. A multiRead exceeding this
+ count is rejected with a BadArguments (Code = BADARGUMENTS) error and is not executed.
+ Note that neither *jute.maxbuffer* nor *largeRequestMaxBytes* protects against this,
+ as both bound only the inbound request, not the response it generates. Set to 0 (or a
+ negative value) to disable the check. The default is 1000.
+
+* *multiRead.maxResponseBytes* :
+ (Java system property: **zookeeper.multiRead.maxResponseBytes**)
+ **New in 3.9.7:**
+ The maximum cumulative size, in bytes, of the data materialized while serving a single
+ multiRead request. This is the primary guard against a multiRead response-amplification
+ denial of service. The server accumulates the size of each sub-operation result as the
+ request is processed and rejects the request with a BadArguments (Code = BADARGUMENTS)
+ error as soon as the running total exceeds this value, before the full response is built
+ in heap. Set to 0 (or a negative value) to disable the check. The default is 67108864
+ (64 * 1024 * 1024, i.e. 64 MB).
+
* *outstandingHandshake.limit*
(Java system property only: **zookeeper.netty.server.outstandingHandshake.limit**)
The maximum in-flight TLS handshake connections could have in ZooKeeper,
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java
index 911583f9f..cd360c231 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java
@@ -270,20 +270,41 @@ public void processRequest(Request request) {
case OpCode.multiRead: {
lastOp = "MLTR";
MultiOperationRecord multiReadRecord = request.readRequestRecord(MultiOperationRecord::new);
+
+ // Guard against multiRead response-amplification DoS. A multiRead request
+ // can be tiny on the wire (and thus pass jute.maxbuffer and the large-request
+ // throttle, which only bound the inbound request) yet force the server to
+ // materialize an unbounded response in heap: every sub-op result is copied and
+ // held in the MultiResponse before serialization. Cap the operation count and
+ // the cumulative response size so the response stays bounded.
+ final int maxOps = zks.getMultiReadMaxOps();
+ if (maxOps > 0 && multiReadRecord.size() > maxOps) {
+ throw new KeeperException.BadArgumentsException(
+ "multiRead operation count " + multiReadRecord.size()
+ + " exceeds the configured maximum of " + maxOps);
+ }
+ final long maxResponseBytes = zks.getMultiReadMaxResponseBytes();
+ long responseBytes = 0;
+
rsp = new MultiResponse();
OpResult subResult;
for (Op readOp : multiReadRecord) {
+ long opBytes = 0;
try {
Record rec;
switch (readOp.getType()) {
case OpCode.getChildren:
rec = handleGetChildrenRequest(readOp.toRequestRecord(), cnxn, request.authInfo);
- subResult = new GetChildrenResult(((GetChildrenResponse) rec).getChildren());
+ List<String> children = ((GetChildrenResponse) rec).getChildren();
+ opBytes = childrenSizeInBytes(children);
+ subResult = new GetChildrenResult(children);
break;
case OpCode.getData:
rec = handleGetDataRequest(readOp.toRequestRecord(), cnxn, request.authInfo);
GetDataResponse gdr = (GetDataResponse) rec;
- subResult = new GetDataResult(gdr.getData(), gdr.getStat());
+ byte[] data = gdr.getData();
+ opBytes = data == null ? 0 : data.length;
+ subResult = new GetDataResult(data, gdr.getStat());
break;
default:
throw new IOException("Invalid type of readOp");
@@ -291,6 +312,14 @@ public void processRequest(Request request) {
} catch (KeeperException e) {
subResult = new ErrorResult(e.code().intValue());
}
+ if (maxResponseBytes > 0) {
+ responseBytes += opBytes;
+ if (responseBytes > maxResponseBytes) {
+ throw new KeeperException.BadArgumentsException(
+ "multiRead response size exceeds the configured maximum of "
+ + maxResponseBytes + " bytes");
+ }
+ }
((MultiResponse) rsp).add(subResult);
}
break;
@@ -626,6 +655,22 @@ public void processRequest(Request request) {
}
}
+ /**
+ * Estimates the serialized size of a getChildren result so that multiRead can bound
+ * its cumulative response size. Each child name is counted as its length in bytes
+ * (chars are a safe lower bound) plus the 4-byte length prefix jute writes per string.
+ */
+ private static long childrenSizeInBytes(List<String> children) {
+ if (children == null) {
+ return 0;
+ }
+ long bytes = 0;
+ for (String child : children) {
+ bytes += 4L + (child == null ? 0 : child.length());
+ }
+ return bytes;
+ }
+
private Record handleGetChildrenRequest(Record request, ServerCnxn cnxn, List<Id> authInfo) throws KeeperException, IOException {
GetChildrenRequest getChildrenRequest = (GetChildrenRequest) request;
String path = getChildrenRequest.getPath();
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
index f9a60da1f..7395509d8 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
@@ -327,6 +327,25 @@ protected enum State {
private final AtomicInteger currentLargeRequestBytes = new AtomicInteger(0);
+ /**
+ * Maximum number of read operations allowed in a single multiRead request.
+ * A multiRead can amplify a tiny request into a very large in-memory response
+ * (each sub-op result is fully materialized and held in the MultiResponse before
+ * serialization), so this bounds the operation fan-out. A value of 0 or less
+ * disables the check.
+ */
+ private volatile int multiReadMaxOps = 1000;
+
+ /**
+ * Maximum cumulative size, in bytes, of the data materialized while serving a
+ * single multiRead request. This is the primary guard against a response-
+ * amplification denial of service: without it, a request that is small on the wire
+ * (and therefore not caught by jute.maxbuffer or the large-request throttle, both
+ * of which only bound the inbound request) can force the server to allocate
+ * arbitrarily large amounts of heap. A value of 0 or less disables the check.
+ */
+ private volatile long multiReadMaxResponseBytes = 64L * 1024 * 1024;
+
private final AuthenticationHelper authHelper = new AuthenticationHelper();
void removeCnxn(ServerCnxn cnxn) {
@@ -384,6 +403,8 @@ public ZooKeeperServer(FileTxnSnapLog txnLogFactory, int tickTime, int minSessio
this.initLargeRequestThrottlingSettings();
+ this.initMultiReadThrottlingSettings();
+
LOG.info(
"Created server with"
+ " tickTime {} ms"
@@ -1609,6 +1630,30 @@ private void initLargeRequestThrottlingSettings() {
setLargeRequestThreshold(Integer.getInteger("zookeeper.largeRequestThreshold", -1));
}
+ private void initMultiReadThrottlingSettings() {
+ multiReadMaxOps = Integer.getInteger("zookeeper.multiRead.maxOps", multiReadMaxOps);
+ multiReadMaxResponseBytes = Long.getLong("zookeeper.multiRead.maxResponseBytes", multiReadMaxResponseBytes);
+ LOG.info("multiRead limits: maxOps={}, maxResponseBytes={}", multiReadMaxOps, multiReadMaxResponseBytes);
+ }
+
+ public int getMultiReadMaxOps() {
+ return multiReadMaxOps;
+ }
+
+ public void setMultiReadMaxOps(int maxOps) {
+ this.multiReadMaxOps = maxOps;
+ LOG.info("multiRead maxOps set to {}", maxOps);
+ }
+
+ public long getMultiReadMaxResponseBytes() {
+ return multiReadMaxResponseBytes;
+ }
+
+ public void setMultiReadMaxResponseBytes(long maxBytes) {
+ this.multiReadMaxResponseBytes = maxBytes;
+ LOG.info("multiRead maxResponseBytes set to {}", maxBytes);
+ }
+
public int getLargeRequestMaxBytes() {
return largeRequestMaxBytes;
}
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java
index fd7b8a02f..31c3f04db 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java
@@ -25,6 +25,7 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
+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.ArrayList;
@@ -60,7 +61,9 @@
import org.apache.zookeeper.data.Id;
import org.apache.zookeeper.data.Stat;
import org.apache.zookeeper.server.SyncRequestProcessor;
+import org.apache.zookeeper.server.ZooKeeperServer;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
@@ -960,6 +963,118 @@ public void testMultiRead(boolean useAsync) throws Exception {
assertEquals(0, stat.getNumChildren());
}
+ /**
+ * A multiRead whose operation count exceeds multiRead.maxOps must be rejected
+ * outright (BADARGUMENTS) rather than executed. This is the operation-count
+ * dimension of the multiRead response-amplification DoS guard.
+ */
+ @Test
+ public void testMultiReadOpCountLimitRejected() throws Exception {
+ ZooKeeperServer zks = serverFactory.getZooKeeperServer();
+ zks.setMultiReadMaxOps(5);
+
+ zk.create("/rlimit", "data".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+
+ List<Op> ops = new ArrayList<>();
+ for (int i = 0; i < 6; i++) {
+ ops.add(Op.getData("/rlimit"));
+ }
+ assertThrows(KeeperException.BadArgumentsException.class, () -> multi(zk, ops, false));
+ }
+
+ /**
+ * A multiRead at exactly multiRead.maxOps operations is allowed.
+ */
+ @Test
+ public void testMultiReadOpCountLimitAtBoundaryAllowed() throws Exception {
+ ZooKeeperServer zks = serverFactory.getZooKeeperServer();
+ zks.setMultiReadMaxOps(5);
+
+ zk.create("/rlimit", "data".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+
+ List<Op> ops = new ArrayList<>();
+ for (int i = 0; i < 5; i++) {
+ ops.add(Op.getData("/rlimit"));
+ }
+ List<OpResult> results = multi(zk, ops, false);
+ assertEquals(5, results.size());
+ }
+
+ /**
+ * Setting both limits to 0 disables the guards and restores the historical
+ * (unbounded) behaviour, confirming the checks are the only thing gating it.
+ */
+ @Test
+ public void testMultiReadLimitsDisabled() throws Exception {
+ ZooKeeperServer zks = serverFactory.getZooKeeperServer();
+ zks.setMultiReadMaxOps(0);
+ zks.setMultiReadMaxResponseBytes(0);
+
+ zk.create("/rlimit", "data".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+
+ List<Op> ops = new ArrayList<>();
+ for (int i = 0; i < 2000; i++) { // well over the default cap of 1000
+ ops.add(Op.getData("/rlimit"));
+ }
+ List<OpResult> results = multi(zk, ops, false);
+ assertEquals(2000, results.size());
+ }
+
+ /**
+ * A multiRead with a small operation count whose cumulative response size
+ * exceeds multiRead.maxResponseBytes must be rejected. This is the byte
+ * dimension of the guard and is the shape of the reported DoS (a few ops that
+ * each pull a large znode).
+ */
+ @Test
+ public void testMultiReadResponseByteLimitRejected() throws Exception {
+ ZooKeeperServer zks = serverFactory.getZooKeeperServer();
+ zks.setMultiReadMaxOps(0); // isolate the byte cap
+ zks.setMultiReadMaxResponseBytes(100_000);
+
+ byte[] data = new byte[40_000];
+ Arrays.fill(data, (byte) 'x');
+ zk.create("/big", data, Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+
+ // 3 * 40_000 = 120_000 > 100_000 -> rejected while processing the third op
+ List<Op> ops = Arrays.asList(Op.getData("/big"), Op.getData("/big"), Op.getData("/big"));
+ assertThrows(KeeperException.BadArgumentsException.class, () -> multi(zk, ops, false));
+ }
+
+ /**
+ * A multiRead whose cumulative response size stays under
+ * multiRead.maxResponseBytes is served normally.
+ */
+ @Test
+ public void testMultiReadResponseByteLimitUnderBudgetAllowed() throws Exception {
+ ZooKeeperServer zks = serverFactory.getZooKeeperServer();
+ zks.setMultiReadMaxOps(0);
+ zks.setMultiReadMaxResponseBytes(100_000);
+
+ byte[] data = new byte[40_000];
+ Arrays.fill(data, (byte) 'x');
+ zk.create("/big", data, Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+
+ // 2 * 40_000 = 80_000 < 100_000 -> allowed
+ List<Op> ops = Arrays.asList(Op.getData("/big"), Op.getData("/big"));
+ List<OpResult> results = multi(zk, ops, false);
+ assertEquals(2, results.size());
+ assertArrayEquals(data, ((OpResult.GetDataResult) results.get(0)).getData());
+ }
+
+ /**
+ * A normal, small multiRead must not be affected by the new guards under
+ * their default limits (no false positives).
+ */
+ @Test
+ public void testMultiReadWithinDefaultLimitsSucceeds() throws Exception {
+ zk.create("/n1", "d1".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ zk.create("/n2", "d2".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ List<OpResult> results = multi(zk, Arrays.asList(
+ Op.getData("/n1"), Op.getChildren("/n1"), Op.getData("/n2")), false);
+ assertEquals(3, results.size());
+ }
+
@ParameterizedTest
@ValueSource(booleans = {true, false})
public void testMixedReadAndTransaction(boolean useAsync) throws Exception {
(zookeeper) 01/05: SASL authzid: add extra validation on client port to mirror quorum check
Posted by an...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
anmolnar pushed a commit to branch branch-3.9
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
commit 0afc92745ce5249e8aff511bab6d2208a72e45fb
Author: Andor Molnar <an...@cloudera.com>
AuthorDate: Thu Sep 17 14:56:06 2026 -0500
SASL authzid: add extra validation on client port to mirror quorum check
---
.../server/auth/SaslServerCallbackHandler.java | 36 ++-
.../server/auth/SaslServerCallbackHandlerTest.java | 250 +++++++++++++++++++++
2 files changed, 282 insertions(+), 4 deletions(-)
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java
index 79c57e462..5ab7f70fe 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java
@@ -27,6 +27,7 @@
import javax.security.auth.callback.UnsupportedCallbackException;
import javax.security.sasl.AuthorizeCallback;
import javax.security.sasl.RealmCallback;
+import org.apache.zookeeper.common.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -61,7 +62,8 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
private void handleNameCallback(NameCallback nc) {
// check to see if this user is in the user password database.
if (credentials.get(nc.getDefaultName()) == null) {
- LOG.warn("User '{}' not found in list of DIGEST-MD5 authenticateable users.", nc.getDefaultName());
+ LOG.warn("User '{}' not found in list of DIGEST-MD5 authenticateable users.",
+ StringUtils.sanitizeForLog(nc.getDefaultName()));
return;
}
nc.setName(nc.getDefaultName());
@@ -80,7 +82,7 @@ private void handlePasswordCallback(PasswordCallback pc) {
}
private void handleRealmCallback(RealmCallback rc) {
- LOG.debug("client supplied realm: {}", rc.getDefaultText());
+ LOG.debug("client supplied realm: {}", StringUtils.sanitizeForLog(rc.getDefaultText()));
rc.setText(rc.getDefaultText());
}
@@ -88,8 +90,34 @@ private void handleAuthorizeCallback(AuthorizeCallback ac) {
String authenticationID = ac.getAuthenticationID();
String authorizationID = ac.getAuthorizationID();
+ // A client must not be allowed to authorize as an identity other than the
+ // one it authenticated as. The SASL authorizationID (authzid) is an
+ // attacker-controlled field of the client token. If it differs from the
+ // authenticated identity we must reject it here: otherwise, whenever the
+ // identity canonicalization below fails -- which happens for any principal
+ // not covered by zookeeper.security.auth_to_local rules, i.e. every
+ // cross-realm principal under the default DEFAULT rule -- the JDK SASL
+ // server would fall back to returning the client-requested authzid as the
+ // negotiated identity (AuthorizeCallback.getAuthorizedID()), letting a
+ // low-privilege client assume any identity (e.g. "super"). This mirrors the
+ // equality check in SaslQuorumServerCallbackHandler and
+ // SaslClientCallbackHandler.
+ if (authenticationID == null || !authenticationID.equals(authorizationID)) {
+ // authenticationID and authorizationID are unvalidated, client-controlled
+ // strings from the SASL token; sanitize before logging to prevent log
+ // injection (CWE-117). WARN, not ERROR: a rejected client is not an
+ // operator-actionable server fault, and ERROR should stay meaningful for
+ // alerting since a client can repeat failed attempts at will.
+ LOG.warn("Client attempted to authorize as a different identity: "
+ + "authenticationID={}; requested authorizationID={}. Denying authorization.",
+ StringUtils.sanitizeForLog(authenticationID),
+ StringUtils.sanitizeForLog(authorizationID));
+ ac.setAuthorized(false);
+ return;
+ }
+
LOG.info("Successfully authenticated client: authenticationID={}; authorizationID={}.",
- authenticationID, authorizationID);
+ StringUtils.sanitizeForLog(authenticationID), StringUtils.sanitizeForLog(authorizationID));
ac.setAuthorized(true);
// canonicalize authorization id according to system properties:
@@ -104,7 +132,7 @@ private void handleAuthorizeCallback(AuthorizeCallback ac) {
if (shouldAppendRealm(kerberosName)) {
userNameBuilder.append("@").append(kerberosName.getRealm());
}
- LOG.info("Setting authorizedID: {}", userNameBuilder);
+ LOG.info("Setting authorizedID: {}", StringUtils.sanitizeForLog(userNameBuilder.toString()));
ac.setAuthorizedID(userNameBuilder.toString());
} catch (IOException e) {
LOG.error("Failed to set name based on Kerberos authentication rules.", e);
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandlerTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandlerTest.java
new file mode 100644
index 000000000..d303fc035
--- /dev/null
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandlerTest.java
@@ -0,0 +1,250 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.auth;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import ch.qos.logback.classic.Level;
+import java.util.Collections;
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.NameCallback;
+import javax.security.sasl.AuthorizeCallback;
+import org.apache.zookeeper.test.LoggerTestTool;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Unit tests for {@link SaslServerCallbackHandler} authorization handling.
+ *
+ * <p>These reproduce the client-port SASL authorization-ID (authzid) impersonation
+ * issue and lock in the fix. The handler runs the same code path a real DIGEST-MD5
+ * or GSSAPI negotiation drives via {@link AuthorizeCallback}; the SASL server
+ * mechanism later returns {@code AuthorizeCallback.getAuthorizedID()} as the
+ * connection identity, so asserting on it is equivalent to asserting on the
+ * identity ZooKeeperServer.processSasl would adopt.
+ *
+ * <p>No server, KDC, or network is required.
+ */
+public class SaslServerCallbackHandlerTest {
+
+ private static LoggerTestTool loggerTestTool;
+
+ @BeforeAll
+ public static void setupBeforeClass() {
+ loggerTestTool = new LoggerTestTool(SaslServerCallbackHandler.class, Level.INFO);
+ }
+
+ @AfterAll
+ public static void tearDownAfterClass() throws Exception {
+ loggerTestTool.close();
+ }
+
+ @BeforeEach
+ public void resetLog() {
+ // discard output from earlier tests so each test sees only its own log lines
+ loggerTestTool.getOutputStream().reset();
+ }
+
+ private static SaslServerCallbackHandler newHandler() {
+ return new SaslServerCallbackHandler(Collections.emptyMap());
+ }
+
+ private static AuthorizeCallback authorize(String authnId, String authzId) throws Exception {
+ SaslServerCallbackHandler handler = newHandler();
+ AuthorizeCallback ac = new AuthorizeCallback(authnId, authzId);
+ handler.handle(new Callback[]{ac});
+ return ac;
+ }
+
+ /**
+ * The core exploit: a validly authenticated but low-privilege client requests
+ * authzid "super". Before the fix, canonicalization of the cross-realm principal
+ * threw NoMatchingRule, the exception was swallowed, authorized stayed true, and
+ * getAuthorizedID() returned the client-chosen "super". The fix must deny it.
+ */
+ @Test
+ public void authzidSuperMustBeRejectedWhenItDiffersFromAuthenticationId() throws Exception {
+ AuthorizeCallback ac = authorize("bob@CROSSREALM", "super");
+
+ assertFalse(ac.isAuthorized(),
+ "client must not be authorized to assume a different identity ('super')");
+ assertNull(ac.getAuthorizedID(),
+ "no authorized identity may be granted when authorization is denied");
+ }
+
+ /**
+ * Any authzid different from the authenticated identity is impersonation and
+ * must be denied, not just the literal "super".
+ */
+ @Test
+ public void anyMismatchedAuthzidMustBeRejected() throws Exception {
+ AuthorizeCallback ac = authorize("mallory@ATTACK.REALM", "realadmin@PROD.REALM");
+
+ assertFalse(ac.isAuthorized());
+ assertNull(ac.getAuthorizedID());
+ }
+
+ /**
+ * Legitimate case: the client did not request a distinct authzid, so the SASL
+ * layer defaults authorizationID to authenticationID. A simple (realm-less) name
+ * canonicalizes to itself and must be authorized.
+ */
+ @Test
+ public void matchingSimpleNameIsAuthorized() throws Exception {
+ AuthorizeCallback ac = authorize("alice", "alice");
+
+ assertTrue(ac.isAuthorized(), "a client authorizing as itself must be allowed");
+ assertEquals("alice", ac.getAuthorizedID());
+ }
+
+ /**
+ * Regression / compatibility guard for cross-realm users. When authzid equals the
+ * authenticated principal, the client is authorized as itself even though
+ * canonicalization fails under the default auth_to_local rules. The critical
+ * property is that the adopted identity is the client's OWN principal and can
+ * never be an attacker-chosen value.
+ *
+ * <p>Assumes the test host's default Kerberos realm is not literally "CROSSREALM"
+ * (true on any normal build/CI host), so getShortName() throws NoMatchingRule and
+ * the identity falls back to the authenticated principal.
+ */
+ @Test
+ public void matchingCrossRealmPrincipalIsAuthorizedAsItselfNotSpoofable() throws Exception {
+ AuthorizeCallback ac = authorize("bob@CROSSREALM", "bob@CROSSREALM");
+
+ assertTrue(ac.isAuthorized());
+ assertEquals("bob@CROSSREALM", ac.getAuthorizedID(),
+ "cross-realm client must be authorized as its own principal");
+ assertNotEquals("super", ac.getAuthorizedID());
+ }
+
+ // ---------------------------------------------------------------------
+ // Log sanitization (CWE-117). authenticationID and authorizationID are
+ // client-controlled SASL token fields; control characters must be stripped
+ // before logging so a client cannot forge adjacent log lines.
+ // ---------------------------------------------------------------------
+
+ private static final String FORGED_MARKER = "FORGED";
+
+ /**
+ * Captures everything the handler logged during this test, then asserts that
+ * no physical log line begins with the injected marker, i.e. no line was forged.
+ */
+ private static String capturedLogAssertingNoForgedLines() {
+ String output = loggerTestTool.getOutputStream().toString();
+ for (String line : output.split("\\R")) {
+ assertFalse(line.startsWith(FORGED_MARKER),
+ "client-controlled input started a new log line (log forgery): " + line);
+ }
+ return output;
+ }
+
+ private static String lineContaining(String output, String needle) {
+ for (String line : output.split("\\R")) {
+ if (line.contains(needle)) {
+ return line;
+ }
+ }
+ return null;
+ }
+
+ @Test
+ public void denialLogSanitizesControlCharactersAndLogsAtWarn() throws Exception {
+ String authnId = "mallory\n" + FORGED_MARKER + " AUTHN\r\t LINE";
+ String authzId = "super\n" + FORGED_MARKER + " AUTHZ\r\t LINE";
+
+ AuthorizeCallback ac = authorize(authnId, authzId);
+ assertFalse(ac.isAuthorized());
+
+ String output = capturedLogAssertingNoForgedLines();
+ String line = lineContaining(output, "Client attempted to authorize as a different identity");
+ assertNotNull(line, "denial was not logged");
+
+ // CR, LF and TAB are removed; the remaining text stays on the single log line
+ assertTrue(line.contains("authenticationID=mallory" + FORGED_MARKER + " AUTHN LINE"),
+ "authenticationID not logged sanitized on one line: " + line);
+ assertTrue(line.contains("requested authorizationID=super" + FORGED_MARKER + " AUTHZ LINE"),
+ "authorizationID not logged sanitized on one line: " + line);
+
+ // denial must be WARN, not ERROR
+ assertTrue(line.contains(" WARN "), "denial should be logged at WARN: " + line);
+ assertFalse(line.contains(" ERROR "), "denial must not be logged at ERROR: " + line);
+ }
+
+ @Test
+ public void successLogSanitizesControlCharacters() throws Exception {
+ // authcid == authzid, so this takes the success path; the realm-less name
+ // canonicalizes to itself without throwing
+ String id = "alice\n" + FORGED_MARKER + " SUCCESS\r\t LINE";
+
+ AuthorizeCallback ac = authorize(id, id);
+ assertTrue(ac.isAuthorized());
+
+ String output = capturedLogAssertingNoForgedLines();
+ String line = lineContaining(output, "Successfully authenticated client");
+ assertNotNull(line, "successful authorization was not logged");
+ assertTrue(line.contains("authorizationID=alice" + FORGED_MARKER + " SUCCESS LINE"),
+ "authorizationID not logged sanitized on one line: " + line);
+
+ // the canonicalized identity is logged separately; for a realm-less name it
+ // is the client-supplied string verbatim, so it must be sanitized as well
+ String setLine = lineContaining(output, "Setting authorizedID");
+ assertNotNull(setLine, "canonicalized authorizedID was not logged");
+ assertTrue(setLine.contains("Setting authorizedID: alice" + FORGED_MARKER + " SUCCESS LINE"),
+ "canonicalized authorizedID not logged sanitized on one line: " + setLine);
+
+ // only the log output is sanitized; the identity itself is not altered
+ assertEquals(id, ac.getAuthorizedID());
+ }
+
+ @Test
+ public void unknownDigestUserLogSanitizesControlCharacters() throws Exception {
+ // this runs before authentication, so any client can reach it
+ String userName = "ghost\n" + FORGED_MARKER + " UNKNOWN\r\t USER";
+ NameCallback nc = new NameCallback("username: ", userName);
+ newHandler().handle(new Callback[]{nc});
+
+ assertNull(nc.getName(), "unknown user must not be accepted");
+
+ String output = capturedLogAssertingNoForgedLines();
+ String line = lineContaining(output, "not found in list of DIGEST-MD5 authenticateable users");
+ assertNotNull(line, "unknown user was not logged");
+ assertTrue(line.contains("User 'ghost" + FORGED_MARKER + " UNKNOWN USER'"),
+ "username not logged sanitized on one line: " + line);
+ }
+
+ @Test
+ public void denialWithNullAuthenticationIdIsLoggedWithoutError() throws Exception {
+ AuthorizeCallback ac = authorize(null, "super");
+
+ assertFalse(ac.isAuthorized());
+ assertNull(ac.getAuthorizedID());
+
+ String line = lineContaining(loggerTestTool.getOutputStream().toString(),
+ "Client attempted to authorize as a different identity");
+ assertNotNull(line, "denial with null authenticationID was not logged");
+ assertTrue(line.contains("authenticationID=null"), line);
+ }
+}