URL: https://github.com/SSSD/sssd/pull/717 Author: jhrozek Title: #717: NSS: Avoid changing the memory cache ownership away from the sssd user (sssd-1-16 backport) Action: opened
PR body: """ Resolves: https://pagure.io/SSSD/sssd/issue/3890 In case SSSD is compiled --with-sssd-user but run as root (which is the default on RHEL and derivatives), then the memory cache will be owned by the user that sssd_nss runs as, so root. This conflicts with the packaging which specifies sssd.sssd as the owner. And in turn, this means that users can't reliably assess the package integrity using rpm -V. This patch makes sure that the memory cache files are chowned to sssd.sssd even if the nss responder runs as root. Also, this patch changes the sssd_nss responder so that is becomes a member of the supplementary sssd group. Even though in traditional UNIX sense, a process running as root could write to a file owned by sssd:sssd, with SELinux enforcing mode this becomes problematic as SELinux emits an error such as: type=AVC msg=audit(1543524888.125:1495): avc: denied { fsetid } for pid=7706 comm="sssd_nss" capability=4 scontext=system_u:system_r:sssd_t:s0 tcontext=system_u:system_r:sssd_t:s0 tclass=capability To make it possible for the sssd_nss process to write to the files, the files are also made group-writable. The 'others' permission is still set to read only. Reviewed-by: Michal Židek <mzi...@redhat.com> (cherry picked from commit 61e4ba58934b20a950255e05797aca25aadc1242) """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/717/head:pr717 git checkout pr717
From e7e942ceb1f8402d00f5f14a9e065d3fc434b711 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Thu, 23 Aug 2018 13:55:51 +0200 Subject: [PATCH 01/19] SELINUX: Always add SELinux user to the semanage database if it doesn't exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, we tried to optimize too much and only set the SELinux user to Linux user mapping in case the SELinux user was different from the system default. But this doesn't work for the case where the Linux user has a non-standard home directory, because then SELinux would not have any idea that this user's home directory should be labeled as a home directory. This patch relaxes the optimization in the sense that on the first login, the SELinux context is saved regardless of whether it is the same as the default or different. Resolves: https://pagure.io/SSSD/sssd/issue/3819 Reviewed-by: Michal Židek <mzi...@redhat.com> (cherry picked from commit 945865ae16120ffade267227ca48cefd58822fd2) --- src/providers/ipa/selinux_child.c | 10 ++++++++-- src/util/sss_semanage.c | 30 ++++++++++++++++++++++++++++++ src/util/util.h | 1 + src/util/util_errors.c | 1 + src/util/util_errors.h | 1 + 5 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/providers/ipa/selinux_child.c b/src/providers/ipa/selinux_child.c index d061417a5..925591ec9 100644 --- a/src/providers/ipa/selinux_child.c +++ b/src/providers/ipa/selinux_child.c @@ -176,13 +176,16 @@ static bool seuser_needs_update(const char *username, ret = sss_get_seuser(username, &db_seuser, &db_mls_range); DEBUG(SSSDBG_TRACE_INTERNAL, - "getseuserbyname: ret: %d seuser: %s mls: %s\n", + "sss_get_seuser: ret: %d seuser: %s mls: %s\n", ret, db_seuser ? db_seuser : "unknown", db_mls_range ? db_mls_range : "unknown"); if (ret == EOK && db_seuser && db_mls_range && strcmp(db_seuser, seuser) == 0 && strcmp(db_mls_range, mls_range) == 0) { - needs_update = false; + ret = sss_seuser_exists(username); + if (ret == EOK) { + needs_update = false; + } } /* OR */ if (ret == ERR_SELINUX_NOT_MANAGED) { @@ -191,6 +194,9 @@ static bool seuser_needs_update(const char *username, free(db_seuser); free(db_mls_range); + DEBUG(SSSDBG_TRACE_FUNC, + "The SELinux user does %sneed an update\n", + needs_update ? "" : "not "); return needs_update; } diff --git a/src/util/sss_semanage.c b/src/util/sss_semanage.c index bcce57b60..aea03852a 100644 --- a/src/util/sss_semanage.c +++ b/src/util/sss_semanage.c @@ -248,6 +248,36 @@ static int sss_semanage_user_mod(semanage_handle_t *handle, return ret; } +int sss_seuser_exists(const char *linuxuser) +{ + int ret; + int exists; + semanage_seuser_key_t *sm_key = NULL; + semanage_handle_t *sm_handle = NULL; + + ret = sss_semanage_init(&sm_handle); + if (ret != EOK) { + return ret; + } + + ret = semanage_seuser_key_create(sm_handle, linuxuser, &sm_key); + if (ret < 0) { + sss_semanage_close(sm_handle); + return EIO; + } + + ret = semanage_seuser_exists(sm_handle, sm_key, &exists); + semanage_seuser_key_free(sm_key); + sss_semanage_close(sm_handle); + if (ret < 0) { + return EIO; + } + + DEBUG(SSSDBG_TRACE_FUNC, "seuser exists: %s\n", exists ? "yes" : "no"); + + return exists ? EOK : ERR_SELINUX_USER_NOT_FOUND; +} + int sss_get_seuser(const char *linuxuser, char **selinuxuser, char **level) diff --git a/src/util/util.h b/src/util/util.h index bc89ecbc2..c78615b9c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -660,6 +660,7 @@ int sss_del_seuser(const char *login_name); int sss_get_seuser(const char *linuxuser, char **selinuxuser, char **level); +int sss_seuser_exists(const char *linuxuser); /* convert time from generalized form to unix time */ errno_t sss_utc_to_time_t(const char *str, const char *format, time_t *unix_time); diff --git a/src/util/util_errors.c b/src/util/util_errors.c index e2bb2a014..e5c5bd140 100644 --- a/src/util/util_errors.c +++ b/src/util/util_errors.c @@ -75,6 +75,7 @@ struct err_string error_to_str[] = { { "LDAP search returned a referral" }, /* ERR_REFERRAL */ { "Error setting SELinux user context" }, /* ERR_SELINUX_CONTEXT */ { "SELinux is not managed by libsemanage" }, /* ERR_SELINUX_NOT_MANAGED */ + { "SELinux user does not exist" }, /* ERR_SELINUX_USER_NOT_FOUND */ { "Username format not allowed by re_expression" }, /* ERR_REGEX_NOMATCH */ { "Time specification not supported" }, /* ERR_TIMESPEC_NOT_SUPPORTED */ { "Invalid SSSD configuration detected" }, /* ERR_INVALID_CONFIG */ diff --git a/src/util/util_errors.h b/src/util/util_errors.h index 49501727d..a4760a1db 100644 --- a/src/util/util_errors.h +++ b/src/util/util_errors.h @@ -97,6 +97,7 @@ enum sssd_errors { ERR_REFERRAL, ERR_SELINUX_CONTEXT, ERR_SELINUX_NOT_MANAGED, + ERR_SELINUX_USER_NOT_FOUND, ERR_REGEX_NOMATCH, ERR_TIMESPEC_NOT_SUPPORTED, ERR_INVALID_CONFIG, From 3dc885344e6c90a0db7a759b52f56c1b4a4f6d6d Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Mon, 10 Sep 2018 15:35:45 +0200 Subject: [PATCH 02/19] intg: flush the SSSD caches to sync with files To make sure that SSSD has synced with the latest data added to the passwd file sss_cache is called in two places where the current sync scheme was not reliable. This was mainly observed when running the integration tests on Debian. Reviewed-by: Jakub Hrozek <jhro...@redhat.com> (cherry picked from commit 1e2398870e8aa512ead3012d46cbe6252429467a) --- src/tests/intg/test_files_provider.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/tests/intg/test_files_provider.py b/src/tests/intg/test_files_provider.py index 9f30d2bb4..ead1cc4c3 100644 --- a/src/tests/intg/test_files_provider.py +++ b/src/tests/intg/test_files_provider.py @@ -644,6 +644,10 @@ def test_enum_users(setup_pw_with_canary, files_domain_only): user = user_generator(i) setup_pw_with_canary.useradd(**user) + # syncing with the help of the canary is not reliable after adding + # multiple users because the canary might still be in some caches so that + # the data is not refreshed properly. + subprocess.call(["sss_cache", "-E"]) sssd_getpwnam_sync(CANARY["name"]) user_list = call_sssd_enumeration() # +1 because the canary is added @@ -1043,6 +1047,10 @@ def test_getgrnam_add_remove_ghosts(setup_pw_with_canary, # Add this user and verify it's been added as a member pwd_ops.useradd(**USER2) + # The negative cache might still have user2 from the previous request, + # flushing the caches might help to prevent a failed lookup after adding + # the user. + subprocess.call(["sss_cache", "-E"]) res, groups = sssd_id_sync('user2') assert res == sssd_id.NssReturnCode.SUCCESS assert len(groups) == 2 From 2d9286102f23ea9d13213f1176ba669b9315a75f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> Date: Thu, 6 Sep 2018 13:38:56 +0200 Subject: [PATCH 03/19] sudo: respect case sensitivity in sudo responder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the domain is not case sensitive and the case of the original user or group name differs from the name in the rule we failed to find the rule. Now we filter the rule only with lower cased values in such domain. Steps to reproduce: 1. Add user/group with upper case, e.g. USER-1 2. Add sudo rule with lower cased name, e.g. sudoUser: user-1 3. Login to system with lower case, e.g. user-1 4. Run sudo -l Without the patch, rule is not found. Resolves: https://pagure.io/SSSD/sssd/issue/3820 Reviewed-by: Michal Židek <mzi...@redhat.com> (cherry picked from commit d7f0b58e2896ed2ef9ed5a390815c1e4df6caaee) --- src/db/sysdb_sudo.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/db/sysdb_sudo.c b/src/db/sysdb_sudo.c index 3ad462d8f..19ed97b86 100644 --- a/src/db/sysdb_sudo.c +++ b/src/db/sysdb_sudo.c @@ -418,7 +418,17 @@ sysdb_get_sudo_user_info(TALLOC_CTX *mem_ctx, ret = EINVAL; goto done; } - DEBUG(SSSDBG_TRACE_FUNC, "original name: %s\n", orig_name); + + DEBUG(SSSDBG_TRACE_FUNC, "Original name: %s\n", orig_name); + + orig_name = sss_get_cased_name(tmp_ctx, orig_name, domain->case_sensitive); + if (orig_name == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory!\n"); + ret = ENOMEM; + goto done; + } + + DEBUG(SSSDBG_TRACE_FUNC, "Cased name: %s\n", orig_name); if (_uid != NULL) { uid = ldb_msg_find_attr_as_uint64(res->msgs[0], SYSDB_UIDNUM, 0); @@ -450,8 +460,9 @@ sysdb_get_sudo_user_info(TALLOC_CTX *mem_ctx, continue; } - sysdb_groupnames[num_groups] = talloc_strdup(sysdb_groupnames, - groupname); + sysdb_groupnames[num_groups] = \ + sss_get_cased_name(sysdb_groupnames, groupname, + domain->case_sensitive); if (sysdb_groupnames[num_groups] == NULL) { DEBUG(SSSDBG_MINOR_FAILURE, "Cannot strdup %s\n", groupname); continue; From bca1935761a8ac21a45abcda9f82feec259dc65c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 2 Oct 2018 10:14:10 +0200 Subject: [PATCH 04/19] pep8: Ignore W504 and W605 to silence warnings on Debian MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This code: pkcs11_txt.write("library=libsoftokn3.so\nname=soft\n" + "parameters=configdir='sql:" + config.ABS_BUILDDIR + "/../test_CA/p11_nssdb' " + "dbSlotDescription='SSSD Test Slot' " + "dbTokenDescription='SSSD Test Token' " + "secmod='secmod.db' flags=readOnly)\n\n") pkcs11_txt.close() Was producing warnings such as: ./src/tests/intg/test_pam_responder.py:143:22: W504 line break after binary operator Even though it looks OK visually and conforms to pep8's written form. Additionaly, this regular expression compilation: Template = re.compile( ' *<template name="(\S+)">(.*?)</template>\r?\n?', re.MULTILINE | re.DOTALL ) Was producing a warning such as: ./src/sbus/codegen/sbus_Template.py:156:29: W605 invalid escape sequence '\S' Since the \S literal is part of a regular expression, let's suppress this warning as well. Reviewed-by: Michal Židek <mzi...@redhat.com> (cherry picked from commit ec7665973936897ab6be58308e655f08d91bec5c) --- contrib/ci/run | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/ci/run b/contrib/ci/run index b481419f9..bf29f8753 100755 --- a/contrib/ci/run +++ b/contrib/ci/run @@ -41,7 +41,7 @@ declare -r COVERAGE_MIN_FUNCS=0 # Those values are a sum up of the default warnings in all our # supported distros in our CI. -# debian_testing: E121,E123,E126,E226,E24,E704,W503 +# debian_testing: E121,E123,E126,E226,E24,E704,W503,W504,W605 # fedora22: # fedora23: # fedora24: E121,E123,E126,E226,E24,E704 @@ -51,7 +51,7 @@ declare -r COVERAGE_MIN_FUNCS=0 # fedora_rawhide: E121,E123,E126,E226,E24,E704 # rhel6: # rhel7: -declare PEP8_IGNORE="--ignore=E121,E123,E126,E226,E24,E704,W503" +declare PEP8_IGNORE="--ignore=E121,E123,E126,E226,E24,E704,W503,W504,W605" declare BASE_PFX="" declare DEPS=true declare BASE_DIR=`pwd` From 720a423a0119f23606c6029c3f48be98841ca910 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> Date: Tue, 30 Oct 2018 13:21:28 +0100 Subject: [PATCH 05/19] nss: use enumeration context as talloc parent for cache req result Otherwise we end up with memory leak since the result is never freed. We need to convert nctx->*ent structures into talloc pointer so we can use enum_ctx as parent. Resolves: https://pagure.io/SSSD/sssd/issue/3870 Reviewed-by: Jakub Hrozek <jhro...@redhat.com> (cherry picked from commit 406b731ddfbeb62623640cc37a7adc76af0a4b22) --- src/responder/nss/nss_cmd.c | 12 ++++++------ src/responder/nss/nss_enum.c | 2 +- src/responder/nss/nss_private.h | 6 +++--- src/responder/nss/nsssrv.c | 21 +++++++++++++++++++++ 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/responder/nss/nss_cmd.c b/src/responder/nss/nss_cmd.c index 9ee6ca805..25e663ed5 100644 --- a/src/responder/nss/nss_cmd.c +++ b/src/responder/nss/nss_cmd.c @@ -942,7 +942,7 @@ static errno_t nss_cmd_setpwent(struct cli_ctx *cli_ctx) nss_ctx = talloc_get_type(cli_ctx->rctx->pvt_ctx, struct nss_ctx); - return nss_setent(cli_ctx, CACHE_REQ_ENUM_USERS, &nss_ctx->pwent); + return nss_setent(cli_ctx, CACHE_REQ_ENUM_USERS, nss_ctx->pwent); } static errno_t nss_cmd_getpwent(struct cli_ctx *cli_ctx) @@ -955,7 +955,7 @@ static errno_t nss_cmd_getpwent(struct cli_ctx *cli_ctx) return nss_getent(cli_ctx, CACHE_REQ_ENUM_USERS, &state_ctx->pwent, nss_protocol_fill_pwent, - &nss_ctx->pwent); + nss_ctx->pwent); } static errno_t nss_cmd_endpwent(struct cli_ctx *cli_ctx) @@ -998,7 +998,7 @@ static errno_t nss_cmd_setgrent(struct cli_ctx *cli_ctx) nss_ctx = talloc_get_type(cli_ctx->rctx->pvt_ctx, struct nss_ctx); - return nss_setent(cli_ctx, CACHE_REQ_ENUM_GROUPS, &nss_ctx->grent); + return nss_setent(cli_ctx, CACHE_REQ_ENUM_GROUPS, nss_ctx->grent); } static errno_t nss_cmd_getgrent(struct cli_ctx *cli_ctx) @@ -1011,7 +1011,7 @@ static errno_t nss_cmd_getgrent(struct cli_ctx *cli_ctx) return nss_getent(cli_ctx, CACHE_REQ_ENUM_GROUPS, &state_ctx->grent, nss_protocol_fill_grent, - &nss_ctx->grent); + nss_ctx->grent); } static errno_t nss_cmd_endgrent(struct cli_ctx *cli_ctx) @@ -1093,7 +1093,7 @@ static errno_t nss_cmd_setservent(struct cli_ctx *cli_ctx) nss_ctx = talloc_get_type(cli_ctx->rctx->pvt_ctx, struct nss_ctx); - return nss_setent(cli_ctx, CACHE_REQ_ENUM_SVC, &nss_ctx->svcent); + return nss_setent(cli_ctx, CACHE_REQ_ENUM_SVC, nss_ctx->svcent); } static errno_t nss_cmd_getservent(struct cli_ctx *cli_ctx) @@ -1106,7 +1106,7 @@ static errno_t nss_cmd_getservent(struct cli_ctx *cli_ctx) return nss_getent(cli_ctx, CACHE_REQ_ENUM_SVC, &state_ctx->svcent, nss_protocol_fill_svcent, - &nss_ctx->svcent); + nss_ctx->svcent); } static errno_t nss_cmd_endservent(struct cli_ctx *cli_ctx) diff --git a/src/responder/nss/nss_enum.c b/src/responder/nss/nss_enum.c index a45b65233..9588943c9 100644 --- a/src/responder/nss/nss_enum.c +++ b/src/responder/nss/nss_enum.c @@ -138,7 +138,7 @@ static void nss_setent_internal_done(struct tevent_req *subreq) switch (ret) { case EOK: talloc_zfree(state->enum_ctx->result); - state->enum_ctx->result = talloc_steal(state->nss_ctx, result); + state->enum_ctx->result = talloc_steal(state->enum_ctx, result); if (state->type == CACHE_REQ_NETGROUP_BY_NAME) { /* We need to expand the netgroup into triples and members. */ diff --git a/src/responder/nss/nss_private.h b/src/responder/nss/nss_private.h index aa8d8e9cd..cd0d35517 100644 --- a/src/responder/nss/nss_private.h +++ b/src/responder/nss/nss_private.h @@ -78,9 +78,9 @@ struct nss_ctx { const char **extra_attributes; /* Enumeration. */ - struct nss_enum_ctx pwent; - struct nss_enum_ctx grent; - struct nss_enum_ctx svcent; + struct nss_enum_ctx *pwent; + struct nss_enum_ctx *grent; + struct nss_enum_ctx *svcent; hash_table_t *netgrent; /* Memory cache. */ diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c index 004e6c1a1..d6c5a08a9 100644 --- a/src/responder/nss/nsssrv.c +++ b/src/responder/nss/nsssrv.c @@ -378,6 +378,27 @@ int nss_process_init(TALLOC_CTX *mem_ctx, goto fail; } + nctx->pwent = talloc_zero(nctx, struct nss_enum_ctx); + if (nctx->pwent == NULL) { + DEBUG(SSSDBG_FATAL_FAILURE, "Unable to initialize pwent context!\n"); + ret = ENOMEM; + goto fail; + } + + nctx->grent = talloc_zero(nctx, struct nss_enum_ctx); + if (nctx->grent == NULL) { + DEBUG(SSSDBG_FATAL_FAILURE, "Unable to initialize grent context!\n"); + ret = ENOMEM; + goto fail; + } + + nctx->svcent = talloc_zero(nctx, struct nss_enum_ctx); + if (nctx->svcent == NULL) { + DEBUG(SSSDBG_FATAL_FAILURE, "Unable to initialize svcent context!\n"); + ret = ENOMEM; + goto fail; + } + nctx->netgrent = sss_ptr_hash_create(nctx, NULL, NULL); if (nctx->netgrent == NULL) { DEBUG(SSSDBG_FATAL_FAILURE, "Unable to initialize netgroups table!\n"); From 876f1cb87d1649d0681bf6475ab589287f15babb Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Thu, 22 Nov 2018 12:51:14 +0100 Subject: [PATCH 06/19] LDAP: minor refactoring in auth_send() to conform to our coding style Related: https://pagure.io/SSSD/sssd/issue/3451 A tevent _send() function should only return NULL on ENOMEM, otherwise it should mark the request as failed but return the req pointer. This was not much of an issue, before, but the next patch will add another function call to the auth_send call which would make error handling awkward. Reviewed-by: Sumit Bose <sb...@redhat.com> (cherry picked from commit 09091b4b60456a989ecc8c3b6f76661a14c108ba) --- src/providers/ldap/ldap_auth.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index d40bc9414..c409353d9 100644 --- a/src/providers/ldap/ldap_auth.c +++ b/src/providers/ldap/ldap_auth.c @@ -636,6 +636,7 @@ static struct tevent_req *auth_send(TALLOC_CTX *memctx, { struct tevent_req *req; struct auth_state *state; + errno_t ret; req = tevent_req_create(memctx, &state, struct auth_state); if (!req) return NULL; @@ -645,11 +646,11 @@ static struct tevent_req *auth_send(TALLOC_CTX *memctx, if (sss_authtok_get_type(authtok) == SSS_AUTHTOK_TYPE_SC_PIN || sss_authtok_get_type(authtok) == SSS_AUTHTOK_TYPE_SC_KEYPAD) { /* Tell frontend that we do not support Smartcard authentication */ - tevent_req_error(req, ERR_SC_AUTH_NOT_SUPPORTED); + ret = ERR_SC_AUTH_NOT_SUPPORTED; } else { - tevent_req_error(req, ERR_AUTH_FAILED); + ret = ERR_AUTH_FAILED; } - return tevent_req_post(req, ev); + goto fail; } state->ev = ev; @@ -663,13 +664,17 @@ static struct tevent_req *auth_send(TALLOC_CTX *memctx, state->sdap_service = ctx->service; } - if (!auth_connect_send(req)) goto fail; + if (auth_connect_send(req) == NULL) { + ret = ENOMEM; + goto fail; + } return req; fail: - talloc_zfree(req); - return NULL; + tevent_req_error(req, ret); + tevent_req_post(req, ev); + return req; } static struct tevent_req *auth_connect_send(struct tevent_req *req) From 7eb18ab68762d1b1ddbcbdc32dbcbd0df183d4f1 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Thu, 22 Nov 2018 12:17:51 +0100 Subject: [PATCH 07/19] LDAP: Only authenticate the auth connection if we need to look up user information Related: https://pagure.io/SSSD/sssd/issue/3451 Commit add72860c7a7a2c418f4d8b6790b5caeaf7dfb7b initially addressed #3451 by using the full sdap_cli_connect() request during LDAP authentication. This was a good idea as it addressed the case where the authentication connection must also look up some user information (typically with id_provider=proxy where you don't know the DN to bind as during authentication), but this approach also broke the use-case of id_provider=ldap and auth_provider=ldap with ldap_sasl_auth=gssapi. This is because (for reason I don't know) AD doesn't like if you use both GSSAPI and startTLS on the same connection. But the code would force TLS during the authentication as a general measure to not transmit passwords in the clear, but then, the connection would also see that ldap_sasl_auth=gssapi is set and also bind with GSSAPI. This patch checks if the user DN is already known and if yes, then doesn't authenticate the connection as the connection will then only be used for the user simple bind. Reviewed-by: Sumit Bose <sb...@redhat.com> (cherry picked from commit 57fc60c9dc77698cf824813c36eb0f90d767b315) --- src/providers/ldap/ldap_auth.c | 53 +++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index c409353d9..b4d045a65 100644 --- a/src/providers/ldap/ldap_auth.c +++ b/src/providers/ldap/ldap_auth.c @@ -664,6 +664,18 @@ static struct tevent_req *auth_send(TALLOC_CTX *memctx, state->sdap_service = ctx->service; } + ret = get_user_dn(state, state->ctx->be->domain, + state->ctx->opts, state->username, &state->dn, + &state->pw_expire_type, &state->pw_expire_data); + if (ret == EAGAIN) { + DEBUG(SSSDBG_TRACE_FUNC, + "Need to look up the DN of %s later\n", state->username); + } else if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "Cannot get user DN [%d]: %s\n", ret, sss_strerror(ret)); + goto fail; + } + if (auth_connect_send(req) == NULL) { ret = ENOMEM; goto fail; @@ -683,6 +695,8 @@ static struct tevent_req *auth_connect_send(struct tevent_req *req) struct auth_state *state = tevent_req_data(req, struct auth_state); bool use_tls; + bool skip_conn_auth = false; + const char *sasl_mech; /* Check for undocumented debugging feature to disable TLS * for authentication. This should never be used in production @@ -695,10 +709,33 @@ static struct tevent_req *auth_connect_send(struct tevent_req *req) "for debugging purposes only."); } + if (state->dn != NULL) { + /* In case the user's DN is known, the connection will only be used + * to bind as the user to perform the authentication. In that case, + * we don't need to authenticate the connection, because we're not + * looking up any information using the connection. This might be + * needed e.g. in case both ID and AUTH providers are set to LDAP + * and the server is AD, because otherwise the connection would + * both do a startTLS and later bind using GSSAPI which doesn't work + * well with AD. + */ + skip_conn_auth = true; + } + + if (skip_conn_auth == false) { + sasl_mech = dp_opt_get_string(state->ctx->opts->basic, + SDAP_SASL_MECH); + if (sasl_mech && strcasecmp(sasl_mech, "GSSAPI") == 0) { + /* Don't force TLS on if we're told to use GSSAPI */ + use_tls = false; + } + } + subreq = sdap_cli_connect_send(state, state->ev, state->ctx->opts, state->ctx->be, state->sdap_service, false, - use_tls ? CON_TLS_ON : CON_TLS_OFF, false); + use_tls ? CON_TLS_ON : CON_TLS_OFF, + skip_conn_auth); if (subreq == NULL) { tevent_req_error(req, ENOMEM); @@ -739,15 +776,7 @@ static void auth_connect_done(struct tevent_req *subreq) return; } - ret = get_user_dn(state, state->ctx->be->domain, - state->ctx->opts, state->username, &state->dn, - &state->pw_expire_type, &state->pw_expire_data); - if (ret == EOK) { - /* All required user data was pre-cached during an identity lookup. - * We can proceed with the bind */ - auth_do_bind(req); - return; - } else if (ret == EAGAIN) { + if (state->dn == NULL) { /* The cached user entry was missing the bind DN. Need to look * it up based on user name in order to perform the bind */ subreq = get_user_dn_send(req, state->ev, state->ctx->be->domain, @@ -760,7 +789,9 @@ static void auth_connect_done(struct tevent_req *subreq) return; } - tevent_req_error(req, ret); + /* All required user data was pre-cached during an identity lookup. + * We can proceed with the bind */ + auth_do_bind(req); return; } From 1a7c6ab6efce3720d27def426aad49ee99eb339d Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Mon, 26 Nov 2018 12:38:40 +0100 Subject: [PATCH 08/19] LDAP: Log the encryption used during LDAP authentication Reviewed-by: Jakub Hrozek <jhro...@redhat.com> (cherry picked from commit 6f113c7ddeaa5c82558e10118b499d22bf7a2b14) --- src/providers/ldap/ldap_auth.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index b4d045a65..4666dbfbb 100644 --- a/src/providers/ldap/ldap_auth.c +++ b/src/providers/ldap/ldap_auth.c @@ -747,6 +747,31 @@ static struct tevent_req *auth_connect_send(struct tevent_req *req) return subreq; } +static void check_encryption(LDAP *ldap) +{ + ber_len_t sasl_ssf = 0; + int tls_inplace = 0; + int ret; + + ret = ldap_get_option(ldap, LDAP_OPT_X_SASL_SSF, &sasl_ssf); + if (ret != LDAP_SUCCESS) { + DEBUG(SSSDBG_TRACE_LIBS, "ldap_get_option failed to get sasl ssf, " + "assuming SASL is not used.\n"); + } + + tls_inplace = ldap_tls_inplace(ldap); + + DEBUG(SSSDBG_TRACE_ALL, + "Encryption used: SASL SSF [%lu] tls_inplace [%s].\n", sasl_ssf, + tls_inplace == 1 ? "TLS inplace" : "TLS NOT inplace"); + + if (sasl_ssf <= 1 && tls_inplace != 1) { + DEBUG(SSSDBG_CRIT_FAILURE, + "No encryption detected on LDAP connection.\n"); + sss_log(SSS_LOG_CRIT, "No encryption detected on LDAP connection.\n"); + } +} + static void auth_connect_done(struct tevent_req *subreq) { struct tevent_req *req = tevent_req_callback_data(subreq, @@ -776,6 +801,8 @@ static void auth_connect_done(struct tevent_req *subreq) return; } + check_encryption(state->sh->ldap); + if (state->dn == NULL) { /* The cached user entry was missing the bind DN. Need to look * it up based on user name in order to perform the bind */ From 4d3841ca379afc01184453ba45ab3e75ffec60da Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik <lsleb...@redhat.com> Date: Wed, 7 Nov 2018 23:06:10 +0000 Subject: [PATCH 09/19] UTIL: Fix compilation with curl 7.62.0 The macro CURLE_SSL_CACERT is deprecated in upstream curl since commit 3f3b26d6feb0667714902e836af608094235fca2. commit 3f3b26d6feb0667714902e836af608094235fca2 Author: Han Han <h...@thousandeyes.com> Date: Wed Aug 22 11:13:32 2018 -0700 ssl: deprecate CURLE_SSL_CACERT in favour of a unified error code Long live CURLE_PEER_FAILED_VERIFICATION sh$ git tag --contains 3f3b26d6feb0667714902e836af608094235fca2 curl-7_62_0 It was not removed. It is just an alias to CURLE_PEER_FAILED_VERIFICATION which causes compile time failures in switch/case. ./src/util/tev_curl.c: In function 'curl_code2errno': ./src/util/tev_curl.c:113:5: error: duplicate case value case CURLE_PEER_FAILED_VERIFICATION: ^~~~ ./src/util/tev_curl.c: 100:5: note: previously used here case CURLE_SSL_CACERT: ^~~~ Merges: https://pagure.io/SSSD/sssd/pull-request/3878 Resolves: https://pagure.io/SSSD/sssd/issue/3875 Reviewed-by: Sumit Bose <sb...@redhat.com> (cherry picked from commit 1ee12b05570fcfb8e4190c9ec704c5563138344d) --- src/util/tev_curl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index 6a7a580d5..d70a42972 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -97,7 +97,9 @@ static errno_t curl_code2errno(CURLcode crv) return ETIMEDOUT; case CURLE_SSL_ISSUER_ERROR: case CURLE_SSL_CACERT_BADFILE: +#if LIBCURL_VERSION_NUM < 0x073e00 case CURLE_SSL_CACERT: +#endif case CURLE_SSL_CERTPROBLEM: return ERR_INVALID_CERT; From e80e869a92c4689888f6f5d967acfe9f085ce27a Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik <lsleb...@redhat.com> Date: Sat, 10 Nov 2018 20:34:28 +0000 Subject: [PATCH 10/19] test_pac_responder: Skip test if pac responder is not installed Merges: https://pagure.io/SSSD/sssd/pull-request/3881 Reviewed-by: Sumit Bose <sb...@redhat.com> (cherry picked from commit 4f824eca24e185e6463167b7bcc20d1398c60414) --- src/tests/intg/test_pac_responder.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/tests/intg/test_pac_responder.py b/src/tests/intg/test_pac_responder.py index adc31965b..a93c6ca6b 100644 --- a/src/tests/intg/test_pac_responder.py +++ b/src/tests/intg/test_pac_responder.py @@ -27,6 +27,12 @@ from util import unindent +def have_sssd_responder(responder_name): + responder_path = os.path.join(config.LIBEXEC_PATH, "sssd", responder_name) + + return os.access(responder_path, os.X_OK) + + def stop_sssd(): with open(config.PIDFILE_PATH, "r") as pid_file: pid = int(pid_file.read()) @@ -99,6 +105,8 @@ def timeout_handler(signum, frame): raise Exception("Timeout") +@pytest.mark.skipif(not have_sssd_responder("sssd_pac"), + reason="No PAC responder, skipping") def test_multithreaded_pac_client(local_domain_only, sssd_pac_test_client): """ Test for ticket From de7f877304cbc82cb13ff8a932560a439b690b02 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik <lsleb...@redhat.com> Date: Sat, 10 Nov 2018 20:34:37 +0000 Subject: [PATCH 11/19] INTG: Show extra test summary info with pytest It will show reasons why tests were skipped. e.g. ====================== test session starts ======================== platform linux -- Python 3.7.1, pytest-3.9.3, py-1.5.4, pluggy-0.7.1 -- /usr/bin/python3 cachedir: .pytest_cache rootdir: /dev/shm/sssd/src/tests/intg, inifile: collected 286 items / 285 deselected test_pac_responder.py::test_multithreaded_pac_client SKIPPED [100%] ==================== short test summary info ====================== SKIP [1] test_pac_responder.py:108: No PAC responder, skipping Merges: https://pagure.io/SSSD/sssd/pull-request/3881 Reviewed-by: Sumit Bose <sb...@redhat.com> (cherry picked from commit fdbe67a88d1894471031b0aabc74bd1b29eef294) --- src/tests/intg/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am index 65da9ca17..91dc86a4f 100644 --- a/src/tests/intg/Makefile.am +++ b/src/tests/intg/Makefile.am @@ -140,5 +140,5 @@ intgcheck-installed: config.py passwd group DBUS_SESSION_BUS_ADDRESS="unix:path=$$DBUS_SOCK_DIR/fake_socket" \ DBUS_SYSTEM_BUS_ADDRESS="unix:path=$$DBUS_SOCK_DIR/system_bus_socket" \ DBUS_SYSTEM_BUS_DEFAULT_ADDRESS="$$DBUS_SYSTEM_BUS_ADDRESS" \ - fakeroot $(PYTHON2) $(PYTEST) -v --tb=native $(INTGCHECK_PYTEST_ARGS) . + fakeroot $(PYTHON2) $(PYTEST) -v -r a --tb=native $(INTGCHECK_PYTEST_ARGS) . rm -f $(DESTDIR)$(logpath)/* From 517fe071081619050df6d0d2983057f9007ce66b Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik <lsleb...@redhat.com> Date: Wed, 14 Nov 2018 22:33:26 +0000 Subject: [PATCH 12/19] CI: Modify suppression file for c-ares-1.15.0 Valgrind does not generate full stack trace for errors. It is just limited amount of frames. Therefore we cannot see main function with the new c-ares. The suppression file generated with c-ares-1.14.0 { <insert_a_suppression_name_here> Memcheck:Leak match-leak-kinds: possible fun:malloc fun:strdup fun:ares_init_options fun:recreate_ares_channel fun:resolv_init fun:be_res_init fun:be_res_init fun:be_init_failover fun:test_ipa_server_create_trusts_setup obj:/usr/lib64/libcmocka.so.0.5.1 fun:_cmocka_run_group_tests fun:main } The suppression file generated with c-ares-1.15.0 { <insert_a_suppression_name_here> Memcheck:Leak match-leak-kinds: possible fun:malloc fun:strdup obj:/usr/lib64/libcares.so.2.3.0 obj:/usr/lib64/libcares.so.2.3.0 fun:ares_init_options fun:recreate_ares_channel fun:resolv_init fun:be_res_init fun:be_res_init fun:be_init_failover fun:test_ipa_server_create_trusts_setup obj:/usr/lib64/libcmocka.so.0.5.1 fun:_cmocka_run_group_tests } Merges: https://pagure.io/SSSD/sssd/pull-request/3884 Reviewed-by: Sumit Bose <sb...@redhat.com> (cherry picked from commit f02714d6f0f5d9cdd504d5f0527849a6d6b88fab) --- contrib/ci/sssd.supp | 1 - 1 file changed, 1 deletion(-) diff --git a/contrib/ci/sssd.supp b/contrib/ci/sssd.supp index 38254ca8d..d27e006a0 100644 --- a/contrib/ci/sssd.supp +++ b/contrib/ci/sssd.supp @@ -169,7 +169,6 @@ fun:test_ipa_server_create_trusts_setup ... fun:_cmocka_run_group_tests - fun:main } # Leaks in bash if p11_child returns and error because due to libtool the From 9e858795648909d17ba8bd72ba64fd6e8817a149 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Thu, 22 Nov 2018 11:33:20 +0100 Subject: [PATCH 13/19] BUILD: Accept krb5 1.17 for building the PAC plugin Reviewed-by: Jakub Hrozek <jhro...@redhat.com> (cherry picked from commit 53e6fdfd881f051898e85448832eafdd2ea09454) --- src/external/pac_responder.m4 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/external/pac_responder.m4 b/src/external/pac_responder.m4 index e0685f0ce..dc986a1b8 100644 --- a/src/external/pac_responder.m4 +++ b/src/external/pac_responder.m4 @@ -18,7 +18,8 @@ then Kerberos\ 5\ release\ 1.13* | \ Kerberos\ 5\ release\ 1.14* | \ Kerberos\ 5\ release\ 1.15* | \ - Kerberos\ 5\ release\ 1.16*) + Kerberos\ 5\ release\ 1.16* | \ + Kerberos\ 5\ release\ 1.17*) krb5_version_ok=yes AC_MSG_RESULT([yes]) ;; From d1c930809fc4ea9b7abdecd4b17cd3da735beef7 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Thu, 22 Nov 2018 11:36:57 +0100 Subject: [PATCH 14/19] tests: fix mocking krb5_creds in test_copy_ccache To just test some ccache related functionality without talking to an actual KDC to get the tickets some needed libkrb5 structs were mocked based on tests from the MIT Kerberos source code. One struct member (is_skey) was so far not regarded by libkrb5 for out test case. But a recent fix for http://krbdev.mit.edu/rt/Ticket/Display.html?id=8718 changed this and we have to change the mocking. Reviewed-by: Jakub Hrozek <jhro...@redhat.com> (cherry picked from commit 08bba3a6e3e4e21f2e20b71cca463d50420aa9ee) --- src/tests/cmocka/test_copy_ccache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/cmocka/test_copy_ccache.c b/src/tests/cmocka/test_copy_ccache.c index 84225b6bf..7c76c00e8 100644 --- a/src/tests/cmocka/test_copy_ccache.c +++ b/src/tests/cmocka/test_copy_ccache.c @@ -88,7 +88,7 @@ static int setup_ccache(void **state) test_creds.times.starttime = 2222; test_creds.times.endtime = 3333; test_creds.times.renew_till = 4444; - test_creds.is_skey = 1; + test_creds.is_skey = 0; test_creds.ticket_flags = 5555; test_creds.addresses = addrs; From 19e6c50df215e40ddd46cf9dc7583706c0a3e02b Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Thu, 22 Nov 2018 12:12:00 +0100 Subject: [PATCH 15/19] tests: increase p11_child_timeout With recent version of valgrind some tests failed during a CI run with a timeout. To avoid this the related p11_child_timeout is increased for the affected tests. Reviewed-by: Jakub Hrozek <jhro...@redhat.com> (cherry picked from commit 1617f3e3dc90788ef50bbc7948c3870d08cc9c2d) --- src/tests/cmocka/test_cert_utils.c | 2 +- src/tests/cmocka/test_ssh_srv.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/cmocka/test_cert_utils.c b/src/tests/cmocka/test_cert_utils.c index 26fffb870..ec5858b6e 100644 --- a/src/tests/cmocka/test_cert_utils.c +++ b/src/tests/cmocka/test_cert_utils.c @@ -50,7 +50,7 @@ /* When run under valgrind with --trace-children=yes we have to increase the * timeout not because p11_child needs much more time under valgrind but * because of the way valgrind handles the children. */ -#define P11_CHILD_TIMEOUT 40 +#define P11_CHILD_TIMEOUT 80 /* TODO: create a certificate for this test */ const uint8_t test_cert_der[] = { diff --git a/src/tests/cmocka/test_ssh_srv.c b/src/tests/cmocka/test_ssh_srv.c index 93217a197..d611bdcff 100644 --- a/src/tests/cmocka/test_ssh_srv.c +++ b/src/tests/cmocka/test_ssh_srv.c @@ -223,7 +223,7 @@ static int ssh_test_setup(void **state) * the timeout not because p11_child needs much more time under valgrind * but because of the way valgrind handles the children. */ struct sss_test_conf_param ssh_params[] = { - { "p11_child_timeout", "40" }, + { "p11_child_timeout", "80" }, { NULL, NULL }, /* Sentinel */ }; From d33ec64423087261fcc14acb5922793fadb83342 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Tue, 4 Dec 2018 13:08:11 +0100 Subject: [PATCH 16/19] Revert "IPA: use forest name when looking up the Global Catalog" This reverts commit 149174acae677d1e72a0da431bf0850d55f2ccb4. Reviewed-by: Jakub Hrozek <jhro...@redhat.com> (cherry picked from commit 9096fc01cca8fcaeb19c36a27f3a9fa09d60772a) --- src/providers/ipa/ipa_subdomains_server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/providers/ipa/ipa_subdomains_server.c b/src/providers/ipa/ipa_subdomains_server.c index e5ea4bd02..43a3053cb 100644 --- a/src/providers/ipa/ipa_subdomains_server.c +++ b/src/providers/ipa/ipa_subdomains_server.c @@ -266,7 +266,7 @@ ipa_ad_ctx_new(struct be_ctx *be_ctx, DEBUG(SSSDBG_TRACE_ALL, "No extra attrs set.\n"); } - gc_service_name = talloc_asprintf(ad_options, "sd_gc_%s", subdom->forest); + gc_service_name = talloc_asprintf(ad_options, "sd_gc_%s", subdom->name); if (gc_service_name == NULL) { talloc_free(ad_options); return ENOMEM; From 74568bdde833f752187cb1a38b39715556c91279 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Tue, 4 Dec 2018 13:06:23 +0100 Subject: [PATCH 17/19] ipa: use only the global catalog service of the forest root While creating the domains and sub-domains each domain gets a global catalog services assigned but only one should be used because the global catalog is by definition responsible for the whole forest so it does not make sense to use a global catalog service for each domain and in the worst case connect to the same GC multiple times. In the AD provider this is simple because the GC service of the configured domain AD_GC_SERVICE_NAME ("AD_GC") can be used. In the IPA case all domains from the trusted forest are on the level of sub-domains so we have to pick one. Since the forest root is linked from all domain of the same forest it will be the most straight forward choice. Related to https://pagure.io/SSSD/sssd/issue/3902 Reviewed-by: Jakub Hrozek <jhro...@redhat.com> (cherry picked from commit 62d671b874a66101c0f4bff39fc6d7f49cb8fca6) --- src/providers/ipa/ipa_subdomains_id.c | 50 +++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c index a16eed284..48cf74460 100644 --- a/src/providers/ipa/ipa_subdomains_id.c +++ b/src/providers/ipa/ipa_subdomains_id.c @@ -713,6 +713,52 @@ int ipa_get_subdom_acct_recv(struct tevent_req *req, int *dp_error_out) return EOK; } +static struct ad_id_ctx *ipa_get_ad_id_ctx(struct ipa_id_ctx *ipa_ctx, + struct sss_domain_info *dom); + +static struct sdap_id_conn_ctx ** +ipa_ad_gc_conn_list(TALLOC_CTX *mem_ctx, struct ipa_id_ctx *ipa_ctx, + struct ad_id_ctx *ad_ctx, struct sss_domain_info *dom) +{ + struct ad_id_ctx *forest_root_ad_id_ctx; + struct sdap_id_conn_ctx **clist; + int cindex = 0; + + /* While creating the domains and sub-domains each domain gets a global + * catalog services assigned but only one should be used because the + * global catalog is by definition responsible for the whole forest so it + * does not make sense to use a global catalog service for each domain and + * in the worst case connect to the same GC multiple times. + * + * In the AD provider this is simple because the GC service of the + * configured domain AD_GC_SERVICE_NAME ("AD_GC") can be used. In the IPA + * case all domains from the trusted forest are on the level of + * sub-domains so we have to pick one. Since the forest root is linked + * from all domain of the same forest it will be the most straight forward + * choice. */ + forest_root_ad_id_ctx = ipa_get_ad_id_ctx(ipa_ctx, dom->forest_root); + if (forest_root_ad_id_ctx == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "Missing ad_id_ctx for forest root.\n"); + return NULL; + } + + clist = talloc_zero_array(mem_ctx, struct sdap_id_conn_ctx *, 3); + if (clist == NULL) return NULL; + + /* Always try GC first */ + if (dp_opt_get_bool(forest_root_ad_id_ctx->ad_options->basic, + AD_ENABLE_GC)) { + clist[cindex] = forest_root_ad_id_ctx->gc_ctx; + clist[cindex]->ignore_mark_offline = true; + clist[cindex]->no_mpg_user_fallback = true; + cindex++; + } + + clist[cindex] = ad_get_dom_ldap_conn(ad_ctx, dom); + + return clist; +} + /* IPA lookup for server mode. Directly to AD. */ struct ipa_get_ad_acct_state { int dp_error; @@ -731,8 +777,6 @@ static errno_t ipa_get_ad_apply_override_step(struct tevent_req *req); static errno_t ipa_get_ad_ipa_membership_step(struct tevent_req *req); static void ipa_id_get_groups_overrides_done(struct tevent_req *subreq); static void ipa_get_ad_acct_done(struct tevent_req *subreq); -static struct ad_id_ctx *ipa_get_ad_id_ctx(struct ipa_id_ctx *ipa_ctx, - struct sss_domain_info *dom); static struct tevent_req * ipa_get_ad_acct_send(TALLOC_CTX *mem_ctx, @@ -785,7 +829,7 @@ ipa_get_ad_acct_send(TALLOC_CTX *mem_ctx, case BE_REQ_INITGROUPS: case BE_REQ_BY_SECID: case BE_REQ_GROUP: - clist = ad_gc_conn_list(req, ad_id_ctx, state->obj_dom); + clist = ipa_ad_gc_conn_list(req, ipa_ctx, ad_id_ctx, state->obj_dom); break; default: clist = ad_ldap_conn_list(req, ad_id_ctx, state->obj_dom); From 28792523a01a7d21bcc8931794164f253e691a68 Mon Sep 17 00:00:00 2001 From: Tomas Halman <thal...@redhat.com> Date: Mon, 3 Dec 2018 14:11:31 +0100 Subject: [PATCH 18/19] nss: sssd returns '/' for emtpy home directories For empty home directory in passwd file sssd returns "/". Sssd should respect system behaviour and return the same as nsswitch "files" module - return empty string. Resolves: https://pagure.io/SSSD/sssd/issue/3901 Reviewed-by: Simo Sorce <s...@redhat.com> Reviewed-by: Jakub Hrozek <jhro...@redhat.com> (cherry picked from commit 90f32399b4100ce39cf665649fde82d215e5eb49) --- src/confdb/confdb.c | 9 +++++++++ src/man/include/ad_modified_defaults.xml | 19 +++++++++++++++++++ src/responder/nss/nss_protocol_pwent.c | 2 +- src/tests/intg/test_files_provider.py | 2 +- 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index a3eb9c66d..17bb4f827 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -1301,6 +1301,15 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb, ret = ENOMEM; goto done; } + } else { + if (strcasecmp(domain->provider, "ad") == 0) { + /* ad provider default */ + domain->fallback_homedir = talloc_strdup(domain, "/home/%d/%u"); + if (!domain->fallback_homedir) { + ret = ENOMEM; + goto done; + } + } } tmp = ldb_msg_find_attr_as_string(res->msgs[0], diff --git a/src/man/include/ad_modified_defaults.xml b/src/man/include/ad_modified_defaults.xml index 818a2bf78..425b7e8ee 100644 --- a/src/man/include/ad_modified_defaults.xml +++ b/src/man/include/ad_modified_defaults.xml @@ -76,4 +76,23 @@ </listitem> </itemizedlist> </refsect2> + <refsect2 id='nss_modifications'> + <title>NSS configuration</title> + <itemizedlist> + <listitem> + <para> + fallback_homedir = /home/%d/%u + </para> + <para> + The AD provider automatically sets + "fallback_homedir = /home/%d/%u" to provide personal + home directories for users without the homeDirectory + attribute. If your AD Domain is properly + populated with Posix attributes, and you want to avoid + this fallback behavior, you can explicitly + set "fallback_homedir = %o". + </para> + </listitem> + </itemizedlist> + </refsect2> </refsect1> diff --git a/src/responder/nss/nss_protocol_pwent.c b/src/responder/nss/nss_protocol_pwent.c index af9e74fc8..86fa4ec46 100644 --- a/src/responder/nss/nss_protocol_pwent.c +++ b/src/responder/nss/nss_protocol_pwent.c @@ -118,7 +118,7 @@ nss_get_homedir(TALLOC_CTX *mem_ctx, homedir = nss_get_homedir_override(mem_ctx, msg, nss_ctx, domain, &hd_ctx); if (homedir == NULL) { - return "/"; + return ""; } return homedir; diff --git a/src/tests/intg/test_files_provider.py b/src/tests/intg/test_files_provider.py index ead1cc4c3..4761f1bd1 100644 --- a/src/tests/intg/test_files_provider.py +++ b/src/tests/intg/test_files_provider.py @@ -678,7 +678,7 @@ def test_user_no_dir(setup_pw_with_canary, files_domain_only): Test that resolving a user without a homedir defined works and returns a fallback value """ - check_user(incomplete_user_setup(setup_pw_with_canary, 'dir', '/')) + check_user(incomplete_user_setup(setup_pw_with_canary, 'dir', '')) def test_user_no_gecos(setup_pw_with_canary, files_domain_only): From 1088f96fb3893d3b86ff1595073a525d0749a93a Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Fri, 30 Nov 2018 13:06:13 +0100 Subject: [PATCH 19/19] NSS: Avoid changing the memory cache ownership away from the sssd user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves: https://pagure.io/SSSD/sssd/issue/3890 In case SSSD is compiled --with-sssd-user but run as root (which is the default on RHEL and derivatives), then the memory cache will be owned by the user that sssd_nss runs as, so root. This conflicts with the packaging which specifies sssd.sssd as the owner. And in turn, this means that users can't reliably assess the package integrity using rpm -V. This patch makes sure that the memory cache files are chowned to sssd.sssd even if the nss responder runs as root. Also, this patch changes the sssd_nss responder so that is becomes a member of the supplementary sssd group. Even though in traditional UNIX sense, a process running as root could write to a file owned by sssd:sssd, with SELinux enforcing mode this becomes problematic as SELinux emits an error such as: type=AVC msg=audit(1543524888.125:1495): avc: denied { fsetid } for pid=7706 comm="sssd_nss" capability=4 scontext=system_u:system_r:sssd_t:s0 tcontext=system_u:system_r:sssd_t:s0 tclass=capability To make it possible for the sssd_nss process to write to the files, the files are also made group-writable. The 'others' permission is still set to read only. Reviewed-by: Michal Židek <mzi...@redhat.com> (cherry picked from commit 61e4ba58934b20a950255e05797aca25aadc1242) --- contrib/sssd.spec.in | 8 +- src/responder/nss/nss_private.h | 2 + src/responder/nss/nsssrv.c | 106 ++++++++++++++++++++++++-- src/responder/nss/nsssrv_mmap_cache.c | 51 ++++++++++++- src/responder/nss/nsssrv_mmap_cache.h | 5 +- 5 files changed, 158 insertions(+), 14 deletions(-) diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index 89e4d7509..cd5f7a714 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -1025,11 +1025,11 @@ done %dir %{sssdstatedir} %dir %{_localstatedir}/cache/krb5rcache %attr(700,sssd,sssd) %dir %{dbpath} -%attr(755,sssd,sssd) %dir %{mcpath} +%attr(775,sssd,sssd) %dir %{mcpath} %attr(751,sssd,sssd) %dir %{deskprofilepath} -%ghost %attr(0644,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/passwd -%ghost %attr(0644,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/group -%ghost %attr(0644,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/initgroups +%ghost %attr(0664,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/passwd +%ghost %attr(0664,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/group +%ghost %attr(0664,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/initgroups %attr(755,sssd,sssd) %dir %{pipepath} %attr(750,sssd,root) %dir %{pipepath}/private %attr(755,sssd,sssd) %dir %{pubconfpath} diff --git a/src/responder/nss/nss_private.h b/src/responder/nss/nss_private.h index cd0d35517..bae5fe074 100644 --- a/src/responder/nss/nss_private.h +++ b/src/responder/nss/nss_private.h @@ -87,6 +87,8 @@ struct nss_ctx { struct sss_mc_ctx *pwd_mc_ctx; struct sss_mc_ctx *grp_mc_ctx; struct sss_mc_ctx *initgr_mc_ctx; + uid_t mc_uid; + gid_t mc_gid; }; struct sss_cmd_table *get_nss_cmds(void); diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c index d6c5a08a9..87a3f1d50 100644 --- a/src/responder/nss/nsssrv.c +++ b/src/responder/nss/nsssrv.c @@ -101,7 +101,8 @@ static int nss_clear_memcache(struct sbus_request *dbus_req, void *data) /* TODO: read cache sizes from configuration */ DEBUG(SSSDBG_TRACE_FUNC, "Clearing memory caches.\n"); - ret = sss_mmap_cache_reinit(nctx, SSS_MC_CACHE_ELEMENTS, + ret = sss_mmap_cache_reinit(nctx, nctx->mc_uid, nctx->mc_gid, + SSS_MC_CACHE_ELEMENTS, (time_t) memcache_timeout, &nctx->pwd_mc_ctx); if (ret != EOK) { @@ -110,7 +111,8 @@ static int nss_clear_memcache(struct sbus_request *dbus_req, void *data) return ret; } - ret = sss_mmap_cache_reinit(nctx, SSS_MC_CACHE_ELEMENTS, + ret = sss_mmap_cache_reinit(nctx, nctx->mc_uid, nctx->mc_gid, + SSS_MC_CACHE_ELEMENTS, (time_t) memcache_timeout, &nctx->grp_mc_ctx); if (ret != EOK) { @@ -119,7 +121,8 @@ static int nss_clear_memcache(struct sbus_request *dbus_req, void *data) return ret; } - ret = sss_mmap_cache_reinit(nctx, SSS_MC_CACHE_ELEMENTS, + ret = sss_mmap_cache_reinit(nctx, nctx->mc_uid, nctx->mc_gid, + SSS_MC_CACHE_ELEMENTS, (time_t)memcache_timeout, &nctx->initgr_mc_ctx); if (ret != EOK) { @@ -284,21 +287,27 @@ static int setup_memcaches(struct nss_ctx *nctx) } /* TODO: read cache sizes from configuration */ - ret = sss_mmap_cache_init(nctx, "passwd", SSS_MC_PASSWD, + ret = sss_mmap_cache_init(nctx, "passwd", + nctx->mc_uid, nctx->mc_gid, + SSS_MC_PASSWD, SSS_MC_CACHE_ELEMENTS, (time_t)memcache_timeout, &nctx->pwd_mc_ctx); if (ret) { DEBUG(SSSDBG_CRIT_FAILURE, "passwd mmap cache is DISABLED\n"); } - ret = sss_mmap_cache_init(nctx, "group", SSS_MC_GROUP, + ret = sss_mmap_cache_init(nctx, "group", + nctx->mc_uid, nctx->mc_gid, + SSS_MC_GROUP, SSS_MC_CACHE_ELEMENTS, (time_t)memcache_timeout, &nctx->grp_mc_ctx); if (ret) { DEBUG(SSSDBG_CRIT_FAILURE, "group mmap cache is DISABLED\n"); } - ret = sss_mmap_cache_init(nctx, "initgroups", SSS_MC_INITGROUPS, + ret = sss_mmap_cache_init(nctx, "initgroups", + nctx->mc_uid, nctx->mc_gid, + SSS_MC_INITGROUPS, SSS_MC_CACHE_ELEMENTS, (time_t)memcache_timeout, &nctx->initgr_mc_ctx); if (ret) { @@ -308,6 +317,79 @@ static int setup_memcaches(struct nss_ctx *nctx) return EOK; } +static int sssd_supplementary_group(struct nss_ctx *nss_ctx) +{ + errno_t ret; + int size; + gid_t *supp_gids = NULL; + + /* + * We explicitly read the IDs of the SSSD user even though the server + * receives --uid and --gid by parameters to account for the case where + * the SSSD is compiled --with-sssd-user=sssd but the default of the + * user option is root (this is what RHEL does) + */ + ret = sss_user_by_name_or_uid(SSSD_USER, + &nss_ctx->mc_uid, + &nss_ctx->mc_gid); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, "Cannot get info on "SSSD_USER); + return ret; + } + + if (getgid() == nss_ctx->mc_gid) { + DEBUG(SSSDBG_TRACE_FUNC, "Already running as the sssd group\n"); + return EOK; + } + + size = getgroups(0, NULL); + if (size == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, "Getgroups failed! (%d, %s)\n", + ret, sss_strerror(ret)); + return ret; + } + + if (size > 0) { + supp_gids = talloc_zero_array(NULL, gid_t, size); + if (supp_gids == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "Allocation failed!\n"); + ret = ENOMEM; + goto done; + } + } + + size = getgroups(size, supp_gids); + if (size == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, "Getgroups failed! (%d, %s)\n", + ret, sss_strerror(ret)); + goto done; + } + + for (int i = 0; i < size; i++) { + if (supp_gids[i] == nss_ctx->mc_gid) { + DEBUG(SSSDBG_TRACE_FUNC, + "Already assigned to the SSSD supplementary group\n"); + ret = EOK; + goto done; + } + } + + ret = setgroups(1, &nss_ctx->mc_gid); + if (ret != EOK) { + ret = errno; + DEBUG(SSSDBG_OP_FAILURE, + "Cannot setgroups [%d]: %s\n", ret, sss_strerror(ret)); + goto done; + } + + ret = EOK; +done: + talloc_free(supp_gids); + return ret; +} + int nss_process_init(TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct confdb_ctx *cdb) @@ -405,6 +487,18 @@ int nss_process_init(TALLOC_CTX *mem_ctx, ret = EFAULT; goto fail; } + /* + * Adding the NSS process to the SSSD supplementary group avoids + * dac_override AVC messages from SELinux in case sssd_nss runs + * as root and tries to write to memcache owned by sssd:sssd + */ + ret = sssd_supplementary_group(nctx); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Cannot add process to the sssd supplementary group [%d]: %s\n", + ret, sss_strerror(ret)); + goto fail; + } ret = setup_memcaches(nctx); if (ret != EOK) { diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index de9e67513..d5181d771 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c @@ -57,6 +57,9 @@ struct sss_mc_ctx { char *file; /* mmap cache file name */ int fd; /* file descriptor */ + uid_t uid; /* User ID of owner */ + gid_t gid; /* Group ID of owner */ + uint32_t seed; /* pseudo-random seed to avoid collision attacks */ time_t valid_time_slot; /* maximum time the entry is valid in seconds */ @@ -623,7 +626,9 @@ static errno_t sss_mc_get_record(struct sss_mc_ctx **_mcc, if (ret == EFAULT) { DEBUG(SSSDBG_CRIT_FAILURE, "Fatal internal mmap cache error, invalidating cache!\n"); - (void)sss_mmap_cache_reinit(talloc_parent(mcc), -1, -1, _mcc); + (void)sss_mmap_cache_reinit(talloc_parent(mcc), + -1, -1, -1, -1, + _mcc); } return ret; } @@ -1144,6 +1149,26 @@ static errno_t sss_mc_create_file(struct sss_mc_ctx *mc_ctx) return ret; } + /* Make sure that the memory cache files are chowned to sssd.sssd even + * if the nss responder runs as root. This is because the specfile + * has the ownership recorded as sssd.sssd + */ + ret = fchown(mc_ctx->fd, mc_ctx->uid, mc_ctx->gid); + if (ret != 0) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, "Failed to chown mmap file %s: %d(%s)\n", + mc_ctx->file, ret, strerror(ret)); + return ret; + } + + ret = fchmod(mc_ctx->fd, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, "Failed to chmod mmap file %s: %d(%s)\n", + mc_ctx->file, ret, strerror(ret)); + return ret; + } + ret = sss_br_lock_file(mc_ctx->fd, 0, 1, retries, t); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, @@ -1224,6 +1249,7 @@ static int mc_ctx_destructor(struct sss_mc_ctx *mc_ctx) } errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name, + uid_t uid, gid_t gid, enum sss_mc_type type, size_t n_elem, time_t timeout, struct sss_mc_ctx **mcc) { @@ -1259,6 +1285,9 @@ errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name, goto done; } + mc_ctx->uid = uid; + mc_ctx->gid = gid; + mc_ctx->type = type; mc_ctx->valid_time_slot = timeout; @@ -1352,7 +1381,9 @@ errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name, return ret; } -errno_t sss_mmap_cache_reinit(TALLOC_CTX *mem_ctx, size_t n_elem, +errno_t sss_mmap_cache_reinit(TALLOC_CTX *mem_ctx, + uid_t uid, gid_t gid, + size_t n_elem, time_t timeout, struct sss_mc_ctx **mc_ctx) { errno_t ret; @@ -1389,12 +1420,26 @@ errno_t sss_mmap_cache_reinit(TALLOC_CTX *mem_ctx, size_t n_elem, timeout = (*mc_ctx)->valid_time_slot; } + if (uid == (uid_t)-1) { + uid = (*mc_ctx)->uid; + } + + if (gid == (gid_t)-1) { + gid = (*mc_ctx)->gid; + } + talloc_free(*mc_ctx); /* make sure we do not leave a potentially freed pointer around */ *mc_ctx = NULL; - ret = sss_mmap_cache_init(mem_ctx, name, type, n_elem, timeout, mc_ctx); + ret = sss_mmap_cache_init(mem_ctx, + name, + uid, gid, + type, + n_elem, + timeout, + mc_ctx); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to re-initialize mmap cache.\n"); goto done; diff --git a/src/responder/nss/nsssrv_mmap_cache.h b/src/responder/nss/nsssrv_mmap_cache.h index b84fbc8ed..e06257949 100644 --- a/src/responder/nss/nsssrv_mmap_cache.h +++ b/src/responder/nss/nsssrv_mmap_cache.h @@ -34,6 +34,7 @@ enum sss_mc_type { }; errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name, + uid_t uid, gid_t gid, enum sss_mc_type type, size_t n_elem, time_t valid_time, struct sss_mc_ctx **mcc); @@ -70,7 +71,9 @@ errno_t sss_mmap_cache_gr_invalidate_gid(struct sss_mc_ctx *mcc, gid_t gid); errno_t sss_mmap_cache_initgr_invalidate(struct sss_mc_ctx *mcc, struct sized_string *name); -errno_t sss_mmap_cache_reinit(TALLOC_CTX *mem_ctx, size_t n_elem, +errno_t sss_mmap_cache_reinit(TALLOC_CTX *mem_ctx, + uid_t uid, gid_t gid, + size_t n_elem, time_t timeout, struct sss_mc_ctx **mc_ctx); void sss_mmap_cache_reset(struct sss_mc_ctx *mc_ctx);
_______________________________________________ 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://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
