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(a)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 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.