On Thu, Feb 12, 2015 at 03:52:55PM +0100, Pavel Reichl wrote:
> On 02/11/2015 05:50 PM, 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
>>
>>> }
>>> break;
>>> case SSS_PAM_CHAUTHTOK:
>>> --
>>> 2.1.0
>>>
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel(a)lists.fedorahosted.org
>>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel(a)lists.fedorahosted.org
>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> Thanks for comments. Please see updated patch.
Thank you, that patch looks good and is working as expected.
As a side note, when using IPA or AD with passwords already the
authentication fails for expired account and it looks like ssh does not
show PAM messages during the authentication phase, you will only see:
# ssh -l exp_user(a)ad.devel localhost
exp_user@ad.devel(a)localhost's password:
Permission denied, please try again.
exp_user@ad.devel(a)localhost's password:
Permission denied, please try again.
exp_user@ad.devel(a)localhost's password:
Permission denied (publickey,gssapi-keyex,gssapi-with-mic,password).
But I think it is ok, since ssh says 'Permission denied' and the PAM
message can still be found in the secure log or journal.
I was thinking about the "Your account has expired. " message which
might give an attacker the information that it is not worth to continue
with this account. My first idea was to only show the message is
pam_verbosity is 2 or higher. But this won't cover the ssh case in the
default installation.
My suggestion would be to make the message mandatory in
SSS_PAM_USER_INFO_ACCOUNT_EXPIRED and by default added 'Permission
denied' as a message in the PAM responder. For more flexibility we might
want to add a pam_account_expired_message option which defaults to
'Permission denied' but can be set by the admin to something like
'Account expired, please call help desk'.
Additionally you might want to consider to add this message for all
services if pam_verbosity is 2 or higher. This would make is easier to
cover other services which do not add useful error messages on their own
like ssh. And since it is not the default having multiple messages for
services like su might be acceptable.
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel Thanks, for feedback.
Updated patch set is attached.
I didn't do any changes in the first patch.