URL: https://github.com/SSSD/sssd/pull/389 Author: sumit-bose Title: #389: sssd_client: add mutex protected call to the PAC responder Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/389/head:pr389 git checkout pr389
From 8f27be08378768ed52176d383bf804e82bcd4b74 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Mon, 18 Sep 2017 15:00:53 +0200 Subject: [PATCH] sssd_client: add mutex protected call to the PAC responder SSSD's plugin for MIT Kerberos to send the PAC to the PAC responder currently uses sss_pac_make_request() which does not protect the communication with the PAC responder with a mutex as e.g. the NSS and PAM clients. If an application using threads loads this plugin via libkrb5 in different threads and is heavily processing Kerberos tickets with PACs chances are that two threads try to communicate with SSSD at once. In this case one of the threads will miss a reply and will wait for it until the default client timeout of 300s is passed. This patch adds a call which uses a mutex to protect the communication which will avoid the 300s delay mentioned above. Resolves https://pagure.io/SSSD/sssd/issue/3518 --- Makefile.am | 20 +++++ src/sss_client/common.c | 30 +++++++ src/sss_client/sss_cli.h | 7 ++ src/sss_client/sss_pac_responder_client.c | 137 ++++++++++++++++++++++++++++++ src/sss_client/sssd_pac.c | 4 +- src/tests/intg/Makefile.am | 1 + src/tests/intg/test_pac_responder.py | 120 ++++++++++++++++++++++++++ 7 files changed, 317 insertions(+), 2 deletions(-) create mode 100644 src/sss_client/sss_pac_responder_client.c create mode 100644 src/tests/intg/test_pac_responder.py diff --git a/Makefile.am b/Makefile.am index f1f467100..ed50c4219 100644 --- a/Makefile.am +++ b/Makefile.am @@ -3523,6 +3523,9 @@ endif if BUILD_WITH_LIBCURL noinst_PROGRAMS += tcurl-test-tool endif +if BUILD_PAC_RESPONDER + noinst_PROGRAMS += sssd_pac_test_client +endif if BUILD_AUTOFS autofs_test_client_SOURCES = \ @@ -4257,6 +4260,23 @@ sssd_pac_plugin_la_LDFLAGS = \ -avoid-version \ -module +sssd_pac_test_client_SOURCES = \ + src/sss_client/sss_pac_responder_client.c \ + src/sss_client/common.c \ + src/util/strtonum.c \ + src/sss_client/sss_cli.h \ + src/sss_client/krb5_authdata_int.h \ + $(NULL) +sssd_pac_test_client_CFLAGS = \ + $(AM_CFLAGS) \ + $(KRB5_CFLAGS) \ + $(NULL) +sssd_pac_test_client_LDADD = \ + $(CLIENT_LIBS) \ + $(KRB5_LIBS) \ + -lpthread \ + $(NULL) + # python[23] bindings pysss_la_SOURCES = \ $(SSSD_TOOLS_OBJ) \ diff --git a/src/sss_client/common.c b/src/sss_client/common.c index b7a5ed760..b527c046e 100644 --- a/src/sss_client/common.c +++ b/src/sss_client/common.c @@ -821,6 +821,22 @@ int sss_pac_make_request(enum sss_cli_command cmd, } } +int sss_pac_make_request_with_lock(enum sss_cli_command cmd, + struct sss_cli_req_data *rd, + uint8_t **repbuf, size_t *replen, + int *errnop) +{ + int ret; + + sss_pac_lock(); + + ret = sss_pac_make_request(cmd, rd, repbuf, replen, errnop); + + sss_pac_unlock(); + + return ret; +} + errno_t check_server_cred(int sockfd) { #ifdef HAVE_UCRED @@ -1079,6 +1095,8 @@ static struct sss_mutex sss_pam_mtx = { .mtx = PTHREAD_MUTEX_INITIALIZER }; static struct sss_mutex sss_nss_mc_mtx = { .mtx = PTHREAD_MUTEX_INITIALIZER }; +static struct sss_mutex sss_pac_mtx = { .mtx = PTHREAD_MUTEX_INITIALIZER }; + static void sss_mt_lock(struct sss_mutex *m) { pthread_mutex_lock(&m->mtx); @@ -1121,6 +1139,16 @@ void sss_nss_mc_unlock(void) sss_mt_unlock(&sss_nss_mc_mtx); } +/* PAC mutex wrappers */ +void sss_pac_lock(void) +{ + sss_mt_lock(&sss_pac_mtx); +} +void sss_pac_unlock(void) +{ + sss_mt_unlock(&sss_pac_mtx); +} + #else /* sorry no mutexes available */ @@ -1130,6 +1158,8 @@ void sss_pam_lock(void) { return; } void sss_pam_unlock(void) { return; } void sss_nss_mc_lock(void) { return; } void sss_nss_mc_unlock(void) { return; } +void sss_pac_lock(void) { return; } +void sss_pac_unlock(void) { return; } #endif diff --git a/src/sss_client/sss_cli.h b/src/sss_client/sss_cli.h index 038406dec..0b97d492e 100644 --- a/src/sss_client/sss_cli.h +++ b/src/sss_client/sss_cli.h @@ -585,6 +585,11 @@ int sss_pac_make_request(enum sss_cli_command cmd, uint8_t **repbuf, size_t *replen, int *errnop); +int sss_pac_make_request_with_lock(enum sss_cli_command cmd, + struct sss_cli_req_data *rd, + uint8_t **repbuf, size_t *replen, + int *errnop); + int sss_sudo_make_request(enum sss_cli_command cmd, struct sss_cli_req_data *rd, uint8_t **repbuf, size_t *replen, @@ -634,6 +639,8 @@ void sss_pam_lock(void); void sss_pam_unlock(void); void sss_nss_mc_lock(void); void sss_nss_mc_unlock(void); +void sss_pac_lock(void); +void sss_pac_unlock(void); errno_t sss_readrep_copy_string(const char *in, size_t *offset, diff --git a/src/sss_client/sss_pac_responder_client.c b/src/sss_client/sss_pac_responder_client.c new file mode 100644 index 000000000..cd6df2906 --- /dev/null +++ b/src/sss_client/sss_pac_responder_client.c @@ -0,0 +1,137 @@ + +#include <stdio.h> +#include <stdbool.h> +#include <pthread.h> +#include <pwd.h> +#include <unistd.h> +#include <sys/types.h> +#include <errno.h> + +#include <unistd.h> +#include <sys/syscall.h> + +#include "sss_client/sss_cli.h" + +const uint8_t pac[] = { +0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x10, +0x02, 0x00, 0x00, 0x58, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a, 0x00, +0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x68, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, +0x00, 0x0c, 0x00, 0x00, 0x00, 0x40, 0x00, 0x00, 0x00, 0x78, 0x02, 0x00, 0x00, +0x00, 0x00, 0x00, 0x00, 0x06, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0xb8, +0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x10, 0x00, +0x00, 0x00, 0xc8, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x10, 0x08, +0x00, 0xcc, 0xcc, 0xcc, 0xcc, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, +0x00, 0x00, 0x02, 0x00, 0x30, 0xe3, 0xd6, 0x9e, 0x99, 0x2b, 0xd3, 0x01, 0xff, +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, +0xff, 0x7f, 0xe2, 0xf7, 0x8a, 0xaf, 0x00, 0x0f, 0xd0, 0x01, 0xe2, 0xb7, 0xf4, +0xd9, 0xc9, 0x0f, 0xd0, 0x01, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f, +0x06, 0x00, 0x06, 0x00, 0x04, 0x00, 0x02, 0x00, 0x06, 0x00, 0x06, 0x00, 0x08, +0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0c, 0x00, 0x02, 0x00, 0x00, 0x00, +0x00, 0x00, 0x10, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x14, 0x00, 0x02, +0x00, 0x00, 0x00, 0x00, 0x00, 0x18, 0x00, 0x02, 0x00, 0x45, 0x02, 0x00, 0x00, +0x50, 0x04, 0x00, 0x00, 0x01, 0x02, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x1c, +0x00, 0x02, 0x00, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x12, 0x00, 0x14, +0x00, 0x20, 0x00, 0x02, 0x00, 0x04, 0x00, 0x06, 0x00, 0x24, 0x00, 0x02, 0x00, +0x28, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, +0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, +0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x02, 0x00, +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x74, 0x00, +0x75, 0x00, 0x31, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, +0x00, 0x03, 0x00, 0x00, 0x00, 0x74, 0x00, 0x20, 0x00, 0x75, 0x00, 0x00, 0x00, +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, +0xfd, 0xa2, 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x01, 0x02, 0x00, 0x00, 0x07, +0x00, 0x00, 0x00, 0x5c, 0x04, 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x56, 0x04, +0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x89, 0xa6, 0x00, 0x00, 0x07, 0x00, 0x00, +0x00, 0x0a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x09, 0x00, 0x00, 0x00, +0x41, 0x00, 0x44, 0x00, 0x2d, 0x00, 0x53, 0x00, 0x45, 0x00, 0x52, 0x00, 0x56, +0x00, 0x45, 0x00, 0x52, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, +0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x41, 0x00, 0x44, 0x00, 0x04, 0x00, 0x00, +0x00, 0x01, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x15, 0x00, 0x00, 0x00, +0xf8, 0x12, 0x13, 0xdc, 0x47, 0xf3, 0x1c, 0x76, 0x47, 0x2f, 0x2e, 0xd7, 0x02, +0x00, 0x00, 0x00, 0x30, 0x00, 0x02, 0x00, 0x07, 0x00, 0x00, 0x00, 0x34, 0x00, +0x02, 0x00, 0x07, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x01, 0x05, 0x00, +0x00, 0x00, 0x00, 0x00, 0x05, 0x15, 0x00, 0x00, 0x00, 0x29, 0xc9, 0x4f, 0xd9, +0xc2, 0x3c, 0xc3, 0x78, 0x36, 0x55, 0x87, 0xf8, 0x54, 0x04, 0x00, 0x00, 0x05, +0x00, 0x00, 0x00, 0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x15, 0x00, +0x00, 0x00, 0x25, 0xe1, 0xff, 0x1c, 0xf7, 0x87, 0x6b, 0x2c, 0x25, 0xd2, 0x0c, +0xe3, 0xf2, 0x03, 0x00, 0x00, 0x00, 0x2c, 0x29, 0x89, 0x65, 0x2d, 0xd3, 0x01, +0x06, 0x00, 0x74, 0x00, 0x75, 0x00, 0x31, 0x00, 0x20, 0x00, 0x10, 0x00, 0x10, +0x00, 0x30, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x74, 0x00, +0x75, 0x00, 0x31, 0x00, 0x74, 0x00, 0x65, 0x00, 0x73, 0x00, 0x74, 0x00, 0x40, +0x00, 0x61, 0x00, 0x64, 0x00, 0x2e, 0x00, 0x64, 0x00, 0x65, 0x00, 0x76, 0x00, +0x65, 0x00, 0x6c, 0x00, 0x41, 0x00, 0x44, 0x00, 0x2e, 0x00, 0x44, 0x00, 0x45, +0x00, 0x56, 0x00, 0x45, 0x00, 0x4c, 0x00, 0x10, 0x00, 0x00, 0x00, 0x76, 0x8e, +0x25, 0x32, 0x7c, 0x85, 0x00, 0x32, 0xac, 0x8f, 0x02, 0x2c, 0x10, 0x00, 0x00, +0x00, 0x6b, 0xe8, 0x51, 0x03, 0x30, 0xed, 0xca, 0x7d, 0xe2, 0x12, 0xa5, 0xde}; + +enum nss_status _nss_sss_getpwuid_r(uid_t uid, struct passwd *result, + char *buffer, size_t buflen, int *errnop); +static void *pac_client(void *arg) +{ + struct sss_cli_req_data sss_data = { sizeof(pac), pac}; + int errnop = -1; + int ret; + size_t c; + + fprintf(stderr, "[%ld][%d][%ld][%s] started\n", time(NULL),getpid(), + syscall(SYS_gettid), + (char *) arg); + for(c = 0; c < 1000; c++) { + /* sss_pac_make_request() does not protect the client's file + * descriptor to the PAC responder. With this one thread will miss a + * reply for a SSS_GET_VERSION request and will wait until + * SSS_CLI_SOCKET_TIMEOUT is passed. + + ret = sss_pac_make_request(SSS_PAC_ADD_PAC_USER, &sss_data, + NULL, NULL, &errnop); + */ + ret = sss_pac_make_request_with_lock(SSS_PAC_ADD_PAC_USER, &sss_data, + NULL, NULL, &errnop); + if (ret != NSS_STATUS_SUCCESS + && !(ret == NSS_STATUS_UNAVAIL && errnop != ECONNREFUSED)) { + /* NSS_STATUS_UNAVAIL is returned if the PAC responder rejects + * the request which is ok becasue the client is waiting for a + * response here as well. Only errnop == ECONNREFUSED should + * be treated as error becasue this means that the PAC + * responder is not running. */ + fprintf(stderr, "pac: [%s][%d][%d]\n", (char *) arg, ret, errnop); + return ((void *)((uintptr_t)("X"))); + } + } + + fprintf(stderr, "[%ld][%s] done\n", time(NULL),(char *) arg); + return NULL; +} + +int main(void) +{ + pthread_t thread1; + pthread_t thread2; + int ret; + void *t_ret; + + pthread_create(&thread1, NULL, pac_client, + ((void *)((uintptr_t)("Thread 1")))); + pthread_create(&thread2, NULL, pac_client, + ((void *)((uintptr_t)("Thread 2")))); + + ret = pthread_join(thread1, &t_ret); + if (ret != 0 || t_ret != NULL) { + fprintf(stderr, "Thread 1 failed.\n"); + return EIO; + } + + ret = pthread_join(thread2, &t_ret); + if (ret != 0 || t_ret != NULL) { + fprintf(stderr, "Thread 1 failed.\n"); + return EIO; + } + + return 0; +} diff --git a/src/sss_client/sssd_pac.c b/src/sss_client/sssd_pac.c index 1d98e3882..8444834a7 100644 --- a/src/sss_client/sssd_pac.c +++ b/src/sss_client/sssd_pac.c @@ -169,8 +169,8 @@ static krb5_error_code sssdpac_verify(krb5_context kcontext, sss_data.len = sssdctx->data.length; sss_data.data = sssdctx->data.data; - ret = sss_pac_make_request(SSS_PAC_ADD_PAC_USER, &sss_data, - NULL, NULL, &errnop); + ret = sss_pac_make_request_with_lock(SSS_PAC_ADD_PAC_USER, &sss_data, + NULL, NULL, &errnop); if (ret != 0) { /* Ignore the error */ } diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am index abf6237fc..302825fc2 100644 --- a/src/tests/intg/Makefile.am +++ b/src/tests/intg/Makefile.am @@ -30,6 +30,7 @@ dist_noinst_DATA = \ kdc.py \ krb5utils.py \ test_kcm.py \ + test_pac_responder.py \ $(NULL) config.py: config.py.m4 diff --git a/src/tests/intg/test_pac_responder.py b/src/tests/intg/test_pac_responder.py new file mode 100644 index 000000000..8ffcf675f --- /dev/null +++ b/src/tests/intg/test_pac_responder.py @@ -0,0 +1,120 @@ +# +# SSSD PAC responder tests +# +# Copyright (c) 2017 Red Hat, Inc. +# Author: Sumit Bose <sb...@redhat.com> +# +# This is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; version 2 only +# +# This program is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# +import os +import stat +import time +import config +import signal +import subprocess +import pytest +from util import unindent + + +def stop_sssd(): + pid_file = open(config.PIDFILE_PATH, "r") + pid = int(pid_file.read()) + os.kill(pid, signal.SIGTERM) + while True: + try: + os.kill(pid, signal.SIGCONT) + except: + break + time.sleep(1) + + +def create_conf_fixture(request, contents): + """Generate sssd.conf and add teardown for removing it""" + conf = open(config.CONF_PATH, "w") + conf.write(contents) + conf.close() + os.chmod(config.CONF_PATH, stat.S_IRUSR | stat.S_IWUSR) + request.addfinalizer(lambda: os.unlink(config.CONF_PATH)) + + +def create_sssd_fixture(request): + """Start sssd and add teardown for stopping it and removing state""" + if subprocess.call(["sssd", "-D", "-f"]) != 0: + raise Exception("sssd start failed") + + def teardown(): + try: + stop_sssd() + except: + pass + for path in os.listdir(config.DB_PATH): + os.unlink(config.DB_PATH + "/" + path) + for path in os.listdir(config.MCACHE_PATH): + os.unlink(config.MCACHE_PATH + "/" + path) + request.addfinalizer(teardown) + + +@pytest.fixture +def local_domain_only(request): + conf = unindent("""\ + [sssd] + domains = LOCAL + services = nss, pac + + [nss] + memcache_timeout = 0 + + [domain/LOCAL] + id_provider = local + min_id = 10000 + max_id = 20000 + """).format(**locals()) + create_conf_fixture(request, conf) + create_sssd_fixture(request) + return None + + +@pytest.fixture +def sssd_pac_test_client(request): + path = os.path.join(config.ABS_BUILDDIR, + "..", "..", "..", "sssd_pac_test_client") + if os.access(path, os.X_OK): + return path + + return None + + +def timeout_handler(signum, frame): + raise Exception("Timeout") + + +def test_multithreaded_pac_client(local_domain_only, sssd_pac_test_client): + """ + Test for ticket + https://pagure.io/SSSD/sssd/issue/3518 + """ + + if not sssd_pac_test_client: + pytest.skip("The sssd_pac_test_client is not available, skipping test") + + signal.signal(signal.SIGALRM, timeout_handler) + signal.alarm(10) + + try: + subprocess.check_call(sssd_pac_test_client) + except: + # cancel alarm + signal.alarm(0) + raise Exception("sssd_pac_test_client failed") + + signal.alarm(0)
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
