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

[Martin Kosek]

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:

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.

Martin