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 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
