On Tue, May 26, 2015 at 03:56:35PM -0400, Stephen Gallagher wrote: > Sorry for the delay; two new patches attached. > > This patch fixes the two missing error checks in the AD GPO code as > well as making several changes to the general LDAP support of > referrals. > > While I was looking through this, I discovered a bug that resulted in > referrals for more information not being returned to the caller (this > is different from referral-as-final-result). I tweaked the code so that > this would come back as well. I also added some extra debugging at > level 9 so we can see when these occur, what they were and that they > were ignored. > > As discussed above, I just changed the referral check around > sdap_get_generic_ext_recv() to check for ref_count > 0 instead of > ERR_REFERRALS. I didn't remove that completely from the system just in > case we decide to use the error for something else involving referrals > in the future. > > I retested these patches against the problematic cross-realm GPO case, > which worked successfully.
> From 20191f9c34336b3920db8d5774593c4562cefdb7 Mon Sep 17 00:00:00 2001 > From: Stephen Gallagher <sgall...@redhat.com> > Date: Fri, 1 May 2015 11:42:06 -0400 > Subject: [PATCH 1/2] LDAP: Support returning referral information Thank you, this was really useful. I tested the referral patch with universal groups and GC support disabled which always triggers referrals to the trusted domains. The referrals were ignored as they're supposed to, so we're not going to regress there.. ACK > From 9369111f0775cbc4c10b5857046c756ae510033f Mon Sep 17 00:00:00 2001 > From: Stephen Gallagher <sgall...@redhat.com> > Date: Fri, 1 May 2015 16:26:36 -0400 > Subject: [PATCH 2/2] AD GPO: Support processing referrals > > For GPOs assigned to a site, it's possible that their definition > actually exists in another domain. To retrieve this information, > we need to follow the referral and perform a base search on > another domain controller. > > Resolves: > https://fedorahosted.org/sssd/ticket/2645 Some nitpicks are inline, but mostly the patch looks good! [...] > +errno_t > +ad_gpo_get_sd_referral_recv(struct tevent_req *req, > + TALLOC_CTX *mem_ctx, > + char **_smb_host, > + struct sysdb_attrs **_reply); > + > static void > ad_gpo_get_gpo_attrs_done(struct tevent_req *subreq) > { > struct tevent_req *req; > struct ad_gpo_process_gpo_state *state; > int ret; > int dp_error; > - size_t num_results; > + size_t num_results, refcount; > struct sysdb_attrs **results; > + char **refs; > + > + req = tevent_req_callback_data(subreq, struct tevent_req); > + state = tevent_req_data(req, struct ad_gpo_process_gpo_state); > + > + ret = sdap_sd_search_recv(subreq, state, > + &num_results, &results, > + &refcount, &refs); > + talloc_zfree(subreq); > + > + if (ret != EOK) { > + ret = sdap_id_op_done(state->sdap_op, ret, &dp_error); > + > + DEBUG(SSSDBG_OP_FAILURE, > + "Unable to get GPO attributes: [%d](%s)\n", > + ret, sss_strerror(ret)); > + ret = ENOENT; > + goto done; > + } > + > + if ((num_results < 1) || (results == NULL)) { > + if (refcount == 1) { > + /* If we were redirected to a referral, process it. > + * There must be a single referral result here; if we get > + * more than one (or zero) it's a bug. > + */ > + > + subreq = ad_gpo_get_sd_referral_send(state, state->ev, > + state->access_ctx, > + state->opts, > + refs[0], > + state->host_domain, > + state->timeout); Missing NULL check for subreq > + tevent_req_set_callback(subreq, ad_gpo_get_sd_referral_done, > req); > + ret = EAGAIN; > + goto done; > + > + } else { > + const char *gpo_dn = > state->candidate_gpos[state->gpo_index]->gpo_dn; > + > + DEBUG(SSSDBG_OP_FAILURE, > + "No attrs found for GPO [%s].", gpo_dn); > + ret = ENOENT; > + goto done; > + } > + } else if (num_results > 1) { > + DEBUG(SSSDBG_OP_FAILURE, "Received multiple replies\n"); > + ret = ERR_INTERNAL; > + goto done; > + } > + > + ret = ad_gpo_sd_process_attrs(req, state->server_hostname, results[0]); > + > +done: > + > + if (ret == EOK) { > + tevent_req_done(req); > + } else if (ret != EAGAIN) { > + tevent_req_error(req, ret); > + } > +} [...] > +static void > +ad_gpo_get_sd_referral_conn_done(struct tevent_req *subreq) > +{ > + errno_t ret; > + int dp_error; > + const char *attrs[] = AD_GPO_ATTRS; > + > + struct tevent_req *req = > + tevent_req_callback_data(subreq, struct tevent_req); > + struct ad_gpo_get_sd_referral_state *state = > + tevent_req_data(req, struct ad_gpo_get_sd_referral_state); > + > + ret = sdap_id_op_connect_recv(subreq, &dp_error); > + talloc_zfree(subreq); > + if (ret != EOK) { > + if (dp_error == DP_ERR_OFFLINE) { > + DEBUG(SSSDBG_TRACE_FUNC, > + "Backend is marked offline, retry later!\n"); > + tevent_req_done(req); > + } else { > + DEBUG(SSSDBG_MINOR_FAILURE, > + "Cross-realm GPO processing failed to connect to " \ > + "referred LDAP server: (%d)[%s]\n", > + ret, sss_strerror(ret)); > + tevent_req_error(req, ret); > + } > + return; > + } > + > + /* Request the referred GPO data */ > + subreq = sdap_sd_search_send(state, state->ev, state->opts, > + sdap_id_op_handle(state->ref_op), > + state->gpo_dn, > + SECINFO_DACL, > + attrs, > + state->timeout); > + if (subreq == NULL) { > + DEBUG(SSSDBG_OP_FAILURE, "sdap_sd_search_send failed.\n"); Copy-n-paste bug; the error handler should read: tevent_req_error(req, ENOMEM); return; Interestingly enough, gcc emits a warning here, but Coverity didn't catch this bug.. > + return ENOMEM; > + } > + tevent_req_set_callback(subreq, ad_gpo_get_sd_referral_search_done, req); > + > +} [...] > index > 2ffc2a170c2277aa2ea40e84bb697c62542aa266..760fb3df5148f46bf0e7e0fdb9110456685a914c > 100644 > --- a/src/providers/ldap/sdap_async.c > +++ b/src/providers/ldap/sdap_async.c The changes to the sdap_async module look OK to me, but can you split them into a separate patch, please? > @@ -2004,10 +2004,14 @@ struct sdap_sd_search_state { > LDAPControl **ctrls; > struct sdap_options *opts; > size_t reply_count; > struct sysdb_attrs **reply; > struct sdap_reply sreply; > + > + /* Referrals returned by the search */ > + size_t ref_count; > + char **refs; > }; > > static int sdap_sd_search_create_control(struct sdap_handle *sh, > int val, > LDAPControl **ctrl); > @@ -2135,16 +2139,30 @@ static errno_t sdap_sd_search_parse_entry(struct > sdap_handle *sh, > return EOK; > } > > static void sdap_sd_search_done(struct tevent_req *subreq) > { > + int ret; > + > struct tevent_req *req = tevent_req_callback_data(subreq, > struct tevent_req); > struct sdap_sd_search_state *state = > tevent_req_data(req, struct sdap_sd_search_state); > > - return generic_ext_search_handler(subreq, state->opts); > + ret = sdap_get_generic_ext_recv(subreq, state, > + &state->ref_count, > + &state->refs); > + talloc_zfree(subreq); > + if (ret != EOK) { > + DEBUG(SSSDBG_OP_FAILURE, > + "sdap_get_generic_ext_recv failed [%d]: %s\n", > + ret, sss_strerror(ret)); > + tevent_req_error(req, ret); > + return; > + } > + > + tevent_req_done(req); > } > > static int sdap_sd_search_ctrls_destructor(void *ptr) > { > LDAPControl **ctrls = talloc_get_type(ptr, LDAPControl *);; > @@ -2156,19 +2174,24 @@ static int sdap_sd_search_ctrls_destructor(void *ptr) > } > > int sdap_sd_search_recv(struct tevent_req *req, > TALLOC_CTX *mem_ctx, > size_t *_reply_count, > - struct sysdb_attrs ***_reply) > + struct sysdb_attrs ***_reply, > + size_t *_ref_count, > + char ***_refs) > { > struct sdap_sd_search_state *state = tevent_req_data(req, > struct sdap_sd_search_state); > TEVENT_REQ_RETURN_ON_ERROR(req); > > *_reply_count = state->sreply.reply_count; > *_reply = talloc_steal(mem_ctx, state->sreply.reply); > > + *_ref_count = state->ref_count; > + *_refs = talloc_steal(mem_ctx, state->refs); > + Would it make sense to allow NULL ref_count and refs? Like this: if (_ref_count) { *_ref_count = state->ref_count; } if (_refs) { *_refs = talloc_steal(mem_ctx, state->refs); } > return EOK; > } > > /* ==Attribute scoped search============================================ */ > struct sdap_asq_search_state { > diff --git a/src/providers/ldap/sdap_async.h b/src/providers/ldap/sdap_async.h > index > f2ea9bf2ecdaeefa897d414df051532c6219cb97..1861b3e7cbf4edb2df7a963b528ef18fd21d6f46 > 100644 > --- a/src/providers/ldap/sdap_async.h > +++ b/src/providers/ldap/sdap_async.h > @@ -249,13 +249,25 @@ sdap_sd_search_send(TALLOC_CTX *memctx, > const char *base_dn, > int sd_flags, > const char **attrs, > int timeout); > int sdap_sd_search_recv(struct tevent_req *req, > - TALLOC_CTX *mem_ctx, > - size_t *reply_count, > - struct sysdb_attrs ***reply); > + TALLOC_CTX *mem_ctx, > + size_t *_reply_count, > + struct sysdb_attrs ***_reply, > + size_t *_ref_count, > + char ***_refs); > + > +struct tevent_req * > +sdap_sd_follow_referral_send(TALLOC_CTX *mem_ctx, > + const char *ref); > + > +errno_t > +sdap_sd_follow_referral_recv(struct tevent_req *req, > + TALLOC_CTX *mem_ctx, > + size_t *_reply_count, > + struct sysdb_attrs ***_reply); I only see declarations, not the function bodies or uses? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
