From 763ceeb7ba278f6fc2886e85fb296c5de6babdfb Mon Sep 17 00:00:00 2001 From: Derek Lee Date: Wed, 3 Aug 2022 16:28:12 -0700 Subject: [PATCH] logging: Replace nix::Error::EINVAL with more descriptive msgs Replaces instances of anyhow!(nix::Error::EINVAL) with other messages to make it easier to debug. Fixes #954 Signed-off-by: Derek Lee --- src/agent/rustjail/src/cgroups/fs/mod.rs | 2 +- src/agent/rustjail/src/container.rs | 25 ++++----- src/agent/rustjail/src/mount.rs | 67 +++++++++++++++++++++--- src/agent/rustjail/src/validator.rs | 60 ++++++++++++--------- src/agent/src/netlink.rs | 9 +++- src/agent/src/rpc.rs | 10 ++-- 6 files changed, 121 insertions(+), 52 deletions(-) diff --git a/src/agent/rustjail/src/cgroups/fs/mod.rs b/src/agent/rustjail/src/cgroups/fs/mod.rs index 70168d2f0b..b24d6b3276 100644 --- a/src/agent/rustjail/src/cgroups/fs/mod.rs +++ b/src/agent/rustjail/src/cgroups/fs/mod.rs @@ -174,7 +174,7 @@ impl CgroupManager for Manager { freezer_controller.freeze()?; } _ => { - return Err(anyhow!(nix::Error::EINVAL)); + return Err(anyhow!("Invalid FreezerState")); } } diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 2c360cf164..174c269824 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -106,6 +106,11 @@ impl Default for ContainerStatus { } } +// We might want to change this to thiserror in the future +const MissingCGroupManager: &str = "failed to get container's cgroup Manager"; +const MissingLinux: &str = "no linux config"; +const InvalidNamespace: &str = "invalid namespace type"; + pub type Config = CreateOpts; type NamespaceType = String; @@ -292,7 +297,7 @@ impl Container for LinuxContainer { self.status.transition(ContainerState::Paused); return Ok(()); } - Err(anyhow!("failed to get container's cgroup manager")) + Err(anyhow!(MissingCGroupManager)) } fn resume(&mut self) -> Result<()> { @@ -310,7 +315,7 @@ impl Container for LinuxContainer { self.status.transition(ContainerState::Running); return Ok(()); } - Err(anyhow!("failed to get container's cgroup manager")) + Err(anyhow!(MissingCGroupManager)) } } @@ -397,7 +402,7 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { }; if spec.linux.is_none() { - return Err(anyhow!("no linux config")); + return Err(anyhow!(MissingLinux)); } let linux = spec.linux.as_ref().unwrap(); @@ -411,7 +416,7 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { for ns in &nses { let s = NAMESPACES.get(&ns.r#type.as_str()); if s.is_none() { - return Err(anyhow!("invalid ns type")); + return Err(anyhow!(InvalidNamespace)); } let s = s.unwrap(); @@ -1437,18 +1442,10 @@ impl LinuxContainer { Some(unistd::getuid()), Some(unistd::getgid()), ) - .context(format!("cannot change onwer of container {} root", id))?; - - if config.spec.is_none() { - return Err(anyhow!(nix::Error::EINVAL)); - } + .context(format!("Cannot change owner of container {} root", id))?; let spec = config.spec.as_ref().unwrap(); - if spec.linux.is_none() { - return Err(anyhow!(nix::Error::EINVAL)); - } - let linux = spec.linux.as_ref().unwrap(); let cpath = if linux.cgroups_path.is_empty() { @@ -1525,7 +1522,7 @@ pub async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<() let binary = PathBuf::from(h.path.as_str()); let path = binary.canonicalize()?; if !path.exists() { - return Err(anyhow!(nix::Error::EINVAL)); + return Err(anyhow!("Path {:?} does not exist", path)); } let mut args = h.args.clone(); diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index dd980530d0..cdd2a79003 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -1020,9 +1020,7 @@ pub fn finish_rootfs(cfd_log: RawFd, spec: &Spec, process: &Process) -> Result<( } fn mask_path(path: &str) -> Result<()> { - if !path.starts_with('/') || path.contains("..") { - return Err(anyhow!(nix::Error::EINVAL)); - } + check_paths(path)?; match mount( Some("/dev/null"), @@ -1040,9 +1038,7 @@ fn mask_path(path: &str) -> Result<()> { } fn readonly_path(path: &str) -> Result<()> { - if !path.starts_with('/') || path.contains("..") { - return Err(anyhow!(nix::Error::EINVAL)); - } + check_paths(path)?; if let Err(e) = mount( Some(&path[1..]), @@ -1068,6 +1064,16 @@ fn readonly_path(path: &str) -> Result<()> { Ok(()) } +fn check_paths(path: &str) -> Result<()> { + if !path.starts_with('/') || path.contains("..") { + return Err(anyhow!( + "Cannot mount {} (path does not start with '/' or contains '..').", + path + )); + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -1420,6 +1426,55 @@ mod tests { } } + #[test] + fn test_check_paths() { + #[derive(Debug)] + struct TestData<'a> { + name: &'a str, + path: &'a str, + result: Result<()>, + } + + let tests = &[ + TestData { + name: "valid path", + path: "/foo/bar", + result: Ok(()), + }, + TestData { + name: "does not starts with /", + path: "foo/bar", + result: Err(anyhow!( + "Cannot mount foo/bar (path does not start with '/' or contains '..')." + )), + }, + TestData { + name: "contains ..", + path: "../foo/bar", + result: Err(anyhow!( + "Cannot mount ../foo/bar (path does not start with '/' or contains '..')." + )), + }, + ]; + + for (i, d) in tests.iter().enumerate() { + let msg = format!("test[{}]: {:?}", i, d.name); + + let result = check_paths(d.path); + + let msg = format!("{}: result: {:?}", msg, result); + + if d.result.is_ok() { + assert!(result.is_ok()); + continue; + } + + let expected_error = format!("{}", d.result.as_ref().unwrap_err()); + let actual_error = format!("{}", result.unwrap_err()); + assert!(actual_error == expected_error, "{}", msg); + } + } + #[test] fn test_check_proc_mount() { let mount = oci::Mount { diff --git a/src/agent/rustjail/src/validator.rs b/src/agent/rustjail/src/validator.rs index 3b5aeb3619..aea0f8f063 100644 --- a/src/agent/rustjail/src/validator.rs +++ b/src/agent/rustjail/src/validator.rs @@ -4,17 +4,15 @@ // use crate::container::Config; -use anyhow::{anyhow, Context, Error, Result}; +use anyhow::{anyhow, Context, Result}; use oci::{Linux, LinuxIdMapping, LinuxNamespace, Spec}; use std::collections::HashMap; use std::path::{Component, PathBuf}; -fn einval() -> Error { - anyhow!(nix::Error::EINVAL) -} - fn get_linux(oci: &Spec) -> Result<&Linux> { - oci.linux.as_ref().ok_or_else(einval) + oci.linux + .as_ref() + .ok_or_else(|| anyhow!("Unable to get Linux section from Spec")) } fn contain_namespace(nses: &[LinuxNamespace], key: &str) -> bool { @@ -31,7 +29,10 @@ fn rootfs(root: &str) -> Result<()> { let path = PathBuf::from(root); // not absolute path or not exists if !path.exists() || !path.is_absolute() { - return Err(einval()); + return Err(anyhow!( + "Path from {:?} does not exist or is not absolute", + root + )); } // symbolic link? ..? @@ -49,7 +50,7 @@ fn rootfs(root: &str) -> Result<()> { if let Some(v) = c.as_os_str().to_str() { stack.push(v.to_string()); } else { - return Err(einval()); + return Err(anyhow!("Invalid path component (unable to convert to str)")); } } @@ -58,10 +59,13 @@ fn rootfs(root: &str) -> Result<()> { cleaned.push(e); } - let canon = path.canonicalize().context("canonicalize")?; + let canon = path.canonicalize().context("failed to canonicalize path")?; if cleaned != canon { // There is symbolic in path - return Err(einval()); + return Err(anyhow!( + "There may be illegal symbols in the path name. Cleaned ({:?}) and canonicalized ({:?}) paths do not match", + cleaned, + canon)); } Ok(()) @@ -74,7 +78,7 @@ fn hostname(oci: &Spec) -> Result<()> { let linux = get_linux(oci)?; if !contain_namespace(&linux.namespaces, "uts") { - return Err(einval()); + return Err(anyhow!("Linux namespace does not contain uts")); } Ok(()) @@ -88,7 +92,7 @@ fn security(oci: &Spec) -> Result<()> { } if !contain_namespace(&linux.namespaces, "mount") { - return Err(einval()); + return Err(anyhow!("Linux namespace does not contain mount")); } // don't care about selinux at present @@ -103,7 +107,7 @@ fn idmapping(maps: &[LinuxIdMapping]) -> Result<()> { } } - Err(einval()) + Err(anyhow!("No idmap has size > 0")) } fn usernamespace(oci: &Spec) -> Result<()> { @@ -121,7 +125,7 @@ fn usernamespace(oci: &Spec) -> Result<()> { } else { // no user namespace but idmap if !linux.uid_mappings.is_empty() || !linux.gid_mappings.is_empty() { - return Err(einval()); + return Err(anyhow!("No user namespace, but uid or gid mapping exists")); } } @@ -163,7 +167,7 @@ fn sysctl(oci: &Spec) -> Result<()> { if contain_namespace(&linux.namespaces, "ipc") { continue; } else { - return Err(einval()); + return Err(anyhow!("Linux namespace does not contain ipc")); } } @@ -178,11 +182,11 @@ fn sysctl(oci: &Spec) -> Result<()> { } if key == "kernel.hostname" { - return Err(einval()); + return Err(anyhow!("Kernel hostname specfied in Spec")); } } - return Err(einval()); + return Err(anyhow!("Sysctl config contains invalid settings")); } Ok(()) } @@ -191,12 +195,13 @@ fn rootless_euid_mapping(oci: &Spec) -> Result<()> { let linux = get_linux(oci)?; if !contain_namespace(&linux.namespaces, "user") { - return Err(einval()); + return Err(anyhow!("Linux namespace is missing user")); } if linux.uid_mappings.is_empty() || linux.gid_mappings.is_empty() { - // rootless containers requires at least one UID/GID mapping - return Err(einval()); + return Err(anyhow!( + "Rootless containers require at least one UID/GID mapping" + )); } Ok(()) @@ -220,7 +225,7 @@ fn rootless_euid_mount(oci: &Spec) -> Result<()> { let fields: Vec<&str> = opt.split('=').collect(); if fields.len() != 2 { - return Err(einval()); + return Err(anyhow!("Options has invalid field: {:?}", fields)); } let id = fields[1] @@ -229,11 +234,11 @@ fn rootless_euid_mount(oci: &Spec) -> Result<()> { .context(format!("parse field {}", &fields[1]))?; if opt.starts_with("uid=") && !has_idmapping(&linux.uid_mappings, id) { - return Err(einval()); + return Err(anyhow!("uid of {} does not have a valid mapping", id)); } if opt.starts_with("gid=") && !has_idmapping(&linux.gid_mappings, id) { - return Err(einval()); + return Err(anyhow!("gid of {} does not have a valid mapping", id)); } } } @@ -249,15 +254,18 @@ fn rootless_euid(oci: &Spec) -> Result<()> { pub fn validate(conf: &Config) -> Result<()> { lazy_static::initialize(&SYSCTLS); - let oci = conf.spec.as_ref().ok_or_else(einval)?; + let oci = conf + .spec + .as_ref() + .ok_or_else(|| anyhow!("Invalid config spec"))?; if oci.linux.is_none() { - return Err(einval()); + return Err(anyhow!("oci Linux is none")); } let root = match oci.root.as_ref() { Some(v) => v.path.as_str(), - None => return Err(einval()), + None => return Err(anyhow!("oci root is none")), }; rootfs(root).context("rootfs")?; diff --git a/src/agent/src/netlink.rs b/src/agent/src/netlink.rs index c6fc9c2079..a95de5eb4b 100644 --- a/src/agent/src/netlink.rs +++ b/src/agent/src/netlink.rs @@ -523,7 +523,7 @@ impl Handle { .as_ref() .map(|to| to.address.as_str()) // Extract address field .and_then(|addr| if addr.is_empty() { None } else { Some(addr) }) // Make sure it's not empty - .ok_or_else(|| anyhow!(nix::Error::EINVAL))?; + .ok_or_else(|| anyhow!("Unable to determine ip address of ARP neighbor"))?; let ip = IpAddr::from_str(ip_address) .map_err(|e| anyhow!("Failed to parse IP {}: {:?}", ip_address, e))?; @@ -612,7 +612,12 @@ fn parse_mac_address(addr: &str) -> Result<[u8; 6]> { // Parse single Mac address block let mut parse_next = || -> Result { - let v = u8::from_str_radix(split.next().ok_or_else(|| anyhow!(nix::Error::EINVAL))?, 16)?; + let v = u8::from_str_radix( + split + .next() + .ok_or_else(|| anyhow!("Invalid MAC address {}", addr))?, + 16, + )?; Ok(v) }; diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index bef693fe23..89856dd649 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -380,7 +380,7 @@ impl AgentService { let mut process = req .process .into_option() - .ok_or_else(|| anyhow!(nix::Error::EINVAL))?; + .ok_or_else(|| anyhow!("Unable to parse process from ExecProcessRequest"))?; // Apply any necessary corrections for PCI addresses update_env_pci(&mut process.Env, &sandbox.pcimap)?; @@ -629,7 +629,7 @@ impl AgentService { }; if reader.is_none() { - return Err(anyhow!(nix::Error::EINVAL)); + return Err(anyhow!("Unable to determine stream reader, is None")); } let reader = reader.ok_or_else(|| anyhow!("cannot get stream reader"))?; @@ -1843,7 +1843,11 @@ fn do_copy_file(req: &CopyFileRequest) -> Result<()> { let path = PathBuf::from(req.path.as_str()); if !path.starts_with(CONTAINER_BASE) { - return Err(anyhow!(nix::Error::EINVAL)); + return Err(anyhow!( + "Path {:?} does not start with {}", + path, + CONTAINER_BASE + )); } let parent = path.parent();