On Wed, Jan 20, 2016 at 01:35:01PM +0100, Lukas Slebodnik wrote:
ehlo,
attached patch should fix ticket #2926
The default case (no adding, no removing services) is tested by pam-srv-tests.
If the patch is not ready for stable branch sssd-1-13 then we might merge this case with GPO code which uses slightly modified way using hash tables.
Hi Lukas,
sorry for the long delay. Yes, I think it would be a good idea to merge this case with the GPO code, i.e. to make ad_gpo_parse_map_option() and ad_gpo_parse_map_option_helper() some generic utility functions. This of course would make most of my comments below useless.
bye, Sumit
LS
From 2f30fe59d2b5b038160e00b02948b9521e60f9af Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Wed, 20 Jan 2016 13:15:11 +0100 Subject: [PATCH] PAM: Allow to configure pam services for smartcards
Resolves: https://fedorahosted.org/sssd/ticket/2926
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 | 82 ++++++++++++++++++++ src/responder/pam/pamsrv_p11.c | 145 ++++++++++++++++++++++++++++++++++- 5 files changed, 227 insertions(+), 3 deletions(-)
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index fcffcb5a6ff8b3f766ed9a693db874c7c6e3d9b9..9e053305a70952c6076fb37c7a9a869c36e32b54 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -121,6 +121,7 @@ #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" +#define CONFDB_PAM_P11_ALLOWED_SERVICES "pam_p11_allowed_services"
/* 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 647d081255d738c028f73bc5d65eff58cebdbdf1..260e306a95b38dd1ec6cded10360fdb65074e44c 100644 --- a/src/config/SSSDConfig/__init__.py.in +++ b/src/config/SSSDConfig/__init__.py.in @@ -92,6 +92,7 @@ option_strings = { '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'),
'pam_p11_allowed_services' : _('Allowed services for using smartcards'),
# [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 89cf8634ffb8115d9e65cf66dc9b1ed630415c15..74dc588ad0a68aaad9708b45143ffdc9ae90aeb1 100644 --- a/src/config/etc/sssd.api.conf +++ b/src/config/etc/sssd.api.conf @@ -62,6 +62,7 @@ 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 +pam_p11_allowed_services = str, None, false
[sudo] # sudo service diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 73a21bfa0049bc4d3cfacb49201707868c87e533..8d45b9229035d510fc46e7067459b38512892f42 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -1051,6 +1051,88 @@ pam_account_expired_message = Account expired, please call help desk. </para> </listitem> </varlistentry>
<varlistentry><term>pam_p11_allowed_services (integer)</term><listitem><para>A comma-separated list of PAM service names forwhich will be alloed to use smartcards.
allowed
How many seconds will pam_sss wait forp11_child to finish.
copy-and-paste error
</para><para>it is possibel to add another PAM service name to
^ ^
the default set by using<quote>+service_name</quote> or to explicitlyremove a PAM service name from the default set byusing <quote>-service_name</quote>. For example,in order to replace a default PAM service name forthis logon right (e.g. <quote>login</quote>) with
^^^^^^^^^^^
a custom pam service name (e.g.<quote>my_pam_service</quote>),you would use the following configuration:<programlisting>+pam_p11_allowed_services = +my_pam_service, -login
</programlisting></para><para>Default: the default set of PAM service namesincludes:<itemizedlist><listitem><para>login</para></listitem><listitem><para>su</para></listitem><listitem><para>su-l</para></listitem><listitem><para>gdm-smartcard</para></listitem><listitem><para>gdm-password</para></listitem><listitem><para>gnome-screensaver</para></listitem><listitem><para>kdm</para></listitem><listitem><para>sudo</para></listitem><listitem><para>sudo-i</para></listitem><listitem><para>gdm-smartcard</para></listitem></itemizedlist></para></listitem></varlistentry> </variablelist> </refsect2>diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c index ad1670136dbf8efc41df6950af744ff8b06e6a11..f836c96453cfb9ac24fe86e83ca5267784b51d7e 100644 --- a/src/responder/pam/pamsrv_p11.c +++ b/src/responder/pam/pamsrv_p11.c @@ -40,12 +40,141 @@ errno_t p11_child_init(struct pam_ctx *pctx) return child_debug_init(P11_CHILD_LOG_FILE, &pctx->p11_child_debug_fd); }
+static inline bool +service_in_list(char **list, size_t nlist, const char *str) +{
- size_t i;
- for (i = 0; i < nlist; i++) {
if (strcasecmp(list[i], str) == 0) {break;}- }
- return (i < nlist) ? true : false;
I like that you make the return values explicit even though 'return (i < nlist);' would be sufficient.
+}
+static errno_t get_sc_services(TALLOC_CTX *mem_ctx, struct pam_ctx *pctx,
char ***_sc_list)+{
- TALLOC_CTX *tmp_ctx;
- errno_t ret;
- char *conf_str;
- char **conf_list;
- int conf_list_size;
- char **add_list;
- char **remove_list;
- int ai = 0;
- int ri = 0;
- int j = 0;
- char **sc_list;
- int expected_sc_list_size;
- const char *default_sc_services[] = {
"login", "su", "su-l", "gdm-smartcard", "gdm-password", "kdm", "sudo","sudo-i", "gnome-screensaver", NULL,- };
- const int default_sc_services_size =
sizeof(default_sc_services) / sizeof(default_sc_services[0]);- tmp_ctx = talloc_new(mem_ctx);
- if (tmp_ctx == NULL) {
return ENOMEM;- }
- ret = confdb_get_string(pctx->rctx->cdb, tmp_ctx, CONFDB_PAC_CONF_ENTRY,
^^^
I guess you meant CONFDB_PAM_CONF_ENTRY
CONFDB_PAM_P11_ALLOWED_SERVICES, NULL,&conf_str);- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,"confdb_get_string failed %d [%s]\n", ret, sss_strerror(ret));goto done;- }
- if (conf_str != NULL) {
ret = split_on_separator(tmp_ctx, conf_str, ',', true, true,&conf_list, &conf_list_size);if (ret != EOK) {DEBUG(SSSDBG_CRIT_FAILURE,"Cannot parse list of service names '%s': %d [%s]\n",conf_str, ret, sss_strerror(ret));goto done;}
either conf_list_size must be incremented by 1 here
- } else {
conf_list = talloc_zero_array(tmp_ctx, char *, 1);conf_list_size = 1;
or conf_list_size set to 0 here
- }
- add_list = talloc_zero_array(tmp_ctx, char *, conf_list_size);
- remove_list = talloc_zero_array(tmp_ctx, char *, conf_list_size);
'conf_list_size + 1' used here to make sure the arrays are properly terminated with NULL in the case the option is set in sssd.conf.
- if (add_list == NULL || remove_list == NULL) {
ret = ENOMEM;goto done;- }
- for (int i = 0; conf_list[i] != NULL; ++i) {
switch (conf_list[i][0]) {case '+':add_list[ai] = conf_list[i] + 1;++ai;break;case '-':remove_list[ri] = remove_list[i] + 1;
typo: 'remove_list[ri] = conf_list[i] + 1;'
++ri;break;default:DEBUG(SSSDBG_OP_FAILURE,"The option "CONFDB_PAM_P11_ALLOWED_SERVICES" must start""with either '+' (for adding service) or '-' (for ""removing service) got '%s'\n", conf_list[i]);ret = EINVAL;goto done;}- }
- expected_sc_list_size = default_sc_services_size + ai + 1;
- sc_list = talloc_zero_array(tmp_ctx, char *, expected_sc_list_size);
- if (sc_list == NULL) {
ret = ENOMEM;goto done;- }
- for (int i = 0; add_list[i] != NULL; ++i) {
if (service_in_list(remove_list, ri, add_list[i])) {continue;}sc_list[j] = talloc_strdup(sc_list, add_list[i]);
Missing check if talloc_strdup() failed.
++j;- }
- for (int i = 0; default_sc_services[i] != NULL; ++i) {
if (service_in_list(remove_list, ri, default_sc_services[i])) {continue;}sc_list[j] = talloc_strdup(sc_list, default_sc_services[i]);
Missing check if talloc_strdup() failed.
++j;- }
- if (_sc_list != NULL) {
*_sc_list = talloc_steal(mem_ctx, sc_list);- }
+done:
- talloc_zfree(tmp_ctx);
- return ret;
+}
bool may_do_cert_auth(struct pam_ctx *pctx, struct pam_data *pd) { size_t c;
- const char *sc_services[] = { "login", "su", "su-l", "gdm-smartcard",
"gdm-password", "kdm", "sudo", "sudo-i","gnome-screensaver", NULL };
- errno_t ret;
- char **sc_services = NULL;
- if (!pctx->cert_auth) { return false; }
@@ -64,6 +193,14 @@ bool may_do_cert_auth(struct pam_ctx *pctx, struct pam_data *pd)
There is a TODO comment before this line which can be removed with the patch.
if (pd->service == NULL || *pd->service == '\0') { return false; }
- ret = get_sc_services(pctx, pctx, &sc_services);
I think it would be more efficient to generate sc_services only once and store if somewhere in pctx.
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,"Failed to ged p11 allowed services %d[%s]",ret, sss_strerror(ret));- }
- for (c = 0; sc_services[c] != NULL; c++) {
The new service_in_list() call can be used here.
if (strcmp(pd->service, sc_services[c]) == 0) { break;@@ -73,9 +210,11 @@ bool may_do_cert_auth(struct pam_ctx *pctx, struct pam_data *pd) DEBUG(SSSDBG_CRIT_FAILURE, "Smartcard authentication for service [%s] not supported.\n", pd->service);
talloc_zfree(sc_services); return false;}
talloc_zfree(sc_services); return true;
}
-- 2.5.0
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org