[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