This is an automated email from the ASF dual-hosted git repository. anmolnar pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/zookeeper.git
commit 4571e5fb1af2832c30830417dc229fa940c6a424 Author: Andor Molnar <[email protected]> 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 d263fd75c4..0eb266bb7e 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 @@ -555,6 +555,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 3cf993abbb..1d25ad8e00 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 @@ -49,6 +49,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; @@ -292,6 +293,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
