Fix bug #114 (ESC causes failed login) and related timeout issue #193
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
When the user presses ESC or the prompt times out, the auth UI sends PTYPE_RESPONSE_CANCELLED to the PAM conversation handler. Previously, this returned PAM_CONV_ERR, which caused some PAM modules (particularly with PAM 1.4.0+) to attempt authentication with empty input, resulting in PAM_AUTH_ERR. This was counted by pam_faillock as a failed login attempt, potentially locking users out after 3 ESC presses or timeouts. The fix returns PAM_ABORT instead of PAM_CONV_ERR when receiving PTYPE_RESPONSE_CANCELLED. This cleanly aborts the PAM authentication session without counting as a failed attempt, which is the correct semantic meaning of a user-initiated cancellation. Fixes google#114 Amp-Thread-ID: https://ampcode.com/threads/T-60336806-26ca-40b3-bb77-86a913f74a0c Co-authored-by: Amp <[email protected]>
Change the behavior of the ESC key to clear the password input field (similar to Ctrl-U) instead of immediately canceling the authentication prompt. This provides better UX - users who accidentally start typing their password can press ESC to clear it and try again, rather than having the prompt close. Users can still cancel the prompt by waiting for the timeout, and the previous commit ensures timeout/cancellation won't count as a failed login attempt. Related to google#114 Amp-Thread-ID: https://ampcode.com/threads/T-60336806-26ca-40b3-bb77-86a913f74a0c Co-authored-by: Amp <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes two nasty bugs with regard to pam handling. Users reported it in #114
Authored it with help of ampcode, but manually reviewed results and tested.
This also supersedes PR #181