[PATCH] PAM: Allow to configure pam services for smartcards - sssd-devel - Fedora mailing-lists

List overview All Threads
Download

[PATCH] PAM: Allow to configure pam services for smartcards

[PATCH] sssctl: Fix error...

Announce - SSSD 1.13 is the new...

Lukas Slebodnik

20 Jan 2016 20 Jan '16

12:35 p.m.

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.

LS

Attachments:

0001-PAM-Allow-to-configure-pam-services-for-smartcards.patch (text/plain — 11.9 KB)

Reply

Show replies by date

Sumit Bose

15 Feb 15 Feb

12:23 p.m.

New subject: [PATCH] PAM: Allow to configure pam services for smartcards

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 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.

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

Reply

Jakub Hrozek

28 Jun 28 Jun

4:50 p.m.

New subject: [PATCH] PAM: Allow to configure pam services for smartcards

On Mon, Feb 15, 2016 at 01:23:52PM +0100, 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

Do we still want to make this patch part of 1.14?

Reply

Lukas Slebodnik

4:57 p.m.

New subject: [PATCH] PAM: Allow to configure pam services for smartcards

On (28/06/16 18:50), Jakub Hrozek wrote:

On Mon, Feb 15, 2016 at 01:23:52PM +0100, 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

Do we still want to make this patch part of 1.14?

Don't worry it's on my todo list.

LS

Reply

Lukas Slebodnik

6 Jul 6 Jul

9:04 a.m.

New subject: [PATCH] PAM: Allow to configure pam services for smartcards

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 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 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

Reply

Jakub Hrozek

9:10 a.m.

New subject: [PATCH] PAM: Allow to configure pam services for smartcards

On Wed, Jul 06, 2016 at 11:04:56AM +0200, Lukas Slebodnik wrote:

Updated patch is attached.

it is not :-)

Reply

Lukas Slebodnik

9:16 a.m.

New subject: [PATCH] PAM: Allow to configure pam services for smartcards

On (06/07/16 11:10), Jakub Hrozek wrote:

On Wed, Jul 06, 2016 at 11:04:56AM +0200, Lukas Slebodnik wrote:

...
Updated patch is attached.

it is not :-)

Ahh,

sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

Reply

Sumit Bose

8 Jul 8 Jul

4:12 p.m.

New subject: [PATCH] PAM: Allow to configure pam services for smartcards

On Wed, Jul 06, 2016 at 11:10:38AM +0200, Jakub Hrozek wrote:

On Wed, Jul 06, 2016 at 11:04:56AM +0200, Lukas Slebodnik wrote:

...
Updated patch is attached.

it is not :-)

(for whatever reasons Lukas' reply with the patch was not delivered to my inbox, so I picked it from the archive)

The patch looks good and works as expected. I only have a few comments:

diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index d5442d0cd82fbf31db5e0aef5b7fd32d2b7fff94..4cd2e5727922cc32011c96e23ff9e70309405955 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -1126,6 +1126,86 @@ pam_account_locked_message = Account locked, please contact 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 it will be allowed to use smartcards.

```
                   </para>
```
```
                   <para>
```

                       It is possible 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

                       authentication with smartcads

typo ^^^^^^^^^

                       (e.g. <quote>login</quote>) with a custom pam

                       service name (e.g. <quote>my_pam_service</quote>),

                       you would use the following configuration:

...

```
       break;
```
```
   default:
```
```
       DEBUG(SSSDBG_OP_FAILURE,
```

I would suggest to make this SSSDBG_CRIT_FAILURE as well, to match the other messages related to issues parsing the config option.

             "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;
```
```
   }
```
}

...

/* Initialize smartcard allowed services just once */
if (pctx->smartcard_services == NULL) {

   ret = get_sc_services(pctx, pctx, &pctx->smartcard_services);

```
   if (ret != EOK) {
```
```
       DEBUG(SSSDBG_CRIT_FAILURE,
```

             "Failed to get p11 allowed services %d[%s]",

```
             ret, sss_strerror(ret));
```
```
       return false;
```
```
   }
```
}

I wonder if it would be better to initialize pctx->smartcard_services during the initialization of the PAM responder. Because the this point we can fail completely and a broken configuration would be more obvious? If we keep it here maybe a sss_log() messages would make sense to better indicate the config error?

bye, Sumit

sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

Reply

3070

Age (days ago)

3240

Last active (days ago)

sssd-devel@lists.fedorahosted.org

7 comments

3 participants

tags (0)

participants (3)

Jakub Hrozek
Lukas Slebodnik
Sumit Bose