[SSSD] [PATCH] Retain group members in setups where id_provider=ad and ignore_group_members=True

Sumit Bose sbose at redhat.com
Mon May 11 09:31:43 UTC 2015


On Fri, May 08, 2015 at 03:24:29PM +0200, Jakub Hrozek wrote:
> Hi,
> 
> the attached patch fixes https://fedorahosted.org/sssd/ticket/2646. See the
> commit message for the problem description. The bug was found by Lukas
> (thanks!), I just wrote the patch.

Hi Jakub,

thank you for the patch, I haven't checked it closely yet, because I
would like to discuss some alternative approaches first.

The scheme to save the groups with the SID in the DN was introduced to
speed up initgroups request when id-mapping is used. Iirc this path is
currently only used for the parent domain if id-mapping is enabled. I do
not have any statistics but I would expect that there are already many
cases where this code path is not used at all either because POSIX
attributes in AD are used or the user is from a subdomain. This makes me
wonder if we cannot pay the price and always lookup the group and skip
the SID-as-name path completely? Maybe only if ignore_group_members=True
is set?

As a second alternative I was thinking if it is necessary to rename the
group at all? Historically there were some sysdb calls which generate
the DN of a group object for lookups with the aim to speed things up
(assuming a base search is faster then an indexed sub-tree search).
With all the short-name/fully-qualified-name/id-view changes I wonder if
this is used for lookups anymore. If not, I think there is no need to
rename the group at all. If there are still such lookups (iirc Michal is
currently working on the sysdb code, maybe he knows?) we might want to
replace them by proper searches and eliminate the need to rename the
group. Here I do not have any data either but I would expect that there
is no big difference in ldb between a base search and an indexed
sub-tree search.



bye,
Sumit

> 
> I was not really happy with some parts of the patch myself, but I couldn't
> find a better way to fix the ticket. Here are some parts of the patch that I
> think need careful review:
> - is_fake_tgs_group() uses heuristics based on the AD schema, RDN value and
> and POSIX/non-POSIX flag. Is that OK or too much of a hack?
> - sdap_get_tokengroup_members() plays a bit of a trick on the memory
> hierarchy. I wanted the API of this function to only return a single member
> (a ldb_message_element) but at the same time, the cache search returns only
> the top-level ldb_message and the first element as talloc pointers. So the
> functions returns the element, which points to the message, but the element
> owns the message. I checked with valgrind that there are no memory errors. I
> really want to avoid copying all the members, because that number can go to
> thousands. The only alternative that comes to mind is to run a second cache
> search only for the member attribute, then the member would be a talloc
> pointer -- but this way seems to rely a bit on ldb implementation which I
> don't like either.
> - sdap_save_grpmem() and sdap_save_groups() now use a saved_ldap_group
> structure. This means we now need to do an extra allocation per group, but
> the number of groups is usually not that big (max. hundreds, definitely not
> thousands like members).
> 
> Thanks for review!



More information about the sssd-devel mailing list