From fc25726240a631663eab61273b4b5ae664d8c66a Mon Sep 17 00:00:00 2001 From: Aditi Bansal Date: Wed, 12 Aug 2026 11:28:58 +0530 Subject: [PATCH 1/5] Support custom ZooKeeper ACLs when creating a cluster Add HelixAdmin#addCluster(String, boolean, List) so callers can create a cluster whose root znode is owned by a specific ZooKeeper identity instead of the client default ACL. The overload is a default method that throws UnsupportedOperationException so existing HelixAdmin implementations keep compiling, and ZKHelixAdmin routes the two argument version through it with a null ACL. A null or empty ACL preserves the previous behavior exactly. ZooKeeper does not propagate ACLs to children, so only the cluster root carries the supplied ACL. Because ZooKeeper checks the DELETE permission on the parent znode, this is still enough to stop a foreign session from removing the cluster or its top level znodes, but the nodes underneath keep the default open ACL. The added tests document that boundary against a real ZooKeeper server. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../java/org/apache/helix/HelixAdmin.java | 17 ++ .../apache/helix/manager/zk/ZKHelixAdmin.java | 12 +- .../helix/manager/zk/TestZkHelixAdmin.java | 198 ++++++++++++++++++ 3 files changed, 226 insertions(+), 1 deletion(-) diff --git a/helix-core/src/main/java/org/apache/helix/HelixAdmin.java b/helix-core/src/main/java/org/apache/helix/HelixAdmin.java index ed754dcdea..485e0746a3 100644 --- a/helix-core/src/main/java/org/apache/helix/HelixAdmin.java +++ b/helix-core/src/main/java/org/apache/helix/HelixAdmin.java @@ -45,6 +45,7 @@ import org.apache.helix.model.ResourceConfig; import org.apache.helix.model.StateModelDefinition; import org.apache.helix.model.OperationCheckResult; +import org.apache.zookeeper.data.ACL; /* * Helix cluster management @@ -110,6 +111,22 @@ public interface HelixAdmin { */ boolean addCluster(String clusterName, boolean recreateIfExists); + /** + * Add a cluster whose root metadata store node is created with the given ACLs + * @param clusterName + * @param recreateIfExists If the cluster already exists, it will delete it and recreate + * @param acl ACLs applied to the cluster root node ("/{clusterName}"). If null or empty, the + * default ACL of the underlying metadata store client is used, making this equivalent + * to {@link #addCluster(String, boolean)}. Note that ZooKeeper does not propagate ACLs + * to children, so the nodes created underneath the root keep the client default ACL. + * The ACL is only applied when the root node is created by this call; the ACL of a + * pre-existing cluster is left untouched unless recreateIfExists is true. + * @return true if successfully created, or if cluster already exists + */ + default boolean addCluster(String clusterName, boolean recreateIfExists, List acl) { + throw new UnsupportedOperationException("addCluster with ACL is not implemented."); + } + /** * Add a cluster and also add this cluster as a resource group in the super cluster * @param clusterName diff --git a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java index 50a9f04390..96d315dff7 100644 --- a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java +++ b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java @@ -118,6 +118,7 @@ import org.apache.zookeeper.KeeperException; import org.apache.zookeeper.Op; import org.apache.zookeeper.OpResult; +import org.apache.zookeeper.data.ACL; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -1644,6 +1645,11 @@ public boolean addCluster(String clusterName) { @Override public boolean addCluster(String clusterName, boolean recreateIfExists) { + return addCluster(clusterName, recreateIfExists, null); + } + + @Override + public boolean addCluster(String clusterName, boolean recreateIfExists, List acl) { logger.info("Add cluster {}.", clusterName); String root = "/" + clusterName; @@ -1657,7 +1663,11 @@ public boolean addCluster(String clusterName, boolean recreateIfExists) { } } try { - _zkClient.createPersistent(root, true); + if (acl == null || acl.isEmpty()) { + _zkClient.createPersistent(root, true); + } else { + _zkClient.createPersistent(root, true, acl); + } } catch (Exception e) { // some other process might have created the cluster if (_zkClient.exists(root)) { diff --git a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java index b60b909a26..9cacb1bb47 100644 --- a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java +++ b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java @@ -29,6 +29,8 @@ import java.util.List; import java.util.Map; import java.util.UUID; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableMap; @@ -77,13 +79,24 @@ import org.apache.helix.model.builder.HelixConfigScopeBuilder; import org.apache.helix.participant.StateMachineEngine; import org.apache.helix.tools.StateModelConfigGenerator; +import org.apache.helix.zookeeper.api.client.HelixZkClient; import org.apache.helix.zookeeper.api.client.RealmAwareZkClient; import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer; import org.apache.helix.zookeeper.exception.ZkClientException; +import org.apache.helix.zookeeper.impl.factory.DedicatedZkClientFactory; import org.apache.helix.zookeeper.zkclient.NetworkUtil; +import org.apache.helix.zookeeper.zkclient.ZkConnection; import org.apache.helix.zookeeper.zkclient.exception.ZkException; +import org.apache.zookeeper.CreateMode; import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.Watcher; +import org.apache.zookeeper.ZooDefs; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.ACL; +import org.apache.zookeeper.data.Id; import org.apache.zookeeper.data.Stat; +import org.apache.zookeeper.server.auth.DigestAuthenticationProvider; import org.mockito.Mockito; import org.testng.Assert; import org.testng.AssertJUnit; @@ -347,6 +360,191 @@ public void testZkHelixAdmin() { System.out.println("END testZkHelixAdmin at " + new Date(System.currentTimeMillis())); } + @Test + public void testAddClusterWithAcl() throws Exception { + System.out.println("START testAddClusterWithAcl at " + new Date(System.currentTimeMillis())); + + final String clusterName = getShortClassName() + "_withAcl"; + String rootPath = "/" + clusterName; + if (_gZkClient.exists(rootPath)) { + _gZkClient.deleteRecursively(rootPath); + } + + // world:anyone without ADMIN so that the ACL is distinguishable from the default open ACL, + // while still allowing this test to read the cluster back and drop it afterwards + List acl = Collections.singletonList(new ACL( + ZooDefs.Perms.CREATE | ZooDefs.Perms.READ | ZooDefs.Perms.WRITE | ZooDefs.Perms.DELETE, + ZooDefs.Ids.ANYONE_ID_UNSAFE)); + + HelixAdmin tool = new ZKHelixAdmin(_gZkClient); + Assert.assertTrue(tool.addCluster(clusterName, true, acl)); + Assert.assertTrue(ZKUtil.isClusterSetup(clusterName, _gZkClient)); + + // the ACL is applied to the cluster root + Assert.assertEquals(getAcl(rootPath), acl); + // cluster config content is still written after the root is created with the custom ACL + Assert.assertNotNull(_gZkClient.readData(PropertyPathBuilder.clusterConfig(clusterName), true)); + + // ZooKeeper does not propagate ACLs to children, so nodes below the root keep the ZkClient + // default ACL. This documents the boundary of what the ACL argument protects. + Assert.assertEquals(getAcl(PropertyPathBuilder.idealState(clusterName)), + ZooDefs.Ids.OPEN_ACL_UNSAFE); + Assert.assertEquals(getAcl(PropertyPathBuilder.clusterConfig(clusterName)), + ZooDefs.Ids.OPEN_ACL_UNSAFE); + + deleteCluster(clusterName); + System.out.println("END testAddClusterWithAcl at " + new Date(System.currentTimeMillis())); + } + + @Test + public void testAddClusterWithoutAclKeepsDefaultAcl() throws Exception { + System.out.println( + "START testAddClusterWithoutAclKeepsDefaultAcl at " + new Date(System.currentTimeMillis())); + + final String clusterName = getShortClassName() + "_noAcl"; + String rootPath = "/" + clusterName; + if (_gZkClient.exists(rootPath)) { + _gZkClient.deleteRecursively(rootPath); + } + + HelixAdmin tool = new ZKHelixAdmin(_gZkClient); + // an empty ACL list must behave exactly like the two argument overload + Assert.assertTrue(tool.addCluster(clusterName, true, Collections.emptyList())); + Assert.assertTrue(ZKUtil.isClusterSetup(clusterName, _gZkClient)); + Assert.assertEquals(getAcl(rootPath), ZooDefs.Ids.OPEN_ACL_UNSAFE); + + deleteCluster(clusterName); + System.out.println( + "END testAddClusterWithoutAclKeepsDefaultAcl at " + new Date(System.currentTimeMillis())); + } + + private static List getAcl(String path) throws Exception { + return getAcl(rawZooKeeper(_gZkClient), path); + } + + private static List getAcl(ZooKeeper zooKeeper, String path) throws Exception { + return zooKeeper.getACL(path, new Stat()); + } + + private static ZooKeeper rawZooKeeper(Object helixZkClient) { + return ((ZkConnection) ((org.apache.helix.zookeeper.zkclient.ZkClient) helixZkClient) + .getConnection()).getZookeeper(); + } + + /** + * Creates a cluster whose root is owned by a digest user and verifies, with a second client that + * does not present those credentials, what the root ACL actually protects. + */ + @Test + public void testAddClusterAclEnforcement() throws Exception { + System.out.println( + "START testAddClusterAclEnforcement at " + new Date(System.currentTimeMillis())); + + final String clusterName = getShortClassName() + "_aclEnforced"; + final String rootPath = "/" + clusterName; + final String owner = "helixAdmin"; + final String password = "helixAdminPassword"; + final byte[] credentials = (owner + ":" + password).getBytes(); + + // only the digest user gets full permissions on the cluster root + List acl = Collections.singletonList(new ACL(ZooDefs.Perms.ALL, + new Id("digest", DigestAuthenticationProvider.generateDigest(owner + ":" + password)))); + + HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig(); + clientConfig.setZkSerializer(new ZNRecordSerializer()); + HelixZkClient authorizedClient = DedicatedZkClientFactory.getInstance() + .buildZkClient(new HelixZkClient.ZkConnectionConfig(ZK_ADDR), clientConfig); + ZooKeeper unauthorizedClient = null; + try { + ((org.apache.helix.zookeeper.zkclient.ZkClient) authorizedClient) + .addAuthInfo("digest", credentials); + if (authorizedClient.exists(rootPath)) { + authorizedClient.deleteRecursively(rootPath); + } + + HelixAdmin tool = new ZKHelixAdmin(authorizedClient); + Assert.assertTrue(tool.addCluster(clusterName, true, acl)); + Assert.assertEquals(getAcl(rawZooKeeper(authorizedClient), rootPath), acl); + + // a second session that never presents the digest credentials + unauthorizedClient = createUnauthenticatedZkClient(); + + // The root ACL is enforced: a session without the credentials cannot even read the ACL. + try { + getAcl(unauthorizedClient, rootPath); + Assert.fail("Expected reading the ACL of a protected root to be rejected"); + } catch (KeeperException.NoAuthException expected) { + // expected + } + + // Deleting the root itself is rejected because it still has children. ZooKeeper checks the + // DELETE permission on the parent, and the parent here is "/", which is world writable. + try { + unauthorizedClient.delete(rootPath, -1); + Assert.fail("Expected the delete of a non empty cluster root to be rejected"); + } catch (KeeperException.NotEmptyException expected) { + // expected + } + + // Removing or adding a top level znode is checked against the root ACL, so it is blocked. + String idealStatePath = PropertyPathBuilder.idealState(clusterName); + try { + unauthorizedClient.delete(idealStatePath, -1); + Assert.fail("Expected the delete of " + idealStatePath + " to be rejected"); + } catch (KeeperException.NoAuthException expected) { + // expected + } + try { + unauthorizedClient.create(rootPath + "/INJECTED", new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, + CreateMode.PERSISTENT); + Assert.fail("Expected creating a child of the cluster root to be rejected"); + } catch (KeeperException.NoAuthException expected) { + // expected + } + + // A recursive delete therefore cannot get past the first level and the cluster survives. + Assert.assertTrue(authorizedClient.exists(rootPath)); + Assert.assertTrue(authorizedClient.exists(idealStatePath)); + + // However the nodes below the root were created with the default open ACL, so the same + // unauthorized session can still read and modify everything inside the cluster. + Assert.assertEquals(getAcl(unauthorizedClient, idealStatePath), ZooDefs.Ids.OPEN_ACL_UNSAFE); + String clusterConfigPath = PropertyPathBuilder.clusterConfig(clusterName); + Assert.assertNotNull(unauthorizedClient.getData(clusterConfigPath, false, new Stat()), + "Cluster config is readable by an unauthorized session"); + unauthorizedClient.setData(clusterConfigPath, new byte[0], -1); + String injectedIdealState = idealStatePath + "/injectedResource"; + unauthorizedClient.create(injectedIdealState, new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, + CreateMode.PERSISTENT); + Assert.assertTrue(authorizedClient.exists(injectedIdealState), + "An unauthorized session was able to add a resource to the cluster"); + unauthorizedClient.delete(injectedIdealState, -1); + + // The owner of the root ACL can still tear the cluster down. + authorizedClient.deleteRecursively(rootPath); + Assert.assertFalse(authorizedClient.exists(rootPath)); + } finally { + if (unauthorizedClient != null) { + unauthorizedClient.close(); + } + authorizedClient.close(); + } + + System.out.println( + "END testAddClusterAclEnforcement at " + new Date(System.currentTimeMillis())); + } + + private static ZooKeeper createUnauthenticatedZkClient() throws Exception { + CountDownLatch connected = new CountDownLatch(1); + ZooKeeper zooKeeper = new ZooKeeper(ZK_ADDR, 30000, event -> { + if (event.getState() == Watcher.Event.KeeperState.SyncConnected) { + connected.countDown(); + } + }); + Assert.assertTrue(connected.await(30, TimeUnit.SECONDS), "Failed to connect to " + ZK_ADDR); + return zooKeeper; + } + @Test private void testSetInstanceOperation() { System.out.println("START testSetInstanceOperation at " + new Date(System.currentTimeMillis())); From fdedb6f2d4e3fbf1c87bef8882aabf2b64467b3c Mon Sep 17 00:00:00 2001 From: Aditi Bansal Date: Wed, 12 Aug 2026 12:09:06 +0530 Subject: [PATCH 2/5] Address review: honor null/empty ACL in the default overload The default addCluster(String, boolean, List) threw UnsupportedOperationException unconditionally, which contradicted its own javadoc and broke non ZK HelixAdmin implementations that pass a null or empty ACL. It now delegates to addCluster(String, boolean) in that case and only throws when a caller actually asks for custom ACLs. Also type rawZooKeeper's parameter as HelixZkClient instead of Object so the test helper does not silently accept an unrelated type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- helix-core/src/main/java/org/apache/helix/HelixAdmin.java | 5 +++++ .../java/org/apache/helix/manager/zk/TestZkHelixAdmin.java | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/helix-core/src/main/java/org/apache/helix/HelixAdmin.java b/helix-core/src/main/java/org/apache/helix/HelixAdmin.java index 485e0746a3..8300bf5d2a 100644 --- a/helix-core/src/main/java/org/apache/helix/HelixAdmin.java +++ b/helix-core/src/main/java/org/apache/helix/HelixAdmin.java @@ -122,8 +122,13 @@ public interface HelixAdmin { * The ACL is only applied when the root node is created by this call; the ACL of a * pre-existing cluster is left untouched unless recreateIfExists is true. * @return true if successfully created, or if cluster already exists + * @throws UnsupportedOperationException if a non empty ACL is supplied and the implementation + * does not support custom ACLs */ default boolean addCluster(String clusterName, boolean recreateIfExists, List acl) { + if (acl == null || acl.isEmpty()) { + return addCluster(clusterName, recreateIfExists); + } throw new UnsupportedOperationException("addCluster with ACL is not implemented."); } diff --git a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java index 9cacb1bb47..9c1b26899f 100644 --- a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java +++ b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java @@ -426,7 +426,7 @@ private static List getAcl(ZooKeeper zooKeeper, String path) throws Excepti return zooKeeper.getACL(path, new Stat()); } - private static ZooKeeper rawZooKeeper(Object helixZkClient) { + private static ZooKeeper rawZooKeeper(HelixZkClient helixZkClient) { return ((ZkConnection) ((org.apache.helix.zookeeper.zkclient.ZkClient) helixZkClient) .getConnection()).getZookeeper(); } From 876f3017fe3223f743325e88f0a785a88ea8b5cd Mon Sep 17 00:00:00 2001 From: Aditi Bansal Date: Wed, 12 Aug 2026 12:17:00 +0530 Subject: [PATCH 3/5] Address review nits: explicit UTF-8 charset and javadoc wording Use StandardCharsets.UTF_8 when converting the digest credentials to bytes so the test does not depend on the platform default charset, and hyphenate "non-empty" in the javadoc and the test failure message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- helix-core/src/main/java/org/apache/helix/HelixAdmin.java | 2 +- .../java/org/apache/helix/manager/zk/TestZkHelixAdmin.java | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/helix-core/src/main/java/org/apache/helix/HelixAdmin.java b/helix-core/src/main/java/org/apache/helix/HelixAdmin.java index 8300bf5d2a..e85ca02fa0 100644 --- a/helix-core/src/main/java/org/apache/helix/HelixAdmin.java +++ b/helix-core/src/main/java/org/apache/helix/HelixAdmin.java @@ -122,7 +122,7 @@ public interface HelixAdmin { * The ACL is only applied when the root node is created by this call; the ACL of a * pre-existing cluster is left untouched unless recreateIfExists is true. * @return true if successfully created, or if cluster already exists - * @throws UnsupportedOperationException if a non empty ACL is supplied and the implementation + * @throws UnsupportedOperationException if a non-empty ACL is supplied and the implementation * does not support custom ACLs */ default boolean addCluster(String clusterName, boolean recreateIfExists, List acl) { diff --git a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java index 9c1b26899f..1176d31bb6 100644 --- a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java +++ b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java @@ -20,6 +20,7 @@ */ import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -444,7 +445,7 @@ public void testAddClusterAclEnforcement() throws Exception { final String rootPath = "/" + clusterName; final String owner = "helixAdmin"; final String password = "helixAdminPassword"; - final byte[] credentials = (owner + ":" + password).getBytes(); + final byte[] credentials = (owner + ":" + password).getBytes(StandardCharsets.UTF_8); // only the digest user gets full permissions on the cluster root List acl = Collections.singletonList(new ACL(ZooDefs.Perms.ALL, @@ -481,7 +482,7 @@ public void testAddClusterAclEnforcement() throws Exception { // DELETE permission on the parent, and the parent here is "/", which is world writable. try { unauthorizedClient.delete(rootPath, -1); - Assert.fail("Expected the delete of a non empty cluster root to be rejected"); + Assert.fail("Expected the delete of a non-empty cluster root to be rejected"); } catch (KeeperException.NotEmptyException expected) { // expected } From f9887558f2f4e764d9ec6e0e4785b84bcf9b71b4 Mon Sep 17 00:00:00 2001 From: Aditi Bansal Date: Sun, 16 Aug 2026 12:18:00 +0530 Subject: [PATCH 4/5] Apply the cluster ACL to all cluster metadata nodes, not just the root ZooKeeper does not inherit ACLs, so protecting only /{clusterName} left every node below it with the ZkClient default OPEN_ACL_UNSAFE. That blocked adding or removing top level znodes but still allowed any session to read, overwrite and delete cluster state, and even rewrite child ACLs to lock the owner out. Thread the ACL through createZKPaths so every node created by addCluster carries it. Behavior is unchanged when the ACL is null or empty. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../java/org/apache/helix/HelixAdmin.java | 21 +++-- .../apache/helix/manager/zk/ZKHelixAdmin.java | 54 ++++++++----- .../helix/manager/zk/TestZkHelixAdmin.java | 76 ++++++++++++++----- 3 files changed, 108 insertions(+), 43 deletions(-) diff --git a/helix-core/src/main/java/org/apache/helix/HelixAdmin.java b/helix-core/src/main/java/org/apache/helix/HelixAdmin.java index e85ca02fa0..7986d953bd 100644 --- a/helix-core/src/main/java/org/apache/helix/HelixAdmin.java +++ b/helix-core/src/main/java/org/apache/helix/HelixAdmin.java @@ -112,15 +112,22 @@ public interface HelixAdmin { boolean addCluster(String clusterName, boolean recreateIfExists); /** - * Add a cluster whose root metadata store node is created with the given ACLs + * Add a cluster whose metadata store nodes are created with the given ACLs * @param clusterName * @param recreateIfExists If the cluster already exists, it will delete it and recreate - * @param acl ACLs applied to the cluster root node ("/{clusterName}"). If null or empty, the - * default ACL of the underlying metadata store client is used, making this equivalent - * to {@link #addCluster(String, boolean)}. Note that ZooKeeper does not propagate ACLs - * to children, so the nodes created underneath the root keep the client default ACL. - * The ACL is only applied when the root node is created by this call; the ACL of a - * pre-existing cluster is left untouched unless recreateIfExists is true. + * @param acl ACLs applied to the cluster root node ("/{clusterName}") and to every cluster + * metadata node created underneath it by this call. If null or empty, the default ACL + * of the underlying metadata store client is used, making this equivalent to + * {@link #addCluster(String, boolean)}. ZooKeeper does not propagate ACLs to children, + * so nodes created after this call (resources, instances, live instances, ...) are + * NOT covered and keep the client default ACL. The ACL is only applied when the nodes + * are created by this call; the ACL of a pre-existing cluster is left untouched unless + * recreateIfExists is true. + *

+ * The supplied ACL must grant the calling client CREATE on the root, otherwise cluster + * creation fails part way through and leaves an incomplete cluster behind. Deployments + * running a server-side ACL provider that assigns ACLs on create may ignore this + * argument entirely. * @return true if successfully created, or if cluster already exists * @throws UnsupportedOperationException if a non-empty ACL is supplied and the implementation * does not support custom ACLs diff --git a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java index 96d315dff7..0d0f1d6895 100644 --- a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java +++ b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java @@ -1677,7 +1677,7 @@ public boolean addCluster(String clusterName, boolean recreateIfExists, List acl) { String path; // IDEAL STATE - _zkClient.createPersistent(PropertyPathBuilder.idealState(clusterName)); + createPersistent(PropertyPathBuilder.idealState(clusterName), false, acl); // CONFIGURATIONS path = PropertyPathBuilder.clusterConfig(clusterName); - _zkClient.createPersistent(path, true); + createPersistent(path, true, acl); _zkClient.writeData(path, new ZNRecord(clusterName)); path = PropertyPathBuilder.instanceConfig(clusterName); - _zkClient.createPersistent(path); + createPersistent(path, false, acl); path = PropertyPathBuilder.resourceConfig(clusterName); - _zkClient.createPersistent(path); + createPersistent(path, false, acl); path = PropertyPathBuilder.customizedStateConfig(clusterName); - _zkClient.createPersistent(path); + createPersistent(path, false, acl); // PROPERTY STORE path = PropertyPathBuilder.propertyStore(clusterName); - _zkClient.createPersistent(path); + createPersistent(path, false, acl); // LIVE INSTANCES - _zkClient.createPersistent(PropertyPathBuilder.liveInstance(clusterName)); + createPersistent(PropertyPathBuilder.liveInstance(clusterName), false, acl); // MEMBER INSTANCES - _zkClient.createPersistent(PropertyPathBuilder.instance(clusterName)); + createPersistent(PropertyPathBuilder.instance(clusterName), false, acl); // External view - _zkClient.createPersistent(PropertyPathBuilder.externalView(clusterName)); + createPersistent(PropertyPathBuilder.externalView(clusterName), false, acl); // State model definition - _zkClient.createPersistent(PropertyPathBuilder.stateModelDef(clusterName)); + createPersistent(PropertyPathBuilder.stateModelDef(clusterName), false, acl); // controller - _zkClient.createPersistent(PropertyPathBuilder.controller(clusterName)); + createPersistent(PropertyPathBuilder.controller(clusterName), false, acl); path = PropertyPathBuilder.controllerHistory(clusterName); final ZNRecord emptyHistory = new ZNRecord(PropertyType.HISTORY.toString()); final List emptyList = new ArrayList(); emptyHistory.setListField(clusterName, emptyList); - _zkClient.createPersistent(path, emptyHistory); + createPersistent(path, emptyHistory, acl); path = PropertyPathBuilder.controllerMessage(clusterName); - _zkClient.createPersistent(path); + createPersistent(path, false, acl); path = PropertyPathBuilder.controllerStatusUpdate(clusterName); - _zkClient.createPersistent(path); + createPersistent(path, false, acl); path = PropertyPathBuilder.controllerError(clusterName); - _zkClient.createPersistent(path); + createPersistent(path, false, acl); + } + + /** + * Creates a persistent node, applying the given ACL when one is supplied. A null or empty ACL + * falls back to the ZkClient default, preserving the behavior of clusters created without ACLs. + */ + private void createPersistent(String path, boolean createParents, List acl) { + if (acl == null || acl.isEmpty()) { + _zkClient.createPersistent(path, createParents); + } else { + _zkClient.createPersistent(path, createParents, acl); + } + } + + private void createPersistent(String path, Object data, List acl) { + if (acl == null || acl.isEmpty()) { + _zkClient.createPersistent(path, data); + } else { + _zkClient.createPersistent(path, data, acl); + } } @Override diff --git a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java index 1176d31bb6..33a2619278 100644 --- a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java +++ b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java @@ -386,11 +386,30 @@ public void testAddClusterWithAcl() throws Exception { // cluster config content is still written after the root is created with the custom ACL Assert.assertNotNull(_gZkClient.readData(PropertyPathBuilder.clusterConfig(clusterName), true)); - // ZooKeeper does not propagate ACLs to children, so nodes below the root keep the ZkClient - // default ACL. This documents the boundary of what the ACL argument protects. - Assert.assertEquals(getAcl(PropertyPathBuilder.idealState(clusterName)), - ZooDefs.Ids.OPEN_ACL_UNSAFE); - Assert.assertEquals(getAcl(PropertyPathBuilder.clusterConfig(clusterName)), + // every cluster metadata node created by addCluster carries the ACL as well. ZooKeeper has no + // ACL inheritance, so protecting only the root would leave all cluster data world writable. + for (String path : new String[] { + PropertyPathBuilder.idealState(clusterName), PropertyPathBuilder.clusterConfig(clusterName), + PropertyPathBuilder.instanceConfig(clusterName), + PropertyPathBuilder.resourceConfig(clusterName), + PropertyPathBuilder.customizedStateConfig(clusterName), + PropertyPathBuilder.propertyStore(clusterName), + PropertyPathBuilder.liveInstance(clusterName), PropertyPathBuilder.instance(clusterName), + PropertyPathBuilder.externalView(clusterName), + PropertyPathBuilder.stateModelDef(clusterName), PropertyPathBuilder.controller(clusterName), + PropertyPathBuilder.controllerHistory(clusterName), + PropertyPathBuilder.controllerMessage(clusterName), + PropertyPathBuilder.controllerStatusUpdate(clusterName), + PropertyPathBuilder.controllerError(clusterName) + }) { + Assert.assertEquals(getAcl(path), acl, "unexpected ACL on " + path); + } + + // nodes created after addCluster returns are not covered, since they are created by other + // code paths that do not know about this ACL. This documents the boundary of the argument. + tool.addStateModelDef(clusterName, "MasterSlave", MasterSlaveSMD.build()); + Assert.assertEquals( + getAcl(PropertyPathBuilder.stateModelDef(clusterName) + "/MasterSlave"), ZooDefs.Ids.OPEN_ACL_UNSAFE); deleteCluster(clusterName); @@ -433,8 +452,9 @@ private static ZooKeeper rawZooKeeper(HelixZkClient helixZkClient) { } /** - * Creates a cluster whose root is owned by a digest user and verifies, with a second client that - * does not present those credentials, what the root ACL actually protects. + * Creates a cluster owned by a digest user and verifies, with a second client that does not + * present those credentials, that both the root and the cluster metadata nodes below it are + * protected. */ @Test public void testAddClusterAclEnforcement() throws Exception { @@ -507,19 +527,37 @@ public void testAddClusterAclEnforcement() throws Exception { Assert.assertTrue(authorizedClient.exists(rootPath)); Assert.assertTrue(authorizedClient.exists(idealStatePath)); - // However the nodes below the root were created with the default open ACL, so the same - // unauthorized session can still read and modify everything inside the cluster. - Assert.assertEquals(getAcl(unauthorizedClient, idealStatePath), ZooDefs.Ids.OPEN_ACL_UNSAFE); + // The nodes below the root carry the same ACL, so the unauthorized session cannot read or + // modify cluster data either. Without this, the root ACL would only protect the top level + // znodes while leaving every piece of cluster state world writable. + Assert.assertEquals(getAcl(rawZooKeeper(authorizedClient), idealStatePath), acl); String clusterConfigPath = PropertyPathBuilder.clusterConfig(clusterName); - Assert.assertNotNull(unauthorizedClient.getData(clusterConfigPath, false, new Stat()), - "Cluster config is readable by an unauthorized session"); - unauthorizedClient.setData(clusterConfigPath, new byte[0], -1); - String injectedIdealState = idealStatePath + "/injectedResource"; - unauthorizedClient.create(injectedIdealState, new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, - CreateMode.PERSISTENT); - Assert.assertTrue(authorizedClient.exists(injectedIdealState), - "An unauthorized session was able to add a resource to the cluster"); - unauthorizedClient.delete(injectedIdealState, -1); + try { + unauthorizedClient.getData(clusterConfigPath, false, new Stat()); + Assert.fail("Expected reading the cluster config to be rejected"); + } catch (KeeperException.NoAuthException expected) { + // expected + } + try { + unauthorizedClient.setData(clusterConfigPath, new byte[0], -1); + Assert.fail("Expected overwriting the cluster config to be rejected"); + } catch (KeeperException.NoAuthException expected) { + // expected + } + try { + unauthorizedClient.create(idealStatePath + "/injectedResource", new byte[0], + ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); + Assert.fail("Expected injecting a resource into the cluster to be rejected"); + } catch (KeeperException.NoAuthException expected) { + // expected + } + // and it cannot hand itself permissions by rewriting a child ACL + try { + unauthorizedClient.setACL(idealStatePath, ZooDefs.Ids.OPEN_ACL_UNSAFE, -1); + Assert.fail("Expected rewriting the ACL of a cluster node to be rejected"); + } catch (KeeperException.NoAuthException expected) { + // expected + } // The owner of the root ACL can still tear the cluster down. authorizedClient.deleteRecursively(rootPath); From d4500b812caabb0e62f9074f5629a885b8746d56 Mon Sep 17 00:00:00 2001 From: Aditi Bansal Date: Sun, 16 Aug 2026 12:28:22 +0530 Subject: [PATCH 5/5] Reuse existing constants in the ACL tests Use MasterSlaveSMD.name instead of repeating the literal, and hoist the digest scheme into a named constant. ZooKeeper exposes scheme names only through AuthenticationProvider#getScheme, so there is no upstream constant to reuse. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../org/apache/helix/manager/zk/TestZkHelixAdmin.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java index 33a2619278..cdb721b88d 100644 --- a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java +++ b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java @@ -106,6 +106,9 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); + // ZooKeeper exposes scheme names only through AuthenticationProvider#getScheme, so there is no + // public constant to reuse here + private static final String DIGEST_SCHEME = "digest"; @BeforeClass public void beforeClass() { @@ -407,9 +410,9 @@ public void testAddClusterWithAcl() throws Exception { // nodes created after addCluster returns are not covered, since they are created by other // code paths that do not know about this ACL. This documents the boundary of the argument. - tool.addStateModelDef(clusterName, "MasterSlave", MasterSlaveSMD.build()); + tool.addStateModelDef(clusterName, MasterSlaveSMD.name, MasterSlaveSMD.build()); Assert.assertEquals( - getAcl(PropertyPathBuilder.stateModelDef(clusterName) + "/MasterSlave"), + getAcl(PropertyPathBuilder.stateModelDef(clusterName) + "/" + MasterSlaveSMD.name), ZooDefs.Ids.OPEN_ACL_UNSAFE); deleteCluster(clusterName); @@ -469,7 +472,7 @@ public void testAddClusterAclEnforcement() throws Exception { // only the digest user gets full permissions on the cluster root List acl = Collections.singletonList(new ACL(ZooDefs.Perms.ALL, - new Id("digest", DigestAuthenticationProvider.generateDigest(owner + ":" + password)))); + new Id(DIGEST_SCHEME, DigestAuthenticationProvider.generateDigest(owner + ":" + password)))); HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig(); clientConfig.setZkSerializer(new ZNRecordSerializer()); @@ -478,7 +481,7 @@ public void testAddClusterAclEnforcement() throws Exception { ZooKeeper unauthorizedClient = null; try { ((org.apache.helix.zookeeper.zkclient.ZkClient) authorizedClient) - .addAuthInfo("digest", credentials); + .addAuthInfo(DIGEST_SCHEME, credentials); if (authorizedClient.exists(rootPath)) { authorizedClient.deleteRecursively(rootPath); }