Re: [SSSD] [PATCH] Retain group members in setups where id_provider=ad and ignore_group_members=True
On Mon, May 11, 2015 at 12:12:00PM +0200, Jakub Hrozek wrote:
On Mon, May 11, 2015 at 11:31:43AM +0200, Sumit Bose wrote:
> 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?
This could work, but then there would be some performance impact.
Currently the initgroups operation with ID mapping AD domain is precisely
one LDAP lookup. With this change, it would turn into N LDAP lookups for
N groups (if none of the groups is cached) I would prefer not to decrease
performance of the default setup for a corner case (even if often used)
like ignore_group_members.
> Maybe only if ignore_group_members=True
> is set?
Yes, perhaps..maybe as a short-term before we implement what you propose
below?
>
> 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.
I like with this direction more. I was even thinking we should not use name
in RDN at all with the AD domain and always use objectSID. Then we would
also have fewer problems changing numerical IDs..
My only concern is that this is an invasive solution that might bring
bugs. Currently there is around 40 occurences of LDB_SCOPE_BASE in the
code. At least those in sysdb_views.c and sysdb_asq_search(),
sysdb_initgroups() and sysdb_initgroups_with_views() look like they
would need to be converted. And currently the ignore_group_members bug
is hitting users..
Oh and one more argument against renaming -- it is actually performance
intensive, because the memberof plugin needs to establish all the links
again..
So I would prefer to not use the name as RDN value at all, but the scope
seems out of question for 1.12 and even for 1.13 looks like a stretch
goal.