[SSSD] [PATCH] Enhance PAC responder for AD users

Jakub Hrozek jhrozek at redhat.com
Tue Jun 4 15:44:28 UTC 2013


On Fri, May 31, 2013 at 06:20:34PM +0200, Sumit Bose wrote:
> On Fri, May 31, 2013 at 04:29:13PM +0200, Jakub Hrozek wrote:
> > On Fri, May 31, 2013 at 11:19:24AM +0200, Sumit Bose wrote:
> > > On Fri, May 31, 2013 at 10:32:34AM +0200, Lukas Slebodnik wrote:
> > > > On (22/05/13 14:54), Sumit Bose wrote:
> > > > >now with patches.
> > > > >
> > > > >On Wed, May 22, 2013 at 02:53:15PM +0200, Sumit Bose wrote:
> > > > >> Hi,
> > > > >> 
> > > > >> this patch makes sure that the PAC responder can be used with the AD
> > > > >> provider as well and so should fix
> > > > >> https://fedorahosted.org/sssd/ticket/1558. It depends on the SID mapping
> > > > >> patches.
> > > > >> 
> > > > >> It turned out that major parts of the PAC responder had to removed or
> > > > >> changed. E.g. all functions which were tested by the PAC responder unit
> > > > >> test got removed and hence the unit test is removes as well.
> > > > >> 
> > > > >> If tested the patch with IPA and AD provider. But since I certainly did
> > > > >> not cover all the corner cases (and maybe even missed some common
> > > > >> cases :-) I didn't enable the PAC responder for the AD provider by
> > > > >> default. But this can be done easily with an additional patch later.
> > > > >> 
> > > > >> bye,
> > > > >> Sumit
> > > > 
> > > > The socond patch could not be applied to current master.
> > > > 
> > > > error: patch failed: Makefile.am:135
> > > > error: Makefile.am: patch does not apply
> > > > error: patch failed: src/responder/pac/pacsrv_cmd.c:195
> > > > error: src/responder/pac/pacsrv_cmd.c: patch does not apply
> > > > error: patch failed: src/responder/pac/pacsrv_utils.c:765
> > > > error: src/responder/pac/pacsrv_utils.c: patch does not apply
> > > 
> > > Thanks for the hint, rebased versions attached.
> > > 
> > > bye,
> > > Sumit
> > 
> > Hi,
> > 
> > I still couldn't apply patch #2 cleanly with git am (not even with
> > three-way-merge). I was able to apply it manually using patch to proceed
> > with the review, though.
> 
> ah, I forgot to remove another patch in my tree which modified
> Makefile.am. New version attached.
> 
> bye,
> Sumit

My setup seems to be semi-broken at the moment so Sumit is currently
helping me test the patches. Here is a couple of observations about the
code:

Patch 0001: Ack

Patch 0002:
I think we should remove the mention about "subdomains" from the man
pages that describe the PAC responder. This patch makes the PAC
responder usable even without subdomains and the user is not likely to
know what subdomains are.

Given that this is now targeted at AD users, too, maybe we should extend
add_implicit_services() to also autostart PAC responder in case SSSD is
configured with id_provider=ad.

In pac_lookup_sids_done(), there seems to be a missing "return":
267     if (ret != EOK) {
268         talloc_free(pr_ctx);
269         pac_cmd_done(cctx, ret);
            ^^^^^^^

270     }

In the same function, if sysdb_search_object_by_sid() fails, we continue
with next object but if it succeeds but returns multiple values, we
continue. Should we continue in that case, too?

I know this has been the case even before the patches, but if the old
and new user differ, we first delete the old user and then add the new
one. Why not modify the old one instead and save time spent in the
memberof plugin? Is this to support changing ID values?

I still haven't read pacsrv_utils.c too carefully yet, so I might have
more comments later. But perhaps this are useful, too.



More information about the sssd-devel mailing list