Add a long-lived elevated channel to Executor (#132)

[sehkone]

Summary

Adds Executor::open_channel, a long-running counterpart to run_with_input. It starts a command as a given identity, locally or over SSH, and returns a Channel: the caller gets the command's stdin and stdout for as long as the command runs, and at the end gets the command's own exit code. It uses the crate's existing SSH and elevation settings, so bootler can start its root helper without building any ssh or sudo command line itself.

• New public items: ChannelLimits, Channel (take_stdin, take_stdout, wait, kill), ChannelExit and ChannelError. The logic lives in a new private src/executor/channel.rs.
• Same resolution as run: LocalExecutor and SshExecutor build the command through the same private resolve_through and ssh_command as run. No flag is added or removed, and no -t is requested. The transport runs in the caller's process group, so a passphrase prompt allowed by SshPrompt::Allow still reaches the terminal.
• Start checked before returning: whenever sudo or SSH is involved, a fixed sh -c script prints SUDO_OK_SENTINEL on stderr, reads stdin up to a handoff line, and then execs /usr/bin/env -i <command> <args…>. open_channel returns only after it has read the sentinel. Any password line is written first, and stdout is never read.
  • The handoff line carries a per-start random nonce and is written only after the sentinel, once sudo is done with stdin. Whatever precedes it is discarded, so a password line that sudo never read (a NOPASSWD rule, or cached credentials) does not reach the command, and the command's first stdin byte is always the caller's first byte.
  • A transport that exits early, or writes more than 64 KiB ahead of the sentinel (even when the sentinel arrives in the same read that passes the limit), is killed and classified the way run classifies it (Connection, Elevation or SudoRefused).
  • A transport still silent when elevation_timeout passes is killed and reaped, and ElevationTimedOut is returned.
  • The local operator is spawned directly with an empty environment.
• Stderr and exit code: a thread drains stderr for the channel's whole life using poll and non-blocking reads, and is always joined. It keeps the first max_stderr bytes of the command's own stderr. Over SSH it removes the trailing exit-status line and takes the exit code from it, so a remote 255 is the command's code. The line is trusted only when ssh itself exits 0, since the wrapper exits 0 once it has printed it. If the line is missing, or ssh failed (so a status-shaped line can only be the command's own stderr), wait returns ExitUnknown, not a guessed code.
• Ending the channel: wait closes stdin if the caller never took it, waits for the transport, then waits at most 5 seconds for stderr to end. kill, and dropping a channel that was neither waited for nor killed, close the pipes the channel still holds, SIGKILL the local transport process and reap it. The rustdoc states the limit: a command behind sudo or SSH is not signalled directly and sees the end only as EOF on stdin and EPIPE on stdout.
• Other executors: the trait's default body returns Unsupported, so external implementors keep compiling. InDaemonExecutor keeps that default.
• test-support: RecordingExecutor records RecordedCall::OpenChannel. It answers from ScriptedChannel::Spawn, which runs a real program directly as the Channel, or from ScriptedChannel::Fail. An invalid command is recorded and refused without using up a script entry.
• Unchanged: no existing public signature, ExecutorError variant or behaviour changes. bounded.rs only widens a few private helpers to pub(super) so the channel module can reuse them. There is no new dependency, no new unsafe, no pre_exec, and no CHANGELOG.md entry.

Closes #132

Deviations from the issue

None

Test plan

• cargo fmt -- --check --config group_imports=StdExternalCrate
• cargo clippy --all-targets -- -D warnings
• cargo clippy --all-targets --features test-support -- -D warnings
• RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items --features test-support
• Full cargo test and cargo test --features test-support
• CI green, including the ubuntu-24.04-arm and macos-latest jobs
• Echo, every pair: /bin/cat echoes a 256 KiB pattern of every byte value exactly on the local and SSH operator, root and service pairs; wait gives Some(0) with empty stderr (bytes_pass_verbatim_both_ways_on_every_pair)
• Stderr drain: a command writes 1 MiB to stderr while echoing stdin; the echo completes, stderr is exactly max_stderr bytes and stderr_truncated is set (stderr_is_drained_and_held_to_its_limit_while_the_echo_runs_on_every_pair)
• Codes: exits 0, 3 and 255 come back as the command's own code on every pair, 255 over SSH included; stderr contains neither the sentinel nor the exit-status line (the_commands_own_code_and_stderr_come_back_on_every_pair)
• Signal: a command that kills itself gives None locally, where the signal ends the local transport, and the remote shell's 137 over SSH (a_local_transport_ended_by_a_signal_has_no_code)
• Untaken stdin: wait closes stdin the caller never took, so /bin/cat sees end of input on every pair (wait_closes_standard_input_that_was_never_taken_on_every_pair)
• Argv parity with run: a recording sudo stub shows the same words ahead of the shell as run for local and SSH root and service, under NonInteractive and Password; the operator runs with no sudo (identities_resolve_exactly_as_run_resolves_them)
• SSH argv: a recording ssh stub shows the same ssh prefix as run, with BatchMode=yes exactly under SshPrompt::Deny and no -t (the_ssh_invocation_is_run_s_with_no_terminal)
• No stdout read before the start: stdout a transport writes before the sentinel still reaches the caller (the_start_consumes_no_standard_output)
• Password: under SudoAuth::Password, /bin/cat echoes the caller's bytes and not the password, locally and over SSH, both with password_sudo (which reads the password) and with a sudo stub that never reads it, as under NOPASSWD (a_password_line_is_consumed_before_the_callers_bytes)
• Refusals: "a password is required" gives Elevation under NonInteractive, "not in the sudoers file" gives SudoRefused, and a failing ssh stub gives Connection (failures_before_the_start_are_reported_as_run_reports_them)
• Timeout: a sudo stub that never prints the sentinel gives ElevationTimedOut at a 200 ms elevation_timeout, and its pid is gone afterwards (a_start_not_proven_in_time_is_killed_and_reaped)
• Rejected password: a rejected -S password times out with sudo's "Sorry, try again" in the diagnostic (a_rejected_password_times_out_with_sudos_complaint)
• Preamble flood: a transport that writes more than 64 KiB before the sentinel is killed and classified (a_transport_that_floods_stderr_before_the_start_is_killed_and_classified)
• Preamble limit boundary: a sentinel after exactly 64 KiB opens the channel and one after 64 KiB + 1 is refused as SudoRefused, end to end (a_start_at_the_transport_limit_opens_and_one_past_it_is_refused); deterministically, a sentinel arriving in the same read that passes the limit is refused with only the transport's bytes kept, a trailing fragment of the sentinel is not yet counted, and the start waits for the password to be written in full (the_transport_limit_holds_wherever_the_sentinel_lands, the_start_waits_for_the_password_to_be_written_in_full)
• Environment: /usr/bin/env prints nothing on every pair (the_command_sees_an_empty_environment_on_every_pair)
• Lost status: an ssh stub that prints the sentinel, echoes, and exits without the exit-status line gives ExitUnknown (an_ssh_channel_that_loses_its_exit_status_has_no_code)
• Forged status: an ssh stub whose command's stderr ends in a well-formed exit-status line for 0, then exits 255, gives ExitUnknown, not Some(0) (a_failed_ssh_is_not_trusted_for_a_status_line_the_command_forged)
• Kill and drop: after kill, and separately after drop, the transport pid is gone, and a command that is not the transport sees end of input and exits (kill_and_drop_end_the_transport_and_the_command_sees_eof_on_every_pair)
• Stderr grace: when a descendant holds stderr open, wait returns within the 5-second grace with stderr_truncated set (wait_gives_stderr_a_grace_when_a_descendant_holds_it)
• Invalid command: cat and /bin/a=b give InvalidCommand with nothing spawned (a_command_that_is_not_an_absolute_path_is_refused_before_spawning)
• Unsupported: the trait's default body and InDaemonExecutor return Unsupported (the_default_body_and_the_daemon_are_unsupported)
• Recording executor: a scripted Spawn of /bin/cat echoes through the Channel and the call is recorded; Fail is returned; an invalid command is recorded and uses up no script entry (a_scripted_spawn_is_a_channel_to_that_program, a_scripted_failure_is_returned_and_an_invalid_command_consumes_nothing)
• Stderr sink: the exit-status line is read only at the very end of the stream, and the sink keeps exactly its limit (the_status_line_is_read_only_at_the_end_of_the_stream, a_sink_keeps_its_limit_and_removes_the_status_line)

[sehkone]

[Reviewer Round 1]

Request changes. The implementation follows the existing resolution paths and covers the main channel behavior, but two requirements remain unmet:

1. The password can become command input. Under SudoAuth::Password, the channel writes the password line to sudo’s stdin. If sudo uses cached credentials or a NOPASSWD rule, it does not read that line, so the command receives it before the caller’s bytes. The new rustdoc explicitly describes this behavior, while issue Add a long-lived elevated channel to Executor #132 requires the command’s first stdin byte to be the caller’s. The password test uses a stub that always consumes the line, and the PR declares no deviation.

2. A late sentinel bypasses the preamble limit. The start loop checks for the sentinel before checking the 64 KiB limit. A transport can write more than 64 KiB of pre-sentinel stderr and then the sentinel; open_channel accepts the start instead of killing and classifying the transport. The flood test covers only output with no sentinel. Check the sentinel’s position against the limit and test that boundary.

The supplied PR body has the required Closes #132, test plan, and deviations section. Its “None” deviation claim needs updating if the password behavior is retained.

[sehkone]

[Review Verdict Round 1: NOT_APPROVED]

[sehkone]

[Author Round 1]

I fixed both Round 1 items and pushed commit 242b254 to PR #135. Locally, fmt, both clippy runs, CI's doc command and both test runs pass; the new CI run is queued and hasn't finished.

1. The password could reach the command as input: Fixed.

• The problem: under SudoAuth::Password, a sudo that never asks for the password (a NOPASSWD rule, or cached credentials) left the password line unread, and the command read it before the caller's bytes. The issue requires the command's first stdin byte to be the caller's.
• The fix, in src/executor/channel.rs: the start script now prints the sentinel, then reads stdin up to a "handoff" line and throws away everything before it (the password sudo didn't read, if any). Then it starts the command.
  • The handoff line contains a 16-byte random nonce, drawn fresh for each start, so no password can match it by accident.
  • The line is written only after the sentinel arrives, when sudo has finished reading stdin. A sudo that rejects the password therefore never reads the handoff line as a second guess, and the rejected-password case still ends in ElevationTimedOut with sudo's "Sorry, try again".
  • If only part of a password line has been written when the sentinel arrives, the start waits until the whole line is written.
  • The words in front of the shell are unchanged, so it still matches run exactly (no flag added, no -t); only the private script text changed.
• Docs: I removed the open_channel rustdoc paragraph that described the leak as expected behaviour and rewrote the start bullet.
• Tests: a_password_line_is_consumed_before_the_callers_bytes now also covers a sudo stub that never reads the password, locally and over SSH. I adjusted the lost-exit-status test's fake ssh so it reads the handoff line.

2. A late sentinel got past the 64 KiB limit: Fixed.

• The problem: the start loop checked for the sentinel before checking the limit. A transport could write more than 64 KiB and then the sentinel in the same read, and the start was accepted.
• The fix: the decision is now a separate small function, judge. Bytes before the sentinel are held to the limit wherever the sentinel lands, and only those bytes go to classification.
• Tests:
  • The end-to-end test can't reliably make the sentinel land in the same read that crosses the limit, so the boundary is unit-tested on judge directly: exactly 64 KiB then the sentinel starts; 64 KiB + 1 then the sentinel is refused, keeping only the transport's bytes; a partial sentinel at the end isn't counted yet; and the start waits for the full password.
  • An end-to-end test also checks that a sentinel after exactly 64 KiB opens the channel and one after 64 KiB + 1 is refused as SudoRefused.

I checked that the new tests fail when each fix is removed, then restored the code.

PR description: I updated how the start works and added the new tests to the test plan. I also unticked "CI green" until the new run passes. Nothing now departs from the issue, so "Deviations from the issue" stays None, and Closes #132 is kept.

[sehkone]

[Reviewer Round 2]

[Reviewer Round 2] — Request changes.

Both Round 1 items are resolved in the updated diff: the handoff prevents an unread sudo password from reaching the command, and the preamble limit is checked at the sentinel’s position.

One new finding remains. In Channel::wait, the SSH branch accepts a parsed stderr marker without checking the local ssh exit status. If the command writes __BOOTLER_RC__:0 to stderr and the connection fails before the wrapper prints its own status, ssh can exit 255 while wait returns Some(0). That would give bootler false proof of successful cleanup. The wrapper exits zero after printing its status, so a failed SSH transport should return ExitUnknown even if its stderr ends in marker-shaped text. Please cover that case with a failing SSH stub.

The PR’s Closes #132 linkage and declared None deviation otherwise match this review.

[sehkone]

[Review Verdict Round 2: NOT_APPROVED]

[sehkone]

[Author Round 2]

I fixed the one item Reviewer Round 2 raised and pushed it as commit 588cad0 to PR #135. The new CI run has started and hasn't finished yet.

Trusting a status line from a failed ssh: Fixed.

• The problem: over SSH, Channel::wait took the exit code from the line at the end of stderr without checking how ssh itself exited. If the command wrote __BOOTLER_RC__:0 to its own stderr and the connection then dropped, ssh exited 255 but wait returned Some(0). bootler would have read that as proof that cleanup succeeded.
• The fix: wait now accepts that line only when the local ssh exited 0 (in src/executor/channel.rs). The remote wrapper prints the line as its last step and then exits 0, so after a failed ssh the line can only have come from the command. That case now returns ChannelError::ExitUnknown, with a reason naming ssh's exit status and the end of stderr.
• Docs: the # Errors section of Channel::wait and the rustdoc of ChannelError::ExitUnknown now say this.
• Test: a_failed_ssh_is_not_trusted_for_a_status_line_the_command_forged uses a fake ssh that prints the sentinel, echoes the caller's bytes, writes a correctly formed line with code 0 to stderr, then exits 255. wait must return ExitUnknown. The test fails without the fix and passes with it.

Locally, fmt, both clippy runs, CI's doc command and both cargo test runs pass. I updated the PR description: the stderr and exit-code bullet now describes the check, and the test plan lists the new test. "Deviations from the issue" stays None, and Closes #132 is kept.

[sehkone]

[Reviewer Round 3]

[Reviewer Round 3] — Approve.

The Round 2 finding is resolved. Channel::wait now accepts the SSH status marker only when the local ssh process exits successfully, and the new test covers a forged zero marker followed by SSH exit 255. I found no new blocking issue in the updated diff. The PR’s Closes #132 linkage and None deviation claim are consistent with the delivery.

One PR body checkbox is stale: “CI green” remains unchecked despite the controller’s report that CI passed.

[sehkone]

[Review Verdict Round 3: APPROVED]

[sehkone]

Suggested squash commit

Title

Add a long-lived elevated channel to Executor

Body

bootler's recovery runs every operation for a host over one framed
session to a root helper. The helper has to be started locally or over
SSH as a long-lived child with piped stdio, under this crate's SSH and
elevation policy, and bootler must not build any ssh or sudo command
line of its own. run and run_with_input return only after the command
exits, and the resolution they share is private, so nothing public
could start such a child.

Executor::open_channel fills that gap. It resolves the identity exactly
as run does, with no flag added or dropped and no -t, and returns a
Channel whose stdin and stdout belong to the caller. Wherever sudo or
SSH stands in between, the call returns only after the started shell
has printed the sentinel. Failures before that point are classified the
way run classifies them, and a start that is not proven in time is
killed and reaped. The command runs with an empty environment.

bootler treats a zero exit received over the channel as proof that
cleanup succeeded, so the exit code has to be the command's own and
never a guess:

- Under SudoAuth::Password, sudo may never read the password line, as
  under a NOPASSWD rule or cached credentials. The line would then
  reach the command ahead of the caller's bytes. After the sentinel, a
  handoff line carrying a fresh random nonce is written, and the shell
  discards everything up to it before exec'ing the command. The
  command's first stdin byte is therefore always the caller's.
- Over SSH the exit code comes from the trailing exit-status line. The
  remote wrapper prints that line and then exits 0, so when ssh itself
  fails, a status-shaped line can only be the command's own stderr.
  wait trusts the line only when ssh exited 0 and otherwise returns
  ExitUnknown.

Stderr is drained for the channel's whole life with poll and
non-blocking reads, so the command never blocks on a full pipe, and the
drain thread is always joined. kill, and dropping an unfinished
channel, SIGKILL and reap the local transport, so no thread or process
started by the channel outlives it. InDaemonExecutor and external
implementors get the defaulted Unsupported body. RecordingExecutor
records and scripts the new call for dependents' tests.

Closes #132