[SSSD] [PATCH] Connect to GC during enumeration

Sumit Bose sbose at redhat.com
Wed Jan 29 12:33:52 UTC 2014


On Tue, Jan 28, 2014 at 03:34:52PM +0100, Jakub Hrozek wrote:
> 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 at 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.

Code looks good and my tests passed, so ACK.

bye,
Sumit



More information about the sssd-devel mailing list