>From 78a63b704a265e849ea60bb1d4438f4439dc13aa Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sun, 25 Nov 2012 22:25:37 +0100 Subject: [PATCH] MEMBEROF: Implement delete operation for ghost users https://fedorahosted.org/sssd/ticket/1668 The memberof plugin did only expand the ghost users attribute to parents when adding a nested group, but didn't implement the reverse operation. This bug resulted in users being reported as group members even after the direct parent went away as the expanded ghost attributes were never removed from the parent entry. When a ghost entry is removed from a group, all its parent groups are expired from the cache by setting the expire timestamp to 1. Doing so would force the SSSD to re-read the group next time it is requested in order to make sure its members are really up-to-date. --- src/ldb_modules/memberof.c | 233 ++++++++++++++++++++++++++++++++++++++++++++- src/tests/sysdb-tests.c | 107 ++++++++++++++++++++- 2 files changed, 335 insertions(+), 5 deletions(-) diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c index b884228d2ce845a5e52b6f500d47c6ac6dcba42e..1980a64e3c39d3588d2710aef89312f2b30ce9c1 100644 --- a/src/ldb_modules/memberof.c +++ b/src/ldb_modules/memberof.c @@ -33,6 +33,8 @@ #define DB_MEMBERUID "memberuid" #define DB_NAME "name" #define DB_USER_CLASS "user" +#define DB_GROUP_CLASS "group" +#define DB_CACHE_EXPIRE "dataExpireTimestamp" #define DB_OC "objectClass" #ifndef talloc_zfree @@ -133,6 +135,10 @@ struct mbof_del_ctx { int num_muops; int cur_muop; + struct mbof_memberuid_op *ghops; + int num_ghops; + int cur_ghop; + struct mbof_mod_ctx *follow_mod; bool is_mod; }; @@ -165,7 +171,8 @@ static struct mbof_ctx *mbof_init(struct ldb_module *module, return ctx; } -static int entry_is_user_object(struct ldb_message *entry) +static int entry_has_objectclass(struct ldb_message *entry, + const char *objectclass) { struct ldb_message_element *el; struct ldb_val *val; @@ -179,7 +186,7 @@ static int entry_is_user_object(struct ldb_message *entry) /* see if this is a user */ for (i = 0; i < el->num_values; i++) { val = &(el->values[i]); - if (strncasecmp(DB_USER_CLASS, (char *)val->data, val->length) == 0) { + if (strncasecmp(objectclass, (char *)val->data, val->length) == 0) { return LDB_SUCCESS; } } @@ -187,6 +194,16 @@ static int entry_is_user_object(struct ldb_message *entry) return LDB_ERR_NO_SUCH_ATTRIBUTE; } +static int entry_is_user_object(struct ldb_message *entry) +{ + return entry_has_objectclass(entry, DB_USER_CLASS); +} + +static int entry_is_group_object(struct ldb_message *entry) +{ + return entry_has_objectclass(entry, DB_GROUP_CLASS); +} + static int mbof_append_muop(TALLOC_CTX *memctx, struct mbof_memberuid_op **_muops, int *_num_muops, @@ -1181,16 +1198,22 @@ static int mbof_del_get_next(struct mbof_del_operation *delop, struct mbof_del_operation **nextop); static int mbof_del_fill_muop(struct mbof_del_ctx *del_ctx, struct ldb_message *entry); +static int mbof_del_fill_ghop(struct mbof_del_ctx *del_ctx, + struct ldb_message *entry); static int mbof_del_muop(struct mbof_del_ctx *ctx); static int mbof_del_muop_callback(struct ldb_request *req, struct ldb_reply *ares); +static int mbof_del_ghop(struct mbof_del_ctx *del_ctx); +static int mbof_del_ghop_callback(struct ldb_request *req, + struct ldb_reply *ares); static void free_delop_contents(struct mbof_del_operation *delop); static int memberof_del(struct ldb_module *module, struct ldb_request *req) { static const char *attrs[] = { DB_OC, DB_NAME, - DB_MEMBER, DB_MEMBEROF, NULL }; + DB_MEMBER, DB_MEMBEROF, + DB_GHOST, NULL }; struct ldb_context *ldb = ldb_module_get_ctx(module); struct mbof_del_operation *first; struct ldb_request *search; @@ -1415,6 +1438,13 @@ static int mbof_orig_del_callback(struct ldb_request *req, return ldb_module_done(ctx->req, NULL, NULL, ret); } + /* ..or ghost attributes to remove */ + ret = mbof_del_fill_ghop(del_ctx, del_ctx->first->entry); + if (ret != LDB_SUCCESS) { + talloc_zfree(ares); + return ldb_module_done(ctx->req, NULL, NULL, ret); + } + /* if there are any parents, fire a removal sequence */ ret = mbof_del_cleanup_parents(del_ctx); } @@ -1426,6 +1456,10 @@ static int mbof_orig_del_callback(struct ldb_request *req, else if (del_ctx->muops) { return mbof_del_muop(del_ctx); } + /* see if we need to remove some ghost users */ + else if (del_ctx->ghops) { + return mbof_del_ghop(del_ctx); + } else { /* no parents nor children, end ops */ return ldb_module_done(ctx->req, @@ -1537,6 +1571,10 @@ static int mbof_del_clean_par_callback(struct ldb_request *req, else if (del_ctx->muops) { return mbof_del_muop(del_ctx); } + /* see if we need to remove some ghost users */ + else if (del_ctx->ghops) { + return mbof_del_ghop(del_ctx); + } else { /* no children, end ops */ return ldb_module_done(ctx->req, @@ -2217,6 +2255,10 @@ static int mbof_del_progeny(struct mbof_del_operation *delop) if (del_ctx->muops) { return mbof_del_muop(del_ctx); } + /* see if we need to remove some ghost users */ + else if (del_ctx->ghops) { + return mbof_del_ghop(del_ctx); + } /* see if there are follow functions to run */ if (del_ctx->follow_mod) { return mbof_mod_add(del_ctx->follow_mod, @@ -2345,7 +2387,80 @@ static int mbof_del_fill_muop(struct mbof_del_ctx *del_ctx, return LDB_SUCCESS; } -/* del memberuid attributes to parent groups */ +static int mbof_del_fill_ghop(struct mbof_del_ctx *del_ctx, + struct ldb_message *entry) +{ + struct ldb_message_element *mbof; + struct ldb_message_element *ghel; + struct ldb_dn *valdn; + int ret; + int i, j; + + mbof = ldb_msg_find_element(entry, DB_MEMBEROF); + if (!mbof || mbof->num_values == 0) { + /* no memberof attributes ... */ + return LDB_SUCCESS; + } + + ghel = ldb_msg_find_element(entry, DB_GHOST); + if (ghel == NULL || ghel->num_values == 0) { + /* No ghel attribute, just return success */ + return LDB_SUCCESS; + } + + ret = entry_is_group_object(entry); + switch (ret) { + case LDB_SUCCESS: + /* it's a group object, continue */ + break; + + case LDB_ERR_NO_SUCH_ATTRIBUTE: + /* it is not a group object, just return */ + return LDB_SUCCESS; + + default: + /* an error occured, return */ + return ret; + } + + ldb_debug(ldb_module_get_ctx(del_ctx->ctx->module), + LDB_DEBUG_TRACE, + "will delete %d ghost users from %d parents\n", + ghel->num_values, mbof->num_values); + + for (i = 0; i < mbof->num_values; i++) { + valdn = ldb_dn_from_ldb_val(del_ctx->ghops, + ldb_module_get_ctx(del_ctx->ctx->module), + &mbof->values[i]); + if (!valdn || !ldb_dn_validate(valdn)) { + ldb_debug(ldb_module_get_ctx(del_ctx->ctx->module), + LDB_DEBUG_ERROR, + "Invalid dn value: [%s]", + (const char *)mbof->values[i].data); + } + + ldb_debug(ldb_module_get_ctx(del_ctx->ctx->module), + LDB_DEBUG_TRACE, + "processing ghosts in parent [%s]\n", + (const char *) mbof->values[i].data); + + for (j = 0; j < ghel->num_values; j++) { + ret = mbof_append_muop(del_ctx, &del_ctx->ghops, + &del_ctx->num_ghops, + LDB_FLAG_MOD_DELETE, + valdn, + (const char *) ghel->values[j].data, + DB_GHOST); + if (ret != LDB_SUCCESS) { + return ret; + } + } + } + + return LDB_SUCCESS; +} + +/* del memberuid attributes from parent groups */ static int mbof_del_muop(struct mbof_del_ctx *del_ctx) { struct ldb_context *ldb; @@ -2411,6 +2526,116 @@ static int mbof_del_muop_callback(struct ldb_request *req, if (del_ctx->cur_muop < del_ctx->num_muops) { ret = mbof_del_muop(del_ctx); } + /* see if we need to remove some ghost users */ + else if (del_ctx->ghops) { + return mbof_del_ghop(del_ctx); + } + /* see if there are follow functions to run */ + else if (del_ctx->follow_mod) { + return mbof_mod_add(del_ctx->follow_mod, + del_ctx->follow_mod->to_add); + } + else { + return ldb_module_done(ctx->req, + ctx->ret_ctrls, + ctx->ret_resp, + LDB_SUCCESS); + } + + if (ret != LDB_SUCCESS) { + talloc_zfree(ares); + return ldb_module_done(ctx->req, NULL, NULL, ret); + } + } + + talloc_zfree(ares); + return LDB_SUCCESS; +} + +/* del ghost attributes from parent groups */ +static int mbof_del_ghop(struct mbof_del_ctx *del_ctx) +{ + struct ldb_context *ldb; + struct ldb_message *msg; + struct ldb_request *mod_req; + struct mbof_ctx *ctx; + int ret; + + ctx = del_ctx->ctx; + ldb = ldb_module_get_ctx(ctx->module); + + msg = ldb_msg_new(del_ctx); + if (!msg) return LDB_ERR_OPERATIONS_ERROR; + + msg->dn = del_ctx->ghops[del_ctx->cur_ghop].dn; + + ret = ldb_msg_add(msg, del_ctx->ghops[del_ctx->cur_ghop].el, + LDB_FLAG_MOD_DELETE); + if (ret != LDB_SUCCESS) { + return ret; + } + + /* Also expire any parent groups to force reloading direct members in + * case the ghost users we remove now were actually *also* direct members + * of the parent groups + */ + ret = ldb_msg_add_empty(msg, DB_CACHE_EXPIRE, LDB_FLAG_MOD_REPLACE, NULL); + if (ret != LDB_SUCCESS) { + return ret; + } + + ret = ldb_msg_add_string(msg, DB_CACHE_EXPIRE, "1"); + if (ret != LDB_SUCCESS) { + return ret; + } + + ret = ldb_build_mod_req(&mod_req, ldb, del_ctx, + msg, NULL, + del_ctx, mbof_del_ghop_callback, + ctx->req); + if (ret != LDB_SUCCESS) { + return ret; + } + + return ldb_next_request(ctx->module, mod_req); +} + +static int mbof_del_ghop_callback(struct ldb_request *req, + struct ldb_reply *ares) +{ + struct mbof_del_ctx *del_ctx; + struct mbof_ctx *ctx; + int ret; + + del_ctx = talloc_get_type(req->context, struct mbof_del_ctx); + ctx = del_ctx->ctx; + + if (!ares) { + return ldb_module_done(ctx->req, NULL, NULL, + LDB_ERR_OPERATIONS_ERROR); + } + if (ares->error != LDB_SUCCESS) { + return ldb_module_done(ctx->req, + ares->controls, + ares->response, + ares->error); + } + + switch (ares->type) { + case LDB_REPLY_ENTRY: + /* shouldn't happen */ + talloc_zfree(ares); + return ldb_module_done(ctx->req, NULL, NULL, + LDB_ERR_OPERATIONS_ERROR); + case LDB_REPLY_REFERRAL: + /* ignore */ + break; + + case LDB_REPLY_DONE: + del_ctx->cur_ghop++; + if (del_ctx->cur_ghop < del_ctx->num_ghops) { + ret = mbof_del_ghop(del_ctx); + } /* see if there are follow functions to run */ else if (del_ctx->follow_mod) { return mbof_mod_add(del_ctx->follow_mod, diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c index 74b75233d7a5cf027ffdf581cbc9942ba2cc2b95..7b69876ff0dc3dba5168d6f3ac096526bf7ff576 100644 --- a/src/tests/sysdb-tests.c +++ b/src/tests/sysdb-tests.c @@ -2267,6 +2267,106 @@ START_TEST (test_sysdb_memberof_check_memberuid_loop_without_group_5) } END_TEST +START_TEST (test_sysdb_memberof_check_nested_ghosts) +{ + struct sysdb_test_ctx *test_ctx; + struct test_data *data; + int ret; + + /* Setup */ + ret = setup_sysdb_tests(&test_ctx); + if (ret != EOK) { + fail("Could not set up the test"); + return; + } + + data = talloc_zero(test_ctx, struct test_data); + data->ctx = test_ctx; + data->ev = test_ctx->ev; + data->gid = _i; + + data->attrlist = talloc_array(data, const char *, 2); + fail_unless(data->attrlist != NULL, "talloc_array failed."); + data->attrlist[0] = SYSDB_GHOST; + data->attrlist[1] = NULL; + + ret = sysdb_search_group_by_gid(data, test_ctx->sysdb, + data->gid, + data->attrlist, &data->msg); + fail_if(ret != EOK, "Cannot retrieve group %llu\n", (unsigned long long) data->gid); + + fail_unless(strcmp(data->msg->elements[0].name, SYSDB_GHOST) == 0, + "Wrong attribute name"); + fail_unless(data->msg->elements[0].num_values == _i - MBO_GROUP_BASE + 1, + "Wrong number of attribute values, expected [%d] got [%d]", + _i + 1, data->msg->elements[0].num_values); + + talloc_free(test_ctx); +} +END_TEST + +START_TEST (test_sysdb_memberof_remove_child_group_and_check_ghost) +{ + struct sysdb_test_ctx *test_ctx; + struct test_data *data; + int ret; + gid_t delgid; + + /* Setup */ + ret = setup_sysdb_tests(&test_ctx); + if (ret != EOK) { + fail("Could not set up the test"); + return; + } + + data = talloc_zero(test_ctx, struct test_data); + data->ctx = test_ctx; + data->ev = test_ctx->ev; + data->gid = _i; + delgid = data->gid - 1; + + data->attrlist = talloc_array(data, const char *, 2); + fail_unless(data->attrlist != NULL, "talloc_array failed."); + data->attrlist[0] = SYSDB_GHOST; + data->attrlist[1] = NULL; + + ret = sysdb_search_group_by_gid(data, test_ctx->sysdb, + data->gid, + data->attrlist, &data->msg); + fail_if(ret != EOK, "Cannot retrieve group %llu\n", (unsigned long long) data->gid); + + fail_unless(strcmp(data->msg->elements[0].name, SYSDB_GHOST) == 0, + "Wrong attribute name"); + + /* Expect our own and our parent's */ + fail_unless(data->msg->elements[0].num_values == 2, + "Wrong number of attribute values, expected [%d] got [%d]", + 2, data->msg->elements[0].num_values); + + /* Remove the parent */ + ret = sysdb_delete_group(data->ctx->sysdb, NULL, delgid); + fail_if(ret != EOK, "Cannot delete group %llu [%d]: %s\n", + (unsigned long long) data->gid, ret, strerror(ret)); + + talloc_free(data->msg); + + /* Check the parent again. The inherited ghost user should be gone. */ + ret = sysdb_search_group_by_gid(data, test_ctx->sysdb, + data->gid, data->attrlist, &data->msg); + fail_if(ret != EOK, "Cannot retrieve group %llu\n", (unsigned long long) data->gid); + + fail_unless(strcmp(data->msg->elements[0].name, SYSDB_GHOST) == 0, + "Wrong attribute name"); + + /* Expect our own now only */ + fail_unless(data->msg->elements[0].num_values == 1, + "Wrong number of attribute values, expected [%d] got [%d]", + 1, data->msg->elements[0].num_values); + + talloc_free(test_ctx); +} +END_TEST + START_TEST (test_sysdb_memberof_check_ghost) { struct sysdb_test_ctx *test_ctx; @@ -4331,8 +4431,13 @@ Suite *create_sysdb_suite(void) /* Ghost users tests */ tcase_add_loop_test(tc_memberof, test_sysdb_memberof_store_group_with_ghosts, MBO_GROUP_BASE , MBO_GROUP_BASE + 10); - tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid, + tcase_add_loop_test(tc_memberof, test_sysdb_memberof_check_nested_ghosts, MBO_GROUP_BASE , MBO_GROUP_BASE + 10); + tcase_add_loop_test(tc_memberof, test_sysdb_memberof_remove_child_group_and_check_ghost, + MBO_GROUP_BASE + 1, MBO_GROUP_BASE + 10); + /* Only one group should be left now */ + tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid, + MBO_GROUP_BASE + 9 , MBO_GROUP_BASE + 10); /* ghost users - RFC2307 */ /* Add groups with ghost users */ -- 1.8.0