diff --git a/MirgationGuide.md b/MirgationGuide.md index 8bc6ef415..30eae8ea0 100644 --- a/MirgationGuide.md +++ b/MirgationGuide.md @@ -1,3 +1,5 @@ +# Migration Guide + This contains information for migrating library versions. ## V0.1.0 -> v0.2.0 @@ -5,10 +7,14 @@ This contains information for migrating library versions. ### libcontainer - The `Rootless` struct has been re-named as `UserNamespaceConfig` , `RootlessIDMapper` has been re-named to `UserNamespaceIDMapper` , and correspondingly the `RootlessError` has been re-named to `UserNamespaceError` . This is due to the fact that the structure was to be used for containers when a new user namespace is to be created, and that is not strictly only for rootless uses. Accordingly, the fields of various structs has been updated to reflect this change : - - rootless (module name) -> user_ns - - Rootless::rootless_id_mapper -> UserNamespaceConfig::id_mapper - - LibcontainerError::Rootless -> LibcontainerError::UserNamespace - - ContainerBuilderImpl::rootless -> ContainerBuilderImpl::user_ns_config - - ContainerArgs::rootless -> ContainerArgs::user_ns_config + - rootless (module name) -> user_ns + - Rootless::rootless_id_mapper -> UserNamespaceConfig::id_mapper + - LibcontainerError::Rootless -> LibcontainerError::UserNamespace + - ContainerBuilderImpl::rootless -> ContainerBuilderImpl::user_ns_config + - ContainerArgs::rootless -> ContainerArgs::user_ns_config + +- Changes that will occur for properly running in rootless mode : TODO (@YJDoc2) + +- Executor now contains 2 methods for implementation. We introduce a `validate` step in addition to execute. The `validate` should validate the input OCI spec. The step runs after all the namespaces are entered and rootfs is pivoted. -- Changes that will occur for properly running in rootless mode : TODO (@YJDoc2) \ No newline at end of file +- Executor is now composible instead of an array of executor. To implement multiple executor, create a new executor that runs all the executor. The users are now in control of how multiple executor are run. diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 510133a07..29fe26248 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -70,78 +70,14 @@ pub enum InitProcessError { NotifyListener(#[from] notify_socket::NotifyListenerError), #[error(transparent)] Workload(#[from] workload::ExecutorError), + #[error(transparent)] + WorkloadValidation(#[from] workload::ExecutorValidationError), #[error("invalid io priority class: {0}")] IoPriorityClass(String), } type Result = std::result::Result; -fn get_executable_path(name: &str, path_var: &str) -> Option { - // if path has / in it, we have to assume absolute path, as per runc impl - if name.contains('/') && PathBuf::from(name).exists() { - return Some(PathBuf::from(name)); - } - for path in path_var.split(':') { - let potential_path = PathBuf::from(path).join(name); - if potential_path.exists() { - return Some(potential_path); - } - } - None -} - -fn is_executable(path: &Path) -> std::result::Result { - use std::os::unix::fs::PermissionsExt; - let metadata = path.metadata()?; - let permissions = metadata.permissions(); - // we have to check if the path is file and the execute bit - // is set. In case of directories, the execute bit is also set, - // so have to check if this is a file or not - Ok(metadata.is_file() && permissions.mode() & 0o001 != 0) -} - -// this checks if the binary to run actually exists and if we have -// permissions to run it. Taken from -// https://github.com/opencontainers/runc/blob/25c9e888686773e7e06429133578038a9abc091d/libcontainer/standard_init_linux.go#L195-L206 -fn verify_binary(args: &[String], envs: &[String]) -> Result<()> { - let path_vars: Vec<&String> = envs.iter().filter(|&e| e.starts_with("PATH=")).collect(); - if path_vars.is_empty() { - tracing::error!("PATH environment variable is not set"); - return Err(InitProcessError::InvalidExecutable(args[0].clone())); - } - let path_var = path_vars[0].trim_start_matches("PATH="); - match get_executable_path(&args[0], path_var) { - None => { - tracing::error!( - "executable {} for container process not found in PATH", - args[0] - ); - return Err(InitProcessError::InvalidExecutable(args[0].clone())); - } - Some(path) => match is_executable(&path) { - Ok(true) => { - tracing::debug!("found executable {:?}", path); - } - Ok(false) => { - tracing::error!( - "executable {:?} does not have the correct permission set", - path - ); - return Err(InitProcessError::InvalidExecutable(args[0].clone())); - } - Err(err) => { - tracing::error!( - "failed to check permissions for executable {:?}: {}", - path, - err - ); - return Err(InitProcessError::Io(err)); - } - }, - } - Ok(()) -} - fn sysctl(kernel_params: &HashMap) -> Result<()> { let sys = PathBuf::from("/proc/sys"); for (kernel_param, value) in kernel_params { @@ -637,9 +573,7 @@ pub fn container_init_process( tracing::warn!("seccomp not available, unable to set seccomp privileges!") } - if let Some(args) = proc.args() { - verify_binary(args, &envs)?; - } + args.executor.validate(spec)?; // Notify main process that the init process is ready to execute the // payload. Note, because we are already inside the pid namespace, the pid @@ -1091,44 +1025,6 @@ mod tests { assert_eq!(0, got.len()); } - #[test] - fn test_get_executable_path() { - let non_existing_abs_path = "/some/non/existent/absolute/path"; - let existing_abs_path = "/usr/bin/sh"; - let existing_binary = "sh"; - let non_existing_binary = "non-existent"; - let path_value = "/usr/bin:/bin"; - - assert_eq!( - get_executable_path(existing_abs_path, path_value), - Some(PathBuf::from(existing_abs_path)) - ); - assert_eq!(get_executable_path(non_existing_abs_path, path_value), None); - - assert_eq!( - get_executable_path(existing_binary, path_value), - Some(PathBuf::from("/usr/bin/sh")) - ); - - assert_eq!(get_executable_path(non_existing_binary, path_value), None); - } - - #[test] - fn test_is_executable() { - let tmp = tempfile::tempdir().expect("create temp directory for test"); - let executable_path = PathBuf::from("/bin/sh"); - let directory_path = tmp.path(); - let non_executable_path = directory_path.join("non_executable_file"); - let non_existent_path = PathBuf::from("/some/non/existent/path"); - - std::fs::File::create(non_executable_path.as_path()).unwrap(); - - assert!(is_executable(&non_existent_path).is_err()); - assert!(is_executable(&executable_path).unwrap()); - assert!(!is_executable(&non_executable_path).unwrap()); - assert!(!is_executable(directory_path).unwrap()); - } - #[test] fn test_set_io_priority() { let test_command = TestHelperSyscall::default(); diff --git a/crates/libcontainer/src/workload/default.rs b/crates/libcontainer/src/workload/default.rs index 4f008aa02..28f5ef974 100644 --- a/crates/libcontainer/src/workload/default.rs +++ b/crates/libcontainer/src/workload/default.rs @@ -1,9 +1,12 @@ -use std::ffi::CString; +use std::{ + ffi::CString, + path::{Path, PathBuf}, +}; use nix::unistd; use oci_spec::runtime::Spec; -use super::{Executor, ExecutorError, EMPTY}; +use super::{Executor, ExecutorError, ExecutorValidationError}; #[derive(Clone)] pub struct DefaultExecutor {} @@ -15,12 +18,10 @@ impl Executor for DefaultExecutor { .process() .as_ref() .and_then(|p| p.args().as_ref()) - .unwrap_or(&EMPTY); - - if args.is_empty() { - tracing::error!("no arguments provided to execute"); - Err(ExecutorError::InvalidArg)?; - } + .ok_or_else(|| { + tracing::error!("no arguments provided to execute"); + ExecutorError::InvalidArg + })?; let executable = args[0].as_str(); let cstring_path = CString::new(executable.as_bytes()).map_err(|err| { @@ -40,8 +41,123 @@ impl Executor for DefaultExecutor { // payload through execvp, so it should never reach here. unreachable!(); } + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError> { + let proc = spec + .process() + .as_ref() + .ok_or(ExecutorValidationError::InvalidArg)?; + + if let Some(args) = proc.args() { + let envs: Vec = proc.env().as_ref().unwrap_or(&vec![]).clone(); + let path_vars: Vec<&String> = envs.iter().filter(|&e| e.starts_with("PATH=")).collect(); + if path_vars.is_empty() { + tracing::error!("PATH environment variable is not set"); + Err(ExecutorValidationError::InvalidArg)?; + } + let path_var = path_vars[0].trim_start_matches("PATH="); + match get_executable_path(&args[0], path_var) { + None => { + tracing::error!( + executable = ?args[0], + "executable for container process not found in PATH", + ); + Err(ExecutorValidationError::InvalidArg)?; + } + Some(path) => match is_executable(&path) { + Ok(true) => { + tracing::debug!(executable = ?path, "found executable in executor"); + } + Ok(false) => { + tracing::error!( + executable = ?path, + "executable does not have the correct permission set", + ); + Err(ExecutorValidationError::InvalidArg)?; + } + Err(err) => { + tracing::error!( + executable = ?path, + ?err, + "failed to check permissions for executable", + ); + Err(ExecutorValidationError::InvalidArg)?; + } + }, + } + } + + Ok(()) + } } pub fn get_executor() -> Box { Box::new(DefaultExecutor {}) } + +fn get_executable_path(name: &str, path_var: &str) -> Option { + // if path has / in it, we have to assume absolute path, as per runc impl + if name.contains('/') && PathBuf::from(name).exists() { + return Some(PathBuf::from(name)); + } + for path in path_var.split(':') { + let potential_path = PathBuf::from(path).join(name); + if potential_path.exists() { + return Some(potential_path); + } + } + None +} + +fn is_executable(path: &Path) -> std::result::Result { + use std::os::unix::fs::PermissionsExt; + let metadata = path.metadata()?; + let permissions = metadata.permissions(); + // we have to check if the path is file and the execute bit + // is set. In case of directories, the execute bit is also set, + // so have to check if this is a file or not + Ok(metadata.is_file() && permissions.mode() & 0o001 != 0) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_get_executable_path() { + let non_existing_abs_path = "/some/non/existent/absolute/path"; + let existing_abs_path = "/usr/bin/sh"; + let existing_binary = "sh"; + let non_existing_binary = "non-existent"; + let path_value = "/usr/bin:/bin"; + + assert_eq!( + get_executable_path(existing_abs_path, path_value), + Some(PathBuf::from(existing_abs_path)) + ); + assert_eq!(get_executable_path(non_existing_abs_path, path_value), None); + + assert_eq!( + get_executable_path(existing_binary, path_value), + Some(PathBuf::from("/usr/bin/sh")) + ); + + assert_eq!(get_executable_path(non_existing_binary, path_value), None); + } + + #[test] + fn test_is_executable() { + let tmp = tempfile::tempdir().expect("create temp directory for test"); + let executable_path = PathBuf::from("/bin/sh"); + let directory_path = tmp.path(); + let non_executable_path = directory_path.join("non_executable_file"); + let non_existent_path = PathBuf::from("/some/non/existent/path"); + + std::fs::File::create(non_executable_path.as_path()).unwrap(); + + assert!(is_executable(&non_existent_path).is_err()); + assert!(is_executable(&executable_path).unwrap()); + assert!(!is_executable(&non_executable_path).unwrap()); + assert!(!is_executable(directory_path).unwrap()); + } +} diff --git a/crates/libcontainer/src/workload/mod.rs b/crates/libcontainer/src/workload/mod.rs index 1d9b83aa2..722a3cf73 100644 --- a/crates/libcontainer/src/workload/mod.rs +++ b/crates/libcontainer/src/workload/mod.rs @@ -16,6 +16,14 @@ pub enum ExecutorError { CantHandle(&'static str), } +#[derive(Debug, thiserror::Error)] +pub enum ExecutorValidationError { + #[error("{0} executor can't handle spec")] + CantHandle(&'static str), + #[error("invalid argument")] + InvalidArg, +} + // Here is an explanation about the complexity below regarding to // CloneBoxExecutor and Executor traits. This is one of the places rust actually // makes our life harder. The usecase for the executor is to allow users of @@ -46,6 +54,12 @@ pub trait CloneBoxExecutor { pub trait Executor: CloneBoxExecutor { /// Executes the workload fn exec(&self, spec: &Spec) -> Result<(), ExecutorError>; + + /// Validate if the spec can be executed by the executor. This step runs + /// after the container init process is created, entered into the correct + /// namespace and cgroups, and pivot_root into the rootfs. But this step + /// runs before waiting for the container start signal. + fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError>; } impl CloneBoxExecutor for T diff --git a/crates/youki/src/workload/executor.rs b/crates/youki/src/workload/executor.rs index f68b9c15e..cf27a9ac7 100644 --- a/crates/youki/src/workload/executor.rs +++ b/crates/youki/src/workload/executor.rs @@ -1,5 +1,5 @@ use libcontainer::oci_spec::runtime::Spec; -use libcontainer::workload::{Executor, ExecutorError}; +use libcontainer::workload::{Executor, ExecutorError, ExecutorValidationError}; #[derive(Clone)] pub struct DefaultExecutor {} @@ -29,6 +29,29 @@ impl Executor for DefaultExecutor { // container workloads. libcontainer::workload::default::get_executor().exec(spec) } + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError> { + #[cfg(feature = "wasm-wasmer")] + match super::wasmer::get_executor().validate(spec) { + Ok(_) => return Ok(()), + Err(ExecutorValidationError::CantHandle(_)) => (), + Err(err) => return Err(err), + } + #[cfg(feature = "wasm-wasmedge")] + match super::wasmedge::get_executor().validate(spec) { + Ok(_) => return Ok(()), + Err(ExecutorValidationError::CantHandle(_)) => (), + Err(err) => return Err(err), + } + #[cfg(feature = "wasm-wasmtime")] + match super::wasmtime::get_executor().validate(spec) { + Ok(_) => return Ok(()), + Err(ExecutorValidationError::CantHandle(_)) => (), + Err(err) => return Err(err), + } + + libcontainer::workload::default::get_executor().validate(spec) + } } pub fn default_executor() -> DefaultExecutor { diff --git a/crates/youki/src/workload/wasmedge.rs b/crates/youki/src/workload/wasmedge.rs index 7ac6ef6c4..4a6aa6111 100644 --- a/crates/youki/src/workload/wasmedge.rs +++ b/crates/youki/src/workload/wasmedge.rs @@ -4,7 +4,7 @@ use wasmedge_sdk::{ params, VmBuilder, }; -use libcontainer::workload::{Executor, ExecutorError}; +use libcontainer::workload::{Executor, ExecutorError, ExecutorValidationError}; const EXECUTOR_NAME: &str = "wasmedge"; @@ -62,6 +62,14 @@ impl Executor for WasmedgeExecutor { Ok(()) } + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError> { + if !can_handle(spec) { + return Err(ExecutorValidationError::CantHandle(EXECUTOR_NAME)); + } + + Ok(()) + } } pub fn get_executor() -> WasmedgeExecutor { diff --git a/crates/youki/src/workload/wasmer.rs b/crates/youki/src/workload/wasmer.rs index 95d98c6dd..90a1b760c 100644 --- a/crates/youki/src/workload/wasmer.rs +++ b/crates/youki/src/workload/wasmer.rs @@ -2,7 +2,7 @@ use libcontainer::oci_spec::runtime::Spec; use wasmer::{Instance, Module, Store}; use wasmer_wasix::WasiEnv; -use libcontainer::workload::{Executor, ExecutorError, EMPTY}; +use libcontainer::workload::{Executor, ExecutorError, ExecutorValidationError, EMPTY}; const EXECUTOR_NAME: &str = "wasmer"; @@ -80,6 +80,14 @@ impl Executor for WasmerExecutor { Ok(()) } + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError> { + if !can_handle(spec) { + return Err(ExecutorValidationError::CantHandle(EXECUTOR_NAME)); + } + + Ok(()) + } } pub fn get_executor() -> WasmerExecutor { diff --git a/crates/youki/src/workload/wasmtime.rs b/crates/youki/src/workload/wasmtime.rs index 58f977b21..08d7a415e 100644 --- a/crates/youki/src/workload/wasmtime.rs +++ b/crates/youki/src/workload/wasmtime.rs @@ -2,7 +2,7 @@ use libcontainer::oci_spec::runtime::Spec; use wasmtime::*; use wasmtime_wasi::WasiCtxBuilder; -use libcontainer::workload::{Executor, ExecutorError, EMPTY}; +use libcontainer::workload::{Executor, ExecutorError, ExecutorValidationError, EMPTY}; const EXECUTOR_NAME: &str = "wasmtime"; @@ -90,6 +90,14 @@ impl Executor for WasmtimeExecutor { .call(&mut store, &[], &mut []) .map_err(|err| ExecutorError::Execution(err.into())) } + + fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError> { + if !can_handle(spec) { + return Err(ExecutorValidationError::CantHandle(EXECUTOR_NAME)); + } + + Ok(()) + } } pub fn get_executor() -> WasmtimeExecutor {