This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] nsswitch: Add group merging support


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/29/2015 04:58 PM, Mike Frysinger wrote:
> On 16 Dec 2015 10:11, Stephen Gallagher wrote:
>> == Justification == It is common today for users to rely on
>> centrally-managed user stores for handling their user accounts.
>> However, much software existing today does not have an innate
>> understanding of such accounts. Instead, they commonly rely on
>> membership in known groups for managing access-control (for 
>> example the "wheel" group on Fedora and RHEL systems or the "adm"
>> group on Debian-derived systems). In the present incarnation of
>> nsswitch, the only way to have such groups managed by a remote
>> user store such as FreeIPA or Active Directory would be to
>> manually remove the groups from /etc/group on the clients so that
>> nsswitch would then move past nss_files and into the SSSD,
>> nss-ldap or other remote user database.
> 
> you've lost me.  the whole point of nsswitch.conf is to let the
> admin explicitly control the order and precedence of look up
> sources.  so if you want to look up other sources, fix your
> /etc/nsswitch.conf to list the remote sources first over
> /etc/groups.
> 

I think I did lose you, yes. The fundamental problem here is that
glibc/nsswitch assumes that one and only one database in the list is
always authoritative. This does not match reality. The most common
example is the 'wheel' group on Red Hat-derived systems or the 'adm'
group on Debian-derived systems. These groups are shipped by default
in the installation and may have users added to them by the OS
installer (for example, it is very common for an OS installer to add a
local system administrator; someone who has infinite sudo privilege).

However, in many non-single-user systems this machine will later be
added to a domain such as FreeIPA or Active Directory. In this
situation, it is rarely desirable that the local administrators be
locked out of the machine by ordering the domain groups before the
local groups. (In particular, this can make debugging of domain client
issues difficult or impossible if there is no root password, such as
on Ubuntu).

The purpose of this patch is to allow group memberships to be merged,
such that domain members are added as supplemental to the local members.


>> == Solution == With this patch, a new action is introduced for
>> nsswitch: NSS_ACTION_MERGE. To take advantage of it, one will add
>> [SUCCESS=merge] between two database entries in the nsswitch.conf
>> file. When a group is located in the first of the two group
>> entries, processing will continue on to the next one. If the
>> group is also found in the next entry (and the group name and GID
>> are an exact match), the member list of the second entry will be
>> added to the group object to be returned.
> 
> what if group/gid do not match ?  the edge cases must be
> explicitly documented in the manual.
> 

Huh, odd. I think somehow I accidentally dropped that paragraph during
editing. I did originally state that it was explicitly an undefined
result (in reality, the merging code checks this and treats the second
entry safely as NOTFOUND, but this shouldn't be documented as API
since it may still have unexpected and different behavior between
getgrgid() and getgrnam() lookups)

I'll submit a corrected manpage as a follow-up to this email.



>> I performed testing by running the getent utility against my
>> newly-built glibc and configuring /etc/nsswitch.conf with the
>> following entry:
> 
> what will it take to get tests into glibc itself ?  ad-hoc testing
> is a great way for code to rot.
> 

I'm working with Carlos O'Donnell to develop tests for this in
parallel. I agree, we don't want this to bitrot. We decided that it
wasn't strictly necessary to submit them at the same time.


>> +  len = sizeof(char) * (strlen (srcgrp.gr_name) + 1);
> 
> space before the ( ... this comes up a few times in this func
> 

Sorry, that was ambiguous. Were you telling me that it should be:

len = sizeof (char) * (strlen (srcgrp.gr_name) + 1);
or
len = sizeof(char) * (strlen(srcgrp.gr_name) + 1);


I'm guessing it is supposed to be the former and will resubmit the
patch accordingly.


>> +  if (members == NULL) +    { +      return ENOMEM; +    }
> 
> no need for braces in single statements like this.  comes up a few
> times in your patch.
> 

This is a personal habit of mine; In the past, I've had many bugs
creep into code due to a patch merge resulting in an un-bracketed if
statement with two lines. (A popular example of this would be Apple's
goto fail; bug from a while back).

So I always include braces for future peace of mind. If glibc insists
upon this for aesthetic reasons, I will acquiesce, but I don't really
understand it.


>> --- /dev/null +++ b/grp/grp-merge.h @@ -0,0 +1,40 @@
>> 
>> +/* __copy_grp: +   Duplicate a grp struct (and its members). +
>> When no longer needed, the calling function +   must
>> free(newbuf). + */
> 
> the comment style is incorrect.  should be: /* Duplicate a grp
> struct (and its members).  When no longer needed, the calling 
> function must free the newbuf.  */
> 

Fixed.

>> +/* __merge_grp: +   Merge the member lists of two grp structs
>> together. + */
> 
> same here
> 

Fixed.

>> --- a/nss/getXXbyYY_r.c +++ b/nss/getXXbyYY_r.c
>> 
>> +__copy_einval(LOOKUP_TYPE a,
> 
> space before the (
> 

Fixed.

>> +__merge_einval(LOOKUP_TYPE *a,
> 
> same here
> 

Fixed.

>> +#define CHECK_MERGE(err, status)	\ +do {					\ +  if (err)				\ 
>> +    {					\ +      __set_errno (err);		\ +      if (err ==
>> ERANGE)		\ +        {				\ +          status =
>> NSS_STATUS_TRYAGAIN;	\ +      }					\ +      else				\ +        {
>> \ +          status = NSS_STATUS_UNAVAIL;	\ +        }				\
> 
> indentation is wrong w/braces in the inside if block, but you
> should just drop them all -mike
> 

Fixed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlaKfTYACgkQeiVVYja6o6PFXwCfV9WT/JUfWM8mUdNQxDOh51NE
pxcAmwXYg7rvmC/MXslFwOuVRabmVVDB
=Kpaw
-----END PGP SIGNATURE-----


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]