[PATCH] IPA: add support for new extdom plugin version

[Sumit Bose]

Hi,

this patch adds support for the new extdom plugin version as described on
http://www.freeipa.org/page/V4/Extdom_plugin_enhancement_grouplist .

I will send the server side updates to the freeipa-devel list soon.
Nevertheless it would be nice if these changes can already be tested
against the old version to avoid regressions.

bye,
Sumit

[Jakub Hrozek]

On Tue, Sep 23, 2014 at 02:20:27PM +0200, Sumit Bose wrote:
> Hi,
>
> this patch adds support for the new extdom plugin version as described on
> http://www.freeipa.org/page/V4/Extdom_plugin_enhancement_grouplist .
>
> I will send the server side updates to the freeipa-devel list soon.
> Nevertheless it would be nice if these changes can already be tested
> against the old version to avoid regressions.
>
> bye,
> Sumit

So far I read the patch (see some, mostly nitpicks, below) and ran some tests against an IPA server. I'm seeing a segfault when looking up a user, though:

1351        if (attrs->sysdb_attrs == NULL) {
(gdb) bt
#0  0x00007f0719c5c82a in ipa_s2n_save_objects (dom=0x1916b80, req_input=0x193ad40, attrs=0x0, simple_attrs=0x193ab50) at src/providers/ipa/ipa_s2n_exop.c:1351
#1  0x00007f0719c5c6bb in ipa_s2n_get_user_done (subreq=0x0) at src/providers/ipa/ipa_s2n_exop.c:1312
#2  0x00007f0719c58da0 in ipa_s2n_exop_done (op=0x1933730, reply=0x1932cf0, error=0, pvt=0x1933520) at src/providers/ipa/ipa_s2n_exop.c:191
#3  0x00007f0719130372 in sdap_process_message (ev=0x18d8670, sh=0x1935090, msg=0x19384e0) at src/providers/ldap/sdap_async.c:357
#4  0x00007f071912fee4 in sdap_process_result (ev=0x18d8670, pvt=0x1935090) at src/providers/ldap/sdap_async.c:196
#5  0x00007f071912fb7a in sdap_ldap_result (ev=0x18d8670, fde=0x1935710, flags=1, pvt=0x1935090) at src/providers/ldap/sdap_async.c:137
#6  0x00007f07277662a3 in epoll_event_loop_once () from /lib64/libtevent.so.0
#7  0x00007f0727764787 in std_event_loop_once () from /lib64/libtevent.so.0
#8  0x00007f0727760fed in _tevent_loop_once () from /lib64/libtevent.so.0
#9  0x00007f072776118b in tevent_common_loop_wait () from /lib64/libtevent.so.0
#10 0x00007f0727764727 in std_event_loop_wait () from /lib64/libtevent.so.0
#11 0x00007f07279a9be6 in server_loop (main_ctx=0x18d9ac0) at src/util/server.c:587
#12 0x000000000040dad1 in main (argc=4, argv=0x7fffc68fd6e8) at src/providers/data_provider_be.c:2881

I think it's caused by the Coverity warning inline, because the 'attrs' variable is never touched that way.
> From 70a440416c609709c186b0fde80962e9ee036a16 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Fri, 5 Sep 2014 10:34:01 +0200
> Subject: [PATCH] IPA: add support for new extdom plugin version
>
> Initially the extdom plugin was only used to translate SIDs of AD user
> and groups to names or POSIX IDs. On IPA clients group memberships were
> resolved with the help of the PAC in the Kerberos ticket which required
> that the user has logged in at least once. Home directory and the login
> shell were auto generated.
>
> The new version of the extdom plugin can return the complete list of
> group memberships of a user and the list of all members of a group.
> Additionally the gecos field, home directory and login shell are
> returned together with an optional list of key-value pairs for arbitrary
> data which is written unmodified to the cache.
>
> Fixes https://fedorahosted.org/sssd/ticket/2159
>   and https://fedorahosted.org/sssd/ticket/2041
>
> src/providers/ipa/ipa_s2n_exop.c | 819 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 733 insertions(+), 86 deletions(-)
>
> diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c
> index ccf4c23..8a12cba 100644
> --- a/src/providers/ipa/ipa_s2n_exop.c
> +++ b/src/providers/ipa/ipa_s2n_exop.c
> @@ -36,18 +36,22 @@ enum input_types {
>
> enum request_types {
>      REQ_SIMPLE = 1,
> REQ_FULL
> REQ_FULL,
> REQ_FULL_WITH_MEMBERS
>
> };
>
> enum response_types {
>      RESP_SID = 1,
>      RESP_NAME,
>      RESP_USER,
> RESP_GROUP
> RESP_GROUP,
> RESP_USER_GROUPLIST,
> RESP_GROUP_MEMBERS
>
> };
>
> /* ==Sid2Name Extended Operation============================================= */
>  #define EXOP_SID2NAME_OID "2.16.840.1.113730.3.8.10.4"
> +#define EXOP_SID2NAME_V1_OID "2.16.840.1.113730.3.8.10.4.1"
>
> struct ipa_s2n_exop_state {
>      struct sdap_handle *sh;
> @@ -65,6 +69,8 @@ static void ipa_s2n_exop_done(struct sdap_op *op,
>  static struct tevent_req *ipa_s2n_exop_send(TALLOC_CTX *mem_ctx,
>                                              struct tevent_context *ev,
>                                              struct sdap_handle *sh,
>                                        bool is_v1,
>                                        int timeout,
>                                        struct berval *bv)
>
> {
>      struct tevent_req *req = NULL;
> @@ -81,18 +87,19 @@ static struct tevent_req *ipa_s2n_exop_send(TALLOC_CTX *mem_ctx,
>  DEBUG(SSSDBG_TRACE_FUNC, "Executing extended operation\n");
>
> ret = ldap_extended_operation(state->sh->ldap, EXOP_SID2NAME_OID,
>                              bv, NULL, NULL, &msgid);
> ret = ldap_extended_operation(state->sh->ldap,
>                           is_v1 ? EXOP_SID2NAME_V1_OID : EXOP_SID2NAME_OID,
>                           bv, NULL, NULL, &msgid);
> if (ret == -1 || msgid == -1) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "ldap_extended_operation failed\n");
>        ret = ERR_NETWORK_IO;
>        goto fail;
>    }
> DEBUG(SSSDBG_TRACE_INTERNAL, "ldap_extended_operation sent, msgid = %d\n", msgid);
> DEBUG(SSSDBG_TRACE_INTERNAL, "ldap_extended_operation sent, msgid = %d\n",
>                              msgid);
>
> /* FIXME: get timeouts from configuration, for now 10 secs. */
> ret = sdap_op_add(state, ev, state->sh, msgid, ipa_s2n_exop_done, req, 10,
>                  &state->op);
> ret = sdap_op_add(state, ev, state->sh, msgid, ipa_s2n_exop_done, req,
>                  timeout, &state->op);
> if (ret) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "Failed to set up operation!\n");
>        ret = ERR_INTERNAL;
>
> @@ -129,7 +136,8 @@ static void ipa_s2n_exop_done(struct sdap_op *op,
>                              &result, &errmsg, NULL, NULL,
>                              NULL, 0);
>      if (ret != LDAP_SUCCESS) {
>    DEBUG(SSSDBG_OP_FAILURE, "ldap_parse_result failed (%d)\n", state->op->msgid);
>    DEBUG(SSSDBG_OP_FAILURE, "ldap_parse_result failed (%d)\n",
>                             state->op->msgid);
>    ret = ERR_NETWORK_IO;
>    goto done;
> }
>
> @@ -145,7 +153,8 @@ static void ipa_s2n_exop_done(struct sdap_op *op,
>      ret = ldap_parse_extended_result(state->sh->ldap, reply->msg,
>                                        &retoid, &retdata, 0);
>      if (ret != LDAP_SUCCESS) {
>    DEBUG(SSSDBG_OP_FAILURE, "ldap_parse_extendend_result failed (%d)\n", ret);
>    DEBUG(SSSDBG_OP_FAILURE, "ldap_parse_extendend_result failed (%d)\n",
>                             ret);
>    ret = ERR_NETWORK_IO;
>    goto done;
> }
>
> @@ -250,6 +259,7 @@ done:
> requestType ENUMERATED {
>    simple (1),
>    full (2)
>    full_with_members (3)
> },
> data InputData
> }
>
> @@ -370,7 +380,9 @@ done:
>    sid (1),
>    name (2),
>    posix_user (3),
>    posix_group (4)
>    posix_group (4),
>    posix_user_grouplist (5),
>    posix_group_members (6)
> },
> data OutputData
> }
>
> @@ -379,7 +391,10 @@ done:
> sid OCTET STRING,
> name NameDomainData,
> user PosixUser,
> group PosixGroup
> group PosixGroup,
> usergrouplist PosixUserGrouplist,
> groupmembers PosixGroupMembers
> }
> NameDomainData ::= SEQUENCE {
>
> @@ -400,6 +415,27 @@ done:
> gid INTEGER
> }
> PosixUserGrouplist ::= SEQUENCE {
> domain_name OCTET STRING,
> user_name OCTET STRING,
> uid INTEGER,
> gid INTEGER,
> gecos OCTET STRING,
> home_directory OCTET STRING,
> shell OCTET STRING,
> grouplist GroupNameList
> }
> GroupNameList ::= SEQUENCE OF OCTET STRING
> PosixGroupMembers ::= SEQUENCE {
> domain_name OCTET STRING,
> group_name OCTET STRING,
> gid INTEGER,
> members GroupMemberList
> }
> GroupMemberList ::= SEQUENCE OF OCTET STRING
> */
>
> struct resp_attrs {
> @@ -411,8 +447,66 @@ struct resp_attrs {
>          char *sid_str;
>          char *name;
>      } a;
> size_t ngroups;
> char **groups;
> struct sysdb_attrs *sysdb_attrs;
>
> };
>
> +static errno_t get_extra_attrs(BerElement *ber, struct resp_attrs *resp_attrs)
> +{
> ber_tag_t tag;
> ber_len_t ber_len;
> char *ber_cookie;
> char *name;
> struct berval **values;
> struct ldb_val v;
> int ret;
> size_t c;
> if (resp_attrs->sysdb_attrs == NULL) {
>    resp_attrs->sysdb_attrs = sysdb_new_attrs(resp_attrs);
>    if (resp_attrs->sysdb_attrs == NULL) {
>        DEBUG(SSSDBG_OP_FAILURE, "sysdb_new_attrs failed.\n");
>        return ENOMEM;
>    }
> }
> DEBUG(SSSDBG_TRACE_ALL, "Found new sequence, skipping.\n");

~~~~~~~~
                                            Why 'skipping' ?
> for (tag = ber_first_element(ber, &ber_len, &ber_cookie);
>     tag != LBER_DEFAULT;
>     tag = ber_next_element(ber, &ber_len, ber_cookie)) {
>    tag = ber_scanf(ber, "{a{V}}", &name, &values);
>    if (tag == LBER_ERROR) {
>        DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
>        return EINVAL;
>    }
>    DEBUG(SSSDBG_TRACE_ALL, "Extra attribute [%s].\n", name);
>    for (c = 0; values[c] != NULL; c++) {
>        v.data = (uint8_t *) values[c]->bv_val;
>        v.length = values[c]->bv_len;
>        ret = sysdb_attrs_add_val(resp_attrs->sysdb_attrs, name, &v);
>        if (ret != EOK) {
>            DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_add_val failed.\n");
>            ldap_memfree(name);
>            ber_bvecfree(values);
>            return ret;
>        }
>    }
>    ldap_memfree(name);
>    ber_bvecfree(values);
> }
> return EOK;
>
> +}

Can you put a newline here?
> +static errno_t ipa_s2n_save_objects(struct sss_domain_info *dom,
>                                struct req_input *req_input,
>                                struct resp_attrs *attrs,
>                                struct resp_attrs *simple_attrs);
>
> static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
>                                       char *retoid,
>                                       struct berval *retdata,
> @@ -420,6 +514,7 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
>  {
>      BerElement *ber = NULL;
>      ber_tag_t tag;
> ber_len_t ber_len;
> int ret;
> enum response_types type;
> char *domain_name = NULL;
>
> @@ -428,16 +523,26 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
>      gid_t gid;
>      struct resp_attrs *attrs = NULL;
>      char *sid_str;
>
> bool is_v1 = false;
>
> char *gecos = NULL;
>
> char *homedir = NULL;
>
> char *shell = NULL;
>
> char **list = NULL;
>
> size_t c;
>
> if (retoid == NULL || retdata == NULL) {
>     DEBUG(SSSDBG_OP_FAILURE, "Missing OID or data.\n");
>     return EINVAL;
> }
> if (strcmp(retoid, EXOP_SID2NAME_OID) != 0) {
> if (strcmp(retoid, EXOP_SID2NAME_V1_OID) == 0) {
>    is_v1 = true;
> } else if (strcmp(retoid, EXOP_SID2NAME_OID) == 0) {
>    is_v1 = false;
> } else {
>     DEBUG(SSSDBG_OP_FAILURE,
>          "Result has wrong OID, expected [%s], got [%s].\n",
>          EXOP_SID2NAME_OID, retoid);
>          "Result has wrong OID, expected [%s] or [%s], got [%s].\n",
>          EXOP_SID2NAME_OID, EXOP_SID2NAME_V1_OID, retoid);
>    return EINVAL;
> }
>
> @@ -463,7 +568,8 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
>  switch (type) {
>      case RESP_USER:
>        tag = ber_scanf(ber, "{aaii}}", &domain_name, &name, &uid, &gid);
>    case RESP_USER_GROUPLIST:
>        tag = ber_scanf(ber, "{aaii", &domain_name, &name, &uid, &gid);
>        if (tag == LBER_ERROR) {
>            DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
>            ret = EINVAL;
>
> @@ -485,9 +591,76 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
>              attrs->a.user.pw_uid = uid;
>              attrs->a.user.pw_gid = gid;
>        if (is_v1) {

It would be nice to split the v1 block into its own function that would
either fill attrs or fail. Not sure how much work it is, if it's too
much we can file a ticket..
>            tag = ber_scanf(ber, "aaa", &gecos, &homedir, &shell);
>            if (tag == LBER_ERROR) {
>                DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
>                ret = EINVAL;
>                goto done;
>            }
>            attrs->a.user.pw_gecos = talloc_strdup(attrs, gecos);
>            attrs->a.user.pw_dir = talloc_strdup(attrs, homedir);
>            attrs->a.user.pw_shell = talloc_strdup(attrs, shell);
>            if (attrs->a.user.pw_gecos == NULL
>                    || attrs->a.user.pw_dir == NULL
>                    || attrs->a.user.pw_shell == NULL) {
>                DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
>                ret = ENOMEM;
>                goto done;
>            }
>            tag = ber_scanf(ber, "{v}", &list);
>            if (tag == LBER_ERROR) {
>                DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
>                ret = EINVAL;
>                goto done;
>            }
>            for (attrs->ngroups = 0; list[attrs->ngroups] != NULL;
>                 attrs->ngroups++);

Please put a newline here, I was looking for a continuation of the
for-loop initially. Additionally, it would be nice to move each
statement on its own line.
>            if (attrs->ngroups > 0) {
>                attrs->groups = talloc_array(attrs, char *, attrs->ngroups);
>                if (attrs->groups == NULL) {
>                    DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
>                    ret = ENOMEM;
>                    goto done;
>                }
>                for (c = 0; c < attrs->ngroups; c++) {
>                    attrs->groups[c] = talloc_strdup(attrs->groups,
>                                                     list[c]);
>                    if (attrs->groups[c] == NULL) {
>                        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
>                        ret = ENOMEM;
>                        goto done;
>                    }
>                }
>            }
>        }
>        tag = ber_peek_tag(ber, &ber_len);
>        DEBUG(SSSDBG_TRACE_ALL, "BER tag is [%d]\n", (int) tag);
>        if (tag == LBER_SEQUENCE) {
>            ret = get_extra_attrs(ber, attrs);
>            if (ret != EOK) {
>                DEBUG(SSSDBG_OP_FAILURE, "get_extra_attrs failed.\n");
>                goto done;
>            }
>        }
>        tag = ber_scanf(ber, "}}");
>        if (tag == LBER_ERROR) {
>            DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
>            ret = EINVAL;
>            goto done;
>        }
>        break;
>    case RESP_GROUP:
>        tag = ber_scanf(ber, "{aai}}", &domain_name, &name, &gid);
>    case RESP_GROUP_MEMBERS:
>        tag = ber_scanf(ber, "{aai", &domain_name, &name, &gid);
>        if (tag == LBER_ERROR) {
>            DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
>            ret = EINVAL;
>
> @@ -508,6 +681,55 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
>          attrs->a.group.gr_gid = gid;
>
>        if (is_v1) {

Again, I would prefer if this was a separate function, but I don't
insist.
>            tag = ber_scanf(ber, "{v}", &list);
>            if (tag == LBER_ERROR) {
>                DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
>                ret = EINVAL;
>                goto done;
>            }
>            for (attrs->ngroups = 0; list[attrs->ngroups] != NULL;
>                 attrs->ngroups++);

Newline please :-)
>            if (attrs->ngroups > 0) {
>                attrs->a.group.gr_mem = talloc_zero_array(attrs, char *,
>                                                        attrs->ngroups + 1);
>                if (attrs->a.group.gr_mem == NULL) {
>                    DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
>                    ret = ENOMEM;
>                    goto done;
>                }
>                for (c = 0; c < attrs->ngroups; c++) {
>                    attrs->a.group.gr_mem[c] =
>                                        talloc_strdup(attrs->a.group.gr_mem,
>                                                      list[c]);
>                    if (attrs->a.group.gr_mem[c] == NULL) {
>                        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
>                        ret = ENOMEM;
>                        goto done;
>                    }
>                }
>            }
>            tag = ber_peek_tag(ber, &ber_len);
>            DEBUG(SSSDBG_TRACE_ALL, "BER tag is [%d]\n", (int) tag);
>            if (tag == LBER_SEQUENCE) {
>                ret = get_extra_attrs(ber, attrs);
>                if (ret != EOK) {
>                    DEBUG(SSSDBG_OP_FAILURE, "get_extra_attrs failed.\n");
>                    goto done;
>                }
>            }
>        }
>        tag = ber_scanf(ber, "}}");
>        if (tag == LBER_ERROR) {
>            DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
>            ret = EINVAL;
>            goto done;
>        }
>        break;
>    case RESP_SID:
>        tag = ber_scanf(ber, "a}", &sid_str);
>
> @@ -561,6 +783,10 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
>  done:
>      ber_memfree(domain_name);
>      ber_memfree(name);
>
> ber_memfree(gecos);
>
> ber_memfree(homedir);
>
> ber_memfree(shell);
>
> ber_memvfree((void **) list);
> ber_free(ber, 1);
>
> if (ret == EOK) {
>
> @@ -572,6 +798,166 @@ done:
>      return ret;
>  }
>
> +struct ipa_s2n_get_groups_state {
> struct tevent_context *ev;
> struct sss_domain_info *dom;
> struct sdap_handle *sh;
> struct req_input req_input;
> char **group_list;
> size_t group_idx;
> int exop_timeout;
>
> +};
>
> +static errno_t ipa_s2n_get_groups_step(struct tevent_req *req);
> +static void ipa_s2n_get_groups_next(struct tevent_req *subreq);

Newline please.
> +struct tevent_req *ipa_s2n_get_groups_send(TALLOC_CTX *mem_ctx,

This function could be static.
>                                       struct tevent_context *ev,
>                                       struct sss_domain_info *dom,
>                                       struct sdap_handle *sh,
>                                       int exop_timeout,
>                                       char **group_list)
>
> +{
> int ret;
> struct ipa_s2n_get_groups_state *state;
> struct tevent_req *req;
> req = tevent_req_create(mem_ctx, &state, struct ipa_s2n_get_groups_state);
> if (req == NULL) {
>    return NULL;
> }
> state->ev = ev;
> state->dom = dom;
> state->sh = sh;
> state->group_list = group_list;
> state->group_idx = 0;
> state->req_input.type = REQ_INP_NAME;
> state->req_input.inp.name = NULL;
> state->exop_timeout = exop_timeout;
> ret = ipa_s2n_get_groups_step(req);
> if (ret != EOK) {
>    DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_get_groups_step failed.\n");
>    goto done;
> }
>
> +done:
> if (ret != EOK) {
>    tevent_req_error(req, ret);
>    tevent_req_post(req, ev);
> }
> return req;
>
> +}
>
> +static errno_t ipa_s2n_get_groups_step(struct tevent_req *req)
> +{
> int ret;
> struct ipa_s2n_get_groups_state *state = tevent_req_data(req,
>                                           struct ipa_s2n_get_groups_state);
> struct berval *bv_req;
> struct tevent_req *subreq;
> struct sss_domain_info *obj_domain;
> struct sss_domain_info *parent_domain;
> char *group_name = NULL;
> char *domain_name = NULL;
> parent_domain = (state->dom->parent == NULL) ? state->dom
>                                             : state->dom->parent;

I wonder if it makes sense to use get_domains_head() here? Currently we
only use tree-shaped domains with a single depths and a single father,
but in future we might not..
> ret = sss_parse_name(state, parent_domain->names,
>                     state->group_list[state->group_idx],
>                     &domain_name, &group_name);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse name '%s' [%d]: %s\n",
>                                state->group_list[state->group_idx],
>                                ret, sss_strerror(ret));
>    return ret;
> }
> obj_domain = find_domain_by_name(parent_domain, domain_name, true);
> if (obj_domain == NULL) {
>    DEBUG(SSSDBG_OP_FAILURE, "find_domain_by_name failed.\n");
>    return ENOMEM;
> }
> state->req_input.inp.name = group_name;

..or maybe I'm not understanding the code well. So we're trying to parse
the name according to the parent domain regex and then find the child
domain again?
> ret = s2n_encode_request(state, obj_domain->name, BE_REQ_GROUP,
>                         REQ_FULL_WITH_MEMBERS,
>                         &state->req_input, &bv_req);
> if (ret != EOK) {
>    DEBUG(SSSDBG_OP_FAILURE, "s2n_encode_request failed.\n");
>    return ret;
> }
> subreq = ipa_s2n_exop_send(state, state->ev, state->sh, true,
>                           state->exop_timeout, bv_req);
> if (subreq == NULL) {
>    DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_exop_send failed.\n");
>    return ENOMEM;
> }
> tevent_req_set_callback(subreq, ipa_s2n_get_groups_next, req);
> return EOK;
>
> +}

the rest of the request looks good to me, snip...
> +static errno_t process_members(struct sss_domain_info *domain,
>                           struct sysdb_attrs *group_attrs,
>                           char **members)
>
> +{
> int ret;
> size_t c;
> TALLOC_CTX *tmp_ctx;
> struct ldb_message *msg;
> const char *dn_str;
> struct sss_domain_info *obj_domain;
> struct sss_domain_info *parent_domain;
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
>    return ENOMEM;
> }
> parent_domain = (domain->parent == NULL) ? domain : domain->parent;
> for (c = 0; members[c] != NULL; c++) {
>    obj_domain = find_domain_by_object_name(parent_domain, members[c]);

When is obj_domain different from domain here?
>    if (obj_domain == NULL) {
>        DEBUG(SSSDBG_OP_FAILURE, "find_domain_by_object_name failed.\n");

You're leaking tmp_ctx here.
>        return ENOMEM;
>    }
>    ret = sysdb_search_user_by_name(tmp_ctx, obj_domain, members[c], NULL,
>                                    &msg);
>    if (ret == EOK) {
>        dn_str = ldb_dn_get_linearized(msg->dn);
>        if (ret != EOK) {

I think you meant to check dn_str here.
>            DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_get_linearized failed.\n");
>            goto done;
>        }
>        DEBUG(SSSDBG_TRACE_ALL, "Adding member [%s][%s]\n",
>                                members[c], dn_str);
>        ret = sysdb_attrs_add_string(group_attrs, SYSDB_MEMBER, dn_str);
>        if (ret != EOK) {
>            DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_add_string failed.\n");
>            goto done;
>        }
>    } else if (ret == ENOENT) {
>        DEBUG(SSSDBG_TRACE_ALL, "Adding ghost member [%s]\n", members[c]);
>        ret = sysdb_attrs_add_string(group_attrs, SYSDB_GHOST, members[c]);
>        if (ret != EOK) {
>            DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_add_string failed.\n");
>            goto done;
>        }
>    } else {
>        DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_name failed.\n");
>        goto done;
>    }
> }
> ret = EOK;
>
> +done:
> talloc_free(tmp_ctx);
> return ret;
>
> +}
>
> +static errno_t get_group_dn_list(TALLOC_CTX *mem_ctx,
>                             struct sss_domain_info *dom,
>                             size_t ngroups, char **groups,
>                             struct ldb_dn ***_dn_list,
>                             char ***_missing_groups)

snip, looking good

[...]
> +static void ipa_s2n_get_groups_done(struct tevent_req  *subreq);
>  static void ipa_s2n_get_user_done(struct tevent_req *subreq)
>  {
>      struct tevent_req *req = tevent_req_callback_data(subreq,
> @@ -645,18 +1202,9 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
>      char *retoid = NULL;
>      struct berval *retdata = NULL;
>      struct resp_attrs *attrs = NULL;
> struct resp_attrs *simple_attrs = NULL;
> time_t now;
> uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */
> struct sss_nss_homedir_ctx homedir_ctx;
> const char *homedir = NULL;
> struct sysdb_attrs *user_attrs = NULL;
> struct sysdb_attrs *group_attrs = NULL;
> char *name;
> char *realm;
> char *upn;
> struct berval *bv_req = NULL;
> gid_t gid;
>
> char **missing_groups = NULL;
>
> struct ldb_dn **group_dn_list = NULL;
>
> ret = ipa_s2n_exop_recv(subreq, state, &retoid, &retdata);
> talloc_zfree(subreq);
>
> @@ -666,6 +1214,7 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
>      }
>  switch (state->request_type) {
> case REQ_FULL_WITH_MEMBERS:
> case REQ_FULL:
>     ret = s2n_response_to_attrs(state, retoid, retdata, &attrs);
>     if (ret != EOK) {
>
> @@ -688,6 +1237,32 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
>      state->attrs = attrs;
>
>    if (attrs->response_type == RESP_USER_GROUPLIST) {
>        ret = get_group_dn_list(state, state->dom,
>                                attrs->ngroups, attrs->groups,
>                                &group_dn_list, &missing_groups);
>        if (ret != EOK) {
>            DEBUG(SSSDBG_OP_FAILURE, "get_group_dn_list failed.\n");
>            goto done;
>        }
>        if (missing_groups != NULL) {
>            subreq = ipa_s2n_get_groups_send(state, state->ev, state->dom,
>                                             state->sh, state->exop_timeout,
>                                             missing_groups);
>            if (subreq == NULL) {
>                DEBUG(SSSDBG_OP_FAILURE,
>                      "ipa_s2n_get_groups_send failed.\n");
>                ret = ENOMEM;
>                goto done;
>            }
>            tevent_req_set_callback(subreq, ipa_s2n_get_groups_done,
>                                    req);
>            return;
>        }
>    }
>    if (state->req_input->type == REQ_INP_SECID) {
>        /* We already know the SID, we do not have to read it. */
>        break;
>
> @@ -702,7 +1277,8 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
>              goto done;
>          }
>    subreq = ipa_s2n_exop_send(state, state->ev, state->sh, bv_req);
>    subreq = ipa_s2n_exop_send(state, state->ev, state->sh, false,
>                               state->exop_timeout, bv_req);
>    if (subreq == NULL) {
>        DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_exop_send failed.\n");
>        ret = ENOMEM;
>
> @@ -713,7 +1289,8 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
>          return;
>  case REQ_SIMPLE:
>    ret = s2n_response_to_attrs(state, retoid, retdata, &simple_attrs);
>    ret = s2n_response_to_attrs(state, retoid, retdata,
>                                &state->simple_attrs);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_OP_FAILURE, "s2n_response_to_attrs failed.\n");
>        goto done;
>
> @@ -730,40 +1307,79 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
>          DEBUG(SSSDBG_CRIT_FAILURE, "Missing data of full request.\n");
>          ret = EINVAL;
>          goto done;
> }
> ret = ipa_s2n_save_objects(state->dom, state->req_input, attrs,

I wonder if you wanted to use state->attrs instead of attrs here,
because Coverity is complaining about this part of the code:

Error: FORWARD_NULL (CWE-476): [#def2]
sssd-1.12.2/src/providers/ipa/ipa_s2n_exop.c:1204: assign_zero: Assigning: "attrs" = "NULL".
sssd-1.12.2/src/providers/ipa/ipa_s2n_exop.c:1312: var_deref_model: Passing null pointer "attrs" to function "ipa_s2n_save_objects(struct sss_domain_info *, struct req_input *, struct resp_attrs *, struct resp_attrs *)", which dereferences it.
sssd-1.12.2/src/providers/ipa/ipa_s2n_exop.c:1351:5: deref_parm: Directly dereferencing parameter "attrs".
>                           state->simple_attrs);
> if (ret != EOK) {
>    DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_save_objects failed.\n");
>    goto done;
> }
>
> +done:
> if (ret == EOK) {
>    tevent_req_done(req);
> } else {
>    attrs = state->attrs;
>    tevent_req_error(req, ret);
> }
> return;
>
> +}

The rest looks OK as well.

[Sumit Bose]

On Wed, Sep 24, 2014 at 12:23:20PM +0200, Jakub Hrozek wrote:
> On Tue, Sep 23, 2014 at 02:20:27PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > this patch adds support for the new extdom plugin version as described on
> > http://www.freeipa.org/page/V4/Extdom_plugin_enhancement_grouplist .
> >
> > I will send the server side updates to the freeipa-devel list soon.
> > Nevertheless it would be nice if these changes can already be tested
> > against the old version to avoid regressions.
> >
> > bye,
> > Sumit

Thank you for the review.
> So far I read the patch (see some, mostly nitpicks, below) and ran some tests against an IPA server. I'm seeing a segfault when looking up a user, though:
>
> 1351        if (attrs->sysdb_attrs == NULL) {
> (gdb) bt
> #0  0x00007f0719c5c82a in ipa_s2n_save_objects (dom=0x1916b80, req_input=0x193ad40, attrs=0x0, simple_attrs=0x193ab50) at src/providers/ipa/ipa_s2n_exop.c:1351
> #1  0x00007f0719c5c6bb in ipa_s2n_get_user_done (subreq=0x0) at src/providers/ipa/ipa_s2n_exop.c:1312
> #2  0x00007f0719c58da0 in ipa_s2n_exop_done (op=0x1933730, reply=0x1932cf0, error=0, pvt=0x1933520) at src/providers/ipa/ipa_s2n_exop.c:191
> #3  0x00007f0719130372 in sdap_process_message (ev=0x18d8670, sh=0x1935090, msg=0x19384e0) at src/providers/ldap/sdap_async.c:357
> #4  0x00007f071912fee4 in sdap_process_result (ev=0x18d8670, pvt=0x1935090) at src/providers/ldap/sdap_async.c:196
> #5  0x00007f071912fb7a in sdap_ldap_result (ev=0x18d8670, fde=0x1935710, flags=1, pvt=0x1935090) at src/providers/ldap/sdap_async.c:137
> #6  0x00007f07277662a3 in epoll_event_loop_once () from /lib64/libtevent.so.0
> #7  0x00007f0727764787 in std_event_loop_once () from /lib64/libtevent.so.0
> #8  0x00007f0727760fed in _tevent_loop_once () from /lib64/libtevent.so.0
> #9  0x00007f072776118b in tevent_common_loop_wait () from /lib64/libtevent.so.0
> #10 0x00007f0727764727 in std_event_loop_wait () from /lib64/libtevent.so.0
> #11 0x00007f07279a9be6 in server_loop (main_ctx=0x18d9ac0) at src/util/server.c:587
> #12 0x000000000040dad1 in main (argc=4, argv=0x7fffc68fd6e8) at src/providers/data_provider_be.c:2881
>
> I think it's caused by the Coverity warning inline, because the 'attrs' variable is never touched that way.

yes, fixed
> > DEBUG(SSSDBG_TRACE_ALL, "Found new sequence, skipping.\n");
>                                                 ~~~~~~~~
>                                         Why 'skipping' ?

fixed
> > for (tag = ber_first_element(ber, &ber_len, &ber_cookie);
> >     tag != LBER_DEFAULT;
> >     tag = ber_next_element(ber, &ber_len, ber_cookie)) {

...
> >    ldap_memfree(name);
> >    ber_bvecfree(values);
> > }
> > return EOK;
> >
> > +}
>
> Can you put a newline here?

done
> > +static errno_t ipa_s2n_save_objects(struct sss_domain_info *dom,
> >                                struct req_input *req_input,
> >                                struct resp_attrs *attrs,
> >                                struct resp_attrs *simple_attrs);

...
> > @@ -485,9 +591,76 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
> >              attrs->a.user.pw_uid = uid;
> >              attrs->a.user.pw_gid = gid;
> >        if (is_v1) {
>
> It would be nice to split the v1 block into its own function that would
> either fill attrs or fail. Not sure how much work it is, if it's too
> much we can file a ticket..

done
> >            tag = ber_scanf(ber, "aaa", &gecos, &homedir, &shell);
> >            if (tag == LBER_ERROR) {
> >                DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> >                ret = EINVAL;
> >                goto done;
> >            }
> >            attrs->a.user.pw_gecos = talloc_strdup(attrs, gecos);
> >            attrs->a.user.pw_dir = talloc_strdup(attrs, homedir);
> >            attrs->a.user.pw_shell = talloc_strdup(attrs, shell);
> >            if (attrs->a.user.pw_gecos == NULL
> >                    || attrs->a.user.pw_dir == NULL
> >                    || attrs->a.user.pw_shell == NULL) {
> >                DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
> >                ret = ENOMEM;
> >                goto done;
> >            }
> >            tag = ber_scanf(ber, "{v}", &list);
> >            if (tag == LBER_ERROR) {
> >                DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> >                ret = EINVAL;
> >                goto done;
> >            }
> >            for (attrs->ngroups = 0; list[attrs->ngroups] != NULL;
> >                 attrs->ngroups++);
>
> Please put a newline here, I was looking for a continuation of the
> for-loop initially. Additionally, it would be nice to move each
> statement on its own line.

done
> >            if (attrs->ngroups > 0) {
> >                attrs->groups = talloc_array(attrs, char *, attrs->ngroups);
> >                if (attrs->groups == NULL) {
> >                    DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
> >                    ret = ENOMEM;
> >                    goto done;
> >                }
> >                for (c = 0; c < attrs->ngroups; c++) {
> >                    attrs->groups[c] = talloc_strdup(attrs->groups,
> >                                                     list[c]);
> >                    if (attrs->groups[c] == NULL) {
> >                        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
> >                        ret = ENOMEM;
> >                        goto done;
> >                    }
> >                }
> >            }
> >        }
> >        tag = ber_peek_tag(ber, &ber_len);
> >        DEBUG(SSSDBG_TRACE_ALL, "BER tag is [%d]\n", (int) tag);
> >        if (tag == LBER_SEQUENCE) {
> >            ret = get_extra_attrs(ber, attrs);
> >            if (ret != EOK) {
> >                DEBUG(SSSDBG_OP_FAILURE, "get_extra_attrs failed.\n");
> >                goto done;
> >            }
> >        }
> >        tag = ber_scanf(ber, "}}");
> >        if (tag == LBER_ERROR) {
> >            DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> >            ret = EINVAL;
> >            goto done;
> >        }
> >        break;
> >    case RESP_GROUP:
> >        tag = ber_scanf(ber, "{aai}}", &domain_name, &name, &gid);
> >    case RESP_GROUP_MEMBERS:
> >        tag = ber_scanf(ber, "{aai", &domain_name, &name, &gid);
> >        if (tag == LBER_ERROR) {
> >            DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> >            ret = EINVAL;
> >
> > @@ -508,6 +681,55 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
> >          attrs->a.group.gr_gid = gid;
> >
> >        if (is_v1) {
>
> Again, I would prefer if this was a separate function, but I don't
> insist.

done
> >            tag = ber_scanf(ber, "{v}", &list);
> >            if (tag == LBER_ERROR) {
> >                DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> >                ret = EINVAL;
> >                goto done;
> >            }
> >            for (attrs->ngroups = 0; list[attrs->ngroups] != NULL;
> >                 attrs->ngroups++);
>
> Newline please :-)

done
> >            if (attrs->ngroups > 0) {
> >                attrs->a.group.gr_mem = talloc_zero_array(attrs, char *,
> >                                                        attrs->ngroups + 1);
> >                if (attrs->a.group.gr_mem == NULL) {
> >                    DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
> >                    ret = ENOMEM;
> >                    goto done;
> >                }
> >                for (c = 0; c < attrs->ngroups; c++) {
> >                    attrs->a.group.gr_mem[c] =
> >                                        talloc_strdup(attrs->a.group.gr_mem,
> >                                                      list[c]);
> >                    if (attrs->a.group.gr_mem[c] == NULL) {
> >                        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
> >                        ret = ENOMEM;
> >                        goto done;
> >                    }
> >                }
> >            }
> >            tag = ber_peek_tag(ber, &ber_len);
> >            DEBUG(SSSDBG_TRACE_ALL, "BER tag is [%d]\n", (int) tag);
> >            if (tag == LBER_SEQUENCE) {
> >                ret = get_extra_attrs(ber, attrs);
> >                if (ret != EOK) {
> >                    DEBUG(SSSDBG_OP_FAILURE, "get_extra_attrs failed.\n");
> >                    goto done;
> >                }
> >            }
> >        }
> >        tag = ber_scanf(ber, "}}");
> >        if (tag == LBER_ERROR) {
> >            DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> >            ret = EINVAL;
> >            goto done;
> >        }
> >        break;
> >    case RESP_SID:
> >        tag = ber_scanf(ber, "a}", &sid_str);
> >
> > @@ -561,6 +783,10 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
> >  done:
> >      ber_memfree(domain_name);
> >      ber_memfree(name);
> >
> > ber_memfree(gecos);
> >
> > ber_memfree(homedir);
> >
> > ber_memfree(shell);
> >
> > ber_memvfree((void **) list);
> > ber_free(ber, 1);
> >
> > if (ret == EOK) {
> >
> > @@ -572,6 +798,166 @@ done:
> >      return ret;
> >  }
> >
> > +struct ipa_s2n_get_groups_state {
> > struct tevent_context *ev;
> > struct sss_domain_info *dom;
> > struct sdap_handle *sh;
> > struct req_input req_input;
> > char **group_list;
> > size_t group_idx;
> > int exop_timeout;
> >
> > +};
> >
> > +static errno_t ipa_s2n_get_groups_step(struct tevent_req *req);
> > +static void ipa_s2n_get_groups_next(struct tevent_req *subreq);
>
> Newline please.

done
> > +struct tevent_req *ipa_s2n_get_groups_send(TALLOC_CTX *mem_ctx,
>
> This function could be static.

done
> >                                       struct tevent_context *ev,
> >                                       struct sss_domain_info *dom,
> >                                       struct sdap_handle *sh,
> >                                       int exop_timeout,
> >                                       char **group_list)
> >
> > +{
> > int ret;
> > struct ipa_s2n_get_groups_state *state;
> > struct tevent_req *req;
> > req = tevent_req_create(mem_ctx, &state, struct ipa_s2n_get_groups_state);
> > if (req == NULL) {
> >    return NULL;
> > }
> > state->ev = ev;
> > state->dom = dom;
> > state->sh = sh;
> > state->group_list = group_list;
> > state->group_idx = 0;
> > state->req_input.type = REQ_INP_NAME;
> > state->req_input.inp.name = NULL;
> > state->exop_timeout = exop_timeout;
> > ret = ipa_s2n_get_groups_step(req);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_get_groups_step failed.\n");
> >    goto done;
> > }
> >
> > +done:
> > if (ret != EOK) {
> >    tevent_req_error(req, ret);
> >    tevent_req_post(req, ev);
> > }
> > return req;
> >
> > +}
> >
> > +static errno_t ipa_s2n_get_groups_step(struct tevent_req *req)
> > +{
> > int ret;
> > struct ipa_s2n_get_groups_state *state = tevent_req_data(req,
> >                                           struct ipa_s2n_get_groups_state);
> > struct berval *bv_req;
> > struct tevent_req *subreq;
> > struct sss_domain_info *obj_domain;
> > struct sss_domain_info *parent_domain;
> > char *group_name = NULL;
> > char *domain_name = NULL;
> > parent_domain = (state->dom->parent == NULL) ? state->dom
> >                                             : state->dom->parent;
>
> I wonder if it makes sense to use get_domains_head() here? Currently we
> only use tree-shaped domains with a single depths and a single father,
> but in future we might not..

done
> > ret = sss_parse_name(state, parent_domain->names,
> >                     state->group_list[state->group_idx],
> >                     &domain_name, &group_name);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse name '%s' [%d]: %s\n",
> >                                state->group_list[state->group_idx],
> >                                ret, sss_strerror(ret));
> >    return ret;
> > }
> > obj_domain = find_domain_by_name(parent_domain, domain_name, true);
> > if (obj_domain == NULL) {
> >    DEBUG(SSSDBG_OP_FAILURE, "find_domain_by_name failed.\n");
> >    return ENOMEM;
> > }
> > state->req_input.inp.name = group_name;
>
> ..or maybe I'm not understanding the code well. So we're trying to parse
> the name according to the parent domain regex and then find the child
> domain again?

no all groups have to come from the same domain, there might be IPA
groups and groups from all domains in the trusted forest. The extdom
plugin can resolve IPA groups as well, because the new version uses SSSD
to resolve the requests.
> > ret = s2n_encode_request(state, obj_domain->name, BE_REQ_GROUP,
> >                         REQ_FULL_WITH_MEMBERS,
> >                         &state->req_input, &bv_req);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_OP_FAILURE, "s2n_encode_request failed.\n");
> >    return ret;
> > }
> > subreq = ipa_s2n_exop_send(state, state->ev, state->sh, true,
> >                           state->exop_timeout, bv_req);
> > if (subreq == NULL) {
> >    DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_exop_send failed.\n");
> >    return ENOMEM;
> > }
> > tevent_req_set_callback(subreq, ipa_s2n_get_groups_next, req);
> > return EOK;
> >
> > +}
>
> the rest of the request looks good to me, snip...
> > +static errno_t process_members(struct sss_domain_info *domain,
> >                           struct sysdb_attrs *group_attrs,
> >                           char **members)
> >
> > +{
> > int ret;
> > size_t c;
> > TALLOC_CTX *tmp_ctx;
> > struct ldb_message *msg;
> > const char *dn_str;
> > struct sss_domain_info *obj_domain;
> > struct sss_domain_info *parent_domain;
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
> >    return ENOMEM;
> > }
> > parent_domain = (domain->parent == NULL) ? domain : domain->parent;
> > for (c = 0; members[c] != NULL; c++) {
> >    obj_domain = find_domain_by_object_name(parent_domain, members[c]);
>
> When is obj_domain different from domain here?

When there are members from different domains.
> >    if (obj_domain == NULL) {
> >        DEBUG(SSSDBG_OP_FAILURE, "find_domain_by_object_name failed.\n");
>
> You're leaking tmp_ctx here.

fixed
> >        return ENOMEM;
> >    }
> >    ret = sysdb_search_user_by_name(tmp_ctx, obj_domain, members[c], NULL,
> >                                    &msg);
> >    if (ret == EOK) {
> >        dn_str = ldb_dn_get_linearized(msg->dn);
> >        if (ret != EOK) {
>
> I think you meant to check dn_str here.

yes, fixed
> >            DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_get_linearized failed.\n");
> >            goto done;
> >        }
> >        DEBUG(SSSDBG_TRACE_ALL, "Adding member [%s][%s]\n",
> >                                members[c], dn_str);
> >        ret = sysdb_attrs_add_string(group_attrs, SYSDB_MEMBER, dn_str);
> >        if (ret != EOK) {
> >            DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_add_string failed.\n");
> >            goto done;
> >        }
> >    } else if (ret == ENOENT) {
> >        DEBUG(SSSDBG_TRACE_ALL, "Adding ghost member [%s]\n", members[c]);
> >        ret = sysdb_attrs_add_string(group_attrs, SYSDB_GHOST, members[c]);
> >        if (ret != EOK) {
> >            DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_add_string failed.\n");
> >            goto done;
> >        }
> >    } else {
> >        DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_name failed.\n");
> >        goto done;
> >    }
> > }
> > ret = EOK;
> >
> > +done:
> > talloc_free(tmp_ctx);
> > return ret;
> >
> > +}
> >
> > +static errno_t get_group_dn_list(TALLOC_CTX *mem_ctx,
> >                             struct sss_domain_info *dom,
> >                             size_t ngroups, char **groups,
> >                             struct ldb_dn ***_dn_list,
> >                             char ***_missing_groups)
>
> snip, looking good
>
> [...]
> > +static void ipa_s2n_get_groups_done(struct tevent_req  *subreq);
> >  static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> >  {
> >      struct tevent_req *req = tevent_req_callback_data(subreq,
> > @@ -645,18 +1202,9 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> >      char *retoid = NULL;
> >      struct berval *retdata = NULL;
> >      struct resp_attrs *attrs = NULL;
> > struct resp_attrs *simple_attrs = NULL;
> > time_t now;
> > uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */
> > struct sss_nss_homedir_ctx homedir_ctx;
> > const char *homedir = NULL;
> > struct sysdb_attrs *user_attrs = NULL;
> > struct sysdb_attrs *group_attrs = NULL;
> > char *name;
> > char *realm;
> > char *upn;
> > struct berval *bv_req = NULL;
> > gid_t gid;
> >
> > char **missing_groups = NULL;
> >
> > struct ldb_dn **group_dn_list = NULL;
> >
> > ret = ipa_s2n_exop_recv(subreq, state, &retoid, &retdata);
> > talloc_zfree(subreq);
> >
> > @@ -666,6 +1214,7 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> >      }
> >  switch (state->request_type) {
> > case REQ_FULL_WITH_MEMBERS:
> > case REQ_FULL:
> >     ret = s2n_response_to_attrs(state, retoid, retdata, &attrs);
> >     if (ret != EOK) {
> >
> > @@ -688,6 +1237,32 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> >      state->attrs = attrs;
> >
> >    if (attrs->response_type == RESP_USER_GROUPLIST) {
> >        ret = get_group_dn_list(state, state->dom,
> >                                attrs->ngroups, attrs->groups,
> >                                &group_dn_list, &missing_groups);
> >        if (ret != EOK) {
> >            DEBUG(SSSDBG_OP_FAILURE, "get_group_dn_list failed.\n");
> >            goto done;
> >        }
> >        if (missing_groups != NULL) {
> >            subreq = ipa_s2n_get_groups_send(state, state->ev, state->dom,
> >                                             state->sh, state->exop_timeout,
> >                                             missing_groups);
> >            if (subreq == NULL) {
> >                DEBUG(SSSDBG_OP_FAILURE,
> >                      "ipa_s2n_get_groups_send failed.\n");
> >                ret = ENOMEM;
> >                goto done;
> >            }
> >            tevent_req_set_callback(subreq, ipa_s2n_get_groups_done,
> >                                    req);
> >            return;
> >        }
> >    }
> >    if (state->req_input->type == REQ_INP_SECID) {
> >        /* We already know the SID, we do not have to read it. */
> >        break;
> >
> > @@ -702,7 +1277,8 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> >              goto done;
> >          }
> >    subreq = ipa_s2n_exop_send(state, state->ev, state->sh, bv_req);
> >    subreq = ipa_s2n_exop_send(state, state->ev, state->sh, false,
> >                               state->exop_timeout, bv_req);
> >    if (subreq == NULL) {
> >        DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_exop_send failed.\n");
> >        ret = ENOMEM;
> >
> > @@ -713,7 +1289,8 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> >          return;
> >  case REQ_SIMPLE:
> >    ret = s2n_response_to_attrs(state, retoid, retdata, &simple_attrs);
> >    ret = s2n_response_to_attrs(state, retoid, retdata,
> >                                &state->simple_attrs);
> >    if (ret != EOK) {
> >        DEBUG(SSSDBG_OP_FAILURE, "s2n_response_to_attrs failed.\n");
> >        goto done;
> >
> > @@ -730,40 +1307,79 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> >          DEBUG(SSSDBG_CRIT_FAILURE, "Missing data of full request.\n");
> >          ret = EINVAL;
> >          goto done;
> > }
> > ret = ipa_s2n_save_objects(state->dom, state->req_input, attrs,
>
> I wonder if you wanted to use state->attrs instead of attrs here,
> because Coverity is complaining about this part of the code:
>
> Error: FORWARD_NULL (CWE-476): [#def2]
> sssd-1.12.2/src/providers/ipa/ipa_s2n_exop.c:1204: assign_zero: Assigning: "attrs" = "NULL".
> sssd-1.12.2/src/providers/ipa/ipa_s2n_exop.c:1312: var_deref_model: Passing null pointer "attrs" to function "ipa_s2n_save_objects(struct sss_domain_info *, struct req_input *, struct resp_attrs *, struct resp_attrs *)", which dereferences it.
> sssd-1.12.2/src/providers/ipa/ipa_s2n_exop.c:1351:5: deref_parm: Directly dereferencing parameter "attrs".

fixed
> >                           state->simple_attrs);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_save_objects failed.\n");
> >    goto done;
> > }
> >
> > +done:
> > if (ret == EOK) {
> >    tevent_req_done(req);
> > } else {
> >    attrs = state->attrs;
> >    tevent_req_error(req, ret);
> > }
> > return;
> >
> > +}
>
> The rest looks OK as well.

New version attached.

bye,
Sumit

[Sumit Bose]

On Thu, Sep 25, 2014 at 05:09:42PM +0200, Sumit Bose wrote:
> On Wed, Sep 24, 2014 at 12:23:20PM +0200, Jakub Hrozek wrote:
> > On Tue, Sep 23, 2014 at 02:20:27PM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch adds support for the new extdom plugin version as described on
> > > http://www.freeipa.org/page/V4/Extdom_plugin_enhancement_grouplist .
> > >
> > > I will send the server side updates to the freeipa-devel list soon.
> > > Nevertheless it would be nice if these changes can already be tested
> > > against the old version to avoid regressions.
> > >
> > > bye,
> > > Sumit
>
> Thank you for the review.
> > So far I read the patch (see some, mostly nitpicks, below) and ran some tests against an IPA server. I'm seeing a segfault when looking up a user, though:
> >
> > 1351        if (attrs->sysdb_attrs == NULL) {
> > (gdb) bt
> > #0  0x00007f0719c5c82a in ipa_s2n_save_objects (dom=0x1916b80, req_input=0x193ad40, attrs=0x0, simple_attrs=0x193ab50) at src/providers/ipa/ipa_s2n_exop.c:1351
> > #1  0x00007f0719c5c6bb in ipa_s2n_get_user_done (subreq=0x0) at src/providers/ipa/ipa_s2n_exop.c:1312
> > #2  0x00007f0719c58da0 in ipa_s2n_exop_done (op=0x1933730, reply=0x1932cf0, error=0, pvt=0x1933520) at src/providers/ipa/ipa_s2n_exop.c:191
> > #3  0x00007f0719130372 in sdap_process_message (ev=0x18d8670, sh=0x1935090, msg=0x19384e0) at src/providers/ldap/sdap_async.c:357
> > #4  0x00007f071912fee4 in sdap_process_result (ev=0x18d8670, pvt=0x1935090) at src/providers/ldap/sdap_async.c:196
> > #5  0x00007f071912fb7a in sdap_ldap_result (ev=0x18d8670, fde=0x1935710, flags=1, pvt=0x1935090) at src/providers/ldap/sdap_async.c:137
> > #6  0x00007f07277662a3 in epoll_event_loop_once () from /lib64/libtevent.so.0
> > #7  0x00007f0727764787 in std_event_loop_once () from /lib64/libtevent.so.0
> > #8  0x00007f0727760fed in _tevent_loop_once () from /lib64/libtevent.so.0
> > #9  0x00007f072776118b in tevent_common_loop_wait () from /lib64/libtevent.so.0
> > #10 0x00007f0727764727 in std_event_loop_wait () from /lib64/libtevent.so.0
> > #11 0x00007f07279a9be6 in server_loop (main_ctx=0x18d9ac0) at src/util/server.c:587
> > #12 0x000000000040dad1 in main (argc=4, argv=0x7fffc68fd6e8) at src/providers/data_provider_be.c:2881
> >
> > I think it's caused by the Coverity warning inline, because the 'attrs' variable is never touched that way.
>
> yes, fixed
> > > DEBUG(SSSDBG_TRACE_ALL, "Found new sequence, skipping.\n");
> >                                                 ~~~~~~~~
> >                                         Why 'skipping' ?
>
> fixed
> > > for (tag = ber_first_element(ber, &ber_len, &ber_cookie);
> > >     tag != LBER_DEFAULT;
> > >     tag = ber_next_element(ber, &ber_len, ber_cookie)) {
>
> ...
> > >    ldap_memfree(name);
> > >    ber_bvecfree(values);
> > > }
> > > return EOK;
> > >
> > > +}
> >
> > Can you put a newline here?
>
> done
> > > +static errno_t ipa_s2n_save_objects(struct sss_domain_info *dom,
> > >                                struct req_input *req_input,
> > >                                struct resp_attrs *attrs,
> > >                                struct resp_attrs *simple_attrs);
>
> ...
> > > @@ -485,9 +591,76 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
> > >              attrs->a.user.pw_uid = uid;
> > >              attrs->a.user.pw_gid = gid;
> > >        if (is_v1) {
> >
> > It would be nice to split the v1 block into its own function that would
> > either fill attrs or fail. Not sure how much work it is, if it's too
> > much we can file a ticket..
>
> done
> > >            tag = ber_scanf(ber, "aaa", &gecos, &homedir, &shell);
> > >            if (tag == LBER_ERROR) {
> > >                DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> > >                ret = EINVAL;
> > >                goto done;
> > >            }
> > >            attrs->a.user.pw_gecos = talloc_strdup(attrs, gecos);
> > >            attrs->a.user.pw_dir = talloc_strdup(attrs, homedir);
> > >            attrs->a.user.pw_shell = talloc_strdup(attrs, shell);
> > >            if (attrs->a.user.pw_gecos == NULL
> > >                    || attrs->a.user.pw_dir == NULL
> > >                    || attrs->a.user.pw_shell == NULL) {
> > >                DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
> > >                ret = ENOMEM;
> > >                goto done;
> > >            }
> > >            tag = ber_scanf(ber, "{v}", &list);
> > >            if (tag == LBER_ERROR) {
> > >                DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> > >                ret = EINVAL;
> > >                goto done;
> > >            }
> > >            for (attrs->ngroups = 0; list[attrs->ngroups] != NULL;
> > >                 attrs->ngroups++);
> >
> > Please put a newline here, I was looking for a continuation of the
> > for-loop initially. Additionally, it would be nice to move each
> > statement on its own line.
>
> done
> > >            if (attrs->ngroups > 0) {
> > >                attrs->groups = talloc_array(attrs, char *, attrs->ngroups);
> > >                if (attrs->groups == NULL) {
> > >                    DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
> > >                    ret = ENOMEM;
> > >                    goto done;
> > >                }
> > >                for (c = 0; c < attrs->ngroups; c++) {
> > >                    attrs->groups[c] = talloc_strdup(attrs->groups,
> > >                                                     list[c]);
> > >                    if (attrs->groups[c] == NULL) {
> > >                        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
> > >                        ret = ENOMEM;
> > >                        goto done;
> > >                    }
> > >                }
> > >            }
> > >        }
> > >        tag = ber_peek_tag(ber, &ber_len);
> > >        DEBUG(SSSDBG_TRACE_ALL, "BER tag is [%d]\n", (int) tag);
> > >        if (tag == LBER_SEQUENCE) {
> > >            ret = get_extra_attrs(ber, attrs);
> > >            if (ret != EOK) {
> > >                DEBUG(SSSDBG_OP_FAILURE, "get_extra_attrs failed.\n");
> > >                goto done;
> > >            }
> > >        }
> > >        tag = ber_scanf(ber, "}}");
> > >        if (tag == LBER_ERROR) {
> > >            DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> > >            ret = EINVAL;
> > >            goto done;
> > >        }
> > >        break;
> > >    case RESP_GROUP:
> > >        tag = ber_scanf(ber, "{aai}}", &domain_name, &name, &gid);
> > >    case RESP_GROUP_MEMBERS:
> > >        tag = ber_scanf(ber, "{aai", &domain_name, &name, &gid);
> > >        if (tag == LBER_ERROR) {
> > >            DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> > >            ret = EINVAL;
> > >
> > > @@ -508,6 +681,55 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
> > >          attrs->a.group.gr_gid = gid;
> > >
> > >        if (is_v1) {
> >
> > Again, I would prefer if this was a separate function, but I don't
> > insist.
>
> done
> > >            tag = ber_scanf(ber, "{v}", &list);
> > >            if (tag == LBER_ERROR) {
> > >                DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> > >                ret = EINVAL;
> > >                goto done;
> > >            }
> > >            for (attrs->ngroups = 0; list[attrs->ngroups] != NULL;
> > >                 attrs->ngroups++);
> >
> > Newline please :-)
>
> done
> > >            if (attrs->ngroups > 0) {
> > >                attrs->a.group.gr_mem = talloc_zero_array(attrs, char *,
> > >                                                        attrs->ngroups + 1);
> > >                if (attrs->a.group.gr_mem == NULL) {
> > >                    DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
> > >                    ret = ENOMEM;
> > >                    goto done;
> > >                }
> > >                for (c = 0; c < attrs->ngroups; c++) {
> > >                    attrs->a.group.gr_mem[c] =
> > >                                        talloc_strdup(attrs->a.group.gr_mem,
> > >                                                      list[c]);
> > >                    if (attrs->a.group.gr_mem[c] == NULL) {
> > >                        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
> > >                        ret = ENOMEM;
> > >                        goto done;
> > >                    }
> > >                }
> > >            }
> > >            tag = ber_peek_tag(ber, &ber_len);
> > >            DEBUG(SSSDBG_TRACE_ALL, "BER tag is [%d]\n", (int) tag);
> > >            if (tag == LBER_SEQUENCE) {
> > >                ret = get_extra_attrs(ber, attrs);
> > >                if (ret != EOK) {
> > >                    DEBUG(SSSDBG_OP_FAILURE, "get_extra_attrs failed.\n");
> > >                    goto done;
> > >                }
> > >            }
> > >        }
> > >        tag = ber_scanf(ber, "}}");
> > >        if (tag == LBER_ERROR) {
> > >            DEBUG(SSSDBG_OP_FAILURE, "ber_scanf failed.\n");
> > >            ret = EINVAL;
> > >            goto done;
> > >        }
> > >        break;
> > >    case RESP_SID:
> > >        tag = ber_scanf(ber, "a}", &sid_str);
> > >
> > > @@ -561,6 +783,10 @@ static errno_t s2n_response_to_attrs(TALLOC_CTX *mem_ctx,
> > >  done:
> > >      ber_memfree(domain_name);
> > >      ber_memfree(name);
> > >
> > > ber_memfree(gecos);
> > >
> > > ber_memfree(homedir);
> > >
> > > ber_memfree(shell);
> > >
> > > ber_memvfree((void **) list);
> > > ber_free(ber, 1);
> > >
> > > if (ret == EOK) {
> > >
> > > @@ -572,6 +798,166 @@ done:
> > >      return ret;
> > >  }
> > >
> > > +struct ipa_s2n_get_groups_state {
> > > struct tevent_context *ev;
> > > struct sss_domain_info *dom;
> > > struct sdap_handle *sh;
> > > struct req_input req_input;
> > > char **group_list;
> > > size_t group_idx;
> > > int exop_timeout;
> > >
> > > +};
> > >
> > > +static errno_t ipa_s2n_get_groups_step(struct tevent_req *req);
> > > +static void ipa_s2n_get_groups_next(struct tevent_req *subreq);
> >
> > Newline please.
>
> done
> > > +struct tevent_req *ipa_s2n_get_groups_send(TALLOC_CTX *mem_ctx,
> >
> > This function could be static.
>
> done
> > >                                       struct tevent_context *ev,
> > >                                       struct sss_domain_info *dom,
> > >                                       struct sdap_handle *sh,
> > >                                       int exop_timeout,
> > >                                       char **group_list)
> > >
> > > +{
> > > int ret;
> > > struct ipa_s2n_get_groups_state *state;
> > > struct tevent_req *req;
> > > req = tevent_req_create(mem_ctx, &state, struct ipa_s2n_get_groups_state);
> > > if (req == NULL) {
> > >    return NULL;
> > > }
> > > state->ev = ev;
> > > state->dom = dom;
> > > state->sh = sh;
> > > state->group_list = group_list;
> > > state->group_idx = 0;
> > > state->req_input.type = REQ_INP_NAME;
> > > state->req_input.inp.name = NULL;
> > > state->exop_timeout = exop_timeout;
> > > ret = ipa_s2n_get_groups_step(req);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_get_groups_step failed.\n");
> > >    goto done;
> > > }
> > >
> > > +done:
> > > if (ret != EOK) {
> > >    tevent_req_error(req, ret);
> > >    tevent_req_post(req, ev);
> > > }
> > > return req;
> > >
> > > +}
> > >
> > > +static errno_t ipa_s2n_get_groups_step(struct tevent_req *req)
> > > +{
> > > int ret;
> > > struct ipa_s2n_get_groups_state *state = tevent_req_data(req,
> > >                                           struct ipa_s2n_get_groups_state);
> > > struct berval *bv_req;
> > > struct tevent_req *subreq;
> > > struct sss_domain_info *obj_domain;
> > > struct sss_domain_info *parent_domain;
> > > char *group_name = NULL;
> > > char *domain_name = NULL;
> > > parent_domain = (state->dom->parent == NULL) ? state->dom
> > >                                             : state->dom->parent;
> >
> > I wonder if it makes sense to use get_domains_head() here? Currently we
> > only use tree-shaped domains with a single depths and a single father,
> > but in future we might not..
>
> done
> > > ret = sss_parse_name(state, parent_domain->names,
> > >                     state->group_list[state->group_idx],
> > >                     &domain_name, &group_name);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse name '%s' [%d]: %s\n",
> > >                                state->group_list[state->group_idx],
> > >                                ret, sss_strerror(ret));
> > >    return ret;
> > > }
> > > obj_domain = find_domain_by_name(parent_domain, domain_name, true);
> > > if (obj_domain == NULL) {
> > >    DEBUG(SSSDBG_OP_FAILURE, "find_domain_by_name failed.\n");
> > >    return ENOMEM;
> > > }
> > > state->req_input.inp.name = group_name;
> >
> > ..or maybe I'm not understanding the code well. So we're trying to parse
> > the name according to the parent domain regex and then find the child
> > domain again?
>
> no all groups have to come from the same domain, there might be IPA
> groups and groups from all domains in the trusted forest. The extdom
> plugin can resolve IPA groups as well, because the new version uses SSSD
> to resolve the requests.
> > > ret = s2n_encode_request(state, obj_domain->name, BE_REQ_GROUP,
> > >                         REQ_FULL_WITH_MEMBERS,
> > >                         &state->req_input, &bv_req);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_OP_FAILURE, "s2n_encode_request failed.\n");
> > >    return ret;
> > > }
> > > subreq = ipa_s2n_exop_send(state, state->ev, state->sh, true,
> > >                           state->exop_timeout, bv_req);
> > > if (subreq == NULL) {
> > >    DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_exop_send failed.\n");
> > >    return ENOMEM;
> > > }
> > > tevent_req_set_callback(subreq, ipa_s2n_get_groups_next, req);
> > > return EOK;
> > >
> > > +}
> >
> > the rest of the request looks good to me, snip...
> > > +static errno_t process_members(struct sss_domain_info *domain,
> > >                           struct sysdb_attrs *group_attrs,
> > >                           char **members)
> > >
> > > +{
> > > int ret;
> > > size_t c;
> > > TALLOC_CTX *tmp_ctx;
> > > struct ldb_message *msg;
> > > const char *dn_str;
> > > struct sss_domain_info *obj_domain;
> > > struct sss_domain_info *parent_domain;
> > > tmp_ctx = talloc_new(NULL);
> > > if (tmp_ctx == NULL) {
> > >    DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
> > >    return ENOMEM;
> > > }
> > > parent_domain = (domain->parent == NULL) ? domain : domain->parent;
> > > for (c = 0; members[c] != NULL; c++) {
> > >    obj_domain = find_domain_by_object_name(parent_domain, members[c]);
> >
> > When is obj_domain different from domain here?
>
> When there are members from different domains.
> > >    if (obj_domain == NULL) {
> > >        DEBUG(SSSDBG_OP_FAILURE, "find_domain_by_object_name failed.\n");
> >
> > You're leaking tmp_ctx here.
>
> fixed
> > >        return ENOMEM;
> > >    }
> > >    ret = sysdb_search_user_by_name(tmp_ctx, obj_domain, members[c], NULL,
> > >                                    &msg);
> > >    if (ret == EOK) {
> > >        dn_str = ldb_dn_get_linearized(msg->dn);
> > >        if (ret != EOK) {
> >
> > I think you meant to check dn_str here.
>
> yes, fixed
> > >            DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_get_linearized failed.\n");
> > >            goto done;
> > >        }
> > >        DEBUG(SSSDBG_TRACE_ALL, "Adding member [%s][%s]\n",
> > >                                members[c], dn_str);
> > >        ret = sysdb_attrs_add_string(group_attrs, SYSDB_MEMBER, dn_str);
> > >        if (ret != EOK) {
> > >            DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_add_string failed.\n");
> > >            goto done;
> > >        }
> > >    } else if (ret == ENOENT) {
> > >        DEBUG(SSSDBG_TRACE_ALL, "Adding ghost member [%s]\n", members[c]);
> > >        ret = sysdb_attrs_add_string(group_attrs, SYSDB_GHOST, members[c]);
> > >        if (ret != EOK) {
> > >            DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_add_string failed.\n");
> > >            goto done;
> > >        }
> > >    } else {
> > >        DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_name failed.\n");
> > >        goto done;
> > >    }
> > > }
> > > ret = EOK;
> > >
> > > +done:
> > > talloc_free(tmp_ctx);
> > > return ret;
> > >
> > > +}
> > >
> > > +static errno_t get_group_dn_list(TALLOC_CTX *mem_ctx,
> > >                             struct sss_domain_info *dom,
> > >                             size_t ngroups, char **groups,
> > >                             struct ldb_dn ***_dn_list,
> > >                             char ***_missing_groups)
> >
> > snip, looking good
> >
> > [...]
> > > +static void ipa_s2n_get_groups_done(struct tevent_req  *subreq);
> > >  static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> > >  {
> > >      struct tevent_req *req = tevent_req_callback_data(subreq,
> > > @@ -645,18 +1202,9 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> > >      char *retoid = NULL;
> > >      struct berval *retdata = NULL;
> > >      struct resp_attrs *attrs = NULL;
> > > struct resp_attrs *simple_attrs = NULL;
> > > time_t now;
> > > uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */
> > > struct sss_nss_homedir_ctx homedir_ctx;
> > > const char *homedir = NULL;
> > > struct sysdb_attrs *user_attrs = NULL;
> > > struct sysdb_attrs *group_attrs = NULL;
> > > char *name;
> > > char *realm;
> > > char *upn;
> > > struct berval *bv_req = NULL;
> > > gid_t gid;
> > >
> > > char **missing_groups = NULL;
> > >
> > > struct ldb_dn **group_dn_list = NULL;
> > >
> > > ret = ipa_s2n_exop_recv(subreq, state, &retoid, &retdata);
> > > talloc_zfree(subreq);
> > >
> > > @@ -666,6 +1214,7 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> > >      }
> > >  switch (state->request_type) {
> > > case REQ_FULL_WITH_MEMBERS:
> > > case REQ_FULL:
> > >     ret = s2n_response_to_attrs(state, retoid, retdata, &attrs);
> > >     if (ret != EOK) {
> > >
> > > @@ -688,6 +1237,32 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> > >      state->attrs = attrs;
> > >
> > >    if (attrs->response_type == RESP_USER_GROUPLIST) {
> > >        ret = get_group_dn_list(state, state->dom,
> > >                                attrs->ngroups, attrs->groups,
> > >                                &group_dn_list, &missing_groups);
> > >        if (ret != EOK) {
> > >            DEBUG(SSSDBG_OP_FAILURE, "get_group_dn_list failed.\n");
> > >            goto done;
> > >        }
> > >        if (missing_groups != NULL) {
> > >            subreq = ipa_s2n_get_groups_send(state, state->ev, state->dom,
> > >                                             state->sh, state->exop_timeout,
> > >                                             missing_groups);
> > >            if (subreq == NULL) {
> > >                DEBUG(SSSDBG_OP_FAILURE,
> > >                      "ipa_s2n_get_groups_send failed.\n");
> > >                ret = ENOMEM;
> > >                goto done;
> > >            }
> > >            tevent_req_set_callback(subreq, ipa_s2n_get_groups_done,
> > >                                    req);
> > >            return;
> > >        }
> > >    }
> > >    if (state->req_input->type == REQ_INP_SECID) {
> > >        /* We already know the SID, we do not have to read it. */
> > >        break;
> > >
> > > @@ -702,7 +1277,8 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> > >              goto done;
> > >          }
> > >    subreq = ipa_s2n_exop_send(state, state->ev, state->sh, bv_req);
> > >    subreq = ipa_s2n_exop_send(state, state->ev, state->sh, false,
> > >                               state->exop_timeout, bv_req);
> > >    if (subreq == NULL) {
> > >        DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_exop_send failed.\n");
> > >        ret = ENOMEM;
> > >
> > > @@ -713,7 +1289,8 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> > >          return;
> > >  case REQ_SIMPLE:
> > >    ret = s2n_response_to_attrs(state, retoid, retdata, &simple_attrs);
> > >    ret = s2n_response_to_attrs(state, retoid, retdata,
> > >                                &state->simple_attrs);
> > >    if (ret != EOK) {
> > >        DEBUG(SSSDBG_OP_FAILURE, "s2n_response_to_attrs failed.\n");
> > >        goto done;
> > >
> > > @@ -730,40 +1307,79 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> > >          DEBUG(SSSDBG_CRIT_FAILURE, "Missing data of full request.\n");
> > >          ret = EINVAL;
> > >          goto done;
> > > }
> > > ret = ipa_s2n_save_objects(state->dom, state->req_input, attrs,
> >
> > I wonder if you wanted to use state->attrs instead of attrs here,
> > because Coverity is complaining about this part of the code:
> >
> > Error: FORWARD_NULL (CWE-476): [#def2]
> > sssd-1.12.2/src/providers/ipa/ipa_s2n_exop.c:1204: assign_zero: Assigning: "attrs" = "NULL".
> > sssd-1.12.2/src/providers/ipa/ipa_s2n_exop.c:1312: var_deref_model: Passing null pointer "attrs" to function "ipa_s2n_save_objects(struct sss_domain_info *, struct req_input *, struct resp_attrs *, struct resp_attrs *)", which dereferences it.
> > sssd-1.12.2/src/providers/ipa/ipa_s2n_exop.c:1351:5: deref_parm: Directly dereferencing parameter "attrs".
>
> fixed
> > >                           state->simple_attrs);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_save_objects failed.\n");
> > >    goto done;
> > > }
> > >
> > > +done:
> > > if (ret == EOK) {
> > >    tevent_req_done(req);
> > > } else {
> > >    attrs = state->attrs;
> > >    tevent_req_error(req, ret);
> > > }
> > > return;
> > >
> > > +}
> >
> > The rest looks OK as well.
>
> New version attached.
>
> bye,
> Sumit

This new version contain some additional fixes.

bye,
Sumit

[Sumit Bose]

On Fri, Sep 26, 2014 at 04:06:13PM +0200, Sumit Bose wrote:
> On Thu, Sep 25, 2014 at 05:09:42PM +0200, Sumit Bose wrote:
> > On Wed, Sep 24, 2014 at 12:23:20PM +0200, Jakub Hrozek wrote:

...
> This new version contain some additional fixes.
>
> bye,
> Sumit

Hi,

Pavel and Jakub discovered an issue is e.g. the shell is not defined on
the sever, this new version should fix it.

bye,
Sumit

[Jakub Hrozek]

On Mon, Sep 29, 2014 at 04:50:15PM +0200, Sumit Bose wrote:
> On Fri, Sep 26, 2014 at 04:06:13PM +0200, Sumit Bose wrote:
> > On Thu, Sep 25, 2014 at 05:09:42PM +0200, Sumit Bose wrote:
> > > On Wed, Sep 24, 2014 at 12:23:20PM +0200, Jakub Hrozek wrote:
>
> ...
> > This new version contain some additional fixes.
> >
> > bye,
> > Sumit
>
> Hi,
>
> Pavel and Jakub discovered an issue is e.g. the shell is not defined on
> the sever, this new version should fix it.
>
> bye,
> Sumit

This latest version works for me against both the old plugin and the new
plugin. Thank you.

ACK.

[Jakub Hrozek]

On Mon, Sep 29, 2014 at 11:38:54PM +0200, Jakub Hrozek wrote:
> On Mon, Sep 29, 2014 at 04:50:15PM +0200, Sumit Bose wrote:
> > On Fri, Sep 26, 2014 at 04:06:13PM +0200, Sumit Bose wrote:
> > > On Thu, Sep 25, 2014 at 05:09:42PM +0200, Sumit Bose wrote:
> > > > On Wed, Sep 24, 2014 at 12:23:20PM +0200, Jakub Hrozek wrote:
> >
> > ...
> > > This new version contain some additional fixes.
> > >
> > > bye,
> > > Sumit
> >
> > Hi,
> >
> > Pavel and Jakub discovered an issue is e.g. the shell is not defined on
> > the sever, this new version should fix it.
> >
> > bye,
> > Sumit
>
> This latest version works for me against both the old plugin and the new
> plugin. Thank you.
>
> ACK.

With Tomas' help I ran the test through the IPA CI engine which found no
regressions.

Pushed to master: 28c70f003c7b330ab1d998a4eff1248d272a6ba9