From 844a2a044dc3d3bba8695da57113530723f4825b Mon Sep 17 00:00:00 2001 From: Sumit Bose Date: Thu, 20 Dec 2012 21:10:49 +0100 Subject: Refactor gid handling in the PAC responder Instead of using a single array of gid-domain_pointer pairs, Simo suggested to use a gid array for each domain an store it with a pointer to the domain. --- src/responder/pac/pacsrv.h | 11 +- src/responder/pac/pacsrv_cmd.c | 52 ++++++--- src/responder/pac/pacsrv_utils.c | 223 ++++++++++++++++++++++++++++----------- src/tests/pac_responder-tests.c | 57 ++++++---- 4 files changed, 238 insertions(+), 105 deletions(-) diff --git a/src/responder/pac/pacsrv.h b/src/responder/pac/pacsrv.h index 0bf2a07f5..c087c01f0 100644 --- a/src/responder/pac/pacsrv.h +++ b/src/responder/pac/pacsrv.h @@ -71,9 +71,10 @@ struct grp_info { struct ldb_dn *dn; }; -struct pac_grp { - gid_t gid; +struct pac_dom_grps { struct sss_domain_info *grp_dom; + size_t gid_count; + gid_t *gids; }; int pac_cmd_execute(struct cli_ctx *cctx); @@ -104,7 +105,7 @@ errno_t get_gids_from_pac(TALLOC_CTX *mem_ctx, struct local_mapping_ranges *range_map, struct dom_sid *domain_sid, struct PAC_LOGON_INFO *logon_info, - size_t *_gid_count, struct pac_grp **_gids); + size_t *_gid_count, struct pac_dom_grps **_gids); errno_t get_data_from_pac(TALLOC_CTX *mem_ctx, uint8_t *pac_blob, size_t pac_len, @@ -121,9 +122,9 @@ errno_t diff_gid_lists(TALLOC_CTX *mem_ctx, size_t cur_grp_num, struct grp_info *cur_gid_list, size_t new_gid_num, - struct pac_grp *new_gid_list, + struct pac_dom_grps *new_gid_list, size_t *_add_gid_num, - struct pac_grp **_add_gid_list, + struct pac_dom_grps **_add_gid_list, size_t *_del_gid_num, struct grp_info ***_del_gid_list); diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c index 375285f6b..bc897446e 100644 --- a/src/responder/pac/pacsrv_cmd.c +++ b/src/responder/pac/pacsrv_cmd.c @@ -60,13 +60,13 @@ struct pac_req_ctx { struct dom_sid2 *domain_sid; size_t gid_count; - struct pac_grp *gids; + struct pac_dom_grps *gids; size_t current_grp_count; struct grp_info *current_grp_list; size_t add_gid_count; - struct pac_grp *add_gids; + struct pac_dom_grps *add_gids; size_t del_grp_count; struct grp_info **del_grp_list; @@ -83,7 +83,8 @@ static void pac_get_group_done(struct tevent_req *subreq); static errno_t pac_save_memberships_next(struct tevent_req *req); static errno_t pac_store_membership(struct pac_req_ctx *pr_ctx, struct ldb_dn *user_dn, - int gid_iter); + gid_t gid, + struct sss_domain_info *grp_dom); struct tevent_req *pac_save_memberships_send(struct pac_req_ctx *pr_ctx); static void pac_save_memberships_done(struct tevent_req *req); @@ -417,7 +418,8 @@ done: } struct pac_save_memberships_state { - int gid_iter; + size_t gid_iter; + size_t dom_iter; struct ldb_dn *user_dn; struct pac_req_ctx *pr_ctx; @@ -440,6 +442,7 @@ struct tevent_req *pac_save_memberships_send(struct pac_req_ctx *pr_ctx) } state->gid_iter = 0; + state->dom_iter = 0; state->user_dn = sysdb_user_dn(dom->sysdb, state, pr_ctx->fq_name); if (state->user_dn == NULL) { ret = ENOMEM; @@ -585,17 +588,28 @@ static errno_t pac_save_memberships_next(struct tevent_req *req) return EINVAL; } - while (state->gid_iter < pr_ctx->add_gid_count) { + while (pr_ctx->add_gids[state->dom_iter].grp_dom != NULL) { - ret = pac_store_membership(state->pr_ctx, state->user_dn, - state->gid_iter); + if (pr_ctx->add_gids[state->dom_iter].gids == NULL || + pr_ctx->add_gids[state->dom_iter].gid_count == 0) { + state->dom_iter++; + state->gid_iter = 0; + continue; + } + + + gid = pr_ctx->add_gids[state->dom_iter].gids[state->gid_iter]; + grp_dom = pr_ctx->add_gids[state->dom_iter].grp_dom; + + ret = pac_store_membership(state->pr_ctx, state->user_dn, gid, grp_dom); if (ret == EOK) { state->gid_iter++; + if (state->gid_iter >= pr_ctx->add_gids[state->dom_iter].gid_count) { + state->dom_iter++; + state->gid_iter = 0; + } continue; } else if (ret == ENOENT) { - gid = pr_ctx->add_gids[state->gid_iter].gid; - grp_dom = pr_ctx->add_gids[state->gid_iter].grp_dom; - subreq = sss_dp_get_account_send(state, pr_ctx->cctx->rctx, grp_dom, true, SSS_DP_GROUP, NULL, @@ -629,6 +643,9 @@ static void pac_get_group_done(struct tevent_req *subreq) dbus_uint16_t err_maj; dbus_uint32_t err_min; char *err_msg; + gid_t gid; + struct sss_domain_info *grp_dom; + struct pac_req_ctx *pr_ctx = state->pr_ctx; ret = sss_dp_get_account_recv(req, subreq, &err_maj, &err_min, @@ -639,11 +656,17 @@ static void pac_get_group_done(struct tevent_req *subreq) goto error; } - ret = pac_store_membership(state->pr_ctx, state->user_dn, state->gid_iter); + gid = pr_ctx->add_gids[state->dom_iter].gids[state->gid_iter]; + grp_dom = pr_ctx->add_gids[state->dom_iter].grp_dom; + ret = pac_store_membership(state->pr_ctx, state->user_dn, gid, grp_dom); if (ret != EOK) { goto error; } state->gid_iter++; + if (state->gid_iter >= pr_ctx->add_gids[state->dom_iter].gid_count) { + state->dom_iter++; + state->gid_iter = 0; + } ret = pac_save_memberships_next(req); if (ret == EOK) { @@ -661,13 +684,11 @@ error: static errno_t pac_store_membership(struct pac_req_ctx *pr_ctx, struct ldb_dn *user_dn, - int gid_iter) + gid_t gid, struct sss_domain_info *grp_dom) { TALLOC_CTX *tmp_ctx; struct sysdb_attrs *user_attrs; struct ldb_message *group; - uint32_t gid; - struct sss_domain_info *grp_dom; errno_t ret; const char *orig_group_dn; const char *group_attrs[] = { SYSDB_ORIG_DN, NULL }; @@ -677,9 +698,6 @@ pac_store_membership(struct pac_req_ctx *pr_ctx, return ENOMEM; } - gid = pr_ctx->add_gids[gid_iter].gid; - grp_dom = pr_ctx->add_gids[gid_iter].grp_dom; - ret = sysdb_search_group_by_gid(tmp_ctx, grp_dom->sysdb, gid, group_attrs, &group); if (ret != EOK) { diff --git a/src/responder/pac/pacsrv_utils.c b/src/responder/pac/pacsrv_utils.c index 8328d6fbc..a55ba0f36 100644 --- a/src/responder/pac/pacsrv_utils.c +++ b/src/responder/pac/pacsrv_utils.c @@ -420,6 +420,73 @@ bool dom_sid_in_domain(const struct dom_sid *domain_sid, return true; } + +static errno_t get_dom_grps_from_hash(TALLOC_CTX *mem_ctx, + hash_table_t *gid_table, + struct sss_domain_info *grp_dom, + struct pac_dom_grps *dom_grps) +{ + int ret; + size_t gid_count; + size_t g; + struct hash_iter_context_t *iter; + hash_entry_t *entry; + gid_t *gids = NULL; + + if (grp_dom == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("Missing domain for groups.\n")); + return EINVAL; + } + + gid_count = hash_count(gid_table); + if (gid_count == 0) { + DEBUG(SSSDBG_TRACE_FUNC, ("No groups found.\n")); + ret = EOK; + goto done; + } + + gids = talloc_zero_array(mem_ctx, gid_t, gid_count); + if (gids == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n")); + ret = ENOMEM; + goto done; + } + + + iter = new_hash_iter_context(gid_table); + if (iter == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("new_hash_iter_context failed.\n")); + ret = EIO; + goto done; + } + + g = 0; + while ((entry = iter->next(iter)) != NULL) { + gids[g] = entry->key.ul; + g++; + } + + if (gid_count != g) { + DEBUG(SSSDBG_OP_FAILURE, ("Number of hash entries and groups do not " + "match.\n")); + ret = EINVAL; + goto done; + } + + ret = EOK; + +done: + if (ret != EOK) { + talloc_free(gids); + } else { + dom_grps->grp_dom = grp_dom; + dom_grps->gid_count = gid_count; + dom_grps->gids = gids; + } + + return ret; +} + /** * Find all Posix GIDs from a PAC by searching for group SIDs from the local * domain and convert them to GIDs. @@ -429,14 +496,13 @@ errno_t get_gids_from_pac(TALLOC_CTX *mem_ctx, struct local_mapping_ranges *range_map, struct dom_sid *domain_sid, struct PAC_LOGON_INFO *logon_info, - size_t *_gid_count, struct pac_grp **_gids) + size_t *_gid_count, struct pac_dom_grps **_gids) { int ret; - size_t g; size_t gid_count = 0; size_t s; struct netr_SamInfo3 *info3; - struct pac_grp *gids = NULL; + struct pac_dom_grps *gids = NULL; struct sss_domain_info *grp_dom; char *sid_str = NULL; enum idmap_error_code err; @@ -445,8 +511,6 @@ errno_t get_gids_from_pac(TALLOC_CTX *mem_ctx, hash_table_t *gid_table; hash_key_t key; hash_value_t value; - struct hash_iter_context_t *iter; - hash_entry_t *entry; TALLOC_CTX *tmp_ctx = NULL; if (pac_ctx == NULL || range_map == NULL || domain_sid == NULL || @@ -470,8 +534,17 @@ errno_t get_gids_from_pac(TALLOC_CTX *mem_ctx, goto done; } - ret = sss_hash_create(tmp_ctx, info3->sidcount + info3->base.groups.count, - &gid_table); + /* Currently three group containers are allocated, one for the IPA domain, one + * for the trusted AD domain and an empty one to indicate the end of the + * list. */ + gids = talloc_zero_array(tmp_ctx, struct pac_dom_grps, 3); + if (gids == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero_array failed.\n")); + ret = ENOMEM; + goto done; + } + + ret = sss_hash_create(tmp_ctx, info3->sidcount, &gid_table); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, ("sss_hash_create failed.\n")); goto done; @@ -519,6 +592,20 @@ errno_t get_gids_from_pac(TALLOC_CTX *mem_ctx, } } + ret = get_dom_grps_from_hash(gids, gid_table, grp_dom, &gids[0]); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, ("get_dom_grps_from_hash failed.\n")); + goto done; + } + gid_count += gids[0].gid_count; + + talloc_free(gid_table); + ret = sss_hash_create(tmp_ctx, info3->base.groups.count, &gid_table); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, ("sss_hash_create failed.\n")); + goto done; + } + talloc_zfree(sid_str); err = sss_idmap_smb_sid_to_sid(pac_ctx->idmap_ctx, info3->base.domain_sid, &sid_str); @@ -571,40 +658,12 @@ errno_t get_gids_from_pac(TALLOC_CTX *mem_ctx, DEBUG(SSSDBG_TRACE_ALL, ("Found extra group with gid [%d].\n", id)); } - gid_count = hash_count(gid_table); - if (gid_count == 0) { - DEBUG(SSSDBG_TRACE_FUNC, ("No groups found.\n")); - ret = EOK; - goto done; - } - - gids = talloc_zero_array(tmp_ctx, struct pac_grp, gid_count); - if (gids == NULL) { - DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n")); - ret = ENOMEM; - goto done; - } - - iter = new_hash_iter_context(gid_table); - if (iter == NULL) { - DEBUG(SSSDBG_OP_FAILURE, ("new_hash_iter_context failed.\n")); - ret = EIO; - goto done; - } - - g = 0; - while ((entry = iter->next(iter)) != NULL) { - gids[g].gid = entry->key.ul; - gids[g].grp_dom = entry->value.ptr; - g++; - } - - if (gid_count != g) { - DEBUG(SSSDBG_OP_FAILURE, ("Number of hash entries and groups do not " - "match.\n")); - ret = EINVAL; + ret = get_dom_grps_from_hash(gids, gid_table, grp_dom, &gids[1]); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, ("get_dom_grps_from_hash failed.\n")); goto done; } + gid_count += gids[1].gid_count; ret = EOK; @@ -811,24 +870,26 @@ errno_t diff_gid_lists(TALLOC_CTX *mem_ctx, size_t cur_grp_num, struct grp_info *cur_grp_list, size_t new_gid_num, - struct pac_grp *new_gid_list, + struct pac_dom_grps *new_gid_list, size_t *_add_gid_num, - struct pac_grp **_add_gid_list, + struct pac_dom_grps **_add_gid_list, size_t *_del_grp_num, struct grp_info ***_del_grp_list) { int ret; size_t c; + size_t g; hash_table_t *table; hash_key_t key; hash_value_t value; size_t add_gid_num = 0; - struct pac_grp *add_gid_list = NULL; + struct pac_dom_grps *add_gid_list = NULL; size_t del_grp_num = 0; struct grp_info **del_grp_list = NULL; TALLOC_CTX *tmp_ctx = NULL; unsigned long value_count; hash_value_t *values; + size_t new_dom_num = 0; if ((cur_grp_num != 0 && cur_grp_list == NULL) || (new_gid_num != 0 && new_gid_list == NULL)) { @@ -848,17 +909,37 @@ errno_t diff_gid_lists(TALLOC_CTX *mem_ctx, goto done; } + if (new_gid_num != 0) { + for (new_dom_num = 0; new_gid_list[new_dom_num].grp_dom != NULL; + new_dom_num++); + } + if (cur_grp_num == 0 && new_gid_num != 0) { add_gid_num = new_gid_num; - add_gid_list = talloc_array(tmp_ctx, struct pac_grp, add_gid_num); + add_gid_list = talloc_zero_array(tmp_ctx, struct pac_dom_grps, + new_dom_num + 1); if (add_gid_list == NULL) { DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n")); ret = ENOMEM; goto done; } - for (c = 0; c < add_gid_num; c++) { - add_gid_list[c] = new_gid_list[c]; + for (c = 0; c < new_dom_num; c++) { + add_gid_list[c].grp_dom = new_gid_list[c].grp_dom; + add_gid_list[c].gid_count = new_gid_list[c].gid_count; + if (new_gid_list[c].gid_count != 0) { + add_gid_list[c].gids = talloc_zero_array(add_gid_list, gid_t, + new_gid_list[c].gid_count); + if (add_gid_list[c].gids == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero_array failed.\n")); + ret = ENOMEM; + goto done; + } + + for (g = 0; g < new_gid_list[c].gid_count; g++) { + add_gid_list[c].gids[g] = new_gid_list[c].gids[g]; + } + } } ret = EOK; @@ -904,27 +985,45 @@ errno_t diff_gid_lists(TALLOC_CTX *mem_ctx, } } - for (c = 0; c < new_gid_num; c++) { - key.ul = (unsigned long) new_gid_list[c].gid; + add_gid_list = talloc_zero_array(tmp_ctx, struct pac_dom_grps, + new_dom_num + 1); + if (add_gid_list == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n")); + ret = ENOMEM; + goto done; + } - ret = hash_delete(table, &key); - if (ret == HASH_ERROR_KEY_NOT_FOUND) { - /* gid not found, must be added */ - add_gid_num++; - add_gid_list = talloc_realloc(tmp_ctx, add_gid_list, struct pac_grp, - add_gid_num); - if (add_gid_list == NULL) { - DEBUG(SSSDBG_OP_FAILURE, ("talloc_realloc failed.\n")); - ret = ENOMEM; + for (c = 0; c < new_dom_num; c++) { + add_gid_list[c].grp_dom = new_gid_list[c].grp_dom; + add_gid_list[c].gid_count = 0; + + for (g = 0; g < new_gid_list[c].gid_count; g++) { + key.ul = (unsigned long) new_gid_list[c].gids[g]; + + ret = hash_delete(table, &key); + if (ret == HASH_ERROR_KEY_NOT_FOUND) { + /* gid not found, must be added */ + add_gid_list[c].gid_count++; + add_gid_list[c].gids = talloc_realloc(add_gid_list, + add_gid_list, + gid_t, + add_gid_list[c].gid_count); + if (add_gid_list[c].gids == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_realloc failed.\n")); + ret = ENOMEM; + goto done; + } + + add_gid_list[c].gids[add_gid_list[c].gid_count - 1] = + new_gid_list[c].gids[g]; + } else if (ret != HASH_SUCCESS) { + DEBUG(SSSDBG_OP_FAILURE, ("hash_delete failed.\n")); + ret = EIO; goto done; } - - add_gid_list[add_gid_num - 1] = new_gid_list[c]; - } else if (ret != HASH_SUCCESS) { - DEBUG(SSSDBG_OP_FAILURE, ("hash_delete failed.\n")); - ret = EIO; - goto done; } + + add_gid_num += add_gid_list[c].gid_count; } /* the remaining entries in the hash are not in the new list anymore and diff --git a/src/tests/pac_responder-tests.c b/src/tests/pac_responder-tests.c index b2a145784..7d352a0e8 100644 --- a/src/tests/pac_responder-tests.c +++ b/src/tests/pac_responder-tests.c @@ -172,16 +172,25 @@ START_TEST(pac_test_get_gids_to_add_and_remove) int ret; size_t c; size_t add_gid_count = 0; - struct pac_grp *add_gids = NULL; + struct pac_dom_grps *add_gids = NULL; size_t del_gid_count = 0; struct grp_info **del_gids = NULL; + struct sss_domain_info grp_dom; - struct pac_grp pac_grp_2 = {2, NULL}; - struct pac_grp pac_grp_3 = {3, NULL}; + memset(&grp_dom, 0, sizeof(grp_dom)); - struct pac_grp gid_list_2[] = {pac_grp_2}; - struct pac_grp gid_list_3[] = {pac_grp_3}; - struct pac_grp gid_list_23[] = {pac_grp_2, pac_grp_3}; + gid_t gid_list_2[] = {2}; + gid_t gid_list_3[] = {3}; + gid_t gid_list_23[] = {2, 3}; + struct pac_dom_grps empty_dom = {NULL, 0, NULL}; + + struct pac_dom_grps pac_grp_2 = {&grp_dom, 1, gid_list_2}; + struct pac_dom_grps pac_grp_3 = {&grp_dom, 1, gid_list_3}; + struct pac_dom_grps pac_grp_23 = {&grp_dom, 2, gid_list_23}; + + struct pac_dom_grps dom_grp_list_2[] = {pac_grp_2, empty_dom}; + struct pac_dom_grps dom_grp_list_3[] = {pac_grp_3, empty_dom}; + struct pac_dom_grps dom_grp_list_23[] = {pac_grp_23, empty_dom}; struct grp_info grp_info_1 = {1, NULL, NULL}; struct grp_info grp_info_2 = {2, NULL, NULL}; @@ -192,18 +201,18 @@ START_TEST(pac_test_get_gids_to_add_and_remove) size_t cur_gid_count; struct grp_info *cur_gids; size_t gid_count; - struct pac_grp *gids; + struct pac_dom_grps *gids; int exp_ret; size_t exp_add_gid_count; - struct pac_grp *exp_add_gids; + struct pac_dom_grps *exp_add_gids; size_t exp_del_gid_count; struct grp_info *exp_del_gids; } a_and_r_data[] = { - {1, grp_list_1, 1, gid_list_2, EOK, 1, gid_list_2, 1, grp_list_1}, + {1, grp_list_1, 1, dom_grp_list_2, EOK, 1, dom_grp_list_2, 1, grp_list_1}, {1, grp_list_1, 0, NULL, EOK, 0, NULL, 1, grp_list_1}, - {0, NULL, 1, gid_list_2, EOK, 1, gid_list_2, 0, NULL}, - {2, grp_list_12, 1, gid_list_2, EOK, 0, NULL, 1, grp_list_1}, - {2, grp_list_12, 2, gid_list_23, EOK, 1, gid_list_3, 1, grp_list_1}, + {0, NULL, 1, dom_grp_list_2, EOK, 1, dom_grp_list_2, 0, NULL}, + {2, grp_list_12, 1, dom_grp_list_2, EOK, 0, NULL, 1, grp_list_1}, + {2, grp_list_12, 2, dom_grp_list_23, EOK, 1, dom_grp_list_3, 1, grp_list_1}, {0, NULL, 0, NULL, 0, 0, NULL, 0, NULL} }; @@ -254,10 +263,10 @@ START_TEST(pac_test_get_gids_to_add_and_remove) * only look at lists with 1 element. TODO: add code to compare lists * with more than 1 member. */ if (add_gid_count == 1) { - fail_unless(add_gids[0].gid == a_and_r_data[c].exp_add_gids[0].gid, + fail_unless(add_gids[0].gids[0] == a_and_r_data[c].exp_add_gids[0].gids[0], "Unexpected gid to add for test data #%d, " \ "expected [%d], got [%d]", - c, a_and_r_data[c].exp_add_gids[0].gid, add_gids[0].gid); + c, a_and_r_data[c].exp_add_gids[0].gids[0], add_gids[0].gids[0]); } if (del_gid_count == 1) { @@ -326,10 +335,11 @@ START_TEST(pac_test_get_gids_from_pac) { int ret; size_t c; + size_t d; size_t g; size_t t; size_t gid_count; - struct pac_grp *gids; + struct pac_dom_grps *gids; struct PAC_LOGON_INFO *logon_info; bool found; gid_t exp_gid; @@ -380,7 +390,7 @@ START_TEST(pac_test_get_gids_from_pac) found = false; exp_gid = IDMAP_RANGE_MIN + 500 + c; for (g = 0; g < gid_count; g++) { - if (gids[g].gid == exp_gid) { + if (gids[1].gids[g] == exp_gid) { found = true; break; } @@ -401,8 +411,8 @@ START_TEST(pac_test_get_gids_from_pac) fail_unless(ret == EOK, "Failed with 10 duplicated RIDs in PAC"); fail_unless(gid_count == 1, "[%d] groups expected, got [%d]", 1, gid_count); fail_unless(gids != NULL, "Expected gid array."); - fail_unless(gids[0].gid == IDMAP_RANGE_MIN + 500, - "Wrong gid returned, got [%d], expected [%d].", gids[0].gid, + fail_unless(gids[1].gids[0] == IDMAP_RANGE_MIN + 500, + "Wrong gid returned, got [%d], expected [%d].", gids[1].gids[0], IDMAP_RANGE_MIN + 500); talloc_free(gids); gids = NULL; @@ -425,9 +435,14 @@ START_TEST(pac_test_get_gids_from_pac) for (c = 0; exp_gids[c] != 0; c++) { found = false; - for (g = 0; g < gid_count; g++) { - if (gids[g].gid == exp_gids[c]) { - found = true; + for (d = 0; d < 2; d++) { + for (g = 0; g < gids[d].gid_count; g++) { + if (gids[d].gids[g] == exp_gids[c]) { + found = true; + break; + } + } + if (found) { break; } } -- cgit