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 > >>>>>>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. > > > >... > 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@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
