Because each keyboard-interactive attempt is effectively self-contained, when the PAM account check fails, the user is reprompted, even though they can never possible succeed (since do_pam_account() caches the result). Eg: $ ssh localhost Password: Your account has expired; please contact your system administrator Password: sshd should prevent further keyboard-interactive attempts if the PAM account check fails.
Created attachment 1130 [details] prevent retry of keyboard-interactive if PAM account check fails.
Comment on attachment 1130 [details] prevent retry of keyboard-interactive if PAM account check fails. Looks OK to me as far as PAM goes, but you should update the copyright year while you are in there. Would it make sense to use a global flag? so a PAM authorization failure blocks all other methods...
(In reply to comment #2) > Would it make sense to use a global flag? so a PAM authorization > failure blocks all other methods... I don't think that's necessary. do_pam_account() caches its result and it gets called in auth1.c or auth2.c.
PAM acct mgmt can fail for reasons other than password expiry. The patch looks like you assume this is the reason. Also, if the account IS expired, the user should be given a chance to update their password.
(In reply to comment #4) > PAM acct mgmt can fail for reasons other than password expiry. The > patch looks like you assume this is the reason. The patch is about *account* expiry not *password* expiry. Actually, it's about any failures of pam_acct_mgmt that aren't password expiry. do_pam_account() sets force_pwchange and returns success if pam_account_mgmt returns PAM_NEW_AUTHTOK_REQD (but the code already checks for that) or returns a failure for any other non-success code. > Also, if the account IS expired, the user should be given a chance > to update their password. If pam_acct_mgmt failed for any reason other than PAM_NEW_AUTHTOK_REQD then no, they shouldn't.
> do_pam_account() sets force_pwchange and returns success if > pam_account_mgmt returns PAM_NEW_AUTHTOK_REQD (but the code already > checks for that) or returns a failure for any other non-success code. I hadn't looked at do_pam_acct(), I only looked at the patch. So without enough context I mistook the effects of the patch. I did at least say "looks like". Thanks for the additional info, it sounds like the patch DTRT. >> Also, if the account IS expired, the user should be given a chance >> to update their password. > > If pam_acct_mgmt failed for any reason other than PAM_NEW_AUTHTOK_REQD > then no, they shouldn't. That's what I just said. Since the patch doesn't have the effect I thought it did, you can obviously ignore this comment.
(In reply to comment #6) [...] > it sounds like the patch DTRT. Thanks for reviewing it. > >> Also, if the account IS expired, the user should be given a chance > >> to update their password. > > > > If pam_acct_mgmt failed for any reason other than > > PAM_NEW_AUTHTOK_REQD then no, they shouldn't. > > That's what I just said. Since the patch doesn't have the effect I > thought it did, you can obviously ignore this comment. Actually you said "if the *account* is expired. Since you apparently meant "if the password is expired" then we're in agreement and you can ignore my comment too :-)
Patch applied, thanks.
With the release of 4.4, we believe that this bug is now closed. For information about the release please see http://www.openssh.com/txt/release-4.4 .