On (15/02/16 13:23), Sumit Bose wrote: >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 <[email protected]> >> 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 for >> + which will be alloed to use smartcards. > >allowed > >> + How many seconds will pam_sss wait for >> + p11_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 explicitly >> + remove a PAM service name from the default set >> by >> + using <quote>-service_name</quote>. For example, >> + in order to replace a default PAM service name >> for >> + this 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 names >> + includes: >> + <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. > Thank you for careful review. Updated patch is attached.
LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
