-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 05/30/2014 08:52 AM, Pavel Reichl wrote:
Thank you Jakub for comments. I proposed new version of text to
man pages. I updated 2nd patch and generally speaking returned to
solution which I proposed in 1st or 2nd iteration of this patches.
But this time I traded off code beauty for performance, but I think
it's OK.
Regards,
PR
On Tue, 2014-05-27 at 21:37 +0200, Jakub Hrozek wrote:
> On Mon, May 26, 2014 at 07:28:37PM +0200, Pavel Reichl wrote:
>>>> Hello,
>>>>
>>>> I have updated patches and in 3rd patch I added option to
>>>> disable Token-Groups for AD provider and thus make
>>>> nesting_level = 0 work.
>>>>
>>>> 2nd patch may be simplified even more if kind reviewer (or
>>>> Lukas:-) ) thinks that call of is_group_dn should be
>>>> replaced by:
>>>>
>>>> "strstr(strdn, "cn=groups") != NULL"
>>>>
>>> Yes, please.
>>>
>>> For group with 100 users without any nesting group, you will
>>> save 700 allocations,
>>>
>>> strstr approach is much more lightweight :-)
>>>
>>> LS
>>
>> OK, whatever.
>>
>> Updated patches attached.
>>
>>
>>> _______________________________________________ sssd-devel
>>> mailing list sssd-devel(a)lists.fedorahosted.org
>>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
>
>> From 328def1e84e5d5baa81c8beca3484a659c353a6a Mon Sep 17
>> 00:00:00 2001 From: Pavel Reichl <preichl(a)redhat.com> Date:
>> Mon, 12 May 2014 22:45:00 +0000 Subject: [PATCH 1/3] MAN:
>> Detailed ldap_group_nesting_level option
>>
>> Resolves:
https://fedorahosted.org/sssd/ticket/2294 ---
>> src/man/sssd-ldap.5.xml | 12 ++++++++++++ 1 file changed, 12
>> insertions(+)
>>
>> diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml
>> index
>>
6426fe4fca5dc9bb9bc84fcbf633404144052d01..d7bb3475d38ca93044632673bb9c7ac6d83c02f6
>> 100644 --- a/src/man/sssd-ldap.5.xml +++
>> b/src/man/sssd-ldap.5.xml @@ -928,6 +928,18 @@ RFC2307 schema.
>> </para> <para> + Note: This option
>> specifies the minimal level of +
>> nesting that will be processed by the LDAP +
>> provider. However, nested groups beyond this limit +
>> <emphasis>may be</emphasis> returned.
>
> I think this needs further clarification, something like "may be
> returned if previous lookups already resolved the deeper nesting
> levels". As with any man page change, please ask some native
> English speaker for language check.
>
>> + </para> +
>> <para> + If ldap_group_nesting_level
>> is set to 0 and LDAP + provider is
>> used then no nested groups are processed +
>> at all. Same behavior may be achieved for AD +
>> provider if ldap_use_tokengroups is set to false.
>
> I think the last sentence is misleading, the way I read it,
> disabling tokengroups would always disable nesting, which is not
> the case, is it?
>
>> + </para> +
>> <para> Default: 2 </para> </listitem> -- 1.8.4.2
>>
>
>> From 484bda4041269e0e52211a1660ff5e5172cb208f Mon Sep 17
>> 00:00:00 2001 From: Pavel Reichl <preichl(a)redhat.com> Date:
>> Mon, 12 May 2014 15:00:26 +0000 Subject: [PATCH 2/3] SDAP: Make
>> nesting_level = 0 to ignore nested groups
>>
>> Make ldap_group_nesting_level = 0 to ignore group nesting
>> entirely.
>>
>> Resolves:
https://fedorahosted.org/sssd/ticket/2294 ---
>> src/providers/ldap/sdap_async_groups.c | 11 +++++++++++ 1 file
>> changed, 11 insertions(+)
>>
>> diff --git a/src/providers/ldap/sdap_async_groups.c
>> b/src/providers/ldap/sdap_async_groups.c index
>>
89a5afb91e8fe666f834720c6b97e74e266822a6..c3c4e72c0b60cd5e689b5d9b0c42612c1feefa8b
>> 100644 --- a/src/providers/ldap/sdap_async_groups.c +++
>> b/src/providers/ldap/sdap_async_groups.c @@ -1211,6 +1211,9 @@
>> sdap_process_group_members_2307bis(struct tevent_req *req, char
>> *strdn; int ret; int i; + int nesting_level; + +
>> nesting_level = dp_opt_get_int(state->opts->basic,
>> SDAP_NESTING_LEVEL);
>>
>> for (i=0; i < memberel->num_values; i++) { member_dn = (char
>> *)memberel->values[i].data; @@ -1220,7 +1223,15 @@
>> sdap_process_group_members_2307bis(struct tevent_req *req,
>> state->dom, member_dn, &strdn); + if (ret == EOK) { +
>> if (nesting_level == 0) { + /* Ignore group
>> members which are groups themselves. */ + if
>> (strstr(strdn, SYSDB_GROUPS_CONTAINER) != 0 ) {
>
> While I agree with Lukas that saving allocations for these
> insensive operations with potentially many loops should be done,
> could we check that the dn contains the domain group container
> (cn=Groups,cn=$domname I think) and ideally also that the DN ends
> with this string? Computing the group container and its lenght
> could be done just once per group, so it wouldn't be too
> expensive.
>
>> + continue; + } +
>> } + /* * User already cached in sysdb. Remember the sysdb DN
>> for later * use by sdap_save_groups() -- 1.8.4.2
>>
>
>> From 94722009db7fee6320af1cef5adfad9e0e3356e2 Mon Sep 17
>> 00:00:00 2001 From: Pavel Reichl <preichl(a)redhat.com> Date:
>> Wed, 21 May 2014 09:30:13 +0100 Subject: [PATCH 3/3] SDAP: Add
>> option to disable use of Token-Groups
>>
>> Disabling use of Token-Groups is mandatory if expansion of
>> nested groups is not desired (ldap_group_nesting_level = 0) for
>> AD provider.
>>
>> Resolves:
https://fedorahosted.org/sssd/ticket/2294 ---
>> src/config/SSSDConfig/__init__.py.in | 1 +
>> src/config/etc/sssd.api.d/sssd-ad.conf | 1 +
>> src/config/etc/sssd.api.d/sssd-ipa.conf | 1 +
>> src/config/etc/sssd.api.d/sssd-ldap.conf | 1 +
>> src/man/sssd-ldap.5.xml | 13 +++++++++++++
>> 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.h | 1 +
>> src/providers/ldap/sdap_async_initgroups.c | 7 +++++-- 10
>> files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/config/SSSDConfig/__init__.py.in
>> b/src/config/SSSDConfig/__init__.py.in index
>>
e221eba278518b07ead0c39a6ecd7527f86d9b5e..d9b186f73220363f429b51260e6b83bd00a9cdb6
>> 100644 --- a/src/config/SSSDConfig/__init__.py.in +++
>> b/src/config/SSSDConfig/__init__.py.in @@ -321,6 +321,7 @@
>> option_strings = {
>>
>> 'ldap_groups_use_matching_rule_in_chain' : _('Use
>> LDAP_MATCHING_RULE_IN_CHAIN for group lookups'),
>> 'ldap_initgroups_use_matching_rule_in_chain' : _('Use
>> LDAP_MATCHING_RULE_IN_CHAIN for initgroup lookups'), +
>> 'ldap_use_tokengroups' : _('Whether to use Token-Groups'),
>> 'ldap_min_id' : _('Set lower boundary for allowed IDs from the
>> LDAP server'), 'ldap_max_id' : _('Set upper boundary for
>> allowed IDs from the LDAP server'),
>>
>> diff --git a/src/config/etc/sssd.api.d/sssd-ad.conf
>> b/src/config/etc/sssd.api.d/sssd-ad.conf index
>>
b70e74c0ac17da95574756f54538fd4414f6f807..33d460e828607f3640b9d19572b1319d4c0cc001
>> 100644 --- a/src/config/etc/sssd.api.d/sssd-ad.conf +++
>> b/src/config/etc/sssd.api.d/sssd-ad.conf @@ -111,6 +111,7 @@
>> ldap_idmap_default_domain = str, None, false
>> ldap_idmap_default_domain_sid = str, None, false
>> ldap_groups_use_matching_rule_in_chain = bool, None, false
>> ldap_initgroups_use_matching_rule_in_chain = bool, None, false
>> +ldap_use_tokengroups = bool, None, false
>> ldap_rfc2307_fallback_to_local_users = bool, None, false
>>
>> [provider/ad/auth] diff --git
>> a/src/config/etc/sssd.api.d/sssd-ipa.conf
>> b/src/config/etc/sssd.api.d/sssd-ipa.conf index
>>
3a3f6a4cf9586b2a81844e38498ba86b504f1f52..11484e7d4bd9a9da15be38ef2a0fe3a9d931cb50
>> 100644 --- a/src/config/etc/sssd.api.d/sssd-ipa.conf +++
>> b/src/config/etc/sssd.api.d/sssd-ipa.conf @@ -129,6 +129,7 @@
>> ldap_idmap_default_domain = str, None, false
>> ldap_idmap_default_domain_sid = str, None, false
>> ldap_groups_use_matching_rule_in_chain = bool, None, false
>> ldap_initgroups_use_matching_rule_in_chain = bool, None, false
>> +ldap_use_tokengroups = bool, None, false
>> ldap_rfc2307_fallback_to_local_users = bool, None, false
>> ipa_server_mode = bool, None, false
>>
>> diff --git a/src/config/etc/sssd.api.d/sssd-ldap.conf
>> b/src/config/etc/sssd.api.d/sssd-ldap.conf index
>>
0057c080f84d4b5f75107903b03a3cc91f03cc83..fa9cdd69846f142cbeef9b0192360816c9b554fc
>> 100644 --- a/src/config/etc/sssd.api.d/sssd-ldap.conf +++
>> b/src/config/etc/sssd.api.d/sssd-ldap.conf @@ -118,6 +118,7 @@
>> ldap_idmap_default_domain = str, None, false
>> ldap_idmap_default_domain_sid = str, None, false
>> ldap_groups_use_matching_rule_in_chain = bool, None, false
>> ldap_initgroups_use_matching_rule_in_chain = bool, None, false
>> +ldap_use_tokengroups = bool, None, false
>> ldap_rfc2307_fallback_to_local_users = bool, None, false
>> ldap_min_id = int, None, false ldap_max_id = int, None, false
>> diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml
>> index
>>
d7bb3475d38ca93044632673bb9c7ac6d83c02f6..f33e138009f40ed56860d3c5f7ab5fe4772d5746
>> 100644 --- a/src/man/sssd-ldap.5.xml +++
>> b/src/man/sssd-ldap.5.xml @@ -1005,6 +1005,19 @@
>> </varlistentry>
>>
>> <varlistentry> +
>> <term>ldap_use_tokengroups</term> +
>> <listitem> + <para> +
>> This options enables of disables use of Token-Groups +
>> attribute. It is used by AD provider only.
>
> Are you sure? According to the code, the TG lookup is used also
> if you set ldap_schema to ad..
>
>> + </para> +
>> <para> + Default: True +
>> </para> + </listitem> +
>> </varlistentry> + + <varlistentry>
>> <term>ldap_netgroup_object_class (string)</term> <listitem>
>> <para> diff --git a/src/providers/ad/ad_opts.h
>> b/src/providers/ad/ad_opts.h
>
> The rest of the patches looks good to me but I haven't ran any
> tests yet.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird -
http://www.enigmail.net/
iEYEARECAAYFAlOIieYACgkQeiVVYja6o6NgRgCcCWoaLTE6sCTOcV8nTdBgTJpU
bysAn1Zx1NfbAfBNxYcUEJ9zmrxArdd3
=vvGo
-----END PGP SIGNATURE-----