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 <o...@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 <o...@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 { _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
