[SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers

Jakub Hrozek jhrozek at redhat.com
Mon Jul 6 11:35:32 UTC 2015


On Mon, Jul 06, 2015 at 11:52:45AM +0200, Jakub Hrozek wrote:
> On Fri, Jul 03, 2015 at 08:29:30PM +0200, Sumit Bose wrote:
> > On Fri, Jul 03, 2015 at 11:59:53AM +0200, Jakub Hrozek wrote:
> > > On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote:
> > > > Hi,
> > > > 
> > > > the attached patches fix https://fedorahosted.org/sssd/ticket/2701
> > > > 
> > > > The first patch just adds a common function instead of copying the same
> > > > pattern again to the new test.
> > > > 
> > > > The second adds a new request krb5_auth_queue_send() that wraps
> > > > krb5_auth_send() and also uses the Kerberos authentication queue. I hope
> > > > the unit tests cover a lot of use-cases, if not, please suggest more!
> > > > 
> > > > btw I was thinking that the chaining might not always be necessary if
> > > > the ccache is of type MEMORY and I hope that the serializaton wouldn't
> > > > be perceived as performance regression for users. Shall we say that
> > > > Pavel's cached auth patches are a more systematic solution that doesn't
> > > > rely on properties of the ccache type in that case?
> > > 
> > > I'm sorry, but CI fails on Debian because of wrong linking with
> > > libraries. I'm already testing a fix. Review of the rest is appreciated
> > > :-)
> > 
> > ACK to the first patch.
> > 
> > The second patch has a trailing whitespace
> > 
> > > +}
> > > +
> > > +static void test_krb5_wait_mock(struct test_krb5_wait_queue *test_ctx, 
> >                                                                          ^
> > > +                                const char *username,
> > > +                                time_t us_delay,
> > > +                                int ret,
> > > +                                int pam_status,
> > > +                                int dp_err)
> > 
> > Otherwise the patch looks good and works as expected. But since you mentioned
> > that it needs a respin for the Debian issue I would like to ask to add
> > changes similar to the following:
> > 
> > diff --git a/src/providers/krb5/krb5_wait_queue.c b/src/providers/krb5/krb5_wait_queue.c
> > index d0cc0c1..1262096 100644
> > --- a/src/providers/krb5/krb5_wait_queue.c
> > +++ b/src/providers/krb5/krb5_wait_queue.c
> > @@ -271,17 +271,17 @@ struct tevent_req *krb5_auth_queue_send(TALLOC_CTX *mem_ctx,
> >      ret = add_to_wait_queue(be_ctx, req, pd, krb5_ctx);
> >      if (ret == EOK) {
> >          DEBUG(SSSDBG_TRACE_LIBS,
> > -              "Request successfully added to wait queue "
> > -              "of user [%s].\n", pd->user);
> > +              "Request [%p] successfully added to wait queue "
> > +              "of user [%s].\n", req, pd->user);
> >          ret = EOK;
> >          goto immediate;
> >      } else if (ret == ENOENT) {
> >          DEBUG(SSSDBG_TRACE_LIBS, "Wait queue of user [%s] is empty, "
> > -              "running request immediately.\n", pd->user);
> > +              "running request [%p] immediately.\n", pd->user, req);
> >      } else {
> >          DEBUG(SSSDBG_MINOR_FAILURE,
> >                "Failed to add request to wait queue of user [%s], "
> > -              "running request immediately.\n", pd->user);
> > +              "running request [%p] immediately.\n", pd->user, req);
> >      }
> >  
> >      subreq = krb5_auth_send(req, ev, be_ctx, pd, krb5_ctx);
> > @@ -322,7 +322,7 @@ static void krb5_auth_queue_done(struct tevent_req *subreq)
> >          return;
> >      }
> >  
> > -    DEBUG(SSSDBG_TRACE_INTERNAL, "krb5_auth_queue done\n");
> > +    DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n", req);
> >      tevent_req_done(req);
> >  }
> >  
> > @@ -346,6 +346,7 @@ static void krb5_auth_queue_finish(struct tevent_req *req,
> >      if (ret != EOK) {
> >          tevent_req_error(req, ret);
> >      } else {
> > +        DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n", req);
> >          tevent_req_done(req);
> >      }
> >  }
> > 
> > 
> > I was a bit irritated when I verified the by following the logs that every
> > entry to the wait queue was logged but only the exit of the immediate run was
> > found in the logs. This is basically the last change.
> > 
> > The other changes add the address of the request pointer to the log
> > message to make it easier to find matching enter and exit entries. I
> > hope this is in agreement with the recent discussion about improving log
> > messages. 
> > 
> > Finally the log levels are aligned.
> > 
> > Feel free to drop any changes you don't like but I would appreciate it
> > if you can at least add a don message in krb5_auth_queue_finish().
> > 
> > bye,
> > Sumit
> 
> Thank you for the review, I squashed in your changes in full. It's
> really important to keep DEBUG messages helpful.
> 
> I couldn't see the whitespace issue in the patch, maybe I fixed it in
> parallel.
> 
> Attached are new patches, CI is running so far, but I've done the same
> change as you showed me on IRC, so I'm confident it would help.

CI: http://sssd-ci.duckdns.org/logs/job/18/59/summary.html



More information about the sssd-devel mailing list