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!
[...]
+errno_t
+ad_gpo_get_sd_referral_recv(struct tevent_req *req,
+ TALLOC_CTX *mem_ctx,
+ char **_smb_host,
+ struct sysdb_attrs **_reply);
+
static void
ad_gpo_get_gpo_attrs_done(struct tevent_req *subreq)
{
struct tevent_req *req;
struct ad_gpo_process_gpo_state *state;
int ret;
int dp_error;
- size_t num_results;
+ size_t num_results, refcount;
struct sysdb_attrs **results;
+ char **refs;
+
+ req = tevent_req_callback_data(subreq, struct tevent_req);
+ state = tevent_req_data(req, struct ad_gpo_process_gpo_state);
+
+ ret = sdap_sd_search_recv(subreq, state,
+ &num_results, &results,
+ &refcount, &refs);
+ talloc_zfree(subreq);
+
+ if (ret != EOK) {
+ ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
+
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Unable to get GPO attributes: [%d](%s)\n",
+ ret, sss_strerror(ret));
+ ret = ENOENT;
+ goto done;
+ }
+
+ if ((num_results < 1) || (results == NULL)) {
+ if (refcount == 1) {
+ /* If we were redirected to a referral, process it.
+ * There must be a single referral result here; if we get
+ * more than one (or zero) it's a bug.
+ */
+
+ 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
+ tevent_req_set_callback(subreq,
ad_gpo_get_sd_referral_done, req);
+ ret = EAGAIN;
+ goto done;
+
+ } else {
+ const char *gpo_dn =
state->candidate_gpos[state->gpo_index]->gpo_dn;
+
+ DEBUG(SSSDBG_OP_FAILURE,
+ "No attrs found for GPO [%s].", gpo_dn);
+ ret = ENOENT;
+ goto done;
+ }
+ } else if (num_results > 1) {
+ DEBUG(SSSDBG_OP_FAILURE, "Received multiple replies\n");
+ ret = ERR_INTERNAL;
+ goto done;
+ }
+
+ ret = ad_gpo_sd_process_attrs(req, state->server_hostname, results[0]);
+
+done:
+
+ if (ret == EOK) {
+ tevent_req_done(req);
+ } else if (ret != EAGAIN) {
+ tevent_req_error(req, ret);
+ }
+}
[...]
+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);
+ 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..
+ 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?
@@ -2004,10 +2004,14 @@ struct sdap_sd_search_state {
LDAPControl **ctrls;
struct sdap_options *opts;
size_t reply_count;
struct sysdb_attrs **reply;
struct sdap_reply sreply;
+
+ /* Referrals returned by the search */
+ size_t ref_count;
+ char **refs;
};
static int sdap_sd_search_create_control(struct sdap_handle *sh,
int val,
LDAPControl **ctrl);
@@ -2135,16 +2139,30 @@ static errno_t sdap_sd_search_parse_entry(struct sdap_handle
*sh,
return EOK;
}
static void sdap_sd_search_done(struct tevent_req *subreq)
{
+ int ret;
+
struct tevent_req *req = tevent_req_callback_data(subreq,
struct tevent_req);
struct sdap_sd_search_state *state =
tevent_req_data(req, struct sdap_sd_search_state);
- return generic_ext_search_handler(subreq, state->opts);
+ ret = sdap_get_generic_ext_recv(subreq, state,
+ &state->ref_count,
+ &state->refs);
+ talloc_zfree(subreq);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "sdap_get_generic_ext_recv failed [%d]: %s\n",
+ ret, sss_strerror(ret));
+ tevent_req_error(req, ret);
+ return;
+ }
+
+ tevent_req_done(req);
}
static int sdap_sd_search_ctrls_destructor(void *ptr)
{
LDAPControl **ctrls = talloc_get_type(ptr, LDAPControl *);;
@@ -2156,19 +2174,24 @@ static int sdap_sd_search_ctrls_destructor(void *ptr)
}
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;
}
/* ==Attribute scoped search============================================ */
struct sdap_asq_search_state {
diff --git a/src/providers/ldap/sdap_async.h b/src/providers/ldap/sdap_async.h
index f2ea9bf2ecdaeefa897d414df051532c6219cb97..1861b3e7cbf4edb2df7a963b528ef18fd21d6f46
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?