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

Sumit Bose sbose at redhat.com
Mon Jul 6 12:05:04 UTC 2015


On Mon, Jul 06, 2015 at 01:35:32PM +0200, Jakub Hrozek wrote:
> 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.

yes, it's gone now.

> > 
> > 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

CI passes for me as well
http://sssd-ci.duckdns.org/logs/job/18/60/summary.html.

ACK.

bye,
Sumit

> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list