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 > >>>>j...@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 > >>>>j...@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 <prei...@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@lists.fedorahosted.org > >>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > >>>_______________________________________________ > >>>sssd-devel mailing list > >>>sssd-devel@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@ad.devel localhost > >exp_user@ad.devel@localhost's password: > >Permission denied, please try again. > >exp_user@ad.devel@localhost's password: > >Permission denied, please try again. > >exp_user@ad.devel@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@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
