From 68579394c3f5d875a4b69adb0ae686905ab69b55 Mon Sep 17 00:00:00 2001 From: Laszlo Bodor Date: Sun, 19 Jul 2026 08:46:45 +0200 Subject: [PATCH 1/3] TEZ-4739: Improve ACLManager --- .../tez/common/security/ACLManager.java | 12 ++-- .../tez/common/security/TestACLManager.java | 67 +++++++++++++++++++ 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java b/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java index 06ffd88744..7c5143448b 100644 --- a/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java +++ b/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java @@ -57,8 +57,8 @@ public ACLManager(String amUser) { public ACLManager(String amUser, Configuration conf) { this.amUser = amUser; this.dagUser = null; - this.users = new HashMap>(); - this.groups = new HashMap>(); + this.users = new HashMap<>(); + this.groups = new HashMap<>(); aclsEnabled = conf.getBoolean(TezConfiguration.TEZ_AM_ACLS_ENABLED, TezConfiguration.TEZ_AM_ACLS_ENABLED_DEFAULT); if (!aclsEnabled) { @@ -76,12 +76,16 @@ public ACLManager(String amUser, Configuration conf) { public ACLManager(ACLManager amACLManager, String dagUser, ACLInfo aclInfo) { this.amUser = amACLManager.amUser; this.dagUser = dagUser; - this.users = amACLManager.users; - this.groups = amACLManager.groups; + // Copy the AM-level maps so per-DAG entries stay scoped to this DAG. + this.users = new HashMap<>(amACLManager.users); + this.groups = new HashMap<>(amACLManager.groups); this.aclsEnabled = amACLManager.aclsEnabled; if (!aclsEnabled) { return; } + if (aclInfo == null) { + return; + } if (aclInfo.getUsersWithViewAccessCount() > 0) { this.users.put(ACLType.DAG_VIEW_ACL, Sets.newLinkedHashSet(aclInfo.getUsersWithViewAccessList())); diff --git a/tez-api/src/test/java/org/apache/tez/common/security/TestACLManager.java b/tez-api/src/test/java/org/apache/tez/common/security/TestACLManager.java index ab40898fb8..7fab0ddbf2 100644 --- a/tez-api/src/test/java/org/apache/tez/common/security/TestACLManager.java +++ b/tez-api/src/test/java/org/apache/tez/common/security/TestACLManager.java @@ -431,4 +431,71 @@ public void testConvertToYARNACLs() { } + /** + * Per-DAG ACLs should stay scoped to their DAG and not carry over to + * sibling DAGs sharing the same AM ACLManager. + */ + @Test + @Timeout(value = 5000, unit = TimeUnit.MILLISECONDS) + public void testDAGACLsDoNotLeakAcrossDAGs() throws IOException { + UserGroupInformation amUser = UserGroupInformation.createUserForTesting("amUser", noGroups); + UserGroupInformation dag1User = UserGroupInformation.createUserForTesting("dag1User", noGroups); + UserGroupInformation dag2User = UserGroupInformation.createUserForTesting("dag2User", noGroups); + UserGroupInformation user1 = UserGroupInformation.createUserForTesting("user1", noGroups); + String[] grp1 = new String[] {"grp1"}; + UserGroupInformation user2 = UserGroupInformation.createUserForTesting("user2", grp1); + + Configuration conf = new Configuration(false); + ACLManager amAclManager = new ACLManager(amUser.getShortUserName(), conf); + + ACLInfo.Builder dag1Builder = ACLInfo.newBuilder(); + dag1Builder.addUsersWithViewAccess(user1.getShortUserName()); + dag1Builder.addUsersWithModifyAccess(user1.getShortUserName()); + dag1Builder.addGroupsWithViewAccess("grp1"); + dag1Builder.addGroupsWithModifyAccess("grp1"); + ACLManager dag1AclManager = + new ACLManager(amAclManager, dag1User.getShortUserName(), dag1Builder.build()); + + assertTrue(dag1AclManager.checkDAGModifyAccess(user1)); + assertTrue(dag1AclManager.checkDAGViewAccess(user1)); + assertTrue(dag1AclManager.checkDAGModifyAccess(user2)); + assertTrue(dag1AclManager.checkDAGViewAccess(user2)); + + // DAG 2 in the same session has an empty ACLInfo — DAG 1's grants must + // not carry over. + ACLManager dag2AclManager = new ACLManager(amAclManager, dag2User.getShortUserName(), + ACLInfo.getDefaultInstance()); + + assertFalse(dag2AclManager.checkDAGModifyAccess(user1)); + assertFalse(dag2AclManager.checkDAGViewAccess(user1)); + assertFalse(dag2AclManager.checkDAGModifyAccess(user2)); + assertFalse(dag2AclManager.checkDAGViewAccess(user2)); + + // AM-level manager should also be untouched by DAG 1's grants. + assertFalse(amAclManager.checkAccess(user1, ACLType.DAG_MODIFY_ACL)); + assertFalse(amAclManager.checkAccess(user2, ACLType.DAG_MODIFY_ACL)); + } + + /** + * The per-DAG constructor must tolerate a null ACLInfo (the disabled-ACLs + * test above already relies on this via a different code path, but with + * ACLs enabled the null must still not NPE). + */ + @Test + @Timeout(value = 5000, unit = TimeUnit.MILLISECONDS) + public void testDAGConstructorAcceptsNullACLInfo() { + UserGroupInformation amUser = UserGroupInformation.createUserForTesting("amUser", noGroups); + UserGroupInformation dagUser = UserGroupInformation.createUserForTesting("dagUser", noGroups); + UserGroupInformation other = UserGroupInformation.createUserForTesting("other", noGroups); + + Configuration conf = new Configuration(false); + ACLManager amAclManager = new ACLManager(amUser.getShortUserName(), conf); + ACLManager dagAclManager = + new ACLManager(amAclManager, dagUser.getShortUserName(), null); + + assertTrue(dagAclManager.checkDAGViewAccess(dagUser)); + assertTrue(dagAclManager.checkDAGModifyAccess(dagUser)); + assertFalse(dagAclManager.checkDAGViewAccess(other)); + assertFalse(dagAclManager.checkDAGModifyAccess(other)); + } } From f73cbdc6cccd9ca5560fdd58285b1db8c552d764 Mon Sep 17 00:00:00 2001 From: Laszlo Bodor Date: Mon, 20 Jul 2026 11:41:34 +0200 Subject: [PATCH 2/3] PR comments --- .../main/java/org/apache/tez/common/security/ACLManager.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java b/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java index 7c5143448b..3a037c40e5 100644 --- a/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java +++ b/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java @@ -80,10 +80,7 @@ public ACLManager(ACLManager amACLManager, String dagUser, ACLInfo aclInfo) { this.users = new HashMap<>(amACLManager.users); this.groups = new HashMap<>(amACLManager.groups); this.aclsEnabled = amACLManager.aclsEnabled; - if (!aclsEnabled) { - return; - } - if (aclInfo == null) { + if (!aclsEnabled || aclInfo == null) { return; } if (aclInfo.getUsersWithViewAccessCount() > 0) { From 6142f4bc6986ab39dad1d95a21df5f9af7ae5e79 Mon Sep 17 00:00:00 2001 From: Laszlo Bodor Date: Wed, 22 Jul 2026 12:12:48 +0200 Subject: [PATCH 3/3] Deep-copy AM ACL sets in per-DAG ACLManager constructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-DAG ACLManager constructor previously did new HashMap<>(amACLManager.users) and new HashMap<>(amACLManager.groups), which is a shallow copy: the outer map is new, but each Set value is still shared with the AM manager. Today only put(...) is used on those maps, so no leak occurs in practice — but any future this.users.get(ACLType.AM_VIEW_ACL).add(user) inside this class would silently mutate the AM's global ACLs and every sibling DAG's view of them. Deep-copy each inner Set on construction, and add a reflection-based regression test that mutates the DAG manager's set in place and asserts the AM manager is untouched — verified to fail against the shallow-copy version. --- .../tez/common/security/ACLManager.java | 18 +++++++-- .../tez/common/security/TestACLManager.java | 38 +++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java b/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java index 3a037c40e5..6b5080e7d2 100644 --- a/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java +++ b/tez-api/src/main/java/org/apache/tez/common/security/ACLManager.java @@ -22,6 +22,7 @@ import java.util.Collection; import java.util.EnumSet; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.Map; import java.util.Map.Entry; import java.util.Set; @@ -76,9 +77,12 @@ public ACLManager(String amUser, Configuration conf) { public ACLManager(ACLManager amACLManager, String dagUser, ACLInfo aclInfo) { this.amUser = amACLManager.amUser; this.dagUser = dagUser; - // Copy the AM-level maps so per-DAG entries stay scoped to this DAG. - this.users = new HashMap<>(amACLManager.users); - this.groups = new HashMap<>(amACLManager.groups); + // Deep-copy the AM-level maps so per-DAG entries stay scoped to this DAG. + // A shallow copy of the outer map would leave the inner Sets shared with + // the AM manager, so a future set.add(...) on any entry would silently + // mutate the AM's global ACLs and every sibling DAG's view of them. + this.users = deepCopy(amACLManager.users); + this.groups = deepCopy(amACLManager.groups); this.aclsEnabled = amACLManager.aclsEnabled; if (!aclsEnabled || aclInfo == null) { return; @@ -234,6 +238,14 @@ public Map toYARNACls() { return acls; } + private static Map> deepCopy(Map> source) { + Map> copy = new HashMap<>(source.size()); + for (Entry> entry : source.entrySet()) { + copy.put(entry.getKey(), new LinkedHashSet<>(entry.getValue())); + } + return copy; + } + public static String toCommaSeparatedString(Collection collection) { StringBuilder sb = new StringBuilder(); boolean first = true; diff --git a/tez-api/src/test/java/org/apache/tez/common/security/TestACLManager.java b/tez-api/src/test/java/org/apache/tez/common/security/TestACLManager.java index 7fab0ddbf2..e12ada4fe7 100644 --- a/tez-api/src/test/java/org/apache/tez/common/security/TestACLManager.java +++ b/tez-api/src/test/java/org/apache/tez/common/security/TestACLManager.java @@ -23,7 +23,9 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; +import java.lang.reflect.Field; import java.util.Map; +import java.util.Set; import java.util.concurrent.TimeUnit; import org.apache.hadoop.conf.Configuration; @@ -476,6 +478,42 @@ public void testDAGACLsDoNotLeakAcrossDAGs() throws IOException { assertFalse(amAclManager.checkAccess(user2, ACLType.DAG_MODIFY_ACL)); } + /** + * The per-DAG constructor must deep-copy the AM's ACL sets. Otherwise a + * later {@code users.get(ACLType.AM_VIEW_ACL).add(...)} inside the DAG-scoped + * manager would silently mutate the AM's global ACLs (and every sibling + * DAG's view of them). This test simulates that mutation via reflection so + * it catches the shallow-copy regression even though today's code paths + * only replace entries via {@code put}. + */ + @Test + @Timeout(value = 5000, unit = TimeUnit.MILLISECONDS) + public void testDAGConstructorDeepCopiesAMACLs() throws Exception { + UserGroupInformation amUser = UserGroupInformation.createUserForTesting("amUser", noGroups); + UserGroupInformation dagUser = UserGroupInformation.createUserForTesting("dagUser", noGroups); + UserGroupInformation user1 = UserGroupInformation.createUserForTesting("user1", noGroups); + UserGroupInformation leakedUser = UserGroupInformation.createUserForTesting("leakedUser", noGroups); + + Configuration conf = new Configuration(false); + conf.set(TezConfiguration.TEZ_AM_VIEW_ACLS, user1.getShortUserName()); + ACLManager amAclManager = new ACLManager(amUser.getShortUserName(), conf); + + ACLManager dagAclManager = + new ACLManager(amAclManager, dagUser.getShortUserName(), ACLInfo.getDefaultInstance()); + + // Reach into the DAG manager and mutate the AM_VIEW_ACL set in place. + // With a deep copy this only affects the DAG manager; with a shallow + // copy the AM manager would see the change too. + Field usersField = ACLManager.class.getDeclaredField("users"); + usersField.setAccessible(true); + @SuppressWarnings("unchecked") + Map> dagUsers = (Map>) usersField.get(dagAclManager); + dagUsers.get(ACLType.AM_VIEW_ACL).add(leakedUser.getShortUserName()); + + // The AM manager must NOT have picked up the new user. + assertFalse(amAclManager.checkAMViewAccess(leakedUser)); + } + /** * The per-DAG constructor must tolerate a null ACLInfo (the disabled-ACLs * test above already relies on this via a different code path, but with