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

[Martin Kosek]

On 06/28/2012 04:50 PM, Petr Viktorin wrote:
> 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!
> 

Pushed to master.

Martin