Fix : fix gid duplication, additional_gids etc not getting used in exec

crates/libcontainer/src/container/tenant_builder.rs

@@ -14,7 +14,7 @@ use nix::unistd::{pipe2, read, Pid};
 use oci_spec::runtime::{
     Capabilities as SpecCapabilities, Capability as SpecCapability, LinuxBuilder,
     LinuxCapabilities, LinuxCapabilitiesBuilder, LinuxNamespace, LinuxNamespaceBuilder,
-    LinuxNamespaceType, LinuxSchedulerPolicy, Process, ProcessBuilder, Spec,
+    LinuxNamespaceType, LinuxSchedulerPolicy, Process, ProcessBuilder, Spec, UserBuilder,
 };
 use procfs::process::Namespace;
 
@@ -32,6 +32,22 @@ const NAMESPACE_TYPES: &[&str] = &["ipc", "uts", "net", "pid", "mnt", "cgroup"];
 const TENANT_NOTIFY: &str = "tenant-notify-";
 const TENANT_TTY: &str = "tenant-tty-";
 
+fn get_path_from_spec(spec: &Spec) -> Option<String> {
+    let process = match spec.process() {
+        Some(p) => p,
+        None => return None,
+    };
+    let env = match process.env() {
+        Some(e) => e,
+        None => return None,
+    };
+    env.iter()
+        .find(|e| e.starts_with("PATH"))
+        .iter()
+        .next()
+        .map(|s| s.to_string())
+}
+
 /// Builder that can be used to configure the properties of a process
 /// that will join an existing container sandbox
 pub struct TenantContainerBuilder {
@@ -44,6 +60,9 @@ pub struct TenantContainerBuilder {
     process: Option<PathBuf>,
     detached: bool,
     as_sibling: bool,
+    additional_gids: Vec<u32>,
+    user: Option<u32>,
+    group: Option<u32>,
 }
 
 impl TenantContainerBuilder {
@@ -61,6 +80,9 @@ impl TenantContainerBuilder {
             process: None,
             detached: false,
             as_sibling: false,
+            additional_gids: vec![],
+            user: None,
+            group: None,
         }
     }
 
@@ -109,6 +131,21 @@ impl TenantContainerBuilder {
         self
     }
 
+    pub fn with_additional_gids(mut self, gids: Vec<u32>) -> Self {
+        self.additional_gids = gids;
+        self
+    }
+
+    pub fn with_user(mut self, user: Option<u32>) -> Self {
+        self.user = user;
+        self
+    }
+
+    pub fn with_group(mut self, group: Option<u32>) -> Self {
+        self.group = group;
+        self
+    }
+
     /// Joins an existing container
     pub fn build(self) -> Result<Pid, LibcontainerError> {
         let container_dir = self.lookup_container_dir()?;
@@ -323,9 +360,10 @@ impl TenantContainerBuilder {
         let process = if let Some(process) = &self.process {
             self.get_process(process)?
         } else {
+            let original_path_env = get_path_from_spec(spec);
             let mut process_builder = ProcessBuilder::default()
                 .args(self.get_args()?)
-                .env(self.get_environment());
+                .env(self.get_environment(original_path_env));
             if let Some(cwd) = self.get_working_dir()? {
                 process_builder = process_builder.cwd(cwd);
             }
@@ -338,6 +376,22 @@ impl TenantContainerBuilder {
                 process_builder = process_builder.capabilities(caps);
             }
 
+            let mut user_builder = UserBuilder::default();
+
+            if !self.additional_gids.is_empty() {
+                user_builder = user_builder.additional_gids(self.additional_gids.clone());
+            }
+
+            if let Some(uid) = self.user {
+                user_builder = user_builder.uid(uid);
+            }
+
+            if let Some(gid) = self.group {
+                user_builder = user_builder.gid(gid);
+            }
+
+            process_builder = process_builder.user(user_builder.build()?);
+
             process_builder.build()?
         };
 
@@ -397,8 +451,28 @@ impl TenantContainerBuilder {
         Ok(self.args.clone())
     }
 
-    fn get_environment(&self) -> Vec<String> {
-        self.env.iter().map(|(k, v)| format!("{k}={v}")).collect()
+    fn get_environment(&self, path: Option<String>) -> Vec<String> {
+        let mut env_exists = false;
+        let mut env: Vec<String> = self
+            .env
+            .iter()
+            .map(|(k, v)| {
+                if k == "PATH" {
+                    env_exists = true;
+                }
+                format!("{k}={v}")
+            })
+            .collect();
+        // It is not possible in normal flow that path is None. The original container
+        // creation would have failed if path was absent. However we use Option
+        // just as a caution, and if neither exec cmd not original spec has PATH,
+        // the container creation will fail later which is ok
+        if let Some(p) = path {
+            if !env_exists {
+                env.push(p);
+            }
+        }
+        env
     }
 
     fn get_no_new_privileges(&self) -> Option<bool> {

crates/libcontainer/src/process/container_init_process.rs

@@ -1,4 +1,4 @@
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 use std::{env, fs, mem};
@@ -761,6 +761,9 @@ fn set_supplementary_gids(
 
         let gids: Vec<Gid> = additional_gids
             .iter()
+            // this is to remove duplicate ids, so we behave similar to runc
+            .collect::<HashSet<_>>()
+            .into_iter()
             .map(|gid| Gid::from_raw(*gid))
             .collect();
 

crates/youki/src/commands/exec.rs

@@ -9,6 +9,11 @@ use nix::sys::wait::{waitpid, WaitStatus};
 use crate::workload::executor::default_executor;
 
 pub fn exec(args: Exec, root_path: PathBuf) -> Result<i32> {
+    // TODO: not all values from exec are used here. We need to support
+    // the remaining ones.
+    let user = args.user.map(|(u, _)| u);
+    let group = args.user.and_then(|(_, g)| g);
+
     let pid = ContainerBuilder::new(args.container_id.clone(), SyscallType::default())
         .with_executor(default_executor())
         .with_root_path(root_path)?
@@ -22,6 +27,9 @@ pub fn exec(args: Exec, root_path: PathBuf) -> Result<i32> {
         .with_process(args.process.as_ref())
         .with_no_new_privs(args.no_new_privs)
         .with_container_args(args.command.clone())
+        .with_additional_gids(args.additional_gids)
+        .with_user(user)
+        .with_group(group)
         .build()?;
 
     // See https://github.com/containers/youki/pull/1252 for a detailed explanation

tests/contest/contest/src/tests/process_user/process_user_test.rs

@@ -1,4 +1,4 @@
-use anyhow::{Context, Ok, Result};
+use anyhow::{anyhow, Context, Ok, Result};
 use oci_spec::runtime::{ProcessBuilder, Spec, SpecBuilder, UserBuilder};
 use rand::Rng;
 use test_framework::{test_result, Test, TestGroup, TestResult};
@@ -21,12 +21,12 @@ fn generate_unique_random_vec() -> Vec<u32> {
     ret
 }
 
-fn create_spec() -> Result<Spec> {
+fn create_spec(gids: Vec<u32>) -> Result<Spec> {
     let umask = 0o002;
     let user = UserBuilder::default()
         .uid(10u32)
         .gid(10u32)
-        .additional_gids(generate_unique_random_vec())
+        .additional_gids(gids)
         .umask(umask as u32)
         .build()?;
 
@@ -42,16 +42,38 @@ fn create_spec() -> Result<Spec> {
         .context("failed to build spec")?;
     Ok(spec)
 }
-fn process_user_test() -> TestResult {
-    let spec = test_result!(create_spec());
+
+fn process_user_test_unique_gids() -> TestResult {
+    let gids = generate_unique_random_vec();
+    let spec = test_result!(create_spec(gids));
     test_inside_container(spec, &CreateOptions::default(), &|_| Ok(()))
 }
 
+fn process_user_test_duplicate_gids() -> TestResult {
+    let mut gids = generate_unique_random_vec();
+    let duplicate = gids[0];
+    gids.push(duplicate);
+    let spec = test_result!(create_spec(gids));
+    match test_inside_container(spec, &CreateOptions::default(), &|_| Ok(())) {
+        TestResult::Passed => TestResult::Failed(anyhow!(
+            "expected test with duplicate gids to fail, but it passed instead"
+        )),
+        _ => TestResult::Passed,
+    }
+}
+
 pub fn get_process_user_test() -> TestGroup {
     let mut process_user_test_group = TestGroup::new("process_user");
 
-    let test = Test::new("process_user_test", Box::new(process_user_test));
-    process_user_test_group.add(vec![Box::new(test)]);
+    let test1 = Test::new(
+        "process_user_unique_gids_test",
+        Box::new(process_user_test_unique_gids),
+    );
+    let test2 = Test::new(
+        "process_user_duplicate_gids_test",
+        Box::new(process_user_test_duplicate_gids),
+    );
+    process_user_test_group.add(vec![Box::new(test1), Box::new(test2)]);
 
     process_user_test_group
 }