[SSSD] [PATCHES] Support GPOs referred from other domains
Stephen Gallagher
sgallagh at redhat.com
Thu May 14 15:49:17 UTC 2015
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 at 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?
> > + /* 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 <sgallagh at 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
>
> [...]
>
> > +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;
> > + }
> > +
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150514/4fdd6904/attachment.sig>
More information about the sssd-devel
mailing list