[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