You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(sudo): fall back to script(1) when sudoers sets requiretty - #514
Falls back to running sudo inside script(1) when sudoers sets requiretty, keeping the caller's stdin/stdout/stderr outside the terminal. Background and design in #506.
sudo.TTY decorator; RegisterSudo uses it only after sudo's tty rejection and a probe that checks root and intact streams. A warning is logged when it's chosen.
rig_test_regular_user_requiretty runs the integration suite as a non-root user under requiretty. It switches to the original sudo where sudo-rs is the default, because sudo-rs ignores requiretty.
With Defaults requiretty, sudo refuses to run without a terminal, and
rig runs commands without one, so sudo was unusable on such hosts. The
ANSSI-BP-028 hardening profiles set it on current RHEL, Debian and SLES.
requiretty asks for a controlling terminal, not a terminal on stdin.
The new TTY decorator runs sudo inside script(1), which provides one,
and hands sudo the caller's stdin, stdout and stderr, set aside as
descriptors 3 to 5 around script. No data passes through the terminal,
so binary input, end of input and separate stderr work as with plain
sudo. The exit status comes back on descriptor 6, because script exits
0 when it is interrupted.
RegisterSudo picks TTY only when the plain probe fails with sudo's tty
rejection and a TTY probe shows the command runs as root with its
streams intact. Choosing it logs a warning about the extra process and
pseudo-terminal per command. When script is not usable, the requiretty
warning now also suggests installing util-linux.
Refs #506
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
rig_test_regular_user_requiretty provisions rigtest-user with sudo
alone, under Defaults requiretty, with util-linux script installed. It
switches to the original sudo where sudo-rs is the default, as sudo-rs
does not know requiretty. It checks that plain sudo is refused, so the
run cannot quietly test nothing new, and always passes -expect-sudo,
which the plain probe would otherwise leave off.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The exit-status relay lacks execution-backed tests for nonzero exits, interruption, and SSH cancellation, leaving exit-code propagation and elevated-process termination unverified.
The golden and registry tests only checked the command string and a
mocked wrapper, so the exit status relay was not exercised. Run the
wrapper with util-linux script and a stand-in sudo that runs its command
unprivileged: exit status 17 and separate stderr, 1 MiB of binary stdin,
and a script interrupted before the command reports a status, which must
exit 130 rather than script's 0. Skipped where util-linux script is not
available.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Run an elevated command that exits 17, and one the context cuts short,
through whatever sudo runner rig detected, which under requiretty is
sudo.TTY. The cancelled command must report the context error, not
success. Whether the elevated process is gone afterwards is not checked:
over native SSH a command without a terminal outlives the cancelled
session with plain sudo too.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The separate terminal session and cross-shell status relay need real requiretty SSH/OpenSSH cancellation and disconnect validation to assess descendant-process cleanup beyond exit-status assertions.
The extra process and pseudo-terminal per command matter less than a
cancelled or disconnected sudo command staying alive on the host, so
lead with that.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The runner-selection test can pass even when subsequent commands use plain Sudo instead of TTY, so it needs an assertion that the actual whoami command equals sudo.TTY("whoami").
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Test does not verify whoami uses the TTY-wrapped sudo runner
sudo/registry_test.go:94
These assertions can pass even if the selected runner uses plain Sudo instead of TTY: the discovery probe already satisfies the script -qec check, and the unstubbed whoami invocation succeeds because MockRunner.ErrDefault is nil. Assert that the actual whoami command equals sudo.TTY("whoami") so the test catches a fallback that probes successfully but does not wrap subsequent commands.
The probe already sends a script command, so checking for one passed
even if later commands went through plain Sudo. Match the exact
sudo.TTY form of the command instead.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed to evaluate an unpublished Alpine/BusyBox compatibility finding that causes the interruption test to return the wrong exit status.
BusyBox ps has no -o ppid= or -p, so on Alpine with util-linux script
the interruption never happened and the test saw exit 0 instead of 130.
The test only runs on Linux, so read the parent from /proc instead.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The privileged PTY wrapper changes signal and process-lifetime semantics, requiring runtime validation of cancellation and disconnect behavior beyond the discovery probe’s successful-execution and stream-forwarding checks.
A cancelled command over native SSH gets SIGINT, which reached the
wrapper shell but not sudo: script(1) runs sudo in a session of its own,
and sudo does not pass on the hangup it gets when script exits. The
command kept running after its context ended, where plain Sudo stops.
The wrapper and the shell script starts now run their child in the
background and pass SIGINT, SIGTERM and SIGHUP on as SIGTERM, so the
signal reaches sudo, which passes it to the command. The exit status
still comes back on descriptor 6, now through a temporary file, so the
command's own status is reported.
Running script in the background makes the elevated command start with
SIGINT and SIGQUIT ignored, as POSIX shells start background commands.
Without job control, which needs a terminal the wrapper does not have,
a shell cannot forward a signal to a child it waits for in the
foreground, and execing into script would report 0 for an interrupted
command.
The integration test now checks that the elevated process is gone after
a cancel over native SSH. The openssh protocol only stops the local
client, so there the remote command outlives the context with or
without sudo.TTY.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The OpenSSH 9 servers of Debian 12 and Ubuntu 24.04 ignore the signal
request rig sends when a context ends, for a root login, so even an
unelevated command outlives its context there and the check failed on
CI. Require the elevated command to stop only where an unelevated one
does, and kill whichever outlives its context.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Cancellation and disconnect handling across shell sessions and privileged sudo processes need validation over both SSH transports, since unit tests fake sudo and SSH integration may skip process-termination assertions.
…over openssh
doas gives up the user's real uid before running the command, so the
signal the server sends as the user when a context ends is not allowed
to reach it, and the Alpine regular user runs, which escalate with doas,
failed the check. Require the elevated command to stop only when
privileges are escalated through sudo, and log why when the check does
not apply.
The requiretty run now also goes through the openssh client protocol,
so sudo.TTY is exercised over both SSH transports.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The fallback’s multi-layer signal forwarding and privileged-process cleanup require real-sudo/requiretty integration validation across distro and SSH variants, which fake-sudo unit tests cannot establish.
…e TTY
The cancellation check only looked at the command's own process, so a
sudo, script or shell layer left behind by the wrapper went unnoticed.
The command now records its process chain up to the SSH server, and the
check requires all of it to be gone.
The requiretty run passes the new -expect-sudo-tty flag, which requires
script(1) in that chain, so the run fails rather than silently testing
plain sudo, and over native SSH it fails instead of skipping the check
when an unelevated command outlives its context.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The cancellation subtest fails on supported Darwin targets because it unconditionally reads Linux procfs files; the process-chain check needs a procfs availability guard while retaining the portable exit-status check.
The process chain is read from Linux /proc, which other supported
targets such as Darwin lack. Without it, check only that the cancelled
elevated command fails with the context's error and skip the rest,
unless -expect-sudo-tty asks for the check.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
On its own, -expect-sudo-tty let TestSudoExitStatus skip when rig found
no sudo runner, which passed a run that never exercised the fallback.
A host that needs sudo.TTY has sudo, so the flag now also makes the
escalation tests fail instead of skipping, and a Windows target fails.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human review is needed because temporary-file cleanup by an elevated command can break the exit-status relay, causing TTY to return 130 instead of the command’s actual status.
The wrapper read the relayed status back from its temporary file by
path, so an elevated command that cleaned up temporary files removed it
and the wrapper exited 130 instead of the command's own status.
Open the file for writing as descriptor 6 and for reading as descriptor
7, then remove it at once: the status travels through the open
descriptors and there is no path left to remove. Nothing is left behind
when the wrapper is killed either. sudo gets descriptor 6 closed, and
script gets descriptor 7 closed.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Real requiretty integration validation is unavailable, and fake-sudo tests cannot establish that cross-session cancellation leaves no elevated descendants over native SSH and OpenSSH.
No open issues 🎉
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Falls back to running sudo inside
script(1)when sudoers setsrequiretty, keeping the caller's stdin/stdout/stderr outside the terminal. Background and design in #506.sudo.TTYdecorator;RegisterSudouses it only after sudo's tty rejection and a probe that checks root and intact streams. A warning is logged when it's chosen.rig_test_regular_user_requirettyruns the integration suite as a non-root user underrequiretty. It switches to the original sudo where sudo-rs is the default, because sudo-rs ignoresrequiretty.Fixes #506