Skip to content

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

Merged

Conversation

MggMuggins
Copy link
Contributor

@MggMuggins MggMuggins commented Jun 19, 2025

Fixes #1033

This uses poll(2) to implement timeouts; this is simpler (and likely easier to use correctly) than select(2) or epoll(7). It's worth noting that this differs from og-sudo, which uses alarm(2) (see getpass.c). The primary reason for this is that I wrote the code before I knew what og-sudo used (and learned about alarm(2) as a side-effect). That said, I like poll(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.

@MggMuggins MggMuggins force-pushed the passwd-timeout-convdata branch 2 times, most recently from 51ee4d5 to bc09fc0 Compare June 19, 2025 20:15
bjorn3
bjorn3 previously requested changes Jun 20, 2025
@bjorn3
Copy link
Collaborator

bjorn3 commented Jun 20, 2025

It's worth noting that this differs from og-sudo, which uses alarm(2) (see getpass.c). The primary reason for this is that I wrote the code before I knew what og-sudo used (and learned about alarm(2) as a side-effect). That said, I like poll(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.

Yeah, I think poll is a better option than using signal handlers too.

@MggMuggins MggMuggins force-pushed the passwd-timeout-convdata branch from bc09fc0 to b6d2bda Compare June 22, 2025 20:54
@MggMuggins
Copy link
Contributor Author

MggMuggins commented Jun 22, 2025

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)

@MggMuggins MggMuggins force-pushed the passwd-timeout-convdata branch from b6d2bda to 4c5af19 Compare June 23, 2025 21:51
@MggMuggins MggMuggins requested review from bjorn3 and squell June 26, 2025 00:51
@squell squell self-assigned this Jun 30, 2025
@squell squell dismissed bjorn3’s stale review June 30, 2025 10:10

Outdated (feedback was incorporated)

MggMuggins and others added 6 commits June 30, 2025 14:35
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.
@squell squell force-pushed the passwd-timeout-convdata branch from 4f2208c to e5c7516 Compare June 30, 2025 12:35
@squell
Copy link
Member

squell commented Jun 30, 2025

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!

@squell squell merged commit 19778b1 into trifectatechfoundation:main Jun 30, 2025
15 checks passed
@MggMuggins MggMuggins deleted the passwd-timeout-convdata branch June 30, 2025 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement passwd_timeout option
3 participants