From 8b619c9581c018771b2fca4d29b6661a321c53af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 21 May 2026 20:19:31 +0200 Subject: [PATCH 1/4] pam/converse: align no-tty conversation fallback with sudo.ws When `/dev/tty` is unavailable, sudo-rs conversation handling could fail in cases where sudo.ws still has a valid fallback path. In particular: - Password prompt: when askpass and a DISPLAY are set - Non hidden conversation without a tty. This made behavior less predictable in headless/no-tty environments and diverged from sudo.ws. Refactor the PAM CLI converser I/O open path to mirror sudo's conversation behavior when `/dev/tty` is unavailable. Add coverage for the new behavior Closes: https://github.com/trifectatechfoundation/sudo-rs/issues/1595 --- src/pam/converse.rs | 182 ++++++++++++++++-- .../sudo-compliance-tests/src/sudo/pam.rs | 44 +++++ .../src/sudo/pass_auth/askpass.rs | 87 +++++++++ 3 files changed, 297 insertions(+), 16 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index d15a04e50..d5ae03d75 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -129,27 +129,89 @@ impl Drop for SignalGuard { } impl CLIConverser { - fn open(&self) -> PamResult<(Terminal<'_>, SignalGuard)> { - let term = if self.use_askpass { - Terminal::open_askpass()? - } else if self.use_stdin { - Terminal::open_stdie()? - } else { - let mut tty = Terminal::open_tty()?; - if self.bell.replace(false) { - tty.bell()?; - } - - tty + fn open(&self, style: PamMessageStyle) -> PamResult<(Terminal<'_>, SignalGuard)> { + let term = match style { + PamMessageStyle::PromptEchoOff if self.use_askpass => Terminal::open_askpass()?, + PamMessageStyle::PromptEchoOff if self.use_stdin => Terminal::open_stdie()?, + PamMessageStyle::PromptEchoOff => match Terminal::open_tty() { + Ok(mut tty) => { + if self.bell.replace(false) { + tty.bell()?; + } + + tty + } + Err(PamError::TtyRequired) => { + match Self::tty_unavailable_fallback(style, AskpassEnv::from_env()) { + TtyUnavailableFallback::Askpass => Terminal::open_askpass()?, + TtyUnavailableFallback::Stdie => Terminal::open_stdie()?, + TtyUnavailableFallback::Error => return Err(PamError::TtyRequired), + } + } + Err(err) => return Err(err), + }, + _ if self.use_stdin => Terminal::open_stdie()?, + _ => match Terminal::open_tty() { + Ok(mut tty) => { + if self.bell.replace(false) { + tty.bell()?; + } + + tty + } + Err(PamError::TtyRequired) => Terminal::open_stdie()?, + Err(err) => return Err(err), + }, }; Ok((term, SignalGuard::unblock_interrupts())) } + + fn tty_unavailable_fallback( + style: PamMessageStyle, + askpass_env: AskpassEnv, + ) -> TtyUnavailableFallback { + match style { + PamMessageStyle::PromptEchoOff if askpass_env.can_use_askpass() => { + TtyUnavailableFallback::Askpass + } + PamMessageStyle::PromptEchoOff => TtyUnavailableFallback::Error, + PamMessageStyle::PromptEchoOn + | PamMessageStyle::ErrorMessage + | PamMessageStyle::TextInfo => TtyUnavailableFallback::Stdie, + } + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum TtyUnavailableFallback { + Askpass, + Stdie, + Error, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +struct AskpassEnv { + has_display: bool, + has_askpass: bool, +} + +impl AskpassEnv { + fn from_env() -> Self { + Self { + has_display: std::env::var_os("DISPLAY").is_some(), + has_askpass: std::env::var_os("SUDO_ASKPASS").is_some(), + } + } + + fn can_use_askpass(self) -> bool { + self.has_display && self.has_askpass + } } impl Converser for CLIConverser { fn handle_normal_prompt(&self, msg: &str) -> PamResult { - let (mut tty, _guard) = self.open()?; + let (mut tty, _guard) = self.open(PamMessageStyle::PromptEchoOn)?; let input_needed = xlat!("input needed"); tty.read_input( &format!("[{}: {input_needed} {msg} ", self.name), @@ -159,7 +221,7 @@ impl Converser for CLIConverser { } fn handle_hidden_prompt(&self, msg: &str) -> PamResult { - let (mut tty, _guard) = self.open()?; + let (mut tty, _guard) = self.open(PamMessageStyle::PromptEchoOff)?; tty.read_input( msg, self.password_timeout, @@ -172,12 +234,12 @@ impl Converser for CLIConverser { } fn handle_error(&self, msg: &str) -> PamResult<()> { - let (mut tty, _) = self.open()?; + let (mut tty, _) = self.open(PamMessageStyle::ErrorMessage)?; Ok(tty.prompt(&format!("[{} error] {msg}\n", self.name))?) } fn handle_info(&self, msg: &str) -> PamResult<()> { - let (mut tty, _) = self.open()?; + let (mut tty, _) = self.open(PamMessageStyle::TextInfo)?; Ok(tty.prompt(&format!("[{}] {msg}\n", self.name))?) } } @@ -473,4 +535,92 @@ mod test { assert!(hello.panicked); // allowed now } + + #[test] + fn tty_unavailable_fallback_for_hidden_prompt_uses_askpass_only_with_display_and_askpass() { + assert_eq!( + CLIConverser::tty_unavailable_fallback( + PamMessageStyle::PromptEchoOff, + AskpassEnv { + has_display: true, + has_askpass: true, + }, + ), + TtyUnavailableFallback::Askpass + ); + assert_eq!( + CLIConverser::tty_unavailable_fallback( + PamMessageStyle::PromptEchoOff, + AskpassEnv { + has_display: true, + has_askpass: false, + }, + ), + TtyUnavailableFallback::Error + ); + assert_eq!( + CLIConverser::tty_unavailable_fallback( + PamMessageStyle::PromptEchoOff, + AskpassEnv { + has_display: false, + has_askpass: true, + }, + ), + TtyUnavailableFallback::Error + ); + assert_eq!( + CLIConverser::tty_unavailable_fallback( + PamMessageStyle::PromptEchoOff, + AskpassEnv { + has_display: false, + has_askpass: false, + }, + ), + TtyUnavailableFallback::Error + ); + } + + #[test] + fn tty_unavailable_fallback_for_non_hidden_prompt_uses_stdio() { + assert_eq!( + CLIConverser::tty_unavailable_fallback( + PamMessageStyle::PromptEchoOn, + AskpassEnv { + has_display: true, + has_askpass: true, + }, + ), + TtyUnavailableFallback::Stdie + ); + assert_eq!( + CLIConverser::tty_unavailable_fallback( + PamMessageStyle::PromptEchoOn, + AskpassEnv { + has_display: true, + has_askpass: false, + }, + ), + TtyUnavailableFallback::Stdie + ); + assert_eq!( + CLIConverser::tty_unavailable_fallback( + PamMessageStyle::PromptEchoOn, + AskpassEnv { + has_display: false, + has_askpass: true, + }, + ), + TtyUnavailableFallback::Stdie + ); + assert_eq!( + CLIConverser::tty_unavailable_fallback( + PamMessageStyle::PromptEchoOn, + AskpassEnv { + has_display: false, + has_askpass: false, + }, + ), + TtyUnavailableFallback::Stdie + ); + } } diff --git a/test-framework/sudo-compliance-tests/src/sudo/pam.rs b/test-framework/sudo-compliance-tests/src/sudo/pam.rs index 259901707..d2036315f 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/pam.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/pam.rs @@ -472,6 +472,50 @@ auth requisite pam_deny.so .assert_success(); } +#[test] +fn no_tty_pam_text_info_falls_back_to_stdio() { + let env = Env("ALL ALL=(ALL:ALL) ALL") + .user(USERNAME) + .file( + "/etc/pam.d/sudo", + [ + "auth optional pam_echo.so Hello sudo-rs, I am PAM", + "auth sufficient pam_permit.so", + ] + .join("\n"), + ) + .build(); + + Command::new("sh") + .args(["-c", "sudo true /tmp/repro.log 2>&1"]) + .as_user(USERNAME) + .output(&env) + .assert_success(); +} + +#[test] +fn no_tty_pam_text_info_uses_stdio_fallback() { + let env = Env("ALL ALL=(ALL:ALL) NOPASSWD: ALL") + .file( + "/etc/pam.d/sudo", + [ + "auth sufficient pam_permit.so", + "account sufficient pam_permit.so", + "session optional pam_echo.so Hello sudo-rs, I am PAM", + "session sufficient pam_permit.so", + ] + .join("\n"), + ) + .user(USERNAME) + .build(); + + Command::new("sh") + .args(["-c", "sudo true /tmp/repro.log 2>&1", + ]) + .as_user(USERNAME) + .output(&env) + .assert_success(); +} + +#[test] +fn no_tty_uses_askpass_with_custom_prompt_when_display_is_set() { + let env = Env(format!("{USERNAME} ALL=(ALL:ALL) ALL")) + .file( + "/bin/askpass", + TextFile(format!( + "#!/bin/sh\necho \"$1\" > /tmp/prompt\necho {PASSWORD}" + )) + .chmod(CHMOD_EXEC), + ) + .user(User(USERNAME).password(PASSWORD)) + .build(); + + Command::new("sh") + .args([ + "-c", + "SUDO_ASKPASS=/bin/askpass DISPLAY=:0 sudo -p 'my fancy prompt' true /tmp/repro.log 2>&1", + ]) + .as_user(USERNAME) + .output(&env) + .assert_success(); + + let output = Command::new("cat").arg("/tmp/prompt").output(&env); + assert_contains!(output.stdout(), "my fancy prompt"); +} + +#[test] +fn no_tty_does_not_use_askpass_without_display() { + let env = Env(format!("{USERNAME} ALL=(ALL:ALL) ALL")) + .file("/bin/askpass", generate_askpass(PASSWORD)) + .user(User(USERNAME).password(PASSWORD)) + .build(); + + let output = Command::new("sh") + .args([ + "-c", + "SUDO_ASKPASS=/bin/askpass sudo true /tmp/repro.log", + ]) + .as_user(USERNAME) + .output(&env); + + output.assert_exit_code(1); + let diagnostic = if sudo_test::is_original_sudo() { + "a terminal is required to read the password" + } else { + "A terminal is required to authenticate" + }; + assert_contains!(output.stderr(), diagnostic); +} + +#[test] +fn no_tty_with_display_but_without_askpass_still_fails() { + let env = Env(format!("{USERNAME} ALL=(ALL:ALL) ALL")) + .user(User(USERNAME).password(PASSWORD)) + .build(); + + let output = Command::new("sh") + .args(["-c", "DISPLAY=:0 sudo true /tmp/repro.log"]) + .as_user(USERNAME) + .output(&env); + + output.assert_exit_code(1); + let diagnostic = if sudo_test::is_original_sudo() { + "a terminal is required to read the password" + } else { + "A terminal is required to authenticate" + }; + assert_contains!(output.stderr(), diagnostic); +} + #[test] fn incorrect_password() { let env = Env(format!("{USERNAME} ALL=(ALL:ALL) ALL")) From 465dbd5a8f44c97599a8024228099c280f420908 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 21 May 2026 20:33:41 +0200 Subject: [PATCH 2/4] converse: Also use Wayland environment variable to check display state Following what has been proposed via https://github.com/sudo-project/sudo/pull/540 --- src/pam/converse.rs | 4 +- .../src/sudo/pass_auth/askpass.rs | 46 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index d5ae03d75..e1e8d9eda 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -199,7 +199,9 @@ struct AskpassEnv { impl AskpassEnv { fn from_env() -> Self { Self { - has_display: std::env::var_os("DISPLAY").is_some(), + has_display: std::env::var_os("DISPLAY").is_some() + || std::env::var_os("WAYLAND_DISPLAY").is_some() + || std::env::var_os("WAYLAND_SOCKET").is_some(), has_askpass: std::env::var_os("SUDO_ASKPASS").is_some(), } } diff --git a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/askpass.rs b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/askpass.rs index 2001039ac..597a135bd 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/askpass.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/askpass.rs @@ -67,6 +67,52 @@ fn no_tty_uses_askpass_with_custom_prompt_when_display_is_set() { assert_contains!(output.stdout(), "my fancy prompt"); } +#[test] +fn no_tty_uses_askpass_when_wayland_display_is_set() { + if sudo_test::is_original_sudo() { + // TODO: Remove this once sudo-project/sudo commit a9859d3 + // is available in the test container. + return; + } + + let env = Env(format!("{USERNAME} ALL=(ALL:ALL) ALL")) + .file("/bin/askpass", generate_askpass(PASSWORD)) + .user(User(USERNAME).password(PASSWORD)) + .build(); + + Command::new("sh") + .args([ + "-c", + "SUDO_ASKPASS=/bin/askpass WAYLAND_DISPLAY=wayland-0 sudo true /tmp/repro.log 2>&1", + ]) + .as_user(USERNAME) + .output(&env) + .assert_success(); +} + +#[test] +fn no_tty_uses_askpass_when_wayland_socket_is_set() { + if sudo_test::is_original_sudo() { + // TODO: Remove this once sudo-project/sudo commit a9859d3 + // is available in the test container. + return; + } + + let env = Env(format!("{USERNAME} ALL=(ALL:ALL) ALL")) + .file("/bin/askpass", generate_askpass(PASSWORD)) + .user(User(USERNAME).password(PASSWORD)) + .build(); + + Command::new("sh") + .args([ + "-c", + "SUDO_ASKPASS=/bin/askpass WAYLAND_SOCKET=7 sudo true /tmp/repro.log 2>&1", + ]) + .as_user(USERNAME) + .output(&env) + .assert_success(); +} + #[test] fn no_tty_does_not_use_askpass_without_display() { let env = Env(format!("{USERNAME} ALL=(ALL:ALL) ALL")) From 1af2b3602d5eece489642786872deeda5bd17852 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 21 May 2026 21:26:37 +0200 Subject: [PATCH 3/4] pam: Adjust error message to match sudo.ws on missing tty In case no TTY is available, sudo suggests using `-S` option, while we do not do it in sudo-rs. Use the same logic of sudo.ws here. --- src/pam/converse.rs | 16 +++++++++++++++- src/pam/error.rs | 5 +++++ .../src/sudo/pass_auth/askpass.rs | 8 ++++---- .../src/sudo/pass_auth/tty.rs | 4 ++-- 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index e1e8d9eda..d7074bf8b 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -145,7 +145,9 @@ impl CLIConverser { match Self::tty_unavailable_fallback(style, AskpassEnv::from_env()) { TtyUnavailableFallback::Askpass => Terminal::open_askpass()?, TtyUnavailableFallback::Stdie => Terminal::open_stdie()?, - TtyUnavailableFallback::Error => return Err(PamError::TtyRequired), + TtyUnavailableFallback::Error => { + return Err(Self::tty_required_error()); + } } } Err(err) => return Err(err), @@ -181,6 +183,10 @@ impl CLIConverser { | PamMessageStyle::TextInfo => TtyUnavailableFallback::Stdie, } } + + fn tty_required_error() -> PamError { + PamError::TtyRequiredNoTtyPrompt + } } #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -625,4 +631,12 @@ mod test { TtyUnavailableFallback::Stdie ); } + + #[test] + fn tty_required_error_is_no_tty_prompt() { + assert!(matches!( + CLIConverser::tty_required_error(), + PamError::TtyRequiredNoTtyPrompt + )); + } } diff --git a/src/pam/error.rs b/src/pam/error.rs index d5facb08a..f5d6f8241 100644 --- a/src/pam/error.rs +++ b/src/pam/error.rs @@ -177,6 +177,7 @@ pub enum PamError { Pam(PamErrorType), IoError(std::io::Error), TtyRequired, + TtyRequiredNoTtyPrompt, EnvListFailure, InteractionRequired, NoPasswordProvided, @@ -225,6 +226,10 @@ impl fmt::Display for PamError { PamError::Pam(tp) => xlat_write!(f, "PAM error: {error}", error = tp.get_err_msg()), PamError::IoError(e) => xlat_write!(f, "IO error: {error}", error = e), PamError::TtyRequired => xlat_write!(f, "A terminal is required to authenticate"), + PamError::TtyRequiredNoTtyPrompt => xlat_write!( + f, + "A terminal is required to authenticate; either use the -S option to read the password from standard input or configure an askpass helper" + ), PamError::EnvListFailure => { xlat_write!( f, diff --git a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/askpass.rs b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/askpass.rs index 597a135bd..4d8fd60c7 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/askpass.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/askpass.rs @@ -130,9 +130,9 @@ fn no_tty_does_not_use_askpass_without_display() { output.assert_exit_code(1); let diagnostic = if sudo_test::is_original_sudo() { - "a terminal is required to read the password" + "a terminal is required to read the password; either use the -S option to read from standard input or configure an askpass helper" } else { - "A terminal is required to authenticate" + "A terminal is required to authenticate; either use the -S option to read the password from standard input or configure an askpass helper" }; assert_contains!(output.stderr(), diagnostic); } @@ -150,9 +150,9 @@ fn no_tty_with_display_but_without_askpass_still_fails() { output.assert_exit_code(1); let diagnostic = if sudo_test::is_original_sudo() { - "a terminal is required to read the password" + "a terminal is required to read the password; either use the -S option to read from standard input or configure an askpass helper" } else { - "A terminal is required to authenticate" + "A terminal is required to authenticate; either use the -S option to read the password from standard input or configure an askpass helper" }; assert_contains!(output.stderr(), diagnostic); } diff --git a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs index d63bdf0f4..828cf2c99 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs @@ -50,9 +50,9 @@ fn no_tty() { output.assert_exit_code(1); let diagnostic = if sudo_test::is_original_sudo() { - "a terminal is required to read the password" + "a terminal is required to read the password; either use the -S option to read from standard input or configure an askpass helper" } else { - "A terminal is required to authenticate" + "A terminal is required to authenticate; either use the -S option to read the password from standard input or configure an askpass helper" }; assert_contains!(output.stderr(), diagnostic); } From 36901e35b8a8a77d679b7d854e6de4f39c01717b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 21 May 2026 21:41:02 +0200 Subject: [PATCH 4/4] pam/converse: Suggest using ssh -t when no tty is found The current error does not provide any relevant information to the user, so use the same hint that sudo.ws does. Mimic original sudo commit 516f7296. --- src/pam/converse.rs | 24 +++++++++++++-- src/pam/error.rs | 5 ++++ .../src/sudo/pass_auth/tty.rs | 29 +++++++++++++++++++ 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index d7074bf8b..011add969 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -185,8 +185,20 @@ impl CLIConverser { } fn tty_required_error() -> PamError { + if std::env::var_os("SSH_CONNECTION").is_some() && std::env::var_os("SSH_TTY").is_none() { + Self::tty_required_error_ssh() + } else { + Self::tty_required_error_no_tty_prompt() + } + } + + fn tty_required_error_no_tty_prompt() -> PamError { PamError::TtyRequiredNoTtyPrompt } + + fn tty_required_error_ssh() -> PamError { + PamError::TtyRequiredSsh + } } #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -633,10 +645,18 @@ mod test { } #[test] - fn tty_required_error_is_no_tty_prompt() { + fn tty_required_error_no_tty_prompt_uses_s_hint() { assert!(matches!( - CLIConverser::tty_required_error(), + CLIConverser::tty_required_error_no_tty_prompt(), PamError::TtyRequiredNoTtyPrompt )); } + + #[test] + fn tty_required_error_ssh_uses_ssh_t_hint() { + assert!(matches!( + CLIConverser::tty_required_error_ssh(), + PamError::TtyRequiredSsh + )); + } } diff --git a/src/pam/error.rs b/src/pam/error.rs index f5d6f8241..a90f9bf9f 100644 --- a/src/pam/error.rs +++ b/src/pam/error.rs @@ -178,6 +178,7 @@ pub enum PamError { IoError(std::io::Error), TtyRequired, TtyRequiredNoTtyPrompt, + TtyRequiredSsh, EnvListFailure, InteractionRequired, NoPasswordProvided, @@ -230,6 +231,10 @@ impl fmt::Display for PamError { f, "A terminal is required to authenticate; either use the -S option to read the password from standard input or configure an askpass helper" ), + PamError::TtyRequiredSsh => xlat_write!( + f, + "A terminal is required to authenticate; either use ssh's -t option or configure an askpass helper" + ), PamError::EnvListFailure => { xlat_write!( f, diff --git a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs index 828cf2c99..16c02265f 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs @@ -57,6 +57,35 @@ fn no_tty() { assert_contains!(output.stderr(), diagnostic); } +#[test] +fn no_tty_over_ssh_suggests_ssh_t() { + let env = Env(format!("{USERNAME} ALL=(ALL:ALL) ALL")) + .user(User(USERNAME).password(PASSWORD)) + .build(); + + let output = Command::new("sh") + .args([ + "-c", + "LANG=C LC_ALL=C SSH_CONNECTION='127.0.0.1 33860 127.0.0.1 22' sudo true /tmp/repro.log", + ]) + .as_user(USERNAME) + .output(&env); + output.assert_exit_code(1); + + if sudo_test::is_original_sudo() { + // TODO: Remove this once sudo-project/sudo commit 516f72960 (sudo v1.9.17) + // is broadly available in the test container. + return; + } + + let diagnostic = if sudo_test::is_original_sudo() { + "a terminal is required to read the password; either use ssh's -t option or configure an askpass helper" + } else { + "A terminal is required to authenticate; either use ssh's -t option or configure an askpass helper" + }; + assert_contains!(output.stderr(), diagnostic); +} + #[test] fn longest_possible_password_works() { let password = "a".repeat(MAX_PASSWORD_SIZE);