[SSSD] [PATCHES] Improve group handling in the PAC responder

Simo Sorce simo at redhat.com
Wed Dec 5 14:04:46 UTC 2012


On Wed, 2012-12-05 at 13:36 +0100, Sumit Bose wrote:
> Hi,
> 
> this series of patches fixes https://fedorahosted.org/sssd/ticket/1666
> and https://fedorahosted.org/sssd/ticket/1672. Details can be found in
> the commit messages. Just as overview:
> 
> 0001-0006 add new structs and call to make handling groups from
> different domains easier.

Sorry Sumit but I do not really agree with patch 1, it is extremely
wasteful of space, and ugly.

I think you should create a structure for the container and not for each
element, where you have:

container {
    domain1 {
        domain_info
        array_of_gids
    }
    domain2 { ...
    ...
}


Patch 4 sounds fishy, I distinctly recall we do not touch domain_id
because that is the domain identifier. It can't change because if it
'changes' it means it is a different domain. I am not even sure you can
ever match that for updates because IIRC domains are searched by
domain_id. So where is my reasoning wrong ? Tentatively nack if I am
correct.


Patch 5 is a step in the right direction but it sounds wrong that you
search the user in the db and just skip saving if it exist. The user
info you have in the PAC may be updated and contain information that
differs from what's in the cache. At the very least you need to test
that nothing needs to change if you do not want to unconditionally
overwrite.

> 0007 and 0008 add the evaluation of the group memberships from the
> domain the user belongs to (#1666).

The domain sid retrieval looks extremely inefficient, we have a neat
optimization where you can look up the domain once and then just go rid
by rid and you are going back to build a full sid, which will then be
turned into a string and matched once again to find the domain.
I think this should be improved to use the fact you already have the
sids split out with a modicum of caching.
You can uses alexander's patch that just landed in freeipa to do binary
sid comparison w/o converting to string, and a simple array in the
function to keep a list of all dom_sid -> domaiin structs found so far,
they wont be many just a handful in most cases so a linear search there
is also fine.

> 0009 just changes an error code

ACK

> 0010 fixes #1672 by removing duplicated groups

I think using a hash table like function is fine, but I wonder if we can
save a lot of cycles here by using a smarter hash algorithm, namely just
truncate the gid and use the lower N bits. This would be orders of
magnitude more efficient and you pretty much are guaranteed to not have
many collisions because gids are unique and tend to all start from a
common base so if you take the least N significant bits you basically
have an automatic index already built in.

Building a static table on the stack like the following and only
allocating (using talloc) only on collisions would also save tons of
allocations.


struct gid_el {
    gid_t gid;
    struct gid_el *next;
} gid_table[1024] = { 0 };

the 'hash/index' is (gid & 0x2FF)


> 0011 adds a new unit test


Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list