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..
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.
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.
[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.
[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.
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.
[PATCH 5/7] LDAP: Remove unused output parameter _dn from sdap_parse_entry Does what the commit message suggests.
[PATCH 6/7] SDAP: Remove unused function sdap_get_msg_dn This function was unused since 2009
[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..
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 }
On Tue, Jul 08, 2014 at 04:23:37PM +0200, Michal Židek wrote:
[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==============
Done.
[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 }
Thanks, nice catch.
On 07/08/2014 07:31 PM, Jakub Hrozek wrote:
On Tue, Jul 08, 2014 at 04:23:37PM +0200, Michal Židek wrote:
[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==============
Done.
Ack.
[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 }
Thanks, nice catch.
Ack.
On Tue, Jul 08, 2014 at 07:41:41PM +0200, Michal Židek wrote:
On 07/08/2014 07:31 PM, Jakub Hrozek wrote:
On Tue, Jul 08, 2014 at 04:23:37PM +0200, Michal Židek wrote:
[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==============
Done.
Ack.
[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 }
Thanks, nice catch.
Ack.
Pushed all to master: 5ab0dedd52d248b2cf2eb028338919bc0046e2a9 88eac3adf8424b65195e725ff724c79d38500e1d 34de8a00f5b480ef3f46d2516e072e4acf1ebf87 004b3589f85808bcfcb0019aa92e47d3ce4017c9 eed2073f6f7bed7df0327b9fc0f2d410975d5332 e592d5f157be869151983bd1b46d6f4f7a29daaf b1559c6498e791aa870f6e01948d1ad0f1af32e7
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Michal Židek