[SSSD] [PATCHES] Better handling of large Active Directory groups

Jakub Hrozek jhrozek at redhat.com
Wed May 9 20:20:45 UTC 2012


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:
> > 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?
> > 
> 
> 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.
> 
> > 2) 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.



More information about the sssd-devel mailing list