[SSSD] [patches] SDAP: "ldap_group_nesting_level = 0" does not appear to work

Jakub Hrozek jhrozek at redhat.com
Tue May 27 19:37:03 UTC 2014


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 at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> 

> From 328def1e84e5d5baa81c8beca3484a659c353a6a Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at 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 at 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 at 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.



More information about the sssd-devel mailing list