Skip to content

ENT-14418: Added --ask-pass, --password-file and --switch-user-command options - #199

Open
nickanderson wants to merge 7 commits into
cfengine:masterfrom
nickanderson:ENT-14418
Open

ENT-14418: Added --ask-pass, --password-file and --switch-user-command options#199
nickanderson wants to merge 7 commits into
cfengine:masterfrom
nickanderson:ENT-14418

Conversation

@nickanderson

Copy link
Copy Markdown
Member

cf-remote can now switch user on hosts where sudo asks for a password, prompting for it once with --ask-pass or reading it from a file with --password-file, so passwordless sudo is no longer a requirement. --switch-user-command replaces the hardcoded sudo bash -c.

🤖 Generated with Claude Code

nickanderson and others added 4 commits August 14, 2026 11:03
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>
@nickanderson
nickanderson requested review from SimonThalvorsen, larsewi and victormlg and removed request for victormlg August 14, 2026 16:24

@larsewi larsewi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Comment thread cf_remote/aramid.py Outdated
Comment on lines +120 to +122
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@nickanderson nickanderson Aug 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cf_remote/ssh.py Outdated
Comment on lines +50 to +51
_switch_user_command = None
_switch_user_password = None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love these "mutable" globals. Can you achieve the same with function parameters?

@nickanderson nickanderson Aug 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread tests/shell/002_sudo_password.sh Outdated
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'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should run this with debug to make sure it's not leaked in logs either?

@nickanderson nickanderson Aug 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nickanderson and others added 3 commits August 18, 2026 12:30
…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>
@nickanderson
nickanderson requested a review from larsewi August 18, 2026 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants