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

Sumit Bose sbose at redhat.com
Wed Jan 9 13:32:41 UTC 2013


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.

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 ?

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

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

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

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


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

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

>      ret = sysdb_attrs_get_el(attrs,
>                      opts->group_map[SDAP_AT_GROUP_MEMBER].sys_name, &el);
>      if (ret != EOK) {
>          goto fail;
>      }
> -    if (el->num_values == 0) {
> +    if (el->num_values == 0 && nuserdns == 0) {
>          DEBUG(7, ("No members for group [%s]\n", name));
>  
>      } else {
> @@ -581,9 +683,9 @@ static int sdap_save_grpmem(TALLOC_CTX *memctx,
>              goto fail;
>          }
>  
> -        ret = sdap_fill_memberships(group_attrs, ctx, opts, dom,
> -                                    ghosts,
> -                                    el->values, el->num_values);
> +        ret = sdap_fill_memberships(group_attrs, ctx, dom, ghosts,
> +                                    el->values, el->num_values,
> +                                    userdns, nuserdns);
>          if (ret) {
>              goto fail;
>          }
> -- 
> 1.8.0.2
> 

> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list