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

[Petr Viktorin]

On 06/28/2012 02:58 PM, Alexander Bokovoy wrote:
> On Thu, 28 Jun 2012, Petr Viktorin wrote:
>> 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.
> Fixed these too. New patch attached.
>
> Thanks for the thorough review!
>

ACK for the Python part, please push.
Thank you for the patch!

-- 
Petr³