[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