On 11/10/2016 02:13 PM, Lukas Slebodnik wrote:
> On (10/11/16 13:57), Michal Židek wrote:
>> On 11/10/2016 12:29 PM, Jakub Hrozek wrote:
>>> On Thu, Nov 10, 2016 at 10:49:55AM +0100, Michal Židek wrote:
>>>> Hi,
>>>>
>>>> this is continuation of discussion about pull
>>>> request 51 and associated tickets.
>>>>
>>>> For context, see:
>>>>
https://github.com/SSSD/sssd/pull/59
>>>>
https://fedorahosted.org/sssd/ticket/3159
>>>>
https://fedorahosted.org/sssd/ticket/3116
>>>>
>>>> The FreeIPA UQE guys added upstream test for this issue
>>>> because we do not have upstream CI tests in SSSD with
>>>> IPA provider yet and this bug is not present in the
>>>> plain LDAP.
>>>>
>>>> We use hash tables to store members of netgroups while
>>>> processing netgroups (and creating the netgroup triples).
>>>> The netgroup names are lowercased before they are stored
>>>> in the hash table. The reason for this normalization is
>>>> unknown to me.
>>>
>>> I also don't know the reason behind this normalization. There is a
>>> comment above the code that lowercases the DN that says:
>>> 646 /* Transform the DN to lower case.
>>> 647 * this is important, as the member/memberof attributes
>>> 648 * have the value also in lower-case
>>> 649 */
>>>
>>> But I really have no idea what this means. We're using
>>> SYSDB_NETGROUP_MEMBER,
>>> but we're storing names in that attribute and the IPA code lowercases
>>> original DNs, so I'm quite confused about it.
>>>
>>>> FreeIPA only creates lowercased netgroup
>>>> names, so lowercasing only affects the attribute name
>>>> (that is stored as prefix to the netgroup name in the hash table,
>>>> and maybe it can happen that the attribute name can be
>>>> stored in different cases at some point, which would
>>>> explain why we lower case it, however I was not able
>>>> to confirm if this is the case).
>>>
>>> I really think this is about the original DN values. This is what we
>>> enter:
>>> (gdb) p key.str
>>>
>>>
"ipauniqueid=026e49e8-969d-11e6-a2f4-525400f6cf82,cn=ng,cn=alt,dc=ipa,dc=test"
>>>
>>>
>>>>
>>>> When we read the hash table, we do not lowercase the keys,
>>>> so the nested netgroups are not found and this is the
>>>> reason why the bug appears. The patch in PR 51 lower cases
>>>> the keys before reading the hash table and the bug does not
>>>> appear after that. Lukas thinks however that this is not
>>>> good approach, because there should be no need for the lower
>>>> casing in the first place.
>>>>
>>>> Patch that removes the lower casing before adding the keys
>>>> to htable should also fix the issue. I did not send the patch
>>>> with this approach, because I was not sure why the lowercasing
>>>> happens in the first place and I know that lowercasing is not
>>>> harmful for the IPA netgroups, so I find it safer to use
>>>> the approach in the PR 51 especially while we do not have good
>>>> code coverage for IPA provider, however as I mentioned in the
>>>> PR 51, I am looking for opinions on this.
>>>
>>> I'm fine with removing the lowercasing if this moves fixing the issue
>>> forward.
>>
>> I just quickly tried it without the lowercasing and it does work
>> for me.
>>
> awesome
>
> LS
The patch for master is in attachment.
Michal