[PATCHES] AD: Enable TokenGroups initgroups lookup

[Ondrej Kos]

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]

On 09/04/2013 09:34 AM, 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
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

I forgot to mention, that this patch requires the sid group lookup patch 
(already on the list)

Ondra

[Jakub Hrozek]

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 
> 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 
> 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 {

[Ondrej Kos]

New patches are attached.

[PATCH 1/3] LDAP: Make sdap_add_incomplete_groups public

The function is needed for latter patch, when groups are added from ad 
initgroups module.

[PATCH 2/3] AD: Enable TokenGroups initgroups lookup

Prepares the list of SIDs that are not in cache and updates members of 
groups which are found

[PATCH 3/3] AD: Add lookup for SIDs not found in cache during
  TokenGroups

Performs LDAPsearch with list of tokengroups SIDs that are missing from 
sysdb and stores them.

Ondra