On Tue, Mar 22, 2011 at 02:09:27PM -0400, Stephen Gallagher wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 03/21/2011 04:24 PM, Sumit Bose wrote: > > On Thu, Mar 17, 2011 at 05:25:55PM -0400, Stephen Gallagher wrote: > > On 03/16/2011 02:07 PM, Simo Sorce wrote: > >>>> On Wed, 16 Mar 2011 13:28:20 -0400 > >>>> Stephen Gallagher <sgall...@redhat.com> wrote: > >>>> > >>>>> -----BEGIN PGP SIGNED MESSAGE----- > >>>>> Hash: SHA1 > >>>>> > >>>>> Fixes https://fedorahosted.org/sssd/ticket/818 > >>>>> > >>>>> Patch 0001: Create sysdb_get_rdn() function > >>>>> This function takes a DN formatted string and returns the RDN > >>>>> value from it. > >>>> > >>>> Nack, please return both the value and the attribute name > >>>> > >>>>> Patch 0002: Add sysdb_attrs_primary_name() > >>>>> This function will check a sysdb_attrs struct for the primary name > >>>>> of the entity it represents. If there are multiple entries, it > >>>>> will pick the one that matches the RDN. If none match, it will > >>>>> throw an error. > >>>> > >>>> Nack please the attribute used in the RDN matches before checking the > >>>> values. > >>>> > >>>>> Patch 0003: Handle multi-valued usernames correctly > >>>>> Users in ldap with multiple values for their username attribute > >>>>> will now be compared against the RDN of the entry to determine the > >>>>> "primary" username. We will save all of the alternate names to the > >>>>> ldb cache as well, so a lookup for any of them will return the > >>>>> values for the primary name. > >>>>> e.g. > >>>>> getent passwd altusername > >>>>> primaryuser:*:800014:800014:primaryuser:/home/primaryuser:/bin/sh > >>>> > >>>> Nack, turning SYSDB_NAME form a single valued attribute (as used > >>>> throughout the code) to a multivalued attribute is a very dangerous > >>>> proposition IMO. > >>>> > >>>> Although the current version of LDB always ends up returning values in > >>>> the same order, fill_pwent() and other functions seem to be working > >>>> only by accident now. > >>>> > >>>> I would rather use a different attribute name for aliases. > >>>> This assuming we really want to allow aliases, I am personally still > >>>> unconvinced it is a good idea. I would rather throw aliases away and > >>>> allow only the canonical name to be stored in our cache. > >>>> > >>>>> Patch 0004: RFC2307: Handle multi-valued group names correctly > >>>>> Groups in ldap with multiple values for their groupname attribute > >>>>> will now be compared against the RDN of the entry to determine the > >>>>> "primary" group name. We will save all of the alternate names to the > >>>>> ldb cache as well, so a lookup for any of them will return the > >>>>> values for the primary name. > >>>>> e.g. > >>>>> getent group altgroup > >>>>> primarygroup:*:800014:member1,member2 > >>>>> > >>>>> Patch 0005: RFC2307bis: Handle multi-valued group names correctly > >>>>> Groups in ldap with multiple values for their groupname attribute > >>>>> will now be compared against the RDN of the entry to determine the > >>>>> "primary" group name. We will save all of the alternate names to the > >>>>> ldb cache as well, so a lookup for any of them will return the > >>>>> values for the primary name. > >>>>> e.g. > >>>>> getent group altgroup > >>>>> primarygroup:*:800014:member1,member2 > >>>>> > >>>>> > >>>>> I tested with RFC2307, RFC2307bis and FreeIPA v2 data. > >>>> > >>>> I think the fact you are getting back the "right" name is happening by > >>>> accident. > >>>> Change the code to explicitly save the aliases first and add the > >>>> canonical name last, and I think you will see the code to fail > >>>> consistently to give you back the canonical name. > >>>> > >>>> Simo. > >>>> > > > > > > I have made the following changes to the patches. > > > > * sysdb_get_rdn() will return both the attribute name and value of the RDN. > > > > * sysdb_attrs_primary_name() will now validate that the attribute name > > matches the ldap_[user|group]_name value. > > > > * We will no longer attempt to save the aliases at all. Only the > > primary name will be stored in our database. > > > > > > There is a side-effect here that I think is probably okay, but I thought > > I should mention it all the same. A direct request for a user or group > > alias will always cause a cache refresh of the primary group. This is > > because a check of our cache will return no values for this alias, so we > > need to go to the backend to validate it. > > > > This is not really an exploitable issue because we have the grouping > > optimization that ensures that we'd only have a single lookup for this > > alias going on at once (so the worst-case scenario is that a user could > > force us to keep running lookups after the previous one completed). > > > > A potential future solution to this would be to store the alternate > > names in the cache with a different attribute name as a sort of > > secondary negative cache. > > > >> Patch works good, but please put the reoccurring sequence of > >> sysdb_attrs_get_el()-sysdb_attrs_primary_name() calls into a call of its > >> onw. > > > > Thanks for the review. New patches attached makes the requested change. > I also switched to having the sysdb_attrs_get_el() use SYSDB_NAME > always, as that was the net effect anyway (since > user_map[LDAP_USER_NAME].sys_name always maps to this anyway).
Still works as expected. ACK bye, Sumit > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.11 (GNU/Linux) > Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ > > iEYEARECAAYFAk2I5dYACgkQeiVVYja6o6N8SACghAtbU3ItnVT/ZqutVyBKeMcl > v8UAn1N27FyURQvTtxMqWtCloiQ33LuT > =d2TA > -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
