URL: https://github.com/SSSD/sssd/pull/5407 Author: ikerexxe Title: #5407: kcm: check socket path loaded from configuration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5407/head:pr5407 git checkout pr5407
From 318ad7e7d8fba67d4b84fd410ed01ff7991e37ae Mon Sep 17 00:00:00 2001 From: ikerexxe <ipedr...@redhat.com> Date: Tue, 17 Nov 2020 12:17:28 +0100 Subject: [PATCH 1/3] confdb: log message when falling back to default Log message when string attribute is not found in database and value falls back to default. Moreover, implement unit-test for confdb_get_string() method. Resolves: https://github.com/SSSD/sssd/issues/5406 --- src/confdb/confdb.c | 3 + src/tests/cmocka/confdb/test_confdb.c | 91 +++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index d2fc018fd0..7254d8c5ae 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -411,6 +411,9 @@ int confdb_get_string(struct confdb_ctx *cdb, TALLOC_CTX *ctx, return EOK; } + DEBUG(SSSDBG_CONF_SETTINGS, + "Unable to get [%s] from [%s], falling back to default [%s]\n", + attribute, section, defstr); /* Copy the default string */ restr = talloc_strdup(ctx, defstr); } diff --git a/src/tests/cmocka/confdb/test_confdb.c b/src/tests/cmocka/confdb/test_confdb.c index daff41e5cc..7851945878 100644 --- a/src/tests/cmocka/confdb/test_confdb.c +++ b/src/tests/cmocka/confdb/test_confdb.c @@ -251,6 +251,88 @@ static void test_confdb_get_enabled_domain_list(void **state) } +static void test_confdb_get_string_value_present(void **state) +{ + /* + * Given config_file_version exists in the configuration database + * And a default value is defined + * When config_file_version is requested + * Then config_file_version value is returned + */ + struct test_ctx *test_ctx = talloc_get_type(*state, struct test_ctx); + TALLOC_CTX* tmp_ctx = talloc_new(NULL); + char *confdb_value; + int ret = EOK; + + ret = confdb_get_string(test_ctx->confdb, + tmp_ctx, + "config/sssd", + "config_file_version", + NULL, + &confdb_value); + + assert_int_equal(EOK, ret); + assert_string_equal("2", confdb_value); + + TALLOC_FREE(tmp_ctx); +} + + +static void test_confdb_get_string_value_not_present_fallback_default(void **state) +{ + /* + * Given non_existing_value doesn't exist in the configuration database + * And a default value is defined + * When non_existing_value is requested + * Then default value is returned + */ + struct test_ctx *test_ctx = talloc_get_type(*state, struct test_ctx); + TALLOC_CTX* tmp_ctx = talloc_new(NULL); + char *confdb_value; + const char *default_value = "3"; + int ret = EOK; + + ret = confdb_get_string(test_ctx->confdb, + tmp_ctx, + "config/sssd", + "non_existing_value", + default_value, + &confdb_value); + + assert_int_equal(EOK, ret); + assert_string_equal(default_value, confdb_value); + + TALLOC_FREE(tmp_ctx); +} + + +static void test_confdb_get_string_value_and_default_not_present(void **state) +{ + /* + * Given non_existing_value doesn't exist in the configuration database + * And a default value isn't defined + * When non_existing_value is requested + * Then NULL is returned + */ + struct test_ctx *test_ctx = talloc_get_type(*state, struct test_ctx); + TALLOC_CTX* tmp_ctx = talloc_new(NULL); + char *confdb_value; + int ret = EOK; + + ret = confdb_get_string(test_ctx->confdb, + tmp_ctx, + "config/sssd", + "non_existing_value", + NULL, + &confdb_value); + + assert_int_equal(EOK, ret); + assert_null(confdb_value); + + TALLOC_FREE(tmp_ctx); +} + + int main(int argc, const char *argv[]) { poptContext pc; @@ -272,6 +354,15 @@ int main(int argc, const char *argv[]) cmocka_unit_test_setup_teardown(test_confdb_get_enabled_domain_list, confdb_test_setup, confdb_test_teardown), + cmocka_unit_test_setup_teardown(test_confdb_get_string_value_present, + confdb_test_setup, + confdb_test_teardown), + cmocka_unit_test_setup_teardown(test_confdb_get_string_value_not_present_fallback_default, + confdb_test_setup, + confdb_test_teardown), + cmocka_unit_test_setup_teardown(test_confdb_get_string_value_and_default_not_present, + confdb_test_setup, + confdb_test_teardown), }; /* Set debug level to invalid value so we can decide if -d 0 was used. */ From 483c4e40e6bc52e326e745ffcba5964ec4e3189d Mon Sep 17 00:00:00 2001 From: ikerexxe <ipedr...@redhat.com> Date: Tue, 17 Nov 2020 12:27:57 +0100 Subject: [PATCH 2/3] util: extract substring from string Implement a method to extract a substring from the start of a string until the last match of a given character. Besides, implement unit-tests to check that it works as expected. --- src/tests/cmocka/test_string_utils.c | 47 ++++++++++++++++++++++++++++ src/tests/cmocka/test_utils.c | 1 + src/tests/cmocka/test_utils.h | 1 + src/util/string_utils.c | 19 +++++++++++ src/util/util.h | 18 +++++++++++ 5 files changed, 86 insertions(+) diff --git a/src/tests/cmocka/test_string_utils.c b/src/tests/cmocka/test_string_utils.c index 57e6f2617b..7f884ef281 100644 --- a/src/tests/cmocka/test_string_utils.c +++ b/src/tests/cmocka/test_string_utils.c @@ -269,3 +269,50 @@ void test_concatenate_string_array(void **state) assert_true(check_leaks_pop(mem_ctx) == true); talloc_free(mem_ctx); } + +void test_extract_string_last_char_match(void **state) +{ + TALLOC_CTX *mem_ctx; + const char char_to_match = '/'; + const char *simple_correct_string = "/tmp/abc"; + const char *complex_correct_string = "/tmp/abc/def/ghi/jk"; + const char *current_string = "./"; + const char *root_string = "/"; + const char *wrong_string = "blablablablablablablabla"; + const char *substring; + + mem_ctx = talloc_new(NULL); + assert_non_null(mem_ctx); + check_leaks_push(mem_ctx); + + substring = extract_string_last_char_match(mem_ctx, + simple_correct_string, + char_to_match); + assert_string_equal("/tmp", substring); + talloc_free(substring); + + substring = extract_string_last_char_match(mem_ctx, + complex_correct_string, + char_to_match); + assert_string_equal("/tmp/abc/def/ghi", substring); + talloc_free(substring); + + substring = extract_string_last_char_match(mem_ctx, + current_string, + char_to_match); + assert_string_equal(".", substring); + talloc_free(substring); + + substring = extract_string_last_char_match(mem_ctx, + root_string, + char_to_match); + assert_null(substring); + + substring = extract_string_last_char_match(mem_ctx, + wrong_string, + char_to_match); + assert_null(substring); + + assert_true(check_leaks_pop(mem_ctx) == true); + talloc_free(mem_ctx); +} \ No newline at end of file diff --git a/src/tests/cmocka/test_utils.c b/src/tests/cmocka/test_utils.c index 945f5cb44a..2a17c62625 100644 --- a/src/tests/cmocka/test_utils.c +++ b/src/tests/cmocka/test_utils.c @@ -2075,6 +2075,7 @@ int main(int argc, const char *argv[]) cmocka_unit_test(test_guid_blob_to_string_buf), cmocka_unit_test(test_get_last_x_chars), cmocka_unit_test(test_concatenate_string_array), + cmocka_unit_test(test_extract_string_last_char_match), cmocka_unit_test_setup_teardown(test_add_strings_lists, setup_leak_tests, teardown_leak_tests), diff --git a/src/tests/cmocka/test_utils.h b/src/tests/cmocka/test_utils.h index 44b9479f96..c766cd976a 100644 --- a/src/tests/cmocka/test_utils.h +++ b/src/tests/cmocka/test_utils.h @@ -32,6 +32,7 @@ void test_reverse_replace_whitespaces(void **state); void test_guid_blob_to_string_buf(void **state); void test_get_last_x_chars(void **state); void test_concatenate_string_array(void **state); +void test_extract_string_last_char_match(void **state); /* from src/tests/cmocka/test_sss_ptr_hash.c */ void test_sss_ptr_hash_with_free_cb(void **state); diff --git a/src/util/string_utils.c b/src/util/string_utils.c index 1215ec96a5..847e507df1 100644 --- a/src/util/string_utils.c +++ b/src/util/string_utils.c @@ -146,3 +146,22 @@ char **concatenate_string_array(TALLOC_CTX *mem_ctx, return string_array; } + +const char *extract_string_last_char_match(TALLOC_CTX *mem_ctx, + const char *string, + const char character) { + char *substring = NULL; + int pos; + + for (pos = strlen(string); pos > 0; pos--) { + if(string[pos] == '/') { + break; + } + } + + if (pos > 0) { + substring = talloc_strndup(mem_ctx, string, pos); + } + + return substring; +} \ No newline at end of file diff --git a/src/util/util.h b/src/util/util.h index fbcac5cd09..42f89c7109 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -691,6 +691,24 @@ char **concatenate_string_array(TALLOC_CTX *mem_ctx, char **arr1, size_t len1, char **arr2, size_t len2); +/** + * @brief Extract a substring until the last match of a character + * + * Extract a substring from the start of a string until the last match of a + * given character. + * + * @param[in] mem_ctx Talloc memory context for the new list. + * @param[in] string String to extract from. + * @param[in] character Character to find. + * + * @return If the character is found then return substring from + * the start until the last match of the character. If + * it isn't found, then return NULL. + */ +const char *extract_string_last_char_match(TALLOC_CTX *mem_ctx, + const char *string, + const char character); + /* from become_user.c */ errno_t become_user(uid_t uid, gid_t gid); struct sss_creds; From c34a235c0e73cad4e16020144d63936c8920d5f4 Mon Sep 17 00:00:00 2001 From: ikerexxe <ipedr...@redhat.com> Date: Tue, 17 Nov 2020 12:48:26 +0100 Subject: [PATCH 3/3] kcm: check socket path loaded from configuration Check the existence of the socket path loaded from configuration. If it doesn't contain a valid path, then log it and fail. Besides, implement an integration test that checks if sssd_kcm fails when kcm socket is defined in an invalid location. Resolves: https://github.com/SSSD/sssd/issues/5406 --- src/responder/kcm/kcm.c | 28 ++++++++++++++++++++ src/tests/intg/test_kcm.py | 52 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/src/responder/kcm/kcm.c b/src/responder/kcm/kcm.c index f1fc958be2..f6c1a8d450 100644 --- a/src/responder/kcm/kcm.c +++ b/src/responder/kcm/kcm.c @@ -87,6 +87,14 @@ static int kcm_get_config(struct kcm_ctx *kctx) { int ret; char *sock_name; + const char *sock_path; + struct stat stat_info; + TALLOC_CTX *tmp_ctx = NULL; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return ENOMEM; + } ret = confdb_get_int(kctx->rctx->cdb, CONFDB_KCM_CONF_ENTRY, @@ -128,6 +136,25 @@ static int kcm_get_config(struct kcm_ctx *kctx) ret, strerror(ret)); goto done; } + + sock_path = extract_string_last_char_match(tmp_ctx, sock_name, '/'); + if (sock_path != NULL) { + ret = stat (sock_path, &stat_info); + + if (ret != 0) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Folder defined in KCM [%s = %s] doesn't exist\n", + CONFDB_KCM_SOCKET, sock_name); + ret = ENOENT; + goto done; + } + } else { + DEBUG(SSSDBG_CRIT_FAILURE, + "KCM [%s = %s] doesn't contain a valid path\n", + CONFDB_KCM_SOCKET, sock_name); + ret = ENOENT; + goto done; + } kctx->rctx->sock_name = sock_name; ret = kcm_get_ccdb_be(kctx); @@ -156,6 +183,7 @@ static int kcm_get_config(struct kcm_ctx *kctx) } ret = EOK; done: + talloc_free(tmp_ctx); return ret; } diff --git a/src/tests/intg/test_kcm.py b/src/tests/intg/test_kcm.py index 3a43491b96..0ec7f8007b 100644 --- a/src/tests/intg/test_kcm.py +++ b/src/tests/intg/test_kcm.py @@ -585,3 +585,55 @@ def test_kcm_secrets_quota(setup_for_kcm_sec, princ = "%s%d" % ("kcmtest", MAX_SECRETS) out, _, _ = testenv.k5util.kinit(princ, princ) assert out != 0 + + +@pytest.fixture +def setup_kcm_non_existing_socket_path(request, kdc_instance): + """ + Just set up the local provider for tests and enable the KCM + responder + """ + sec_resp_path = os.path.join(config.LIBEXEC_PATH, "sssd", "sssd_secrets") + if not os.access(sec_resp_path, os.X_OK): + # It would be cleaner to use pytest.mark.skipif on the package level + # but upstream insists on supporting RHEL-6. + pytest.skip("No Secrets responder, skipping") + + kcm_path = os.path.join("/non_existing", "kcm.socket") + sssd_conf = create_sssd_conf(kcm_path, "secrets") + + kcm_socket_include = unindent(""" + [libdefaults] + default_ccache_name = KCM: + kcm_socket = {kcm_path} + """).format(**locals()) + kdc_instance.add_config({'kcm_socket': kcm_socket_include}) + + create_conf_fixture(request, sssd_conf) + + +@pytest.mark.skipif(sys.version_info < (3,3), reason="requires python3.3") +def test_kcm_non_existing_socket_path(setup_kcm_non_existing_socket_path): + ''' + Given kcm socket is defined in an invalid location + When sssd_kcm is launched + Then sssd_kcm logs failure and fails + ''' + if subprocess.call(['sssd', "--genconf"]) != 0: + raise Exception("failed to regenerate confdb") + + resp_path = os.path.join(config.LIBEXEC_PATH, "sssd", "sssd_kcm") + if not os.access(resp_path, os.X_OK): + # It would be cleaner to use pytest.mark.skipif on the package level + # but upstream insists on supporting RHEL-6.. + pytest.skip("No KCM responder, skipping") + + try: + subprocess.check_output([resp_path, "--uid=0", "--gid=0"], + stderr=subprocess.STDOUT, timeout=10) + except subprocess.CalledProcessError as e: + assert e.returncode != 0 + except subprocess.TimeoutExpired as e: + # If sssd_kcm time outs it means that the process continues + # its execution and this shouldn't happen + assert "", "Time out, shouldn't arrive to this point"
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
