[SSSD] [PATCH] LDAP: Try all attributes when saving an entry

Michal Židek mzidek at redhat.com
Tue Jul 8 14:23:37 UTC 2014


On 07/07/2014 10:12 PM, Jakub Hrozek wrote:
> On Wed, Oct 23, 2013 at 06:58:16PM +0200, Jakub Hrozek wrote:
>> On Wed, Oct 23, 2013 at 01:21:48PM +0200, Jakub Hrozek wrote:
>>> Hi,
>>>
>>> this bug was reported on #sssd by a user. He was running some flavor of
>>> IBM Tivoli where the entries only had an "ID", not separate "UID" and
>>> "GID". But due to a bug in sssd he couldn't use the same value for both,
>>> this configuration:
>>>
>>> ldap_user_uid_number = idAttribute
>>> ldap_user_gid_number = idAttribute
>>>
>>> only saved the ID into UID and left GID empty. It appears we have a long
>>> standing bug in sdap_parse where we only consider first match. If this
>>> patch is accepted, I would also like to refactor sdap_parse in master
>>> because currently it is a 250-lines long function with multiple
>>> branches..
>>
>> self-nack, this patch breaks parsing of rootDSE. I will prepare a new
>> version.
>
> After a slight delay I'm attaching a revised patchset. I know we're getting
> close to the release of 1.12.0 and this patch changes a critical function,
> so I also prepared a unit test. I think only patches #1 to #3 are important
> for 1.12.0, so I'm fine with postponing the other patches.
>
> [PATCH 1/7] PROVIDERS: Add ldap_common.h to opts.h of each provider
> Trivial header file amendment, please see the commit message for details.

Ack.

>
> [PATCH 2/7] TESTS: Add a unit test for the sdap.c module
> As said earlier, a unit test was added. I haven't covered only the range
> extensions.
>

Ack. I mostly reviewed this by reversing the tests and checking
if they are going to fail or not. They failed every time they
had to. Btw. it is good to see new unit test :)

> [PATCH 3/7] LDAP: Try all attributes when saving an entry
> https://fedorahosted.org/sssd/ticket/2184
>
> I tested by setting the 'telephoneNumber' attribute to a numeric value
> and ensuring that SSSD was able to use the value for both UID and GID.
>

Ack.

> And now the less important patches, mostly just changes I did while I
> was changing that area of code:
> [PATCH 4/7] SDAP: Fix DEBUG message priorities in sdap_parse_entry
> Amends log levels of DEBUG messages.
>

Ack.

> [PATCH 5/7] LDAP: Remove unused output parameter _dn from sdap_parse_entry
> Does what the commit message suggests.
>

Ack.

> [PATCH 6/7] SDAP: Remove unused function sdap_get_msg_dn
> This function was unused since 2009
>

Could you also remove the comment.
  /* =Get-DN-from-message==============

> [PATCH 7/7] SDAP: Free bervals on failure in sdap_parse_entry
> I wasn't able to test this patch, so review would only be with visual
> inspection, I think..
>

I think the ldap_value_free_len call should be here as well
to free the memory allocated for the one NULL pointer:
  447             } else {
  448                 if (!vals[0]) {
  449                     DEBUG(SSSDBG_CRIT_FAILURE,
  450                           "Missing value after ldap_get_values() 
??\n");
  451                     ret = EINVAL;
  452                     goto done;
  453                 }



More information about the sssd-devel mailing list