Merge branch 'main' into docker-as-sudo-playground

.github/workflows/ci.yaml

@@ -266,7 +266,7 @@ jobs:
       - name: Rust Cache
         uses: Swatinem/rust-cache@v2
         with:
-          shared-key: "stable"
+          shared-key: "audit"
       - name: Run audit
         uses: actions-rs/cargo@v1
         with:

lib/sudo-common/src/context.rs

@@ -102,14 +102,19 @@ fn resolve_current_user() -> Result<User, Error> {
 }
 
 fn resolve_target_user(target_name_or_id: &Option<String>) -> Result<User, Error> {
+    let is_default = target_name_or_id.is_none();
     let target_name_or_id = target_name_or_id.as_deref().unwrap_or("root");
 
-    match NameOrId::parse(target_name_or_id) {
+    let mut user = match NameOrId::parse(target_name_or_id) {
         Some(NameOrId::Name(name)) => User::from_name(name)?,
         Some(NameOrId::Id(uid)) => User::from_uid(uid)?,
         _ => None,
     }
-    .ok_or_else(|| Error::UserNotFound(target_name_or_id.to_string()))
+    .ok_or_else(|| Error::UserNotFound(target_name_or_id.to_string()))?;
+
+    user.is_default = is_default;
+
+    Ok(user)
 }
 
 fn resolve_target_group(
@@ -198,6 +203,7 @@ mod tests {
             shell: "/bin/sh".to_string(),
             passwd: String::new(),
             groups: None,
+            is_default: false,
         };
 
         assert_eq!(

lib/sudo-env/tests/env_tests.rs

@@ -86,6 +86,7 @@ fn create_test_context<'a>(sudo_options: &'a SudoOptions) -> Context<'a> {
         shell: "/bin/sh".to_string(),
         passwd: String::new(),
         groups: None,
+        is_default: false,
     };
 
     let current_group = Group {
@@ -104,6 +105,7 @@ fn create_test_context<'a>(sudo_options: &'a SudoOptions) -> Context<'a> {
         shell: "/bin/bash".to_string(),
         passwd: String::new(),
         groups: None,
+        is_default: false,
     };
 
     let root_group = Group {

lib/sudo-exec/src/lib.rs

@@ -31,7 +31,12 @@ pub fn run_command(ctx: Context<'_>, env: Environment) -> io::Result<ExitStatus>
     // reset env and set filtered environment
     command.args(ctx.command.arguments).env_clear().envs(env);
     // set target user and groups
-    set_target_user(&mut command, ctx.target_user, ctx.target_group);
+    set_target_user(
+        &mut command,
+        ctx.current_user,
+        ctx.target_user,
+        ctx.target_group,
+    );
     // spawn and exec to command
     let mut child = command.spawn()?;
 

lib/sudo-system/src/lib.rs

@@ -26,12 +26,44 @@ pub fn hostname() -> String {
 }
 
 /// set target user and groups (uid, gid, additional groups) for a command
-pub fn set_target_user(cmd: &mut std::process::Command, target_user: User, target_group: Group) {
+pub fn set_target_user(
+    cmd: &mut std::process::Command,
+    current_user: User,
+    target_user: User,
+    target_group: Group,
+) {
     use std::os::unix::process::CommandExt;
 
-    let uid = target_user.uid;
-    let gid = target_group.gid;
-    let groups = target_user.groups.unwrap_or_default();
+    // means that we are using the default user because `-u` was not passed.
+    let user_is_default = target_user.is_default;
+    // means that we are using the principal gid of the target user because `-g` was not passed or
+    // was passed with the principal gid.
+    let group_is_default = target_user.uid == target_group.gid;
+
+    let (uid, gid, groups) = if group_is_default {
+        // no `-g`: We just set the uid, gid and groups using the target user.
+        (
+            target_user.uid,
+            target_user.gid,
+            target_user.groups.unwrap_or_default(),
+        )
+    } else if user_is_default {
+        //  `-g` and no `-u`: The set uid must be the one of the current user and the set groups
+        //  must be the ones of the current user extended with the target group gid.
+        let mut groups = current_user.groups.unwrap_or_default();
+        if !groups.contains(&target_group.gid) {
+            groups.push(target_group.gid);
+        }
+        (current_user.uid, target_group.gid, groups)
+    } else {
+        // `-g` and `-u`: The set uid must be the one of the target user and the set groups must be
+        // the ones of the target group extended with the target group gid.
+        let mut groups = target_user.groups.unwrap_or_default();
+        if !groups.contains(&target_group.gid) {
+            groups.push(target_group.gid);
+        }
+        (target_user.uid, target_group.gid, groups)
+    };
 
     // we need to do this in a `pre_exec` call since the `groups` method in `process::Command` is unstable
     // see https://github.com/rust-lang/rust/blob/a01b4cc9f375f1b95fa8195daeea938d3d9c4c34/library/std/src/sys/unix/process/process_unix.rs#L329-L352
@@ -70,6 +102,7 @@ pub struct User {
     pub shell: String,
     pub passwd: String,
     pub groups: Option<Vec<libc::gid_t>>,
+    pub is_default: bool,
 }
 
 impl User {
@@ -83,6 +116,7 @@ impl User {
             shell: unsafe { string_from_ptr(pwd.pw_shell) },
             passwd: unsafe { string_from_ptr(pwd.pw_passwd) },
             groups: None,
+            is_default: false,
         }
     }
 

test-framework/sudo-compliance-tests/src/flag_group.rs

@@ -28,7 +28,6 @@ fn changes_the_group_id() -> Result<()> {
 }
 
 #[test]
-#[ignore]
 fn adds_group_to_groups_output() -> Result<()> {
     let extra_group = "rustaceans";
     let env = Env(SUDOERS_ALL_ALL_NOPASSWD)

test-framework/sudo-compliance-tests/src/sudoers.rs

@@ -3,6 +3,7 @@ use sudo_test::{Command, Env, TextFile, User};
 use crate::{Result, PASSWORD, SUDOERS_ROOT_ALL_NOPASSWD, USERNAME};
 
 mod cmnd;
+mod host_list;
 mod run_as;
 mod user_list;
 

test-framework/sudo-compliance-tests/src/sudoers/host_list.rs

@@ -0,0 +1,94 @@
+use sudo_test::{Command, Env};
+
+use crate::Result;
+
+#[test]
+fn given_specific_hostname_then_sudo_from_said_hostname_is_allowed() -> Result<()> {
+    let hostname = "container";
+    let env = Env(format!("ALL {hostname} = (ALL:ALL) ALL"))
+        .hostname(hostname)
+        .build()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .exec(&env)?
+        .assert_success()
+}
+
+#[test]
+fn given_specific_hostname_then_sudo_from_different_hostname_is_rejected() -> Result<()> {
+    let env = Env("ALL remotehost = (ALL:ALL) ALL")
+        .hostname("container")
+        .build()?;
+
+    let output = Command::new("sudo").arg("true").exec(&env)?;
+
+    assert!(!output.status().success());
+    assert_eq!(Some(1), output.status().code());
+
+    if sudo_test::is_original_sudo() {
+        assert_contains!(
+            output.stderr(),
+            "root is not allowed to run sudo on container"
+        );
+    }
+
+    Ok(())
+}
+
+#[test]
+fn different() -> Result<()> {
+    let env = Env("ALL remotehost, container = (ALL:ALL) ALL")
+        .hostname("container")
+        .build()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .exec(&env)?
+        .assert_success()
+}
+
+#[test]
+fn repeated() -> Result<()> {
+    let env = Env("ALL container, container = (ALL:ALL) ALL")
+        .hostname("container")
+        .build()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .exec(&env)?
+        .assert_success()
+}
+
+#[test]
+fn negation_rejects() -> Result<()> {
+    let env = Env("ALL remotehost, !container = (ALL:ALL) ALL")
+        .hostname("container")
+        .build()?;
+
+    let output = Command::new("sudo").arg("true").exec(&env)?;
+
+    assert!(!output.status().success());
+    assert_eq!(Some(1), output.status().code());
+
+    if sudo_test::is_original_sudo() {
+        assert_contains!(
+            output.stderr(),
+            "root is not allowed to run sudo on container"
+        );
+    }
+
+    Ok(())
+}
+
+#[test]
+fn double_negative_is_positive() -> Result<()> {
+    let env = Env("ALL !!container = (ALL:ALL) ALL")
+        .hostname("container")
+        .build()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .exec(&env)?
+        .assert_success()
+}

test-framework/sudo-compliance-tests/src/sudoers/user_list.rs

@@ -3,7 +3,7 @@
 use pretty_assertions::assert_eq;
 use sudo_test::{Command, Env};
 
-use crate::{Result, USERNAME};
+use crate::{Result, PAMD_SUDO_PAM_PERMIT, USERNAME};
 
 #[test]
 fn no_match() -> Result<()> {
@@ -76,3 +76,96 @@ fn group_id() -> Result<()> {
         .exec(&env)?
         .assert_success()
 }
+
+#[test]
+fn many_different() -> Result<()> {
+    let env = Env(format!("root, {USERNAME} ALL=(ALL:ALL) NOPASSWD: ALL"))
+        .user(USERNAME)
+        .build()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .exec(&env)?
+        .assert_success()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .as_user(USERNAME)
+        .exec(&env)?
+        .assert_success()
+}
+
+#[test]
+fn many_repeated() -> Result<()> {
+    let env = Env("root, root ALL=(ALL:ALL) NOPASSWD: ALL").build()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .exec(&env)?
+        .assert_success()
+}
+
+#[test]
+fn double_negative_is_positive() -> Result<()> {
+    let env = Env("!!root ALL=(ALL:ALL) NOPASSWD: ALL")
+        .user(USERNAME)
+        .build()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .exec(&env)?
+        .assert_success()
+}
+
+#[test]
+fn negation_excludes_group_members() -> Result<()> {
+    let env = Env("%users, !ghost ALL=(ALL:ALL) ALL")
+        // use PAM to avoid `ghost` getting a password prompt
+        .file("/etc/pam.d/sudo", PAMD_SUDO_PAM_PERMIT)
+        // the primary group of all new users is `users`
+        .user("ferris")
+        .user("ghost")
+        .build()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .as_user("ferris")
+        .exec(&env)?
+        .assert_success()?;
+
+    let output = Command::new("sudo")
+        .arg("true")
+        .as_user("ghost")
+        .exec(&env)?;
+
+    assert!(!output.status().success());
+    assert_eq!(Some(1), output.status().code());
+
+    if sudo_test::is_original_sudo() {
+        assert_contains!(output.stderr(), "ghost is not in the sudoers file");
+    }
+
+    Ok(())
+}
+
+#[test]
+fn negation_is_order_sensitive() -> Result<()> {
+    // negated items at the start of a specifier list  are meaningless
+    let env = Env("!ghost, %users ALL=(ALL:ALL) NOPASSWD: ALL")
+        // the primary group of all new users is `users`
+        .user("ferris")
+        .user("ghost")
+        .build()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .as_user("ferris")
+        .exec(&env)?
+        .assert_success()?;
+
+    Command::new("sudo")
+        .arg("true")
+        .as_user("ghost")
+        .exec(&env)?
+        .assert_success()
+}

test-framework/sudo-test/src/docker.rs

@@ -21,11 +21,18 @@ pub struct Container {
 }
 
 impl Container {
-    pub fn new(image: &str) -> Result<Self> {
+    #[cfg(test)]
+    fn new(image: &str) -> Result<Self> {
+        Self::new_with_hostname(image, None)
+    }
+
+    pub fn new_with_hostname(image: &str, hostname: Option<&str>) -> Result<Self> {
         let mut docker_run = StdCommand::new("docker");
-        docker_run
-            .args(["run", "-d", "--rm", image])
-            .args(DOCKER_RUN_COMMAND);
+        docker_run.args(["run", "--detach"]);
+        if let Some(hostname) = hostname {
+            docker_run.args(["--hostname", hostname]);
+        }
+        docker_run.args(["--rm", image]).args(DOCKER_RUN_COMMAND);
         let id = run(&mut docker_run, None)?.stdout()?;
         validate_docker_id(&id, &docker_run)?;