[SSSD] [PATCHES] Support GPOs referred from other domains

Stephen Gallagher sgallagh at redhat.com
Wed May 27 19:58:55 UTC 2015


On Wed, 2015-05-27 at 11:15 +0200, Jakub Hrozek wrote:
> 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 <sgallagh at 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 <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
> 
> Some nitpicks are inline, but mostly the patch looks good!
> 
> [...]
> 
> > +
> > +            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
> 

Good catch. Fixed.


> > +    /* 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..
> 

I'm curious what warning GCC saw. In any case, fixed.

> > +        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?
> 

I considered it, but then decided against splitting it. There really
isn't sufficient value in splitting it. The sdap_async.c portion is
fairly short and clearly only useful in context.


> >  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;
> >  }
> >  

I don't see a real need for it, but since it's not harmful, I added it
anyway.



> > f2ea9bf2ecdaeefa897d414df051532c6219cb97..1861b3e7cbf4edb2df7a963b5
> > 28ef18fd21d6f46
> > 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?
Whoops. That was part of an aborted change I made. I forgot to remove
the prototypes from the header.



New patches attached. I retested the changes as well. (On my new
Vagrant setup, no less)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-AD-GPO-Support-processing-referrals.patch
Type: text/x-patch
Size: 29578 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150527/deb348e8/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-LDAP-Support-returning-referral-information.patch
Type: text/x-patch
Size: 9198 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150527/deb348e8/attachment-0001.bin>
-------------- 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/20150527/deb348e8/attachment.sig>


More information about the sssd-devel mailing list