Re: [PATCH] nsswitch: Add group merging support

[Mike Frysinger <vapier at gentoo dot org>]

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.

> == 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.

> 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.

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

space before the ( ... this comes up a few times in this func

> +  if (members == NULL)
> +    {
> +      return ENOMEM;
> +    }

no need for braces in single statements like this.  comes up a few times
in your patch.

> --- /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.  */

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

same here

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

space before the (

> +__merge_einval(LOOKUP_TYPE *a,

same here

> +#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

Attachment: signature.asc
Description: Digital signature