[Freeipa-devel] [PATCH] 18 Add external domain extop DS plugin

[Martin Kosek]

On 06/28/2012 12:19 PM, Sumit Bose wrote:
> On Thu, Jun 28, 2012 at 09:52:14AM +0200, Martin Kosek wrote:
>> On 06/27/2012 06:38 PM, Alexander Bokovoy wrote:
>>> On Wed, 27 Jun 2012, Sumit Bose wrote:
>>>> On Wed, Jun 13, 2012 at 12:37:49PM +0200, Sumit Bose wrote:
>>>>> On Wed, Jun 13, 2012 at 12:26:43PM +0200, Sumit Bose wrote:
>>>>>> On Mon, Jun 11, 2012 at 05:46:17PM +0300, Alexander Bokovoy wrote:
>>>>>>> On Thu, 07 Jun 2012, Sumit Bose wrote:
>>>>>>>> On Fri, Mar 23, 2012 at 01:52:34PM +0100, Sumit Bose wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> these two patches introduce a new extended operation to the IPA server
>>>>>>>>> which can be used by clients in the IPA domain to obtain information
>>>>>>>>> about users and groups from trusted domains. Currently this exop is used
>>>>>>>>> by the sssd sub-domain patch to map user names from a trusted AD domain
>>>>>>>>> to a SID and back. There is also some code for other kind of requests
>>>>>>>>> which might become useful in future, e.g. with trusted IPA domain.
>>>>>>>>>
>>>>>>>>> I added some unit test and added check for the check unit test framework
>>>>>>>>> for C (http://check.sourceforge.net/) which is used by sssd as well. I
>>>>>>>>> modified the spec file that the test is run during the build of the
>>>>>>>>> packages. I hope this is ok.
>>>>>>>>>
>>>>>>>>> The patches depend on the idmap library patch which was ACKed recently
>>>>>>>>> on sssd-devel and as mentioned before the sub-domain patches on
>>>>>>>>> sssd-devel can only be fully tested with an IPA server which has these
>>>>>>>>> patches applied.
>>>>>>>>>
>>>>>>>>> Since Alexander is currently rewriting parts of the ipa-adtrust-install
>>>>>>>>> utility I stand back from adding activation code for the exop to
>>>>>>>>> ipa-adtrust-install and will send a patch when Alexander's changes are
>>>>>>>>> available. So currently extdom-extop-conf.ldif has to be loaded manually
>>>>>>>>> after replacing $SUFFIX to activate the new exop.
>>>>>>>>>
>>>>>>>>> bye,
>>>>>>>>> Sumit
>>>>>>>>
>>>>>>>> Please find a rebased version of the patches which work on top of
>>>>>>>> Alexander's latest series of patches. The patches now also contain the
>>>>>>>> loading of extdom-extop-conf.ldif and the activation of winbind.
>>>>>>> Thanks for the rebase.
>>>>>>>
>>>>>>> Few comments.
>>>>>>>
>>>>>>> 1.The extdom plugin should support IDMAP_BOTH. We do provide user private
>>>>>>> groups so in our case it should be viewed as preferred output. Thus you
>>>>>>> would need to add new response type to cover this case.
>>>>>>
>>>>>> Currently the plugin only uses winbind to map SIDs to names and back and
>>>>>> in the returned user data the user private groups are already respected
>>>>>> by setting the GID to the UID. On the client side sssd handles the
>>>>>> trusted domains a mpg (magic private group) domains.
>>>>>>
>>>>>>>
>>>>>>> 2. I have tried to look at the plugin description from point of view of
>>>>>>> a system administrator and I failed to understand what it does:
>>>>>>>> +#define IPA_EXTDOM_PLUGIN_NAME   "ipa-extdom-extop"
>>>>>>>> +#define IPA_EXTDOM_FEATURE_DESC  "IPA EXTDOM ID mapper"
>>>>>>>> +#define IPA_EXTDOM_PLUGIN_DESC   "IPA EXTDOM ID mapper Extended
>>>>> Operation plugin"
>>>>>>>
>>>>>>> In the ipa-extdom-extop-conf.ldif you have following description:
>>>>>>>> +nsslapd-plugindescription: Support resolving IDs in trusted domains to
>>>>> names and back
>>>>>>> Probably it is better to reuse the same description in
>>>>> IPA_EXTDOM_PLUGIN_DESC?
>>>>>>>
>>>>>>> This is a minor point but EXTDOM itself is vague. Maybe we should be
>>>>> more clear
>>>>>>> and call it 'IPA trusted domain ID mapper' as it really limits itself to
>>>>>>> only trusted domains? We don't dispatch winbind request if the domain is
>>>>>>> not found in our list of trusted domains.
>>>>>>
>>>>>> I have updated the descriptions. I prefer the EXTDOM prefix because
>>>>>> there might be future use cases where we might want to get some data
>>>>>> from other domains without trust. But I'm happy to change it if you like
>>>>>> a different prefix better.
>>>>>>
>>>>>>>
>>>>>>> 3. Could you please define the oid in ipa_extdom.h so that it could be
>>>>>>> useful for client code as well?
>>>>>>>> +#define EXOP_EXTDOM_OID "2.16.840.1.113730.3.8.10.4"
>>>>>>
>>>>>> done
>>>>>>
>>>>>> New version attached.
>>>>>
>>>>> ah. sorry, forgot to squash in some changes.
>>>>>
>>>>> Additionally I moved the binary to the freeipa-server-trust-ad package
>>>>> to avoid additional dependencies in the freeipa-server package.
>>>>>
>>>>> bye,
>>>>> Sumit
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 4. Do we have 'check' tool in RHEL6?
>>>>>>
>>>>>> yes, current version is check-0.9.8-1.1.el6
>>>>>>
>>>>>> Thank you for the review.
>>>>>>
>>>>>> bye,
>>>>>> Sumit
>>>>>>> --
>>>>>>> / Alexander Bokovoy
>>>>
>>>> rebased version with some changes by Alexander attached.
>>> ACK from my side. Works in tests I've run.
>>
>> Patch 17 pushed to master.
>>
>> Patch 18 does not apply. I also have one question related to this patch:
> 
> a rebased version is attached.
> 
>>
>> We added a winbind service to ADTRUSTInstance which is now being configured as
>> a part of ipa-adtrust-install. To make this cleaner, we may want to write
>> winbind's own service.Service class which would do the necessary configuration
>> and could be also better expanded in the future.
> 
> Currently none of the configuration steps are done exclusively for
> winbind, e.g. winbind will use the same credential as the smbd to access
> the directory server. I would agree to create an class for winbind if it
> turns out that we have to add special winbind options, but for now we
> only need to start the winbind process.

Ok, lets keep current setup and expand when needed. I also did few installation
tests with your patch, everything worked fine.

So ACK #2, pushed to master.

Martin