Hi, here is patch for ticket #3161.
See more in the ticket description. I was thinking why we originally replaced the lists and I think it comes from confusion on how we handle the same keys in single GPO ini file, however that is handled by libini not by SSSD. Michal
>From f1a54ec427fe109c8c166e76615eef2d4d83433b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Tue, 30 Aug 2016 20:03:36 +0200 Subject: [PATCH] GPO: Cat vals with same key from different GPOs Concatenate values from different GPO files that have the same key. This allows to specify GPO deny and allow rules across several GPO files. Previously we replaced values with the same key which caused access denials even to users that were supposed login. Moreover, if the deny rules were specified across several GPO files, this bug could allow users to log into machines they were not supposed to, because we did not create the whole list of denied SIDs and only worked with the part specified in the last processed GPO file. Resolves: https://fedorahosted.org/sssd/ticket/3161 --- src/db/sysdb_gpo.c | 31 +++++++++++++++---------------- src/tests/sysdb-tests.c | 14 ++------------ 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/db/sysdb_gpo.c b/src/db/sysdb_gpo.c index e5af91b..583ba86 100644 --- a/src/db/sysdb_gpo.c +++ b/src/db/sysdb_gpo.c @@ -391,6 +391,7 @@ sysdb_gpo_store_gpo_result_setting(struct sss_domain_info *domain, size_t count; bool in_transaction = false; TALLOC_CTX *tmp_ctx; + const char *old_value = NULL; tmp_ctx = talloc_new(NULL); if (!tmp_ctx) return ENOMEM; @@ -479,22 +480,20 @@ sysdb_gpo_store_gpo_result_setting(struct sss_domain_info *domain, goto done; } - lret = ldb_msg_add_fmt(update_msg, ini_key, "%s", ini_value); - if (lret != LDB_SUCCESS) { - ret = sysdb_error_to_errno(lret); - goto done; - } - } else { - /* If the value is NULL, we need to remove it from the cache */ - DEBUG(SSSDBG_TRACE_FUNC, "Removing setting: key [%s]\n", ini_key); - - /* Update the policy setting */ - lret = ldb_msg_add_empty(update_msg, ini_key, - LDB_FLAG_MOD_DELETE, - NULL); - if (lret != LDB_SUCCESS) { - ret = sysdb_error_to_errno(lret); - goto done; + old_value = ldb_msg_find_attr_as_string(msgs[0], ini_key, NULL); + if (old_value) { + /* Concatenate the old and new values */ + lret = ldb_msg_add_fmt(update_msg, ini_key, "%s,%s", old_value, ini_value); + if (lret != LDB_SUCCESS) { + ret = sysdb_error_to_errno(lret); + goto done; + } + } else { + lret = ldb_msg_add_fmt(update_msg, ini_key, "%s", ini_value); + if (lret != LDB_SUCCESS) { + ret = sysdb_error_to_errno(lret); + goto done; + } } } diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c index d145001..72a8dd0 100644 --- a/src/tests/sysdb-tests.c +++ b/src/tests/sysdb-tests.c @@ -6341,7 +6341,7 @@ START_TEST(test_gpo_result) ck_assert_int_eq(ret, EOK); fail_unless(value == NULL); - /* Updating replaces the original value */ + /* Updating concatenates the original and the new value */ ret = sysdb_gpo_store_gpo_result_setting(test_ctx->domain, allow_key, "allow_val2"); ck_assert_int_eq(ret, EOK); @@ -6349,17 +6349,7 @@ START_TEST(test_gpo_result) ret = sysdb_gpo_get_gpo_result_setting(test_ctx, test_ctx->domain, allow_key, &value); ck_assert_int_eq(ret, EOK); - ck_assert_str_eq(value, "allow_val2"); - - /* NULL removes the value completely */ - ret = sysdb_gpo_store_gpo_result_setting(test_ctx->domain, - allow_key, NULL); - ck_assert_int_eq(ret, EOK); - - ret = sysdb_gpo_get_gpo_result_setting(test_ctx, test_ctx->domain, - allow_key, &value); - ck_assert_int_eq(ret, EOK); - fail_unless(value == NULL); + ck_assert_str_eq(value, "allow_val1,allow_val2"); /* Delete the result */ ret = sysdb_gpo_delete_gpo_result_object(test_ctx, test_ctx->domain); -- 2.5.0
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
