[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