Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move the validation logic into executor #2258

Merged
merged 4 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions MirgationGuide.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
# Migration Guide

This contains information for migrating library versions.

## V0.1.0 -> v0.2.0

### 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)
- 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.
110 changes: 3 additions & 107 deletions crates/libcontainer/src/process/container_init_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> = std::result::Result<T, InitProcessError>;

fn get_executable_path(name: &str, path_var: &str) -> Option<PathBuf> {
// 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<bool, std::io::Error> {
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<String, String>) -> Result<()> {
let sys = PathBuf::from("/proc/sys");
for (kernel_param, value) in kernel_params {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down
132 changes: 124 additions & 8 deletions crates/libcontainer/src/workload/default.rs
Original file line number Diff line number Diff line change
@@ -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 {}
Expand All @@ -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| {
Expand All @@ -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<String> = 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<dyn Executor> {
Box::new(DefaultExecutor {})
}

fn get_executable_path(name: &str, path_var: &str) -> Option<PathBuf> {
// 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(':') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this PR just moved code but I want to raise the concern where @jprendes raised here: containerd/runwasi#156 (comment)

The issue is that the is_executable check should be inside of the for loop, not outside. @jprendes provides an example of illustrating why that's the case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mossaka I have created the issue to address what you mentioned #2277. I think this issue is outside the scope of this PR so we should take care of it in another PR, but I appreciate you pointed out, thanks.

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<bool, std::io::Error> {
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());
}
}
14 changes: 14 additions & 0 deletions crates/libcontainer/src/workload/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<T> CloneBoxExecutor for T
Expand Down
25 changes: 24 additions & 1 deletion crates/youki/src/workload/executor.rs
Original file line number Diff line number Diff line change
@@ -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 {}
Expand Down Expand Up @@ -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 {
Expand Down
Loading