On Fri, Feb 20, 2015 at 04:01:28PM +0100, Pavel Reichl wrote:
On 02/20/2015 11:48 AM, Sumit Bose wrote:
>On Thu, Feb 19, 2015 at 06:03:03PM +0100, Pavel Reichl wrote:
>>On 02/19/2015 10:54 AM, Sumit Bose wrote:
>>>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.
>>
>Thank you, patches are looking good and pass CI
>http://sssd-ci.duckdns.org/logs/job/7/77/summary.html .
>
>Nevertheless I've found two small issue:
>
>...
>
>>diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
>>index
fdf6c9e6da75c9f7eaa7c00d9a5792fbdd97eabc..9b8fe0dcb09ca30b5bc1ff4c9b68b329e1839852 100644
>>--- a/src/sss_client/pam_sss.c
>>+++ b/src/sss_client/pam_sss.c
>>@@ -60,6 +60,9 @@
>> #define OPT_RETRY_KEY "retry="
>> #define OPT_DOMAINS_KEY "domains="
>>+#define EXP_ACC_MSG "Your account has expired. "
>>+#define SRV_MSG "Server message: "
>>+
>> struct pam_items {
>> const char* pam_service;
>> const char* pam_user;
>>@@ -797,6 +800,63 @@ static int user_info_otp_chpass(pam_handle_t *pamh)
>> return PAM_SUCCESS;
>> }
>...
>
>>+
>>+ bufsize = strlen(_(EXP_ACC_MSG)) + 1;
>>+
>>+ if (msg_len > 0) {
>>+ bufsize += strlen(_(SRV_MSG)) + msg_len;
>>+ }
>>+
>>+ user_msg = (char *)malloc(sizeof(char) * bufsize);
>>+ if (!user_msg) {
>>+ D(("Out of memory."));
>>+ return PAM_SYSTEM_ERR;
>>+ }
>>+
>>+ ret = snprintf(user_msg, bufsize, "%s%s%.*s",
>>+ _(EXP_ACC_MSG),
>>+ msg_len > 0 ? _(SRV_MSG) : "",
>>+ msg_len,
>>+ msg_len > 0 ? (char *)(buf + 2 * sizeof(uint32_t)) :
"" );
>>+ if (ret < 0 || ret > bufsize) {
>>+ D(("snprintf failed."));
>>+
>>+ free(user_msg);
>>+ return PAM_SYSTEM_ERR;
>>+ }
>>+
>The messages are not properly added to the pot file, I think you have to
>add the _() already in the #define lines.
>
>...
Fixed.
>>@@ -570,7 +572,6 @@ static void pam_reply(struct pam_auth_req *preq)
>> " [%s]!\n", preq->domain->name);
>> goto done;
>> }
>>-
>> ret = sss_authtok_get_password(pd->authtok, &password,
NULL);
>> if (ret) {
>> DEBUG(SSSDBG_FATAL_FAILURE, "Failed to get
password.\n");
>why do you not like this extra line ?
It's not personal. It's just a typo. :-)
Fixed.
>
>
>bye,
>Sumit
>_______________________________________________
>sssd-devel mailing list
>sssd-devel(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks!
ACK
http://sssd-ci.duckdns.org/logs/job/7/78/summary.html
bye,
Sumit