On Wed, Feb 11, 2015 at 05:50:50PM +0100, Sumit Bose wrote:
On Wed, Feb 11, 2015 at 04:56:48PM +0100, Pavel Reichl wrote:
> Hello,
>
> please see attached patch. I'm not sure whether using pam_strerror() is the
> right thing to do. It might be better to use our own string?
> I'm also not sure about using _(STRING) macro on the output of
> pam_strerror().
The _() macro will not work here. You can use it only to enclose literal
strings. The strings will then be extracted into the *.pot file and
translators can pick them for translation. In the pam_strerror() case
libpam has to take care of the translations.
>
> I attached output of sequence of commands to show differences.
>
> 1) This is output without patch being applied.
> $ su john
> Password:
> su: User account has expired
>
> ssh -l john `hostname`
> Connection closed by 192.168.122.166
>
> #not matching key
> $ ssh -l john `hostname` -i /tmp/local
> john(a)dev.local.test's password:
> Connection closed by 192.168.122.166
>
>
> 2) This is output when patch is applied. Please note the duplicity when
> using su.
The service name is available, so you can add this only if ssh is used.
> $ su john
> Password:
> User account has expired
> su: User account has expired
>
> $ ssh -l john `hostname`
> User account has expired
> Connection closed by 192.168.122.166
>
> #not matching key
> $ ssh -l john `hostname` -i /tmp/local
> john(a)dev.local.test's password:
> User account has expired
> Connection closed by 192.168.122.166
>
> Thanks for comments.
> From 953f1721996e6c2bf8ee53ea232de2240f168d94 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Wed, 11 Feb 2015 19:38:16 -0500
> Subject: [PATCH] PAM: do not reject abruptly
>
> If account has expired use pam_conversation to pass message.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2050
> ---
> src/sss_client/pam_sss.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
> index
fdf6c9e6da75c9f7eaa7c00d9a5792fbdd97eabc..767b2a839e9f001be52c5ff4c7651b0f06ba4221 100644
> --- a/src/sss_client/pam_sss.c
> +++ b/src/sss_client/pam_sss.c
> @@ -1585,6 +1585,13 @@ static int pam_sss(enum sss_cli_command task, pam_handle_t
*pamh,
> D(("do_pam_conversation failed."));
> }
> pam_status = PAM_NEW_AUTHTOK_REQD;
> + } else if (pam_status == PAM_ACCT_EXPIRED) {
> + ret = do_pam_conversation(pamh, PAM_TEXT_INFO,
> + _(pam_strerror(pamh, pam_status)),
> + NULL, NULL);
> + if (ret != PAM_SUCCESS) {
> + D(("do_pam_conversation failed."));
> + }
I would recommend to not do this in pam_sss directly but send a
SSS_PAM_USER_INFO response back to pam_sss. This response can e.g. be
generated in the pam responder if pam_status == PAM_ACCT_EXPIRED and the
service is sshd. Doing it in the pam responder has the advantage that
you do not have to duplicate code in the backends. Additionally it is
more easy configure the behavior. E.g. you can check pam_verbosity and
only add this message is the level is 2 (1?) or higher.
bye,
Sumit
One thing we noticed with Pavel when looking at the issue earlier in the
office was that by default, sshd doesn't display PAM_TEXT_INFO level
messages. And there is one difference between how pam_unix handles
account expiration (they send PAM_ERROR_MSG) and how pam_sss handles it
(PAM_TEXT_INFO), I guess this is also why the original reporter filed
the ticket.
Pavel's idea was to change the PAM_TEXT_INFO messages to PAM_ERROR_MSG on
higher pam_verbosity level..it seems like a bit of a side-effect magic to
me, but I can't think of any better solution either..