Fixes https://fedorahosted.org/sssd/ticket/783
Patch 0001: Adds some better debug messages to sysdb_set_entry_attr() that I found useful while debugging my AD patch
Patch 0002: Modifies build_attrs_from_map() to return a count of the attributes as well. This is necessary because there are a couple places in the code (notably the nested group deref code) where we are appending to the resulting attrs. However, if any attribute in the map was unset (as is the case by default in the ad2008r2_group_map), the resulting additions would be made AFTER a NULL-terminator and would be ignored. (It took me about two hours of debugging to figure out why the users in my deref lookups didn't have names...)
Patch 0003: Here's the meat of the changes. I did not actually implement the standards-decimating range extension from Microsoft for a couple reasons: 1) It was really difficult to shoehorn into our LDAP implementation. 2) I found a better, faster way that does not require the use of special LDAP requests.
It turns out that the ASQ requests against AD will always return all of the members of the groups. We were actually saving all of the fake users into the cache already, but what was missing were just the member/memberOf linkages, because our processing was entirely dependent upon the original lookup. So with this patch, we will add any members discovered by deref/ASQ that were not already in the member element of the initial lookup into the queue to be processed.
We don't need to worry about the non-ASQ case because we have a guarantee from AD that ASQ is always available.
On Tue, May 08, 2012 at 03:08:45PM -0400, Stephen Gallagher wrote:
Fixes https://fedorahosted.org/sssd/ticket/783
Patch 0001: Adds some better debug messages to sysdb_set_entry_attr() that I found useful while debugging my AD patch
Ack
Patch 0002: Modifies build_attrs_from_map() to return a count of the attributes as well. This is necessary because there are a couple places in the code (notably the nested group deref code) where we are appending to the resulting attrs. However, if any attribute in the map was unset (as is the case by default in the ad2008r2_group_map), the resulting additions would be made AFTER a NULL-terminator and would be ignored. (It took me about two hours of debugging to figure out why the users in my deref lookups didn't have names...)
Ack
Patch 0003: Here's the meat of the changes. I did not actually implement the standards-decimating range extension from Microsoft for a couple reasons: 1) It was really difficult to shoehorn into our LDAP implementation. 2) I found a better, faster way that does not require the use of special LDAP requests.
It turns out that the ASQ requests against AD will always return all of the members of the groups. We were actually saving all of the fake users into the cache already, but what was missing were just the member/memberOf linkages, because our processing was entirely dependent upon the original lookup. So with this patch, we will add any members discovered by deref/ASQ that were not already in the member element of the initial lookup into the queue to be processed.
We don't need to worry about the non-ASQ case because we have a guarantee from AD that ASQ is always available.
I like using the ASQ control, it's a smart solution.
Two questions related to sdap_parse_range: 1) The range_offset parameter seems to be unused for now. Does it make sense to set it at all? Would it be faster to only process it if the range_offset parameter was != NULL?
2) because SDAP_RANGE_STRING is a string constant, you can use sizeof(SDAP_RANGE_STRING) to get its length for use in rangestringlen
On Wed, 2012-05-09 at 20:01 +0200, Jakub Hrozek wrote:
On Tue, May 08, 2012 at 03:08:45PM -0400, Stephen Gallagher wrote:
Fixes https://fedorahosted.org/sssd/ticket/783
Patch 0001: Adds some better debug messages to sysdb_set_entry_attr() that I found useful while debugging my AD patch
Ack
Patch 0002: Modifies build_attrs_from_map() to return a count of the attributes as well. This is necessary because there are a couple places in the code (notably the nested group deref code) where we are appending to the resulting attrs. However, if any attribute in the map was unset (as is the case by default in the ad2008r2_group_map), the resulting additions would be made AFTER a NULL-terminator and would be ignored. (It took me about two hours of debugging to figure out why the users in my deref lookups didn't have names...)
Ack
Patch 0003: Here's the meat of the changes. I did not actually implement the standards-decimating range extension from Microsoft for a couple reasons: 1) It was really difficult to shoehorn into our LDAP implementation. 2) I found a better, faster way that does not require the use of special LDAP requests.
It turns out that the ASQ requests against AD will always return all of the members of the groups. We were actually saving all of the fake users into the cache already, but what was missing were just the member/memberOf linkages, because our processing was entirely dependent upon the original lookup. So with this patch, we will add any members discovered by deref/ASQ that were not already in the member element of the initial lookup into the queue to be processed.
We don't need to worry about the non-ASQ case because we have a guarantee from AD that ASQ is always available.
I like using the ASQ control, it's a smart solution.
Two questions related to sdap_parse_range:
- The range_offset parameter seems to be unused for now. Does it make
sense to set it at all? Would it be faster to only process it if the range_offset parameter was != NULL?
Well, I wanted to leave that in place in case we discover moving forward that we really do need to support the range extension. (For example, if AD changes its behavior wrt ASQ requests and we suddenly were unable to get all of the results and had to resort to individual lookups, for example). I'd rather leave the processing in there even if range_offset was NULL, because I'd want to confirm that we have a valid and fully-parseable attribute anyway. Since it's only used in one place, I see no real issues with passing it back.
- because SDAP_RANGE_STRING is a string constant, you can use
sizeof(SDAP_RANGE_STRING) to get its length for use in rangestringlen
Sure, I switched the strlen() to sizeof() - 1. (Although I think GCC's -O2 would do this automatically under the hood anyway).
On Wed, May 09, 2012 at 02:38:26PM -0400, Stephen Gallagher wrote:
On Wed, 2012-05-09 at 20:01 +0200, Jakub Hrozek wrote:
On Tue, May 08, 2012 at 03:08:45PM -0400, Stephen Gallagher wrote:
Fixes https://fedorahosted.org/sssd/ticket/783
Patch 0001: Adds some better debug messages to sysdb_set_entry_attr() that I found useful while debugging my AD patch
Ack
Patch 0002: Modifies build_attrs_from_map() to return a count of the attributes as well. This is necessary because there are a couple places in the code (notably the nested group deref code) where we are appending to the resulting attrs. However, if any attribute in the map was unset (as is the case by default in the ad2008r2_group_map), the resulting additions would be made AFTER a NULL-terminator and would be ignored. (It took me about two hours of debugging to figure out why the users in my deref lookups didn't have names...)
Ack
Patch 0003: Here's the meat of the changes. I did not actually implement the standards-decimating range extension from Microsoft for a couple reasons: 1) It was really difficult to shoehorn into our LDAP implementation. 2) I found a better, faster way that does not require the use of special LDAP requests.
It turns out that the ASQ requests against AD will always return all of the members of the groups. We were actually saving all of the fake users into the cache already, but what was missing were just the member/memberOf linkages, because our processing was entirely dependent upon the original lookup. So with this patch, we will add any members discovered by deref/ASQ that were not already in the member element of the initial lookup into the queue to be processed.
We don't need to worry about the non-ASQ case because we have a guarantee from AD that ASQ is always available.
I like using the ASQ control, it's a smart solution.
Two questions related to sdap_parse_range:
- The range_offset parameter seems to be unused for now. Does it make
sense to set it at all? Would it be faster to only process it if the range_offset parameter was != NULL?
Well, I wanted to leave that in place in case we discover moving forward that we really do need to support the range extension. (For example, if AD changes its behavior wrt ASQ requests and we suddenly were unable to get all of the results and had to resort to individual lookups, for example). I'd rather leave the processing in there even if range_offset was NULL, because I'd want to confirm that we have a valid and fully-parseable attribute anyway. Since it's only used in one place, I see no real issues with passing it back.
- because SDAP_RANGE_STRING is a string constant, you can use
sizeof(SDAP_RANGE_STRING) to get its length for use in rangestringlen
Sure, I switched the strlen() to sizeof() - 1. (Although I think GCC's -O2 would do this automatically under the hood anyway).
Nack, the way ldap_get_dn is used leaks memory:
+ DEBUG(SSSDBG_TRACE_INTERNAL, + ("Matched objectclass [%s] on DN [%s], will use associated map\n", + state->maps[mi].map[0].name, + ldap_get_dn(sh->ldap, msg->msg)));
The result needs to be freed with ldap_memfree().
Luckily it's only when SSSDBG_TRACE_INTERNAL is set but we should still fix it.
On Wed, 2012-05-09 at 22:20 +0200, Jakub Hrozek wrote:
Nack, the way ldap_get_dn is used leaks memory:
DEBUG(SSSDBG_TRACE_INTERNAL,("Matched objectclass [%s] on DN [%s], will use associated map\n",state->maps[mi].map[0].name,ldap_get_dn(sh->ldap, msg->msg)));The result needs to be freed with ldap_memfree().
Luckily it's only when SSSDBG_TRACE_INTERNAL is set but we should still fix it.
I modified the code so we'd look up the DN and talloc_strdup() it onto the tmp_ctx so it will get automatically freed properly.
I thought about limiting this lookup to SSSDBG_TRACE_INTERNAL, but I reason that if we want to reference the DN in any other log level in the future, we're going to have to modify this anyway. It's just easier to always copy it and avoid future heisenbugs.
New (and hopefully final) patches attached.
On Wed, May 09, 2012 at 07:19:29PM -0400, Stephen Gallagher wrote:
On Wed, 2012-05-09 at 22:20 +0200, Jakub Hrozek wrote:
Nack, the way ldap_get_dn is used leaks memory:
DEBUG(SSSDBG_TRACE_INTERNAL,("Matched objectclass [%s] on DN [%s], will use associated map\n",state->maps[mi].map[0].name,ldap_get_dn(sh->ldap, msg->msg)));The result needs to be freed with ldap_memfree().
Luckily it's only when SSSDBG_TRACE_INTERNAL is set but we should still fix it.
I modified the code so we'd look up the DN and talloc_strdup() it onto the tmp_ctx so it will get automatically freed properly.
I thought about limiting this lookup to SSSDBG_TRACE_INTERNAL, but I reason that if we want to reference the DN in any other log level in the future, we're going to have to modify this anyway. It's just easier to always copy it and avoid future heisenbugs.
New (and hopefully final) patches attached.
I think you attached the wrong patches...
From f1fe4c13dfa9ad82bec7c3ed6f0651142ef874ab Mon Sep 17 00:00:00 2001 From: Stephen Gallagher sgallagh@redhat.com Date: Wed, 9 May 2012 11:07:31 -0400
Subject: [PATCH 1/3] NSS: Add fallback_homedir option This option is similar to override_homedir, except that it will take effect only for users that do not have an explicit home directory specified in LDAP.
On Thu, 2012-05-10 at 11:15 +0200, Jakub Hrozek wrote:
On Wed, May 09, 2012 at 07:19:29PM -0400, Stephen Gallagher wrote:
On Wed, 2012-05-09 at 22:20 +0200, Jakub Hrozek wrote:
Nack, the way ldap_get_dn is used leaks memory:
DEBUG(SSSDBG_TRACE_INTERNAL,("Matched objectclass [%s] on DN [%s], will use associated map\n",state->maps[mi].map[0].name,ldap_get_dn(sh->ldap, msg->msg)));The result needs to be freed with ldap_memfree().
Luckily it's only when SSSDBG_TRACE_INTERNAL is set but we should still fix it.
I modified the code so we'd look up the DN and talloc_strdup() it onto the tmp_ctx so it will get automatically freed properly.
I thought about limiting this lookup to SSSDBG_TRACE_INTERNAL, but I reason that if we want to reference the DN in any other log level in the future, we're going to have to modify this anyway. It's just easier to always copy it and avoid future heisenbugs.
New (and hopefully final) patches attached.
I think you attached the wrong patches...
Of course I did... I was on a roll yesterday.
These should be the right ones now.
On Thu, May 10, 2012 at 07:35:01AM -0400, Stephen Gallagher wrote:
On Thu, 2012-05-10 at 11:15 +0200, Jakub Hrozek wrote:
On Wed, May 09, 2012 at 07:19:29PM -0400, Stephen Gallagher wrote:
On Wed, 2012-05-09 at 22:20 +0200, Jakub Hrozek wrote:
Nack, the way ldap_get_dn is used leaks memory:
DEBUG(SSSDBG_TRACE_INTERNAL,("Matched objectclass [%s] on DN [%s], will use associated map\n",state->maps[mi].map[0].name,ldap_get_dn(sh->ldap, msg->msg)));The result needs to be freed with ldap_memfree().
Luckily it's only when SSSDBG_TRACE_INTERNAL is set but we should still fix it.
I modified the code so we'd look up the DN and talloc_strdup() it onto the tmp_ctx so it will get automatically freed properly.
I thought about limiting this lookup to SSSDBG_TRACE_INTERNAL, but I reason that if we want to reference the DN in any other log level in the future, we're going to have to modify this anyway. It's just easier to always copy it and avoid future heisenbugs.
New (and hopefully final) patches attached.
I think you attached the wrong patches...
Of course I did... I was on a roll yesterday.
These should be the right ones now.
Ack
On Thu, 2012-05-10 at 17:07 +0200, Jakub Hrozek wrote:
On Thu, May 10, 2012 at 07:35:01AM -0400, Stephen Gallagher wrote:
On Thu, 2012-05-10 at 11:15 +0200, Jakub Hrozek wrote:
On Wed, May 09, 2012 at 07:19:29PM -0400, Stephen Gallagher wrote:
On Wed, 2012-05-09 at 22:20 +0200, Jakub Hrozek wrote:
Nack, the way ldap_get_dn is used leaks memory:
DEBUG(SSSDBG_TRACE_INTERNAL,("Matched objectclass [%s] on DN [%s], will use associated map\n",state->maps[mi].map[0].name,ldap_get_dn(sh->ldap, msg->msg)));The result needs to be freed with ldap_memfree().
Luckily it's only when SSSDBG_TRACE_INTERNAL is set but we should still fix it.
I modified the code so we'd look up the DN and talloc_strdup() it onto the tmp_ctx so it will get automatically freed properly.
I thought about limiting this lookup to SSSDBG_TRACE_INTERNAL, but I reason that if we want to reference the DN in any other log level in the future, we're going to have to modify this anyway. It's just easier to always copy it and avoid future heisenbugs.
New (and hopefully final) patches attached.
I think you attached the wrong patches...
Of course I did... I was on a roll yesterday.
These should be the right ones now.
Ack
Pushed to master.
On Thu, 2012-05-10 at 11:15 +0200, Jakub Hrozek wrote:
On Wed, May 09, 2012 at 07:19:29PM -0400, Stephen Gallagher wrote:
On Wed, 2012-05-09 at 22:20 +0200, Jakub Hrozek wrote:
Nack, the way ldap_get_dn is used leaks memory:
DEBUG(SSSDBG_TRACE_INTERNAL,("Matched objectclass [%s] on DN [%s], willuse associated map\n", + state->maps[mi].map[0].name,
ldap_get_dn(sh->ldap, msg->msg)));The result needs to be freed with ldap_memfree().
Luckily it's only when SSSDBG_TRACE_INTERNAL is set but we should still fix it.
I modified the code so we'd look up the DN and talloc_strdup() it onto the tmp_ctx so it will get automatically freed properly.
I thought about limiting this lookup to SSSDBG_TRACE_INTERNAL, but I reason that if we want to reference the DN in any other log level in the future, we're going to have to modify this anyway. It's just easier to always copy it and avoid future heisenbugs.
New (and hopefully final) patches attached.
I think you attached the wrong patches...
Of course I did... I was on a roll yesterday.
These should be the right ones now.
After review, I propose this small patch.
Thanks Jan
On Mon, 2012-05-14 at 10:13 +0200, Jan Zelený wrote:
On Thu, 2012-05-10 at 11:15 +0200, Jakub Hrozek wrote:
On Wed, May 09, 2012 at 07:19:29PM -0400, Stephen Gallagher wrote:
On Wed, 2012-05-09 at 22:20 +0200, Jakub Hrozek wrote:
Nack, the way ldap_get_dn is used leaks memory:
DEBUG(SSSDBG_TRACE_INTERNAL,("Matched objectclass [%s] on DN [%s], willuse associated map\n", + state->maps[mi].map[0].name,
ldap_get_dn(sh->ldap, msg->msg)));The result needs to be freed with ldap_memfree().
Luckily it's only when SSSDBG_TRACE_INTERNAL is set but we should still fix it.
I modified the code so we'd look up the DN and talloc_strdup() it onto the tmp_ctx so it will get automatically freed properly.
I thought about limiting this lookup to SSSDBG_TRACE_INTERNAL, but I reason that if we want to reference the DN in any other log level in the future, we're going to have to modify this anyway. It's just easier to always copy it and avoid future heisenbugs.
New (and hopefully final) patches attached.
I think you attached the wrong patches...
Of course I did... I was on a roll yesterday.
These should be the right ones now.
After review, I propose this small patch.
Ack, thanks for catching those.
On Mon, 2012-05-14 at 09:45 -0400, Stephen Gallagher wrote:
On Mon, 2012-05-14 at 10:13 +0200, Jan Zelený wrote:
On Thu, 2012-05-10 at 11:15 +0200, Jakub Hrozek wrote:
On Wed, May 09, 2012 at 07:19:29PM -0400, Stephen Gallagher wrote:
On Wed, 2012-05-09 at 22:20 +0200, Jakub Hrozek wrote:
Nack, the way ldap_get_dn is used leaks memory:
DEBUG(SSSDBG_TRACE_INTERNAL,("Matched objectclass [%s] on DN [%s], willuse associated map\n", + state->maps[mi].map[0].name,
ldap_get_dn(sh->ldap, msg->msg)));The result needs to be freed with ldap_memfree().
Luckily it's only when SSSDBG_TRACE_INTERNAL is set but we should still fix it.
I modified the code so we'd look up the DN and talloc_strdup() it onto the tmp_ctx so it will get automatically freed properly.
I thought about limiting this lookup to SSSDBG_TRACE_INTERNAL, but I reason that if we want to reference the DN in any other log level in the future, we're going to have to modify this anyway. It's just easier to always copy it and avoid future heisenbugs.
New (and hopefully final) patches attached.
I think you attached the wrong patches...
Of course I did... I was on a roll yesterday.
These should be the right ones now.
After review, I propose this small patch.
Ack, thanks for catching those.
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Stephen Gallagher