On Thu, Apr 02, 2015 at 11:58:41AM +0200, Lukas Slebodnik wrote:
On (30/03/15 11:47), Jakub Hrozek wrote:
>Hi,
>
>I noticed this bug while working on the SSH fix with
>default_domain_suffix. It turns out our filter_users/filter_groups don't
>work well in this scenario, which might have effect on the server load
>even. Attached are patches that re-initialize negcache with filter_lists
>contents after the subdomains are (re)discovered. All patches have
>tests.
>From c00a8d74bfe74c3f00e29d1b5c0271bc1bfdb24a Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Fri, 27 Mar 2015 12:57:17 +0100
>Subject: [PATCH 1/5] ncache: Fix sss_ncache_reset_permanent
>
>There was an off-by-one error in sss_ncache_reset_permanent that
>prevented the reset from working.
>---
> src/responder/common/negcache.c | 2 +-
> src/tests/cmocka/test_negcache.c | 11 +++++++++++
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
>diff --git a/src/responder/common/negcache.c b/src/responder/common/negcache.c
>index
88dd18fa5348fca223a0b2db844e3d17286045b7..04c9a53f556497173daf1ef9c562896cb2d5bbc9 100644
>--- a/src/responder/common/negcache.c
>+++ b/src/responder/common/negcache.c
>@@ -556,7 +556,7 @@ static int delete_permanent(struct tdb_context *tdb,
> char *ep;
>
> if (strncmp((char *)key.dptr,
>- NC_ENTRY_PREFIX, sizeof(NC_ENTRY_PREFIX)) != 0) {
>+ NC_ENTRY_PREFIX, sizeof(NC_ENTRY_PREFIX) - 1) != 0) {
> /* not interested in this key */
> return 0;
> }
IMHO this test is not very useful.
All entries in negative cache should start with NC_ENTRY_PREFIX.
But your patch fixes the bug.
ACK
yes, but without fixing delete_permanent the test would not pass because
sss_ncache_reset_permanent() didn't work at all.
>From d2e30c6083b5082b90343ca6be10e1899f44d911 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Fri, 27 Mar 2015 12:30:53 +0100
>Subject: [PATCH 2/5] ncache: Silence critical error from filter_users when
> default_domain_suffix is set
>
>When default_domain_suffix is used and filter_users is set (at least
>root is always, by default), SSSD tried to add the negcache entry to the
>default domain. But since the default domain is not known after start
>up, adding the entries fail with a verbose error message.
>
>This patch handles EAGAIN returned from the parsing function while
>setting negcache entries gracefully and also makes the debug message in
>parsing function more precise.
>---
> src/responder/common/negcache.c | 18 +++++++++--
> src/tests/cmocka/test_negcache.c | 69 ++++++++++++++++++++++++++++++++++++++--
> src/util/usertools.c | 3 +-
> 3 files changed, 82 insertions(+), 8 deletions(-)
>
>diff --git a/src/tests/cmocka/test_negcache.c b/src/tests/cmocka/test_negcache.c
>index
4502c0294e772b15bf230d485e02c40e2ffbf2d6..5ade3fd4b347a6c7c74754730bca4b82c71ddf06 100644
>--- a/src/tests/cmocka/test_negcache.c
>+++ b/src/tests/cmocka/test_negcache.c
>@@ -590,8 +590,8 @@ static void test_sss_ncache_prepopulate(void **state)
> struct sss_domain_info *dom;
>
> struct sss_test_conf_param params[] = {
>- { "filter_users", "testuser1" },
>- { "filter_groups", "testgroup1" },
>+ { "filter_users", "testuser1,
testuser2@"TEST_DOM_NAME", testuser3@somedomain" },
>+ { "filter_groups", "testgroup1,
testgroup2@"TEST_DOM_NAME", testgroup3@somedomain" },
> { NULL, NULL },
> };
>
>@@ -628,6 +628,67 @@ static void test_sss_ncache_prepopulate(void **state)
>
> ret = sss_ncache_check_group(ncache, 1, dom, "testgroup1");
> assert_int_equal(ret, EEXIST);
>+
>+ ret = sss_ncache_check_user(ncache, 1, dom, "testuser2");
>+ assert_int_equal(ret, EEXIST);
>+
>+ ret = sss_ncache_check_group(ncache, 1, dom, "testgroup2");
>+ assert_int_equal(ret, EEXIST);
>+
>+ ret = sss_ncache_check_user(ncache, 1, dom, "testuser3");
>+ assert_int_equal(ret, ENOENT);
>+
>+ ret = sss_ncache_check_group(ncache, 1, dom, "testgroup3");
>+ assert_int_equal(ret, ENOENT);
>+
>+ ret = sss_ncache_check_user(ncache, 1, dom, "testuser3@somedomain");
>+ assert_int_equal(ret, ENOENT);
>+
>+ ret = sss_ncache_check_group(ncache, 1, dom,
"testgroup3@somedomain");
>+ assert_int_equal(ret, ENOENT);
>+}
>+
>+static void test_sss_ncache_default_domain_suffix(void **state)
>+{
>+ int ret;
>+ struct test_state *ts;
>+ struct tevent_context *ev;
>+ struct sss_nc_ctx *ncache;
>+ struct sss_test_ctx *tc;
>+ struct sss_domain_info *dom;
>+
>+ struct sss_test_conf_param params[] = {
>+ { "filter_users", "testuser1,
testuser2@"TEST_DOM_NAME", testuser3@somedomain" },
>+ { "filter_groups", "testgroup1,
testgroup2@"TEST_DOM_NAME", testgroup3@somedomain" },
>+ { NULL, NULL },
>+ };
>+
>+ ts = talloc_get_type_abort(*state, struct test_state);
>+
>+ ev = tevent_context_init(ts);
>+ assert_non_null(ev);
>+
>+ dom = talloc_zero(ts, struct sss_domain_info);
>+ assert_non_null(dom);
>+ dom->name = discard_const_p(char, TEST_DOM_NAME);
>+
>+ ts->nctx = mock_nctx(ts);
>+ assert_non_null(ts->nctx);
>+
>+ tc = create_dom_test_ctx(ts, TESTS_PATH, TEST_CONF_DB,
>+ TEST_DOM_NAME, TEST_ID_PROVIDER, params);
>+ assert_non_null(tc);
>+
>+ ncache = ts->ctx;
>+ ts->rctx = mock_rctx(ts, ev, dom, ts->nctx);
>+ assert_non_null(ts->rctx);
>+ ts->rctx->default_domain = discard_const(TEST_DOM_NAME);
>+
>+ ret = sss_names_init(ts, tc->confdb, TEST_DOM_NAME, &dom->names);
>+ assert_int_equal(ret, EOK);
>+
>+ ret = sss_ncache_prepopulate(ncache, tc->confdb, ts->rctx);
>+ assert_int_equal(ret, EOK);
Could you test "filtered entries" after pre populating negative cache?
Done.
>From e5745bb5384772f25850203c27a355cba5a5c374 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Sun, 29 Mar 2015 16:30:27 +0200
>Subject: [PATCH 3/5] ncache: Add sss_ncache_reset_repopulate_permanent
>
>This new function resets the negative cache and then re-adds the
>permanent entries.
>---
> src/responder/common/negcache.c | 14 ++++++
> src/responder/common/negcache.h | 6 +++
> src/tests/cmocka/test_negcache.c | 93 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 113 insertions(+)
>
ACK
>From 0570429e64f8c42feeed8bed75bf5530d1c1fed2 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Sun, 29 Mar 2015 16:31:19 +0200
>Subject: [PATCH 4/5] responders: reset ncache after domains are discovered
> during startup
>
>After responders start, they add a lookup operation that discovers the
>subdomains so that qualifying users works. After this operation is
>finishes, we need to reset negcache to allow users to be added into the
>newly discovered domains.
>---
ACK
>From 1d9bc4c91131dc8af1cbddeb414f0cb71069724d Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Sun, 29 Mar 2015 20:30:44 +0200
>Subject: [PATCH 5/5] NSS: Reset negcache after checking domains
>
>The NSS responder periodically re-checks subdomains. We need to reset
>the negative cache each time the check finishes to allow the negative
>cache to contain entries from different domains.
>---
ACK
Thank you for the review, new patches are attached.