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

Jakub Hrozek jhrozek at redhat.com
Thu Jun 6 16:48:43 UTC 2013


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.



More information about the sssd-devel mailing list