[SSSD] [PATCH 0/1] RFC: Fix group memberships for subdomain users

Simo Sorce simo at redhat.com
Mon Nov 12 15:02:04 UTC 2012


On Mon, 2012-11-12 at 13:30 +0100, Sumit Bose wrote:
> On Sat, Nov 10, 2012 at 10:05:36PM -0500, Simo Sorce wrote:
> > This patch changes the way subdomain users are stored in the database.
> 
> Thank you for the patch.
> 
> I run couple of test and have not see an issue so far. But I have a
> couple of comments, see below.
> 
> > 
> > The reason for changing the way we do it is that the sysdb code, before the
> > subdomain patches were added assumed a single domain per cache file. This
> > assumption beled in many other interfaces including the way users are read and
> > returned in the nss responder, as well as potentially how hbac and sudo handle
> 
> At least IPA HBAC is using the originalMemberOf attribute to find out
> about group memberships efficiently.

YEah I know, in fact I had to add your patches (which I review and
pushed) in order to make HBAC work.

> > rules for checking if users are part of a rule.
> > 
> > In order to make sure subdomain users are univocally recognized as such the
> > safest way is to change how users are saved and always save subdomain users
> > with sully qualified names.
> 
> Is your plan to store the fully qualified name for users of the
> configured domain, too?

No.
My long term plan is to revisit this whole subdomain concept and see if
we need to change anything. This patchset is a stopgap measure to avoid
potential issues (and fix the actual group membership problem, but I am
not entirely happy with as a final status.

However we need a working fix for now, and this is the shortest path,
any reconsideration of the database layout will require very careful
consideration and design decisions that we'll need to discuss a bit.

> > With this change we solve one of the most eveident issues we currently have
> > where subdomain users are not listed fully wualified in group membership when
> > they should.
> 
> yes, this works in my tests with you patch now.
> 
> > 
> > The side effect of this change is that cache files need to be removed if the
> > admin decides to change the formatting string for representing fully qualified
> > users. An action like this has many other important consequences on the system
> > so I think this limitation is perfectly reasonable.
> 
> Wouldn't is be better to use a generic fully qualified name here and
> transform it to the configured style if needed?

I thought about it, but we have 2 issues:
1) We do not have any reserve chars we can safely use to say: 'hey this
is special do not return as is in fill_grent()'
2) the point of storing memberuid is to make fill_grent fast in the
default case by not having to munge values. If we were to analyze every
single entry there fill_grent() would be impacted more than I'd like to
do on a short notice.

>  I know you are afraid
> that it will cost too much performance in e.g. fill_members() but to
> evaluate this I've written a test for fill_members() to see where we
> are:

It is just not performance it is correctness about all sysdb users. I
wanted the least intrusive patch that made absolutely certain users were
unique. Anything else I think should be deferred to a reconsideration of
this whole business during 1.10 life cycle or later.

> Running suite(s): nss
> Testcase [0], members [1], fqdn [false], case-sensitive [false], body length [6], duration [0s 73us]
> Testcase [1], members [10], fqdn [false], case-sensitive [false], body length [60], duration [0s 12us]
> Testcase [2], members [100], fqdn [false], case-sensitive [false], body length [600], duration [0s 104us]
> Testcase [3], members [1000], fqdn [false], case-sensitive [false], body length [6000], duration [0s 512us]
> Testcase [4], members [10000], fqdn [false], case-sensitive [false], body length [60000], duration [0s 4713us]
> Testcase [5], members [100000], fqdn [false], case-sensitive [false], body length [949624], duration [0s 61037us]
> Testcase [6], members [1], fqdn [true], case-sensitive [false], body length [18], duration [0s 45us]
> Testcase [7], members [10], fqdn [true], case-sensitive [false], body length [180], duration [0s 22us]
> Testcase [8], members [100], fqdn [true], case-sensitive [false], body length [1800], duration [0s 120us]
> Testcase [9], members [1000], fqdn [true], case-sensitive [false], body length [18000], duration [0s 794us]
> Testcase [10], members [10000], fqdn [true], case-sensitive [false], body length [180000], duration [0s 7404us]
> Testcase [11], members [100000], fqdn [true], case-sensitive [false], body length [2149624], duration [0s 90789us]
> Testcase [12], members [1], fqdn [false], case-sensitive [true], body length [6], duration [0s 8us]
> Testcase [13], members [10], fqdn [false], case-sensitive [true], body length [60], duration [0s 12us]
> Testcase [14], members [100], fqdn [false], case-sensitive [true], body length [600], duration [0s 36us]
> Testcase [15], members [1000], fqdn [false], case-sensitive [true], body length [6000], duration [0s 149us]
> Testcase [16], members [10000], fqdn [false], case-sensitive [true], body length [60000], duration [0s 1321us]
> Testcase [17], members [100000], fqdn [false], case-sensitive [true], body length [949624], duration [0s 13968us]
> Testcase [18], members [1], fqdn [true], case-sensitive [true], body length [18], duration [0s 14us]
> Testcase [19], members [10], fqdn [true], case-sensitive [true], body length [180], duration [0s 29us]
> Testcase [20], members [100], fqdn [true], case-sensitive [true], body length [1800], duration [0s 86us]
> Testcase [21], members [1000], fqdn [true], case-sensitive [true], body length [18000], duration [0s 478us]
> Testcase [22], members [10000], fqdn [true], case-sensitive [true], body length [180000], duration [0s 4320us]
> Testcase [23], members [100000], fqdn [true], case-sensitive [true], body length [2149624], duration [0s 44809us]
> 100%: Checks: 1, Failures: 0, Errors: 0
> 
> I think the values are quite comfortable and would allow some extra
> processing in fill_members(). I plan to test if extracting name and
> domain from the DN stored in the member attribute would spoil those
> values of if it can be done efficiently.

not just a performance issue, I would take correctness over performance
every day given we have the mmap cache anyway.

I am shooting for least intrusive patch at this time.

> > This patch is in RFC status because I haven't dealt with database migrations
> > to fix existing subdomain users. Would it be acceptable to simply remove
> > all subdomain user entries on upgrade ?
> 
> I would say yes. We only have to remember to give it a prominent place
> in the release notes to warn about the loss of cached password.

Ok, I'll wait for more opinions on this specific point.


Simo.

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




More information about the sssd-devel mailing list