[SSSD] [PATCH] Return all user members for RFC2307 groups (sssd-1.2)

Jakub Hrozek jhrozek at redhat.com
Wed Oct 6 11:18:35 UTC 2010


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/05/2010 11:12 PM, Simo Sorce wrote:
> On Tue, 05 Oct 2010 19:11:02 +0200
> Jakub Hrozek <jhrozek at redhat.com> wrote:
> 
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> [PATCH 1/3] sysdb interface for adding expired user entries
>> These are added instead of looking up the members.
> 
> I am not sure I like adding a uid and gid number.
> I understand it makes it simpler as you can reuse the existing sysdb
> interface but it makes me really nervous, I think a fake user shouldn't
> have any attribute at all including gidnumber and uidnumber so that it
> is clear it is a fake user.
> 
> In any case, should we decide having uidnumber=0, gidnumber=0 is an
> acceptable compromise, these shouldn't be exposed as arguments in the
> sysdb interface. We should just use internal values store into a macro
> like FAKE_UID_NUMBER and FAKE_GID_NUMBER.
> 


As explained in the initgroups thread, it shouldn't matter as we always
treat 0 as the "default" value. But I agree that a clear sysdb interface
is very important, too, so I have changed the _send() call to only
accept username and not save the UID, GID and additional attrs at all.

> Also if you always return ok in cleanup_users_logged_in() when uid = 0
> then it means you will never clean up these entries. Am I reading it
> right? If so that would be a problem. Fake users can never be logged in
> so you should probably return ENOENT instead ...

Yes, in the version you reviewed they would never get cleaned. The
reason was that the fake entries did not have any SYSDB_LAST_LOGIN
attribute so the cleanup code always marks them as expired (the sysdb
filter in the cleanup code is (SYSDB_CACHE_EXPIRE<time(NULL) && no
SYSDB_LAST_LOGIN attribute). But on another thought, I don't think that
adding SYSDB_LAST_LOGIN attribute should cause any harm on the fake
entry so I added it, too.

So in the attached version, the cleanup_users_logged_in() report the
user entries as they are never logged in (ret == EOK) and their cleanup
is controlled by the account_cache_expiration parameter, same as regular
user entries.

> 
>> [PATCH 2/3] Shortcut for save_group() to accept sysdb DNs as member
>> attributes
>>
>> This patch is a backport of original written by Ralf Haferkamp.
> 
> ack.
> 
>> [PATCH 3/3] Add fake users during saving of RFC2307 group
>> Instead of looking up the user entries, we just add dummy expired
>> users that are refreshed when they are requested with
>> getpwnam/getpwuid
> 
> This looks reasonable, although a quite complex patch (due too async
> sysdb unfortunately :-/).
> 

Unfortunately yes. I plan to give them another round of testing today to
check for any issues but I wanted to get the patches on list to speed
things up and allow Stephen to work on top of the patches.

> One thing I can't see though. At the end of
> sdap_process_missing_member_2307_done() you save the missing group DN
> in the state->sysdb_dns[] array.
> But I don't see where you use these additional names you are saving
> here... I may be missing some relationship, can you shed some light ?
> 

The sysdb_dns[] array points to the member attribute of the group. So
when a fake user is added, its sysdb DN is added to the member attribute
of the group at the same time. Using the second patch, the group is then
saved into sysdb pointing at the cached users with no additional lookup.

This was actually introduced by Ralf in his RFC2307bis patch for master
but I admit that it took me some time to understand this, too. Maybe
this part of code requires a comment or renaming the sysdb_dns[] variable?

> Simo.
> 

Thank you for the review. New patches are attached.

	Jakub
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkysWwoACgkQHsardTLnvCUVqwCfQXS0K1AMhWtvVNJ68DuFOvBb
/coAnReSOxLYM26MA8THo0dnsrLC/xg5
=Z3+l
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sysdb-interface-for-adding-expired-user-entries.patch
Type: text/x-patch
Size: 9632 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101006/b4cb94a8/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Shortcut-for-save_group-to-accept-sysdb-DNs-as-membe.patch
Type: text/x-patch
Size: 4839 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101006/b4cb94a8/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-fake-users-during-saving-of-RFC2307-group.patch
Type: text/x-patch
Size: 22306 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101006/b4cb94a8/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sysdb-interface-for-adding-expired-user-entries.patch.sig
Type: application/pgp-signature
Size: 72 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101006/b4cb94a8/attachment.sig>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Shortcut-for-save_group-to-accept-sysdb-DNs-as-membe.patch.sig
Type: application/pgp-signature
Size: 72 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101006/b4cb94a8/attachment-0001.sig>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-fake-users-during-saving-of-RFC2307-group.patch.sig
Type: application/pgp-signature
Size: 72 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20101006/b4cb94a8/attachment-0002.sig>


More information about the sssd-devel mailing list