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 <jhro...@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.
>From 24ccee7f88fa24fa9bf6cc5cd83318350cd2ba24 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@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 groups as parents of the
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 | 117 ++++++++++++++++++++++++++++++---
1 file changed, 109 insertions(+), 8 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c
b/src/providers/ldap/sdap_async_groups.c
index
e1e84c339618c5fded3a28693b99b2a27058c05a..ae335fc883ba41ce1c3748dbdbf99050975785c3
100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -92,13 +92,95 @@ done:
return ret;
}
+static errno_t
+sdap_get_members_with_primary_gid(TALLOC_CTX *mem_ctx, struct sysdb_ctx *sysdb,
+ gid_t gid, char ***_localdn, size_t *_ndn)
+{
+ 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 == ENOENT) {
+ *_localdn = NULL;
+ *_ndn = 0;
+ return EOK;
+ } else if (ret != EOK) {
+ return ret;
+ }
+
+ 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 ***_dn_list, size_t *_count)
+{
+ gid_t gid;
+ errno_t ret;
+
+ ret = sysdb_attrs_get_uint32_t(ldap_attrs,
+ opts->group_map[SDAP_AT_GROUP_GID].sys_name,
+ &gid);
+ if (ret == ENOENT) {
+ /* Non-posix AD group. Skip. */
+ *_dn_list = NULL;
+ *_count = 0;
+ return EOK;
+ } else if (ret && ret != ENOENT) {
+ return ret;
+ }
+
+ ret = sdap_get_members_with_primary_gid(mem_ctx, sysdb, gid,
+ _dn_list, _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 +196,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 +240,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 +643,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 = NULL;
+ size_t nuserdns = 0;
int ret;
ret = sysdb_attrs_primary_name(ctx, attrs,
@@ -564,12 +654,23 @@ 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
+ */
+ if (opts->schema_type == SDAP_SCHEMA_AD) {
+ ret = sdap_dn_by_primary_gid(memctx, attrs, ctx, opts,
+ &userdns, &nuserdns);
+ if (ret != EOK) {
+ goto fail;
+ }
+ }
+
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 +682,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@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
