[
https://issues.apache.org/jira/browse/HADOOP-12468?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15012956#comment-15012956
]
Yongjun Zhang commented on HADOOP-12468:
----------------------------------------
HI [~jojochuang],
Thanks much for the new rev and sorry for late review. It looks good except I
have some cosmetic comments:
* import statements should be alphabetically ordered
{code}
import org.apache.hadoop.util.Shell.ShellCommandExecutor;
import org.apache.commons.lang.StringUtils;
{code}
* It's better not to add new class member {{user}}, instead, use parameter
passing for it, like what the current implementation does.
* Better names for {{createShellCommandExecutor()}} /
{{createGroupIDExecutor()}} are {{createGroupExecutor()}} /
{{createGroupIDExecutor()}}
* Seems to me that it's better to inline {{getUnixGroupsWithCommandExecutor()}}
into {{getUnixGroups}} so not to have two methods here.
* Shorten the leading comment in resolvePartialGroupNames to (I think we don't
have to
mention TestJNIGroupsMapping here, you may add comment to this test code if
prefer):
{code}
// Exception may indicate that some group names are not resolvable. Shell-based
// implementation should tolerate unresolvable groups names, and return
resolvable
// ones, similar to what JNI-based implementation does.
{code}
* Better to include the "user" info in logs, for example, add user name to the
following exception (some other places too):
{code}
throw new PartialGroupNameException("failed to get group id list");
{code}
* add cause when throwing exception
change
{code}
throw new PartialGroupNameException("failed to get group id list");
{code}
to
{code}
throw new PartialGroupNameException("Failed to get group id list for " + user,
ece);
{code}
Similarly other places.
* Suggest to move the class {{PartialGroupNameException}} to earlier in the
code instead of at the end.
* In PartialGroupNameException, do something like (refer to
{{KerberosName#BadFormatString}}) :
{code}
@SuppressWarnings("serial")
private static class PartialGroupNameException extends IOException {
PartialGroupNameException(String msg) {
super(msg);
}
PartialGroupNameException(String msg, Throwable err) {
super(msg, err);
}
}
{code}
Thanks.
> Partial group resolution failure should not result in user lockout
> ------------------------------------------------------------------
>
> Key: HADOOP-12468
> URL: https://issues.apache.org/jira/browse/HADOOP-12468
> Project: Hadoop Common
> Issue Type: Bug
> Components: security
> Affects Versions: 2.6.1
> Environment: Linux
> Reporter: Wei-Chiu Chuang
> Assignee: Wei-Chiu Chuang
> Priority: Minor
> Attachments: HADOOP-12468.001.patch, HADOOP-12468.002.patch,
> HADOOP-12468.003.patch, HADOOP-12468.004.patch, HADOOP-12468.005.patch,
> HADOOP-12468.006.patch
>
>
> If a Hadoop cluster is configured to use ShellBasedUnixGroupsMapping for
> user/group name mapping, occasionally some group names may become
> unresolvable (for example, using SSSD).
> ShellBasedUnixGroupsMapping uses shell command "id -Gn" to retrieve the group
> name of a user; however, the existing logic assumes that if the exit code of
> the command is non-zero, the user has no group name at all. The shell command
> in Linux returns non-zero exit code if a group name is not resolvable.
> Unfortunately, it is possible that a user belongs to multiple groups, and any
> partial failure in group name resolution would denied the user's access.
> On the other hand, the JNI implementation (JniBasedUnixGroupsMapping) is more
> resilient. If any group name is unresolvable, it is simply ignored, and
> whatever are resolvable are returned.
> It is arguable that if the group name is not resolvable, the administrator
> should configure their directory/authentication service correctly, and Hadoop
> is in no position to handle it, but since the existing unit tests assume the
> output of JNI-based and shell-based implementation are the same, we should
> improve the shell-based group name resolution, and make it as resilient as
> the JNI-based one.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
