ENT-14418: Added --ask-pass, --password-file and --switch-user-command options - #199
ENT-14418: Added --ask-pass, --password-file and --switch-user-command options#199nickanderson wants to merge 9 commits into
Conversation
Ticket: ENT-14418 Changelog: title Signed-off-by: Nick Anderson <nick@cmdln.org> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ticket: ENT-14418 Changelog: title Signed-off-by: Nick Anderson <nick@cmdln.org> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ticket: ENT-14418 Changelog: none Signed-off-by: Nick Anderson <nick@cmdln.org> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ticket: ENT-14418 Changelog: none Signed-off-by: Nick Anderson <nick@cmdln.org> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| Anything we send this way (a password for switching user) is small enough | ||
| to fit in the pipe buffer, so writing it up front cannot block. Standard | ||
| input is left alone (inherited) when there is nothing to send. |
There was a problem hiding this comment.
Warning Use communicate() rather than .stdin.write, .stdout.read or .stderr.read to avoid deadlocks due to any of the other OS pipe buffers filling up and blocking the child process.
Is this what you are referring to?
There was a problem hiding this comment.
Yes, that's the warning. The reasoning Claude wrote was that our payload (a password) is far smaller than the pipe buffer, so the write can't block — but that argument only holds as long as nobody sends anything bigger, so I've stopped making it. _popen() now only opens the pipe and communicate(input=...) does the writing, in cd4fa30.
The one wrinkle is that _Task.communicate() polls with a short timeout, and communicate() raises ValueError: Cannot send input after starting communication if a later call hands it the same input again. It remembers what the first call gave it and keeps writing from where it left off, so only the first call passes it (and a retried process gets a fresh one, so the flag resets). tests/test_aramid.py::test_input_is_only_handed_over_once covers that — it fails with that ValueError if the flag goes away.
| _switch_user_command = None | ||
| _switch_user_password = None |
There was a problem hiding this comment.
I don't love these "mutable" globals. Can you achieve the same with function parameters?
There was a problem hiding this comment.
Done — they're gone. There's a SwitchUser object now, built once in main.py from the options and passed down as a switch_user= parameter to the connections it applies to. All the consumers (ssh_sudo, the hint, the "does this host want a password" check) already had a connection in hand, so they read connection.switch_user instead of module state.
The plumbing follows what users already does: auto_connect reads switch_user to make the connection with, and a connection that's handed in already carries its own.
| assert_output -i "rejected" | ||
|
|
||
| echo "=== NOPASSWD host: the password must not reach the command ===" | ||
| run_cfr "$password" cf-remote --ask-pass sudo -H cfnopass@"$host":"$port" 'cat' |
There was a problem hiding this comment.
Maybe you should run this with debug to make sure it's not leaked in logs either?
There was a problem hiding this comment.
Good idea, done. That case runs with --log-level debug now, and there's a second debug case on the host where the password is sent, since that's where a leak would come from. Both grep the whole output for it, and assert [DEBUG] is present so the check can't silently pass on a non-debug run.
…bals Ticket: ENT-14418 Changelog: none Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nick Anderson <nick@cmdln.org>
Ticket: ENT-14418 Changelog: none Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nick Anderson <nick@cmdln.org>
Ticket: ENT-14418 Changelog: none Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nick Anderson <nick@cmdln.org>
| needs_password = ( | ||
| "a terminal is required" in output | ||
| or "a password is required" in output | ||
| or "no tty present" in output | ||
| ) |
There was a problem hiding this comment.
This doesn't feel very robust to me. What if instead cf-remote would by default run sudo non-interactively sudo -n ? This would collapse all these errors into a single one: sudo: a password is required. Then you can accurately print this hint.
However, I don't know how the rest of the code would react to this change
| def needs_password_on(self, connection): | ||
| """Check whether switching user on this host requires a password | ||
|
|
||
| Only interesting when we actually have a password to send. Sending it | ||
| when it isn't needed would leave it on the standard input of the | ||
| command we are running instead. | ||
|
|
||
| 'sudo -n' answers this without ever attempting to authenticate. Asking | ||
| by letting an attempt fail instead would count towards the failed | ||
| attempts that pam_faillock locks accounts out over, once per host and | ||
| run. | ||
| """ |
There was a problem hiding this comment.
Maybe this wouldn't be needed if by default we run sudo -n ? (see other comment)
There was a problem hiding this comment.
Taken the other two: the default is now sudo -n bash -c, and SwitchUser is built inside run_command_with_args instead of being passed in.
-n and -S can't be combined. -n means never prompt, so sudo -n -S refuses the password. When there is a password the command is still sudo -S -p '' bash -c.
needs_password_on() decides whether the password is written to the child's stdin at all. On a NOPASSWD host, sending it puts it on the stdin of the wrapped command instead of into sudo. (tests/shell/002_sudo_password.sh).
| validate_args(args) | ||
|
|
||
| exit_code = run_command_with_args(args.command, args) | ||
| exit_code = run_command_with_args( |
There was a problem hiding this comment.
Why pass it as an argument here, and not just have it create the SwitchUser inside run_command_with_args ?
Ticket: ENT-14418 Changelog: none
Ticket: ENT-14418 Changelog: none
3 participants
cf-remote can now switch user on hosts where sudo asks for a password, prompting for it once with
--ask-passor reading it from a file with--password-file, so passwordless sudo is no longer a requirement.--switch-user-commandreplaces the hardcodedsudo bash -c.