[SSSD] [PATCH] Adding option to disable retrieving large AD groups.
Lukas Slebodnik
lslebodn at redhat.com
Wed May 15 15:30:51 UTC 2013
On (15/05/13 13:54), Jakub Hrozek wrote:
>On Tue, May 14, 2013 at 07:04:42PM +0200, Lukas Slebodnik wrote:
>> On (14/05/13 16:04), Jakub Hrozek wrote:
>> >On Mon, May 13, 2013 at 05:08:44PM +0200, Lukas Slebodnik wrote:
>> >> ehlo,
>> >>
>> >> Jakub, Pavel and me disscus about ticket
>> >> https://fedorahosted.org/sssd/ticket/1823
>> >> "getgrnam / getgrgid for large user groups is too slow due to range retrieval
>> >> functionality"
>> >> We decided to add new option to turn the range retrieval with AD. Another
>> >> solution could be to add new option to limit the number of group members
>> >> processed.
>> >>
>> >> Any comments are welcomed.
>> >>
>> >> This commit adds new option ldap_disable_large_groups with default value FALSE.
>> >> If this option is enabled, large groups(>1500) will not be retrieved and
>> >> behaviour will be similar like was before commit ae8d047122c
>> >> "LDAP: Handle very large Active Directory groups"
>> >>
>> >> LS
>> >
>> >This is a good start, but I would prefer if the option name included
>> >"range retrieval", for two reasons:
>> > * the current option name would make the user think that the option
>> > works with every provider
>> > * in general, the range extension might not be limited to group
>> > objects on the AD side.
>> >
>> >What about "ldap_disable_range_retrieval" ?
>> >
>> Option renamed.
>>
>> >Similarly the parameter of sdap_parse_entry should be renamed,
>> >sdap_parse_entry can be used to parse any generic entry, not just group
>> >(it should have no knowledge about existence of groups).
>> >
>> >I was also wondering if the check could be moved all the way to
>> >sdap_parse_range() to keep the knowledge about range retrievals there.
>> >Maybe introduce a new error code (or use something from errno) that would
>> >be returned when the feature is off. If this is not possible because it
>> >might break the flow of sdap_parse_entry(), then please #define the
>> >";range" string and put it into sdap_range.h.
>> >
>> >Can you also send a follow-up patch that removes the functions
>> >sdap_parse_user() and sdap_parse_group(). Looks like dead code.
>> >
>> functions removed in first patch.
>>
>> >> --- a/src/man/sssd-ldap.5.xml
>> >> +++ b/src/man/sssd-ldap.5.xml
>> >> @@ -1201,6 +1201,25 @@
>> >> </varlistentry>
>> >>
>> >> <varlistentry>
>> >> + <term>ldap_disable_large_groups (boolean)</term>
>> >> + <listitem>
>> >> + <para>
>> >> + Disable very large Active Directory groups.
>> >> + </para>
>> >> + <para>
>> >> + This option has effect only, if LDAP server is
>> >> + Active Directory
>> >
>> > what about "...if SSSD is configured as an Active Directory client..." ?
>> >
>> Changed.
>>
>> Updated patches are attached.
>>
>> LS
>
>[PATCH 1/2] Removing unused functions.
>Ack
>
>[PATCH 2/2] Adding option to disable retrieving large AD groups.
>
>> index 799213300857f182b092287741a40665a0ae62d5..266944908d12a64d439c59a28d60ab9cea5b4b1f 100644
>> --- a/src/man/sssd-ldap.5.xml
>> +++ b/src/man/sssd-ldap.5.xml
>> @@ -1201,6 +1201,27 @@
>> </varlistentry>
>>
>> <varlistentry>
>> + <term>ldap_disable_range_retrieval (boolean)</term>
>> + <listitem>
>> + <para>
>> + Disable Active Directory range retrieval.
>> + </para>
>> + <para>
>> + This option has effect only, if SSSD is configured
>> + as an Active Directory client.
>> + Active Directory 2008R2 allows only 1500 group
>> + members to be retrieved in single lookup. Therefore
>> + if this option is enabled, members of larger groups
>> + will not be retrieved. 1500 is default value of
>> + MaxValRange policy.
>
>One more request to change the man page - can we say something like:
>
>"Active Directory limits the number of members to be retrieved in a
>single lookup using the MaxValRange policy (which defaults to 1500
>members). If a group contains more members, the reply would include an
>AD-specific range extension. This option disables parsing of the
>range extension, therefore large groups will appear as having no
>members".
>
Thank you very much for this paragraph.
>> + </para>
>> + <para>
>> + Default: False
>> + </para>
>> + </listitem>
>> + </varlistentry>
>> +
>> + <varlistentry>
>> <term>ldap_sasl_minssf (integer)</term>
>> <listitem>
>> <para>
>> diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h
>> --- a/src/providers/ldap/sdap.c
>> +++ b/src/providers/ldap/sdap.c
>> @@ -95,7 +95,8 @@ int sdap_get_map(TALLOC_CTX *memctx,
>> int sdap_parse_entry(TALLOC_CTX *memctx,
>> struct sdap_handle *sh, struct sdap_msg *sm,
>> struct sdap_attr_map *map, int attrs_num,
>> - struct sysdb_attrs **_attrs, char **_dn)
>> + struct sysdb_attrs **_attrs, char **_dn,
>> + bool disable_range_retrieval)
>> {
>> struct sysdb_attrs *attrs;
>> BerElement *ber = NULL;
>> @@ -190,23 +191,25 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
>> while (str) {
>> base64 = false;
>>
>
>> - ret = sdap_parse_range(tmp_ctx, str, &base_attr, &range_offset);
>> - if (ret == EAGAIN) {
>> + ret = sdap_parse_range(tmp_ctx, str, &base_attr, &range_offset,
>> + disable_range_retrieval);
>> + switch(ret) {
>> + case EAGAIN:
>> /* This attribute contained range values and needs more to
>> * be retrieved
>> */
>> /* TODO: return the set of attributes that need additional retrieval
>> * For now, we'll continue below and treat it as regular values.
>> */
>> -
>> - } else if (ret != EOK) {
>> + case ECANCELED:
I added comment /* FALLTHROUGH */
>> + case EOK:
>> + break;
>> + default:
>> DEBUG(SSSDBG_MINOR_FAILURE,
>> - ("Could not determine if attribute [%s] was ranged\n",
>> - str));
>> + ("Could not determine if attribute [%s] was ranged\n", str));
>> goto done;
>> }
>>
>> -
>> if (map) {
>
>I would prefer if we could skip checking the schema here and just shortcut
>to next iteration. Maybe use another variable to store the return value
>of sdap_parse_range() to be defensive in case there was another
>assignment to "ret" between sdap_parse_range() is called and ECANCELED
>checked?
I did not found any simple and readable solution without nested "if statments"
or with complicated if conditions with additional booolean variable.
>
>But looking at the 200+ lines function, I'm OK with this approach as long
>as we have a refactoring ticket for sdap_parse_entry.
>
>> for (a = 1; a < attrs_num; a++) {
>> /* check if this attr is valid with the chosen schema */
>> @@ -230,6 +233,11 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
>> store = true;
>> }
>>
>> + if (disable_range_retrieval && ret == ECANCELED) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^
I removed this part, because ECANCELED is returned only if
disable_range_retrieval is true;
>> + ret = EOK;
>> + store = false;
>> + }
>> +
>> if (store) {
>> vals = ldap_get_values_len(sh->ldap, sm->msg, str);
>> if (!vals) {
>
>> @@ -84,6 +85,17 @@ errno_t sdap_parse_range(TALLOC_CTX *mem_ctx,
>> ("[%s] contains sub-attribute other than a range, returning whole\n",
>> attr_desc));
>> goto done;
>> + } else if (disable_range_retrieval) {
>> + /* This is range sub-attribute, but we want to ignore it.
>> + * thing in case it's dealt with elsewhere.
>
>This second sentence in the comment doesn't sound like English to me :-)
>I think we can simply remove it.
>
Removed. (copy&paste problem)
>> + */
>> + *base_attr = talloc_strdup(mem_ctx, attr_desc);
>> + if (!*base_attr) {
>> + ret = ENOMEM;
>> + } else {
>> + ret = ECANCELED;
>> + }
>> + goto done;
>> }
>>
>> /* Get the end of the range */
Updated patch attached.
LS
-------------- next part --------------
>From 06bf30066bfbcffbff1e6d4bd984a426f6980ba8 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Tue, 14 May 2013 18:00:10 +0200
Subject: [PATCH] Adding option to disable retrieving large AD groups.
This commit adds new option ldap_disable_range_retrieval with default value
FALSE. If this option is enabled, large groups(>1500) will not be retrieved and
behaviour will be similar like was before commit ae8d047122c
"LDAP: Handle very large Active Directory groups"
https://fedorahosted.org/sssd/ticket/1823
---
src/config/SSSDConfig/__init__.py.in | 1 +
src/config/etc/sssd.api.d/sssd-ldap.conf | 1 +
src/man/sssd-ldap.5.xml | 21 +++++++++++++++++++++
src/providers/ad/ad_opts.h | 1 +
src/providers/ipa/ipa_opts.h | 1 +
src/providers/ldap/ldap_opts.h | 1 +
src/providers/ldap/sdap.c | 26 ++++++++++++++++++--------
src/providers/ldap/sdap.h | 4 +++-
src/providers/ldap/sdap_async.c | 7 +++++--
src/providers/ldap/sdap_range.c | 13 ++++++++++++-
src/providers/ldap/sdap_range.h | 3 ++-
11 files changed, 66 insertions(+), 13 deletions(-)
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index bc7bb0a781a64b264d350a7766cf45de869273cf..8e1142f2a8ca7388cf4f867962217478700050da 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -221,6 +221,7 @@ option_strings = {
'ldap_connection_expiration_timeout' : _('How long to retain a connection to the LDAP server before disconnecting'),
'ldap_disable_paging' : _('Disable the LDAP paging control'),
+ 'ldap_disable_range_retrieval' : _('Disable Active Directory range retrieval'),
# [provider/ldap/id]
'ldap_search_timeout' : _('Length of time to wait for a search request'),
diff --git a/src/config/etc/sssd.api.d/sssd-ldap.conf b/src/config/etc/sssd.api.d/sssd-ldap.conf
index 40e2aa09d0b4e94976bc18f04f9ca2d0006d444e..14e979da3413fe575e83b9a508c6cd8029d2bfba 100644
--- a/src/config/etc/sssd.api.d/sssd-ldap.conf
+++ b/src/config/etc/sssd.api.d/sssd-ldap.conf
@@ -35,6 +35,7 @@ ldap_sasl_canonicalize = bool, None, false
ldap_sasl_minssf = int, None, false
ldap_connection_expire_timeout = int, None, false
ldap_disable_paging = bool, None, false
+ldap_disable_range_retrieval = bool, 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 799213300857f182b092287741a40665a0ae62d5..37df5ec1b2e0b150b407f5e3e39db21860055580 100644
--- a/src/man/sssd-ldap.5.xml
+++ b/src/man/sssd-ldap.5.xml
@@ -1201,6 +1201,27 @@
</varlistentry>
<varlistentry>
+ <term>ldap_disable_range_retrieval (boolean)</term>
+ <listitem>
+ <para>
+ Disable Active Directory range retrieval.
+ </para>
+ <para>
+ Active Directory limits the number of members to be
+ retrieved in a single lookup using the MaxValRange
+ policy (which defaults to 1500 members). If a group
+ contains more members, the reply would include an
+ AD-specific range extension. This option disables
+ parsing of the range extension, therefore large
+ groups will appear as having no members.
+ </para>
+ <para>
+ Default: False
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term>ldap_sasl_minssf (integer)</term>
<listitem>
<para>
diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h
index 32bbe3db2f4048056c7e96619eaf53ce22bf52f8..18005f7cd3645a3bd0788d15c99a7f16a8398b61 100644
--- a/src/providers/ad/ad_opts.h
+++ b/src/providers/ad/ad_opts.h
@@ -122,6 +122,7 @@ struct dp_option ad_def_ldap_opts[] = {
{ "ldap_groups_use_matching_rule_in_chain", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
{ "ldap_initgroups_use_matching_rule_in_chain", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
{ "ldap_rfc2307_fallback_to_local_users", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
+ { "ldap_disable_range_retrieval", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
DP_OPTION_TERMINATOR
};
diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h
index de9592b85c50ced32b7ab1c19c5f59eef89d6354..57b17e5a1bdcc3ab1ca3a69a8a070e58b0a9eb94 100644
--- a/src/providers/ipa/ipa_opts.h
+++ b/src/providers/ipa/ipa_opts.h
@@ -146,6 +146,7 @@ struct dp_option ipa_def_ldap_opts[] = {
{ "ldap_groups_use_matching_rule_in_chain", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
{ "ldap_initgroups_use_matching_rule_in_chain", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
{ "ldap_rfc2307_fallback_to_local_users", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
+ { "ldap_disable_range_retrieval", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
DP_OPTION_TERMINATOR
};
diff --git a/src/providers/ldap/ldap_opts.h b/src/providers/ldap/ldap_opts.h
index 2ed89f977839e4a7e3ea2c51f04e8db76a883fd9..807716c18244ae70759d9b2eff7e40fd3c634fb2 100644
--- a/src/providers/ldap/ldap_opts.h
+++ b/src/providers/ldap/ldap_opts.h
@@ -113,6 +113,7 @@ struct dp_option default_basic_opts[] = {
{ "ldap_groups_use_matching_rule_in_chain", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
{ "ldap_initgroups_use_matching_rule_in_chain", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
{ "ldap_rfc2307_fallback_to_local_users", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
+ { "ldap_disable_range_retrieval", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
DP_OPTION_TERMINATOR
};
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
index 3a820f62ffeed946cbdaaa599bf3f057f616e116..daa081ce79920f739b62b78d9a4cc9481b604bd3 100644
--- a/src/providers/ldap/sdap.c
+++ b/src/providers/ldap/sdap.c
@@ -95,7 +95,8 @@ int sdap_get_map(TALLOC_CTX *memctx,
int sdap_parse_entry(TALLOC_CTX *memctx,
struct sdap_handle *sh, struct sdap_msg *sm,
struct sdap_attr_map *map, int attrs_num,
- struct sysdb_attrs **_attrs, char **_dn)
+ struct sysdb_attrs **_attrs, char **_dn,
+ bool disable_range_retrieval)
{
struct sysdb_attrs *attrs;
BerElement *ber = NULL;
@@ -190,23 +191,27 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
while (str) {
base64 = false;
- ret = sdap_parse_range(tmp_ctx, str, &base_attr, &range_offset);
- if (ret == EAGAIN) {
+ ret = sdap_parse_range(tmp_ctx, str, &base_attr, &range_offset,
+ disable_range_retrieval);
+ switch(ret) {
+ case EAGAIN:
/* This attribute contained range values and needs more to
* be retrieved
*/
/* TODO: return the set of attributes that need additional retrieval
* For now, we'll continue below and treat it as regular values.
*/
-
- } else if (ret != EOK) {
+ /* FALLTHROUGH */
+ case ECANCELED:
+ /* FALLTHROUGH */
+ case EOK:
+ break;
+ default:
DEBUG(SSSDBG_MINOR_FAILURE,
- ("Could not determine if attribute [%s] was ranged\n",
- str));
+ ("Could not determine if attribute [%s] was ranged\n", str));
goto done;
}
-
if (map) {
for (a = 1; a < attrs_num; a++) {
/* check if this attr is valid with the chosen schema */
@@ -230,6 +235,11 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
store = true;
}
+ if (ret == ECANCELED) {
+ ret = EOK;
+ store = false;
+ }
+
if (store) {
vals = ldap_get_values_len(sh->ldap, sm->msg, str);
if (!vals) {
diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
index 4c9023fe8b4f52640e7404fb1334b107f9678a93..162250fff76295807cce0779bebdf88577ed755c 100644
--- a/src/providers/ldap/sdap.h
+++ b/src/providers/ldap/sdap.h
@@ -217,6 +217,7 @@ enum sdap_basic_opt {
SDAP_AD_MATCHING_RULE_GROUPS,
SDAP_AD_MATCHING_RULE_INITGROUPS,
SDAP_RFC2307_FALLBACK_TO_LOCAL_USERS,
+ SDAP_DISABLE_RANGE_RETRIEVAL,
SDAP_OPTS_BASIC /* opts counter */
};
@@ -433,7 +434,8 @@ int sdap_get_map(TALLOC_CTX *memctx,
int sdap_parse_entry(TALLOC_CTX *memctx,
struct sdap_handle *sh, struct sdap_msg *sm,
struct sdap_attr_map *map, int attrs_num,
- struct sysdb_attrs **_attrs, char **_dn);
+ struct sysdb_attrs **_attrs, char **_dn,
+ bool disable_range_retrieval);
errno_t sdap_parse_deref(TALLOC_CTX *mem_ctx,
struct sdap_attr_map_info *minfo,
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index afa2904f478757fc15b5a54a411656bc7caa0bf0..cae70f07bda0cc831ffe8b0e9cb743acabda0ab7 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1469,6 +1469,7 @@ struct sdap_get_generic_state {
int map_num_attrs;
struct sdap_reply sreply;
+ bool disable_range_rtrvl;
};
static void sdap_get_generic_done(struct tevent_req *subreq);
@@ -1498,6 +1499,8 @@ struct tevent_req *sdap_get_generic_send(TALLOC_CTX *memctx,
state->map = map;
state->map_num_attrs = map_num_attrs;
+ state->disable_range_rtrvl = dp_opt_get_bool(opts->basic,
+ SDAP_DISABLE_RANGE_RETRIEVAL);
subreq = sdap_get_generic_ext_send(state, ev, opts, sh, search_base,
scope, filter, attrs, false, NULL,
@@ -1523,7 +1526,7 @@ static errno_t sdap_get_generic_parse_entry(struct sdap_handle *sh,
ret = sdap_parse_entry(state, sh, msg,
state->map, state->map_num_attrs,
- &attrs, NULL);
+ &attrs, NULL, state->disable_range_rtrvl);
if (ret != EOK) {
DEBUG(3, ("sdap_parse_entry failed [%d]: %s\n", ret, strerror(ret)));
return ret;
@@ -1986,7 +1989,7 @@ static errno_t sdap_asq_search_parse_entry(struct sdap_handle *sh,
ret = sdap_parse_entry(res[mi], sh, msg,
map, num_attrs,
- &res[mi]->attrs, NULL);
+ &res[mi]->attrs, NULL, false);
if (ret != EOK) {
DEBUG(3, ("sdap_parse_entry failed [%d]: %s\n", ret, strerror(ret)));
goto done;
diff --git a/src/providers/ldap/sdap_range.c b/src/providers/ldap/sdap_range.c
index a26443c8244bc58e609b2d9c6b4a2ded71193725..c4bf435393cc6d26b37b1d2719656d5daca71189 100644
--- a/src/providers/ldap/sdap_range.c
+++ b/src/providers/ldap/sdap_range.c
@@ -29,7 +29,8 @@
errno_t sdap_parse_range(TALLOC_CTX *mem_ctx,
const char *attr_desc,
char **base_attr,
- uint32_t *range_offset)
+ uint32_t *range_offset,
+ bool disable_range_retrieval)
{
errno_t ret;
TALLOC_CTX *tmp_ctx;
@@ -84,6 +85,16 @@ errno_t sdap_parse_range(TALLOC_CTX *mem_ctx,
("[%s] contains sub-attribute other than a range, returning whole\n",
attr_desc));
goto done;
+ } else if (disable_range_retrieval) {
+ /* This is range sub-attribute, but we want to ignore it.
+ */
+ *base_attr = talloc_strdup(mem_ctx, attr_desc);
+ if (!*base_attr) {
+ ret = ENOMEM;
+ } else {
+ ret = ECANCELED;
+ }
+ goto done;
}
/* Get the end of the range */
diff --git a/src/providers/ldap/sdap_range.h b/src/providers/ldap/sdap_range.h
index 1dc3ba8f940994c69222c5d428d5249857e8bac2..f11b3be6059e542fcf68cbd30e0b552d5c6ac28c 100644
--- a/src/providers/ldap/sdap_range.h
+++ b/src/providers/ldap/sdap_range.h
@@ -28,6 +28,7 @@
errno_t sdap_parse_range(TALLOC_CTX *mem_ctx,
const char *attr_desc,
char **base_attr,
- uint32_t *range_offset);
+ uint32_t *range_offset,
+ bool disable_range_retrieval);
#endif /* SDAP_RANGE_H_ */
--
1.8.1.4
More information about the sssd-devel
mailing list