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

Reviewed-by: Lukáš Slebodník <lslebodn@redhat.com>
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>

7 files changed, 313 insertions, 2 deletions

diff --git a/Makefile.am b/Makefile.am
index f1f467100..c0586d7ec 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,19 @@ 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 \
+    $(NULL)
+sssd_pac_test_client_CFLAGS = \
+    $(AM_CFLAGS) \
+    $(NULL)
+sssd_pac_test_client_LDADD = \
+    $(CLIENT_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..9eb0cbea6
--- /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..4354a5d78
--- /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 <sbose@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():
+    with open(config.PIDFILE_PATH, "r") as pid_file:
+        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)