On Thu, 2015-05-14 at 17:42 +0200, Jakub Hrozek wrote: > On Wed, May 06, 2015 at 02:26:30PM -0400, Stephen Gallagher wrote: > > Patch 0001: LDAP: Support returning referral information > > > > Some callers may be interested in the raw referral values returned > > from > > a lookup. This patch allows interested consumers to get these > > referrals > > back and process them if they wish. It does not implement a generic > > automatic following of referrals. > > > > > > > > Patch 0002: 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 > > Thanks a lot for the patches! I have one questions about the first > patch > and two nitpicks about the second. I'm also still waiting for the > Coverity results, but the queue seems to be quite long.. > > > From 3f8826061d34639ddaaf245947085ea577e77fbe Mon Sep 17 00:00:00 > > 2001 > > From: Stephen Gallagher <[email protected]> > > Date: Fri, 1 May 2015 11:42:06 -0400 > > Subject: [PATCH 1/2] LDAP: Support returning referral information > > > > Some callers may be interested in the raw referral values returned > > from > > a lookup. This patch allows interested consumers to get these > > referrals > > back and process them if they wish. It does not implement a generic > > automatic following of referrals. > > [...] > > > } else if (result == LDAP_REFERRAL) { > > - if (refs != NULL) { > > - for (i = 0; refs[i]; i++) { > > - DEBUG(SSSDBG_TRACE_LIBS, "Ref: %s\n", > > refs[i]); > > - } > > - ldap_memvfree((void **) refs); > > + ret = sdap_get_generic_ext_add_references(state, > > refs); > > + if (ret != EOK) { > > + DEBUG(SSSDBG_OP_FAILURE, > > + "sdap_get_generic_ext_add_references failed: > > %s(%d)", > > + sss_strerror(ret), ret); > > + tevent_req_error(req, ret); > > } > > - ldap_memfree(errmsg); > > - tevent_req_error(req, ERR_REFERRAL); > > - return; > > This is a change in behaviour. Previously, we would always return > ERR_REFERRAL and let the caller handle the error code based on > whether > the caller ignores referrals (like the AD Provider does by default) > or > not. > > Do you think it's OK to always treat referrals as a success now, even > for the plain LDAP provider? If so, then I guess we can remove the > ERR_REFERRAL special-case and the error code as well. >
Hmm, that's a difficult question. I'd *prefer* to always return the
reference list (as I'm doing here) and leave the decision on whether or
how to handle it up to the callers. But you're right, it's a change in
behavior right now.
That being said, it shouldn't functionally be any different, since we
pretty much ignore ERR_REFERRAL anyway (we only return the records we
actually received, if any). Or am I mistaken in that?
> > + /* For referrals, we need to fall through as if it was
> > LDAP_SUCCESS */
> > } else if (result != LDAP_SUCCESS && result !=
> > LDAP_NO_SUCH_OBJECT) {
> > DEBUG(SSSDBG_OP_FAILURE,
> > "Unexpected result from ldap: %s(%d), %s\n",
> > sss_ldap_err2string(result), result,
> > errmsg ? errmsg : "no errmsg set");
>
> > From 9a45e8523540c9eff8a8b9d84da81c9ee77a91de Mon Sep 17 00:00:00
> > 2001
> > From: Stephen Gallagher <[email protected]>
> > 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
>
> [...]
>
> > +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);
>
> You missed a NULL check here.
>
Oops, will add in the next round of patches, once we decide how to
handle the first patch.
> > + tevent_req_set_callback(subreq,
> > ad_gpo_get_sd_referral_search_done, req);
> > +
> > +}
> > +
> > +static void
> > +ad_gpo_get_sd_referral_search_done(struct tevent_req *subreq)
> > +{
> > + errno_t ret;
> > + int dp_error;
> > + size_t num_results, num_refs;
> > + struct sysdb_attrs **results = NULL;
> > + char **refs;
> > + 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_sd_search_recv(subreq, NULL,
> > + &num_results, &results,
> > + &num_refs, &refs);
>
> And a 'ret' check here.
ditto
>
> > + talloc_zfree(subreq);
> > +
> > + ret = sdap_id_op_done(state->ref_op, ret, &dp_error);
> > + if (ret != EOK) {
> > + DEBUG(SSSDBG_OP_FAILURE,
> > + "Unable to get GPO attributes: [%d](%s)\n",
> > + ret, sss_strerror(ret));
> > + ret = ENOENT;
> > + goto done;
> > + }
> > +
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
