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