On Thu, Jun 06, 2013 at 06:48:43PM +0200, Jakub Hrozek wrote:
> On Wed, Jun 05, 2013 at 04:12:15PM +0200, Sumit Bose wrote:
> > 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.
>
> OK, that makes sense.
>
> >
> > >
> > > 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.
> >
>
> Thanks for the patch, it works well but it sounds like you didn't like
> the change, is there any reason to prefer other methods than PAC? I was
> under the impression that PAC was preferred and more accurate.
>
> Alternative would be to document the need to start the PAC responder,
> but I just wasn't aware of any reason not to use the PAC.
>
> > >
> > > 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.
>
> OK, I agree then.
>
> >
> > >
> > > 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.
>
> Then I think sysdb_store_user would have handled the case on its own.
> But it's not really an issue, sysdb_store_user would just call delete
> internally, which is what you do in the code as well.
>
> >
> > >
> > > 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.
>
> Can we just lower the DEBUG message severity in the krb5_child from
> OP_FAILURE to MINOR_FAILURE since failing to contact the PAC responder
> is not fatal?
>
> But in general these patches work for me. I tested both password
> authentication and GSSAPI (on a different machine than the initial test,
> I never figured out why that one wasn't working). Logins work fine,
> changes in group membership are propagated, the group memberships are
> read correctly.
>
> ACK to all patches, just please rebase patch #2 on the current master
> again, it doesn't apply cleanly and if you agree lower the DEBUG message
> in patch #4.
I rebased the patches and lowered the debug level. There is no real
reason against using the PAC so please commit patch 5 as well.
New versions attached.