On Wed, Sep 04, 2013 at 09:34:25AM +0200, Ondrej Kos wrote:
Hi,
Attached find two patches for issue
https://fedorahosted.org/sssd/ticket/1568
[PATCH 1/2]: LDAP: move sdap_get_initgr_state structure to private
header
- moves the initgr state structure to private header so it can be
used also in the ad initgroups module.
[PATCH 2/2] AD: Enable TokenGroups initgroups lookup
This is first implementation of getting TokenGroups lookup working.
If all of the group SIDs that are fetched via the users TokenGroups
attribute are in sysdb, the membership is processed this way. If any of
the groups is missing in the cache, it falls back to rfc2307bis
I'd file a ticket to enhance this to look up only groups which are
missing in the sysdb, as we talked about with Jakub.
Ondra
--
Ondrej Kos
Associate Software Engineer
Identity Management - SSSD
Red Hat Czech
From: Ondrej Kos <okos(a)redhat.com>
Date: Tue, 3 Sep 2013 13:20:41 +0200
Subject: [PATCH 1/2] LDAP: move sdap_get_initgr_state structure to private
ACK
From e28b760ac18f0c51ee6cdfc65f9c69abfe9bd126 Mon Sep 17 00:00:00
2001
From: Ondrej Kos <okos(a)redhat.com>
Date: Tue, 3 Sep 2013 12:27:17 +0200
Subject: [PATCH 2/2] AD: Enable TokenGroups initgroups lookup
This is first implementation of getting TokenGroups lookup working.
If all of the group SIDs that are fetched via the users TokenGroups
attribute are in sysdb, the membership is processed this way. If any of
the groups is missing in the cache, it falls back to rfc2307bis
initgroups lookup.
Resolves:
https://fedorahosted.org/sssd/ticket/1568
---
src/providers/ldap/sdap_async.h | 1 +
src/providers/ldap/sdap_async_initgroups.c | 64 ++++++++++++++++++++----
src/providers/ldap/sdap_async_initgroups_ad.c | 71 +++++++++++++++++----------
src/providers/ldap/sdap_async_private.h | 2 +
4 files changed, 103 insertions(+), 35 deletions(-)
diff --git a/src/providers/ldap/sdap_async.h b/src/providers/ldap/sdap_async.h
index c8031c9a9d527a6d808f1ddce096de23850ebfd6..432a561237fa70075b9c20609a5d661078d53019
100644
--- a/src/providers/ldap/sdap_async.h
+++ b/src/providers/ldap/sdap_async.h
@@ -279,6 +279,7 @@ sdap_get_ad_match_rule_initgroups_recv(struct tevent_req *req);
struct tevent_req *
sdap_get_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
+ void *istate,
nack, sorry. Poking into another requests's state is not an acceptable
way of passing data between requests. You need to extend the
sdap_get_ad_tokengroups_initgroups_recv function with another output
parameter that indicates whether the tokenGroups were found.
And even if it was, it should never be a void pointer, but a pointer to
the type itself.
The state is a structure private to the request and should be completely
opaque outside the request.
struct tevent_context *ev,
struct sdap_options *opts,
struct sysdb_ctx *sysdb,
diff --git a/src/providers/ldap/sdap_async_initgroups.c
b/src/providers/ldap/sdap_async_initgroups.c
index 315834153408ebc8b5e4837ea51b26460dc0469a..163654023c7aa7ea140950452302f03eb0206486
100644
--- a/src/providers/ldap/sdap_async_initgroups.c
+++ b/src/providers/ldap/sdap_async_initgroups.c
@@ -2571,8 +2571,10 @@ struct tevent_req *sdap_get_initgr_send(TALLOC_CTX *memctx,
state->name = name;
state->grp_attrs = grp_attrs;
state->orig_user = NULL;
+ state->cname = NULL;
state->timeout = dp_opt_get_int(state->opts->basic, SDAP_SEARCH_TIMEOUT);
state->user_base_iter = 0;
+ state->failed_tokengroups = false;
I would prefer a different name, there is nothing "failed" if we can't
find the SIDs groups in cache. Something like "cached_sids" or
"refresh_sids" etc.
state->user_search_bases = sdom->user_search_bases;
if (!state->user_search_bases) {
DEBUG(SSSDBG_CRIT_FAILURE,
@@ -2745,13 +2747,16 @@ static void sdap_get_initgr_user(struct tevent_req *subreq)
return;
}
+ state->cname = cname;
+ talloc_steal(state, cname);
+
No need to steal the pointer, you can simply assign right to
state->cname.
DEBUG(9, ("Process user's groups\n"));
switch (state->opts->schema_type) {
case SDAP_SCHEMA_RFC2307:
subreq = sdap_initgr_rfc2307_send(state, state->ev, state->opts,
state->sysdb, state->dom, state->sh,
- cname);
+ state->cname);
if (!subreq) {
tevent_req_error(req, ENOMEM);
return;
@@ -2851,6 +2861,40 @@ static void sdap_get_initgr_done(struct
tevent_req *subreq)
char *dom_sid_str;
char *group_sid_str;
struct sdap_options *opts = state->opts;
+ const char *orig_dn;
+
+ if (state->failed_tokengroups) {
+ DEBUG(SSSDBG_MINOR_FAILURE, ("TokenGroups lookup failed, falling "
Same comment about failed. Also, it's not a minor failure, just some
tracing info.
+ "back to rfc2307bis initgroups
call.\n"));
+
+ state->failed_tokengroups = false;
+
+ ret = sysdb_attrs_get_string(state->orig_user,
+ SYSDB_ORIG_DN,
+ &orig_dn);
+ if (ret != EOK) {
+ tevent_req_error(req, ret);
+ return;
+ }
+
+ subreq = sdap_initgr_rfc2307bis_send(state, state->ev,
+ state->opts,
+ state->sysdb,
+ state->dom,
+ state->sh,
+ state->cname,
+ orig_dn);
+
+ if (!subreq) {
+ tevent_req_error(req, ENOMEM);
+ return;
+ }
+
+ talloc_steal(subreq, orig_dn);
Why do you steal the orig_dn onto the subreq?
+ tevent_req_set_callback(subreq, sdap_get_initgr_done, req);
+
+ return;
+ }
DEBUG(9, ("Initgroups done\n"));
diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c
b/src/providers/ldap/sdap_async_initgroups_ad.c
index e5649a2b9793253a55c48fba69cd3a902c0dc967..d4f598db1195f635cf773ab55828a92c0dfcc1eb
100644
--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -305,6 +305,7 @@ struct sdap_ad_tokengroups_initgr_state {
struct sss_domain_info *domain;
struct sdap_handle *sh;
const char *username;
+ struct sdap_get_initgr_state *istate;
};
static void
@@ -312,6 +313,7 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
*req);
struct tevent_req *
sdap_get_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
+ void *istate,
struct tevent_context *ev,
struct sdap_options *opts,
struct sysdb_ctx *sysdb,
@@ -336,6 +338,7 @@ sdap_get_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
state->domain = domain;
state->sh = sh;
state->username = name;
+ state->istate = (struct sdap_get_initgr_state *)istate;
subreq = sdap_get_generic_send(
state, state->ev, state->opts, state->sh,
@@ -464,20 +467,29 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
*subreq)
DEBUG(SSSDBG_TRACE_FUNC, ("Skipping built-in object.\n"));
ret = EOK;
continue;
- } else if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE,
- ("Could not convert SID to GID: [%s]. Skipping\n",
- strerror(ret)));
- continue;
}
- DEBUG(SSSDBG_TRACE_LIBS,
- ("Processing membership GID [%lu]\n",
- gid));
+ if (state->istate->use_id_mapping) {
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Could not convert SID to GID: [%s]. Skipping\n",
+ strerror(ret)));
+ continue;
+ }
+
+ DEBUG(SSSDBG_TRACE_LIBS,
+ ("Processing membership GID [%lu]\n",
+ gid));
+
+ /* Check whether this GID already exists in the sysdb */
+ ret = sysdb_search_group_by_gid(tmp_ctx, state->sysdb, state->domain,
+ gid, attrs, &msg);
+ } else {
+ ret = sysdb_search_group_by_sid_str(tmp_ctx, state->sysdb,
+ state->domain, sid_str, attrs,
+ &msg);
+ }
- /* Check whether this GID already exists in the sysdb */
- ret = sysdb_search_group_by_gid(tmp_ctx, state->sysdb, state->domain,
- gid, attrs, &msg);
if (ret == EOK) {
group_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
if (!group_name) {
@@ -487,20 +499,29 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req
*subreq)
goto done;
}
} else if (ret == ENOENT) {
- /* This is a new group. For now, we will store it
- * under the name of its SID. When a direct lookup of
- * the group or its GID occurs, it will replace this
- * temporary entry.
- */
- group_name = sid_str;
- ret = sysdb_add_incomplete_group(state->sysdb,
- state->domain,
- group_name, gid,
- NULL, sid_str, false, now);
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE,
- ("Could not create incomplete group: [%s]\n",
- strerror(ret)));
+ if (state->istate->use_id_mapping) {
+ /* This is a new group. For now, we will store it
+ * under the name of its SID. When a direct lookup of
+ * the group or its GID occurs, it will replace this
+ * temporary entry.
+ */
+ group_name = sid_str;
+ ret = sysdb_add_incomplete_group(state->sysdb,
+ state->domain,
+ group_name, gid,
+ NULL, sid_str, false, now);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Could not create incomplete group: [%s]\n",
+ strerror(ret)));
+ goto done;
+ }
+ } else {
+ DEBUG(SSSDBG_MINOR_FAILURE, ("SID [%s] not in cache, "
+ "performing fallback.\n", sid_str));
+
+ state->istate->failed_tokengroups = true;
+ ret = EOK;
It would maybe also be nice if we could return a list of SIDs missing
from the cache and only look up those that are missing. But this can be
a follow-up patch.
goto done;
}
} else {