[SSSD] [PATCHES] Support for multiple LDAP search bases

Jan Zeleny jzeleny at redhat.com
Tue Oct 18 22:38:47 UTC 2011


Jan Zelený <jzeleny at redhat.com> wrote:
> > > On Tue, 2011-10-04 at 08:04 -0400, Stephen Gallagher wrote:
> > > > On Tue, 2011-10-04 at 12:55 +0200, Jan Zelený wrote:
> > > > > > On Mon, 2011-10-03 at 14:51 -0400, Stephen Gallagher wrote:
> > > > > > > These patches add support for multiple search bases for users
> > > > > > > and groups in both direct-lookup and enumeration modes.
> > > > > > > 
> > > > > > > Addresses https://fedorahosted.org/sssd/ticket/868
> > > > > > > 
> > > > > > > Some notes: There is no patch adding multiple search base
> > > > > > > support for group lookups in RFC2307bis because it's
> > > > > > > meaningless right now. Since the group memberships are direct
> > > > > > > DNs, we do all of our searches as base searches, ignoring
> > > > > > > group_search_base. There is a separate ticket,
> > > > > > > https://fedorahosted.org/sssd/ticket/960 that will need to
> > > > > > > address this, taking advantage of the multiple search base
> > > > > > > features.
> > > > > > > 
> > > > > > > Also, while working through this, I opened
> > > > > > > https://fedorahosted.org/sssd/ticket/1006 as I noticed that we
> > > > > > > are using far too many separate transactions while processing
> > > > > > > RFC2307bis (which is likely the cause of the extreme slowdown
> > > > > > > that some of our users were reporting with AD).
> > > > > > > 
> > > > > > > These patches don't really work separately, but they've been
> > > > > > > broken up to make review much easier.
> > > > > > > 
> > > > > > > 
> > > > > > > Patch 0001: Remove some unused options in a struct
> > > > > > > 
> > > > > > > Patch 0002: Fix size return for split_on_separator()
> > > > > > > It was returning the size of the array, rather than the number
> > > > > > > of elements. (The array was NULL-terminated). This argument
> > > > > > > was only used in one place that was actually working around
> > > > > > > this odd return value.
> > > > > > > 
> > > > > > > Patch 0003: Make sdap_get_id_specific_filter() more strict
> > > > > > > Just makes it take const char * instead of char *.
> > > > > > > 
> > > > > > > Patch 0004: Add parser for multiple search bases
> > > > > > > As discussed on the list, this will the ldap_*_search_base
> > > > > > > options in the form of:
> > > > > > > search_base[?scope?[filter][?search_base?scope?[filter]]*]
> > > > > > > This is backwards-compatible (just use a search base)
> > > > > > > 
> > > > > > > Patch 0005: Add support for multiple search bases for users
> > > > > > > 
> > > > > > > Patch 0006: Add support for multiple search bases for netgroups
> > > > > > > 
> > > > > > > Patch 0007: Add support for multiple search bases for RFC2307
> > > > > > > groups
> > > > > > > 
> > > > > > > Patch 0008: Add support for multiple search bases for
> > > > > > > initgroups (user portion)
> > > > > > > 
> > > > > > > Patch 0009: Add support for multiple search bases for
> > > > > > > initgroups (RFC2307 group portion)
> > > > > > > 
> > > > > > > Patch 0010: Add support for multiple search bases for
> > > > > > > initgroups (RFC2307 group portion)
> > > > > > > This patch I'm not 100% sure of. It may need more processing.
> > > > > > > With RFC2307, it was safe to have duplicate groups in the list,
> > > > > > > because only the group name is important (and we guarantee that
> > > > > > > the name list has only unique values before saving it). With
> > > > > > > RFC2307bis, I'm not sure if it's safe to add two groups that
> > > > > > > may have the same name. Comments welcome.
> > > > > > > 
> > > > > > > Patch 0011: Update manpages with multiple search base
> > > > > > > information
> > > > > > > 
> > > > > > > Patch 0012: Convert ldap_*_search_filter
> > > > > > > Instead of making this a global option for all user lookups,
> > > > > > > make it only used if the search base is passed without an
> > > > > > > explicit filter. The idea here is to deprecate the old
> > > > > > > separate ldap_user_search_filter and ldap_group_search_filter
> > > > > > > options in favor of the new representation (which is closer to
> > > > > > > the traditional nss_ldap representation).
> > > > > > > 
> > > > > > > Patch 0013: Add support for multiple search bases for user
> > > > > > > enumeration
> > > > > > > 
> > > > > > > Patch 0014: Add support for multiple search bases for group
> > > > > > > enumeration This changes our behavior slightly with regard to
> > > > > > > handling direct lookups that return more than one entry. In the
> > > > > > > old code, we had a bug that would cause SSSD to treat this as
> > > > > > > an enumeration (instead of a direct lookup). This was a bug
> > > > > > > and has been fixed incidentally as part of this modification.
> > > > > > 
> > > > > > For the record, because I forgot to mention it in my first email:
> > > > > > I tried to reproduce the issue jzeleny saw in the earlier thread
> > > > > > without success. Either I inadvertently fixed it with the
> > > > > > changes I made or it was caused by other changes in his
> > > > > > environment (possibly the memberOf work).
> > > > > > 
> > > > > > I tested it with exactly the same users and groups, using SSSD
> > > > > > speaking to a FreeIPA server (in both rfc2307bis mode and ipa
> > > > > > backend).
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > His original comment:
> > > > > > 
> > > > > > So NACK after all. Following test failed:
> > > > > > 
> > > > > > I tried following membership structure. I have 3 groups:
> > > > > > topgroup, middlegroup
> > > > > > and bottomgroup and 2 users:  user1 and user2
> > > > > > 
> > > > > > The relationship is following:
> > > > > > topgroup -> middlegroup, user1
> > > > > > middlegroup -> user2, bottomgroup
> > > > > > bottomgroup -> user1
> > > > > > 
> > > > > > 
> > > > > > getent group topgroup gives me result with no members, the same
> > > > > > test on master
> > > > > > returns the expected result.
> > > > > 
> > > > > Stephen,
> > > > > I still see the same error I reported. This time I had time to take
> > > > > a look into logs and I found out that with your patches, a lookup
> > > > > on IPA server is performed on the entire tree whereas the lookup
> > > > > without your patch is performed only on cn=accounts subtree. As a
> > > > > result, the lookup with your patches finds two group records (one
> > > > > in cn=accounts and one in cn=compat) and refuses to continue. I
> > > > > have no ldap search base configured manually, it is taken from IPA
> > > > > server.
> > > > 
> > > > Yeah, this is a bug in FreeIPA. If we use autodetection in FreeIPA
> > > > right now, it returns the common search base for both compat and
> > > > standard. It's not an SSSD issue. I've opened
> > > > https://fedorahosted.org/freeipa/ticket/1919 upstream to deal with
> > > > this. In the meantime, please just set the search base appropriately
> > > > (which is done automatically by ipa-client-install).
> > > 
> > > Well, I have a bit of egg on my face. Jan was completely right, and I
> > > had forgotten to ensure that the IPA provider was picking up the
> > > correct search bases. I've now updated patch 0004 to correct this.
> > 
> > Steve,
> > I finally finished the review:
> > 
> > #1-9: ACK
> > 
> > #11:
> > - mention what other values are acceptable for scope (you have only
> > subtree in the example)
> > - maybe mention a reference to a document about LDAP filters? Or a brief
> > description how the filter can look like. This is not necessary, just a
> > thought to avoid some user questions in the future.
> > 
> > 
> > 
> > Consider following patches ACKed, I leave suggested changes at your
> > discretion:
> > 
> > #10:
> > sdap_initgr_rfc2307bis_send
> > - how about making one fail/done label and some gotos?
> > sdap_initgr_rfc2307bis_process
> > - I'd rather use memcpy than the cycle
> > 
> > #13:
> > sdap_get_users_process
> > - I'd rather use memcpy than the cycle
> > 
> > #14:
> > sdap_get_groups_process
> > - I'd rather use memcpy than the cycle
> 
> Implementing multiple search bases into IPA provider, I noticed today
> something else in patch #0006.
> 
> in function sdap_get_netgroups_process() you call talloc_zfree() on two
> variables. I believe both of those are redundant:
> - if count is zero then state->netgroups will be NULL
> - state->filter is either freed in repeated call of
> sdap_get_netgroups_next_base() or it will be freed with the entire context
> due to an error.
> 
> I noticed similar code also being in patches #0005, #0007, #0008, however I
> didn't inspect it very deeply there, so I don't know for sure that it is
> redundant there as well.

One more nack,
I noticed that search bases aren't parsed in IPA provider when they are 
explicitly set in config file.


Jan



More information about the sssd-devel mailing list