[SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
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?
>From ad138ded237a1f3433fa4f379c70f96d66d6797a Mon Sep 17 00:00:00 2001
From: Jakub Hrozek
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);
in
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
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 :-) ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, 2015-07-03 at 11:59 +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 > :-) If we are serializing all authentications then a busy server would be swamped. I do not see a per-user/per-cache queue so I would tentatively NACK the approach sorry. If clobbering is the only issues we have then what we can do is to always use a custom memory ccache for each auth attempt and then use a locking mechanism to copy from the memory ccache to the actual ccache. But we should really do either per-ccache queuing (maybe not per user as in pathological cases we may have the same ccache for different users ?) or use memory ccaches and copy them with locking, but fully serializing all authentications is not really a solution, when a full auth may require multiple network roundtrips. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
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
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > On Fri, 2015-07-03 at 11:59 +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 > > :-) > > If we are serializing all authentications then a busy server would be > swamped. I do not see a per-user/per-cache queue so I would tentatively > NACK the approach sorry. Thanks for the feedback! I had my doubts about serialization by default as well, but I would like to note that serialization had been the default in the krb5 responder, just not IPA or AD responder since 2010... Please note I'm not bashing the existing code, I think the serialization is the only good approach for old platforms that use FILE: ccache by default..which I guess is why we used it. > > If clobbering is the only issues > we have then what we can do is to > always use a custom memory ccache for each auth attempt and then use a > locking mechanism to copy from the memory ccache to the actual ccache. This is an issue on old platforms like RHEL-6 where FILE is the only (supported) ccache. So memory cache might not be available.. For better or worse, these platforms are used in critical environments and high-load servers and are hitting the bug. > > But we should really do either per-ccache queuing Can you elaborate a bit? Are you suggesting to use ccache as the key in the hash table? I would have to think about a bit, but so far it seems like a bit of an overkill, because even now if the same ccache was used for different users we would clobber it anyway I think. > (maybe not per user as > in pathological cases we may have the same ccache for different users ?) > or use memory ccaches and copy them with locking Yes, but memory ccache is not available on all platforms.. >, but fully serializing > all authentications is not really a solution, when a full auth may > require multiple network roundtrips. That's why I was suggesting using the cached auth feature as a remedy. But I do see your point. Would serializing only for FILE: ccache be an accebtable option for you? Something like: def krb5_queue_auth_send() if ccache_type == FILE: queue() else: send_auth() Then we could document for admins that on busy servers that are unfortunate to only support FILE ccache, use cached auth. If you're running on a recent platform (RHEL-7+), though, serialization is not used. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, 2015-07-03 at 20:33 +0200, Jakub Hrozek wrote: > On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > > On Fri, 2015-07-03 at 11:59 +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 > > > :-) > > > > If we are serializing all authentications then a busy server would be > > swamped. I do not see a per-user/per-cache queue so I would tentatively > > NACK the approach sorry. > > Thanks for the feedback! > > I had my doubts about serialization by default as well, but I would like > to note that serialization had been the default in the krb5 responder, > just not IPA or AD responder since 2010... > > Please note I'm not bashing the existing code, I think the serialization > is the only good approach for old platforms that use FILE: ccache by > default..which I guess is why we used it. I'd have no problems bashing the existing code, it is not perfect, but we should use this chance to improve it :-) > > If clobbering is the only issues > > we have then what we can do is to > > always use a custom memory ccache for each auth attempt and then use a > > locking mechanism to copy from the memory ccache to the actual ccache. > > This is an issue on old platforms like RHEL-6 where FILE is the only > (supported) ccache. So memory cache might not be available.. > > For better or worse, these platforms are used in critical environments > and high-load servers and are hitting the bug. Afaik the Memory ccache is available in all OSs we support including very old RHEL versions. > > > > But we should really do either per-ccache queuing > > Can you elaborate a bit? Are you suggesting to use ccache as the key in > the hash table? I would have to think about a bit, but so far it seems > like a bit of an overkill, because even now if the same ccache was used > for different users we would clobber it anyway I think. Sure, but it is as easy to index it by user name or by ccache, so we can just go by ccache. > > (maybe not per user as > > in pathological cases we may have the same ccache for different users ?) > > or use memory ccaches and copy them with locking > > Yes, but memory ccache is not available on all platforms.. Which platforms miss it ? I am not aware we support any platform that lack the MEMORY ccache type, but I may be wrong. > >, but fully serializing > > all authentications is not really a solution, when a full auth may > > require multiple network roundtrips. > > That's why I was suggesting using the cached auth feature as a remedy. It is not a remedy, if you have a few hundred users logging in, in the morning, all at once, there is no reason to serialize all their operations, and serializing them would make some user wait way too much. Caching wouldn't help as there'd be no cache to check. > But I do see your point. Would serializing only for FILE: ccache be an > accebtable option for you? Something like: No, it would be wrong, DIR or KEYRING ccahes have the same issues IMO. > def krb5_queue_auth_send() > if ccache_type == FILE: > queue() > else: > send_auth() > > Then we could document for admins that on busy servers that are > unfortunate to only support FILE ccache, use cached auth. If you're > running on a recent platform (RHEL-7+), though, serialization is > not used. Nope, I think per-user serialization (and actually maybe even auth de-duplication, why not, if user/pw are the same why run the whole stack twice ?) is what we need in all cases. Actually in most cases de-duplication would successfully handle 99.9% of the issues/races. The only case left would be people firing in rapid succession auth attempts with different parameters (which could still happen if we think of Optional 2FA logins, but would be much rarer). Simo. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > On Fri, 2015-07-03 at 11:59 +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 > > :-) > > If we are serializing all authentications then a busy server would be > swamped. I do not see a per-user/per-cache queue so I would tentatively > NACK the approach sorry. The current cache queue is per user, see add_to_wait_queue() for details. bye, Sumit > > If clobbering is the only issues we have then what we can do is to > always use a custom memory ccache for each auth attempt and then use a > locking mechanism to copy from the memory ccache to the actual ccache. > > But we should really do either per-ccache queuing (maybe not per user as > in pathological cases we may have the same ccache for different users ?) > or use memory ccaches and copy them with locking, but fully serializing > all authentications is not really a solution, when a full auth may > require multiple network roundtrips. > > Simo. > > -- > Simo Sorce * Red Hat, Inc * New York > > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: > On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > > On Fri, 2015-07-03 at 11:59 +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 > > > :-) > > > > If we are serializing all authentications then a busy server would be > > swamped. I do not see a per-user/per-cache queue so I would tentatively > > NACK the approach sorry. > > The current cache queue is per user, see add_to_wait_queue() for > details. I actually did look to check that and failed, I blame the late hour :-) Per-user is fine by me, though I would seriously consider de-duplication while we are here. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, Jul 03, 2015 at 05:01:13PM -0400, Simo Sorce wrote: > On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: > > On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > > > On Fri, 2015-07-03 at 11:59 +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 > > > > :-) > > > > > > If we are serializing all authentications then a busy server would be > > > swamped. I do not see a per-user/per-cache queue so I would tentatively > > > NACK the approach sorry. > > > > The current cache queue is per user, see add_to_wait_queue() for > > details. > > I actually did look to check that and failed, I blame the late hour :-) Ah, I thought that goes without saying it's not a responder-global queue :-) > > Per-user is fine by me, though I would seriously consider de-duplication > while we are here. What exactly do you propose here? ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Mon, Jul 06, 2015 at 09:24:50AM +0200, Jakub Hrozek wrote: > On Fri, Jul 03, 2015 at 05:01:13PM -0400, Simo Sorce wrote: > > On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: > > > On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > > > > On Fri, 2015-07-03 at 11:59 +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 > > > > > :-) > > > > > > > > If we are serializing all authentications then a busy server would be > > > > swamped. I do not see a per-user/per-cache queue so I would tentatively > > > > NACK the approach sorry. > > > > > > The current cache queue is per user, see add_to_wait_queue() for > > > details. > > > > I actually did look to check that and failed, I blame the late hour :-) > > Ah, I thought that goes without saying it's not a responder-global queue ~~~ backend-global, sorry > :-) > > > > > Per-user is fine by me, though I would seriously consider de-duplication > > while we are here. > > What exactly do you propose here? > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, Jul 03, 2015 at 05:01:13PM -0400, Simo Sorce wrote: > On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: > > On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > > > On Fri, 2015-07-03 at 11:59 +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 > > > > :-) > > > > > > If we are serializing all authentications then a busy server would be > > > swamped. I do not see a per-user/per-cache queue so I would tentatively > > > NACK the approach sorry. > > > > The current cache queue is per user, see add_to_wait_queue() for > > details. > > I actually did look to check that and failed, I blame the late hour :-) > > Per-user is fine by me, though I would seriously consider de-duplication > while we are here. I think this might be a useful feature, although I think it should not be on by default. We have discussed this already a bit 5 years ago together with the initial implementation of the auth queues in the krb5 responder. Here are some links for reference: https://fedorahosted.org/sssd/ticket/533 https://lists.fedorahosted.org/pipermail/sssd-devel/2010-October/004821.html https://lists.fedorahosted.org/pipermail/sssd-devel/2010-December/005307.html But I think this feature would need some more discussion. E.g. where should it be implemented, in the PAM responder on in the auth backends? How does is related to the cached authentication feature? How should failed authentication be handled, de-dup as well or send them to the backend to allow the server to update the failed logon counter properly? Shall de-duplication be done by user or by user and by service? Given that I would recommend to commit the patches in the current state and add a RFE ticket to discussion de-duplication for one of the next releases. bye, Sumit > > Simo. > > -- > Simo Sorce * Red Hat, Inc * New York > > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
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.
>From 4706bef89f951e3f9ca686b0b24297df12500c56 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek
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/
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
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
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
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
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Mon, 2015-07-06 at 11:46 +0200, Sumit Bose wrote: > On Fri, Jul 03, 2015 at 05:01:13PM -0400, Simo Sorce wrote: > > On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: > > > On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > > > > On Fri, 2015-07-03 at 11:59 +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 > > > > > :-) > > > > > > > > If we are serializing all authentications then a busy server would be > > > > swamped. I do not see a per-user/per-cache queue so I would tentatively > > > > NACK the approach sorry. > > > > > > The current cache queue is per user, see add_to_wait_queue() for > > > details. > > > > I actually did look to check that and failed, I blame the late hour :-) > > > > Per-user is fine by me, though I would seriously consider de-duplication > > while we are here. > > I think this might be a useful feature, although I think it should not > be on by default. We have discussed this already a bit 5 years ago > together with the initial implementation of the auth queues in the krb5 > responder. Here are some links for reference: > > https://fedorahosted.org/sssd/ticket/533 > https://lists.fedorahosted.org/pipermail/sssd-devel/2010-October/004821.html > https://lists.fedorahosted.org/pipermail/sssd-devel/2010-December/005307.html > > But I think this feature would need some more discussion. E.g. where > should it be implemented, in the PAM responder on in the auth backends? auth backend, the pam responder may not have enough context to properly decide. > How does is related to the cached authentication feature? It doesn't relate in any way IMO. > How should failed authentication be handled, de-dup as well or send them to > the > backend to allow the server to update the failed logon counter properly? The backend de-dups them and you won't know if an authentication is successful or failed until the answer comes back, so de-duping has already happened. > Shall de-duplication be done by user or by user and by service? This is a good question, I think it makes sense to have it per-user, however you must always check that everything is the same, user, password, any other option. Reading #533 I think the original concern about counting failures is a little bit misguided. The reason why we have unsuccessful counts is to avoid password guessing where an attacker tries a different password at every attempt. It is not a measure to somehow punish the user for sending the wrong password. So if an "attacker" where to send 100 auth requests all with the same password it makes no sense to lock up the user account, they SHOULD, ideally, count as a single failure. And actually de-duplication may end up saving a user with scripts. Think of a situation where the user has a script that makes 10 actions that require authentication when he inputs his password. If he enters the wrong password he may lock himself out for X time with just one attempt. With de-dup instead if chewed only one attempt and he can try again (assuming all auths happen at the same time). > Given that I would recommend to commit the patches in the current state > and add a RFE ticket to discussion de-duplication for one of the next > releases. Sure, we can open a separate ticket. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Mon, Jul 06, 2015 at 02:05:04PM +0200, Sumit Bose wrote: > CI passes for me as well > http://sssd-ci.duckdns.org/logs/job/18/60/summary.html. > > ACK. Thank you for the review, I pushed the patches upstream: * 01ec08efd0e166ac6f390f8627c6d08dcc63ccc4 * eca74a9559ce1b0f123c14906ad8394fc303f468 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
