Copilot commented on code in PR #8492:
URL: https://github.com/apache/hadoop/pull/8492#discussion_r3230897473
##########
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/test/java/org/apache/hadoop/mapreduce/v2/app/security/authorize/TestTaskLevelSecurityEnforcer.java:
##########
@@ -128,6 +147,54 @@ public void testJobConfigCanNotOverwriteMapreduceConfig() {
assertDenied(mapreduceConf);
}
+ @Test
+ public void testAllowedGroup() {
+ UserGroupInformation.createUserForTesting("alice",
+ new String[] {"hadoop"});
+ JobConf conf = jobConfForSubmitUser("alice");
+ conf.setBoolean(MRConfig.MAPREDUCE_TASK_SECURITY_ENABLED, true);
+ conf.setStrings(MRConfig.SECURITY_DENIED_TASKS,
"org.apache.hadoop.streaming");
+ conf.setStrings(MRConfig.SECURITY_ALLOWED_GROUPS, "hadoop");
+ conf.set(MRJobConfig.MAP_CLASS_ATTR,
"org.apache.hadoop.streaming.PipeMapper");
+ assertPass(conf);
+ }
+
+ @Test
+ public void testDeniedGroup() {
+ UserGroupInformation.createUserForTesting("bob",
+ new String[] {"other"});
+ JobConf conf = jobConfForSubmitUser("bob");
+ conf.setBoolean(MRConfig.MAPREDUCE_TASK_SECURITY_ENABLED, true);
+ conf.setStrings(MRConfig.SECURITY_DENIED_TASKS,
"org.apache.hadoop.streaming");
+ conf.setStrings(MRConfig.SECURITY_ALLOWED_GROUPS, "hadoop");
+ conf.set(MRJobConfig.MAP_CLASS_ATTR,
"org.apache.hadoop.streaming.PipeMapper");
+ assertDenied(conf);
Review Comment:
The new allowed-groups bypass logic can throw if the submitter username is
missing/blank (non-secure mode). There isn’t a regression test covering that
scenario (e.g., `mapreduce.security.allowed-groups` configured but
`mapreduce.job.user.name` unset) to ensure validation fails deterministically
instead of raising an `IllegalArgumentException`. Adding such a test would help
prevent this from recurring.
##########
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/security/authorize/TaskLevelSecurityEnforcer.java:
##########
@@ -92,6 +95,19 @@ public static void validate(JobConf conf,
UserGroupInformation currentUser)
return;
}
+ String[] allowedGroupNames = conf.getTrimmedStrings(
+ MRConfig.SECURITY_ALLOWED_GROUPS,
+ MRConfig.DEFAULT_SECURITY_ALLOWED_GROUPS);
+ if (allowedGroupNames.length > 0) {
+ UserGroupInformation submitterUgi =
+ UserGroupInformation.createRemoteUser(currentUserName);
+ if (isUserInAllowedGroups(submitterUgi, allowedGroupNames)) {
+ LOG.debug("The {} is allowed to execute every task via allowed-groups",
+ currentUserName);
+ return;
Review Comment:
`currentUserName` can be null/empty in non-secure mode if
`mapreduce.job.user.name` is missing from the JobConf. When
`mapreduce.security.allowed-groups` is configured, this path calls
`UserGroupInformation.createRemoteUser(currentUserName)` which throws
`IllegalArgumentException("Null user")`, turning a policy check into an
unexpected runtime failure. Please guard against missing/blank submitter names
(e.g., skip group lookup, fall back to `currentUser.getShortUserName()`, or
fail with a controlled `TaskLevelSecurityException`/clear error).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
