[SSSD] [PATCH] AD: Add user as a direct member of his primary group

Sumit Bose sbose at redhat.com
Wed Jan 9 16:43:15 UTC 2013


On Wed, Jan 09, 2013 at 03:37:17PM +0100, Jakub Hrozek wrote:
> On Wed, Jan 09, 2013 at 02:32:41PM +0100, Sumit Bose wrote:
> > On Sun, Jan 06, 2013 at 02:44:14PM +0100, Jakub Hrozek wrote:
> > > There is one quirk that comes with using tokenGroups - the user is
> > > reported as a member of groups that are parent groups of his primary
> > > SID/GID as well. These group memberships are reflected in the database
> > > with member/memberof links.
> > > 
> > > Later on, when the groups are resolved using LDAP calls (for instance
> > > during id $user or simply getent group), the member/memberof links are
> > > lost and not established until another initgroups call.
> > > 
> > > The attached patch changes the behaviour of the AD provider (and AD
> > > provider only) such that when saving groups, a direct member link is
> > > also established between the user and the group with GID same as user's
> > > primary GID number. Then, the ghost/member links are propagated in the
> > > hierarchy.
> > > 
> > > There is one unwanted side-effect of this change - the user would be
> > > returned as a member of his primary group, too, when running
> > > getgrnam(primary-group). If these patches were accepted, we'd have to
> > > document this change in a manual page.
> > 
> > Please find my comments below.
> > 
> 
> Thanks for the review!
> 
> > bye,
> > Sumit
> > 
> > > From 1d4a7a73a4591fddd7086e7fe86d28685718553e Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek <jhrozek at redhat.com>
> > > Date: Sun, 6 Jan 2013 16:04:54 +0100
> > > Subject: [PATCH] AD: Add user as a direct member of his primary group
> > > 
> > > In the AD case, deployments sometimes add users as parents of the
> > 
> > *add users _in_ parents ?
> 
> add groups as parents of the primary group :-)
> 
> > 
> > > primary GID group. These groups are then returned during initgroups
> > > in the tokenGroups attribute and member/memberof links are established
> > > between the user and the group. However, any update of these groups
> > > would remove the links, so a sequence of calls: id -G user; id user; id
> > > -G user would return different group memberships.
> > > 
> > > The downside of this approach is that the user is returned as a group
> > > member during getgrgid call as well.
> > > ---
> > >  src/providers/ldap/sdap_async_groups.c | 118 ++++++++++++++++++++++++++++++---
> > >  1 file changed, 110 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
> > > index e1e84c339618c5fded3a28693b99b2a27058c05a..8508417fbde689923374dbd0cd153b4ecb86fd86 100644
> > > --- a/src/providers/ldap/sdap_async_groups.c
> > > +++ b/src/providers/ldap/sdap_async_groups.c
> > > @@ -92,13 +92,97 @@ done:
> > >      return ret;
> > >  }
> > >  
> > > +static errno_t
> > > +sdap_get_members_by_gid(TALLOC_CTX *mem_ctx, struct sysdb_ctx *sysdb,
> > > +                        gid_t gid, char ***_localdn, size_t *_ndn)
> > 
> > I would make it more clear that this call only returns  group members
> > where the primary gid matches, maybe sdap_get_members_with_primary_gid?
> 
> Done
> 
> > 
> > > +{
> > > +    static const char *search_attrs[] = { SYSDB_NAME, NULL };
> > > +    char *filter;
> > > +    struct ldb_message **msgs;
> > > +    size_t count;
> > > +    size_t i;
> > > +    errno_t ret;
> > > +    char **localdn;
> > > +
> > > +    /* Don't search if the group is non-posix */
> > > +    if (!gid) return EOK;
> > > +
> > > +    filter = talloc_asprintf(mem_ctx, "(%s=%llu)", SYSDB_GIDNUM,
> > > +                             (unsigned long long) gid);
> > > +    if (!filter) {
> > > +        return ENOMEM;
> > > +    }
> > > +
> > > +    ret = sysdb_search_users(mem_ctx, sysdb, filter,
> > > +                             search_attrs, &count, &msgs);
> > > +    talloc_free(filter);
> > > +    if (ret && ret != ENOENT) {
> > > +        return ret;
> > > +    }
> > 
> > You should handle ENOENT, because I think count is not set in this case.
> > 
> 
> Done. Thank you, this would be a potential segfault. I tested this
> codepath carefully this time.
> 
> > > +
> > > +    localdn = talloc_array(mem_ctx, char *, count);
> > > +    if (!localdn) {
> > > +        talloc_free(msgs);
> > > +        return ENOMEM;
> > > +    }
> > > +
> > > +    for (i=0; i < count; i++) {
> > > +        localdn[i] = talloc_strdup(localdn,
> > > +                                   ldb_dn_get_linearized(msgs[i]->dn));
> > > +        if (!localdn[i]) {
> > > +            talloc_free(localdn);
> > > +            talloc_free(msgs);
> > > +            return ENOMEM;
> > > +        }
> > > +    }
> > > +
> > > +    talloc_free(msgs);
> > > +    *_localdn = localdn;
> > > +    *_ndn = count;
> > > +    return EOK;
> > > +}
> > > +
> > > +static errno_t
> > > +sdap_dn_by_primary_gid(TALLOC_CTX *mem_ctx, struct sysdb_attrs *ldap_attrs,
> > > +                       struct sysdb_ctx *sysdb, struct sdap_options *opts,
> > > +                       char ***_dns, size_t *_count)
> > 
> > minor: _dns is a bit irritating, maybe _dn_list or userdns would be more clear?
> > 
> 
> Done
> 
> > > +{
> > > +    gid_t gid;
> > > +    errno_t ret;
> > > +
> > > +    if (opts->schema_type != SDAP_SCHEMA_AD) {
> > > +        *_dns = NULL;
> > > +        *_count = 0;
> > > +        return EOK;
> > > +    }
> > > +
> > > +    ret = sysdb_attrs_get_uint32_t(ldap_attrs,
> > > +                                   opts->group_map[SDAP_AT_GROUP_GID].name,
> > > +                                   &gid);
> > 
> > I always forget what right here, but shouldn't it be .sys_name here?
> > Please check.
> > 
> 
> Yes you're right. Fixed!
> 
> > 
> > > +    if (ret == ENOENT) {
> > > +        /* Non-posix AD group. Skip. */
> > > +        *_dns = NULL;
> > > +        *_count = 0;
> > > +        return EOK;
> > > +    } else if (ret && ret != ENOENT) {
> > > +        return ret;
> > > +    }
> > > +
> > > +    ret = sdap_get_members_by_gid(mem_ctx, sysdb, gid,
> > > +                                  _dns, _count);
> > > +    if (ret) return ret;
> > > +
> > > +    return EOK;
> > > +}
> > > +
> > >  static int sdap_fill_memberships(struct sysdb_attrs *group_attrs,
> > >                                   struct sysdb_ctx *ctx,
> > > -                                 struct sdap_options *opts,
> > >                                   struct sss_domain_info *domain,
> > >                                   hash_table_t *ghosts,
> > >                                   struct ldb_val *values,
> > > -                                 int num_values)
> > > +                                 int num_values,
> > > +                                 char **userdns,
> > > +                                 size_t nuserdns)
> > >  {
> > >      struct ldb_message_element *el;
> > >      int i, j;
> > > @@ -114,13 +198,12 @@ static int sdap_fill_memberships(struct sysdb_attrs *group_attrs,
> > >  
> > >      /* Just allocate both big enough to contain all members for now */
> > >      el->values = talloc_realloc(group_attrs, el->values, struct ldb_val,
> > > -                                el->num_values + num_values);
> > > +                                el->num_values + num_values + nuserdns);
> > >      if (!el->values) {
> > >          ret = ENOMEM;
> > >          goto done;
> > >      }
> > >  
> > > -    /* Just allocate both big enough to contain all members for now */
> > >      j = el->num_values;
> > >      for (i = 0; i < num_values; i++) {
> > >          if (ghosts == NULL) {
> > > @@ -159,6 +242,13 @@ static int sdap_fill_memberships(struct sysdb_attrs *group_attrs,
> > >      }
> > >      el->num_values = j;
> > >  
> > > +    for (i=0; i < nuserdns; i++) {
> > > +        el->values[el->num_values + i].data = (uint8_t *) \
> > > +                                          talloc_steal(group_attrs, userdns[i]);
> > > +        el->values[el->num_values + i].length = strlen(userdns[i]);
> > > +    }
> > > +    el->num_values += nuserdns;
> > > +
> > >      ret = EOK;
> > >  
> > >  done:
> > > @@ -555,6 +645,8 @@ static int sdap_save_grpmem(TALLOC_CTX *memctx,
> > >      struct ldb_message_element *el;
> > >      struct sysdb_attrs *group_attrs = NULL;
> > >      const char *name;
> > > +    char **userdns;
> > > +    size_t nuserdns;
> > 
> > You should initialize userdns and nuserdns to avoid warnings.
> > 
> 
> Done
> 
> > >      int ret;
> > >  
> > >      ret = sysdb_attrs_primary_name(ctx, attrs,
> > > @@ -564,12 +656,22 @@ static int sdap_save_grpmem(TALLOC_CTX *memctx,
> > >          goto fail;
> > >      }
> > >  
> > > +    /* With AD we also want to merge in parent groups of primary GID as they
> > > +     * are reported with tokenGroups, too
> > > +     * FIXME - only do this if tokenGroups is on?
> > > +     */
> > > +    ret = sdap_dn_by_primary_gid(memctx, attrs, ctx, opts,
> > > +                                 &userdns, &nuserdns);
> > > +    if (ret != EOK) {
> > > +        goto fail;
> > > +    }
> > > +
> > 
> > nitpick: the comment says that this is only relevant for AD, but it is
> > not clear from the code here. I would recommend to either move the
> > "opts->schema_type != SDAP_SCHEMA_AD" check here or move the AD specific
> > part of the comment to sdap_dn_by_primary_gid().
> > 
> 
> OK, initially I wanted to avoid any more nesting in sdap_save_grpmem(),
> but it's better to make it clear when the function has any sense, so I
> put the condition one level above.
> 
> 
> New patches attached.

Patches are looking good and testing went well together with Patch "AD:
replace GID/UID, do not add another one"

ACK

bye,
Sumit



More information about the sssd-devel mailing list