Skip to content

Commit

Permalink
Set some implicit options for og-sudo to match sudo-rs defaults (#958)
Browse files Browse the repository at this point in the history
  • Loading branch information
squell authored Jan 21, 2025
2 parents 4b644d8 + 93daf67 commit f0cb82f
Show file tree
Hide file tree
Showing 11 changed files with 79 additions and 26 deletions.
3 changes: 3 additions & 0 deletions src/defaults/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ defaults! {
always_query_group_plugin = false #ignored
always_set_home = false #ignored
env_reset = true #ignored
fqdn = false #ignored
lecture = false #ignored
mailerpath = None (!= None) #ignored
mail_badpass = true #ignored
match_group_by_gid = false #ignored
use_pty = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
source: sudo-compliance-tests/src/sudo/sudoers/cmnd.rs
expression: stderr
---
/etc/sudoers:1:19: expected a fully-qualified path name
/etc/sudoers:2:19: expected a fully-qualified path name
ALL ALL=(ALL:ALL) true
^~~~
root is not in the sudoers file.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
source: sudo-compliance-tests/src/sudo/sudoers/cmnd_alias.rs
expression: stderr
---
/etc/sudoers:1:24: expected a fully-qualified path name
/etc/sudoers:2:24: expected a fully-qualified path name
Cmnd_Alias CMDSGROUP = true, /usr/bin/ls
^~~~
Sorry, user root is not allowed to execute '/usr/bin/true' as root on [host].
30 changes: 24 additions & 6 deletions test-framework/sudo-compliance-tests/src/sudo/flag_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,23 @@ mod not_allowed;
mod short_format;
mod sudoers_list;

// sudo-rs doesn't yet support showing Defaults in the `-l` output, so strip
// them with og-sudo to get the same output between both.
fn strip_matching_defaults_message(s: &str) -> &str {
if s.starts_with("Matching Defaults entries for") {
s.split_once("\n")
.unwrap()
.1
.split_once("\n")
.unwrap()
.1
.strip_prefix("\n")
.unwrap()
} else {
s
}
}

#[test]
fn root_cannot_use_list_when_empty_sudoers() -> Result<()> {
let hostname = "container";
Expand Down Expand Up @@ -87,7 +104,7 @@ fn lists_privileges_for_root() -> Result<()> {
(ALL : ALL) NOPASSWD: ALL"
);
let actual = output.stdout()?;
assert_eq!(actual, expected);
assert_eq!(strip_matching_defaults_message(&actual), expected);

Ok(())
}
Expand All @@ -106,7 +123,7 @@ fn works_with_long_form_list_flag() -> Result<()> {
(ALL : ALL) NOPASSWD: ALL"
);
let actual = output.stdout()?;
assert_eq!(actual, expected);
assert_eq!(strip_matching_defaults_message(&actual), expected);

Ok(())
}
Expand All @@ -132,7 +149,7 @@ fn lists_privileges_for_invoking_user_on_current_host() -> Result<()> {
(ALL : ALL) NOPASSWD: ALL"
);
let actual = output.stdout()?;
assert_eq!(actual, expected);
assert_eq!(strip_matching_defaults_message(&actual), expected);

Ok(())
}
Expand All @@ -157,7 +174,7 @@ fn works_with_uppercase_u_flag() -> Result<()> {
(ALL : ALL) NOPASSWD: ALL"
);
let actual = output.stdout()?;
assert_eq!(actual, expected);
assert_eq!(strip_matching_defaults_message(&actual), expected);

Ok(())
}
Expand Down Expand Up @@ -257,7 +274,8 @@ Sudoers entry:
);
let actual = output.stdout()?;
assert_eq!(
actual.replace(&format!("Sudoers entry: {ETC_SUDOERS}"), "Sudoers entry:"),
strip_matching_defaults_message(&actual)
.replace(&format!("Sudoers entry: {ETC_SUDOERS}"), "Sudoers entry:"),
expected
);

Expand Down Expand Up @@ -355,7 +373,7 @@ fn uppercase_u_flag_matches_on_first_component_of_sudoers_rules() -> Result<()>
(root : ALL) /usr/bin/whoami"
);
let actual = output.stdout()?;
assert_eq!(actual, expected);
assert_eq!(strip_matching_defaults_message(&actual), expected);

Ok(())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ macro_rules! assert_snapshot {
filters => vec![
(BIN_LS, "<BIN_LS>"),
(&format!("Sudoers entry: {ETC_SUDOERS}"), "Sudoers entry:"),
("Matching Defaults entries for root on container:
!fqdn, !lecture, !mailerpath
", "")
],
prepend_module_to_snapshot => false,
}, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ use crate::{Result, HOSTNAME};
macro_rules! assert_snapshot {
($($tt:tt)*) => {
insta::with_settings!({
filters => vec![(BIN_LS, "<BIN_LS>")],
filters => vec![
(BIN_LS, "<BIN_LS>"),
("Matching Defaults entries for root on container:
!fqdn, !lecture, !mailerpath
", "")],
prepend_module_to_snapshot => false,
}, {
insta::assert_snapshot!($($tt)*)
Expand Down
11 changes: 6 additions & 5 deletions test-framework/sudo-compliance-tests/src/visudo.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::{thread, time::Duration};

use sudo_test::{
helpers::assert_ls_output, Command, Env, TextFile, ETC_DIR, ETC_SUDOERS, ROOT_GROUP,
helpers::assert_ls_output, Command, Env, EnvNoImplicit, TextFile, ETC_DIR, ETC_SUDOERS,
ROOT_GROUP,
};

use crate::{Result, PANIC_EXIT_CODE, SUDOERS_ALL_ALL_NOPASSWD};
Expand Down Expand Up @@ -207,7 +208,7 @@ echo '{expected}' >> $2"#
#[test]
fn stderr_message_when_file_is_not_modified() -> Result<()> {
let expected = SUDOERS_ALL_ALL_NOPASSWD;
let env = Env(expected)
let env = EnvNoImplicit(expected)
.file(
DEFAULT_EDITOR,
TextFile(
Expand Down Expand Up @@ -244,7 +245,7 @@ fn stderr_message_when_file_is_not_modified() -> Result<()> {
#[test]
fn does_not_save_the_file_if_there_are_syntax_errors() -> Result<()> {
let expected = SUDOERS_ALL_ALL_NOPASSWD;
let env = Env(expected)
let env = EnvNoImplicit(expected)
.file(
DEFAULT_EDITOR,
TextFile(
Expand Down Expand Up @@ -274,7 +275,7 @@ echo 'this is fine' > $2",
#[test]
fn editor_exits_with_a_nonzero_code() -> Result<()> {
let expected = SUDOERS_ALL_ALL_NOPASSWD;
let env = Env(SUDOERS_ALL_ALL_NOPASSWD)
let env = EnvNoImplicit(SUDOERS_ALL_ALL_NOPASSWD)
.file(
DEFAULT_EDITOR,
TextFile(
Expand Down Expand Up @@ -332,7 +333,7 @@ rm $2",
#[test]
fn temp_file_initial_contents() -> Result<()> {
let expected = SUDOERS_ALL_ALL_NOPASSWD;
let env = Env(expected)
let env = EnvNoImplicit(expected)
.file(
DEFAULT_EDITOR,
TextFile(format!(
Expand Down
3 changes: 2 additions & 1 deletion test-framework/sudo-compliance-tests/src/visudo/flag_file.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use sudo_test::EnvNoImplicit;
use sudo_test::{helpers::assert_ls_output, Command, Env, TextFile, ROOT_GROUP};

use crate::{
Expand Down Expand Up @@ -133,7 +134,7 @@ echo '{expected}' > $2"#
fn etc_sudoers_is_not_modified() -> Result<()> {
let expected = SUDOERS_ALL_ALL_NOPASSWD;
let unexpected = SUDOERS_ROOT_ALL;
let env = Env(expected)
let env = EnvNoImplicit(expected)
.file(
DEFAULT_EDITOR,
TextFile(format!(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use sudo_test::{Command, Env, TextFile};
use sudo_test::{Command, Env, EnvNoImplicit, TextFile};

use crate::{
visudo::{CHMOD_EXEC, DEFAULT_EDITOR, ETC_SUDOERS, LOGS_PATH},
Expand Down Expand Up @@ -54,7 +54,7 @@ fn on_e_re_edits() -> Result<()> {
#[test]
fn on_x_closes_without_saving_changes() -> Result<()> {
let expected = SUDOERS_ALL_ALL_NOPASSWD;
let env = Env(expected)
let env = EnvNoImplicit(expected)
.file(DEFAULT_EDITOR, TextFile(editor()).chmod(CHMOD_EXEC))
.build()?;

Expand Down
12 changes: 4 additions & 8 deletions test-framework/sudo-test/src/docker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,10 @@ impl Container {
pub fn new_with_hostname(image: &str, hostname: Option<&str>) -> Result<Self> {
let mut docker_run = docker_command();
docker_run.args(["run", "--detach"]);
if !crate::is_original_sudo() {
// Disable network access for the containers. This removes the overhead of setting up a
// new network namespace and associated firewall rule adjustments. We still need to keep
// network access enabled for original sudo however as it needs to be able to resolve
// it's own hostname to a fully qualified domain name, which isn't possible with
// `--net=none`.
docker_run.arg("--net=none");
}
// Disable network access for the containers. This removes the overhead
// of setting up a new network namespace and associated firewall rule
// adjustments.
docker_run.arg("--net=none");
if let Some(hostname) = hostname {
docker_run.args(["--hostname", hostname]);
}
Expand Down
29 changes: 28 additions & 1 deletion test-framework/sudo-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,24 @@ pub struct Env {
/// creates a new test environment builder that contains the specified `/etc/sudoers` file
#[allow(non_snake_case)]
pub fn Env(sudoers: impl Into<TextFile>) -> EnvBuilder {
let mut builder = EnvBuilder::default();
let mut sudoers: TextFile = sudoers.into();
// Change a couple of flags to match the defaults of sudo-rs. In particular some sudo builds
// have lecture or mailing enabled by default while sudo-rs doesn't implement those at all.
// And fqdn breaks when --net=none passed to docker and sudo-rs doesn't implement it either.
sudoers.contents.insert_str(0, "Defaults !fqdn, !lecture, !mailerpath\n");
builder.file(ETC_SUDOERS, sudoers);
if cfg!(target_os = "freebsd") {
// Many tests expect the users group to exist, but FreeBSD doesn't actually use it.
builder.group("users");
}
builder
}

/// creates a new test environment builder that contains the specified `/etc/sudoers` file without
/// any implicit extra rules
#[allow(non_snake_case)]
pub fn EnvNoImplicit(sudoers: impl Into<TextFile>) -> EnvBuilder {
let mut builder = EnvBuilder::default();
builder.file(ETC_SUDOERS, sudoers);
if cfg!(target_os = "freebsd") {
Expand Down Expand Up @@ -898,8 +916,17 @@ mod tests {

#[test]
fn sudoers_file_get_created_with_expected_contents() -> Result<()> {
let expected = "Defaults !fqdn, !lecture, !mailerpath\nHello, root!";
let env = Env("Hello, root!").build()?;

let actual = Command::new("cat")
.arg(ETC_SUDOERS)
.output(&env)?
.stdout()?;
assert_eq!(expected, actual);

let expected = "Hello, root!";
let env = Env(expected).build()?;
let env = EnvNoImplicit(expected).build()?;

let actual = Command::new("cat")
.arg(ETC_SUDOERS)
Expand Down

0 comments on commit f0cb82f

Please sign in to comment.