-----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 <sgallagh(a)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).
-----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-----