Merge pull request #4815 from bookinabox/improve-agent-errors

logging: Replace nix::Error::EINVAL with more descriptive msgs

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"));
             }
         }
 

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() {
@@ -1530,7 +1527,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();

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 {

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")?;

src/agent/src/netlink.rs

@@ -522,7 +522,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))?;
@@ -566,7 +566,12 @@ fn parse_mac_address(addr: &str) -> Result<[u8; 6]> {
 
     // Parse single Mac address block
     let mut parse_next = || -> Result<u8> {
-        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)
     };
 

src/agent/src/rpc.rs

@@ -348,7 +348,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)?;
@@ -597,7 +597,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"))?;
@@ -1839,7 +1839,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();