Add a bounded, killable run with stdin to Executor (#129)

[sehkone]

Summary

Adds Executor::run_with_input, which runs a command as an Identity with a request on stdin, a byte limit on each of stdout and stderr, and a timeout that kills the command. roxyd needs it to drive REview's core-update recovery subcommands without adding a separate private way to run privileged commands. LocalExecutor, SshExecutor and InDaemonExecutor all implement it, and so does the test-support recording executor.

Final names

• Method: Executor::run_with_input(&self, identity: Identity, command: &str, args: &[&str], input: &[u8], limits: RunLimits) -> Result<CommandOutput, RunWithInputError>

• Limits: RunLimits { max_stdout: usize, max_stderr: usize, timeout: Duration }. It has no Default, because the bounds are the caller's decision.

• Error: RunWithInputError, with these variants:

  • InvalidCommand { command }
  • OutputLimit { command, stream: OutputStream, limit }
  • TimedOut { command, timeout }
  • Unsupported
  • Executor(ExecutorError), which carries spawn, transport and elevation failures exactly as run reports them.

  It is a separate type rather than new ExecutorError variants. That way, existing exhaustive matches on ExecutorError keep compiling, and callers of other methods never see variants that only this method can produce.

• Streams: OutputStream::{Stdout, Stderr}

• Test support: executor::test_support::RecordingExecutor, with RecordedCall::RunWithInput and ScriptedRun::{Output, OutputLimit, TimedOut, Error}.

The trait gets a default body that returns Unsupported, so existing outside implementations of Executor still compile. Every executor in this crate overrides it.

Semantics

• Command path: command must be an absolute path with no = in it. Anything else is refused with InvalidCommand before anything is spawned or the identity is looked at. = is refused because the command is started through env -i, which would read it as a variable assignment.
• Input: input is written to the command's stdin, which is then closed. Empty input closes it at once, and the bytes pass through every transport unchanged. On an elevating transport with SudoAuth::Password, the password line goes ahead of the input and sudo consumes it.
• Environment: the command runs with an empty environment, so there is no PATH lookup.
  • A command spawned directly has its environment cleared with env_clear.
  • Under sudo or SSH, a supervising /bin/sh script starts the command through /usr/bin/env -i. Whatever environment sudo or the SSH session set up therefore never reaches it.
• Output limits: stdout and stderr are each read up to their limit, and one byte over is an OutputLimit error. The limits apply only to the command's own bytes:
  • The supervisor prints a start sentinel on every transport it runs on. Stderr written before the sentinel comes from the transport (a sudo refusal, an ssh connection diagnostic). It is not counted against max_stderr and is not returned. That way, a refusal or a failed connection is still reported as the Elevation, SudoRefused or Connection error that run returns, however small the limit. That stderr has a fixed 64 KiB cap of its own instead. Past the cap, the run is abandoned and classified as a transport failure.
  • The sentinel and this run's own timeout marker are not counted either. The SSH exit-status line is a fixed string a command could print itself, and either marker can arrive split across reads, so trailing bytes that are or may still become one of them are held as undecided instead. If counting them would pass max_stderr, they get a one-second grace (FRAMING_GRACE) to turn out to be framing — real framing is written whole and the stream closes right behind it. When the grace runs out with the command still running, they count and the run ends as OutputLimit. A command that writes one byte that could open a marker and then hangs is therefore killed about a second after passing its limit, not left running until the timeout.
• Timeout: passing timeout is a TimedOut error. Neither a breach nor a timeout ever returns a CommandOutput. When the supervisor's own deadline ends a run, it prints a marker that is unique to that run: a fixed prefix plus a 128-bit nonce from aws_lc_rs::rand::SystemRandom. A command that prints a marker-shaped string and exits is therefore still reported as having exited.
• Exit status: a non-zero exit is still a CommandOutput, as with run.
• Identity: identities resolve through the same code run uses, in resolve_through on each executor. Only the script that sudo runs changes, so sudo's flags and descent are identical. InDaemonExecutor runs root without a prefix, descends to a service account with sudo -u without ever prompting, and refuses Identity::Operator.

Kill and no-survivor behaviour

The engine is in src/executor/bounded.rs. A single thread handles all three pipes through poll(2) with non-blocking I/O. It doesn't use one thread per pipe because a thread blocked in read can't be cancelled when a descendant keeps a pipe open. The child is started in its own process group.

• Spawned directly (local operator, in-daemon root): the process group is sent SIGKILL and the child is reaped.
• Under sudo (local root and service, in-daemon service): sudo is sent SIGTERM and relays it to the supervisor. The supervisor then sends SIGKILL to the command's whole process group from the inside, even if the command ignores SIGTERM. After a grace period, sudo's own group is killed as a backstop. The supervisor also enforces its own deadline, a sleep of timeout rounded up to a whole second.
• Over SSH: the local ssh process group is killed at the deadline. No signal reaches the remote side over the connection, so the remote supervisor ends the command at its own deadline, and this call doesn't wait for that. This is the documented limitation. On a breach, the remote command is ended the same way: once ssh is gone its pipes close, and the supervisor's deadline ends it if writing to them doesn't.
• On every transport: a descendant that leaves the process group (for example, a daemon that calls setsid) can't be reached. A supervised command killed by a signal reports exit code 128 + signal rather than none, and starts with SIGINT and SIGQUIT ignored. All of this is written in the method's rustdoc.

Dependencies

No new crate is added. The existing rustix dependency gets its event feature enabled, for poll. The timeout marker's nonce comes from aws-lc-rs, which the crate already uses. The standard library has no way to wait for a pipe to become ready and no read timeout on a pipe, and both are needed to enforce the limits and deadline from one thread. The reason is also recorded in a comment in Cargo.toml.

Unchanged

run, run_root_checked, Identity, ServiceAccount and Principal are unchanged, and ServiceAccount gets no runtime constructor. CHANGELOG.md gets no entry because deploy-core has not been released. AGENTS.md and README.md now list the recording executor among the test-support fixtures.

Tests

The tests run every identity/transport pair against stub sudo and ssh programs, with the supervisor really running on each path. They check:

• stdin arrives exactly for empty, 65,536-byte and small JSON inputs, with and without a password line ahead of it;
• the command sees an empty environment;
• relative commands and commands containing = are refused before spawning;
• a non-zero exit comes back as a CommandOutput, including when the command leaves its input unread;
• each stream passes at exactly its limit and fails one byte over;
• a breach, or a timeout, kills a command that ignores SIGTERM along with its SIGTERM-ignoring child, and both processes are confirmed gone;
• the supervisor's own deadline and a relayed SIGTERM each kill the command without any outside help;
• sudo's argument prefix matches run's on every transport;
• a command that prints a marker-shaped string and exits normally is reported as having exited, and those bytes count against its stderr limit;
• a command that writes one byte past a zero stderr limit that could open a marker (_ or a newline) and then hangs is killed as OutputLimit before the timeout, and no process survives;
• elevation and transport failures come back as run reports them, even with a stderr limit of zero;
• a transport that floods stderr before the command starts is abandoned before the timeout and still classified as a refusal.

Closes #129

Deviations from the issue

• Error type. The issue's example signature returns Result<CommandOutput, ExecutorError>, and the new cases were expected as ExecutorError variants. The method instead returns Result<CommandOutput, RunWithInputError>. That is a new thiserror enum with InvalidCommand, OutputLimit, TimedOut and Unsupported, and it wraps every existing failure in Executor(ExecutorError). New variants on ExecutorError would break exhaustive matches in bootler and roxyd, which the "no existing public signature changes" criterion rules out. Those variants would also sit on every other method's error even though only this method can produce them. Callers can still match each case separately.
• Command check. The issue asks only that command be an absolute path. The implementation also refuses an absolute path containing =, with the same InvalidCommand error and before anything is spawned. On every transport that goes through sudo or SSH, the command starts as /usr/bin/env -i <command> …, and env treats an argument containing = as a variable assignment. That would silently run something else or fail oddly, so the check refuses it up front on every transport.
• Default trait body. The issue asks for one new Executor method with no mention of a default. The method has a default body that returns RunWithInputError::Unsupported. A required method would break every Executor implementation outside this crate, which the "no existing public signature changes" constraint rules out. Every executor this crate ships overrides it, including the test-support RecordingExecutor.
• Exit code and signals under the supervisor. The issue says a non-zero exit is a CommandOutput "as with run". Where a supervising shell runs the command (under sudo, and on every SSH identity), a command killed by a signal reports 128 + signal as its exit code instead of None. It also starts with SIGINT and SIGQUIT ignored. The shell has to run the command in the background so its SIGTERM trap can fire while it waits, and a background job in a shell without job control inherits those two signals as ignored. The shell's exit status is all that crosses sudo or SSH. Both differences are documented on the method.

Test plan

• cargo fmt -- --check --config group_imports=StdExternalCrate
• cargo clippy --all-targets -- -D warnings
• cargo clippy --all-targets --features test-support -- -D warnings
• cargo test
• cargo test --features test-support
• RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items --features test-support
• CI passes on every job, including ubuntu-24.04-arm and macos-latest
• stdin_arrives_exactly_on_every_pair: an empty input, a 65,536-byte input covering every byte value, and a small JSON request each come back unchanged on all eight identity/transport pairs
• a_password_line_is_consumed_before_the_input_on_the_elevating_transports: with SudoAuth::Password, the local and SSH transports feed the password line to sudo and the 65,536-byte input still reaches the command intact
• the_command_sees_an_empty_environment_on_every_pair: /usr/bin/env prints nothing on every pair
• a_command_that_is_not_an_absolute_path_is_refused_before_spawning: touch, ./touch and /usr/bin/touch=x are refused with InvalidCommand and nothing runs; InDaemonExecutor refuses a relative command before it rejects Identity::Operator
• a_nonzero_exit_is_a_command_output_on_every_pair: exit 7 comes back as a CommandOutput holding only the command's own stdout and stderr
• a_command_that_leaves_its_input_unread_still_reports_its_exit_on_every_pair: a command that exits without reading 256 KiB of input still reports its exit code and output
• each_stream_may_reach_its_limit_but_not_pass_it_on_every_pair: stdout and stderr pass at exactly their limit, and one byte over is OutputLimit naming the right stream and limit
• a_breach_kills_a_command_that_ignores_sigterm_on_every_pair: a stdout flood from a command that ignores SIGTERM ends as OutputLimit before the timeout, and both the command and its SIGTERM-ignoring child are gone
• a_timeout_kills_a_command_that_ignores_sigterm_on_every_pair: the same command ends as TimedOut with the configured timeout, and no process survives
• a_command_that_closes_its_streams_is_still_held_to_the_timeout: a command that closes all three pipes is still killed at the deadline
• an_unbounded_timeout_runs_the_command_to_completion_on_every_pair: Duration::MAX finishes normally, with no immediate timeout from an oversized sleep
• the_supervisor_kills_the_command_at_its_own_deadline: with no outside signal and no usable PATH, the supervisor script prints its timeout marker and kills the whole group
• a_relayed_sigterm_makes_the_supervisor_kill_the_command: a SIGTERM to the supervisor alone kills a command that ignores it
• identities_resolve_through_sudo_exactly_as_run_resolves_them: sudo's arguments ahead of the shell match run's for local root and service, SSH root and service, and the in-daemon service descent; local operator, SSH operator and in-daemon root run with no sudo
• a_command_that_prints_a_timeout_marker_still_exits_on_every_pair: a command prints the old fixed marker and a well-formed marker with another run's nonce, then exits. It comes back as a CommandOutput at exactly its stderr limit, and as OutputLimit one byte under
• a_byte_over_the_limit_that_could_open_a_marker_kills_a_running_command_on_every_pair: with max_stderr 0, a command that ignores SIGTERM writes _ or a newline to stderr and then hangs. Every pair ends it as OutputLimit on stderr before the timeout, and the command and its child are gone
• elevation_and_transport_failures_are_reported_as_run_reports_them: roomy limits and zero limits both give the same ExecutorError variants that run returns, for each of these: a password-wanting sudo (local and over SSH), a rejected password, a sudoers denial on the in-daemon descent, an unreachable SSH host (operator and root), and Identity::Operator inside the daemon
• a_transport_that_floods_stderr_before_the_command_starts_is_refused: a sudo writes 1 MiB of stderr without granting and then hangs. The run is abandoned before its timeout and reported as SudoRefused
• an_executor_that_does_not_implement_it_refuses_as_unsupported: the trait's default body returns Unsupported without calling run
• Unit tests in executor::bounded:
  • stderr before the start sentinel is attributed to the transport;
  • stderr accounting ignores the sentinel and this run's own timeout marker, and holds a trailing exit-status line or a fragment of either marker as pending;
  • a pending fragment past the limit counts once its grace runs out, both while the command's streams are open and after it has closed them;
  • a pending fragment that completes into the exit-status line before the command exits is not counted;
  • another run's marker counts, and settles as an exit rather than a timeout;
  • transport stderr ahead of the sentinel is not returned;
  • each supervisor draws a distinct marker;
  • the supervisor deadline rounds up to a whole second and is capped.
• Unit tests in executor::test_support: RecordingExecutor records the identity, command, arguments, stdin bytes and limits of each call, returns each scripted outcome (output, limit breach, timeout, error), and refuses a relative command as the real executors do
• Existing executor tests pass unchanged

[sehkone]

[Reviewer Round 1]

Verdict: Request changes. The overall approach covers the three executors and has substantial failure-path tests, but two error cases remain incorrect.

• src/executor/bounded.rs:632 treats __BOOTLER_TIMEOUT__ anywhere in stderr as proof of a timeout. A supervised command can print those bytes and exit normally; run_with_input then returns TimedOut instead of its CommandOutput. The same bytes are discounted from the stderr limit at line 123. The timeout signal needs framing that cannot be confused with command output, plus a regression test.

• src/executor/bounded.rs:381 applies max_stderr before the transport failure is classified. With a small limit, a sudo refusal or SSH connection diagnostic returns OutputLimit instead of the matchable Elevation, SudoRefused, or Connection error promised by the method documentation. The failure tests use roomy limits, so they miss this case.

The declared deviations are reasonable and accurately described: the separate error type preserves existing exhaustive matches, the = restriction protects the env -i invocation, and the default trait body preserves outside implementations. The supervised signal behavior and SSH limitation are disclosed. The PR body has the required Closes #129, deviations section, and test plan.

[sehkone]

[Review Verdict Round 1: NOT_APPROVED]

[sehkone]

[Author Round 1]

I fixed both review items in ef7be89 and pushed it to PR #130. I updated the PR description to match. All the test-plan commands pass locally (fmt, both clippy runs, both test runs, cargo doc); CI has not run on the new commit yet.

1. The fixed timeout marker could be mistaken for the command's output — Fixed.

• Cause: the supervisor announced its own deadline with the fixed string __BOOTLER_TIMEOUT__. A command that printed that string and exited normally was reported as TimedOut, and those bytes were not counted against its stderr limit.
• Fix: each run's supervisor now prints its own marker: a fixed prefix plus a 128-bit random value from aws_lc_rs::rand::SystemRandom. aws-lc-rs is already a dependency. Only that exact marker is treated as a timeout or left out of the stderr count. The code is Supervisor::new in src/executor/bounded.rs, used by all three executors.
• Why this is enough: a command can only learn the marker by deliberately reading its supervisor's arguments. Forging it gains nothing, since the command could get itself reported as timed out just by not exiting.
• Regression tests:
  • A new test on every identity/transport pair runs a command that prints the old marker and a well-formed marker from another run, then exits. It comes back as a CommandOutput at exactly its stderr limit, and as OutputLimit one byte under.
  • Engine unit tests check that only the run's own marker is treated as a timeout.

2. max_stderr was applied before a transport failure was classified — Fixed.

• Cause: with a small limit, a sudo refusal or an ssh connection error was reported as OutputLimit instead of Elevation, SudoRefused or Connection.
• Fix: every supervisor now prints the start sentinel, including the one for the SSH operator. Stderr written before the sentinel comes from sudo or ssh:
  • It no longer counts against max_stderr. A separate fixed 64 KiB cap keeps memory bounded instead.
  • Past that cap the run is stopped. Its output is kept, so the failure is still classified the way run classifies it.
  • On a successful run, that stderr is left out of the returned CommandOutput, so the final limit check matches the running one.
• Regression tests:
  • The existing failure test now runs every case under both roomy and zero limits. It also gained a wrong-password case, a sudoers denial on the in-daemon path, an SSH root connection failure and a refusal by the remote sudo.
  • A new test uses a sudo stub that writes 1 MiB of stderr and hangs. It is stopped before the timeout and reported as SudoRefused.

Other changes:

• The run_with_input docs now say that stderr written before the command starts is neither counted nor returned. The engine's docs are updated to match.
• One clippy too_many_lines allow, with a comment, on the enlarged failure test. The project instructions allow that lint loosely.
• The PR's "Deviations from the issue" section is unchanged, because this push adds or withdraws none. There is no CHANGELOG entry, since deploy-core has not been released.

[sehkone]

[Reviewer Round 2]

[Reviewer Round 2] — Request changes.

The two Round 1 items are resolved. The per-run nonce prevents ordinary command output from being mistaken for the timeout marker, and stderr written before the start sentinel is classified as transport output. The updated tests cover both fixes. The declared deviations remain reasonable, and the PR body has the required Closes #129, deviations section, and test plan.

One new limit-handling issue remains:

• src/executor/bounded.rs:145: Framing::attribute excludes a trailing fragment that might become a supervisor marker. With max_stderr = 0, a supervised command can write just _ to stderr and then hang. That byte is treated as a possible timeout-marker prefix, so the executor leaves the command running and eventually returns TimedOut instead of killing it with OutputLimit. On SSH, a trailing newline can likewise be deferred as a possible exit-status prefix. The final limit check catches these bytes only if the command exits. The required “one byte over” behavior needs to hold while it remains running, and needs a regression test for that case.

[sehkone]

[Review Verdict Round 2: NOT_APPROVED]

[sehkone]

[Author Round 2]

Round 2 had one review item and it is fixed. The fix is in b47a0fe, pushed to PR #130. CI passes on every job, including ubuntu-24.04-arm and macos-latest, and every test-plan command passes locally.

1. Stderr bytes that might become a marker were never counted — Fixed.

• Cause: on a supervised run, trailing stderr bytes that could still grow into the timeout marker or the SSH exit-status line were left out of the count. They stayed out for as long as the command kept running. A command that wrote _ or a newline past max_stderr and then hung was left running until its timeout and reported as TimedOut instead of OutputLimit.
• Fix, in src/executor/bounded.rs:
  • Stderr is now split into bytes that are certainly the command's and trailing bytes still pending as possible framing.
  • Only this run's complete timeout marker is discounted outright; its random nonce makes it unforgeable. The exit-status line is a fixed string a command could print, so even a complete one stays pending.
  • If counting the pending bytes would pass max_stderr, they get a one-second grace (FRAMING_GRACE) to turn out to be framing. When it runs out with the command still running, they count and the run ends as OutputLimit.
  • Real framing is written in one go and the stream closes right behind it, so the grace only delays killing a command that faked a fragment.
  • The grace is enforced in two places: while the command's output streams are open, and after it has closed them and is only waited on.
• Trade-off: a command that stops on such a byte is killed up to a second after it passes its limit, rather than at that exact byte. This is written in the run_with_input rustdoc.

Regression tests:

• The new a_byte_over_the_limit_that_could_open_a_marker_kills_a_running_command_on_every_pair sets max_stderr to 0 on all eight identity/transport pairs. The command writes _ or a newline, ignores SIGTERM and hangs. Every pair ends it as a stderr OutputLimit before the timeout, and the command and its child are confirmed gone.
• I checked that this test catches the bug: with the grace set to 600 s (roughly the old behaviour) it fails with local root underscore: got TimedOut.
• New unit tests in the engine cover both places the grace is enforced. Another checks that an exit-status line arriving in two writes is still not counted as the command's.
• The existing tests that expected a split marker to be uncounted now expect it to be held as pending.

Other changes:

• Docs: the run_with_input rustdoc, the attribute, pump and await_exit docs, and the new constant's doc all describe the grace.
• PR description:
  • The output-limits paragraph now explains the grace.
  • The test list and test plan name the new tests.
  • I unchecked the CI box while the new run was going and re-checked it once it passed.
• Unchanged: no deviations were added or removed, and there is no CHANGELOG entry because deploy-core has not been released.

[sehkone]

[Reviewer Round 3]

[Reviewer Round 3] — Approve.

The Round 2 item is resolved. Pending stderr that could be framing now gets a one-second deadline while pipes are open and after they close; if it remains over the limit, the run returns OutputLimit and kills the child (bounded.rs, bounded.rs). The new regression test exercises _ and newline on every supported identity and transport pair and checks that the processes are gone (executor.rs).

I found no further issues in the updated diff. The declared deviations remain accurate and justified, and the PR body carries Closes #129 and the required test plan.

[sehkone]

[Review Verdict Round 3: APPROVED]

[sehkone]

Suggested squash commit

Title

Add a bounded, killable run with stdin to Executor

Body

roxyd has to drive REview's core-update recovery through REview's own
subcommands: one JSON request on stdin, one JSON reply on stdout, under
a timeout, as root or as the REview service account. It must not grow a
second, private way of running privileged commands beside Executor, so
the capability lives here.

run_with_input feeds the input, reads stdout and stderr each up to a
limit, and kills the command at a deadline, on every transport. The
bounds are enforced with poll(2) over non-blocking pipes rather than a
reader thread per pipe, since a thread blocked in read cannot be
cancelled once a surviving descendant holds a pipe open.

A process this one may not signal - a command sudo started as root, or
one on the far side of an SSH connection - is killed from the inside by
a supervising shell, on the SIGTERM sudo relays and on a deadline of its
own. Its cancellation is a SIGKILL to a bare sleep rather than a SIGTERM
to a subshell, because dash drops a signal that lands before the
subshell installs its trap. The shell, env and sleep are named by
absolute path so the supervisor depends on no PATH, and the sleep is
capped at what macOS accepts so a very long timeout keeps its deadline.

The supervisor's timeout marker carries a random nonce drawn for each
run, so a command that prints a marker-shaped string and exits is still
reported as having exited. Stderr written before the supervisor's start
sentinel belongs to the transport: it is held to a fixed cap instead of
max_stderr and left out of the output, so a sudo refusal or an SSH
connection failure is classified exactly as run classifies it, however
small the limit. Trailing stderr that may still grow into framing gets
a one-second grace to do so; after that it counts, so a command cannot
dodge its limit by writing a marker's first byte and hanging.

The errors get a type of their own so ExecutorError, which bootler
matches exhaustively, is unchanged, and the trait method has a refusing
default so the executors dependents implement in their tests still
compile.

Closes #129