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

Pavel Reichl preichl at redhat.com
Fri May 30 12:52:50 UTC 2014


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-MAN-Detailed-ldap_group_nesting_level-option.patch
Type: text/x-patch
Size: 1875 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140530/e44f4af2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-SDAP-Make-nesting_level-0-to-ignore-nested-groups.patch
Type: text/x-patch
Size: 4544 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140530/e44f4af2/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-SDAP-Add-option-to-disable-use-of-Token-Groups.patch
Type: text/x-patch
Size: 9137 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140530/e44f4af2/attachment-0002.bin>


More information about the sssd-devel mailing list