On 09/14/2015 05:49 PM, Jakub Hrozek wrote:
On Mon, Sep 07, 2015 at 03:38:32PM +0200, Michal Židek wrote:
Hi,
patch for ticket https://fedorahosted.org/sssd/ticket/2773
is attached.
Michal
From 96215f618f61b8b2b303f0398a41af94292ccf57 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
Date: Mon, 7 Sep 2015 15:19:53 +0200
Subject: [PATCH] PAM: Make p11_child timeout configurable
Ticket:
https://fedorahosted.org/sssd/ticket/2773
---
Makefile.am | 2 +-
src/confdb/confdb.h | 1 +
src/config/SSSDConfig/__init__.py.in | 1 +
src/man/sssd.conf.5.xml | 12 ++++++++++++
src/responder/pam/pamsrv_cmd.c | 35 ++++++++++++++++++++++++++++++-----
5 files changed, 45 insertions(+), 6 deletions(-)
Please also add the new parameter to src/config/etc/sssd.api.conf.
One style-question inline (but really, more of a question..)
static errno_t
@@ -1126,8 +1125,21 @@ static int pam_forwarder(struct cli_ctx *cctx, int
pam_cmd)
}
if (may_do_cert_auth(pctx, pd)) {
+ int p11_child_timeout;
I wonder if the block starts getting so big that it makes sense to add
variables to its scope..isn't it better to just split the block into tis
own function?
Especially if the block is unit-tested?
Yes, the block grew a little too much with the added
option. See the new version.
Michal
>From d094c0f9814b23a8d669940e4c6a7cd8c7077164 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
Date: Mon, 7 Sep 2015 15:19:53 +0200
Subject: [PATCH 1/2] PAM: Make p11_child timeout configurable
Ticket:
https://fedorahosted.org/sssd/ticket/2773
---
Makefile.am | 2 +-
src/confdb/confdb.h | 1 +
src/config/SSSDConfig/__init__.py.in | 1 +
src/config/etc/sssd.api.conf | 1 +
src/man/sssd.conf.5.xml | 12 +++++++
src/responder/pam/pamsrv_cmd.c | 64 +++++++++++++++++++++++-------------
6 files changed, 57 insertions(+), 24 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index dc0670a..1298522 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1893,7 +1893,7 @@ pam_srv_tests_SOURCES = \
pam_srv_tests_CFLAGS = \
-U SSSD_LIBEXEC_PATH -DSSSD_LIBEXEC_PATH=\"$(abs_builddir)\" \
$(AM_CFLAGS) \
- -DSSS_P11_CHILD_TIMEOUT=30 \
+ -DSSS_P11_CHILD_TIMEOUT_DEFAULT=30 \
$(NULL)
pam_srv_tests_LDFLAGS = \
-Wl,-wrap,sss_packet_get_body \
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index 427c309..96b2444 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -118,6 +118,7 @@
#define CONFDB_PAM_ACCOUNT_EXPIRED_MESSAGE "pam_account_expired_message"
#define CONFDB_PAM_CERT_AUTH "pam_cert_auth"
#define CONFDB_PAM_CERT_DB_PATH "pam_cert_db_path"
+#define CONFDB_PAM_P11_CHILD_TIMEOUT "p11_child_timeout"
/* SUDO */
#define CONFDB_SUDO_CONF_ENTRY "config/sudo"
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index cb9c368..038de16 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -89,6 +89,7 @@ option_strings = {
'pam_trusted_users' : _('List of trusted uids or user\'s name'),
'pam_public_domains' : _('List of domains accessible even for untrusted users.'),
'pam_account_expired_message' : _('Message printed when user account is expired.'),
+ 'p11_child_timeout' : _('How many seconds will pam_sss wait for p11_child to finish'),
# [sudo]
'sudo_timed' : _('Whether to evaluate the time-based attributes in sudo rules'),
diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
index f280548..72abb8b 100644
--- a/src/config/etc/sssd.api.conf
+++ b/src/config/etc/sssd.api.conf
@@ -59,6 +59,7 @@ get_domains_timeout = int, None, false
pam_trusted_users = str, None, false
pam_public_domains = str, None, false
pam_account_expired_message = str, None, false
+p11_child_timeout = int, None, false
[sudo]
# sudo service
diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index b858347..9701f2a 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -988,6 +988,18 @@ pam_account_expired_message = Account expired, please call help desk.
</para>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>p11_child_timeout (integer)</term>
+ <listitem>
+ <para>
+ How many seconds will pam_sss wait for
+ p11_child to finish.
+ </para>
+ <para>
+ Default: 10
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</refsect2>
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index aa5c209..9e2adc8 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -43,9 +43,8 @@ enum pam_verbosity {
#define DEFAULT_PAM_VERBOSITY PAM_VERBOSITY_IMPORTANT
-/* TODO: Should we make this configurable? */
-#ifndef SSS_P11_CHILD_TIMEOUT
-#define SSS_P11_CHILD_TIMEOUT 10
+#ifndef SSS_P11_CHILD_TIMEOUT_DEFAULT
+#define SSS_P11_CHILD_TIMEOUT_DEFAULT 10
#endif
static errno_t
@@ -1027,6 +1026,40 @@ static bool is_domain_public(char *name,
return false;
}
+static errno_t check_cert(struct cli_ctx *cctx,
+ struct pam_ctx *pctx,
+ struct tevent_req *req,
+ struct pam_auth_req *preq,
+ struct pam_data *pd)
+{
+ int p11_child_timeout;
+ errno_t ret;
+
+ ret = confdb_get_int(pctx->rctx->cdb, CONFDB_PAM_CONF_ENTRY,
+ CONFDB_PAM_P11_CHILD_TIMEOUT,
+ SSS_P11_CHILD_TIMEOUT_DEFAULT,
+ &p11_child_timeout);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Failed to read p11_child_timeout from confdb: [%d]: %s\n",
+ ret, sss_strerror(ret));
+ return ret;
+ }
+
+ req = pam_check_cert_send(cctx, cctx->ev, pctx->p11_child_debug_fd,
+ pctx->nss_db, p11_child_timeout, pd);
+ if (req == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "pam_check_cert_send failed.\n");
+ return ENOMEM;
+ } else {
+ tevent_req_set_callback(req, pam_forwarder_cert_cb, preq);
+ return EAGAIN;
+ }
+
+ return EOK;
+
+}
+
static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
{
struct sss_domain_info *dom;
@@ -1125,17 +1158,10 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
}
}
- if (may_do_cert_auth(pctx, pd)) {
- req = pam_check_cert_send(cctx, cctx->ev, pctx->p11_child_debug_fd,
- pctx->nss_db, SSS_P11_CHILD_TIMEOUT, pd);
- if (req == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, "pam_check_cert_send failed.\n");
- ret = ENOMEM;
- } else {
- tevent_req_set_callback(req, pam_forwarder_cert_cb, preq);
- ret = EAGAIN;
- }
+ if (may_do_cert_auth(pctx, pd)) {
+ ret = check_cert(cctx, pctx, req, preq, pd);
+ /* Finish here */
goto done;
}
@@ -1342,16 +1368,8 @@ static void pam_forwarder_cb(struct tevent_req *req)
}
if (may_do_cert_auth(pctx, pd)) {
- req = pam_check_cert_send(cctx, cctx->ev, pctx->p11_child_debug_fd,
- pctx->nss_db, SSS_P11_CHILD_TIMEOUT, pd);
- if (req == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, "pam_check_cert_send failed.\n");
- ret = ENOMEM;
- } else {
- tevent_req_set_callback(req, pam_forwarder_cert_cb, preq);
- ret = EAGAIN;
- }
-
+ ret = check_cert(cctx, pctx, req, preq, pd);
+ /* Finish here */
goto done;
}
--
2.1.0
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
