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

Pavel Březina pbrezina at redhat.com
Tue Nov 13 12:23:13 UTC 2012


On 11/12/2012 04:02 PM, Simo Sorce wrote:
> 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.

I'm OK with it given the circumstances.




More information about the sssd-devel mailing list