On (08/04/15 15:38), Jakub Hrozek wrote:
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.
ACK
http://sssd-ci.duckdns.org/logs/job/12/74/summary.html
LS