On Tue, Jun 04, 2013 at 05:44:28PM +0200, Jakub Hrozek wrote:
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.
I think the man page explains the usage of the subdomain provider quite
well "The sub-domain provider collects domain SID and ID ranges of the
domain the client is joined to and of remote trusted domains from the
local domain controller.". Since it is important for the PAC responder
that e.g. the domain SID is available I think it should be mentioned in
the man page to make clear that it won't work properly e.g. with a plain
LDAP provider.
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.
I was a bit reluctant to add this, because it is not as important as
for the IPA provider, where there is no other way to determine the
group-memberships of trusted users. I've added patch 5 for this.
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 }
fixed.
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?
The SID should be unique and if multiple objects are returned it
indicates some issues in the cache which should be fixed. I think it is
valid to return a real error in this case and stop processing.
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?
no, to support name changes.
I still haven't read pacsrv_utils.c too carefully yet, so I might have
more comments later. But perhaps this are useful, too.
Thanks for the review.
We also discussed some issue Jakub were seeing while testing on irc. As
a result I added two more patches. Patch 3 is only a temporary fix. I
already have a final fix which depends on Jakub's changes to
users_get_recv() which are still under review. I will send it when these
changes are available.
Patch 4 removes a restriction in krb5_child which currently prevents
that the PAC is evaluated for password login with the AD provider.
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel