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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel