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

Jakub Hrozek jhrozek at redhat.com
Thu Jun 6 22:11:31 UTC 2013


On Thu, Jun 06, 2013 at 08:59:56PM +0200, Sumit Bose wrote:
> 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.

Ack to all patches, pushed to master.



More information about the sssd-devel mailing list