[Freeipa-devel] [PATCH] External group membership for trusted domains

[Petr Viktorin]

On 06/28/2012 02:16 PM, Alexander Bokovoy wrote:
> On Wed, 27 Jun 2012, Alexander Bokovoy wrote:
>> On Wed, 27 Jun 2012, Petr Viktorin wrote:
>>> On 06/27/2012 12:36 PM, Sumit Bose wrote:
>>>> On Wed, Jun 27, 2012 at 12:56:56PM +0300, Alexander Bokovoy wrote:
>>>>> On Mon, 25 Jun 2012, Alexander Bokovoy wrote:
>>>>>> On Mon, 25 Jun 2012, Sumit Bose wrote:
>>>>>>> Hi Alexander,
>>>>>>>
>>>>>>> On Thu, Jun 21, 2012 at 06:26:02PM +0300, Alexander Bokovoy wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> Attached is the patch to support external group membership for
>>>>>>>> trusted
>>>>>>>> domains. This is needed to get proper group membership with the
>>>>>>>> work
>>>>>>>> Sumit and Jan are doing on both IPA and SSSD sides.
>>>>>>>>
>>>>>>>> We already have ipaExternalGroup class that includes
>>>>>>>> ipaExternalMember
>>>>>>>> attribute (multivalued case-insensitive string). The group that has
>>>>>>>> ipaExternalGroup object class will have to be non-POSIX and
>>>>>>>> ipaExternalMember
>>>>>>>> attribute will contain security identifiers (SIDs) of members from
>>>>>>>> trusted domains.
>>>>>>>>
>>>>>>>> The patch takes care of three things:
>>>>>>>> 1. Extends 'ipa group-add' with --external option to add
>>>>>>>>   ipaExternalGroup object class to a new group
>>>>>>>> 2. Modifies 'ipa group-add-member' to accept --external CSV
>>>>>>>> argument
>>>>>>>>   to specify SIDs
>>>>>>>> 3. Modifies 'ipa group-del-member' to allow removing external
>>>>>>>> members.
>>>>>>>
>>>>>>> thank you for the patch, it works as expected, but I have a few
>>>>>>> comments:
>>>>>>>
>>>>>>> - there is a trailing whitespace at the end of the "This means we
>>>>>>> can't
>>>>>>> check the correctness of a trusted domain SIDs" line
>>>>>>> - when using ipa group-add-member with --external there are still
>>>>>>> prompt
>>>>>>> for [member user] and [member group], can those be suppressed?
>>>>>>> - with ipa group-mod --posix it is possible to add the posxiGroup
>>>>>>> objectclass together with a GID to the extern group object. This
>>>>>>> should result in an error and also the other way round, adding
>>>>>>> --external to Posix groups.
>>>>>> Updated patch is attached. It fixes whitespace and group-mod.
>>>>> New revision.
>>>>
>>>> Thank you. This version works well in my tests, so ACK.
>>>>
>>>> It would be nice if someone can have a short look at the changes to
>>>> baseldap.py to see if there are any unexpected side effects.
>>>>
>>>> bye,
>>>> Sumit
>>>>
>>>
>>>
>>> I'm concerned about this:
>>>
>>>     membername = entry[0].lower()
>>>     member_dn = api.Object[membertype].get_dn(membername)
>>>     if membername not in external_entries and \
>>> +      entry[0] not in external_entries and \
>>>       member_dn not in members:
>>>
>>> Do you want to do a case-insensitive compare here? In that case it
>>> would be better to do:
>>>
>>>   lowercase_external_entries = set(e.lower() for e in external_entries)
>>>   if membername not in lowercase_external_entries ...
>>>
>>> instead of comparing the lowercased entry and the entry itself to the
>>> original list.
>>> The `in` operator is also faster on a set.
>> Given that this list going to be short (~dozen members in most cases) it
>> is affordable to produce new set. I'll change it.
>>
>>> You should also update the `elif membername in external_entries`
>>> block below this one.
>>> There's a similar situation in remove_external_post_callback.
>>>
>>> Anyway, if you ran into a situation where the `entry[0] not in
>>> external_entries` check is needed, there should be a test for it.
>> Originally this callback was forcing all references to lower case before
>> comparing. This was applied both to existing and truly external
>> references. However, for external references we cannot guarantee that
>> lower case is the right one -- and, indeed, with SIDs one has to follow
>> SID format which is S-1-* so lowcasing the value is not possible as that
>> value will be used by SSSD and other sides (DCERPC requests) which don't
>> expect it to break the format.
>>
>> Thus I tried to keep the format.
>>
>> I've added several tests:
>> 1. Create group with external membership
>> 2. Attempt to convert posix group to external one
>> 3. Attempt to convert external group to posix
>> 4. Attempt to add external member to it.
>> 5. Delete external membership group to avoid disturbing other tests
>>    (group-find, etc) that depend on number of groups.
>>
>> In the #4 I'm only checking that we are getting exceptions --
>> either ValidationError or NotFound. You can't do more without setting up
>> the full trust.
>>
>> Even to do that I had to introduce new type of checkers -- checkers that
>> can be activated with a 'expected' attribute being a callable in a
>> declarative test definition in xmlrpc tests. This is an easiest way
>> to deal with multiple exceptions -- just define a lambda that tests
>> various conditions and let it be executed by the checker.
>>
>>> I think something is rotten with add_external_post_callback: it's
>>> defined as add_external_post_callback(... *keys, **options), but
>>> invariably called as add_external_post_callback(... keys, options).
>>> That existed before the patch, though, so I guess it warrants a
>>> separate ticket.
>>>
>>>
>>> I also have a few obligatory style nitpicks.
>>>
>>> For line continuation, instead of backslashes:
>>>
>>>    if membername not in external_entries and \
>>>      entry[0] not in external_entries and \
>>>      member_dn not in members:
>>>
>>> we prefer parentheses:
>>>
>>>    if (membername not in external_entries and
>>>        entry[0] not in external_entries and
>>>        member_dn not in members):
>> Don't shoot the follower, it is what was there before me. :)
>>
>> Fixed.
>>
>>> Instead of:
>>>
>>>    normalize = True
>>>    if 'external_callback_normalize' in options:
>>>        normalize = options['external_callback_normalize']
>>>
>>> you can use:
>>>
>>>    options.get('external_callback_normalize', True)
>> Fixed.
>>
>>>
>>> And in group.py:
>>>
>>> - 'memberofindirect': ['group', 'netgroup', 'role', 'hbacrule',
>>> - 'sudorule'],
>>> + 'memberofindirect': ['group', 'netgroup', 'role', 'hbacrule',
>>> 'sudorule'],
>>>
>>> Our style guide limits lines to 80 characters. Not much of IPA
>>> follows that rule currently, but I don't see a reason for a change
>>> that *only* breaks the rule.
>> I find it unreadable when a single element of a list is on the separate
>> line and also doesn't follow logical identation for its level.
>>
>> New patch is attached.
> And revised patch after review on IRC with Petr.
>

I'm definitely not a fan of adding new magic to the test suite, but we 
couldn't find a better way to check for one of two errors that didn't 
involve rewriting the Declarative tests.

So, with this patch, the 'expected' value of a test can be a callable, 
in which case it's called with two arguments (exception, output) and 
must return true for the test to pass.


There are still some failures in test_cmdline/test_cli.py, caused by the 
"external" flag added to group-add. Otherwise the patch works fine.

-- 
Petr³