[SSSD] [PATCH] Fixes for NSS getnamebysid requests

Jakub Hrozek jhrozek at redhat.com
Mon Mar 30 16:59:07 UTC 2015


Hi,

another problem I found while testing another patchset was that NSS
getnamebysid never consulted cache and went straight to BE. It was
because the cacheExpire attribute wasn't requested, so the default value
of 0 was used. Also negative cache wasn't consulted, but that's not so
much of a problem.

Attached are patches that fix both bugs and a unit test.
-------------- next part --------------
>From 96121cf6f50988f3c3c22a80c4240cadb3ef0d99 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 30 Mar 2015 12:18:23 +0200
Subject: [PATCH 1/3] sysdb: Add cache_expire to the default
 sysdb_search_object_by_str_attr set

---
 src/db/sysdb_ops.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 8895b62c4e357ee40d63dfbd0934ed0261af6aa2..54cd714a4c9e55a160480ca0a47eae7f2d414e36 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -3538,7 +3538,8 @@ static errno_t sysdb_search_object_by_str_attr(TALLOC_CTX *mem_ctx,
     TALLOC_CTX *tmp_ctx;
     const char *def_attrs[] = { SYSDB_NAME, SYSDB_UIDNUM, SYSDB_GIDNUM,
                                 ORIGINALAD_PREFIX SYSDB_NAME,
-                                SYSDB_OBJECTCLASS, NULL };
+                                SYSDB_DEFAULT_ATTRS,
+                                NULL };
     struct ldb_dn *basedn;
     int ret;
     struct ldb_result *res = NULL;
-- 
2.1.0

-------------- next part --------------
>From c883989e52cd770c19e8b23f0a64195569c30ccb Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 30 Mar 2015 18:34:42 +0200
Subject: [PATCH 2/3] nss: Use negcache for getbysid requests

---
 src/responder/nss/nsssrv_cmd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 72da865d0bbe2142799a93415f0a645ae9cac13d..45d65717312f90cadb88f3a7334aca0629e33d58 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -4539,6 +4539,15 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
         return EIO;
     }
 
+    /* verify this user has not yet been negatively cached,
+        * or has been permanently filtered */
+    ret = sss_ncache_check_sid(nctx->ncache, nctx->neg_timeout, cmdctx->secid);
+    if (ret == EEXIST) {
+        DEBUG(SSSDBG_TRACE_FUNC,
+              "SID [%s] does not exist! (negative cache)\n", cmdctx->secid);
+        return ENOENT;
+    }
+
     ret = sysdb_search_object_by_sid(cmdctx, dom, cmdctx->secid, NULL,
                                      &dctx->res);
     if (ret == ENOENT) {
-- 
2.1.0

-------------- next part --------------
>From 15cf2a6bdaa445b2daf19170b3c2239217a616e5 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 30 Mar 2015 18:54:34 +0200
Subject: [PATCH 3/3] tests: Add NSS responder tests for bysid requests

---
 Makefile.am                     |   1 +
 src/tests/cmocka/test_nss_srv.c | 219 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 220 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index 38a2ee3e45b5c37e4367b82f321327062f6e2bb4..64485a08725a3902b95d0b49533543fe30642cf4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1756,6 +1756,7 @@ nss_srv_tests_CFLAGS = \
 nss_srv_tests_LDFLAGS = \
     -Wl,-wrap,sss_ncache_check_user \
     -Wl,-wrap,sss_ncache_check_uid \
+    -Wl,-wrap,sss_ncache_check_sid \
     -Wl,-wrap,sss_packet_get_body \
     -Wl,-wrap,sss_packet_get_cmd \
     -Wl,-wrap,sss_cmd_send_empty \
diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
index 480d65aec502cd60e11a7a23911c7c7afb59833b..e8947174f3f851dcc30d2df73e3dece452eba852 100644
--- a/src/tests/cmocka/test_nss_srv.c
+++ b/src/tests/cmocka/test_nss_srv.c
@@ -38,6 +38,7 @@
 #define TEST_DOM_NAME "nss_test"
 #define TEST_SUBDOM_NAME "test.subdomain"
 #define TEST_ID_PROVIDER "ldap"
+#define TEST_DOM_SID "S-1-5-21-444379608-1639770488-2995963434"
 
 #define N_ELEMENTS(arr) \
     (sizeof(arr) / sizeof(arr[0]))
@@ -180,6 +181,21 @@ int __wrap_sss_ncache_check_uid(struct sss_nc_ctx *ctx, int ttl, uid_t uid)
     return ret;
 }
 
+int __real_sss_ncache_check_sid(struct sss_nc_ctx *ctx,
+                                int ttl, const char *sid);
+
+int __wrap_sss_ncache_check_sid(struct sss_nc_ctx *ctx,
+                                int ttl, const char *sid)
+{
+    int ret;
+
+    ret = __real_sss_ncache_check_sid(ctx, ttl, sid);
+    if (ret == EEXIST) {
+        nss_test_ctx->ncache_hits++;
+    }
+    return ret;
+}
+
 /* Mock input from the client library */
 static void mock_input_user_or_group(const char *username)
 {
@@ -208,6 +224,12 @@ static void mock_fill_user(void)
     will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);
 }
 
+static void mock_fill_bysid(void)
+{
+    /* One packet for the entry and one for num entries */
+    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);
+}
+
 static void mock_fill_initgr_user(void)
 {
     will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);
@@ -1007,6 +1029,8 @@ void test_nss_setup(struct sss_test_conf_param params[],
                                              TEST_ID_PROVIDER, params);
     assert_non_null(nss_test_ctx->tctx);
 
+    nss_test_ctx->tctx->dom->domain_id = discard_const(TEST_DOM_SID);
+
     nss_test_ctx->nss_cmds = get_nss_cmds();
     assert_non_null(nss_test_ctx->nss_cmds);
 
@@ -2577,6 +2601,195 @@ static int nss_test_teardown(void **state)
     return 0;
 }
 
+static int test_nss_getnamebysid_check(uint32_t status, uint8_t *body, size_t blen)
+{
+    size_t rp = 2 * sizeof(uint32_t); /* num_results and reserved */
+    uint32_t id_type;
+    const char *name;
+
+    assert_int_equal(status, EOK);
+
+    SAFEALIGN_COPY_UINT32(&id_type, body+rp, &rp);
+    assert_int_equal(id_type, SSS_ID_TYPE_UID);
+
+    name = (const char *) body + rp;
+    assert_string_equal(name, "testsiduser");
+
+    return EOK;
+}
+
+static void test_nss_getnamebysid(void **state)
+{
+    errno_t ret;
+    struct sysdb_attrs *attrs;
+    char *user_sid;
+
+    attrs = sysdb_new_attrs(nss_test_ctx);
+    assert_non_null(attrs);
+
+    user_sid = talloc_asprintf(attrs, "%s-500",
+                               nss_test_ctx->tctx->dom->domain_id);
+    assert_non_null(user_sid);
+
+    ret = sysdb_attrs_add_string(attrs, SYSDB_SID_STR, user_sid);
+    assert_int_equal(ret, EOK);
+
+    /* Prime the cache with a valid user */
+    ret = sysdb_add_user(nss_test_ctx->tctx->dom,
+                         "testsiduser", 12345, 6890, "test sid user",
+                         "/home/testsiduser", "/bin/sh", NULL,
+                         attrs, 300, 0);
+    assert_int_equal(ret, EOK);
+
+    mock_input_user_or_group(user_sid);
+    will_return(__wrap_sss_packet_get_cmd, SSS_NSS_GETNAMEBYSID);
+    mock_fill_bysid();
+
+    /* Query for that user, call a callback when command finishes */
+    /* Should go straight to back end, without contacting DP */
+    set_cmd_cb(test_nss_getnamebysid_check);
+    ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETNAMEBYSID,
+                          nss_test_ctx->nss_cmds);
+    assert_int_equal(ret, EOK);
+
+    /* Wait until the test finishes with EOK */
+    ret = test_ev_loop(nss_test_ctx->tctx);
+    assert_int_equal(ret, EOK);
+}
+
+/* Test that searching for a nonexistant user yields ENOENT.
+ * Account callback will be called
+ */
+void test_nss_getnamebysid_neg(void **state)
+{
+    errno_t ret;
+    char *user_sid;
+
+    user_sid = talloc_asprintf(nss_test_ctx, "%s-499",
+                               nss_test_ctx->tctx->dom->domain_id);
+    assert_non_null(user_sid);
+
+    mock_input_user_or_group(user_sid);
+    mock_account_recv_simple();
+
+    assert_int_equal(nss_test_ctx->ncache_hits, 0);
+
+    ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETNAMEBYSID,
+                          nss_test_ctx->nss_cmds);
+    assert_int_equal(ret, EOK);
+
+    /* Wait until the test finishes with ENOENT */
+    ret = test_ev_loop(nss_test_ctx->tctx);
+    assert_int_equal(ret, ENOENT);
+    assert_int_equal(nss_test_ctx->ncache_hits, 0);
+
+    /* Test that subsequent search for a nonexistent user yields
+     * ENOENT and Account callback is not called, on the other hand
+     * the ncache functions will be called
+     */
+    nss_test_ctx->tctx->done = false;
+
+    mock_input_user_or_group(user_sid);
+    ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETNAMEBYSID,
+                          nss_test_ctx->nss_cmds);
+    assert_int_equal(ret, EOK);
+
+    /* Wait until the test finishes with ENOENT */
+    ret = test_ev_loop(nss_test_ctx->tctx);
+    assert_int_equal(ret, ENOENT);
+    /* Negative cache was hit this time */
+    assert_int_equal(nss_test_ctx->ncache_hits, 1);
+}
+
+static int test_nss_getnamebysid_update_check(uint32_t status,
+                                              uint8_t *body,
+                                              size_t blen)
+{
+    size_t rp = 2 * sizeof(uint32_t); /* num_results and reserved */
+    uint32_t id_type;
+    const char *name;
+
+    assert_int_equal(status, EOK);
+
+    SAFEALIGN_COPY_UINT32(&id_type, body+rp, &rp);
+    assert_int_equal(id_type, SSS_ID_TYPE_UID);
+
+    name = (const char *) body + rp;
+    assert_string_equal(name, "testsidbyname_update");
+
+    return EOK;
+}
+
+static int test_nss_getnamebysid_update_acct_cb(void *pvt)
+{
+    errno_t ret;
+    struct nss_test_ctx *ctx = talloc_get_type(pvt, struct nss_test_ctx);
+
+    ret = sysdb_store_user(ctx->tctx->dom, "testsidbyname_update", NULL,
+                           123456, 789, "test user",
+                           "/home/testsidbyname_update", "/bin/ksh", NULL,
+                           NULL, NULL, 300, 0);
+    assert_int_equal(ret, EOK);
+
+    return EOK;
+}
+
+void test_nss_getnamebysid_update(void **state)
+{
+    errno_t ret;
+    struct ldb_result *res;
+    struct sysdb_attrs *attrs;
+    const char *shell;
+    char *user_sid;
+
+    attrs = sysdb_new_attrs(nss_test_ctx);
+    assert_non_null(attrs);
+
+    user_sid = talloc_asprintf(attrs, "%s-123456",
+                               nss_test_ctx->tctx->dom->domain_id);
+    assert_non_null(user_sid);
+
+    ret = sysdb_attrs_add_string(attrs, SYSDB_SID_STR, user_sid);
+    assert_int_equal(ret, EOK);
+
+    /* Prime the cache with a valid but expired user */
+    ret = sysdb_add_user(nss_test_ctx->tctx->dom,
+                         "testsidbyname_update", 123456, 789, "test user",
+                         "/home/testsidbyname_update", "/bin/sh", NULL,
+                         attrs, 1, 1);
+    assert_int_equal(ret, EOK);
+
+    /* Mock client input */
+    mock_input_user_or_group(user_sid);
+    /* Mock client command */
+    will_return(__wrap_sss_packet_get_cmd, SSS_NSS_GETNAMEBYSID);
+    /* Call this function when user is updated by the mock DP request */
+    mock_account_recv(0, 0, NULL, test_nss_getnamebysid_update_acct_cb,
+                      nss_test_ctx);
+    /* Call this function to check what the responder returned to the client */
+    set_cmd_cb(test_nss_getnamebysid_update_check);
+    /* Mock output buffer */
+    mock_fill_bysid();
+
+    /* Fire the command */
+    ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETNAMEBYSID,
+                          nss_test_ctx->nss_cmds);
+    assert_int_equal(ret, EOK);
+
+    /* Wait until the test finishes with EOK */
+    ret = test_ev_loop(nss_test_ctx->tctx);
+    assert_int_equal(ret, EOK);
+
+    /* Check the user was updated in the cache */
+    ret = sysdb_getpwnam(nss_test_ctx, nss_test_ctx->tctx->dom,
+                         "testsidbyname_update", &res);
+    assert_int_equal(ret, EOK);
+    assert_int_equal(res->count, 1);
+
+    shell = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_SHELL, NULL);
+    assert_string_equal(shell, "/bin/ksh");
+}
+
 int main(int argc, const char *argv[])
 {
     int rv;
@@ -2682,6 +2895,12 @@ int main(int argc, const char *argv[])
                                         nss_test_setup, nss_test_teardown),
         cmocka_unit_test_setup_teardown(test_nss_initgr_neg_upn,
                                         nss_test_setup, nss_test_teardown),
+        cmocka_unit_test_setup_teardown(test_nss_getnamebysid,
+                                        nss_test_setup, nss_test_teardown),
+        cmocka_unit_test_setup_teardown(test_nss_getnamebysid_neg,
+                                        nss_test_setup, nss_test_teardown),
+        cmocka_unit_test_setup_teardown(test_nss_getnamebysid_update,
+                                        nss_test_setup, nss_test_teardown),
     };
 
     /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
2.1.0



More information about the sssd-devel mailing list