[SSSD] [PATCHES] SDAP: Set initgroups expire attribute at the end

Lukas Slebodnik lslebodn at redhat.com
Fri May 15 13:54:24 UTC 2015


ehlo,

2nd and 3rd patch fixes https://fedorahosted.org/sssd/ticket/2634

Steps to reproduce are written in upstream and downstream ticket.
Patches are quite small so it should be clear from patches
how to reproduce bug.

LS
-------------- next part --------------
>From f97f93e9923285d630077a6fac26494f108a46cc Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Fri, 15 May 2015 14:08:40 +0200
Subject: [PATCH 1/4] test_nss_srv: Use right function for storing time_t

The size of time_t can be 8 bytes on some platforms.
It is because of year 2038 problem.
---
 src/tests/cmocka/test_nss_srv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
index e8947174f3f851dcc30d2df73e3dece452eba852..86b2dd69fbdf25e5def486cefb6fbd982d053b44 100644
--- a/src/tests/cmocka/test_nss_srv.c
+++ b/src/tests/cmocka/test_nss_srv.c
@@ -2233,7 +2233,7 @@ void test_nss_initgroups(void **state)
     attrs = sysdb_new_attrs(nss_test_ctx);
     assert_non_null(attrs);
 
-    ret = sysdb_attrs_add_uint32(attrs, SYSDB_INITGR_EXPIRE,
+    ret = sysdb_attrs_add_time_t(attrs, SYSDB_INITGR_EXPIRE,
                                  time(NULL) + 300);
     assert_int_equal(ret, EOK);
 
@@ -2335,7 +2335,7 @@ static int test_nss_initgr_search_acct_cb(void *pvt)
     attrs = sysdb_new_attrs(nss_test_ctx);
     assert_non_null(attrs);
 
-    ret = sysdb_attrs_add_uint32(attrs, SYSDB_INITGR_EXPIRE,
+    ret = sysdb_attrs_add_time_t(attrs, SYSDB_INITGR_EXPIRE,
                                  time(NULL) + 300);
     assert_int_equal(ret, EOK);
 
@@ -2418,7 +2418,7 @@ static int test_nss_initgr_update_acct_cb(void *pvt)
     attrs = sysdb_new_attrs(nss_test_ctx);
     assert_non_null(attrs);
 
-    ret = sysdb_attrs_add_uint32(attrs, SYSDB_INITGR_EXPIRE,
+    ret = sysdb_attrs_add_time_t(attrs, SYSDB_INITGR_EXPIRE,
                                  time(NULL) + 300);
     assert_int_equal(ret, EOK);
 
@@ -2458,7 +2458,7 @@ void test_nss_initgr_update(void **state)
     attrs = sysdb_new_attrs(nss_test_ctx);
     assert_non_null(attrs);
 
-    ret = sysdb_attrs_add_uint32(attrs, SYSDB_INITGR_EXPIRE,
+    ret = sysdb_attrs_add_time_t(attrs, SYSDB_INITGR_EXPIRE,
                                  time(NULL) - 1);
     assert_int_equal(ret, EOK);
 
-- 
2.4.0

-------------- next part --------------
>From 824b187092000199ba1556ed5378a3a2fbcf6b8c Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Fri, 15 May 2015 14:11:56 +0200
Subject: [PATCH 2/4] nss: Do not ignore default vaue of SYSDB_INITGR_EXPIRE

When SYSDB_INITGR_EXPIRE had default value (0) then value of
SYSDB_CACHE_EXPIRE was used as initgroups expire attribute.
The right apoach is already used in responder_cache_req.c
---
 src/responder/nss/nsssrv_cmd.c  |   5 +-
 src/tests/cmocka/test_nss_srv.c | 100 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 41586e9fc0fd05b4a485f6c7541f0fef6bfc2d50..012c4e701a4ee211e8e3bb66713d7edc43a48717 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -632,9 +632,8 @@ errno_t check_cache(struct nss_dom_ctx *dctx,
             if (req_type == SSS_DP_INITGROUPS) {
                 cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
                                                           SYSDB_INITGR_EXPIRE,
-                                                          1);
-            }
-            if (cacheExpire == 0) {
+                                                          0);
+            } else {
                 cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
                                                           SYSDB_CACHE_EXPIRE,
                                                           0);
diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
index 86b2dd69fbdf25e5def486cefb6fbd982d053b44..9119d4b154473e1df72d23a795cc5bc2efd24d2a 100644
--- a/src/tests/cmocka/test_nss_srv.c
+++ b/src/tests/cmocka/test_nss_srv.c
@@ -2495,6 +2495,104 @@ void test_nss_initgr_update(void **state)
     assert_int_equal(ret, EOK);
 }
 
+static int test_nss_initgr_update_acct_2expire_attributes_cb(void *pvt)
+{
+    errno_t ret;
+    struct sysdb_attrs *attrs;
+
+    attrs = sysdb_new_attrs(nss_test_ctx);
+    assert_non_null(attrs);
+
+    ret = sysdb_attrs_add_time_t(attrs, SYSDB_INITGR_EXPIRE,
+                                 time(NULL) + 300);
+    assert_int_equal(ret, EOK);
+
+    ret = sysdb_set_user_attr(nss_test_ctx->tctx->dom,
+                              "testinitgr_2attr",
+                              attrs, SYSDB_MOD_REP);
+    assert_int_equal(ret, EOK);
+
+    ret = sysdb_add_group(nss_test_ctx->tctx->dom,
+                          "testinitgr_2attr_gr12", 5222,
+                          NULL, 300, 0);
+    assert_int_equal(ret, EOK);
+
+    ret = sysdb_add_group_member(nss_test_ctx->tctx->dom,
+                                 "testinitgr_2attr_gr12",
+                                 "testinitgr_2attr",
+                                 SYSDB_MEMBER_USER, false);
+    assert_int_equal(ret, EOK);
+
+    return EOK;
+}
+
+static int test_nss_initgr_update_2expire_attributes_check(uint32_t status,
+                                                           uint8_t *body,
+                                                           size_t blen)
+{
+    gid_t expected_gids[] = { 5221, 5222 };
+
+    assert_int_equal(status, EOK);
+    check_initgr_packet(body, blen, expected_gids, N_ELEMENTS(expected_gids));
+    return EOK;
+}
+
+/*
+ * SYSDB_INITGR_EXPIRE has default value 0 => initgroups was not finished.
+ * SYSDB_CACHE_EXPIRE has value from future => getpwnam finished successfully
+ *
+ * Test result: DP should be contacted for update.
+ */
+void test_nss_initgr_update_two_expire_attributes(void **state)
+{
+    errno_t ret;
+    struct sysdb_attrs *attrs;
+
+    attrs = sysdb_new_attrs(nss_test_ctx);
+    assert_non_null(attrs);
+
+    ret = sysdb_attrs_add_time_t(attrs, SYSDB_INITGR_EXPIRE,
+                                 0);
+    assert_int_equal(ret, EOK);
+
+    ret = sysdb_attrs_add_time_t(attrs, SYSDB_CACHE_EXPIRE,
+                                 time(NULL) + 100);
+    assert_int_equal(ret, EOK);
+
+    ret = sysdb_add_user(nss_test_ctx->tctx->dom,
+                         "testinitgr_2attr", 522, 655, "test initgroups2",
+                         "/home/testinitgr_2attr", "/bin/sh", NULL,
+                         attrs, 300, 0);
+    assert_int_equal(ret, EOK);
+
+    ret = sysdb_add_group(nss_test_ctx->tctx->dom,
+                          "testinitgr_2attr_gr11", 5221,
+                          NULL, 300, 0);
+    assert_int_equal(ret, EOK);
+
+    ret = sysdb_add_group_member(nss_test_ctx->tctx->dom,
+                                 "testinitgr_2attr_gr11", "testinitgr_2attr",
+                                 SYSDB_MEMBER_USER, false);
+    assert_int_equal(ret, EOK);
+
+    mock_input_user_or_group("testinitgr_2attr");
+    mock_account_recv(0, 0, NULL,
+                      test_nss_initgr_update_acct_2expire_attributes_cb,
+                      nss_test_ctx);
+    will_return(__wrap_sss_packet_get_cmd, SSS_NSS_INITGR);
+    mock_fill_initgr_user();
+    set_cmd_cb(test_nss_initgr_update_2expire_attributes_check);
+
+    /* Query for that user, call a callback when command finishes */
+    ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_INITGR,
+                          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);
+}
+
 void test_nss_initgroups_upn(void **state)
 {
     errno_t ret;
@@ -2891,6 +2989,8 @@ int main(int argc, const char *argv[])
                                         nss_test_setup, nss_test_teardown),
         cmocka_unit_test_setup_teardown(test_nss_initgr_update,
                                         nss_test_setup, nss_test_teardown),
+        cmocka_unit_test_setup_teardown(test_nss_initgr_update_two_expire_attributes,
+                                        nss_test_setup, nss_test_teardown),
         cmocka_unit_test_setup_teardown(test_nss_initgroups_upn,
                                         nss_test_setup, nss_test_teardown),
         cmocka_unit_test_setup_teardown(test_nss_initgr_neg_upn,
-- 
2.4.0

-------------- next part --------------
>From 00884ca81e26e3f515bbade9cf4d6be29a847b41 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Fri, 15 May 2015 15:05:28 +0200
Subject: [PATCH 3/4] SDAP: Set initgroups expire attribute at the end

Initgrups consisted of two main steps:
1. store user to cache
2. store all user groups to cache.

Previously the attribute SYSDB_INITGR_EXPIRE was set in the first step.
So in case of epmty cache and parallel initgroups request in responders
there was a small period when SYSDB_INITGR_EXPIRE was valid but groups were
not cached. Therefore sometime responder could return zero supplementary
groups.

In case of parallel initgroups requests in responder there are two
other ways how we could get correct results even thought there was a bug.
 a) Time between two request was too small. User was not stored in cache
    yet and 2nd request waited for response from DP.
 b) Time between two request was big enough. All users groups were
    successfully stored in cache and 2nd request returned correct results.

Resolves:
https://fedorahosted.org/sssd/ticket/2634
---
 src/providers/ldap/ldap_id.c          | 42 +++++++++++++++++++++++++++++++++++
 src/providers/ldap/sdap_async_users.c |  4 +---
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
index 997313bec051c3ccb04f50eb094b0d0cda0af173..29642ecb27e5170058e34fe9ae3c21b731955268 100644
--- a/src/providers/ldap/ldap_id.c
+++ b/src/providers/ldap/ldap_id.c
@@ -978,6 +978,40 @@ static int groups_by_user_retry(struct tevent_req *req);
 static void groups_by_user_connect_done(struct tevent_req *subreq);
 static void groups_by_user_done(struct tevent_req *subreq);
 
+static errno_t set_initgroups_expire_attribute(struct sss_domain_info *domain,
+                                               const char *name)
+{
+    errno_t ret;
+    time_t cache_timeout;
+    struct sysdb_attrs *attrs;
+
+    attrs = sysdb_new_attrs(NULL);
+    if (attrs == NULL) {
+        return ENOMEM;
+    }
+
+    cache_timeout = domain->user_timeout;
+
+    ret = sysdb_attrs_add_time_t(attrs, SYSDB_INITGR_EXPIRE,
+                                 (cache_timeout ?
+                                     (time(NULL) + cache_timeout) : 0));
+    if (ret != EOK) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Could not set up attrs\n");
+        goto done;
+    }
+
+    ret = sysdb_set_user_attr(domain, name, attrs, SYSDB_MOD_REP);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              "Failed to set initgroups expire attribute\n");
+        goto done;
+    }
+
+done:
+    talloc_zfree(attrs);
+    return ret;
+}
+
 static struct tevent_req *groups_by_user_send(TALLOC_CTX *memctx,
                                               struct tevent_context *ev,
                                               struct sdap_id_ctx *ctx,
@@ -1122,6 +1156,14 @@ static void groups_by_user_done(struct tevent_req *subreq)
         }
     }
 
+    ret = set_initgroups_expire_attribute(state->ctx->be->domain,
+                                          state->name);
+    if (ret != EOK) {
+        state->dp_error = DP_ERR_FATAL;
+        tevent_req_error(req, ret);
+        return;
+    }
+
     state->dp_error = DP_ERR_OK;
     tevent_req_done(req);
 }
diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c
index 82b4df4793f5f0679046f259c251f5897af831cf..da803d0ca3769f179b49e7a0505bed9636ee283b 100644
--- a/src/providers/ldap/sdap_async_users.c
+++ b/src/providers/ldap/sdap_async_users.c
@@ -468,9 +468,7 @@ int sdap_save_user(TALLOC_CTX *memctx,
     cache_timeout = dom->user_timeout;
 
     if (is_initgr) {
-        ret = sysdb_attrs_add_time_t(user_attrs, SYSDB_INITGR_EXPIRE,
-                                     (cache_timeout ?
-                                      (time(NULL) + cache_timeout) : 0));
+        ret = sysdb_attrs_add_time_t(user_attrs, SYSDB_INITGR_EXPIRE, 0);
         if (ret) {
             goto done;
         }
-- 
2.4.0

-------------- next part --------------
>From 479b99db20e24b66f39f1e46f9fd799f0111efc0 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Fri, 15 May 2015 15:22:47 +0200
Subject: [PATCH 4/4] SDAP: Remove unnecessary argument from sdap_save_user

---
 src/providers/ldap/ldap_id.c               |  2 +-
 src/providers/ldap/sdap_async_initgroups.c |  2 +-
 src/providers/ldap/sdap_async_users.c      | 11 +----------
 src/providers/ldap/sdap_users.h            |  1 -
 4 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
index 29642ecb27e5170058e34fe9ae3c21b731955268..d000bcd6bab744b45e5cbb8341e1c7168b275557 100644
--- a/src/providers/ldap/ldap_id.c
+++ b/src/providers/ldap/ldap_id.c
@@ -434,7 +434,7 @@ static void users_get_done(struct tevent_req *subreq)
                                            name, uid, &usr_attrs);
             if (ret == EOK) {
                 ret = sdap_save_user(state, state->ctx->opts, state->domain,
-                                     usr_attrs[0], false, NULL, 0);
+                                     usr_attrs[0], NULL, 0);
             }
         }
     }
diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c
index 4f775d76b77a311c3394beec4546c4f6c7dc5f6f..4d9738ee6bdfaf6cb3c50ad8bf5a953ed6c7a488 100644
--- a/src/providers/ldap/sdap_async_initgroups.c
+++ b/src/providers/ldap/sdap_async_initgroups.c
@@ -2930,7 +2930,7 @@ static void sdap_get_initgr_user(struct tevent_req *subreq)
     DEBUG(SSSDBG_TRACE_ALL, "Storing the user\n");
 
     ret = sdap_save_user(state, state->opts, state->dom, state->orig_user,
-                         true, NULL, 0);
+                         NULL, 0);
     if (ret) {
         goto fail;
     }
diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c
index da803d0ca3769f179b49e7a0505bed9636ee283b..416bedda8491fae5385e6b6a074b4cf05ae86b65 100644
--- a/src/providers/ldap/sdap_async_users.c
+++ b/src/providers/ldap/sdap_async_users.c
@@ -116,7 +116,6 @@ int sdap_save_user(TALLOC_CTX *memctx,
                    struct sdap_options *opts,
                    struct sss_domain_info *dom,
                    struct sysdb_attrs *attrs,
-                   bool is_initgr,
                    char **_usn_value,
                    time_t now)
 {
@@ -467,13 +466,6 @@ int sdap_save_user(TALLOC_CTX *memctx,
 
     cache_timeout = dom->user_timeout;
 
-    if (is_initgr) {
-        ret = sysdb_attrs_add_time_t(user_attrs, SYSDB_INITGR_EXPIRE, 0);
-        if (ret) {
-            goto done;
-        }
-    }
-
     ret = sdap_save_all_names(user_name, attrs, dom, user_attrs);
     if (ret != EOK) {
         DEBUG(SSSDBG_CRIT_FAILURE, "Failed to save user names\n");
@@ -554,8 +546,7 @@ int sdap_save_users(TALLOC_CTX *memctx,
     for (i = 0; i < num_users; i++) {
         usn_value = NULL;
 
-        ret = sdap_save_user(tmpctx, opts, dom, users[i], false,
-                             &usn_value, now);
+        ret = sdap_save_user(tmpctx, opts, dom, users[i], &usn_value, now);
 
         /* Do not fail completely on errors.
          * Just report the failure to save and go on */
diff --git a/src/providers/ldap/sdap_users.h b/src/providers/ldap/sdap_users.h
index e452a44b9e376d55304e463733adc451406f8687..1cc1cd1fa665a6c872236c14abbb9fc8482116ec 100644
--- a/src/providers/ldap/sdap_users.h
+++ b/src/providers/ldap/sdap_users.h
@@ -35,7 +35,6 @@ int sdap_save_user(TALLOC_CTX *memctx,
                    struct sdap_options *opts,
                    struct sss_domain_info *dom,
                    struct sysdb_attrs *attrs,
-                   bool is_initgr,
                    char **_usn_value,
                    time_t now);
 
-- 
2.4.0



More information about the sssd-devel mailing list