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(a)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(a)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)