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.
...
@@ -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 ?
bye,
Sumit