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. 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).
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.
Thanks,
Michal
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.
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.
Michal
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
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
On 11/10/2016 03:18 PM, Michal Židek wrote:
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
Lukas wanted a new PR, so here it is: https://github.com/SSSD/sssd/pull/78
Michal
sssd-devel@lists.fedorahosted.org