-
Notifications
You must be signed in to change notification settings - Fork 116
Implement passwd_timeout
#1165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement passwd_timeout
#1165
Conversation
51ee4d5
to
bc09fc0
Compare
Yeah, I think poll is a better option than using signal handlers too. |
bc09fc0
to
b6d2bda
Compare
Thanks for the review! Force pushed a rebase and dropped a tag on my first revision. (Guessing the CI fail is just a flaky test; it's og-sudo and passes locally) |
b6d2bda
to
4c5af19
Compare
When the conversation function panics, pam_authenticate returns ConversationError.
Regardless of what error code is returned from the conversation function, pam_authenticate always returns ConversationError. This propogates timeout error information back to the password retry logic so that sudo doesn't prompt the user again after a timeout.
4f2208c
to
e5c7516
Compare
Thanks so much for this contribution. I want to add that this is really a very nice bit of work; you've not only added this feature but also added integration tests, worked with the available data structures, and very clearly organized your changes in nice commits and helped a smooth review process by dropping the tag. We really appreciate this! |
Fixes #1033
This uses
poll(2)
to implement timeouts; this is simpler (and likely easier to use correctly) thanselect(2)
orepoll(7)
. It's worth noting that this differs from og-sudo, which usesalarm(2)
(seegetpass.c
). The primary reason for this is that I wrote the code before I knew what og-sudo used (and learned aboutalarm(2)
as a side-effect). That said, I likepoll(2)
because it doesn't affect the entire sudo process (by sending a signal), just the fd that we're interested in. Happy to discuss alternatives.There is some (quite frankly) nasty stuff around passing the timeout error back through PAM; see the commit message for details. Would welcome alternative approaches here.
Also worth noting that I've structured the commits such that they can be reviewed one at a time, but there are a few that won't build. Happy to modify/squash as requested.