These patches add support for multiple search bases for users and groups in both direct-lookup and enumeration modes.
Addresses https://fedorahosted.org/sssd/ticket/868
Some notes: There is no patch adding multiple search base support for group lookups in RFC2307bis because it's meaningless right now. Since the group memberships are direct DNs, we do all of our searches as base searches, ignoring group_search_base. There is a separate ticket, https://fedorahosted.org/sssd/ticket/960 that will need to address this, taking advantage of the multiple search base features.
Also, while working through this, I opened https://fedorahosted.org/sssd/ticket/1006 as I noticed that we are using far too many separate transactions while processing RFC2307bis (which is likely the cause of the extreme slowdown that some of our users were reporting with AD).
These patches don't really work separately, but they've been broken up to make review much easier.
Patch 0001: Remove some unused options in a struct
Patch 0002: Fix size return for split_on_separator() It was returning the size of the array, rather than the number of elements. (The array was NULL-terminated). This argument was only used in one place that was actually working around this odd return value.
Patch 0003: Make sdap_get_id_specific_filter() more strict Just makes it take const char * instead of char *.
Patch 0004: Add parser for multiple search bases As discussed on the list, this will the ldap_*_search_base options in the form of: search_base[?scope?[filter][?search_base?scope?[filter]]*] This is backwards-compatible (just use a search base)
Patch 0005: Add support for multiple search bases for users
Patch 0006: Add support for multiple search bases for netgroups
Patch 0007: Add support for multiple search bases for RFC2307 groups
Patch 0008: Add support for multiple search bases for initgroups (user portion)
Patch 0009: Add support for multiple search bases for initgroups (RFC2307 group portion)
Patch 0010: Add support for multiple search bases for initgroups (RFC2307 group portion) This patch I'm not 100% sure of. It may need more processing. With RFC2307, it was safe to have duplicate groups in the list, because only the group name is important (and we guarantee that the name list has only unique values before saving it). With RFC2307bis, I'm not sure if it's safe to add two groups that may have the same name. Comments welcome.
Patch 0011: Update manpages with multiple search base information
Patch 0012: Convert ldap_*_search_filter Instead of making this a global option for all user lookups, make it only used if the search base is passed without an explicit filter. The idea here is to deprecate the old separate ldap_user_search_filter and ldap_group_search_filter options in favor of the new representation (which is closer to the traditional nss_ldap representation).
Patch 0013: Add support for multiple search bases for user enumeration
Patch 0014: Add support for multiple search bases for group enumeration This changes our behavior slightly with regard to handling direct lookups that return more than one entry. In the old code, we had a bug that would cause SSSD to treat this as an enumeration (instead of a direct lookup). This was a bug and has been fixed incidentally as part of this modification.
On Mon, 2011-10-03 at 14:51 -0400, Stephen Gallagher wrote:
These patches add support for multiple search bases for users and groups in both direct-lookup and enumeration modes.
Addresses https://fedorahosted.org/sssd/ticket/868
Some notes: There is no patch adding multiple search base support for group lookups in RFC2307bis because it's meaningless right now. Since the group memberships are direct DNs, we do all of our searches as base searches, ignoring group_search_base. There is a separate ticket, https://fedorahosted.org/sssd/ticket/960 that will need to address this, taking advantage of the multiple search base features.
Also, while working through this, I opened https://fedorahosted.org/sssd/ticket/1006 as I noticed that we are using far too many separate transactions while processing RFC2307bis (which is likely the cause of the extreme slowdown that some of our users were reporting with AD).
These patches don't really work separately, but they've been broken up to make review much easier.
Patch 0001: Remove some unused options in a struct
Patch 0002: Fix size return for split_on_separator() It was returning the size of the array, rather than the number of elements. (The array was NULL-terminated). This argument was only used in one place that was actually working around this odd return value.
Patch 0003: Make sdap_get_id_specific_filter() more strict Just makes it take const char * instead of char *.
Patch 0004: Add parser for multiple search bases As discussed on the list, this will the ldap_*_search_base options in the form of: search_base[?scope?[filter][?search_base?scope?[filter]]*] This is backwards-compatible (just use a search base)
Patch 0005: Add support for multiple search bases for users
Patch 0006: Add support for multiple search bases for netgroups
Patch 0007: Add support for multiple search bases for RFC2307 groups
Patch 0008: Add support for multiple search bases for initgroups (user portion)
Patch 0009: Add support for multiple search bases for initgroups (RFC2307 group portion)
Patch 0010: Add support for multiple search bases for initgroups (RFC2307 group portion) This patch I'm not 100% sure of. It may need more processing. With RFC2307, it was safe to have duplicate groups in the list, because only the group name is important (and we guarantee that the name list has only unique values before saving it). With RFC2307bis, I'm not sure if it's safe to add two groups that may have the same name. Comments welcome.
Patch 0011: Update manpages with multiple search base information
Patch 0012: Convert ldap_*_search_filter Instead of making this a global option for all user lookups, make it only used if the search base is passed without an explicit filter. The idea here is to deprecate the old separate ldap_user_search_filter and ldap_group_search_filter options in favor of the new representation (which is closer to the traditional nss_ldap representation).
Patch 0013: Add support for multiple search bases for user enumeration
Patch 0014: Add support for multiple search bases for group enumeration This changes our behavior slightly with regard to handling direct lookups that return more than one entry. In the old code, we had a bug that would cause SSSD to treat this as an enumeration (instead of a direct lookup). This was a bug and has been fixed incidentally as part of this modification.
For the record, because I forgot to mention it in my first email: I tried to reproduce the issue jzeleny saw in the earlier thread without success. Either I inadvertently fixed it with the changes I made or it was caused by other changes in his environment (possibly the memberOf work).
I tested it with exactly the same users and groups, using SSSD speaking to a FreeIPA server (in both rfc2307bis mode and ipa backend).
His original comment:
So NACK after all. Following test failed:
I tried following membership structure. I have 3 groups: topgroup, middlegroup and bottomgroup and 2 users: user1 and user2
The relationship is following: topgroup -> middlegroup, user1 middlegroup -> user2, bottomgroup bottomgroup -> user1
getent group topgroup gives me result with no members, the same test on master returns the expected result.
On Mon, 2011-10-03 at 14:51 -0400, Stephen Gallagher wrote:
These patches add support for multiple search bases for users and groups in both direct-lookup and enumeration modes.
Addresses https://fedorahosted.org/sssd/ticket/868
Some notes: There is no patch adding multiple search base support for group lookups in RFC2307bis because it's meaningless right now. Since the group memberships are direct DNs, we do all of our searches as base searches, ignoring group_search_base. There is a separate ticket, https://fedorahosted.org/sssd/ticket/960 that will need to address this, taking advantage of the multiple search base features.
Also, while working through this, I opened https://fedorahosted.org/sssd/ticket/1006 as I noticed that we are using far too many separate transactions while processing RFC2307bis (which is likely the cause of the extreme slowdown that some of our users were reporting with AD).
These patches don't really work separately, but they've been broken up to make review much easier.
Patch 0001: Remove some unused options in a struct
Patch 0002: Fix size return for split_on_separator() It was returning the size of the array, rather than the number of elements. (The array was NULL-terminated). This argument was only used in one place that was actually working around this odd return value.
Patch 0003: Make sdap_get_id_specific_filter() more strict Just makes it take const char * instead of char *.
Patch 0004: Add parser for multiple search bases As discussed on the list, this will the ldap_*_search_base options in the form of: search_base[?scope?[filter][?search_base?scope?[filter]]*] This is backwards-compatible (just use a search base)
Patch 0005: Add support for multiple search bases for users
Patch 0006: Add support for multiple search bases for netgroups
Patch 0007: Add support for multiple search bases for RFC2307 groups
Patch 0008: Add support for multiple search bases for initgroups (user portion)
Patch 0009: Add support for multiple search bases for initgroups (RFC2307 group portion)
Patch 0010: Add support for multiple search bases for initgroups (RFC2307 group portion) This patch I'm not 100% sure of. It may need more processing. With RFC2307, it was safe to have duplicate groups in the list, because only the group name is important (and we guarantee that the name list has only unique values before saving it). With RFC2307bis, I'm not sure if it's safe to add two groups that may have the same name. Comments welcome.
Patch 0011: Update manpages with multiple search base information
Patch 0012: Convert ldap_*_search_filter Instead of making this a global option for all user lookups, make it only used if the search base is passed without an explicit filter. The idea here is to deprecate the old separate ldap_user_search_filter and ldap_group_search_filter options in favor of the new representation (which is closer to the traditional nss_ldap representation).
Patch 0013: Add support for multiple search bases for user enumeration
Patch 0014: Add support for multiple search bases for group enumeration This changes our behavior slightly with regard to handling direct lookups that return more than one entry. In the old code, we had a bug that would cause SSSD to treat this as an enumeration (instead of a direct lookup). This was a bug and has been fixed incidentally as part of this modification.
For the record, because I forgot to mention it in my first email: I tried to reproduce the issue jzeleny saw in the earlier thread without success. Either I inadvertently fixed it with the changes I made or it was caused by other changes in his environment (possibly the memberOf work).
I tested it with exactly the same users and groups, using SSSD speaking to a FreeIPA server (in both rfc2307bis mode and ipa backend).
His original comment:
So NACK after all. Following test failed:
I tried following membership structure. I have 3 groups: topgroup, middlegroup and bottomgroup and 2 users: user1 and user2
The relationship is following: topgroup -> middlegroup, user1 middlegroup -> user2, bottomgroup bottomgroup -> user1
getent group topgroup gives me result with no members, the same test on master returns the expected result.
Stephen, I still see the same error I reported. This time I had time to take a look into logs and I found out that with your patches, a lookup on IPA server is performed on the entire tree whereas the lookup without your patch is performed only on cn=accounts subtree. As a result, the lookup with your patches finds two group records (one in cn=accounts and one in cn=compat) and refuses to continue. I have no ldap search base configured manually, it is taken from IPA server.
Jan
On Tue, 2011-10-04 at 12:55 +0200, Jan Zelený wrote:
On Mon, 2011-10-03 at 14:51 -0400, Stephen Gallagher wrote:
These patches add support for multiple search bases for users and groups in both direct-lookup and enumeration modes.
Addresses https://fedorahosted.org/sssd/ticket/868
Some notes: There is no patch adding multiple search base support for group lookups in RFC2307bis because it's meaningless right now. Since the group memberships are direct DNs, we do all of our searches as base searches, ignoring group_search_base. There is a separate ticket, https://fedorahosted.org/sssd/ticket/960 that will need to address this, taking advantage of the multiple search base features.
Also, while working through this, I opened https://fedorahosted.org/sssd/ticket/1006 as I noticed that we are using far too many separate transactions while processing RFC2307bis (which is likely the cause of the extreme slowdown that some of our users were reporting with AD).
These patches don't really work separately, but they've been broken up to make review much easier.
Patch 0001: Remove some unused options in a struct
Patch 0002: Fix size return for split_on_separator() It was returning the size of the array, rather than the number of elements. (The array was NULL-terminated). This argument was only used in one place that was actually working around this odd return value.
Patch 0003: Make sdap_get_id_specific_filter() more strict Just makes it take const char * instead of char *.
Patch 0004: Add parser for multiple search bases As discussed on the list, this will the ldap_*_search_base options in the form of: search_base[?scope?[filter][?search_base?scope?[filter]]*] This is backwards-compatible (just use a search base)
Patch 0005: Add support for multiple search bases for users
Patch 0006: Add support for multiple search bases for netgroups
Patch 0007: Add support for multiple search bases for RFC2307 groups
Patch 0008: Add support for multiple search bases for initgroups (user portion)
Patch 0009: Add support for multiple search bases for initgroups (RFC2307 group portion)
Patch 0010: Add support for multiple search bases for initgroups (RFC2307 group portion) This patch I'm not 100% sure of. It may need more processing. With RFC2307, it was safe to have duplicate groups in the list, because only the group name is important (and we guarantee that the name list has only unique values before saving it). With RFC2307bis, I'm not sure if it's safe to add two groups that may have the same name. Comments welcome.
Patch 0011: Update manpages with multiple search base information
Patch 0012: Convert ldap_*_search_filter Instead of making this a global option for all user lookups, make it only used if the search base is passed without an explicit filter. The idea here is to deprecate the old separate ldap_user_search_filter and ldap_group_search_filter options in favor of the new representation (which is closer to the traditional nss_ldap representation).
Patch 0013: Add support for multiple search bases for user enumeration
Patch 0014: Add support for multiple search bases for group enumeration This changes our behavior slightly with regard to handling direct lookups that return more than one entry. In the old code, we had a bug that would cause SSSD to treat this as an enumeration (instead of a direct lookup). This was a bug and has been fixed incidentally as part of this modification.
For the record, because I forgot to mention it in my first email: I tried to reproduce the issue jzeleny saw in the earlier thread without success. Either I inadvertently fixed it with the changes I made or it was caused by other changes in his environment (possibly the memberOf work).
I tested it with exactly the same users and groups, using SSSD speaking to a FreeIPA server (in both rfc2307bis mode and ipa backend).
His original comment:
So NACK after all. Following test failed:
I tried following membership structure. I have 3 groups: topgroup, middlegroup and bottomgroup and 2 users: user1 and user2
The relationship is following: topgroup -> middlegroup, user1 middlegroup -> user2, bottomgroup bottomgroup -> user1
getent group topgroup gives me result with no members, the same test on master returns the expected result.
Stephen, I still see the same error I reported. This time I had time to take a look into logs and I found out that with your patches, a lookup on IPA server is performed on the entire tree whereas the lookup without your patch is performed only on cn=accounts subtree. As a result, the lookup with your patches finds two group records (one in cn=accounts and one in cn=compat) and refuses to continue. I have no ldap search base configured manually, it is taken from IPA server.
Yeah, this is a bug in FreeIPA. If we use autodetection in FreeIPA right now, it returns the common search base for both compat and standard. It's not an SSSD issue. I've opened https://fedorahosted.org/freeipa/ticket/1919 upstream to deal with this. In the meantime, please just set the search base appropriately (which is done automatically by ipa-client-install).
On Tue, 2011-10-04 at 08:04 -0400, Stephen Gallagher wrote:
On Tue, 2011-10-04 at 12:55 +0200, Jan Zelený wrote:
On Mon, 2011-10-03 at 14:51 -0400, Stephen Gallagher wrote:
These patches add support for multiple search bases for users and groups in both direct-lookup and enumeration modes.
Addresses https://fedorahosted.org/sssd/ticket/868
Some notes: There is no patch adding multiple search base support for group lookups in RFC2307bis because it's meaningless right now. Since the group memberships are direct DNs, we do all of our searches as base searches, ignoring group_search_base. There is a separate ticket, https://fedorahosted.org/sssd/ticket/960 that will need to address this, taking advantage of the multiple search base features.
Also, while working through this, I opened https://fedorahosted.org/sssd/ticket/1006 as I noticed that we are using far too many separate transactions while processing RFC2307bis (which is likely the cause of the extreme slowdown that some of our users were reporting with AD).
These patches don't really work separately, but they've been broken up to make review much easier.
Patch 0001: Remove some unused options in a struct
Patch 0002: Fix size return for split_on_separator() It was returning the size of the array, rather than the number of elements. (The array was NULL-terminated). This argument was only used in one place that was actually working around this odd return value.
Patch 0003: Make sdap_get_id_specific_filter() more strict Just makes it take const char * instead of char *.
Patch 0004: Add parser for multiple search bases As discussed on the list, this will the ldap_*_search_base options in the form of: search_base[?scope?[filter][?search_base?scope?[filter]]*] This is backwards-compatible (just use a search base)
Patch 0005: Add support for multiple search bases for users
Patch 0006: Add support for multiple search bases for netgroups
Patch 0007: Add support for multiple search bases for RFC2307 groups
Patch 0008: Add support for multiple search bases for initgroups (user portion)
Patch 0009: Add support for multiple search bases for initgroups (RFC2307 group portion)
Patch 0010: Add support for multiple search bases for initgroups (RFC2307 group portion) This patch I'm not 100% sure of. It may need more processing. With RFC2307, it was safe to have duplicate groups in the list, because only the group name is important (and we guarantee that the name list has only unique values before saving it). With RFC2307bis, I'm not sure if it's safe to add two groups that may have the same name. Comments welcome.
Patch 0011: Update manpages with multiple search base information
Patch 0012: Convert ldap_*_search_filter Instead of making this a global option for all user lookups, make it only used if the search base is passed without an explicit filter. The idea here is to deprecate the old separate ldap_user_search_filter and ldap_group_search_filter options in favor of the new representation (which is closer to the traditional nss_ldap representation).
Patch 0013: Add support for multiple search bases for user enumeration
Patch 0014: Add support for multiple search bases for group enumeration This changes our behavior slightly with regard to handling direct lookups that return more than one entry. In the old code, we had a bug that would cause SSSD to treat this as an enumeration (instead of a direct lookup). This was a bug and has been fixed incidentally as part of this modification.
For the record, because I forgot to mention it in my first email: I tried to reproduce the issue jzeleny saw in the earlier thread without success. Either I inadvertently fixed it with the changes I made or it was caused by other changes in his environment (possibly the memberOf work).
I tested it with exactly the same users and groups, using SSSD speaking to a FreeIPA server (in both rfc2307bis mode and ipa backend).
His original comment:
So NACK after all. Following test failed:
I tried following membership structure. I have 3 groups: topgroup, middlegroup and bottomgroup and 2 users: user1 and user2
The relationship is following: topgroup -> middlegroup, user1 middlegroup -> user2, bottomgroup bottomgroup -> user1
getent group topgroup gives me result with no members, the same test on master returns the expected result.
Stephen, I still see the same error I reported. This time I had time to take a look into logs and I found out that with your patches, a lookup on IPA server is performed on the entire tree whereas the lookup without your patch is performed only on cn=accounts subtree. As a result, the lookup with your patches finds two group records (one in cn=accounts and one in cn=compat) and refuses to continue. I have no ldap search base configured manually, it is taken from IPA server.
Yeah, this is a bug in FreeIPA. If we use autodetection in FreeIPA right now, it returns the common search base for both compat and standard. It's not an SSSD issue. I've opened https://fedorahosted.org/freeipa/ticket/1919 upstream to deal with this. In the meantime, please just set the search base appropriately (which is done automatically by ipa-client-install).
Well, I have a bit of egg on my face. Jan was completely right, and I had forgotten to ensure that the IPA provider was picking up the correct search bases. I've now updated patch 0004 to correct this.
On Tue, 2011-10-04 at 08:04 -0400, Stephen Gallagher wrote:
On Tue, 2011-10-04 at 12:55 +0200, Jan Zelený wrote:
On Mon, 2011-10-03 at 14:51 -0400, Stephen Gallagher wrote:
These patches add support for multiple search bases for users and groups in both direct-lookup and enumeration modes.
Addresses https://fedorahosted.org/sssd/ticket/868
Some notes: There is no patch adding multiple search base support for group lookups in RFC2307bis because it's meaningless right now. Since the group memberships are direct DNs, we do all of our searches as base searches, ignoring group_search_base. There is a separate ticket, https://fedorahosted.org/sssd/ticket/960 that will need to address this, taking advantage of the multiple search base features.
Also, while working through this, I opened https://fedorahosted.org/sssd/ticket/1006 as I noticed that we are using far too many separate transactions while processing RFC2307bis (which is likely the cause of the extreme slowdown that some of our users were reporting with AD).
These patches don't really work separately, but they've been broken up to make review much easier.
Patch 0001: Remove some unused options in a struct
Patch 0002: Fix size return for split_on_separator() It was returning the size of the array, rather than the number of elements. (The array was NULL-terminated). This argument was only used in one place that was actually working around this odd return value.
Patch 0003: Make sdap_get_id_specific_filter() more strict Just makes it take const char * instead of char *.
Patch 0004: Add parser for multiple search bases As discussed on the list, this will the ldap_*_search_base options in the form of: search_base[?scope?[filter][?search_base?scope?[filter]]*] This is backwards-compatible (just use a search base)
Patch 0005: Add support for multiple search bases for users
Patch 0006: Add support for multiple search bases for netgroups
Patch 0007: Add support for multiple search bases for RFC2307 groups
Patch 0008: Add support for multiple search bases for initgroups (user portion)
Patch 0009: Add support for multiple search bases for initgroups (RFC2307 group portion)
Patch 0010: Add support for multiple search bases for initgroups (RFC2307 group portion) This patch I'm not 100% sure of. It may need more processing. With RFC2307, it was safe to have duplicate groups in the list, because only the group name is important (and we guarantee that the name list has only unique values before saving it). With RFC2307bis, I'm not sure if it's safe to add two groups that may have the same name. Comments welcome.
Patch 0011: Update manpages with multiple search base information
Patch 0012: Convert ldap_*_search_filter Instead of making this a global option for all user lookups, make it only used if the search base is passed without an explicit filter. The idea here is to deprecate the old separate ldap_user_search_filter and ldap_group_search_filter options in favor of the new representation (which is closer to the traditional nss_ldap representation).
Patch 0013: Add support for multiple search bases for user enumeration
Patch 0014: Add support for multiple search bases for group enumeration This changes our behavior slightly with regard to handling direct lookups that return more than one entry. In the old code, we had a bug that would cause SSSD to treat this as an enumeration (instead of a direct lookup). This was a bug and has been fixed incidentally as part of this modification.
For the record, because I forgot to mention it in my first email: I tried to reproduce the issue jzeleny saw in the earlier thread without success. Either I inadvertently fixed it with the changes I made or it was caused by other changes in his environment (possibly the memberOf work).
I tested it with exactly the same users and groups, using SSSD speaking to a FreeIPA server (in both rfc2307bis mode and ipa backend).
His original comment:
So NACK after all. Following test failed:
I tried following membership structure. I have 3 groups: topgroup, middlegroup and bottomgroup and 2 users: user1 and user2
The relationship is following: topgroup -> middlegroup, user1 middlegroup -> user2, bottomgroup bottomgroup -> user1
getent group topgroup gives me result with no members, the same test on master returns the expected result.
Stephen, I still see the same error I reported. This time I had time to take a look into logs and I found out that with your patches, a lookup on IPA server is performed on the entire tree whereas the lookup without your patch is performed only on cn=accounts subtree. As a result, the lookup with your patches finds two group records (one in cn=accounts and one in cn=compat) and refuses to continue. I have no ldap search base configured manually, it is taken from IPA server.
Yeah, this is a bug in FreeIPA. If we use autodetection in FreeIPA right now, it returns the common search base for both compat and standard. It's not an SSSD issue. I've opened https://fedorahosted.org/freeipa/ticket/1919 upstream to deal with this. In the meantime, please just set the search base appropriately (which is done automatically by ipa-client-install).
Well, I have a bit of egg on my face. Jan was completely right, and I had forgotten to ensure that the IPA provider was picking up the correct search bases. I've now updated patch 0004 to correct this.
Steve, I finally finished the review:
#1-9: ACK
#11: - mention what other values are acceptable for scope (you have only subtree in the example) - maybe mention a reference to a document about LDAP filters? Or a brief description how the filter can look like. This is not necessary, just a thought to avoid some user questions in the future.
Consider following patches ACKed, I leave suggested changes at your discretion:
#10: sdap_initgr_rfc2307bis_send - how about making one fail/done label and some gotos? sdap_initgr_rfc2307bis_process - I'd rather use memcpy than the cycle
#13: sdap_get_users_process - I'd rather use memcpy than the cycle
#14: sdap_get_groups_process - I'd rather use memcpy than the cycle
Thanks Jan
On Tue, 2011-10-04 at 08:04 -0400, Stephen Gallagher wrote:
On Tue, 2011-10-04 at 12:55 +0200, Jan Zelený wrote:
On Mon, 2011-10-03 at 14:51 -0400, Stephen Gallagher wrote:
These patches add support for multiple search bases for users and groups in both direct-lookup and enumeration modes.
Addresses https://fedorahosted.org/sssd/ticket/868
Some notes: There is no patch adding multiple search base support for group lookups in RFC2307bis because it's meaningless right now. Since the group memberships are direct DNs, we do all of our searches as base searches, ignoring group_search_base. There is a separate ticket, https://fedorahosted.org/sssd/ticket/960 that will need to address this, taking advantage of the multiple search base features.
Also, while working through this, I opened https://fedorahosted.org/sssd/ticket/1006 as I noticed that we are using far too many separate transactions while processing RFC2307bis (which is likely the cause of the extreme slowdown that some of our users were reporting with AD).
These patches don't really work separately, but they've been broken up to make review much easier.
Patch 0001: Remove some unused options in a struct
Patch 0002: Fix size return for split_on_separator() It was returning the size of the array, rather than the number of elements. (The array was NULL-terminated). This argument was only used in one place that was actually working around this odd return value.
Patch 0003: Make sdap_get_id_specific_filter() more strict Just makes it take const char * instead of char *.
Patch 0004: Add parser for multiple search bases As discussed on the list, this will the ldap_*_search_base options in the form of: search_base[?scope?[filter][?search_base?scope?[filter]]*] This is backwards-compatible (just use a search base)
Patch 0005: Add support for multiple search bases for users
Patch 0006: Add support for multiple search bases for netgroups
Patch 0007: Add support for multiple search bases for RFC2307 groups
Patch 0008: Add support for multiple search bases for initgroups (user portion)
Patch 0009: Add support for multiple search bases for initgroups (RFC2307 group portion)
Patch 0010: Add support for multiple search bases for initgroups (RFC2307 group portion) This patch I'm not 100% sure of. It may need more processing. With RFC2307, it was safe to have duplicate groups in the list, because only the group name is important (and we guarantee that the name list has only unique values before saving it). With RFC2307bis, I'm not sure if it's safe to add two groups that may have the same name. Comments welcome.
Patch 0011: Update manpages with multiple search base information
Patch 0012: Convert ldap_*_search_filter Instead of making this a global option for all user lookups, make it only used if the search base is passed without an explicit filter. The idea here is to deprecate the old separate ldap_user_search_filter and ldap_group_search_filter options in favor of the new representation (which is closer to the traditional nss_ldap representation).
Patch 0013: Add support for multiple search bases for user enumeration
Patch 0014: Add support for multiple search bases for group enumeration This changes our behavior slightly with regard to handling direct lookups that return more than one entry. In the old code, we had a bug that would cause SSSD to treat this as an enumeration (instead of a direct lookup). This was a bug and has been fixed incidentally as part of this modification.
For the record, because I forgot to mention it in my first email: I tried to reproduce the issue jzeleny saw in the earlier thread without success. Either I inadvertently fixed it with the changes I made or it was caused by other changes in his environment (possibly the memberOf work).
I tested it with exactly the same users and groups, using SSSD speaking to a FreeIPA server (in both rfc2307bis mode and ipa backend).
His original comment:
So NACK after all. Following test failed:
I tried following membership structure. I have 3 groups: topgroup, middlegroup and bottomgroup and 2 users: user1 and user2
The relationship is following: topgroup -> middlegroup, user1 middlegroup -> user2, bottomgroup bottomgroup -> user1
getent group topgroup gives me result with no members, the same test on master returns the expected result.
Stephen, I still see the same error I reported. This time I had time to take a look into logs and I found out that with your patches, a lookup on IPA server is performed on the entire tree whereas the lookup without your patch is performed only on cn=accounts subtree. As a result, the lookup with your patches finds two group records (one in cn=accounts and one in cn=compat) and refuses to continue. I have no ldap search base configured manually, it is taken from IPA server.
Yeah, this is a bug in FreeIPA. If we use autodetection in FreeIPA right now, it returns the common search base for both compat and standard. It's not an SSSD issue. I've opened https://fedorahosted.org/freeipa/ticket/1919 upstream to deal with this. In the meantime, please just set the search base appropriately (which is done automatically by ipa-client-install).
Well, I have a bit of egg on my face. Jan was completely right, and I had forgotten to ensure that the IPA provider was picking up the correct search bases. I've now updated patch 0004 to correct this.
Steve, I finally finished the review:
#1-9: ACK
#11:
- mention what other values are acceptable for scope (you have only subtree
in the example)
- maybe mention a reference to a document about LDAP filters? Or a brief
description how the filter can look like. This is not necessary, just a thought to avoid some user questions in the future.
Consider following patches ACKed, I leave suggested changes at your discretion:
#10: sdap_initgr_rfc2307bis_send
- how about making one fail/done label and some gotos?
sdap_initgr_rfc2307bis_process
- I'd rather use memcpy than the cycle
#13: sdap_get_users_process
- I'd rather use memcpy than the cycle
#14: sdap_get_groups_process
- I'd rather use memcpy than the cycle
Implementing multiple search bases into IPA provider, I noticed today something else in patch #0006.
in function sdap_get_netgroups_process() you call talloc_zfree() on two variables. I believe both of those are redundant: - if count is zero then state->netgroups will be NULL - state->filter is either freed in repeated call of sdap_get_netgroups_next_base() or it will be freed with the entire context due to an error.
I noticed similar code also being in patches #0005, #0007, #0008, however I didn't inspect it very deeply there, so I don't know for sure that it is redundant there as well.
Thanks Jan
Jan Zelený jzeleny@redhat.com wrote:
On Tue, 2011-10-04 at 08:04 -0400, Stephen Gallagher wrote:
On Tue, 2011-10-04 at 12:55 +0200, Jan Zelený wrote:
On Mon, 2011-10-03 at 14:51 -0400, Stephen Gallagher wrote: > These patches add support for multiple search bases for users > and groups in both direct-lookup and enumeration modes. > > Addresses https://fedorahosted.org/sssd/ticket/868 > > Some notes: There is no patch adding multiple search base > support for group lookups in RFC2307bis because it's > meaningless right now. Since the group memberships are direct > DNs, we do all of our searches as base searches, ignoring > group_search_base. There is a separate ticket, > https://fedorahosted.org/sssd/ticket/960 that will need to > address this, taking advantage of the multiple search base > features. > > Also, while working through this, I opened > https://fedorahosted.org/sssd/ticket/1006 as I noticed that we > are using far too many separate transactions while processing > RFC2307bis (which is likely the cause of the extreme slowdown > that some of our users were reporting with AD). > > These patches don't really work separately, but they've been > broken up to make review much easier. > > > Patch 0001: Remove some unused options in a struct > > Patch 0002: Fix size return for split_on_separator() > It was returning the size of the array, rather than the number > of elements. (The array was NULL-terminated). This argument > was only used in one place that was actually working around > this odd return value. > > Patch 0003: Make sdap_get_id_specific_filter() more strict > Just makes it take const char * instead of char *. > > Patch 0004: Add parser for multiple search bases > As discussed on the list, this will the ldap_*_search_base > options in the form of: > search_base[?scope?[filter][?search_base?scope?[filter]]*] > This is backwards-compatible (just use a search base) > > Patch 0005: Add support for multiple search bases for users > > Patch 0006: Add support for multiple search bases for netgroups > > Patch 0007: Add support for multiple search bases for RFC2307 > groups > > Patch 0008: Add support for multiple search bases for > initgroups (user portion) > > Patch 0009: Add support for multiple search bases for > initgroups (RFC2307 group portion) > > Patch 0010: Add support for multiple search bases for > initgroups (RFC2307 group portion) > This patch I'm not 100% sure of. It may need more processing. > With RFC2307, it was safe to have duplicate groups in the list, > because only the group name is important (and we guarantee that > the name list has only unique values before saving it). With > RFC2307bis, I'm not sure if it's safe to add two groups that > may have the same name. Comments welcome. > > Patch 0011: Update manpages with multiple search base > information > > Patch 0012: Convert ldap_*_search_filter > Instead of making this a global option for all user lookups, > make it only used if the search base is passed without an > explicit filter. The idea here is to deprecate the old > separate ldap_user_search_filter and ldap_group_search_filter > options in favor of the new representation (which is closer to > the traditional nss_ldap representation). > > Patch 0013: Add support for multiple search bases for user > enumeration > > Patch 0014: Add support for multiple search bases for group > enumeration This changes our behavior slightly with regard to > handling direct lookups that return more than one entry. In the > old code, we had a bug that would cause SSSD to treat this as > an enumeration (instead of a direct lookup). This was a bug > and has been fixed incidentally as part of this modification.
For the record, because I forgot to mention it in my first email: I tried to reproduce the issue jzeleny saw in the earlier thread without success. Either I inadvertently fixed it with the changes I made or it was caused by other changes in his environment (possibly the memberOf work).
I tested it with exactly the same users and groups, using SSSD speaking to a FreeIPA server (in both rfc2307bis mode and ipa backend).
His original comment:
So NACK after all. Following test failed:
I tried following membership structure. I have 3 groups: topgroup, middlegroup and bottomgroup and 2 users: user1 and user2
The relationship is following: topgroup -> middlegroup, user1 middlegroup -> user2, bottomgroup bottomgroup -> user1
getent group topgroup gives me result with no members, the same test on master returns the expected result.
Stephen, I still see the same error I reported. This time I had time to take a look into logs and I found out that with your patches, a lookup on IPA server is performed on the entire tree whereas the lookup without your patch is performed only on cn=accounts subtree. As a result, the lookup with your patches finds two group records (one in cn=accounts and one in cn=compat) and refuses to continue. I have no ldap search base configured manually, it is taken from IPA server.
Yeah, this is a bug in FreeIPA. If we use autodetection in FreeIPA right now, it returns the common search base for both compat and standard. It's not an SSSD issue. I've opened https://fedorahosted.org/freeipa/ticket/1919 upstream to deal with this. In the meantime, please just set the search base appropriately (which is done automatically by ipa-client-install).
Well, I have a bit of egg on my face. Jan was completely right, and I had forgotten to ensure that the IPA provider was picking up the correct search bases. I've now updated patch 0004 to correct this.
Steve, I finally finished the review:
#1-9: ACK
#11:
- mention what other values are acceptable for scope (you have only
subtree in the example)
- maybe mention a reference to a document about LDAP filters? Or a brief
description how the filter can look like. This is not necessary, just a thought to avoid some user questions in the future.
Consider following patches ACKed, I leave suggested changes at your discretion:
#10: sdap_initgr_rfc2307bis_send
- how about making one fail/done label and some gotos?
sdap_initgr_rfc2307bis_process
- I'd rather use memcpy than the cycle
#13: sdap_get_users_process
- I'd rather use memcpy than the cycle
#14: sdap_get_groups_process
- I'd rather use memcpy than the cycle
Implementing multiple search bases into IPA provider, I noticed today something else in patch #0006.
in function sdap_get_netgroups_process() you call talloc_zfree() on two variables. I believe both of those are redundant:
- if count is zero then state->netgroups will be NULL
- state->filter is either freed in repeated call of
sdap_get_netgroups_next_base() or it will be freed with the entire context due to an error.
I noticed similar code also being in patches #0005, #0007, #0008, however I didn't inspect it very deeply there, so I don't know for sure that it is redundant there as well.
One more nack, I noticed that search bases aren't parsed in IPA provider when they are explicitly set in config file.
Jan
On Wed, 2011-10-19 at 00:38 +0200, Jan Zeleny wrote:
Jan Zelený jzeleny@redhat.com wrote:
Steve, I finally finished the review:
#1-9: ACK
#11:
- mention what other values are acceptable for scope (you have only
subtree in the example)
- maybe mention a reference to a document about LDAP filters? Or a brief
description how the filter can look like. This is not necessary, just a thought to avoid some user questions in the future.
More detail added to the manpage
Consider following patches ACKed, I leave suggested changes at your discretion:
#10: sdap_initgr_rfc2307bis_send
- how about making one fail/done label and some gotos?
Agreed and done.
sdap_initgr_rfc2307bis_process
- I'd rather use memcpy than the cycle
I disagree. I find memcpy to be hard to read. With the cycle, it's clear exactly what assignment is being made. (Besides, gcc will probably optimize to the same result).
#13: sdap_get_users_process
- I'd rather use memcpy than the cycle
See above
#14: sdap_get_groups_process
- I'd rather use memcpy than the cycle
See above
Implementing multiple search bases into IPA provider, I noticed today something else in patch #0006.
in function sdap_get_netgroups_process() you call talloc_zfree() on two variables. I believe both of those are redundant:
- if count is zero then state->netgroups will be NULL
- state->filter is either freed in repeated call of
sdap_get_netgroups_next_base() or it will be freed with the entire context due to an error.
I noticed similar code also being in patches #0005, #0007, #0008, however I didn't inspect it very deeply there, so I don't know for sure that it is redundant there as well.
You're right, they were redundant. Harmless, but better to be defensive so we don't have to fix two places in the future if we change something. Fixed.
One more nack, I noticed that search bases aren't parsed in IPA provider when they are explicitly set in config file.
Fixed.
New patches attached are rebased atop the current master.
On Wed, 2011-10-19 at 00:38 +0200, Jan Zeleny wrote:
Jan Zelený jzeleny@redhat.com wrote:
Steve, I finally finished the review:
#1-9: ACK
#11:
- mention what other values are acceptable for scope (you have only
subtree in the example)
- maybe mention a reference to a document about LDAP filters? Or a
brief description how the filter can look like. This is not necessary, just a thought to avoid some user questions in the future.
More detail added to the manpage
Consider following patches ACKed, I leave suggested changes at your discretion:
#10: sdap_initgr_rfc2307bis_send
- how about making one fail/done label and some gotos?
Agreed and done.
sdap_initgr_rfc2307bis_process
- I'd rather use memcpy than the cycle
I disagree. I find memcpy to be hard to read. With the cycle, it's clear exactly what assignment is being made. (Besides, gcc will probably optimize to the same result).
#13: sdap_get_users_process
- I'd rather use memcpy than the cycle
See above
#14: sdap_get_groups_process
- I'd rather use memcpy than the cycle
See above
Implementing multiple search bases into IPA provider, I noticed today something else in patch #0006.
in function sdap_get_netgroups_process() you call talloc_zfree() on two variables. I believe both of those are redundant:
- if count is zero then state->netgroups will be NULL
- state->filter is either freed in repeated call of
sdap_get_netgroups_next_base() or it will be freed with the entire context due to an error.
I noticed similar code also being in patches #0005, #0007, #0008, however I didn't inspect it very deeply there, so I don't know for sure that it is redundant there as well.
You're right, they were redundant. Harmless, but better to be defensive so we don't have to fix two places in the future if we change something. Fixed.
One more nack, I noticed that search bases aren't parsed in IPA provider when they are explicitly set in config file.
Fixed.
New patches attached are rebased atop the current master.
Ack to all
Jan
On Wed, 2011-11-02 at 14:23 +0100, Jan Zelený wrote:
On Wed, 2011-10-19 at 00:38 +0200, Jan Zeleny wrote:
Jan Zelený jzeleny@redhat.com wrote:
Steve, I finally finished the review:
#1-9: ACK
#11:
- mention what other values are acceptable for scope (you have only
subtree in the example)
- maybe mention a reference to a document about LDAP filters? Or a
brief description how the filter can look like. This is not necessary, just a thought to avoid some user questions in the future.
More detail added to the manpage
Consider following patches ACKed, I leave suggested changes at your discretion:
#10: sdap_initgr_rfc2307bis_send
- how about making one fail/done label and some gotos?
Agreed and done.
sdap_initgr_rfc2307bis_process
- I'd rather use memcpy than the cycle
I disagree. I find memcpy to be hard to read. With the cycle, it's clear exactly what assignment is being made. (Besides, gcc will probably optimize to the same result).
#13: sdap_get_users_process
- I'd rather use memcpy than the cycle
See above
#14: sdap_get_groups_process
- I'd rather use memcpy than the cycle
See above
Implementing multiple search bases into IPA provider, I noticed today something else in patch #0006.
in function sdap_get_netgroups_process() you call talloc_zfree() on two variables. I believe both of those are redundant:
- if count is zero then state->netgroups will be NULL
- state->filter is either freed in repeated call of
sdap_get_netgroups_next_base() or it will be freed with the entire context due to an error.
I noticed similar code also being in patches #0005, #0007, #0008, however I didn't inspect it very deeply there, so I don't know for sure that it is redundant there as well.
You're right, they were redundant. Harmless, but better to be defensive so we don't have to fix two places in the future if we change something. Fixed.
One more nack, I noticed that search bases aren't parsed in IPA provider when they are explicitly set in config file.
Fixed.
New patches attached are rebased atop the current master.
Ack to all
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jan Zelený -
Stephen Gallagher