On Mon, Nov 12, 2012 at 01:30:33PM +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.
Also native IPA netgroups and SELinux rules.
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?
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 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:
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.
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.
+1 given that not many people use the trusted domains yet, this is still OK to do. Better now than later..
Also in order to fix this important issue for 1.9 I have refrained from significantly changing sysdb interfaces or other code around domain manipulation. I think such changes are necessary in future. The consequence of not changing sysdb interfaces is that knowledge of the fact the subdonains users need to be fully qualified bleeds in various other callers. I hope I caught all the callers that need to know about this difference but I haven't yet checked sudo related code for example.
The patch is so far fully tested and allows password based logins with full HABC checking. getent passwd/group commands also return the extected outputs.