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

Jakub Hrozek jhrozek at redhat.com
Mon Jul 6 09:52:45 UTC 2015


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.
-------------- next part --------------
>From 4706bef89f951e3f9ca686b0b24297df12500c56 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Fri, 3 Jul 2015 11:15:32 +0200
Subject: [PATCH 1/2] tests: Reduce duplication with new function test_ev_done

---
 src/tests/cmocka/test_ipa_subdomains_server.c | 11 ++---------
 src/tests/cmocka/test_resolv_fake.c           |  3 +--
 src/tests/cmocka/test_responder_common.c      |  8 ++++----
 src/tests/common.h                            |  3 +++
 src/tests/common_tev.c                        |  6 ++++++
 5 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/src/tests/cmocka/test_ipa_subdomains_server.c b/src/tests/cmocka/test_ipa_subdomains_server.c
index 28e998b57489eccb5d3b0357df650f0faf3976fc..a4cb8c2b7538dc84b74e0227205b73780844b652 100644
--- a/src/tests/cmocka/test_ipa_subdomains_server.c
+++ b/src/tests/cmocka/test_ipa_subdomains_server.c
@@ -266,7 +266,6 @@ static int test_ipa_server_create_trusts_teardown(void **state)
 
 static void test_ipa_server_create_trusts_none(struct tevent_req *req);
 static void test_ipa_server_create_trusts_twoway(struct tevent_req *req);
-static void test_ipa_server_create_trusts_finish(struct trust_test_ctx *test_ctx);
 
 static void test_ipa_server_create_trusts(void **state)
 {
@@ -427,13 +426,7 @@ static void test_ipa_server_create_trusts_twoway(struct tevent_req *req)
                         DOM_REALM);
     assert_null(test_ctx->ipa_ctx->server_mode->trusts->next);
 
-    test_ipa_server_create_trusts_finish(test_ctx);
-}
-
-static void test_ipa_server_create_trusts_finish(struct trust_test_ctx *test_ctx)
-{
-    test_ctx->tctx->error = EOK;
-    test_ctx->tctx->done = true;
+    test_ev_done(test_ctx->tctx, EOK);
 }
 
 static void
@@ -669,7 +662,7 @@ static void test_ipa_server_create_trusts_oneway(struct tevent_req *req)
                         SUBDOM_REALM);
 
     assert_null(test_ctx->ipa_ctx->server_mode->trusts->next->next);
-    test_ipa_server_create_trusts_finish(test_ctx);
+    test_ev_done(test_ctx->tctx, EOK);
 }
 
 static void test_ipa_server_create_oneway_kt_exists(void **state)
diff --git a/src/tests/cmocka/test_resolv_fake.c b/src/tests/cmocka/test_resolv_fake.c
index 2c846ce4cc63cbf80b30bd15091136002ec84cb3..a1e9ee4cba9b17d506eeae7e6088de28c301364d 100644
--- a/src/tests/cmocka/test_resolv_fake.c
+++ b/src/tests/cmocka/test_resolv_fake.c
@@ -293,8 +293,7 @@ void test_resolv_fake_srv_done(struct tevent_req *req)
     assert_int_equal(ttl, 500);
 
     talloc_free(tmp_ctx);
-    test_ctx->ctx->error = EOK;
-    test_ctx->ctx->done = true;
+    test_ev_done(test_ctx->ctx, EOK);
 }
 
 void test_resolv_fake_srv(void **state)
diff --git a/src/tests/cmocka/test_responder_common.c b/src/tests/cmocka/test_responder_common.c
index 0a4d4bb49c5058b5a006912b6b3b61ab0332cc29..cb57c97fe6b0995c9ec69f969015ef500e065fad 100644
--- a/src/tests/cmocka/test_responder_common.c
+++ b/src/tests/cmocka/test_responder_common.c
@@ -161,8 +161,9 @@ void parse_inp_simple_done(struct tevent_req *req)
     char *domname = NULL;
 
     ret = sss_parse_inp_recv(req, parse_inp_ctx, &name, &domname);
-    parse_inp_ctx->tctx->done = true;
     assert_int_equal(ret, EOK);
+
+    test_ev_done(parse_inp_ctx->tctx, EOK);
     talloc_free(req);
 
     assert_string_equal(name, NAME);
@@ -239,8 +240,8 @@ void parse_inp_neg_done(struct tevent_req *req)
     char *domname = NULL;
 
     ret = sss_parse_inp_recv(req, parse_inp_ctx, &name, &domname);
-    parse_inp_ctx->tctx->done = true;
     assert_int_equal(ret, ERR_INPUT_PARSE);
+    test_ev_done(parse_inp_ctx->tctx, EOK);
     talloc_free(req);
 
     assert_null(name);
@@ -273,8 +274,7 @@ struct sss_nc_ctx {
 errno_t sss_ncache_reset_repopulate_permanent(struct resp_ctx *rctx,
                                               struct sss_nc_ctx *dummy_ncache_ptr)
 {
-    dummy_ncache_ptr->pctx->tctx->error = EOK;
-    dummy_ncache_ptr->pctx->tctx->done = true;
+    test_ev_done(dummy_ncache_ptr->pctx->tctx, EOK);
     return EOK;
 }
 
diff --git a/src/tests/common.h b/src/tests/common.h
index 714be6988aa032d935c09bc7db524545a4c9154e..0b351f5d647a8f72bdbc399c6fe02579d4b4e1be 100644
--- a/src/tests/common.h
+++ b/src/tests/common.h
@@ -112,6 +112,9 @@ errno_t test_request_recv(struct tevent_req *req);
 
 int test_ev_loop(struct sss_test_ctx *tctx);
 
+/* Mark the test as done with an error code */
+void test_ev_done(struct sss_test_ctx *tctx, errno_t ret);
+
 bool ldb_modules_path_is_set(void);
 
 DBusConnection *
diff --git a/src/tests/common_tev.c b/src/tests/common_tev.c
index 770f97d83684696498ff691dc14b45c233008fd3..808efd459ca66a52874eda2897ab9cdd084523e1 100644
--- a/src/tests/common_tev.c
+++ b/src/tests/common_tev.c
@@ -83,3 +83,9 @@ int test_ev_loop(struct sss_test_ctx *tctx)
 
     return tctx->error;
 }
+
+void test_ev_done(struct sss_test_ctx *tctx, errno_t ret)
+{
+    tctx->error = ret;
+    tctx->done = true;
+}
-- 
2.4.3

-------------- next part --------------
>From 5a520cbdf3dc14cdbf210a04074c704c67a77fb6 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 30 Jun 2015 19:40:46 +0200
Subject: [PATCH 2/2] KRB5: Add and use krb5_auth_queue_send to queue requests
 by default

Resolves:
https://fedorahosted.org/sssd/ticket/2701

Previously, only the krb5 provides used to queue requests, which
resulted in concurrent authentication requests stepping on one another.

This patch queues requests by default.
---
 Makefile.am                                        |  17 +
 src/providers/ipa/ipa_auth.c                       |  12 +-
 src/providers/krb5/krb5_auth.c                     |  28 +-
 src/providers/krb5/krb5_auth.h                     |  18 +-
 .../krb5/krb5_delayed_online_authentication.c      |  11 +-
 src/providers/krb5/krb5_renew_tgt.c                |   6 +-
 src/providers/krb5/krb5_wait_queue.c               | 184 ++++++++++-
 src/tests/cmocka/test_krb5_wait_queue.c            | 365 +++++++++++++++++++++
 8 files changed, 587 insertions(+), 54 deletions(-)
 create mode 100644 src/tests/cmocka/test_krb5_wait_queue.c

diff --git a/Makefile.am b/Makefile.am
index 1b427b17f8a83e66a4c2b67dee3f7bb475028b64..b8cbc6df23ded1edb945a709b6dbe1c44eb54017 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -230,6 +230,7 @@ if HAVE_CMOCKA
         pam-srv-tests \
         test_ipa_subdom_util \
         test_ipa_subdom_server \
+        test_krb5_wait_queue \
         test_cert_utils \
         $(NULL)
 
@@ -2515,6 +2516,22 @@ test_ipa_subdom_server_LDADD = \
     libsss_test_common.la \
     $(NULL)
 
+test_krb5_wait_queue_SOURCES = \
+    src/tests/cmocka/common_mock_be.c \
+    src/tests/cmocka/test_krb5_wait_queue.c \
+    src/providers/krb5/krb5_wait_queue.c \
+    $(NULL)
+test_krb5_wait_queue_CFLAGS = \
+    $(AM_CFLAGS) \
+    $(NULL)
+test_krb5_wait_queue_LDADD = \
+    $(CMOCKA_LIBS) \
+    $(POPT_LIBS) \
+    $(DHASH_LIBS) \
+    $(SSSD_INTERNAL_LTLIBS) \
+    libsss_test_common.la \
+    $(NULL)
+
 test_cert_utils_SOURCES = \
     src/tests/cmocka/test_cert_utils.c \
     $(NULL)
diff --git a/src/providers/ipa/ipa_auth.c b/src/providers/ipa/ipa_auth.c
index 79e891b77205f75671c146af0192b764f12e13b0..b1bfa3ffe6aaae6b19ba3dd5334fd4444933ef51 100644
--- a/src/providers/ipa/ipa_auth.c
+++ b/src/providers/ipa/ipa_auth.c
@@ -224,8 +224,8 @@ void ipa_auth(struct be_req *be_req)
             goto fail;
     }
 
-    req = krb5_auth_send(state, state->ev, be_ctx, state->pd,
-                         state->ipa_auth_ctx->krb5_auth_ctx);
+    req = krb5_auth_queue_send(state, state->ev, be_ctx, state->pd,
+                               state->ipa_auth_ctx->krb5_auth_ctx);
     if (req == NULL) {
         DEBUG(SSSDBG_OP_FAILURE, "krb5_auth_send failed.\n");
         goto fail;
@@ -248,7 +248,7 @@ static void ipa_auth_handler_done(struct tevent_req *req)
     int pam_status = PAM_SYSTEM_ERR;
     int dp_err;
 
-    ret = krb5_auth_recv(req, &pam_status, &dp_err);
+    ret = krb5_auth_queue_recv(req, &pam_status, &dp_err);
     talloc_zfree(req);
     state->pd->pam_status = pam_status;
     if (ret != EOK && pam_status != PAM_CRED_ERR) {
@@ -423,8 +423,8 @@ static void ipa_auth_ldap_done(struct tevent_req *req)
     DEBUG(SSSDBG_TRACE_FUNC, "LDAP authentication succeded, "
                               "trying Kerberos authentication again.\n");
 
-    req = krb5_auth_send(state, state->ev, be_ctx, state->pd,
-                         state->ipa_auth_ctx->krb5_auth_ctx);
+    req = krb5_auth_queue_send(state, state->ev, be_ctx, state->pd,
+                               state->ipa_auth_ctx->krb5_auth_ctx);
     if (req == NULL) {
         DEBUG(SSSDBG_OP_FAILURE, "krb5_auth_send failed.\n");
         goto done;
@@ -445,7 +445,7 @@ static void ipa_auth_handler_retry_done(struct tevent_req *req)
     int pam_status;
     int dp_err;
 
-    ret = krb5_auth_recv(req, &pam_status, &dp_err);
+    ret = krb5_auth_queue_recv(req, &pam_status, &dp_err);
     talloc_zfree(req);
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE, "krb5_auth_recv request failed.\n");
diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index 812e215240ec28e2cd627da19da0059286909793..c94f2b26f8d4b8d5d3bd5d7836052c478e13aba7 100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -1139,7 +1139,6 @@ void krb5_pam_handler(struct be_req *be_req)
     struct pam_data *pd;
     struct krb5_ctx *krb5_ctx;
     int dp_err = DP_ERR_FATAL;
-    int ret;
 
     pd = talloc_get_type(be_req_get_data(be_req), struct pam_data);
     pd->pam_status = PAM_SYSTEM_ERR;
@@ -1155,22 +1154,7 @@ void krb5_pam_handler(struct be_req *be_req)
         case SSS_CMD_RENEW:
         case SSS_PAM_CHAUTHTOK_PRELIM:
         case SSS_PAM_CHAUTHTOK:
-            ret = add_to_wait_queue(be_req, pd, krb5_ctx);
-            if (ret == EOK) {
-                DEBUG(SSSDBG_TRACE_LIBS,
-                      "Request successfully added to wait queue "
-                          "of user [%s].\n", pd->user);
-                return;
-            } else if (ret == ENOENT) {
-                DEBUG(SSSDBG_TRACE_LIBS, "Wait queue of user [%s] is empty, "
-                          "running request immediately.\n", pd->user);
-            } else {
-                DEBUG(SSSDBG_TRACE_LIBS,
-                      "Failed to add request to wait queue of user [%s], "
-                          "running request immediately.\n", pd->user);
-            }
-
-            req = krb5_auth_send(be_req, be_ctx->ev, be_ctx, pd, krb5_ctx);
+            req = krb5_auth_queue_send(be_req, be_ctx->ev, be_ctx, pd, krb5_ctx);
             if (req == NULL) {
                 DEBUG(SSSDBG_CRIT_FAILURE, "krb5_auth_send failed.\n");
                 goto done;
@@ -1215,11 +1199,10 @@ void krb5_pam_handler_auth_done(struct tevent_req *req)
     int pam_status;
     int dp_err;
     struct pam_data *pd;
-    struct krb5_ctx *krb5_ctx;
 
     pd = talloc_get_type(be_req_get_data(be_req), struct pam_data);
 
-    ret = krb5_auth_recv(req, &pam_status, &dp_err);
+    ret = krb5_auth_queue_recv(req, &pam_status, &dp_err);
     talloc_zfree(req);
     if (ret) {
         pd->pam_status = PAM_SYSTEM_ERR;
@@ -1228,13 +1211,6 @@ void krb5_pam_handler_auth_done(struct tevent_req *req)
         pd->pam_status = pam_status;
     }
 
-    krb5_ctx = get_krb5_ctx(be_req);
-    if (krb5_ctx != NULL) {
-        check_wait_queue(krb5_ctx, pd->user);
-    } else {
-        DEBUG(SSSDBG_CRIT_FAILURE, "Kerberos context not available.\n");
-    }
-
     be_req_terminate(be_req, dp_err, pd->pam_status, NULL);
 }
 
diff --git a/src/providers/krb5/krb5_auth.h b/src/providers/krb5/krb5_auth.h
index a00f1d9d9be38453da11c9e64ab73fe23f67d475..205e70665531803d6374f6fef084cd24114a6a97 100644
--- a/src/providers/krb5/krb5_auth.h
+++ b/src/providers/krb5/krb5_auth.h
@@ -66,6 +66,11 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
 void krb5_pam_handler(struct be_req *be_req);
 void krb5_pam_handler_auth_done(struct tevent_req *req);
 
+/* Please use krb5_auth_send/recv *only* if you're certain there can't
+ * be concurrent logins happening. With some ccache back ends, the ccache
+ * files might clobber one another. Please use krb5_auth_queue_send()
+ * instead that queues the requests
+ */
 struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
                                   struct tevent_context *ev,
                                   struct be_ctx *be_ctx,
@@ -114,7 +119,14 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx,
 int krb5_access_recv(struct tevent_req *req, bool *access_allowed);
 
 /* krb5_wait_queue.c */
-errno_t add_to_wait_queue(struct be_req *be_req, struct pam_data *pd,
-                          struct krb5_ctx *krb5_ctx);
-void check_wait_queue(struct krb5_ctx *krb5_ctx, char *username);
+struct tevent_req *krb5_auth_queue_send(TALLOC_CTX *mem_ctx,
+                                        struct tevent_context *ev,
+                                        struct be_ctx *be_ctx,
+                                        struct pam_data *pd,
+                                        struct krb5_ctx *krb5_ctx);
+
+int krb5_auth_queue_recv(struct tevent_req *req,
+                         int *_pam_status,
+                         int *_dp_err);
+
 #endif /* __KRB5_AUTH_H__ */
diff --git a/src/providers/krb5/krb5_delayed_online_authentication.c b/src/providers/krb5/krb5_delayed_online_authentication.c
index 5f13eac8e1dbe7494423b553458f75aacab8c258..bf2ef775573ba6bad79a99ad43b5d9748516e794 100644
--- a/src/providers/krb5/krb5_delayed_online_authentication.c
+++ b/src/providers/krb5/krb5_delayed_online_authentication.c
@@ -103,8 +103,8 @@ static void authenticate_user(struct tevent_context *ev,
     }
 #endif
 
-    req = krb5_auth_send(auth_data, ev, auth_data->be_ctx, auth_data->pd,
-                         auth_data->krb5_ctx);
+    req = krb5_auth_queue_send(auth_data, ev, auth_data->be_ctx,
+                               auth_data->pd, auth_data->krb5_ctx);
     if (req == NULL) {
         DEBUG(SSSDBG_CRIT_FAILURE, "krb5_auth_send failed.\n");
         talloc_free(auth_data);
@@ -114,14 +114,15 @@ static void authenticate_user(struct tevent_context *ev,
     tevent_req_set_callback(req, authenticate_user_done, auth_data);
 }
 
-static void authenticate_user_done(struct tevent_req *req) {
+static void authenticate_user_done(struct tevent_req *req)
+{
     struct auth_data *auth_data = tevent_req_callback_data(req,
                                                            struct auth_data);
     int ret;
     int pam_status = PAM_SYSTEM_ERR;
-    int dp_err;
+    int dp_err = DP_ERR_OK;
 
-    ret = krb5_auth_recv(req, &pam_status, &dp_err);
+    ret = krb5_auth_queue_recv(req, &pam_status, &dp_err);
     talloc_free(req);
     if (ret) {
         DEBUG(SSSDBG_CRIT_FAILURE, "krb5_auth request failed.\n");
diff --git a/src/providers/krb5/krb5_renew_tgt.c b/src/providers/krb5/krb5_renew_tgt.c
index 5277c0f7691dd272a69b3024cb2c0bd7545a8fbd..498097792b65e68dc849c09501ad5b1882e4a498 100644
--- a/src/providers/krb5/krb5_renew_tgt.c
+++ b/src/providers/krb5/krb5_renew_tgt.c
@@ -66,8 +66,8 @@ static void renew_tgt(struct tevent_context *ev, struct tevent_timer *te,
                                                   struct auth_data);
     struct tevent_req *req;
 
-    req = krb5_auth_send(auth_data, ev, auth_data->be_ctx, auth_data->pd,
-                         auth_data->krb5_ctx);
+    req = krb5_auth_queue_send(auth_data, ev, auth_data->be_ctx, auth_data->pd,
+                               auth_data->krb5_ctx);
     if (req == NULL) {
         DEBUG(SSSDBG_CRIT_FAILURE, "krb5_auth_send failed.\n");
 /* Give back the pam data to the renewal item to be able to retry at the next
@@ -90,7 +90,7 @@ static void renew_tgt_done(struct tevent_req *req)
     int dp_err;
     hash_value_t value;
 
-    ret = krb5_auth_recv(req, &pam_status, &dp_err);
+    ret = krb5_auth_queue_recv(req, &pam_status, &dp_err);
     talloc_free(req);
     if (ret) {
         DEBUG(SSSDBG_CRIT_FAILURE, "krb5_auth request failed.\n");
diff --git a/src/providers/krb5/krb5_wait_queue.c b/src/providers/krb5/krb5_wait_queue.c
index 10c50d81b8c8b81aab0b5f73fa86b57f708a2e54..126209620dcd22889cf7bcb1cbec1890b8891b41 100644
--- a/src/providers/krb5/krb5_wait_queue.c
+++ b/src/providers/krb5/krb5_wait_queue.c
@@ -25,6 +25,8 @@
 #include <tevent.h>
 #include <dhash.h>
 
+#include <security/pam_modules.h>
+
 #include "src/providers/krb5/krb5_auth.h"
 
 #define INIT_HASH_SIZE 5
@@ -33,28 +35,53 @@ struct queue_entry {
     struct queue_entry *prev;
     struct queue_entry *next;
 
+    struct be_ctx *be_ctx;
     struct be_req *be_req;
+    struct tevent_req *parent_req;
     struct pam_data *pd;
     struct krb5_ctx *krb5_ctx;
 };
 
+static void wait_queue_auth_done(struct tevent_req *req);
+
+static void krb5_auth_queue_finish(struct tevent_req *req, errno_t ret,
+                                   int pam_status, int dp_err);
+
 static void wait_queue_auth(struct tevent_context *ev, struct tevent_timer *te,
-                             struct timeval current_time, void *private_data)
+                            struct timeval current_time, void *private_data)
 {
     struct queue_entry *qe = talloc_get_type(private_data, struct queue_entry);
-    struct be_ctx *be_ctx = be_req_get_be_ctx(qe->be_req);
     struct tevent_req *req;
 
-    req = krb5_auth_send(qe->be_req, be_ctx->ev, be_ctx, qe->pd, qe->krb5_ctx);
+    req = krb5_auth_send(qe->parent_req, qe->be_ctx->ev,
+                         qe->be_ctx, qe->pd, qe->krb5_ctx);
     if (req == NULL) {
         DEBUG(SSSDBG_CRIT_FAILURE, "krb5_auth_send failed.\n");
     } else {
-        tevent_req_set_callback(req, krb5_pam_handler_auth_done, qe->be_req);
+        tevent_req_set_callback(req, wait_queue_auth_done,
+                                qe->parent_req);
     }
 
     talloc_zfree(qe);
 }
 
+static void wait_queue_auth_done(struct tevent_req *req)
+{
+    struct tevent_req *parent_req = \
+                tevent_req_callback_data(req, struct tevent_req);
+    int pam_status;
+    int dp_err;
+    errno_t ret;
+
+    ret = krb5_auth_recv(req, &pam_status, &dp_err);
+    talloc_zfree(req);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, "krb5_auth_recv failed: %d\n", ret);
+    }
+
+    krb5_auth_queue_finish(parent_req, ret, pam_status, dp_err);
+}
+
 static void wait_queue_del_cb(hash_entry_t *entry, hash_destroy_enum type,
                               void *pvt)
 {
@@ -70,8 +97,10 @@ static void wait_queue_del_cb(hash_entry_t *entry, hash_destroy_enum type,
           "Unexpected value type [%d].\n", entry->value.type);
 }
 
-errno_t add_to_wait_queue(struct be_req *be_req, struct pam_data *pd,
-                          struct krb5_ctx *krb5_ctx)
+static errno_t add_to_wait_queue(struct be_ctx *be_ctx,
+                                 struct tevent_req *parent_req,
+                                 struct pam_data *pd,
+                                 struct krb5_ctx *krb5_ctx)
 {
     int ret;
     hash_key_t key;
@@ -108,7 +137,8 @@ errno_t add_to_wait_queue(struct be_req *be_req, struct pam_data *pd,
                 return ENOMEM;
             }
 
-            queue_entry->be_req = be_req;
+            queue_entry->be_ctx = be_ctx;
+            queue_entry->parent_req = parent_req;
             queue_entry->pd = pd;
             queue_entry->krb5_ctx = krb5_ctx;
 
@@ -144,7 +174,7 @@ errno_t add_to_wait_queue(struct be_req *be_req, struct pam_data *pd,
     }
 }
 
-void check_wait_queue(struct krb5_ctx *krb5_ctx, char *username)
+static void check_wait_queue(struct krb5_ctx *krb5_ctx, char *username)
 {
     int ret;
     hash_key_t key;
@@ -152,7 +182,6 @@ void check_wait_queue(struct krb5_ctx *krb5_ctx, char *username)
     struct queue_entry *head;
     struct queue_entry *queue_entry;
     struct tevent_timer *te;
-    struct be_ctx *be_ctx;
 
     if (krb5_ctx->wait_queue_hash == NULL) {
         DEBUG(SSSDBG_CRIT_FAILURE, "No wait queue available.\n");
@@ -181,8 +210,7 @@ void check_wait_queue(struct krb5_ctx *krb5_ctx, char *username)
 
                 DLIST_REMOVE(head, queue_entry);
 
-                be_ctx = be_req_get_be_ctx(queue_entry->be_req);
-                te = tevent_add_timer(be_ctx->ev, krb5_ctx,
+                te = tevent_add_timer(queue_entry->be_ctx->ev, krb5_ctx,
                                       tevent_timeval_current(), wait_queue_auth,
                                       queue_entry);
                 if (te == NULL) {
@@ -211,3 +239,137 @@ void check_wait_queue(struct krb5_ctx *krb5_ctx, char *username)
     return;
 }
 
+struct krb5_auth_queue_state {
+    struct krb5_ctx *krb5_ctx;
+    struct pam_data *pd;
+
+    int pam_status;
+    int dp_err;
+};
+
+static void krb5_auth_queue_done(struct tevent_req *subreq);
+
+struct tevent_req *krb5_auth_queue_send(TALLOC_CTX *mem_ctx,
+                                        struct tevent_context *ev,
+                                        struct be_ctx *be_ctx,
+                                        struct pam_data *pd,
+                                        struct krb5_ctx *krb5_ctx)
+{
+    errno_t ret;
+    struct tevent_req *req;
+    struct tevent_req *subreq;
+    struct krb5_auth_queue_state *state;
+
+    req = tevent_req_create(mem_ctx, &state, struct krb5_auth_queue_state);
+    if (req == NULL) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create failed.\n");
+        return NULL;
+    }
+    state->krb5_ctx = krb5_ctx;
+    state->pd = pd;
+
+    ret = add_to_wait_queue(be_ctx, req, pd, krb5_ctx);
+    if (ret == EOK) {
+        DEBUG(SSSDBG_TRACE_LIBS,
+              "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 [%p] immediately.\n", pd->user, req);
+    } else {
+        DEBUG(SSSDBG_MINOR_FAILURE,
+              "Failed to add request to wait queue of user [%s], "
+              "running request [%p] immediately.\n", pd->user, req);
+    }
+
+    subreq = krb5_auth_send(req, ev, be_ctx, pd, krb5_ctx);
+    if (req == NULL) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "krb5_auth_send failed.\n");
+        ret = ENOMEM;
+        goto immediate;
+    }
+
+    tevent_req_set_callback(subreq, krb5_auth_queue_done, req);
+
+    ret = EOK;
+
+immediate:
+    if (ret != EOK) {
+        tevent_req_error(req, ret);
+        tevent_req_post(req, ev);
+    }
+    return req;
+}
+
+static void krb5_auth_queue_done(struct tevent_req *subreq)
+{
+    struct tevent_req *req = \
+                tevent_req_callback_data(subreq, struct tevent_req);
+    struct krb5_auth_queue_state *state = \
+                tevent_req_data(req, struct krb5_auth_queue_state);
+    errno_t ret;
+
+    ret = krb5_auth_recv(subreq, &state->pam_status, &state->dp_err);
+    talloc_zfree(subreq);
+
+    check_wait_queue(state->krb5_ctx, state->pd->user);
+
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, "krb5_auth_recv failed with: %d\n", ret);
+        tevent_req_error(req, ret);
+        return;
+    }
+
+    DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n", req);
+    tevent_req_done(req);
+}
+
+/* This is a violation of the tevent_req style. Ideally, the wait queue would
+ * be rewritten to the tevent_req style in the future, expose per-request recv
+ * and not hide the request underneath. But this function allows us to expose
+ * a tevent_req API for users of this module
+ */
+static void krb5_auth_queue_finish(struct tevent_req *req,
+                                   errno_t ret,
+                                   int pam_status,
+                                   int dp_err)
+{
+    struct krb5_auth_queue_state *state = \
+                tevent_req_data(req, struct krb5_auth_queue_state);
+
+    check_wait_queue(state->krb5_ctx, state->pd->user);
+
+    state->pam_status = pam_status;
+    state->dp_err = dp_err;
+    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);
+    }
+}
+
+int krb5_auth_queue_recv(struct tevent_req *req,
+                         int *_pam_status,
+                         int *_dp_err)
+{
+    struct krb5_auth_queue_state *state = \
+                tevent_req_data(req, struct krb5_auth_queue_state);
+
+    /* Returning values even on failure is not typical, but IPA password migration
+     * relies on receiving PAM_CRED_ERR even if the request fails..
+     */
+    if (_pam_status) {
+        *_pam_status = state->pam_status;
+    }
+
+    if (_dp_err) {
+        *_dp_err = state->pam_status;
+    }
+
+    TEVENT_REQ_RETURN_ON_ERROR(req);
+
+    return EOK;
+}
diff --git a/src/tests/cmocka/test_krb5_wait_queue.c b/src/tests/cmocka/test_krb5_wait_queue.c
new file mode 100644
index 0000000000000000000000000000000000000000..4add979156b88440959abb2a45f40d2abb2ca14b
--- /dev/null
+++ b/src/tests/cmocka/test_krb5_wait_queue.c
@@ -0,0 +1,365 @@
+/*
+    Copyright (C) 2015 Red Hat
+
+    SSSD tests: Kerberos wait queue tests
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 3 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <talloc.h>
+#include <tevent.h>
+#include <errno.h>
+#include <popt.h>
+#include <stdlib.h>
+#include <security/pam_modules.h>
+
+#include "util/util.h"
+#include "providers/krb5/krb5_common.h"
+#include "providers/krb5/krb5_auth.h"
+#include "tests/cmocka/common_mock.h"
+#include "tests/cmocka/common_mock_be.h"
+
+struct krb5_mocked_auth_state {
+    const char *user;
+    time_t us_delay;
+    int ret;
+    int pam_status;
+    int dp_err;
+};
+
+static void krb5_mocked_auth_done(struct tevent_context *ev,
+                                  struct tevent_timer *tt,
+                                  struct timeval tv,
+                                  void *pvt);
+
+struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
+                                  struct tevent_context *ev,
+                                  struct be_ctx *be_ctx,
+                                  struct pam_data *pd,
+                                  struct krb5_ctx *krb5_ctx)
+{
+    struct tevent_req *req;
+    struct krb5_mocked_auth_state *state;
+    struct tevent_timer *tt;
+    struct timeval tv;
+
+    req = tevent_req_create(mem_ctx, &state, struct krb5_mocked_auth_state);
+    if (req == NULL) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create failed.\n");
+        return NULL;
+    }
+
+    state->user = sss_mock_ptr_type(const char *);
+    state->us_delay = sss_mock_type(time_t);
+    state->ret = sss_mock_type(int);
+    state->pam_status = sss_mock_type(int);
+    state->dp_err = sss_mock_type(int);
+
+    tv = tevent_timeval_current_ofs(0, state->us_delay);
+
+    tt = tevent_add_timer(ev, req, tv, krb5_mocked_auth_done, req);
+    if (tt == NULL) {
+        return NULL;
+    }
+
+    return req;
+}
+
+static void krb5_mocked_auth_done(struct tevent_context *ev,
+                                  struct tevent_timer *tt,
+                                  struct timeval tv,
+                                  void *pvt)
+{
+    struct tevent_req *req;
+    struct krb5_mocked_auth_state *state;
+
+    req = talloc_get_type(pvt, struct tevent_req);
+    state = tevent_req_data(req, struct krb5_mocked_auth_state);
+
+    DEBUG(SSSDBG_TRACE_LIBS, "Finished auth request of %s\n", state->user);
+
+    if (state->ret == 0) {
+        tevent_req_done(req);
+    } else {
+        tevent_req_error(req, state->ret);
+    }
+}
+
+int krb5_auth_recv(struct tevent_req *req,
+                   int *_pam_status,
+                   int *_dp_err)
+{
+    struct krb5_mocked_auth_state *state;
+
+    state = tevent_req_data(req, struct krb5_mocked_auth_state);
+
+    if (_pam_status != NULL) {
+        *_pam_status = state->pam_status;
+    }
+
+    if (_dp_err != NULL) {
+        *_dp_err = state->dp_err;
+    }
+
+    TEVENT_REQ_RETURN_ON_ERROR(req);
+    return EOK;
+}
+
+struct test_krb5_wait_queue {
+    struct sss_test_ctx *tctx;
+    int num_auths;
+    int num_finished_auths;
+
+    struct be_ctx *be_ctx;
+    struct pam_data *pd;
+    struct krb5_ctx *krb5_ctx;
+};
+
+static int test_krb5_wait_queue_setup(void **state)
+{
+    struct test_krb5_wait_queue *test_ctx;
+
+    test_ctx = talloc_zero(global_talloc_context,
+                           struct test_krb5_wait_queue);
+    assert_non_null(test_ctx);
+
+    test_ctx->tctx = create_ev_test_ctx(test_ctx);
+    assert_non_null(test_ctx);
+
+    test_ctx->be_ctx = mock_be_ctx(test_ctx, test_ctx->tctx);
+    assert_non_null(test_ctx->be_ctx);
+
+    test_ctx->pd = talloc_zero(test_ctx, struct pam_data);
+    assert_non_null(test_ctx->pd);
+
+    test_ctx->krb5_ctx = talloc_zero(test_ctx, struct krb5_ctx);
+    assert_non_null(test_ctx->krb5_ctx);
+
+    *state = test_ctx;
+    return 0;
+}
+
+static int test_krb5_wait_queue_teardown(void **state)
+{
+    struct test_krb5_wait_queue *test_ctx =
+        talloc_get_type(*state, struct test_krb5_wait_queue);
+
+    talloc_free(test_ctx);
+    return 0;
+}
+
+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)
+{
+    test_ctx->pd->user = discard_const(username);
+
+    will_return(krb5_auth_send, username);
+    will_return(krb5_auth_send, us_delay);
+    will_return(krb5_auth_send, ret);
+    will_return(krb5_auth_send, pam_status);
+    will_return(krb5_auth_send, dp_err);
+}
+
+static void test_krb5_wait_mock_success(struct test_krb5_wait_queue *test_ctx,
+                                        const char *username)
+{
+    return test_krb5_wait_mock(test_ctx, username, 200, 0, 0, 0);
+}
+
+static void test_krb5_wait_queue_single_done(struct tevent_req *req);
+
+static void test_krb5_wait_queue_single(void **state)
+{
+    errno_t ret;
+    struct tevent_req *req;
+    struct test_krb5_wait_queue *test_ctx =
+        talloc_get_type(*state, struct test_krb5_wait_queue);
+
+    test_krb5_wait_mock_success(test_ctx, "krb5_user");
+
+    req = krb5_auth_queue_send(test_ctx,
+                               test_ctx->tctx->ev,
+                               test_ctx->be_ctx,
+                               test_ctx->pd,
+                               test_ctx->krb5_ctx);
+    assert_non_null(req);
+    tevent_req_set_callback(req, test_krb5_wait_queue_single_done, test_ctx);
+
+    ret = test_ev_loop(test_ctx->tctx);
+    assert_int_equal(ret, EOK);
+}
+
+static void test_krb5_wait_queue_single_done(struct tevent_req *req)
+{
+    struct test_krb5_wait_queue *test_ctx = \
+        tevent_req_callback_data(req, struct test_krb5_wait_queue);
+    errno_t ret;
+    int pam_status;
+    int dp_err;
+
+    ret = krb5_auth_queue_recv(req, &pam_status, &dp_err);
+    talloc_free(req);
+    assert_int_equal(ret, EOK);
+
+    test_ev_done(test_ctx->tctx, EOK);
+}
+
+static void test_krb5_wait_queue_multi_done(struct tevent_req *req);
+
+static void test_krb5_wait_queue_multi(void **state)
+{
+    int i;
+    errno_t ret;
+    struct tevent_req *req;
+    struct test_krb5_wait_queue *test_ctx =
+        talloc_get_type(*state, struct test_krb5_wait_queue);
+
+    test_ctx->num_auths = 1000;
+
+    for (i=0; i < test_ctx->num_auths; i++) {
+        test_krb5_wait_mock_success(test_ctx, "krb5_user");
+
+        req = krb5_auth_queue_send(test_ctx,
+                                   test_ctx->tctx->ev,
+                                   test_ctx->be_ctx,
+                                   test_ctx->pd,
+                                   test_ctx->krb5_ctx);
+        assert_non_null(req);
+        tevent_req_set_callback(req, test_krb5_wait_queue_multi_done, test_ctx);
+    }
+
+    ret = test_ev_loop(test_ctx->tctx);
+    assert_int_equal(ret, EOK);
+}
+
+static void test_krb5_wait_queue_multi_done(struct tevent_req *req)
+{
+    struct test_krb5_wait_queue *test_ctx = \
+        tevent_req_callback_data(req, struct test_krb5_wait_queue);
+    errno_t ret;
+    int pam_status;
+    int dp_err;
+
+    ret = krb5_auth_queue_recv(req, &pam_status, &dp_err);
+    talloc_free(req);
+    assert_int_equal(ret, EOK);
+
+    test_ctx->num_finished_auths++;
+
+    if (test_ctx->num_finished_auths == test_ctx->num_auths) {
+        test_ev_done(test_ctx->tctx, EOK);
+    }
+}
+
+static void test_krb5_wait_queue_fail_odd_done(struct tevent_req *req);
+
+static void test_krb5_wait_queue_fail_odd(void **state)
+{
+    int i;
+    errno_t ret;
+    struct tevent_req *req;
+    struct test_krb5_wait_queue *test_ctx =
+        talloc_get_type(*state, struct test_krb5_wait_queue);
+
+    test_ctx->num_auths = 10;
+
+    for (i=0; i < test_ctx->num_auths; i++) {
+        test_krb5_wait_mock(test_ctx, "krb5_user", 0, i+1 % 2, PAM_SUCCESS, 0);
+
+        req = krb5_auth_queue_send(test_ctx,
+                                   test_ctx->tctx->ev,
+                                   test_ctx->be_ctx,
+                                   test_ctx->pd,
+                                   test_ctx->krb5_ctx);
+        assert_non_null(req);
+        tevent_req_set_callback(req, test_krb5_wait_queue_fail_odd_done, test_ctx);
+    }
+
+    ret = test_ev_loop(test_ctx->tctx);
+    assert_int_equal(ret, EOK);
+}
+
+static void test_krb5_wait_queue_fail_odd_done(struct tevent_req *req)
+{
+    struct test_krb5_wait_queue *test_ctx = \
+        tevent_req_callback_data(req, struct test_krb5_wait_queue);
+    errno_t ret;
+    int pam_status;
+    int dp_err;
+
+    ret = krb5_auth_queue_recv(req, &pam_status, &dp_err);
+    talloc_free(req);
+    assert_int_equal(ret, test_ctx->num_finished_auths+1 % 2);
+
+    test_ctx->num_finished_auths++;
+
+    if (test_ctx->num_finished_auths == test_ctx->num_auths) {
+        test_ev_done(test_ctx->tctx, EOK);
+    }
+}
+
+int main(int argc, const char *argv[])
+{
+    poptContext pc;
+    int opt;
+    struct poptOption long_options[] = {
+        POPT_AUTOHELP
+        SSSD_DEBUG_OPTS
+        POPT_TABLEEND
+    };
+
+    const struct CMUnitTest tests[] = {
+        /* Run a single auth request */
+        cmocka_unit_test_setup_teardown(test_krb5_wait_queue_single,
+                                        test_krb5_wait_queue_setup,
+                                        test_krb5_wait_queue_teardown),
+
+        /* Run multiple auth requests */
+        cmocka_unit_test_setup_teardown(test_krb5_wait_queue_multi,
+                                        test_krb5_wait_queue_setup,
+                                        test_krb5_wait_queue_teardown),
+
+        /* Make sure that all requests in queue run even if some fail */
+        cmocka_unit_test_setup_teardown(test_krb5_wait_queue_fail_odd,
+                                        test_krb5_wait_queue_setup,
+                                        test_krb5_wait_queue_teardown),
+    };
+
+    /* Set debug level to invalid value so we can deside if -d 0 was used. */
+    debug_level = SSSDBG_INVALID;
+
+    pc = poptGetContext(argv[0], argc, argv, long_options, 0);
+    while((opt = poptGetNextOpt(pc)) != -1) {
+        switch(opt) {
+        default:
+            fprintf(stderr, "\nInvalid option %s: %s\n\n",
+                    poptBadOption(pc, 0), poptStrerror(opt));
+            poptPrintUsage(pc, stderr, 0);
+            return 1;
+        }
+    }
+    poptFreeContext(pc);
+
+    DEBUG_CLI_INIT(debug_level);
+
+    /* Even though normally the tests should clean up after themselves
+     * they might not after a failed run. Remove the old db to be sure */
+    tests_set_cwd();
+
+    return cmocka_run_group_tests(tests, NULL, NULL);
+}
-- 
2.4.3



More information about the sssd-devel mailing list