Details

Description

Currently the IPC client sends the UGI which contains the user/group information for the Server. However this represents the groups for the user on the client-end. The more pertinent mapping from user to groups is actually the one seen by the Server. Hence the client should only send the user and we should add a 'group mapping service' so that the Server can query it for the mapping.

Activity

HADOOP-4348 is switching IPC to use the JAAS Subject rather than UGI (which will become an internal artifact). While we are adding the user-to-group mapping service, I propose we change the IPC Client to send the JAAS Subject in the header rather than UGI, this will also be compatible with the way we will do Kerberos-based authentication via the GSS API.

Arun C Murthy
added a comment - 14/Nov/08 17:38 HADOOP-4348 is switching IPC to use the JAAS Subject rather than UGI (which will become an internal artifact). While we are adding the user-to-group mapping service, I propose we change the IPC Client to send the JAAS Subject in the header rather than UGI, this will also be compatible with the way we will do Kerberos-based authentication via the GSS API.

Arun C Murthy
added a comment - 08/Jan/09 08:01 I propose a new abstract class Groups with a single method 'getGroups' as below:
Groups.java
public abstract class Groups {
List< String > getGroups( String username);
}
with a concrete implementation which gets the unix groups for the given user.

> I propose we change the IPC Client to send the JAAS Subject in the header rather than UGI, this will also be compatible with the way we will do Kerberos-based authentication via the GSS API.

Just want to clarify that application code doesn't send anything when using Kerberos. It's all hiding inside the GSS API library. After authentication, server can query the established GSS context to get client ID as GSSName which can be converted to a String. So for compatibility, IPC Client doesn't have to send JAAS Subject in the header. Send a String is fine.

Kan Zhang
added a comment - 08/Jan/09 18:32 > I propose we change the IPC Client to send the JAAS Subject in the header rather than UGI, this will also be compatible with the way we will do Kerberos-based authentication via the GSS API.
Just want to clarify that application code doesn't send anything when using Kerberos. It's all hiding inside the GSS API library. After authentication, server can query the established GSS context to get client ID as GSSName which can be converted to a String. So for compatibility, IPC Client doesn't have to send JAAS Subject in the header. Send a String is fine.

Groups should definitely come from asking the host OS in some form using the Java equivalent of getgrent() and friends. [ Be aware that getgroups() is BSD-specific and may not be available on System V, such as Solaris and HP-UX.] Doing this via shell call out is just going to exasperate the memory problems we already see, especially on the secondary name node that requires more memory than the primary due to the fork of whoami/id!

It also opens up yet another security hole where any random groups command on the name nodes path can be used to override. Not Good(tm).

Allen Wittenauer
added a comment - 12/Jan/09 19:51 Groups should definitely come from asking the host OS in some form using the Java equivalent of getgrent() and friends. [ Be aware that getgroups() is BSD-specific and may not be available on System V, such as Solaris and HP-UX.] Doing this via shell call out is just going to exasperate the memory problems we already see, especially on the secondary name node that requires more memory than the primary due to the fork of whoami/id!
It also opens up yet another security hole where any random groups command on the name nodes path can be used to override. Not Good(tm).

One of the big advantages of talking to the OS is that many systems include a naming services caching daemon that handles caching group and similar content for the entire machine. nscd generally includes great support for controlling the size, ttl, negative ttl, etc, for the cache. Duplicating that functionality seems like overkill and, worse, will act as a cache against a cache!

Allen Wittenauer
added a comment - 13/Jan/09 17:37 Privately, someone asked about caching the group content.
One of the big advantages of talking to the OS is that many systems include a naming services caching daemon that handles caching group and similar content for the entire machine. nscd generally includes great support for controlling the size, ttl, negative ttl, etc, for the cache. Duplicating that functionality seems like overkill and, worse, will act as a cache against a cache!

FROHNER Ákos
added a comment - 14/Sep/09 15:51 Please consider passing the authentication context to the getGroups() method,
as it might be easier to retrieve the associated groups using that information,
then based only on the username.
For example in POSIX environments it is faster to do a lookup based on the
numeric UID, than based on the username.
If you are using Kerberos with PAC, then the authentication context may already
contain a list of associated groups:
http://k5wiki.kerberos.org/wiki/Projects/PAC_and_principal_APIs
There is a similar solution based on X509 authentication, where the associated
list of groups is embedded into the authentication context.

Allen Wittenauer
added a comment - 14/Sep/09 17:59 AFAIK, Hadoop has no concept of uid, as everything in the HDFS, etc, is stored as a string. So the username is about all the context you can probably get.

This patch will create two versions of SecurityUtil.getSubject. One that builds list of group principles from UGI group list and another one that builds the list from UNIX id command. Do we really need the first one? I suggest we remove it.

Boris Shkolnik
added a comment - 21/Nov/09 00:43 This patch will create two versions of SecurityUtil.getSubject. One that builds list of group principles from UGI group list and another one that builds the list from UNIX id command. Do we really need the first one? I suggest we remove it.

Jakob Homan
added a comment - 02/Dec/09 01:59 Reviewed patch:
Nit: Calling an abstract class GroupMappingImpl seems a bit odd, even if it is technically correct for this. Service provider, maybe?
In Groups.java the previous timer-based code is still present, but commented out. Needs removed.
Note: HADOOP-6299 , if added as-is from the draft posted, will introduce code duplication in terms of executing the shell. When that code is reviewed, we should try to eliminate that.
In the unit test, principal is spelled as principle.
In the second-to-last line of the unit test, there is a spelling error of subject.
The provided unit test is very happy pathy. It'd be great if there were more testing of failures. Gary suggested testing what happens if we pass a user name that doesn't exist.

Of note, with the patch applied I'm seeing a time-out for TestIPC and a failure of TestIPCServerResponder:testServerResponder (null value encountered). I'm not seeing this without the patch applied. We should investigate why this is happening before submitting an updated patch.

Jakob Homan
added a comment - 02/Dec/09 02:38 Of note, with the patch applied I'm seeing a time-out for TestIPC and a failure of TestIPCServerResponder:testServerResponder (null value encountered). I'm not seeing this without the patch applied. We should investigate why this is happening before submitting an updated patch.

It may be a good idea to explicitly document that getGroups (both in GroupMappingServiceProvider and Groups) will return an empty list for a non-existent user. This avoids any future null-related confusion.

Comment on RefreshUserToGroupMappingProtocol seems incomplete

"just return the emptly list" - empty spelled incorrectly

The change to the exception message in setUserGroupNames no longer reflects the fact that userName shouldn't be a zero-length string

In TestUnixUserGroupInformation,

testConstructorFailures(USER_NAME, newString[0]); // valid case now

is commented out, and should be removed.

In the negative test, an extra assert that getPrincipals is zero-length wouldn't hurt.

Jakob Homan
added a comment - 16/Dec/09 20:06 Reviewed v6 of patch:
It may be a good idea to explicitly document that getGroups (both in GroupMappingServiceProvider and Groups) will return an empty list for a non-existent user. This avoids any future null-related confusion.
Comment on RefreshUserToGroupMappingProtocol seems incomplete
"just return the emptly list" - empty spelled incorrectly
The change to the exception message in setUserGroupNames no longer reflects the fact that userName shouldn't be a zero-length string
In TestUnixUserGroupInformation,
testConstructorFailures(USER_NAME, new String [0]); // valid case now
is commented out, and should be removed.
In the negative test, an extra assert that getPrincipals is zero-length wouldn't hurt.
fail( "fakeUser should have no grups" );
- groups is spelled incorrectly