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

[Martin Kosek]

On 06/28/2012 01:09 PM, Martin Kosek wrote:
> 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

There was a missing Requires for libsss_idmap on freeipa-server-trust-ad package.

Fixed and pushed as a one-liner (attached).

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-280-add-missing-libsss_idmap-requires-on-freeipa-server-.patch
Type: text/x-patch
Size: 768 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120628/e74b683c/attachment.bin>