On Thu, May 14, 2015 at 11:49:17AM -0400, Stephen Gallagher wrote:
> 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 <sgallagh(a)redhat.com>
> > > 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?
The current sdap_get_generic_ext_recv() handler looks like this:
ret = sdap_get_generic_ext_recv(subreq);
talloc_zfree(subreq);
if (ret == ERR_REFERRAL) {
if (dp_opt_get_bool(opts->basic, SDAP_REFERRALS)) {
tevent_req_error(req, ret);
return;
}
}
So unless referrals are supposed to be ignored, we fail the request on
receiving ERR_REFERRAL. We could easily turn that into:
ret = sdap_get_generic_ext_recv(subreq, mem_ctx, &ref_count, &refs);
talloc_zfree(subreq);
if (ref_count > 0) {
if (dp_opt_get_bool(opts->basic, SDAP_REFERRALS)) {
tevent_req_error(req, ret);
return;
}
}
I guess ignoring the referrals makes sense, if SDAP_REFERRALS is enabled,
the referral chasing would follow legitimate referrals. The referrals we
receive in LDAP results processing are just noise more or less...