On Fri, Jun 03, 2016 at 05:56:45PM +0200, Jakub Hrozek wrote:
On Wed, Jun 01, 2016 at 06:31:29PM +0200, Sumit Bose wrote:
> Hi,
>
> that attached two patches would allow to use the Smartcard support in
> gdm with SSSD. To use it you should replace pam_pkcs11 in
> /etc/pam.d/smartcard-auth in the auth section by
>
> auth sufficient pam_sss.so allow_missing_name
>
> and drop the password section completely.
>
> To enable the Smartcard support in gdm the easiest way is to use
> dconf-editor:
>
> DCONF_PROFILE=gdm dconf-editor
>
> In the org/gnome/login-screen section you can switch the Smartcard
> support on and off. Additionally you might want to tune the removal
> action in org/gnome/settings-daemon/peripherals/smartcard .
>
> If now a Smartcard is inserted gdm should register it, call
> /etc/pam.d/gdm-smartcard which calls /etc/pam.d/smartcard-auth without a
> user name. With the new option from the first patch pam_sss will accept
> this and send it to the pam responder. The pam responder can handle this
> if Smartcard authentication is enabled, tries to read the certificate
> from the Smartcard, tries to find and matching user and if successful,
> returns the user name to pam_sss which puts it on the PAM stack and
> continues with the authentication.
>
> It would be nice if someone can review the code even without testing the
> functionality. In this case I will ask someone else with access to
> Smartcards and reader to do some functional testing.
>
> I think these patches are candidates for the pam wrapper based tests
> Jakub has for review on the list. I'll start reviewing those and add
> tests when they are in master.
The code looks good to me with some minor nitpicks (see inline) but at
least for me, the tests are failing:
[ RUN ] test_pam_offline_chauthtok_prelim
[ ERROR ] --- 0x2 != 0x3
[ LINE ] --- /home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_pam_srv.c:641:
error: Failure!
[ FAILED ] test_pam_offline_chauthtok_prelim
[ RUN ] test_pam_offline_chauthtok
[ ERROR ] --- 0x2 != 0x3
[ LINE ] --- /home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_pam_srv.c:641:
error: Failure!
[ FAILED ] test_pam_offline_chauthtok
Do I need some other patches applied as well?
Not that I'm aware of. So far I was not able to reproduce the error
locally not with CI
http://sssd-ci.duckdns.org/logs/job/44/49/summary.html . Do you maybe
have your pam wrapper patches applied to check for regressions?
>
> bye,
> Sumit
> From 0a58ab569a7746aab54ec8e38cebce4584f0b145 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Mon, 14 Mar 2016 17:27:01 +0100
> Subject: [PATCH 1/2] PAM: add pam_sss option allow_missing_name
>
> With this option SSSD can be used with the gdm Smartcard feature.
>
> Resolves
https://fedorahosted.org/sssd/ticket/2941
> ---
> src/man/pam_sss.8.xml | 27 +++++++++++++++++++++++++++
> src/sss_client/pam_sss.c | 40 ++++++++++++++++++++++++++++++++++++----
> 2 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/src/man/pam_sss.8.xml b/src/man/pam_sss.8.xml
> index
7794d3acfdfdbde491a3e1ada44481b73588e41f..5dc08f8a5a3dee9d3bf8979594ce9c0b16bb8bbf 100644
> --- a/src/man/pam_sss.8.xml
> +++ b/src/man/pam_sss.8.xml
> @@ -46,6 +46,9 @@
> <arg choice='opt'>
> <replaceable>domains=X</replaceable>
> </arg>
> + <arg choice='opt'>
> + <replaceable>allow_missing_name</replaceable>
> + </arg>
> </cmdsynopsis>
> </refsynopsisdiv>
>
> @@ -157,6 +160,30 @@
> </para>
> </listitem>
> </varlistentry>
> + <varlistentry>
> + <term>
> + <option>allow_missing_name</option>
> + </term>
> + <listitem>
> + <para>
> + The main purpose of this option is to let SSSD determine
> + the user name based on additional information, e.g. the
> + certificate from a Smartcard.
> + </para>
> + <para>
I would prefer to capitalize PAM instead of pam to be consistent with the
rest of the manpage.
Make sense, I fixed it in the new version.
> + The current use case are login managers which can
> + monitor a Smartcard reader for card events. In case a
> + Smartcard is inserted the login manager will call a pam
> + stack which includes a line like
> + <programlisting>
> + auth sufficient pam_sss.so allow_missing_name
> + </programlisting>
> + In this case SSSD will try to determine the user name
> + based on the content of the Smartcard, returns it to
> + pam_sss which will finally put it on the pam stack.
> + </para>
> + </listitem>
> + </varlistentry>
> </variablelist>
> </refsect1>
>
> diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
> index
5edbe96609b1599a0fab5d0e5b0b06cf43b9c27a..25140c6d78583885d0c1ca202b2d76055f9923f0 100644
> --- a/src/sss_client/pam_sss.c
> +++ b/src/sss_client/pam_sss.c
> @@ -53,6 +53,7 @@
> #define FLAGS_IGNORE_UNKNOWN_USER (1 << 3)
> #define FLAGS_IGNORE_AUTHINFO_UNAVAIL (1 << 4)
> #define FLAGS_USE_2FA (1 << 5)
> +#define FLAGS_ALLOW_MISSING_NAME (1 << 6)
>
> #define PWEXP_FLAG "pam_sss:password_expired_flag"
> #define FD_DESTRUCTOR "pam_sss:fd_destructor"
> @@ -977,6 +978,27 @@ static int eval_response(pam_handle_t *pamh, size_t buflen,
uint8_t *buf,
> break;
> }
>
> + if (pi->pam_user == NULL || *(pi->pam_user) == '\0')
{
Can the first condition ever be true? We set pam_user to "" forcibly..
Currently not, but I wanted to be on the safe side to avoid issues with
changes in future. Additionally it might irritate the occasional reader
to dereference a pointer without checking that it is not NULL. So, if
you don't mind I would prefer to keep it.
bye,
Sumit
> + ret = pam_set_item(pamh, PAM_USER, pi->cert_user);
> + if (ret != PAM_SUCCESS) {
> + D(("Failed to set PAM_USER during "
> + "Smartcard authentication [%s]",
> + pam_strerror(pamh,ret)));
> + break;
> + }
> +
> + ret = pam_get_item(pamh, PAM_USER,
> + (const void **) &(pi->pam_user));
> + if (ret != PAM_SUCCESS) {
> + D(("Failed to get PAM_USER during "
> + "Smartcard authentication [%s]",
> + pam_strerror(pamh,ret)));
> + break;
> + }
> +
> + pi->pam_user_size=strlen(pi->pam_user)+1;
> + }
> +
> offset = strlen(pi->cert_user) + 1;
> if (offset >= len) {
> From 1335e1fdb21e55b66d32746dabdf86d4692fc5de Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Thu, 17 Mar 2016 17:20:52 +0100
> Subject: [PATCH 2/2] p11: add PKCS11_LOGIN_TOKEN_NAME environment variable
ACK
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org