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

[Sumit Bose]

On Thu, Jun 28, 2012 at 01:51:28PM +0200, Martin Kosek wrote:
> 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).

Thank you.

bye,
Sumit
> 
> Martin