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 <sb...@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 <sb...@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@lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
From 85f01de41a9fa4764aa387afbab5a013756a2682 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@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..b03ac2dc8616aabf27810b76e9156c2da2192310
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>
+ 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
5b2307c1b59e2de5d52fdc871b12afaa90780f76..8f9336aaefea3373633a3d6d58306e86e62c7abb
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') {
+ 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) {
D(("Cert message size mismatch"));
@@ -1003,7 +1025,7 @@ static int eval_response(pam_handle_t *pamh, size_t
buflen, uint8_t *buf,
return PAM_SUCCESS;
}
-static int get_pam_items(pam_handle_t *pamh, struct pam_items *pi)
+static int get_pam_items(pam_handle_t *pamh, uint32_t flags, struct pam_items
*pi)
{
int ret;
@@ -1021,10 +1043,18 @@ static int get_pam_items(pam_handle_t *pamh, struct
pam_items *pi)
pi->pam_service_size=strlen(pi->pam_service)+1;
ret = pam_get_item(pamh, PAM_USER, (const void **) &(pi->pam_user));
+ if (ret == PAM_PERM_DENIED && (flags & FLAGS_ALLOW_MISSING_NAME)) {
+ pi->pam_user = "";
+ ret = PAM_SUCCESS;
+ }
if (ret != PAM_SUCCESS) return ret;
if (pi->pam_user == NULL) {
- D(("No user found, aborting."));
- return PAM_BAD_ITEM;
+ if (flags & FLAGS_ALLOW_MISSING_NAME) {
+ pi->pam_user = "";
+ } else {
+ D(("No user found, aborting."));
+ return PAM_BAD_ITEM;
+ }
}
if (strcmp(pi->pam_user, "root") == 0) {
D(("pam_sss will not handle root."));
@@ -1512,6 +1542,8 @@ static void eval_argv(pam_handle_t *pamh, int argc, const
char **argv,
*flags |= FLAGS_IGNORE_AUTHINFO_UNAVAIL;
} else if (strcmp(*argv, "use_2fa") == 0) {
*flags |= FLAGS_USE_2FA;
+ } else if (strcmp(*argv, "allow_missing_name") == 0) {
+ *flags |= FLAGS_ALLOW_MISSING_NAME;
} else {
logger(pamh, LOG_WARNING, "unknown option: %s", *argv);
}
@@ -1676,7 +1708,7 @@ static int pam_sss(enum sss_cli_command task,
pam_handle_t *pamh,
pi.requested_domains = domains;
- ret = get_pam_items(pamh, &pi);
+ ret = get_pam_items(pamh, flags, &pi);
if (ret != PAM_SUCCESS) {
D(("get items returned error: %s", pam_strerror(pamh,ret)));
if (flags & FLAGS_IGNORE_UNKNOWN_USER && ret == PAM_USER_UNKNOWN) {
--
2.1.0
From c6a799c0fd2f5fe3b346b6eafdd34f34b91d0e5d Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Thu, 17 Mar 2016 17:20:52 +0100
Subject: [PATCH 2/2] p11: add PKCS11_LOGIN_TOKEN_NAME environment variable
The PKCS11_LOGIN_TOKEN_NAME environment variable is e.g. used by the
Gnome Settings Daemon to determine the name of the token used for login.
---
src/responder/pam/pamsrv_p11.c | 25 +++++++++++++++++++++++++
src/tests/cmocka/test_pam_srv.c | 14 +++++++++++++-
2 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c
index
4d35e1d34b75fa32e21770076a67750a21c5b779..5fb7ac36a43dc6b397084f20c022dbd798ea62bb
100644
--- a/src/responder/pam/pamsrv_p11.c
+++ b/src/responder/pam/pamsrv_p11.c
@@ -504,10 +504,15 @@ errno_t pam_check_cert_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
return EOK;
}
+/* The PKCS11_LOGIN_TOKEN_NAME environment variable is e.g. used by the Gnome
+ * Settings Daemon to determine the name of the token used for login */
+#define PKCS11_LOGIN_TOKEN_ENV_NAME "PKCS11_LOGIN_TOKEN_NAME"
+
errno_t add_pam_cert_response(struct pam_data *pd, const char *user,
const char *token_name)
{
uint8_t *msg = NULL;
+ char *env = NULL;
size_t user_len;
size_t msg_len;
size_t slot_len;
@@ -533,6 +538,26 @@ errno_t add_pam_cert_response(struct pam_data *pd, const
char *user,
ret = pam_add_response(pd, SSS_PAM_CERT_INFO, msg_len, msg);
talloc_free(msg);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "pam_add_response failed to add certificate info.\n");
+ return ret;
+ }
+
+ env = talloc_asprintf(pd, "%s=%s", PKCS11_LOGIN_TOKEN_ENV_NAME,
token_name);
+ if (env == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n");
+ return ENOMEM;
+ }
+
+ ret = pam_add_response(pd, SSS_PAM_ENV_ITEM, strlen(env) + 1,
+ (uint8_t *) env);
+ talloc_free(env);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "pam_add_response failed to add environment variable.\n");
+ return ret;
+ }
return ret;
}
diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c
index
1e3ac542cf4610cd411f7b335930d8e1a1753e89..c859862234f1cdc45e4d00dc7c7ae7ccfdfdc66c
100644
--- a/src/tests/cmocka/test_pam_srv.c
+++ b/src/tests/cmocka/test_pam_srv.c
@@ -572,6 +572,8 @@ static int test_pam_simple_check(uint32_t status, uint8_t
*body, size_t blen)
return EOK;
}
+#define PKCS11_LOGIN_TOKEN_ENV_NAME "PKCS11_LOGIN_TOKEN_NAME"
+
static int test_pam_cert_check(uint32_t status, uint8_t *body, size_t blen)
{
size_t rp = 0;
@@ -583,7 +585,7 @@ static int test_pam_cert_check(uint32_t status, uint8_t
*body, size_t blen)
assert_int_equal(val, pam_test_ctx->exp_pam_status);
SAFEALIGN_COPY_UINT32(&val, body + rp, &rp);
- assert_int_equal(val, 2);
+ assert_int_equal(val, 3);
SAFEALIGN_COPY_UINT32(&val, body + rp, &rp);
assert_int_equal(val, SSS_PAM_DOMAIN_NAME);
@@ -596,6 +598,16 @@ static int test_pam_cert_check(uint32_t status, uint8_t
*body, size_t blen)
rp += val;
SAFEALIGN_COPY_UINT32(&val, body + rp, &rp);
+ assert_int_equal(val, SSS_PAM_ENV_ITEM);
+
+ SAFEALIGN_COPY_UINT32(&val, body + rp, &rp);
+ assert_int_equal(val, (strlen(PKCS11_LOGIN_TOKEN_ENV_NAME "=")
+ + sizeof(TEST_TOKEN_NAME)));
+ assert_string_equal(body + rp,
+ PKCS11_LOGIN_TOKEN_ENV_NAME "=" TEST_TOKEN_NAME);
+ rp += val;
+
+ SAFEALIGN_COPY_UINT32(&val, body + rp, &rp);
assert_int_equal(val, SSS_PAM_CERT_INFO);
SAFEALIGN_COPY_UINT32(&val, body + rp, &rp);
--
2.1.0
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
