From 6c7e85c5c95e2953e92b2e47c45ff1e5d368ab9b Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Tue, 10 Dec 2024 19:08:44 +0530 Subject: [PATCH 1/2] fix duplicate gids in container creation Signed-off-by: Yashodhan Joshi --- .../src/process/container_init_process.rs | 5 ++- .../tests/process_user/process_user_test.rs | 36 +++++++++++++++---- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 0d16e6094..c49868b2c 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/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 = additional_gids .iter() + // this is to remove duplicate ids, so we behave similar to runc + .collect::>() + .into_iter() .map(|gid| Gid::from_raw(*gid)) .collect(); diff --git a/tests/contest/contest/src/tests/process_user/process_user_test.rs b/tests/contest/contest/src/tests/process_user/process_user_test.rs index 5329ccbbf..1d7787d6d 100644 --- a/tests/contest/contest/src/tests/process_user/process_user_test.rs +++ b/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 { ret } -fn create_spec() -> Result { +fn create_spec(gids: Vec) -> Result { 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 { .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 } From c856244d4068324be9dff5220ce5ea05c341f645 Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Tue, 10 Dec 2024 20:21:56 +0530 Subject: [PATCH 2/2] use additional gids,user,group in exec, inject path iif not given Signed-off-by: Yashodhan Joshi --- .../src/container/tenant_builder.rs | 82 ++++++++++++++++++- crates/youki/src/commands/exec.rs | 8 ++ 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/crates/libcontainer/src/container/tenant_builder.rs b/crates/libcontainer/src/container/tenant_builder.rs index 845c1af3d..047f48b97 100644 --- a/crates/libcontainer/src/container/tenant_builder.rs +++ b/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 { + 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, detached: bool, as_sibling: bool, + additional_gids: Vec, + user: Option, + group: Option, } 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) -> Self { + self.additional_gids = gids; + self + } + + pub fn with_user(mut self, user: Option) -> Self { + self.user = user; + self + } + + pub fn with_group(mut self, group: Option) -> Self { + self.group = group; + self + } + /// Joins an existing container pub fn build(self) -> Result { 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 { - self.env.iter().map(|(k, v)| format!("{k}={v}")).collect() + fn get_environment(&self, path: Option) -> Vec { + let mut env_exists = false; + let mut env: Vec = 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 { diff --git a/crates/youki/src/commands/exec.rs b/crates/youki/src/commands/exec.rs index dc68094cd..5efcc4def 100644 --- a/crates/youki/src/commands/exec.rs +++ b/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 { + // 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 { .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