On Tue, Jan 28, 2014 at 11:17:55AM +0100, Sumit Bose wrote:
> > Thank you for the review. A new set of patches is attached.
>
> Patches 1, 2, 3 and 5 are looking good.
>
> In patch 4 you add ad_enum_subdom_send() and ad_enum_subdom_done()
> (without calling them) and in patch 6 you remove them again.
Right, that's a remnant from a previous iteration. The code is
completely removed now.
>
> Other comments for patch 6 are inline.
>
> bye,
> Sumit
>
>
> > From 066ce9945b6af3748f0ead06213f0180a742c542 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek <jhrozek(a)redhat.com>
> > Date: Wed, 22 Jan 2014 15:21:24 +0100
> > Subject: [PATCH 6/6] AD: Establish cross-domain memberships after enumeration
> > finishes
> >
> > Because domain enumeration currently works for each domain separately,
> > the code has to establish cross-domain memberships after all domains are
> > enumerated. The code works as follows:
> >
> > 1) check if any *sub*domains were enumerated. If not, do nothing
> > 2) if any of the groups saved had more original members than
> > sysdb members, check if members of these groups can be linked now
> > that all users and groups are saved using the orig_member
> > attribute of the group matched against originalDN member of the
> > user.
> >
> > Related:
> >
https://fedorahosted.org/sssd/ticket/2142
> > ---
> > src/providers/ad/ad_id.c | 515
+++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 390 insertions(+), 125 deletions(-)
> >
> > diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
> > index
1bdacf74f0e6de2120603401bcf115549a03ac4f..8818869f73be4afa8b36c0da0341391b569aafd2 100644
> > --- a/src/providers/ad/ad_id.c
> > +++ b/src/providers/ad/ad_id.c
> > @@ -420,10 +420,13 @@ struct ad_enumeration_state {
> > struct tevent_context *ev;
> >
> > struct sdap_domain *sdom;
> > + struct sdap_domain *sditer;
> > };
> >
[snip]
> > -struct tevent_req *
> > -ad_enum_subdom_send(TALLOC_CTX *mem_ctx,
> > - struct tevent_context *ev,
> > - struct be_ctx *be_ctx,
> > - struct be_ptask *be_ptask,
> > - void *pvt)
> > -{
>
> see above.
Squashed into patch 4
> > + state->sditer = state->sditer->next;
> > + if (state->sditer != NULL) {
> > + subdom_id_ctx = talloc_get_type(state->sdom->pvt, struct
ad_id_ctx);
> > + if (subdom_id_ctx == NULL) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, ("Cannot retrieve subdomain
ad_id_ctx!\n"));
> > + tevent_req_error(req, EFAULT);
> > + return;
> > + }
> > +
> > + ret = ad_enum_sdom(req, state->sditer, state->sditer->pvt);
>
> ret is not checked here as well.
Fixed. I will also run this version of patches through Coverity to see
if there are any new warnings.
> > + filter = talloc_asprintf(tmp_ctx, "(%s=*)", SYSDB_NAME);
> > + if (filter == NULL) {
> > + ret = ENOMEM;
> > + goto done;
> > + }
> > +
> > + ret = sysdb_search_groups(tmp_ctx, dom, filter, attrs, &count,
&msgs);
> > + if (ret != EOK) {
> > + goto done;
> > + }
>
> You try to process all groups in a domain in a single batch. We might
> run into performance/memory issues if there are a lot of groups. But I
> think we can keep it as it is for the time being since the easy
> workaround is to disable enumeration :-)
Yes, I thought this would come up during review, but I couldn't think of
a better way with the current sysdb code. ldb seems to have some kind of
paging support (at least judging by a quick look to ldb.h), but our API
does not use it. So in this code I just limited the number of attributes
we request to a minimum to keep the reply small.
btw the same problem is on the responder side as well -- sysdb_enumpwent
and similar calls also retrieve everything in a single go.
And I agree subdomain enumeration should be strongly discouraged.
Unfortunately, many users are still running enumeration and I'm afraid
they'll continue doing so even with trusted domains.
>
> > +
> > + for (i = 0; i < count; i++) {
> > + ret = ad_group_extra_members(tmp_ctx, msgs[i], dom, &group_only);
> > + if (ret != EOK) {
> > + DEBUG(SSSDBG_OP_FAILURE, ("Failed to check extra
members\n"));
> > + } else if (group_only == NULL) {
> > + DEBUG(SSSDBG_TRACE_INTERNAL, ("No extra members\n"));
> > + continue;
> > + }
> > +
> > + /* Group has extra members */
> > + for (mi = 0; group_only[mi]; mi++) {
> > + ret = ad_group_add_member(opts, dom, msgs[i]->dn,
group_only[mi]);
> > + if (ret != EOK) {
> > + DEBUG(SSSDBG_MINOR_FAILURE, ("Failed to add [%s]:
%s\n",
> > + group_only[mi], strerror(ret)));
> > + continue;
> > + }
> > + }
I also added a talloc_free(group_only) here to keep the memory usage
down.
> > + }
> > +
> > + ret = sysdb_transaction_commit(dom->sysdb);
> > + if (ret != EOK) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to commit
transaction\n"));
> > + goto done;
> > + }
> > + in_transaction = false;
> > +
> > + ret = EOK;
> > +done:
> > + if (in_transaction) {
> > + sret = sysdb_transaction_cancel(dom->sysdb);
> > + if (sret != EOK) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, ("Could not cancel
transaction\n"));
> > + }
> > + }
> > + talloc_free(tmp_ctx);
> > + return ret;
> > +}
> > +
[snip]
> > +static char **
> > +ad_group_orig_members(TALLOC_CTX *mem_ctx, struct ldb_message_element *om)
>
> this looks like a generic ldb_message_element_to_string_list call. I
> would recommend to rename it and maybe move it to utils/ or db/ ebcause
> it might be useful for others as well?
Done. I choose db/ after all because all the includes (ldb.h and
similar) are already there. This is done in a new patch along with a
unit test.
>
> > +{
> > + char **odn_list;
> > + size_t i;
> > +
> > + /* Get the list of original DN attributes the group had in AD */
> > + odn_list = talloc_zero_array(mem_ctx, char *, om->num_values + 1);
> > + if (odn_list == NULL) {
> > + return NULL;
> > + }
> > +
> > + for (i=0; i < om->num_values; i++) {
> > + odn_list[i] = (char *) om->values[i].data;
> > + }
> > +
> > + return odn_list;
> > +}
[snip]
> > + /* Get a list of their original DNs */
> > + oi = 0;
> > + for (i = 0; i < m_count; i++) {
> > + odn = ldb_msg_find_attr_as_string(members[i], SYSDB_ORIG_DN, NULL);
> > + if (odn == NULL) {
> > + continue;
> > + }
> > +
> > + odn_list[oi] = talloc_strdup(odn_list, odn);
> > + if (odn_list[oi] == NULL) {
> > + continue;
>
> do you really want to continue if talloc_strdup() fails?
No, fixed.
Thank you for the review. New patches are attached.