On Mon, 2011-04-25 at 16:21 -0400, Stephen Gallagher wrote: > On Mon, 2011-04-25 at 15:33 -0400, Stephen Gallagher wrote: > > On Mon, 2011-04-25 at 14:26 -0400, Stephen Gallagher wrote: > > > Patch 0001: Added a debug message to see which record type we're > > > processing on each loop through sdap_process_message(). This is purely > > > informational. > > > > > > Patch 0002: Add support for paged LDAP results. > > > I changed the internals of sdap_get_generic_send() somewhat here and > > > added a new sdap_get_generic_internal() routine that can be used to > > > handle multiple calls to LDAP for a single request. We should be able to > > > use this in the future to handle Active Directory's non-standard > > > attribute-level paging as well (though that's not addressed in this > > > patch). > > > > > > Patch 0003: Add ldap_page_size configuration option > > > I made this a separate patch for simplicity of review. I set 1000 > > > records as the default, as this seemed to be the most-compatible value > > > among 389, OpenLDAP and ActiveDirectory as best I could determine. > > > > > > Simo made some comments on IRC that I have incorporated. > > > > First, I renamed sdap_get_generic_send_internal() to > > sdap_get_generic_step() and reordered it according to the tevent_req > > style. Second, I added talloc_zfree(state->op) to the beginning of > > sdap_get_generic_step() to ensure that we aren't carrying around extra > > sdap_op memory for subsequent pages. > > Simo had a few remaining comments. To follow tevent_req style, I've > moved forward declarations to be above the sdap_get_generic_send(). > > Also, there was a memory issue (I was accessing memory that had been > leaked). The cookie had gone out of scope, but I hadn't freed the memory > (which is why I saw no issues in my tests). > > I now make an appropriate copy of the cookie berval and free the > original memory.
One more change: the cookie is now copied with talloc_memdup() instead of talloc_strdup(), since it may not be a proper string (I'm looking at YOU Active Directory).
From 8cde8b64e572b737a493750755e1c4b152b70414 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Mon, 25 Apr 2011 13:11:16 -0400
Subject: [PATCH 1/4] Log the LDAP message type we're processing
---
src/providers/ldap/sdap_async.c | 57 +++++++++++++++++++++++++++++++++++++++
1 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index ed85a910818a38e5677ce7a2f409371a634b75a9..b96bbdea438cffb61988d8fccaf2c0cb375b201c 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -207,6 +207,61 @@ static void sdap_process_result(struct tevent_context *ev, void *pvt)
sdap_process_message(ev, sh, msg);
}
+static const char *sdap_ldap_result_str(int msgtype)
+{
+ switch (msgtype) {
+ case LDAP_RES_BIND:
+ return "LDAP_RES_BIND";
+
+ case LDAP_RES_SEARCH_ENTRY:
+ return "LDAP_RES_SEARCH_ENTRY";
+
+ case LDAP_RES_SEARCH_REFERENCE:
+ return "LDAP_RES_SEARCH_REFERENCE";
+
+ case LDAP_RES_SEARCH_RESULT:
+ return "LDAP_RES_SEARCH_RESULT";
+
+ case LDAP_RES_MODIFY:
+ return "LDAP_RES_MODIFY";
+
+ case LDAP_RES_ADD:
+ return "LDAP_RES_ADD";
+
+ case LDAP_RES_DELETE:
+ return "LDAP_RES_DELETE";
+
+ case LDAP_RES_MODDN:
+ /* These are the same result
+ case LDAP_RES_MODRDN:
+ case LDAP_RES_RENAME:
+ */
+ return "LDAP_RES_RENAME";
+
+ case LDAP_RES_COMPARE:
+ return "LDAP_RES_COMPARE";
+
+ case LDAP_RES_EXTENDED:
+ return "LDAP_RES_EXTENDED";
+
+ case LDAP_RES_INTERMEDIATE:
+ return "LDAP_RES_INTERMEDIATE";
+
+ case LDAP_RES_ANY:
+ return "LDAP_RES_ANY";
+
+ case LDAP_RES_UNSOLICITED:
+ return "LDAP_RES_UNSOLICITED";
+
+ default:
+ /* Unmatched, fall through */
+ break;
+ }
+
+ /* Unknown result type */
+ return "Unknown result type!";
+}
+
/* process a messgae calling the right operation callback.
* msg is completely taken care of (including freeeing it)
* NOTE: this function may even end up freeing the sdap_handle
@@ -248,6 +303,8 @@ static void sdap_process_message(struct tevent_context *ev,
return;
}
+ DEBUG(9, ("Message type: [%s]\n", sdap_ldap_result_str(msgtype)));
+
switch (msgtype) {
case LDAP_RES_SEARCH_ENTRY:
/* go and process entry */
--
1.7.4.4
From c7aebb7b4323f065e6e2c8bf55ea7f00bf431e08 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Mon, 25 Apr 2011 13:14:01 -0400
Subject: [PATCH 2/4] Enable paging support for LDAP
---
src/providers/ldap/sdap_async.c | 141 ++++++++++++++++++++++++++++++++------
1 files changed, 118 insertions(+), 23 deletions(-)
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index b96bbdea438cffb61988d8fccaf2c0cb375b201c..2d82c67740fa5adc78c0596261b3ddef6795937e 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -806,21 +806,24 @@ struct sdap_get_generic_state {
const char **attrs;
struct sdap_attr_map *map;
int map_num_attrs;
+ int timeout;
struct sdap_op *op;
+ struct berval cookie;
+
size_t reply_max;
size_t reply_count;
struct sysdb_attrs **reply;
};
+static errno_t sdap_get_generic_step(struct tevent_req *req);
+static void sdap_get_generic_done(struct sdap_op *op,
+ struct sdap_msg *reply,
+ int error, void *pvt);
static errno_t add_to_reply(struct sdap_get_generic_state *state,
struct sysdb_attrs *msg);
-static void sdap_get_generic_done(struct sdap_op *op,
- struct sdap_msg *reply,
- int error, void *pvt);
-
struct tevent_req *sdap_get_generic_send(TALLOC_CTX *memctx,
struct tevent_context *ev,
struct sdap_options *opts,
@@ -833,13 +836,9 @@ struct tevent_req *sdap_get_generic_send(TALLOC_CTX *memctx,
int map_num_attrs,
int timeout)
{
- struct tevent_req *req = NULL;
- struct sdap_get_generic_state *state = NULL;
- char *errmsg;
- int lret;
- int optret;
- int ret;
- int msgid;
+ errno_t ret;
+ struct sdap_get_generic_state *state;
+ struct tevent_req *req;
req = tevent_req_create(memctx, &state, struct sdap_get_generic_state);
if (!req) return NULL;
@@ -857,6 +856,40 @@ struct tevent_req *sdap_get_generic_send(TALLOC_CTX *memctx,
state->reply_max = 0;
state->reply_count = 0;
state->reply = NULL;
+ state->timeout = timeout;
+ state->cookie.bv_len = 0;
+ state->cookie.bv_val = NULL;
+
+ ret = sdap_get_generic_step(req);
+ if (ret != EOK) {
+ tevent_req_error(req, ret);
+ tevent_req_post(req, ev);
+ return req;
+ }
+
+ return req;
+}
+
+static errno_t sdap_get_generic_step(struct tevent_req *req)
+{
+ struct sdap_get_generic_state *state =
+ tevent_req_data(req, struct sdap_get_generic_state);
+ char *errmsg;
+ int lret;
+ int optret;
+ errno_t ret;
+ int msgid;
+
+ ber_int_t page_size = 1000;
+ char paging_criticality = 'F';
+ LDAPControl *page_control = NULL;
+ LDAPControl *m_controls[2] = { NULL, NULL };
+
+ /* Make sure to free any previous operations so
+ * if we are handling a large number of pages we
+ * don't waste memory.
+ */
+ talloc_zfree(state->op);
DEBUG(6, ("calling ldap_search_ext with [%s][%s].\n", state->filter,
state->search_base));
@@ -870,10 +903,26 @@ struct tevent_req *sdap_get_generic_send(TALLOC_CTX *memctx,
}
}
+ if (sdap_is_control_supported(state->sh,
+ LDAP_CONTROL_PAGEDRESULTS)) {
+ lret = ldap_create_page_control(state->sh->ldap,
+ page_size,
+ state->cookie.bv_val ?
+ &state->cookie :
+ NULL,
+ paging_criticality,
+ &page_control);
+ if (lret != LDAP_SUCCESS) {
+ ret = EIO;
+ goto done;
+ }
+ m_controls[0] = page_control;
+ }
+
lret = ldap_search_ext(state->sh->ldap, state->search_base,
state->scope, state->filter,
discard_const(state->attrs),
- false, NULL, NULL, NULL, 0, &msgid);
+ false, m_controls, NULL, NULL, 0, &msgid);
if (lret != LDAP_SUCCESS) {
DEBUG(3, ("ldap_search_ext failed: %s\n", ldap_err2string(lret)));
if (lret == LDAP_SERVER_DOWN) {
@@ -893,27 +942,23 @@ struct tevent_req *sdap_get_generic_send(TALLOC_CTX *memctx,
else {
ret = EIO;
}
- goto fail;
+ goto done;
}
DEBUG(8, ("ldap_search_ext called, msgid = %d\n", msgid));
ret = sdap_op_add(state, state->ev, state->sh, msgid,
- sdap_get_generic_done, req, timeout,
+ sdap_get_generic_done, req,
+ state->timeout,
&state->op);
if (ret != EOK) {
DEBUG(1, ("Failed to set up operation!\n"));
- goto fail;
+ goto done;
}
- return req;
-
-fail:
- tevent_req_error(req, ret);
- tevent_req_post(req, ev);
- return req;
+done:
+ return ret;
}
-
static void sdap_get_generic_done(struct sdap_op *op,
struct sdap_msg *reply,
int error, void *pvt)
@@ -925,6 +970,11 @@ static void sdap_get_generic_done(struct sdap_op *op,
char *errmsg = NULL;
int result;
int ret;
+ int lret;
+ ber_int_t total_count;
+ struct berval cookie;
+ LDAPControl **returned_controls = NULL;
+ LDAPControl *page_control;
if (error) {
tevent_req_error(req, error);
@@ -962,7 +1012,8 @@ static void sdap_get_generic_done(struct sdap_op *op,
case LDAP_RES_SEARCH_RESULT:
ret = ldap_parse_result(state->sh->ldap, reply->msg,
- &result, NULL, &errmsg, NULL, NULL, 0);
+ &result, NULL, &errmsg, NULL,
+ &returned_controls, 0);
if (ret != LDAP_SUCCESS) {
DEBUG(2, ("ldap_parse_result failed (%d)\n", state->op->msgid));
tevent_req_error(req, EIO);
@@ -978,6 +1029,50 @@ static void sdap_get_generic_done(struct sdap_op *op,
}
ldap_memfree(errmsg);
+ /* Determine if there are more pages to retrieve */
+ page_control = ldap_control_find(LDAP_CONTROL_PAGEDRESULTS,
+ returned_controls, NULL );
+ if (!page_control) {
+ /* No paging support. We are done */
+ tevent_req_done(req);
+ return;
+ }
+
+ lret = ldap_parse_pageresponse_control(state->sh->ldap, page_control,
+ &total_count, &cookie);
+ if (lret != LDAP_SUCCESS) {
+ DEBUG(1, ("Could not determine page control"));
+ tevent_req_error(req, EIO);
+ return;
+ }
+ DEBUG(7, ("Total count [%lu]\n", total_count));
+
+ if (cookie.bv_val != NULL && cookie.bv_len > 0) {
+ /* Cookie contains data, which means there are more requests
+ * to be processed.
+ */
+ talloc_zfree(state->cookie.bv_val);
+ state->cookie.bv_len = cookie.bv_len;
+ state->cookie.bv_val = talloc_memdup(state,
+ cookie.bv_val,
+ cookie.bv_len);
+ if (!state->cookie.bv_val) {
+ tevent_req_error(req, ENOMEM);
+ return;
+ }
+ ber_memfree(cookie.bv_val);
+
+ ret = sdap_get_generic_step(req);
+ if (ret != EOK) {
+ tevent_req_error(req, ENOMEM);
+ return;
+ }
+
+ return;
+ }
+
+ /* This was the last page. We're done */
+
tevent_req_done(req);
return;
--
1.7.4.4
From 5f988f1333e77f9768285e25f847620d1ef1382e Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Mon, 25 Apr 2011 14:17:45 -0400
Subject: [PATCH 3/4] Add ldap_page_size configuration option
---
src/config/SSSDConfig.py | 1 +
src/config/etc/sssd.api.d/sssd-ldap.conf | 1 +
src/man/sssd-ldap.5.xml | 14 ++++++++++++++
src/providers/ipa/ipa_common.c | 3 ++-
src/providers/ipa/ipa_common.h | 2 +-
src/providers/ldap/ldap_common.c | 3 ++-
src/providers/ldap/sdap.h | 2 ++
src/providers/ldap/sdap_async.c | 3 +--
src/providers/ldap/sdap_async_connection.c | 4 ++++
9 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/src/config/SSSDConfig.py b/src/config/SSSDConfig.py
index 02f76af28a0011c36faf4c99bd0abc6ae74bf03d..b613cfe4e435888906fac423df986be7b534faf1 100644
--- a/src/config/SSSDConfig.py
+++ b/src/config/SSSDConfig.py
@@ -140,6 +140,7 @@ option_strings = {
'ldap_krb5_ticket_lifetime' : _('Lifetime of TGT for LDAP connection'),
'ldap_deref' : _('How to dereference aliases'),
'ldap_dns_service_name' : _('Service name for DNS service lookups'),
+ 'ldap_page_size' : _('The number of records to retrieve in a single LDAP query'),
'ldap_entry_usn' : _('entryUSN attribute'),
'ldap_rootdse_last_usn' : _('lastUSN attribute'),
diff --git a/src/config/etc/sssd.api.d/sssd-ldap.conf b/src/config/etc/sssd.api.d/sssd-ldap.conf
index 8672f0b24fb698881f0d1d8d2c705c7acda6e198..e568c74d3d1763ada387fa11fe39876c7879a7bc 100644
--- a/src/config/etc/sssd.api.d/sssd-ldap.conf
+++ b/src/config/etc/sssd.api.d/sssd-ldap.conf
@@ -27,6 +27,7 @@ ldap_referrals = bool, None, false
ldap_krb5_ticket_lifetime = int, None, false
ldap_dns_service_name = str, None, false
ldap_deref = str, None, false
+ldap_page_size = int, None, false
[provider/ldap/id]
ldap_search_timeout = int, None, false
diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml
index 9d585e2ae87eab81444a4295ad246638673ae80a..49c9e49157f45449617028e75934e0f28a408278 100644
--- a/src/man/sssd-ldap.5.xml
+++ b/src/man/sssd-ldap.5.xml
@@ -855,6 +855,20 @@
</varlistentry>
<varlistentry>
+ <term>ldap_page_size (integer)</term>
+ <listitem>
+ <para>
+ Specify the number of records to retrieve from
+ LDAP in a single request. Some LDAP servers
+ enforce a maximum limit per-request.
+ </para>
+ <para>
+ Default: 1000
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term>ldap_tls_reqcert (string)</term>
<listitem>
<para>
diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c
index 7ba4fd5a43e54a6b2d8d24ce58d91f31a6ee8d9b..a0c728ef43a63e6d1c3fffd131362741a5e59d96 100644
--- a/src/providers/ipa/ipa_common.c
+++ b/src/providers/ipa/ipa_common.c
@@ -93,7 +93,8 @@ struct dp_option ipa_def_ldap_opts[] = {
/* Do not include ldap_auth_disable_tls_never_use_in_production in the
* manpages or SSSDConfig API
*/
- { "ldap_auth_disable_tls_never_use_in_production", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }
+ { "ldap_auth_disable_tls_never_use_in_production", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
+ { "ldap_page_size", DP_OPT_NUMBER, { .number = 1000 }, NULL_NUMBER }
};
struct sdap_attr_map ipa_attr_map[] = {
diff --git a/src/providers/ipa/ipa_common.h b/src/providers/ipa/ipa_common.h
index 12a4927072eaed47072b32e18eced342be5969c4..02c4276cab7382749ac113f75e1019636daca991 100644
--- a/src/providers/ipa/ipa_common.h
+++ b/src/providers/ipa/ipa_common.h
@@ -35,7 +35,7 @@ struct ipa_service {
/* the following defines are used to keep track of the options in the ldap
* module, so that if they change and ipa is not updated correspondingly
* this will trigger a runtime abort error */
-#define IPA_OPTS_BASIC_TEST 49
+#define IPA_OPTS_BASIC_TEST 50
/* the following define is used to keep track of the options in the krb5
* module, so that if they change and ipa is not updated correspondingly
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index 11c4491f90990c512becdc06c58ddbfec02940c5..12028b0131f4f4cdaca2800bdf135ee98617d045 100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -86,7 +86,8 @@ struct dp_option default_basic_opts[] = {
/* Do not include ldap_auth_disable_tls_never_use_in_production in the
* manpages or SSSDConfig API
*/
- { "ldap_auth_disable_tls_never_use_in_production", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }
+ { "ldap_auth_disable_tls_never_use_in_production", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
+ { "ldap_page_size", DP_OPT_NUMBER, { .number = 1000 }, NULL_NUMBER }
};
struct sdap_attr_map generic_attr_map[] = {
diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
index c06b8a3b76920b5f25250d54749ff841673e8d8a..0f6b755041681e1b59d2e07583d38ab51eea1d1e 100644
--- a/src/providers/ldap/sdap.h
+++ b/src/providers/ldap/sdap.h
@@ -77,6 +77,7 @@ struct sdap_handle {
bool connected;
/* Authentication ticket expiration time (if any) */
time_t expire_time;
+ ber_int_t page_size;
struct sdap_fd_events *sdap_fd_events;
@@ -192,6 +193,7 @@ enum sdap_basic_opt {
SDAP_CHPASS_DNS_SERVICE_NAME,
SDAP_ENUM_SEARCH_TIMEOUT,
SDAP_DISABLE_AUTH_TLS,
+ SDAP_PAGE_SIZE,
SDAP_OPTS_BASIC /* opts counter */
};
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 2d82c67740fa5adc78c0596261b3ddef6795937e..6953426171e9a8dc3872c83722b54f92176d8906 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -880,7 +880,6 @@ static errno_t sdap_get_generic_step(struct tevent_req *req)
errno_t ret;
int msgid;
- ber_int_t page_size = 1000;
char paging_criticality = 'F';
LDAPControl *page_control = NULL;
LDAPControl *m_controls[2] = { NULL, NULL };
@@ -906,7 +905,7 @@ static errno_t sdap_get_generic_step(struct tevent_req *req)
if (sdap_is_control_supported(state->sh,
LDAP_CONTROL_PAGEDRESULTS)) {
lret = ldap_create_page_control(state->sh->ldap,
- page_size,
+ state->sh->page_size,
state->cookie.bv_val ?
&state->cookie :
NULL,
diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
index 500e5f886f0855f10171096fc9dc29a4a5233fea..40ed585c6bf555478da93b09e267292a878080a7 100644
--- a/src/providers/ldap/sdap_async_connection.c
+++ b/src/providers/ldap/sdap_async_connection.c
@@ -108,6 +108,10 @@ struct tevent_req *sdap_connect_send(TALLOC_CTX *memctx,
talloc_zfree(req);
return NULL;
}
+
+ state->sh->page_size = dp_opt_get_int(state->opts->basic,
+ SDAP_PAGE_SIZE);
+
/* Initialize LDAP handler */
lret = ldap_initialize(&state->sh->ldap, uri);
if (lret != LDAP_SUCCESS) {
--
1.7.4.4
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
