From d8f39fb2690fb40afe1a40571213d31f019b8f29 Mon Sep 17 00:00:00 2001 From: "Eduardo Lima (Etrunko)" Date: Fri, 18 Mar 2022 10:21:48 -0300 Subject: [PATCH 01/22] agent/random: Rename RNDRESEEDRNG to RNDRESEEDCRNG Make this definition match the one in kernel: https://github.com/torvalds/linux/blob/5bfc75d92efd494db37f5c4c173d3639d4772966/include/uapi/linux/random.h#L38-L39 Signed-off-by: Eduardo Lima (Etrunko) --- src/agent/src/random.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/agent/src/random.rs b/src/agent/src/random.rs index 4713134a8..ec64e89c9 100644 --- a/src/agent/src/random.rs +++ b/src/agent/src/random.rs @@ -13,7 +13,7 @@ use tracing::instrument; pub const RNGDEV: &str = "/dev/random"; pub const RNDADDTOENTCNT: libc::c_int = 0x40045201; -pub const RNDRESEEDRNG: libc::c_int = 0x5207; +pub const RNDRESEEDCRNG: libc::c_int = 0x5207; // Handle the differing ioctl(2) request types for different targets #[cfg(target_env = "musl")] @@ -41,7 +41,7 @@ pub fn reseed_rng(data: &[u8]) -> Result<()> { }; Errno::result(ret).map(drop)?; - let ret = unsafe { libc::ioctl(f.as_raw_fd(), RNDRESEEDRNG as IoctlRequestType, 0) }; + let ret = unsafe { libc::ioctl(f.as_raw_fd(), RNDRESEEDCRNG as IoctlRequestType, 0) }; Errno::result(ret).map(drop)?; Ok(()) From 39a35b693a628829de103f1a51af10facc34e363 Mon Sep 17 00:00:00 2001 From: Wainer dos Santos Moschetta Date: Thu, 8 Apr 2021 14:34:35 -0400 Subject: [PATCH 02/22] agent: Add test to random::reseed_rng() Introduced an unit test for the random::reseed_rng() function. Fixes #291 Signed-off-by: Wainer dos Santos Moschetta --- src/agent/src/random.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/agent/src/random.rs b/src/agent/src/random.rs index ec64e89c9..4e7005a35 100644 --- a/src/agent/src/random.rs +++ b/src/agent/src/random.rs @@ -46,3 +46,24 @@ pub fn reseed_rng(data: &[u8]) -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::skip_if_not_root; + use std::fs::File; + use std::io::prelude::*; + + #[test] + fn test_reseed_rng() { + skip_if_not_root!(); + const POOL_SIZE: usize = 512; + let mut f = File::open("/dev/urandom").unwrap(); + let mut seed = [0; POOL_SIZE]; + let n = f.read(&mut seed).unwrap(); + // Ensure the buffer was filled. + assert!(n == POOL_SIZE); + let ret = reseed_rng(&seed); + assert!(ret.is_ok()); + } +} From 33c953ace4cfeb1b7670054ffb3926fddeb70f8e Mon Sep 17 00:00:00 2001 From: "Eduardo Lima (Etrunko)" Date: Fri, 18 Mar 2022 10:22:33 -0300 Subject: [PATCH 03/22] agent: Add test_ressed_rng_not_root Same as previous test, but does not skip if it is not running as root. Signed-off-by: Eduardo Lima (Etrunko) --- src/agent/src/random.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/agent/src/random.rs b/src/agent/src/random.rs index 4e7005a35..79ee25d96 100644 --- a/src/agent/src/random.rs +++ b/src/agent/src/random.rs @@ -66,4 +66,20 @@ mod tests { let ret = reseed_rng(&seed); assert!(ret.is_ok()); } + + #[test] + fn test_reseed_rng_not_root() { + const POOL_SIZE: usize = 512; + let mut f = File::open("/dev/urandom").unwrap(); + let mut seed = [0; POOL_SIZE]; + let n = f.read(&mut seed).unwrap(); + // Ensure the buffer was filled. + assert!(n == POOL_SIZE); + let ret = reseed_rng(&seed); + if nix::unistd::Uid::effective().is_root() { + assert!(ret.is_ok()); + } else { + assert!(!ret.is_ok()); + } + } } From 1cad3a4696058d1119c54c9a23768dfac330363a Mon Sep 17 00:00:00 2001 From: "Eduardo Lima (Etrunko)" Date: Fri, 18 Mar 2022 11:39:49 -0300 Subject: [PATCH 04/22] agent/random: Ensure data.len > 0 Also adds a test to cover this scenario Signed-off-by: Eduardo Lima (Etrunko) --- src/agent/src/random.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/agent/src/random.rs b/src/agent/src/random.rs index 79ee25d96..c2506ac24 100644 --- a/src/agent/src/random.rs +++ b/src/agent/src/random.rs @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 // -use anyhow::Result; +use anyhow::{ensure, Result}; use nix::errno::Errno; use nix::fcntl::{self, OFlag}; use nix::sys::stat::Mode; @@ -24,6 +24,9 @@ type IoctlRequestType = libc::c_ulong; #[instrument] pub fn reseed_rng(data: &[u8]) -> Result<()> { let len = data.len() as libc::c_long; + + ensure!(len > 0, "missing entropy data"); + fs::write(RNGDEV, data)?; let f = { @@ -82,4 +85,11 @@ mod tests { assert!(!ret.is_ok()); } } + + #[test] + fn test_reseed_rng_zero_data() { + let seed = []; + let ret = reseed_rng(&seed); + assert!(!ret.is_ok()); + } } From 0e7f1a5e3ae5b1c66b02933f08aacab573bab7d3 Mon Sep 17 00:00:00 2001 From: Braden Rayhorn Date: Fri, 15 Apr 2022 17:19:43 -0500 Subject: [PATCH 05/22] agent: move assert_result macro to test_utils file Move the assert_result macro to the shared test_utils file so that it is not duplicated in individual files. Fixes: #4093 Signed-off-by: Braden Rayhorn --- src/agent/src/config.rs | 28 ++-------------------------- src/agent/src/rpc.rs | 28 +--------------------------- src/agent/src/test_utils.rs | 25 +++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 53 deletions(-) diff --git a/src/agent/src/config.rs b/src/agent/src/config.rs index 759b696c4..0bdf20c2d 100644 --- a/src/agent/src/config.rs +++ b/src/agent/src/config.rs @@ -432,6 +432,8 @@ fn get_container_pipe_size(param: &str) -> Result { #[cfg(test)] mod tests { + use crate::assert_result; + use super::*; use anyhow::anyhow; use std::fs::File; @@ -439,32 +441,6 @@ mod tests { use std::time; use tempfile::tempdir; - // Parameters: - // - // 1: expected Result - // 2: actual Result - // 3: string used to identify the test on error - macro_rules! assert_result { - ($expected_result:expr, $actual_result:expr, $msg:expr) => { - if $expected_result.is_ok() { - let expected_level = $expected_result.as_ref().unwrap(); - let actual_level = $actual_result.unwrap(); - assert!(*expected_level == actual_level, "{}", $msg); - } else { - let expected_error = $expected_result.as_ref().unwrap_err(); - let expected_error_msg = format!("{:?}", expected_error); - - if let Err(actual_error) = $actual_result { - let actual_error_msg = format!("{:?}", actual_error); - - assert!(expected_error_msg == actual_error_msg, "{}", $msg); - } else { - assert!(expected_error_msg == "expected error, got OK", "{}", $msg); - } - } - }; - } - #[test] fn test_new() { let config: AgentConfig = Default::default(); diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index f100e0c43..94e3879b9 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -1894,37 +1894,11 @@ fn load_kernel_module(module: &protocols::agent::KernelModule) -> Result<()> { #[cfg(test)] mod tests { use super::*; - use crate::{namespace::Namespace, protocols::agent_ttrpc::AgentService as _}; + use crate::{assert_result, namespace::Namespace, protocols::agent_ttrpc::AgentService as _}; use oci::{Hook, Hooks, Linux, LinuxNamespace}; use tempfile::{tempdir, TempDir}; use ttrpc::{r#async::TtrpcContext, MessageHeader}; - // Parameters: - // - // 1: expected Result - // 2: actual Result - // 3: string used to identify the test on error - macro_rules! assert_result { - ($expected_result:expr, $actual_result:expr, $msg:expr) => { - if $expected_result.is_ok() { - let expected_level = $expected_result.as_ref().unwrap(); - let actual_level = $actual_result.unwrap(); - assert!(*expected_level == actual_level, "{}", $msg); - } else { - let expected_error = $expected_result.as_ref().unwrap_err(); - let expected_error_msg = format!("{:?}", expected_error); - - if let Err(actual_error) = $actual_result { - let actual_error_msg = format!("{:?}", actual_error); - - assert!(expected_error_msg == actual_error_msg, "{}", $msg); - } else { - assert!(expected_error_msg == "expected error, got OK", "{}", $msg); - } - } - }; - } - fn mk_ttrpc_context() -> TtrpcContext { TtrpcContext { fd: -1, diff --git a/src/agent/src/test_utils.rs b/src/agent/src/test_utils.rs index f8f2d7ab3..becb845fa 100644 --- a/src/agent/src/test_utils.rs +++ b/src/agent/src/test_utils.rs @@ -53,4 +53,29 @@ mod test_utils { } }; } + + // Parameters: + // + // 1: expected Result + // 2: actual Result + // 3: string used to identify the test on error + #[macro_export] + macro_rules! assert_result { + ($expected_result:expr, $actual_result:expr, $msg:expr) => { + if $expected_result.is_ok() { + let expected_value = $expected_result.as_ref().unwrap(); + let actual_value = $actual_result.unwrap(); + assert!(*expected_value == actual_value, "{}", $msg); + } else { + assert!($actual_result.is_err(), "{}", $msg); + + let expected_error = $expected_result.as_ref().unwrap_err(); + let expected_error_msg = format!("{:?}", expected_error); + + let actual_error_msg = format!("{:?}", $actual_result.unwrap_err()); + + assert!(expected_error_msg == actual_error_msg, "{}", $msg); + } + }; + } } From f385b21b05fd85cef7d0f372a1a75aee39067cb2 Mon Sep 17 00:00:00 2001 From: Braden Rayhorn Date: Wed, 20 Apr 2022 17:54:08 -0500 Subject: [PATCH 06/22] rustjail: add tests for mount_from function Add tests for the mount_from function in rustjail mount.rs file. Fixes: #4121 Signed-off-by: Braden Rayhorn --- src/agent/rustjail/src/mount.rs | 107 ++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index e8ddb3a6b..3c6d60695 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -1286,6 +1286,113 @@ mod tests { let ret = stat::stat(path); assert!(ret.is_ok(), "Should pass. Got: {:?}", ret); } + + #[test] + fn test_mount_from() { + #[derive(Debug)] + struct TestData<'a> { + source: &'a str, + destination: &'a str, + r#type: &'a str, + flags: MsFlags, + error_contains: &'a str, + + // if true, a directory will be created at path in source + make_source_directory: bool, + // if true, a file will be created at path in source + make_source_file: bool, + } + + impl Default for TestData<'_> { + fn default() -> Self { + TestData { + source: "tmp", + destination: "dest", + r#type: "tmpfs", + flags: MsFlags::empty(), + error_contains: "", + make_source_directory: true, + make_source_file: false, + } + } + } + + let tests = &[ + TestData { + ..Default::default() + }, + TestData { + flags: MsFlags::MS_BIND, + ..Default::default() + }, + TestData { + r#type: "bind", + ..Default::default() + }, + TestData { + r#type: "cgroup2", + ..Default::default() + }, + TestData { + r#type: "bind", + make_source_directory: false, + error_contains: &format!("{}", std::io::Error::from_raw_os_error(libc::ENOENT)), + ..Default::default() + }, + TestData { + r#type: "bind", + make_source_directory: false, + make_source_file: true, + ..Default::default() + }, + ]; + + for (i, d) in tests.iter().enumerate() { + let msg = format!("test[{}]: {:?}", i, d); + let tempdir = tempdir().unwrap(); + + let (rfd, wfd) = unistd::pipe2(OFlag::O_CLOEXEC).unwrap(); + defer!({ + unistd::close(rfd).unwrap(); + unistd::close(wfd).unwrap(); + }); + + let source_path = tempdir.path().join(d.source).to_str().unwrap().to_string(); + if d.make_source_directory { + std::fs::create_dir_all(&source_path).unwrap(); + } else if d.make_source_file { + std::fs::write(&source_path, []).unwrap(); + } + + let mount = Mount { + source: source_path, + destination: d.destination.to_string(), + r#type: d.r#type.to_string(), + options: vec![], + }; + + let result = mount_from( + wfd, + &mount, + tempdir.path().to_str().unwrap(), + d.flags, + "", + "", + ); + + let msg = format!("{}: result: {:?}", msg, result); + + if d.error_contains.is_empty() { + assert!(result.is_ok(), "{}", msg); + } else { + assert!(result.is_err(), "{}", msg); + + let error_msg = format!("{}", result.unwrap_err()); + assert!(error_msg.contains(d.error_contains), "{}", msg); + } + } + } + #[test] fn test_check_proc_mount() { let mount = oci::Mount { From b63774ec61ac57100f196cdba6e82e202b5d2722 Mon Sep 17 00:00:00 2001 From: Liu Jiang Date: Mon, 10 Jan 2022 18:06:19 +0800 Subject: [PATCH 07/22] libs/safe-path: add crate to safely resolve fs paths There are always path(symlink) based attacks, so the `safe-path` crate tries to provde some mechanisms to harden path resolution related code. Fixes: #3451 Signed-off-by: Liu Jiang --- README.md | 1 + src/libs/{logging => }/Cargo.lock | 108 ++--- src/libs/Cargo.toml | 6 + src/libs/README.md | 10 + src/libs/safe-path/Cargo.toml | 18 + src/libs/safe-path/README.md | 21 + src/libs/safe-path/src/lib.rs | 65 +++ src/libs/safe-path/src/pinned_path_buf.rs | 400 ++++++++++++++++++ src/libs/safe-path/src/scoped_dir_builder.rs | 261 ++++++++++++ .../safe-path/src/scoped_path_resolver.rs | 324 ++++++++++++++ 10 files changed, 1144 insertions(+), 70 deletions(-) rename src/libs/{logging => }/Cargo.lock (76%) create mode 100644 src/libs/Cargo.toml create mode 100644 src/libs/README.md create mode 100644 src/libs/safe-path/Cargo.toml create mode 100644 src/libs/safe-path/README.md create mode 100644 src/libs/safe-path/src/lib.rs create mode 100644 src/libs/safe-path/src/pinned_path_buf.rs create mode 100644 src/libs/safe-path/src/scoped_dir_builder.rs create mode 100644 src/libs/safe-path/src/scoped_path_resolver.rs diff --git a/README.md b/README.md index 0d8c5d2b8..431d687de 100644 --- a/README.md +++ b/README.md @@ -118,6 +118,7 @@ The table below lists the core parts of the project: | [runtime](src/runtime) | core | Main component run by a container manager and providing a containerd shimv2 runtime implementation. | | [agent](src/agent) | core | Management process running inside the virtual machine / POD that sets up the container environment. | | [documentation](docs) | documentation | Documentation common to all components (such as design and install documentation). | +| [libraries](src/libs) | core | Library crates shared by multiple Kata Container components or published to [`crates.io`](https://crates.io/index.html) | | [tests](https://github.com/kata-containers/tests) | tests | Excludes unit tests which live with the main code. | ### Additional components diff --git a/src/libs/logging/Cargo.lock b/src/libs/Cargo.lock similarity index 76% rename from src/libs/logging/Cargo.lock rename to src/libs/Cargo.lock index 78f8feef6..ad9d8672c 100644 --- a/src/libs/logging/Cargo.lock +++ b/src/libs/Cargo.lock @@ -1,7 +1,5 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 - [[package]] name = "arc-swap" version = "1.5.0" @@ -41,9 +39,9 @@ dependencies = [ [[package]] name = "crossbeam-channel" -version = "0.5.1" +version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "06ed27e177f16d65f0f0c22a213e17c696ace5dd64b14258b52f9417ccb52db4" +checksum = "e54ea8bc3fb1ee042f5aace6e3c6e025d3874866da222930f70ce62aceba0bfa" dependencies = [ "cfg-if", "crossbeam-utils", @@ -51,23 +49,30 @@ dependencies = [ [[package]] name = "crossbeam-utils" -version = "0.8.5" +version = "0.8.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d82cfc11ce7f2c3faef78d8a684447b40d503d9681acebed6cb728d45940c4db" +checksum = "cfcae03edb34f947e64acdb1c33ec169824e20657e9ecb61cef6c8c74dcb8120" dependencies = [ "cfg-if", "lazy_static", ] [[package]] -name = "getrandom" -version = "0.2.3" +name = "fastrand" +version = "1.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7fcd999463524c52659517fe2cea98493cfe485d10565e7b0fb07dbba7ad2753" +checksum = "779d043b6a0b90cc4c0ed7ee380a6504394cee7efd7db050e3774eee387324b2" +dependencies = [ + "instant", +] + +[[package]] +name = "instant" +version = "0.1.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7a5bbe824c507c5da5956355e86a746d82e0e1464f65d862cc5e71da70e94b2c" dependencies = [ "cfg-if", - "libc", - "wasi", ] [[package]] @@ -84,9 +89,9 @@ checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" [[package]] name = "libc" -version = "0.2.112" +version = "0.2.113" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1b03d17f364a3a042d5e5d46b053bbbf82c92c9430c592dd4c064dc6ee997125" +checksum = "eef78b64d87775463c549fbd80e19249ef436ea3bf1de2a1eb7e717ec7fab1e9" [[package]] name = "logging" @@ -125,52 +130,6 @@ version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "da32515d9f6e6e489d7bc9d84c71b060db7247dc035bbe44eac88cf87486d8d5" -[[package]] -name = "ppv-lite86" -version = "0.2.15" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ed0cfbc8191465bed66e1718596ee0b0b35d5ee1f41c5df2189d0fe8bde535ba" - -[[package]] -name = "rand" -version = "0.8.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2e7573632e6454cf6b99d7aac4ccca54be06da05aca2ef7423d22d27d4d4bcd8" -dependencies = [ - "libc", - "rand_chacha", - "rand_core", - "rand_hc", -] - -[[package]] -name = "rand_chacha" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" -dependencies = [ - "ppv-lite86", - "rand_core", -] - -[[package]] -name = "rand_core" -version = "0.6.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d34f1408f55294453790c48b2f1ebbb1c5b4b7563eb1f418bcfcfdbb06ebb4e7" -dependencies = [ - "getrandom", -] - -[[package]] -name = "rand_hc" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d51e9f596de227fda2ea6c84607f5558e196eeaf43c986b724ba4fb8fdf497e7" -dependencies = [ - "rand_core", -] - [[package]] name = "redox_syscall" version = "0.2.10" @@ -195,17 +154,25 @@ version = "1.0.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "73b4b750c782965c211b42f022f59af1fbceabdd026623714f104152f1ec149f" +[[package]] +name = "safe-path" +version = "0.1.0" +dependencies = [ + "libc", + "tempfile", +] + [[package]] name = "serde" -version = "1.0.131" +version = "1.0.133" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b4ad69dfbd3e45369132cc64e6748c2d65cdfb001a2b1c232d128b4ad60561c1" +checksum = "97565067517b60e2d1ea8b268e59ce036de907ac523ad83a0475da04e818989a" [[package]] name = "serde_json" -version = "1.0.73" +version = "1.0.75" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bcbd0344bc6533bc7ec56df11d42fb70f1b912351c0825ccb7211b59d8af7cf5" +checksum = "c059c05b48c5c0067d4b4b2b4f0732dd65feb52daf7e0ea09cd87e7dadc1af79" dependencies = [ "itoa", "ryu", @@ -261,13 +228,13 @@ checksum = "f764005d11ee5f36500a149ace24e00e3da98b0158b3e2d53a7495660d3f4d60" [[package]] name = "tempfile" -version = "3.2.0" +version = "3.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dac1c663cfc93810f88aed9b8941d48cabf856a1b111c29a40439018d870eb22" +checksum = "5cdb1ef4eaeeaddc8fbd371e5017057064af0911902ef36b39801f67cc6d79e4" dependencies = [ "cfg-if", + "fastrand", "libc", - "rand", "redox_syscall", "remove_dir_all", "winapi", @@ -284,19 +251,20 @@ dependencies = [ [[package]] name = "time" -version = "0.1.43" +version = "0.1.44" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ca8a50ef2360fbd1eeb0ecd46795a87a19024eb4b53c5dc916ca1fd95fe62438" +checksum = "6db9e6914ab8b1ae1c260a4ae7a49b6c5611b40328a735b21862567685e73255" dependencies = [ "libc", + "wasi", "winapi", ] [[package]] name = "wasi" -version = "0.10.2+wasi-snapshot-preview1" +version = "0.10.0+wasi-snapshot-preview1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fd6fbd9a79829dd1ad0cc20627bf1ed606756a7f77edff7b66b7064f9cb327c6" +checksum = "1a143597ca7c7793eff794def352d41792a93c481eb1042423ff7ff72ba2c31f" [[package]] name = "winapi" diff --git a/src/libs/Cargo.toml b/src/libs/Cargo.toml new file mode 100644 index 000000000..f4b707a14 --- /dev/null +++ b/src/libs/Cargo.toml @@ -0,0 +1,6 @@ +[workspace] +members = [ + "logging", + "safe-path", +] +resolver = "2" diff --git a/src/libs/README.md b/src/libs/README.md new file mode 100644 index 000000000..36f2f00d7 --- /dev/null +++ b/src/libs/README.md @@ -0,0 +1,10 @@ +The `src/libs` directory hosts library crates which may be shared by multiple Kata Containers components +or published to [`crates.io`](https://crates.io/index.html). + +### Library Crates +Currently it provides following library crates: + +| Library | Description | +|-|-|-| +| [logging](logging/) | Facilities to setup logging subsystem based slog. | +| [safe-path](safe-path/) | Utilities to safely resolve filesystem paths. | diff --git a/src/libs/safe-path/Cargo.toml b/src/libs/safe-path/Cargo.toml new file mode 100644 index 000000000..92e0ee948 --- /dev/null +++ b/src/libs/safe-path/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "safe-path" +version = "0.1.0" +description = "A library to safely handle file system paths for container runtimes" +keywords = ["kata", "container", "path", "securejoin"] +categories = ["parser-implementations", "filesystem"] +authors = ["The Kata Containers community "] +repository = "https://github.com/kata-containers/kata-containers.git" +homepage = "https://katacontainers.io/" +readme = "README.md" +license = "Apache-2.0" +edition = "2018" + +[dependencies] +libc = "0.2.100" + +[dev-dependencies] +tempfile = "3.2.0" diff --git a/src/libs/safe-path/README.md b/src/libs/safe-path/README.md new file mode 100644 index 000000000..3c08f7265 --- /dev/null +++ b/src/libs/safe-path/README.md @@ -0,0 +1,21 @@ +Safe Path +==================== +[![CI](https://github.com/magiclen/path-absolutize/actions/workflows/ci.yml/badge.svg)](https://github.com/magiclen/path-absolutize/actions/workflows/ci.yml) + +A library to safely handle filesystem paths, typically for container runtimes. + +There are often path related attacks, such as symlink based attacks, TOCTTOU attacks. The `safe-path` crate +provides several functions and utility structures to protect against path resolution related attacks. + +## Support + +**Operating Systems**: +- Linux + +## Reference +- [`filepath-securejoin`](https://github.com/cyphar/filepath-securejoin): secure_join() written in Go. +- [CVE-2021-30465](https://github.com/advisories/GHSA-c3xm-pvg7-gh7r): symlink related TOCTOU flaw in `runC`. + +## License + +This code is licensed under [Apache-2.0](../../../LICENSE). diff --git a/src/libs/safe-path/src/lib.rs b/src/libs/safe-path/src/lib.rs new file mode 100644 index 000000000..6b853a3c8 --- /dev/null +++ b/src/libs/safe-path/src/lib.rs @@ -0,0 +1,65 @@ +// Copyright (c) 2022 Alibaba Cloud +// +// SPDX-License-Identifier: Apache-2.0 +// + +//! A library to safely handle filesystem paths, typically for container runtimes. +//! +//! Linux [mount namespace](https://man7.org/linux/man-pages/man7/mount_namespaces.7.html) +//! provides isolation of the list of mounts seen by the processes in each +//! [namespace](https://man7.org/linux/man-pages/man7/namespaces.7.html) instance. +//! Thus, the processes in each of the mount namespace instances will see distinct single-directory +//! hierarchies. +//! +//! Containers are used to isolate workloads from the host system. Container on Linux systems +//! depends on the mount namespace to build an isolated root filesystem for each container, +//! thus protect the host and containers from each other. When creating containers, the container +//! runtime needs to setup filesystem mounts for container rootfs/volumes. Configuration for +//! mounts/paths may be indirectly controlled by end users through: +//! - container images +//! - Kubernetes pod specifications +//! - hook command line arguments +//! +//! These volume configuration information may be controlled by end users/malicious attackers, +//! so it must not be trusted by container runtimes. When the container runtime is preparing mount +//! namespace for a container, it must be very careful to validate user input configuration +//! information and ensure data out of the container rootfs directory won't be affected +//! by the container. There are several types of attacks related to container mount namespace: +//! - symlink based attack +//! - Time of check to time of use (TOCTTOU) +//! +//! This crate provides several mechanisms for container runtimes to safely handle filesystem paths +//! when preparing mount namespace for containers. +//! - [scoped_join()](crate::scoped_join()): safely join `unsafe_path` to `root`, and ensure +//! `unsafe_path` is scoped under `root`. +//! - [scoped_resolve()](crate::scoped_resolve()): resolve `unsafe_path` to a relative path, +//! rooted at and constrained by `root`. +//! - [struct PinnedPathBuf](crate::PinnedPathBuf): safe version of `PathBuf` to protect from +//! TOCTTOU style of attacks, which ensures: +//! - the value of [`PinnedPathBuf::as_path()`] never changes. +//! - the path returned by [`PinnedPathBuf::as_path()`] is always a symlink. +//! - the filesystem object referenced by the symlink [`PinnedPathBuf::as_path()`] never changes. +//! - the value of [`PinnedPathBuf::target()`] never changes. +//! - [struct ScopedDirBuilder](crate::ScopedDirBuilder): safe version of `DirBuilder` to protect +//! from symlink race and TOCTTOU style of attacks, which enhances security by: +//! - ensuring the new directories are created under a specified `root` directory. +//! - avoiding symlink race attacks during making directories. +//! - returning a [PinnedPathBuf] for the last level of directory, so it could be used for other +//! operations safely. +//! +//! The work is inspired by: +//! - [`filepath-securejoin`](https://github.com/cyphar/filepath-securejoin): secure_join() written +//! in Go. +//! - [CVE-2021-30465](https://github.com/advisories/GHSA-c3xm-pvg7-gh7r): symlink related TOCTOU +//! flaw in `runC`. + +#![deny(missing_docs)] + +mod pinned_path_buf; +pub use pinned_path_buf::PinnedPathBuf; + +mod scoped_dir_builder; +pub use scoped_dir_builder::ScopedDirBuilder; + +mod scoped_path_resolver; +pub use scoped_path_resolver::{scoped_join, scoped_resolve}; diff --git a/src/libs/safe-path/src/pinned_path_buf.rs b/src/libs/safe-path/src/pinned_path_buf.rs new file mode 100644 index 000000000..e25b43769 --- /dev/null +++ b/src/libs/safe-path/src/pinned_path_buf.rs @@ -0,0 +1,400 @@ +// Copyright (c) 2022 Alibaba Cloud +// +// SPDX-License-Identifier: Apache-2.0 +// + +use std::ffi::{CString, OsStr}; +use std::fs::{self, File, Metadata, OpenOptions}; +use std::io::{Error, ErrorKind, Result}; +use std::ops::Deref; +use std::os::unix::ffi::OsStrExt; +use std::os::unix::fs::OpenOptionsExt; +use std::os::unix::io::{AsRawFd, FromRawFd, RawFd}; +use std::path::{Component, Path, PathBuf}; + +use crate::scoped_join; + +/// A safe version of [`PathBuf`] pinned to an underlying filesystem object to protect from +/// `TOCTTOU` style of attacks. +/// +/// A [`PinnedPathBuf`] is a resolved path buffer pinned to an underlying filesystem object, which +/// guarantees: +/// - the value of [`PinnedPathBuf::as_path()`] never changes. +/// - the path returned by [`PinnedPathBuf::as_path()`] is always a symlink. +/// - the filesystem object referenced by the symlink [`PinnedPathBuf::as_path()`] never changes. +/// - the value of [`PinnedPathBuf::target()`] never changes. +/// +/// Note: +/// - Though the filesystem object referenced by the symlink [`PinnedPathBuf::as_path()`] never +/// changes, the value of `fs::read_link(PinnedPathBuf::as_path())` may change due to filesystem +/// operations. +/// - The value of [`PinnedPathBuf::target()`] is a cached version of +/// `fs::read_link(PinnedPathBuf::as_path())` generated when creating the `PinnedPathBuf` object. +/// - It's a sign of possible attacks if `[PinnedPathBuf::target()]` doesn't match +/// `fs::read_link(PinnedPathBuf::as_path())`. +/// - Once the [`PinnedPathBuf`] object gets dropped, the [`Path`] returned by +/// [`PinnedPathBuf::as_path()`] becomes invalid. +/// +/// With normal [`PathBuf`], there's a race window for attackers between time to validate a path and +/// time to use the path. An attacker may maliciously change filesystem object referenced by the +/// path by using symlinks to compose an attack. +/// +/// The [`PinnedPathBuf`] is introduced to protect from such attacks, by using the +/// `/proc/self/fd/xxx` files on Linux. The `/proc/self/fd/xxx` file on Linux is a symlink to the +/// real target corresponding to the process's file descriptor `xxx`. And the target filesystem +/// object referenced by the symlink will be kept stable until the file descriptor has been closed. +/// Combined with `O_PATH`, a safe version of `PathBuf` could be built by: +/// - Generate a safe path from `root` and `path` by using [`crate::scoped_join()`]. +/// - Open the safe path with O_PATH | O_CLOEXEC flags, say the fd number is `fd_num`. +/// - Read the symlink target of `/proc/self/fd/fd_num`. +/// - Compare the symlink target with the safe path, it's safe if these two paths equal. +/// - Use the proc file path as a safe version of [`PathBuf`]. +/// - Close the `fd_num` when dropping the [`PinnedPathBuf`] object. +#[derive(Debug)] +pub struct PinnedPathBuf { + handle: File, + path: PathBuf, + target: PathBuf, +} + +impl PinnedPathBuf { + /// Create a [`PinnedPathBuf`] object from `root` and `path`. + /// + /// The `path` must be a subdirectory of `root`, otherwise error will be returned. + pub fn new, U: AsRef>(root: R, path: U) -> Result { + let path = scoped_join(root, path)?; + Self::from_path(path) + } + + /// Create a `PinnedPathBuf` from `path`. + /// + /// If the resolved value of `path` doesn't equal to `path`, an error will be returned. + pub fn from_path>(orig_path: P) -> Result { + let orig_path = orig_path.as_ref(); + let handle = Self::open_by_path(orig_path)?; + Self::new_from_file(handle, orig_path) + } + + /// Try to clone the [`PinnedPathBuf`] object. + pub fn try_clone(&self) -> Result { + let fd = unsafe { libc::dup(self.path_fd()) }; + if fd < 0 { + Err(Error::last_os_error()) + } else { + Ok(Self { + handle: unsafe { File::from_raw_fd(fd) }, + path: Self::get_proc_path(fd), + target: self.target.clone(), + }) + } + } + + /// Return the underlying file descriptor representing the pinned path. + /// + /// Following operations are supported by the returned `RawFd`: + /// - fchdir + /// - fstat/fstatfs + /// - openat/linkat/fchownat/fstatat/readlinkat/mkdirat/*at + /// - fcntl(F_GETFD, F_SETFD, F_GETFL) + pub fn path_fd(&self) -> RawFd { + self.handle.as_raw_fd() + } + + /// Get the symlink path referring the target filesystem object. + pub fn as_path(&self) -> &Path { + self.path.as_path() + } + + /// Get the cached real path of the target filesystem object. + /// + /// The target path is cached version of `fs::read_link(PinnedPathBuf::as_path())` generated + /// when creating the `PinnedPathBuf` object. On the other hand, the value of + /// `fs::read_link(PinnedPathBuf::as_path())` may change due to underlying filesystem operations. + /// So it's a sign of possible attacks if `PinnedPathBuf::target()` does not match + /// `fs::read_link(PinnedPathBuf::as_path())`. + pub fn target(&self) -> &Path { + &self.target + } + + /// Get [`Metadata`] about the path handle. + pub fn metadata(&self) -> Result { + self.handle.metadata() + } + + /// Open a direct child of the filesystem objected referenced by the `PinnedPathBuf` object. + pub fn open_child(&self, path_comp: &OsStr) -> Result { + let name = Self::prepare_path_component(path_comp)?; + let oflags = libc::O_PATH | libc::O_CLOEXEC; + let res = unsafe { libc::openat(self.path_fd(), name.as_ptr(), oflags, 0) }; + if res < 0 { + Err(Error::last_os_error()) + } else { + let handle = unsafe { File::from_raw_fd(res) }; + Self::new_from_file(handle, self.target.join(path_comp)) + } + } + + /// Create or open a child directory if current object is a directory. + pub fn mkdir(&self, path_comp: &OsStr, mode: libc::mode_t) -> Result { + let path_name = Self::prepare_path_component(path_comp)?; + let res = unsafe { libc::mkdirat(self.handle.as_raw_fd(), path_name.as_ptr(), mode) }; + if res < 0 { + Err(Error::last_os_error()) + } else { + self.open_child(path_comp) + } + } + + /// Open a directory/file by path. + /// + /// Obtain a file descriptor that can be used for two purposes: + /// - indicate a location in the filesystem tree + /// - perform operations that act purely at the file descriptor level + fn open_by_path>(path: P) -> Result { + // When O_PATH is specified in flags, flag bits other than O_CLOEXEC, O_DIRECTORY, and + // O_NOFOLLOW are ignored. + let o_flags = libc::O_PATH | libc::O_CLOEXEC; + OpenOptions::new() + .read(true) + .custom_flags(o_flags) + .open(path.as_ref()) + } + + fn get_proc_path(file: F) -> PathBuf { + PathBuf::from(format!("/proc/self/fd/{}", file.as_raw_fd())) + } + + fn new_from_file>(handle: File, orig_path: P) -> Result { + let path = Self::get_proc_path(handle.as_raw_fd()); + let link_path = fs::read_link(path.as_path())?; + if link_path != orig_path.as_ref() { + Err(Error::new( + ErrorKind::Other, + format!( + "Path changed from {} to {} on open, possible attack", + orig_path.as_ref().display(), + link_path.display() + ), + )) + } else { + Ok(PinnedPathBuf { + handle, + path, + target: link_path, + }) + } + } + + #[inline] + fn prepare_path_component(path_comp: &OsStr) -> Result { + let path = Path::new(path_comp); + let mut comps = path.components(); + let name = comps.next(); + if !matches!(name, Some(Component::Normal(_))) || comps.next().is_some() { + return Err(Error::new( + ErrorKind::Other, + format!("Path component {} is invalid", path_comp.to_string_lossy()), + )); + } + let name = name.unwrap(); + if name.as_os_str() != path_comp { + return Err(Error::new( + ErrorKind::Other, + format!("Path component {} is invalid", path_comp.to_string_lossy()), + )); + } + + CString::new(path_comp.as_bytes()).map_err(|_e| { + Error::new( + ErrorKind::Other, + format!("Path component {} is invalid", path_comp.to_string_lossy()), + ) + }) + } +} + +impl Deref for PinnedPathBuf { + type Target = PathBuf; + + fn deref(&self) -> &Self::Target { + &self.path + } +} + +impl AsRef for PinnedPathBuf { + fn as_ref(&self) -> &Path { + self.path.as_path() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs::DirBuilder; + use std::io::Write; + use std::os::unix::fs::{symlink, MetadataExt}; + use std::sync::{Arc, Barrier}; + use std::thread; + + #[test] + fn test_pinned_path_buf() { + // Create a root directory, which itself contains symlinks. + let rootfs_dir = tempfile::tempdir().expect("failed to create tmpdir"); + DirBuilder::new() + .create(rootfs_dir.path().join("b")) + .unwrap(); + symlink(rootfs_dir.path().join("b"), rootfs_dir.path().join("a")).unwrap(); + let rootfs_path = &rootfs_dir.path().join("a"); + + // Create a file and a symlink to it. + fs::create_dir(rootfs_path.join("symlink_dir")).unwrap(); + symlink("/endpoint", rootfs_path.join("symlink_dir/endpoint")).unwrap(); + fs::write(rootfs_path.join("endpoint"), "test").unwrap(); + + // Pin the target and validate the path/content. + let path = PinnedPathBuf::new(rootfs_path.to_path_buf(), "symlink_dir/endpoint").unwrap(); + assert!(!path.is_dir()); + let path_ref = path.deref(); + let target = fs::read_link(path_ref).unwrap(); + assert_eq!(target, rootfs_path.join("endpoint").canonicalize().unwrap()); + let content = fs::read_to_string(&path).unwrap(); + assert_eq!(&content, "test"); + + // Remove the target file and validate that we could still read data from the pinned path. + fs::remove_file(&target).unwrap(); + fs::read_to_string(&target).unwrap_err(); + let content = fs::read_to_string(&path).unwrap(); + assert_eq!(&content, "test"); + } + + #[test] + fn test_pinned_path_buf_race() { + let root_dir = tempfile::tempdir().expect("failed to create tmpdir"); + let root_path = root_dir.path(); + let barrier = Arc::new(Barrier::new(2)); + + fs::write(root_path.join("a"), b"a").unwrap(); + fs::write(root_path.join("b"), b"b").unwrap(); + fs::write(root_path.join("c"), b"c").unwrap(); + symlink("a", root_path.join("s")).unwrap(); + + let root_path2 = root_path.to_path_buf(); + let barrier2 = barrier.clone(); + let thread = thread::spawn(move || { + // step 1 + barrier2.wait(); + fs::remove_file(root_path2.join("a")).unwrap(); + symlink("b", root_path2.join("a")).unwrap(); + barrier2.wait(); + + // step 2 + barrier2.wait(); + fs::remove_file(root_path2.join("b")).unwrap(); + symlink("c", root_path2.join("b")).unwrap(); + barrier2.wait(); + }); + + let path = scoped_join(&root_path, "s").unwrap(); + let data = fs::read_to_string(&path).unwrap(); + assert_eq!(&data, "a"); + assert!(path.is_file()); + barrier.wait(); + barrier.wait(); + // Verify the target has been redirected. + let data = fs::read_to_string(&path).unwrap(); + assert_eq!(&data, "b"); + PinnedPathBuf::from_path(&path).unwrap_err(); + + let pinned_path = PinnedPathBuf::new(&root_path, "s").unwrap(); + let data = fs::read_to_string(&pinned_path).unwrap(); + assert_eq!(&data, "b"); + + // step2 + barrier.wait(); + barrier.wait(); + // Verify it still points to the old target. + let data = fs::read_to_string(&pinned_path).unwrap(); + assert_eq!(&data, "b"); + + thread.join().unwrap(); + } + + #[test] + fn test_new_pinned_path_buf() { + let rootfs_dir = tempfile::tempdir().expect("failed to create tmpdir"); + let rootfs_path = rootfs_dir.path(); + let path = PinnedPathBuf::from_path(rootfs_path).unwrap(); + let _ = OpenOptions::new().read(true).open(&path).unwrap(); + } + + #[test] + fn test_pinned_path_try_clone() { + let rootfs_dir = tempfile::tempdir().expect("failed to create tmpdir"); + let rootfs_path = rootfs_dir.path(); + let path = PinnedPathBuf::from_path(rootfs_path).unwrap(); + let path2 = path.try_clone().unwrap(); + assert_ne!(path.as_path(), path2.as_path()); + } + + #[test] + fn test_new_pinned_path_buf_from_nonexist_file() { + let rootfs_dir = tempfile::tempdir().expect("failed to create tmpdir"); + let rootfs_path = rootfs_dir.path(); + PinnedPathBuf::new(rootfs_path, "does_not_exist").unwrap_err(); + } + + #[test] + fn test_new_pinned_path_buf_without_read_perm() { + let rootfs_dir = tempfile::tempdir().expect("failed to create tmpdir"); + let rootfs_path = rootfs_dir.path(); + let path = rootfs_path.join("write_only_file"); + + let mut file = OpenOptions::new() + .read(false) + .write(true) + .create(true) + .mode(0o200) + .open(&path) + .unwrap(); + file.write_all(&[0xa5u8]).unwrap(); + let md = fs::metadata(&path).unwrap(); + let umask = unsafe { libc::umask(0022) }; + unsafe { libc::umask(umask) }; + assert_eq!(md.mode() & 0o700, 0o200 & !umask); + PinnedPathBuf::from_path(&path).unwrap(); + } + + #[test] + fn test_pinned_path_buf_path_fd() { + let rootfs_dir = tempfile::tempdir().expect("failed to create tmpdir"); + let rootfs_path = rootfs_dir.path(); + let path = rootfs_path.join("write_only_file"); + + let mut file = OpenOptions::new() + .read(false) + .write(true) + .create(true) + .mode(0o200) + .open(&path) + .unwrap(); + file.write_all(&[0xa5u8]).unwrap(); + let handle = PinnedPathBuf::from_path(&path).unwrap(); + // Check that `fstat()` etc works with the fd returned by `path_fd()`. + let fd = handle.path_fd(); + let mut stat: libc::stat = unsafe { std::mem::zeroed() }; + let res = unsafe { libc::fstat(fd, &mut stat as *mut _) }; + assert_eq!(res, 0); + } + + #[test] + fn test_pinned_path_buf_open_child() { + let rootfs_dir = tempfile::tempdir().expect("failed to create tmpdir"); + let rootfs_path = rootfs_dir.path(); + let path = PinnedPathBuf::from_path(rootfs_path).unwrap(); + + fs::write(path.join("child"), "test").unwrap(); + let path = path.open_child(OsStr::new("child")).unwrap(); + let content = fs::read_to_string(&path).unwrap(); + assert_eq!(&content, "test"); + } +} diff --git a/src/libs/safe-path/src/scoped_dir_builder.rs b/src/libs/safe-path/src/scoped_dir_builder.rs new file mode 100644 index 000000000..656da1c0f --- /dev/null +++ b/src/libs/safe-path/src/scoped_dir_builder.rs @@ -0,0 +1,261 @@ +// Copyright (c) 2022 Alibaba Cloud +// +// SPDX-License-Identifier: Apache-2.0 +// + +use std::io::{Error, ErrorKind, Result}; +use std::path::Path; + +use crate::{scoped_join, scoped_resolve, PinnedPathBuf}; + +const DIRECTORY_MODE_DEFAULT: u32 = 0o777; +const DIRECTORY_MODE_MASK: u32 = 0o777; + +/// Safe version of `DirBuilder` to protect from TOCTOU style of attacks. +/// +/// The `ScopedDirBuilder` is a counterpart for `DirBuilder`, with safety enhancements of: +/// - ensuring the new directories are created under a specified `root` directory. +/// - ensuring all created directories are still scoped under `root` even under symlink based +/// attacks. +/// - returning a [PinnedPathBuf] for the last level of directory, so it could be used for other +/// operations safely. +#[derive(Debug)] +pub struct ScopedDirBuilder { + root: PinnedPathBuf, + mode: u32, + recursive: bool, +} + +impl ScopedDirBuilder { + /// Create a new instance of `ScopedDirBuilder` with with default mode/security settings. + pub fn new>(root: P) -> Result { + let root = root.as_ref().canonicalize()?; + let root = PinnedPathBuf::from_path(root)?; + if !root.metadata()?.is_dir() { + return Err(Error::new( + ErrorKind::Other, + format!("Invalid root path: {}", root.display()), + )); + } + + Ok(ScopedDirBuilder { + root, + mode: DIRECTORY_MODE_DEFAULT, + recursive: false, + }) + } + + /// Indicates that directories should be created recursively, creating all parent directories. + /// + /// Parents that do not exist are created with the same security and permissions settings. + pub fn recursive(&mut self, recursive: bool) -> &mut Self { + self.recursive = recursive; + self + } + + /// Sets the mode to create new directories with. This option defaults to 0o755. + pub fn mode(&mut self, mode: u32) -> &mut Self { + self.mode = mode & DIRECTORY_MODE_MASK; + self + } + + /// Creates the specified directory with the options configured in this builder. + /// + /// This is a helper to create subdirectory with an absolute path, without stripping off + /// `self.root`. So error will be returned if path does start with `self.root`. + /// It is considered an error if the directory already exists unless recursive mode is enabled. + pub fn create_with_unscoped_path>(&self, path: P) -> Result { + if !path.as_ref().is_absolute() { + return Err(Error::new( + ErrorKind::Other, + format!( + "Expected absolute directory path: {}", + path.as_ref().display() + ), + )); + } + // Partially canonicalize `path` so we can strip the `root` part. + let scoped_path = scoped_join("/", path)?; + let stripped_path = scoped_path.strip_prefix(self.root.target()).map_err(|_| { + Error::new( + ErrorKind::Other, + format!( + "Path {} is not under {}", + scoped_path.display(), + self.root.target().display() + ), + ) + })?; + + self.do_mkdir(&stripped_path) + } + + /// Creates sub-directory with the options configured in this builder. + /// + /// It is considered an error if the directory already exists unless recursive mode is enabled. + pub fn create>(&self, path: P) -> Result { + let path = scoped_resolve(&self.root, path)?; + self.do_mkdir(&path) + } + + fn do_mkdir(&self, path: &Path) -> Result { + assert!(path.is_relative()); + if path.file_name().is_none() { + if !self.recursive { + return Err(Error::new( + ErrorKind::AlreadyExists, + "directory already exists", + )); + } else { + return self.root.try_clone(); + } + } + + // Safe because `path` have at least one level. + let levels = path.iter().count() - 1; + let mut dir = self.root.try_clone()?; + for (idx, comp) in path.iter().enumerate() { + match dir.open_child(comp) { + Ok(v) => { + if !v.metadata()?.is_dir() { + return Err(Error::new( + ErrorKind::Other, + format!("Path {} is not a directory", v.display()), + )); + } else if !self.recursive && idx == levels { + return Err(Error::new( + ErrorKind::AlreadyExists, + "directory already exists", + )); + } + dir = v; + } + Err(_e) => { + if !self.recursive && idx != levels { + return Err(Error::new( + ErrorKind::NotFound, + format!("parent directory does not exist"), + )); + } + dir = dir.mkdir(comp, self.mode)?; + } + } + } + + Ok(dir) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use std::fs::DirBuilder; + use std::os::unix::fs::{symlink, MetadataExt}; + use tempfile::tempdir; + + #[test] + fn test_scoped_dir_builder() { + // create temporary directory to emulate container rootfs with symlink + let rootfs_dir = tempdir().expect("failed to create tmpdir"); + DirBuilder::new() + .create(rootfs_dir.path().join("b")) + .unwrap(); + symlink(rootfs_dir.path().join("b"), rootfs_dir.path().join("a")).unwrap(); + let rootfs_path = &rootfs_dir.path().join("a"); + + // root directory doesn't exist + ScopedDirBuilder::new(rootfs_path.join("__does_not_exist__")).unwrap_err(); + ScopedDirBuilder::new("__does_not_exist__").unwrap_err(); + + // root is a file + fs::write(rootfs_path.join("txt"), "test").unwrap(); + ScopedDirBuilder::new(rootfs_path.join("txt")).unwrap_err(); + + let mut builder = ScopedDirBuilder::new(&rootfs_path).unwrap(); + + // file with the same name already exists. + builder + .create_with_unscoped_path(rootfs_path.join("txt")) + .unwrap_err(); + // parent is a file + builder.create("/txt/a").unwrap_err(); + // Not starting with root + builder.create_with_unscoped_path("/txt/a").unwrap_err(); + // creating "." without recursive mode should fail + builder + .create_with_unscoped_path(rootfs_path.join(".")) + .unwrap_err(); + // parent doesn't exist + builder + .create_with_unscoped_path(rootfs_path.join("a/b")) + .unwrap_err(); + builder.create("a/b/c").unwrap_err(); + + let path = builder.create("a").unwrap(); + assert!(rootfs_path.join("a").is_dir()); + assert_eq!(path.target(), rootfs_path.join("a").canonicalize().unwrap()); + + // Creating an existing directory without recursive mode should fail. + builder + .create_with_unscoped_path(rootfs_path.join("a")) + .unwrap_err(); + + // Creating an existing directory with recursive mode should succeed. + builder.recursive(true); + let path = builder + .create_with_unscoped_path(rootfs_path.join("a")) + .unwrap(); + assert_eq!(path.target(), rootfs_path.join("a").canonicalize().unwrap()); + let path = builder.create(".").unwrap(); + assert_eq!(path.target(), rootfs_path.canonicalize().unwrap()); + + let umask = unsafe { libc::umask(0022) }; + unsafe { libc::umask(umask) }; + + builder.mode(0o740); + let path = builder.create("a/b/c/d").unwrap(); + assert_eq!( + path.target(), + rootfs_path.join("a/b/c/d").canonicalize().unwrap() + ); + assert!(rootfs_path.join("a/b/c/d").is_dir()); + assert_eq!( + rootfs_path.join("a").metadata().unwrap().mode() & 0o777, + DIRECTORY_MODE_DEFAULT & !umask, + ); + assert_eq!( + rootfs_path.join("a/b").metadata().unwrap().mode() & 0o777, + 0o740 & !umask + ); + assert_eq!( + rootfs_path.join("a/b/c").metadata().unwrap().mode() & 0o777, + 0o740 & !umask + ); + assert_eq!( + rootfs_path.join("a/b/c/d").metadata().unwrap().mode() & 0o777, + 0o740 & !umask + ); + + // Creating should fail if some components are not directory. + builder.create("txt/e/f").unwrap_err(); + fs::write(rootfs_path.join("a/b/txt"), "test").unwrap(); + builder.create("a/b/txt/h/i").unwrap_err(); + } + + #[test] + fn test_create_root() { + let mut builder = ScopedDirBuilder::new("/").unwrap(); + builder.recursive(true); + builder.create("/").unwrap(); + builder.create(".").unwrap(); + builder.create("..").unwrap(); + builder.create("../../.").unwrap(); + builder.create("").unwrap(); + builder.create_with_unscoped_path("/").unwrap(); + builder.create_with_unscoped_path(".").unwrap(); + builder.create_with_unscoped_path("..").unwrap(); + builder.create_with_unscoped_path("../../.").unwrap(); + builder.create_with_unscoped_path("").unwrap(); + } +} diff --git a/src/libs/safe-path/src/scoped_path_resolver.rs b/src/libs/safe-path/src/scoped_path_resolver.rs new file mode 100644 index 000000000..8d61111d7 --- /dev/null +++ b/src/libs/safe-path/src/scoped_path_resolver.rs @@ -0,0 +1,324 @@ +// Copyright (c) 2022 Alibaba Cloud +// +// SPDX-License-Identifier: Apache-2.0 +// + +use std::io::{Error, ErrorKind, Result}; +use std::path::{Component, Path, PathBuf}; + +// Follow the same configuration as +// [secure_join](https://github.com/cyphar/filepath-securejoin/blob/master/join.go#L51) +const MAX_SYMLINK_DEPTH: u32 = 255; + +fn do_scoped_resolve, U: AsRef>( + root: R, + unsafe_path: U, +) -> Result<(PathBuf, PathBuf)> { + let root = root.as_ref().canonicalize()?; + + let mut nlinks = 0u32; + let mut curr_path = unsafe_path.as_ref().to_path_buf(); + 'restart: loop { + let mut subpath = PathBuf::new(); + let mut iter = curr_path.components(); + + 'next_comp: while let Some(comp) = iter.next() { + match comp { + // Linux paths don't have prefixes. + Component::Prefix(_) => { + return Err(Error::new( + ErrorKind::Other, + format!("Invalid path prefix in: {}", unsafe_path.as_ref().display()), + )); + } + // `RootDir` should always be the first component, and Path::components() ensures + // that. + Component::RootDir | Component::CurDir => { + continue 'next_comp; + } + Component::ParentDir => { + subpath.pop(); + } + Component::Normal(n) => { + let path = root.join(&subpath).join(n); + if let Ok(v) = path.read_link() { + nlinks += 1; + if nlinks > MAX_SYMLINK_DEPTH { + return Err(Error::new( + ErrorKind::Other, + format!( + "Too many levels of symlinks: {}", + unsafe_path.as_ref().display() + ), + )); + } + curr_path = if v.is_absolute() { + v.join(iter.as_path()) + } else { + subpath.join(v).join(iter.as_path()) + }; + continue 'restart; + } else { + subpath.push(n); + } + } + } + } + + return Ok((root, subpath)); + } +} + +/// Resolve `unsafe_path` to a relative path, rooted at and constrained by `root`. +/// +/// The `scoped_resolve()` function assumes `root` exists and is an absolute path. It processes +/// each path component in `unsafe_path` as below: +/// - assume it's not a symlink and output if the component doesn't exist yet. +/// - ignore if it's "/" or ".". +/// - go to parent directory but constrained by `root` if it's "..". +/// - recursively resolve to the real path if it's a symlink. All symlink resolutions will be +/// constrained by `root`. +/// - otherwise output the path component. +/// +/// # Arguments +/// - `root`: the absolute path to constrain the symlink resolution. +/// - `unsafe_path`: the path to resolve. +/// +/// Note that the guarantees provided by this function only apply if the path components in the +/// returned PathBuf are not modified (in other words are not replaced with symlinks on the +/// filesystem) after this function has returned. You may use [crate::PinnedPathBuf] to protect +/// from such TOCTOU attacks. +pub fn scoped_resolve, U: AsRef>(root: R, unsafe_path: U) -> Result { + do_scoped_resolve(root, unsafe_path).map(|(_root, path)| path) +} + +/// Safely join `unsafe_path` to `root`, and ensure `unsafe_path` is scoped under `root`. +/// +/// The `scoped_join()` function assumes `root` exists and is an absolute path. It safely joins the +/// two given paths and ensures: +/// - The returned path is guaranteed to be scoped inside `root`. +/// - Any symbolic links in the path are evaluated with the given `root` treated as the root of the +/// filesystem, similar to a chroot. +/// +/// It's modelled after [secure_join](https://github.com/cyphar/filepath-securejoin), but only +/// for Linux systems. +/// +/// # Arguments +/// - `root`: the absolute path to scope the symlink evaluation. +/// - `unsafe_path`: the path to evaluated and joint with `root`. It is unsafe since it may try to +/// escape from the `root` by using "../" or symlinks. +/// +/// # Security +/// On success return, the `scoped_join()` function guarantees that: +/// - The resulting PathBuf must be a child path of `root` and will not contain any symlink path +/// components (they will all get expanded). +/// - When expanding symlinks, all symlink path components must be resolved relative to the provided +/// `root`. In particular, this can be considered a userspace implementation of how chroot(2) +/// operates on file paths. +/// - Non-existent path components are unaffected. +/// +/// Note that the guarantees provided by this function only apply if the path components in the +/// returned string are not modified (in other words are not replaced with symlinks on the +/// filesystem) after this function has returned. You may use [crate::PinnedPathBuf] to protect +/// from such TOCTTOU attacks. +pub fn scoped_join, U: AsRef>(root: R, unsafe_path: U) -> Result { + do_scoped_resolve(root, unsafe_path).map(|(root, path)| root.join(path)) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs::DirBuilder; + use std::os::unix::fs; + use tempfile::tempdir; + + #[allow(dead_code)] + #[derive(Debug)] + struct TestData<'a> { + name: &'a str, + rootfs: &'a Path, + unsafe_path: &'a str, + result: &'a str, + } + + fn exec_tests(tests: &[TestData]) { + for (i, t) in tests.iter().enumerate() { + // Create a string containing details of the test + let msg = format!("test[{}]: {:?}", i, t); + let result = scoped_resolve(t.rootfs, t.unsafe_path).unwrap(); + let msg = format!("{}, result: {:?}", msg, result); + + // Perform the checks + assert_eq!(&result, Path::new(t.result), "{}", msg); + } + } + + #[test] + fn test_scoped_resolve() { + // create temporary directory to emulate container rootfs with symlink + let rootfs_dir = tempdir().expect("failed to create tmpdir"); + DirBuilder::new() + .create(rootfs_dir.path().join("b")) + .unwrap(); + fs::symlink(rootfs_dir.path().join("b"), rootfs_dir.path().join("a")).unwrap(); + let rootfs_path = &rootfs_dir.path().join("a"); + + let tests = [ + TestData { + name: "normal path", + rootfs: rootfs_path, + unsafe_path: "a/b/c", + result: "a/b/c", + }, + TestData { + name: "path with .. at beginning", + rootfs: rootfs_path, + unsafe_path: "../../../a/b/c", + result: "a/b/c", + }, + TestData { + name: "path with complex .. pattern", + rootfs: rootfs_path, + unsafe_path: "../../../a/../../b/../../c", + result: "c", + }, + TestData { + name: "path with .. in middle", + rootfs: rootfs_path, + unsafe_path: "/usr/bin/../../bin/ls", + result: "bin/ls", + }, + TestData { + name: "path with . and ..", + rootfs: rootfs_path, + unsafe_path: "/usr/./bin/../../bin/./ls", + result: "bin/ls", + }, + TestData { + name: "path with . at end", + rootfs: rootfs_path, + unsafe_path: "/usr/./bin/../../bin/./ls/.", + result: "bin/ls", + }, + TestData { + name: "path try to escape by ..", + rootfs: rootfs_path, + unsafe_path: "/usr/./bin/../../../../bin/./ls/../ls", + result: "bin/ls", + }, + TestData { + name: "path with .. at the end", + rootfs: rootfs_path, + unsafe_path: "/usr/./bin/../../bin/./ls/..", + result: "bin", + }, + TestData { + name: "path ..", + rootfs: rootfs_path, + unsafe_path: "..", + result: "", + }, + TestData { + name: "path .", + rootfs: rootfs_path, + unsafe_path: ".", + result: "", + }, + TestData { + name: "path /", + rootfs: rootfs_path, + unsafe_path: "/", + result: "", + }, + TestData { + name: "empty path", + rootfs: rootfs_path, + unsafe_path: "", + result: "", + }, + ]; + + exec_tests(&tests); + } + + #[test] + fn test_scoped_resolve_invalid() { + scoped_resolve("./root_is_not_absolute_path", ".").unwrap_err(); + scoped_resolve("C:", ".").unwrap_err(); + scoped_resolve(r#"\\server\test"#, ".").unwrap_err(); + scoped_resolve(r#"http://localhost/test"#, ".").unwrap_err(); + // Chinese Unicode characters + scoped_resolve(r#"您好"#, ".").unwrap_err(); + } + + #[test] + fn test_scoped_resolve_symlink() { + // create temporary directory to emulate container rootfs with symlink + let rootfs_dir = tempdir().expect("failed to create tmpdir"); + let rootfs_path = &rootfs_dir.path(); + std::fs::create_dir(rootfs_path.join("symlink_dir")).unwrap(); + + fs::symlink("../../../", rootfs_path.join("1")).unwrap(); + let tests = [TestData { + name: "relative symlink beyond root", + rootfs: rootfs_path, + unsafe_path: "1", + result: "", + }]; + exec_tests(&tests); + + fs::symlink("/dddd", rootfs_path.join("2")).unwrap(); + let tests = [TestData { + name: "abs symlink pointing to non-exist directory", + rootfs: rootfs_path, + unsafe_path: "2", + result: "dddd", + }]; + exec_tests(&tests); + + fs::symlink("/", rootfs_path.join("3")).unwrap(); + let tests = [TestData { + name: "abs symlink pointing to /", + rootfs: rootfs_path, + unsafe_path: "3", + result: "", + }]; + exec_tests(&tests); + + fs::symlink("usr/bin/../bin/ls", rootfs_path.join("4")).unwrap(); + let tests = [TestData { + name: "symlink with one ..", + rootfs: rootfs_path, + unsafe_path: "4", + result: "usr/bin/ls", + }]; + exec_tests(&tests); + + fs::symlink("usr/bin/../../bin/ls", rootfs_path.join("5")).unwrap(); + let tests = [TestData { + name: "symlink with two ..", + rootfs: rootfs_path, + unsafe_path: "5", + result: "bin/ls", + }]; + exec_tests(&tests); + + fs::symlink( + "../usr/bin/../../../bin/ls", + rootfs_path.join("symlink_dir/6"), + ) + .unwrap(); + let tests = [TestData { + name: "symlink try to escape", + rootfs: rootfs_path, + unsafe_path: "symlink_dir/6", + result: "bin/ls", + }]; + exec_tests(&tests); + + // Detect symlink loop. + fs::symlink("/endpoint_b", rootfs_path.join("endpoint_a")).unwrap(); + fs::symlink("/endpoint_a", rootfs_path.join("endpoint_b")).unwrap(); + scoped_join(rootfs_path, "endpoint_a").unwrap_err(); + } +} From 0ad89ebd7c5405eca124c936006dfa75346e94f9 Mon Sep 17 00:00:00 2001 From: Liu Jiang Date: Wed, 6 Apr 2022 09:41:18 +0800 Subject: [PATCH 08/22] safe-path: add more unit test cases Add more unit test cases to improve code coverage. Signed-off-by: Liu Jiang --- src/libs/safe-path/src/pinned_path_buf.rs | 44 +++++++++ src/libs/safe-path/src/scoped_dir_builder.rs | 41 ++++++++- .../safe-path/src/scoped_path_resolver.rs | 91 +++++++++++++++++++ 3 files changed, 172 insertions(+), 4 deletions(-) diff --git a/src/libs/safe-path/src/pinned_path_buf.rs b/src/libs/safe-path/src/pinned_path_buf.rs index e25b43769..4310637df 100644 --- a/src/libs/safe-path/src/pinned_path_buf.rs +++ b/src/libs/safe-path/src/pinned_path_buf.rs @@ -230,6 +230,7 @@ impl AsRef for PinnedPathBuf { #[cfg(test)] mod tests { use super::*; + use std::ffi::OsString; use std::fs::DirBuilder; use std::io::Write; use std::os::unix::fs::{symlink, MetadataExt}; @@ -396,5 +397,48 @@ mod tests { let path = path.open_child(OsStr::new("child")).unwrap(); let content = fs::read_to_string(&path).unwrap(); assert_eq!(&content, "test"); + + path.open_child(&OsString::from("__does_not_exist__")) + .unwrap_err(); + path.open_child(&OsString::from("test/a")).unwrap_err(); + } + + #[test] + fn test_prepare_path_component() { + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from(".")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("..")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("/")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("//")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("a/b")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("./b")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("a/.")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("a/..")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("a/./")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("a/../")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("a/./a")).is_err()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("a/../a")).is_err()); + + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("a")).is_ok()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("a.b")).is_ok()); + assert!(PinnedPathBuf::prepare_path_component(&OsString::from("a..b")).is_ok()); + } + + #[test] + fn test_target_fs_object_changed() { + let rootfs_dir = tempfile::tempdir().expect("failed to create tmpdir"); + let rootfs_path = rootfs_dir.path(); + let file = rootfs_path.join("child"); + fs::write(&file, "test").unwrap(); + + let path = PinnedPathBuf::from_path(&file).unwrap(); + let path3 = fs::read_link(path.as_path()).unwrap(); + assert_eq!(&path3, path.target()); + fs::rename(file, rootfs_path.join("child2")).unwrap(); + let path4 = fs::read_link(path.as_path()).unwrap(); + assert_ne!(&path4, path.target()); + fs::remove_file(rootfs_path.join("child2")).unwrap(); + let path5 = fs::read_link(path.as_path()).unwrap(); + assert_ne!(&path4, &path5); } } diff --git a/src/libs/safe-path/src/scoped_dir_builder.rs b/src/libs/safe-path/src/scoped_dir_builder.rs index 656da1c0f..39ceac107 100644 --- a/src/libs/safe-path/src/scoped_dir_builder.rs +++ b/src/libs/safe-path/src/scoped_dir_builder.rs @@ -253,9 +253,42 @@ mod tests { builder.create("../../.").unwrap(); builder.create("").unwrap(); builder.create_with_unscoped_path("/").unwrap(); - builder.create_with_unscoped_path(".").unwrap(); - builder.create_with_unscoped_path("..").unwrap(); - builder.create_with_unscoped_path("../../.").unwrap(); - builder.create_with_unscoped_path("").unwrap(); + builder.create_with_unscoped_path("/..").unwrap(); + builder.create_with_unscoped_path("/../.").unwrap(); + } + + #[test] + fn test_create_with_absolute_path() { + // create temporary directory to emulate container rootfs with symlink + let rootfs_dir = tempdir().expect("failed to create tmpdir"); + DirBuilder::new() + .create(rootfs_dir.path().join("b")) + .unwrap(); + symlink(rootfs_dir.path().join("b"), rootfs_dir.path().join("a")).unwrap(); + let rootfs_path = &rootfs_dir.path().join("a"); + + let mut builder = ScopedDirBuilder::new(&rootfs_path).unwrap(); + builder.create_with_unscoped_path("/").unwrap_err(); + builder + .create_with_unscoped_path(rootfs_path.join("../__xxxx___xxx__")) + .unwrap_err(); + builder + .create_with_unscoped_path(rootfs_path.join("c/d")) + .unwrap_err(); + + // Return `AlreadyExist` when recursive is false + builder.create_with_unscoped_path(&rootfs_path).unwrap_err(); + builder + .create_with_unscoped_path(rootfs_path.join(".")) + .unwrap_err(); + + builder.recursive(true); + builder.create_with_unscoped_path(&rootfs_path).unwrap(); + builder + .create_with_unscoped_path(rootfs_path.join(".")) + .unwrap(); + builder + .create_with_unscoped_path(rootfs_path.join("c/d")) + .unwrap(); } } diff --git a/src/libs/safe-path/src/scoped_path_resolver.rs b/src/libs/safe-path/src/scoped_path_resolver.rs index 8d61111d7..59b06bfe7 100644 --- a/src/libs/safe-path/src/scoped_path_resolver.rs +++ b/src/libs/safe-path/src/scoped_path_resolver.rs @@ -319,6 +319,97 @@ mod tests { // Detect symlink loop. fs::symlink("/endpoint_b", rootfs_path.join("endpoint_a")).unwrap(); fs::symlink("/endpoint_a", rootfs_path.join("endpoint_b")).unwrap(); + scoped_resolve(rootfs_path, "endpoint_a").unwrap_err(); + } + + #[test] + fn test_scoped_join() { + // create temporary directory to emulate container rootfs with symlink + let rootfs_dir = tempdir().expect("failed to create tmpdir"); + let rootfs_path = &rootfs_dir.path(); + + assert_eq!( + scoped_join(&rootfs_path, "a").unwrap(), + rootfs_path.join("a") + ); + assert_eq!( + scoped_join(&rootfs_path, "./a").unwrap(), + rootfs_path.join("a") + ); + assert_eq!( + scoped_join(&rootfs_path, "././a").unwrap(), + rootfs_path.join("a") + ); + assert_eq!( + scoped_join(&rootfs_path, "c/d/../../a").unwrap(), + rootfs_path.join("a") + ); + assert_eq!( + scoped_join(&rootfs_path, "c/d/../../../.././a").unwrap(), + rootfs_path.join("a") + ); + assert_eq!( + scoped_join(&rootfs_path, "../../a").unwrap(), + rootfs_path.join("a") + ); + assert_eq!( + scoped_join(&rootfs_path, "./../a").unwrap(), + rootfs_path.join("a") + ); + } + + #[test] + fn test_scoped_join_symlink() { + // create temporary directory to emulate container rootfs with symlink + let rootfs_dir = tempdir().expect("failed to create tmpdir"); + let rootfs_path = &rootfs_dir.path(); + DirBuilder::new() + .recursive(true) + .create(rootfs_dir.path().join("b/c")) + .unwrap(); + fs::symlink("b/c", rootfs_dir.path().join("a")).unwrap(); + + let target = rootfs_path.join("b/c"); + assert_eq!(scoped_join(&rootfs_path, "a").unwrap(), target); + assert_eq!(scoped_join(&rootfs_path, "./a").unwrap(), target); + assert_eq!(scoped_join(&rootfs_path, "././a").unwrap(), target); + assert_eq!(scoped_join(&rootfs_path, "b/c/../../a").unwrap(), target); + assert_eq!( + scoped_join(&rootfs_path, "b/c/../../../.././a").unwrap(), + target + ); + assert_eq!(scoped_join(&rootfs_path, "../../a").unwrap(), target); + assert_eq!(scoped_join(&rootfs_path, "./../a").unwrap(), target); + assert_eq!(scoped_join(&rootfs_path, "a/../../../a").unwrap(), target); + assert_eq!(scoped_join(&rootfs_path, "a/../../../b/c").unwrap(), target); + } + + #[test] + fn test_scoped_join_symlink_loop() { + // create temporary directory to emulate container rootfs with symlink + let rootfs_dir = tempdir().expect("failed to create tmpdir"); + let rootfs_path = &rootfs_dir.path(); + fs::symlink("/endpoint_b", rootfs_path.join("endpoint_a")).unwrap(); + fs::symlink("/endpoint_a", rootfs_path.join("endpoint_b")).unwrap(); scoped_join(rootfs_path, "endpoint_a").unwrap_err(); } + + #[test] + fn test_scoped_join_unicode_character() { + // create temporary directory to emulate container rootfs with symlink + let rootfs_dir = tempdir().expect("failed to create tmpdir"); + let rootfs_path = &rootfs_dir.path().canonicalize().unwrap(); + + let path = scoped_join(rootfs_path, "您好").unwrap(); + assert_eq!(path, rootfs_path.join("您好")); + + let path = scoped_join(rootfs_path, "../../../您好").unwrap(); + assert_eq!(path, rootfs_path.join("您好")); + + let path = scoped_join(rootfs_path, "。。/您好").unwrap(); + assert_eq!(path, rootfs_path.join("。。/您好")); + + let path = scoped_join(rootfs_path, "您好/../../test").unwrap(); + assert_eq!(path, rootfs_path.join("test")); + } } From 2eeb5dc2235d43d0cc1a7b8c4c8411621d398cce Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 5 Apr 2022 14:39:06 +1000 Subject: [PATCH 09/22] runtime: Don't use fixed /tmp/mountPoint path Several tests in kata_agent_test.go create /tmp/mountPoint as a dummy directory to mount. This is not cleaned up after the test. Although it is in /tmp, that's still a little messy and can be confusing to a user. In addition, because it uses the same name every time, it allows for one run of the test to interfere with the next. Use the built in t.TempDir() to use an automatically named and deleted temporary directory instead. Signed-off-by: David Gibson --- src/runtime/virtcontainers/kata_agent_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/runtime/virtcontainers/kata_agent_test.go b/src/runtime/virtcontainers/kata_agent_test.go index af26edc0c..f4d996736 100644 --- a/src/runtime/virtcontainers/kata_agent_test.go +++ b/src/runtime/virtcontainers/kata_agent_test.go @@ -191,8 +191,7 @@ func TestKataAgentSendReq(t *testing.T) { func TestHandleEphemeralStorage(t *testing.T) { k := kataAgent{} var ociMounts []specs.Mount - mountSource := "/tmp/mountPoint" - os.Mkdir(mountSource, 0755) + mountSource := t.TempDir() mount := specs.Mount{ Type: KataEphemeralDevType, @@ -212,8 +211,7 @@ func TestHandleEphemeralStorage(t *testing.T) { func TestHandleLocalStorage(t *testing.T) { k := kataAgent{} var ociMounts []specs.Mount - mountSource := "/tmp/mountPoint" - os.Mkdir(mountSource, 0755) + mountSource := t.TempDir() mount := specs.Mount{ Type: KataLocalDevType, @@ -688,8 +686,7 @@ func TestHandleShm(t *testing.T) { // In case the type of mount is ephemeral, the container mount is not // shared with the sandbox shm. ociMounts[0].Type = KataEphemeralDevType - mountSource := "/tmp/mountPoint" - os.Mkdir(mountSource, 0755) + mountSource := t.TempDir() ociMounts[0].Source = mountSource k.handleShm(ociMounts, sandbox) From 90b2f5b7760d80134e4c93869e53669f5e870717 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 5 Apr 2022 19:10:10 +1000 Subject: [PATCH 10/22] runtime: Make SetupOCIConfigFile clean up after itself SetupOCIConfigFile creates a temporary directory with os.MkDirTemp(). This means the callers need to register a deferred function to remove it again. At least one of them was commented out meaning that a /temp/katatest- directory was leftover after the unit tests ran. Change to using t.TempDir() which as well as better matching other parts of the tests means the testing framework will handle cleaning it up. Signed-off-by: David Gibson --- src/runtime/pkg/containerd-shim-v2/create_test.go | 6 ------ src/runtime/pkg/containerd-shim-v2/delete_test.go | 5 ++--- src/runtime/pkg/containerd-shim-v2/service_test.go | 3 +-- src/runtime/pkg/katatestutils/utils.go | 5 ++--- src/runtime/pkg/katautils/create_test.go | 12 +++--------- 5 files changed, 8 insertions(+), 23 deletions(-) diff --git a/src/runtime/pkg/containerd-shim-v2/create_test.go b/src/runtime/pkg/containerd-shim-v2/create_test.go index 40ffde59f..994de7c43 100644 --- a/src/runtime/pkg/containerd-shim-v2/create_test.go +++ b/src/runtime/pkg/containerd-shim-v2/create_test.go @@ -50,7 +50,6 @@ func TestCreateSandboxSuccess(t *testing.T) { }() tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - // defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -99,7 +98,6 @@ func TestCreateSandboxFail(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -137,7 +135,6 @@ func TestCreateSandboxConfigFail(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -187,7 +184,6 @@ func TestCreateContainerSuccess(t *testing.T) { } tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -227,7 +223,6 @@ func TestCreateContainerFail(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -278,7 +273,6 @@ func TestCreateContainerConfigFail(t *testing.T) { }() tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) diff --git a/src/runtime/pkg/containerd-shim-v2/delete_test.go b/src/runtime/pkg/containerd-shim-v2/delete_test.go index f84f5e596..7d959e15a 100644 --- a/src/runtime/pkg/containerd-shim-v2/delete_test.go +++ b/src/runtime/pkg/containerd-shim-v2/delete_test.go @@ -7,7 +7,6 @@ package containerdshim import ( - "os" "testing" taskAPI "github.com/containerd/containerd/runtime/v2/task" @@ -25,8 +24,8 @@ func TestDeleteContainerSuccessAndFail(t *testing.T) { MockID: testSandboxID, } - rootPath, bundlePath, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(rootPath) + _, bundlePath, _ := ktu.SetupOCIConfigFile(t) + _, err := compatoci.ParseConfigJSON(bundlePath) assert.NoError(err) diff --git a/src/runtime/pkg/containerd-shim-v2/service_test.go b/src/runtime/pkg/containerd-shim-v2/service_test.go index b501df99c..f895b4e85 100644 --- a/src/runtime/pkg/containerd-shim-v2/service_test.go +++ b/src/runtime/pkg/containerd-shim-v2/service_test.go @@ -41,8 +41,7 @@ func TestServiceCreate(t *testing.T) { assert := assert.New(t) - tmpdir, bundleDir, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) + _, bundleDir, _ := ktu.SetupOCIConfigFile(t) ctx := context.Background() diff --git a/src/runtime/pkg/katatestutils/utils.go b/src/runtime/pkg/katatestutils/utils.go index 7750c23de..527c9cfbc 100644 --- a/src/runtime/pkg/katatestutils/utils.go +++ b/src/runtime/pkg/katatestutils/utils.go @@ -346,11 +346,10 @@ func IsInGitHubActions() bool { func SetupOCIConfigFile(t *testing.T) (rootPath string, bundlePath, ociConfigFile string) { assert := assert.New(t) - tmpdir, err := os.MkdirTemp("", "katatest-") - assert.NoError(err) + tmpdir := t.TempDir() bundlePath = filepath.Join(tmpdir, "bundle") - err = os.MkdirAll(bundlePath, testDirMode) + err := os.MkdirAll(bundlePath, testDirMode) assert.NoError(err) ociConfigFile = filepath.Join(bundlePath, "config.json") diff --git a/src/runtime/pkg/katautils/create_test.go b/src/runtime/pkg/katautils/create_test.go index 56e7fa27a..15b456113 100644 --- a/src/runtime/pkg/katautils/create_test.go +++ b/src/runtime/pkg/katautils/create_test.go @@ -212,7 +212,6 @@ func TestCreateSandboxConfigFail(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -246,7 +245,6 @@ func TestCreateSandboxFail(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -269,7 +267,6 @@ func TestCreateSandboxAnnotations(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -342,8 +339,7 @@ func TestCheckForFips(t *testing.T) { func TestCreateContainerContainerConfigFail(t *testing.T) { assert := assert.New(t) - tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) + _, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) spec, err := compatoci.ParseConfigJSON(bundlePath) assert.NoError(err) @@ -370,8 +366,7 @@ func TestCreateContainerContainerConfigFail(t *testing.T) { func TestCreateContainerFail(t *testing.T) { assert := assert.New(t) - tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) + _, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) spec, err := compatoci.ParseConfigJSON(bundlePath) assert.NoError(err) @@ -405,8 +400,7 @@ func TestCreateContainer(t *testing.T) { mockSandbox.CreateContainerFunc = nil }() - tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) + _, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) spec, err := compatoci.ParseConfigJSON(bundlePath) assert.NoError(err) From f7ba21c86fc78fc36699204f4333df49995d5ebf Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 5 Apr 2022 21:01:28 +1000 Subject: [PATCH 11/22] runtime: Clean up mock hook logs in tests The tests in hook_test.go run a mock hook binary, which does some debug logging to /tmp/mock_hook.log. Currently we don't clean up those logs when the tests are done. Use a test cleanup function to do this. Signed-off-by: David Gibson --- src/runtime/pkg/katautils/hook_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/runtime/pkg/katautils/hook_test.go b/src/runtime/pkg/katautils/hook_test.go index 9ef48499b..b7bb03409 100644 --- a/src/runtime/pkg/katautils/hook_test.go +++ b/src/runtime/pkg/katautils/hook_test.go @@ -22,6 +22,7 @@ var testContainerIDHook = "test-container-id" var testControllerIDHook = "test-controller-id" var testBinHookPath = "mockhook/hook" var testBundlePath = "/test/bundle" +var mockHookLogFile = "/tmp/mock_hook.log" func getMockHookBinPath() string { return testBinHookPath @@ -49,12 +50,17 @@ func createWrongHook() specs.Hook { } } +func cleanMockHookLogFile() { + _ = os.Remove(mockHookLogFile) +} + func TestRunHook(t *testing.T) { if tc.NotValid(ktu.NeedRoot()) { t.Skip(ktu.TestDisabledNeedRoot) } assert := assert.New(t) + t.Cleanup(cleanMockHookLogFile) ctx := context.Background() spec := specs.Spec{} @@ -87,6 +93,7 @@ func TestPreStartHooks(t *testing.T) { } assert := assert.New(t) + t.Cleanup(cleanMockHookLogFile) ctx := context.Background() @@ -129,6 +136,7 @@ func TestPostStartHooks(t *testing.T) { } assert := assert.New(t) + t.Cleanup(cleanMockHookLogFile) ctx := context.Background() @@ -173,6 +181,7 @@ func TestPostStopHooks(t *testing.T) { assert := assert.New(t) ctx := context.Background() + t.Cleanup(cleanMockHookLogFile) // Hooks field is nil spec := specs.Spec{} From bec59f9e398247d3946443c6c57f731f185a9b03 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 12:10:59 +1000 Subject: [PATCH 12/22] runtime: Make bind mount tests better clean up after themselves There are several tests in mount_test.go which perform a sample bind mount. These need a corresponding unmount to clean up afterwards or attempting to delete the temporary files will fail due to the existing mountpoint. Most of them had such an unmount, but TestBindMountInvalidPgtypes was missing one. In addition, the existing unmounts where done inconsistently - one was simply inline (so wouldn't be executed if the test fails too early) and one is a defer. Change them all to use the t.Cleanup mechanism. For the dummy mountpoint files, rather than cleaning them up after the test, the tests were removing them at the beginning of the test. That stops the test being messed up by a previous run, but messily. Since these are created in a private temporary directory anyway, if there's something already there, that indicates a problem we shouldn't ignore. In fact we don't need to explicitly remove these at all - they'll be removed along with the rest of the private temporary directory. Signed-off-by: David Gibson --- src/runtime/virtcontainers/mount_test.go | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/runtime/virtcontainers/mount_test.go b/src/runtime/virtcontainers/mount_test.go index b658de829..056fa2c14 100644 --- a/src/runtime/virtcontainers/mount_test.go +++ b/src/runtime/virtcontainers/mount_test.go @@ -381,6 +381,12 @@ func TestBindMountFailingMount(t *testing.T) { assert.Error(err) } +func cleanupFooMount() { + dest := filepath.Join(testDir, "fooDirDest") + + syscall.Unmount(dest, 0) +} + func TestBindMountSuccessful(t *testing.T) { assert := assert.New(t) if tc.NotValid(ktu.NeedRoot()) { @@ -389,9 +395,7 @@ func TestBindMountSuccessful(t *testing.T) { source := filepath.Join(testDir, "fooDirSrc") dest := filepath.Join(testDir, "fooDirDest") - syscall.Unmount(dest, 0) - os.Remove(source) - os.Remove(dest) + t.Cleanup(cleanupFooMount) err := os.MkdirAll(source, mountPerm) assert.NoError(err) @@ -401,8 +405,6 @@ func TestBindMountSuccessful(t *testing.T) { err = bindMount(context.Background(), source, dest, false, "private") assert.NoError(err) - - syscall.Unmount(dest, 0) } func TestBindMountReadonlySuccessful(t *testing.T) { @@ -413,9 +415,7 @@ func TestBindMountReadonlySuccessful(t *testing.T) { source := filepath.Join(testDir, "fooDirSrc") dest := filepath.Join(testDir, "fooDirDest") - syscall.Unmount(dest, 0) - os.Remove(source) - os.Remove(dest) + t.Cleanup(cleanupFooMount) err := os.MkdirAll(source, mountPerm) assert.NoError(err) @@ -426,8 +426,6 @@ func TestBindMountReadonlySuccessful(t *testing.T) { err = bindMount(context.Background(), source, dest, true, "private") assert.NoError(err) - defer syscall.Unmount(dest, 0) - // should not be able to create file in read-only mount destFile := filepath.Join(dest, "foo") _, err = os.OpenFile(destFile, os.O_CREATE, mountPerm) @@ -442,9 +440,7 @@ func TestBindMountInvalidPgtypes(t *testing.T) { source := filepath.Join(testDir, "fooDirSrc") dest := filepath.Join(testDir, "fooDirDest") - syscall.Unmount(dest, 0) - os.Remove(source) - os.Remove(dest) + t.Cleanup(cleanupFooMount) err := os.MkdirAll(source, mountPerm) assert.NoError(err) From 1719a8b49152d4ee5f3e944324fd9dd1f70bbbb9 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 15:29:06 +1000 Subject: [PATCH 13/22] runtime: Don't abuse MockStorageRootPath() for factory tests A number of unit tests under virtcontainers/factory use MockStorageRootPath() as a general purpose temporary directory. This doesn't make sense: the mockfs driver isn't even in use here since we only call EnableMockTesting for the pase virtcontainers package, not the subpackages. Instead use t.TempDir() which is for exactly this purpose. As a bonus it also handles the cleanup, so we don't need MockStorageDestroy any more. Signed-off-by: David Gibson --- .../virtcontainers/factory/cache/cache_test.go | 4 +--- .../factory/direct/direct_test.go | 4 +--- .../virtcontainers/factory/factory_test.go | 18 +++++++----------- .../factory/template/template_test.go | 4 +--- .../virtcontainers/persist/fs/mockfs.go | 4 ---- 5 files changed, 10 insertions(+), 24 deletions(-) diff --git a/src/runtime/virtcontainers/factory/cache/cache_test.go b/src/runtime/virtcontainers/factory/cache/cache_test.go index d0971ef12..9f2fa3a5b 100644 --- a/src/runtime/virtcontainers/factory/cache/cache_test.go +++ b/src/runtime/virtcontainers/factory/cache/cache_test.go @@ -13,14 +13,12 @@ import ( vc "github.com/kata-containers/kata-containers/src/runtime/virtcontainers" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/factory/direct" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" ) func TestTemplateFactory(t *testing.T) { assert := assert.New(t) - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, diff --git a/src/runtime/virtcontainers/factory/direct/direct_test.go b/src/runtime/virtcontainers/factory/direct/direct_test.go index 724b6cb66..862fcb636 100644 --- a/src/runtime/virtcontainers/factory/direct/direct_test.go +++ b/src/runtime/virtcontainers/factory/direct/direct_test.go @@ -12,14 +12,12 @@ import ( "github.com/stretchr/testify/assert" vc "github.com/kata-containers/kata-containers/src/runtime/virtcontainers" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" ) func TestTemplateFactory(t *testing.T) { assert := assert.New(t) - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, diff --git a/src/runtime/virtcontainers/factory/factory_test.go b/src/runtime/virtcontainers/factory/factory_test.go index 71af52d71..80cff101d 100644 --- a/src/runtime/virtcontainers/factory/factory_test.go +++ b/src/runtime/virtcontainers/factory/factory_test.go @@ -12,7 +12,6 @@ import ( vc "github.com/kata-containers/kata-containers/src/runtime/virtcontainers" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/factory/base" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/mock" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils" "github.com/sirupsen/logrus" @@ -39,10 +38,10 @@ func TestNewFactory(t *testing.T) { _, err = NewFactory(ctx, config, false) assert.Error(err) - defer fs.MockStorageDestroy() + testDir := t.TempDir() config.VMConfig.HypervisorConfig = vc.HypervisorConfig{ - KernelPath: fs.MockStorageRootPath(), - ImagePath: fs.MockStorageRootPath(), + KernelPath: testDir, + ImagePath: testDir, } // direct @@ -69,7 +68,7 @@ func TestNewFactory(t *testing.T) { defer hybridVSockTTRPCMock.Stop() config.Template = true - config.TemplatePath = fs.MockStorageRootPath() + config.TemplatePath = testDir f, err = NewFactory(ctx, config, false) assert.Nil(err) f.CloseFactory(ctx) @@ -134,8 +133,7 @@ func TestCheckVMConfig(t *testing.T) { err = checkVMConfig(config1, config2) assert.Nil(err) - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() config1.HypervisorConfig = vc.HypervisorConfig{ KernelPath: testDir, @@ -155,8 +153,7 @@ func TestCheckVMConfig(t *testing.T) { func TestFactoryGetVM(t *testing.T) { assert := assert.New(t) - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, @@ -321,8 +318,7 @@ func TestDeepCompare(t *testing.T) { config.VMConfig = vc.VMConfig{ HypervisorType: vc.MockHypervisor, } - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() config.VMConfig.HypervisorConfig = vc.HypervisorConfig{ KernelPath: testDir, diff --git a/src/runtime/virtcontainers/factory/template/template_test.go b/src/runtime/virtcontainers/factory/template/template_test.go index 55c7bc596..c067c793e 100644 --- a/src/runtime/virtcontainers/factory/template/template_test.go +++ b/src/runtime/virtcontainers/factory/template/template_test.go @@ -16,7 +16,6 @@ import ( "github.com/stretchr/testify/assert" vc "github.com/kata-containers/kata-containers/src/runtime/virtcontainers" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/mock" ) @@ -32,8 +31,7 @@ func TestTemplateFactory(t *testing.T) { templateWaitForAgent = 1 * time.Microsecond - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, diff --git a/src/runtime/virtcontainers/persist/fs/mockfs.go b/src/runtime/virtcontainers/persist/fs/mockfs.go index f972782cf..23e09c8c0 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs.go @@ -30,10 +30,6 @@ func MockRunVMStoragePath() string { return filepath.Join(MockStorageRootPath(), vmPathSuffix) } -func MockStorageDestroy() { - os.RemoveAll(MockStorageRootPath()) -} - func MockFSInit() (persistapi.PersistDriver, error) { driver, err := Init() if err != nil { From 963d03ea8a7cf9d9dc298c5c6c80e98a0dec7606 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 13:11:56 +1000 Subject: [PATCH 14/22] runtime: Export StoragePathSuffix storagePathSuffix defines the file path suffix - "vc" - used for Kata's persistent storage information, as a private constant. We duplicate this information in fc.go which also needs it. Export it from fs.go instead, so it can be used in fc.go. Signed-off-by: David Gibson --- src/runtime/virtcontainers/fc.go | 5 ++--- src/runtime/virtcontainers/persist/fs/fs.go | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/runtime/virtcontainers/fc.go b/src/runtime/virtcontainers/fc.go index 6137e32cf..4630a810c 100644 --- a/src/runtime/virtcontainers/fc.go +++ b/src/runtime/virtcontainers/fc.go @@ -27,6 +27,7 @@ import ( hv "github.com/kata-containers/kata-containers/src/runtime/pkg/hypervisors" "github.com/kata-containers/kata-containers/src/runtime/pkg/katautils/katatrace" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/config" + "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/firecracker/client" models "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/firecracker/client/models" ops "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/firecracker/client/operations" @@ -84,8 +85,6 @@ const ( fcMetricsFifo = "metrics.fifo" defaultFcConfig = "fcConfig.json" - // storagePathSuffix mirrors persist/fs/fs.go:storagePathSuffix - storagePathSuffix = "vc" ) // Specify the minimum version of firecracker supported @@ -244,7 +243,7 @@ func (fc *firecracker) setPaths(hypervisorConfig *HypervisorConfig) { // /// hypervisorName := filepath.Base(hypervisorConfig.HypervisorPath) //fs.RunStoragePath cannot be used as we need exec perms - fc.chrootBaseDir = filepath.Join("/run", storagePathSuffix) + fc.chrootBaseDir = filepath.Join("/run", fs.StoragePathSuffix) fc.vmPath = filepath.Join(fc.chrootBaseDir, hypervisorName, fc.id) fc.jailerRoot = filepath.Join(fc.vmPath, "root") // auto created by jailer diff --git a/src/runtime/virtcontainers/persist/fs/fs.go b/src/runtime/virtcontainers/persist/fs/fs.go index 630ed7636..407f76556 100644 --- a/src/runtime/virtcontainers/persist/fs/fs.go +++ b/src/runtime/virtcontainers/persist/fs/fs.go @@ -29,11 +29,11 @@ const dirMode = os.FileMode(0700) | os.ModeDir // fileMode is the permission bits used for creating a file const fileMode = os.FileMode(0600) -// storagePathSuffix is the suffix used for all storage paths +// StoragePathSuffix is the suffix used for all storage paths // // Note: this very brief path represents "virtcontainers". It is as // terse as possible to minimise path length. -const storagePathSuffix = "vc" +const StoragePathSuffix = "vc" // sandboxPathSuffix is the suffix used for sandbox storage const sandboxPathSuffix = "sbs" @@ -64,7 +64,7 @@ func Init() (persistapi.PersistDriver, error) { return &FS{ sandboxState: &persistapi.SandboxState{}, containerState: make(map[string]persistapi.ContainerState), - storageRootPath: filepath.Join("/run", storagePathSuffix), + storageRootPath: filepath.Join("/run", StoragePathSuffix), driverName: "fs", }, nil } From 5d8438e9397c5f44ad460bbb695501c46b3616da Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 14:46:44 +1000 Subject: [PATCH 15/22] runtime: Move mockfs control global into mockfs.go virtcontainers/persist/fs/mockfs.go defines a mock filesystem type for testing. A global variable in virtcontainers/persist/manager.go is used to force use of the mock fs rather than a normal one. This patch moves the global, and the EnableMockTesting() function which sets it into mockfs.go. This is slightly cleaner to begin with, and will allow some further enhancements. Signed-off-by: David Gibson --- .../virtcontainers/persist/fs/mockfs.go | 13 +++++++ .../virtcontainers/persist/fs/mockfs_test.go | 34 +++++++++++++++++++ src/runtime/virtcontainers/persist/manager.go | 10 ++---- .../virtcontainers/persist/manager_test.go | 14 -------- .../virtcontainers/virtcontainers_test.go | 3 +- 5 files changed, 51 insertions(+), 23 deletions(-) create mode 100644 src/runtime/virtcontainers/persist/fs/mockfs_test.go diff --git a/src/runtime/virtcontainers/persist/fs/mockfs.go b/src/runtime/virtcontainers/persist/fs/mockfs.go index 23e09c8c0..4c4f49269 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs.go @@ -13,11 +13,17 @@ import ( persistapi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/api" ) +var mockTesting = false + type MockFS struct { // inherit from FS. Overwrite if needed. *FS } +func EnableMockTesting() { + mockTesting = true +} + func MockStorageRootPath() string { return filepath.Join(os.TempDir(), "vc", "mockfs") } @@ -46,3 +52,10 @@ func MockFSInit() (persistapi.PersistDriver, error) { return &MockFS{fsDriver}, nil } + +func MockAutoInit() (persistapi.PersistDriver, error) { + if mockTesting { + return MockFSInit() + } + return nil, nil +} diff --git a/src/runtime/virtcontainers/persist/fs/mockfs_test.go b/src/runtime/virtcontainers/persist/fs/mockfs_test.go new file mode 100644 index 000000000..99709a78e --- /dev/null +++ b/src/runtime/virtcontainers/persist/fs/mockfs_test.go @@ -0,0 +1,34 @@ +// Copyright Red Hat. +// +// SPDX-License-Identifier: Apache-2.0 +// + +package fs + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestMockAutoInit(t *testing.T) { + assert := assert.New(t) + orgMockTesting := mockTesting + defer func() { + mockTesting = orgMockTesting + }() + + mockTesting = false + + fsd, err := MockAutoInit() + assert.Nil(fsd) + assert.NoError(err) + + // Testing mock driver + mockTesting = true + fsd, err = MockAutoInit() + assert.NoError(err) + expectedFS, err := MockFSInit() + assert.NoError(err) + assert.Equal(expectedFS, fsd) +} diff --git a/src/runtime/virtcontainers/persist/manager.go b/src/runtime/virtcontainers/persist/manager.go index a104bdcab..32504c932 100644 --- a/src/runtime/virtcontainers/persist/manager.go +++ b/src/runtime/virtcontainers/persist/manager.go @@ -28,13 +28,8 @@ var ( RootFSName: fs.Init, RootlessFSName: fs.RootlessInit, } - mockTesting = false ) -func EnableMockTesting() { - mockTesting = true -} - // GetDriver returns new PersistDriver according to driver name func GetDriverByName(name string) (persistapi.PersistDriver, error) { if expErr != nil { @@ -56,8 +51,9 @@ func GetDriver() (persistapi.PersistDriver, error) { return nil, expErr } - if mockTesting { - return fs.MockFSInit() + mock, err := fs.MockAutoInit() + if mock != nil || err != nil { + return mock, err } if rootless.IsRootless() { diff --git a/src/runtime/virtcontainers/persist/manager_test.go b/src/runtime/virtcontainers/persist/manager_test.go index 074ca9266..4347f9adc 100644 --- a/src/runtime/virtcontainers/persist/manager_test.go +++ b/src/runtime/virtcontainers/persist/manager_test.go @@ -27,12 +27,6 @@ func TestGetDriverByName(t *testing.T) { func TestGetDriver(t *testing.T) { assert := assert.New(t) - orgMockTesting := mockTesting - defer func() { - mockTesting = orgMockTesting - }() - - mockTesting = false fsd, err := GetDriver() assert.NoError(err) @@ -46,12 +40,4 @@ func TestGetDriver(t *testing.T) { assert.NoError(err) assert.Equal(expectedFS, fsd) - - // Testing mock driver - mockTesting = true - fsd, err = GetDriver() - assert.NoError(err) - expectedFS, err = fs.MockFSInit() - assert.NoError(err) - assert.Equal(expectedFS, fsd) } diff --git a/src/runtime/virtcontainers/virtcontainers_test.go b/src/runtime/virtcontainers/virtcontainers_test.go index cb03e2351..6a3d7fa58 100644 --- a/src/runtime/virtcontainers/virtcontainers_test.go +++ b/src/runtime/virtcontainers/virtcontainers_test.go @@ -15,7 +15,6 @@ import ( "syscall" "testing" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils" "github.com/sirupsen/logrus" @@ -108,7 +107,7 @@ func setupClh() { func TestMain(m *testing.M) { var err error - persist.EnableMockTesting() + fs.EnableMockTesting() flag.Parse() From ef6d54a78106f2a1c8e8c25dae3bb91f2d76cf65 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 15:02:48 +1000 Subject: [PATCH 16/22] runtime: Let MockFSInit create a mock fs driver at any path Currently MockFSInit always creates the mockfs at the fixed path /tmp/vc/mockfs. This change allows it to be initialized at any path given as a parameter. This allows the tests in fs_test.go to be simplified, because the by using a temporary directory from t.TempDir(), which is automatically cleaned up, we don't need to manually trigger initTestDir() (which is misnamed, it's actually a cleanup function). For now we still use the fixed path when auto-creating the mockfs in MockAutoInit(), but we'll change that later. Signed-off-by: David Gibson --- .../virtcontainers/persist/fs/fs_test.go | 26 +++++-------------- .../virtcontainers/persist/fs/mockfs.go | 6 ++--- .../virtcontainers/persist/fs/mockfs_test.go | 2 +- 3 files changed, 10 insertions(+), 24 deletions(-) diff --git a/src/runtime/virtcontainers/persist/fs/fs_test.go b/src/runtime/virtcontainers/persist/fs/fs_test.go index 91af4af15..370b87c52 100644 --- a/src/runtime/virtcontainers/persist/fs/fs_test.go +++ b/src/runtime/virtcontainers/persist/fs/fs_test.go @@ -14,8 +14,8 @@ import ( "github.com/stretchr/testify/assert" ) -func getFsDriver() (*FS, error) { - driver, err := MockFSInit() +func getFsDriver(t *testing.T) (*FS, error) { + driver, err := MockFSInit(t.TempDir()) if err != nil { return nil, fmt.Errorf("failed to init fs driver") } @@ -27,16 +27,8 @@ func getFsDriver() (*FS, error) { return fs.FS, nil } -func initTestDir() func() { - return func() { - os.RemoveAll(MockStorageRootPath()) - } -} - func TestFsLockShared(t *testing.T) { - defer initTestDir()() - - fs, err := getFsDriver() + fs, err := getFsDriver(t) assert.Nil(t, err) assert.NotNil(t, fs) @@ -61,9 +53,7 @@ func TestFsLockShared(t *testing.T) { } func TestFsLockExclusive(t *testing.T) { - defer initTestDir()() - - fs, err := getFsDriver() + fs, err := getFsDriver(t) assert.Nil(t, err) assert.NotNil(t, fs) @@ -89,9 +79,7 @@ func TestFsLockExclusive(t *testing.T) { } func TestFsDriver(t *testing.T) { - defer initTestDir()() - - fs, err := getFsDriver() + fs, err := getFsDriver(t) assert.Nil(t, err) assert.NotNil(t, fs) @@ -162,12 +150,10 @@ func TestFsDriver(t *testing.T) { } func TestGlobalReadWrite(t *testing.T) { - defer initTestDir()() - relPath := "test/123/aaa.json" data := "hello this is testing global read write" - fs, err := getFsDriver() + fs, err := getFsDriver(t) assert.Nil(t, err) assert.NotNil(t, fs) diff --git a/src/runtime/virtcontainers/persist/fs/mockfs.go b/src/runtime/virtcontainers/persist/fs/mockfs.go index 4c4f49269..dca4acad4 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs.go @@ -36,7 +36,7 @@ func MockRunVMStoragePath() string { return filepath.Join(MockStorageRootPath(), vmPathSuffix) } -func MockFSInit() (persistapi.PersistDriver, error) { +func MockFSInit(rootPath string) (persistapi.PersistDriver, error) { driver, err := Init() if err != nil { return nil, fmt.Errorf("Could not create Mock FS driver: %v", err) @@ -47,7 +47,7 @@ func MockFSInit() (persistapi.PersistDriver, error) { return nil, fmt.Errorf("Could not create Mock FS driver") } - fsDriver.storageRootPath = MockStorageRootPath() + fsDriver.storageRootPath = rootPath fsDriver.driverName = "mockfs" return &MockFS{fsDriver}, nil @@ -55,7 +55,7 @@ func MockFSInit() (persistapi.PersistDriver, error) { func MockAutoInit() (persistapi.PersistDriver, error) { if mockTesting { - return MockFSInit() + return MockFSInit(MockStorageRootPath()) } return nil, nil } diff --git a/src/runtime/virtcontainers/persist/fs/mockfs_test.go b/src/runtime/virtcontainers/persist/fs/mockfs_test.go index 99709a78e..9cd583289 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs_test.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs_test.go @@ -28,7 +28,7 @@ func TestMockAutoInit(t *testing.T) { mockTesting = true fsd, err = MockAutoInit() assert.NoError(err) - expectedFS, err := MockFSInit() + expectedFS, err := MockFSInit(MockStorageRootPath()) assert.NoError(err) assert.Equal(expectedFS, fsd) } From 1b931f420339f8849e98517b38ba609a7fa7c9d3 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 15:12:49 +1000 Subject: [PATCH 17/22] runtime: Allock mockfs storage to be placed in any directory Currently EnableMockTesting() takes no arguments and will always place the mock storage in the fixed location /tmp/vc/mockfs. This means that one test run can interfere with the next one if anything isn't cleaned up (and there are other bugs which means that happens). If if those were fixed this would allow developers testing on the same machine to interfere with each other. So, allow the mockfs to be placed at an arbitrary place given as a parameter to EnableMockTesting(). In TestMain() we place it under our existing temporary directory, so we don't need any additional cleanup just for the mockfs. fixes #4140 Signed-off-by: David Gibson --- src/runtime/virtcontainers/persist/fs/mockfs.go | 14 ++++++++------ .../virtcontainers/persist/fs/mockfs_test.go | 8 ++++---- src/runtime/virtcontainers/virtcontainers_test.go | 6 ++---- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/runtime/virtcontainers/persist/fs/mockfs.go b/src/runtime/virtcontainers/persist/fs/mockfs.go index dca4acad4..18045f1ee 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs.go @@ -7,25 +7,27 @@ package fs import ( "fmt" - "os" "path/filepath" persistapi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/api" ) -var mockTesting = false +var mockRootPath = "" type MockFS struct { // inherit from FS. Overwrite if needed. *FS } -func EnableMockTesting() { - mockTesting = true +func EnableMockTesting(rootPath string) { + mockRootPath = rootPath } func MockStorageRootPath() string { - return filepath.Join(os.TempDir(), "vc", "mockfs") + if mockRootPath == "" { + panic("Using uninitialized mock storage root path") + } + return mockRootPath } func MockRunStoragePath() string { @@ -54,7 +56,7 @@ func MockFSInit(rootPath string) (persistapi.PersistDriver, error) { } func MockAutoInit() (persistapi.PersistDriver, error) { - if mockTesting { + if mockRootPath != "" { return MockFSInit(MockStorageRootPath()) } return nil, nil diff --git a/src/runtime/virtcontainers/persist/fs/mockfs_test.go b/src/runtime/virtcontainers/persist/fs/mockfs_test.go index 9cd583289..f2c1abd76 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs_test.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs_test.go @@ -13,19 +13,19 @@ import ( func TestMockAutoInit(t *testing.T) { assert := assert.New(t) - orgMockTesting := mockTesting + orgMockRootPath := mockRootPath defer func() { - mockTesting = orgMockTesting + mockRootPath = orgMockRootPath }() - mockTesting = false + mockRootPath = "" fsd, err := MockAutoInit() assert.Nil(fsd) assert.NoError(err) // Testing mock driver - mockTesting = true + mockRootPath = t.TempDir() fsd, err = MockAutoInit() assert.NoError(err) expectedFS, err := MockFSInit(MockStorageRootPath()) diff --git a/src/runtime/virtcontainers/virtcontainers_test.go b/src/runtime/virtcontainers/virtcontainers_test.go index 6a3d7fa58..b7117000c 100644 --- a/src/runtime/virtcontainers/virtcontainers_test.go +++ b/src/runtime/virtcontainers/virtcontainers_test.go @@ -57,8 +57,6 @@ var testHyperstartTtySocket = "" // cleanUp Removes any stale sandbox/container state that can affect // the next test to run. func cleanUp() { - os.RemoveAll(fs.MockRunStoragePath()) - os.RemoveAll(fs.MockRunVMStoragePath()) syscall.Unmount(GetSharePath(testSandboxID), syscall.MNT_DETACH|UmountNoFollow) os.RemoveAll(testDir) os.MkdirAll(testDir, DirMode) @@ -107,8 +105,6 @@ func setupClh() { func TestMain(m *testing.M) { var err error - fs.EnableMockTesting() - flag.Parse() logger := logrus.NewEntry(logrus.New()) @@ -125,6 +121,8 @@ func TestMain(m *testing.M) { panic(err) } + fs.EnableMockTesting(filepath.Join(testDir, "mockfs")) + fmt.Printf("INFO: Creating virtcontainers test directory %s\n", testDir) err = os.MkdirAll(testDir, DirMode) if err != nil { From 02395027816a0ac4e287a1e4d9c7a72dc2a6f8cd Mon Sep 17 00:00:00 2001 From: holyfei Date: Wed, 20 Apr 2022 15:04:54 +0800 Subject: [PATCH 18/22] agent: modify the type of swappiness to u64 The type of MemorySwappiness in runtime is uint64, and the type of swappiness in agent is int64, if we set max uint64 in runtime and pass it to agent, the value will be equal to -1. We should modify the type of swappiness to u64 Fixes: #4123 Signed-off-by: holyfei --- src/agent/rustjail/src/cgroups/fs/mod.rs | 2 +- src/agent/rustjail/src/lib.rs | 2 +- src/libs/oci/src/lib.rs | 2 +- src/tools/agent-ctl/src/utils.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/agent/rustjail/src/cgroups/fs/mod.rs b/src/agent/rustjail/src/cgroups/fs/mod.rs index 035bb8a6b..84aa9bc50 100644 --- a/src/agent/rustjail/src/cgroups/fs/mod.rs +++ b/src/agent/rustjail/src/cgroups/fs/mod.rs @@ -391,7 +391,7 @@ fn set_memory_resources(cg: &cgroups::Cgroup, memory: &LinuxMemory, update: bool if let Some(swappiness) = memory.swappiness { if (0..=100).contains(&swappiness) { - mem_controller.set_swappiness(swappiness as u64)?; + mem_controller.set_swappiness(swappiness)?; } else { return Err(anyhow!( "invalid value:{}. valid memory swappiness range is 0-100", diff --git a/src/agent/rustjail/src/lib.rs b/src/agent/rustjail/src/lib.rs index 3861bce48..14985ad35 100644 --- a/src/agent/rustjail/src/lib.rs +++ b/src/agent/rustjail/src/lib.rs @@ -265,7 +265,7 @@ pub fn resources_grpc_to_oci(res: &grpc::LinuxResources) -> oci::LinuxResources swap: Some(mem.Swap), kernel: Some(mem.Kernel), kernel_tcp: Some(mem.KernelTCP), - swappiness: Some(mem.Swappiness as i64), + swappiness: Some(mem.Swappiness), disable_oom_killer: Some(mem.DisableOOMKiller), }) } else { diff --git a/src/libs/oci/src/lib.rs b/src/libs/oci/src/lib.rs index 69a2a98e1..f47f2df4b 100644 --- a/src/libs/oci/src/lib.rs +++ b/src/libs/oci/src/lib.rs @@ -381,7 +381,7 @@ pub struct LinuxMemory { #[serde(default, skip_serializing_if = "Option::is_none", rename = "kernelTCP")] pub kernel_tcp: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub swappiness: Option, + pub swappiness: Option, #[serde( default, skip_serializing_if = "Option::is_none", diff --git a/src/tools/agent-ctl/src/utils.rs b/src/tools/agent-ctl/src/utils.rs index 8dbfa53f4..064b54486 100644 --- a/src/tools/agent-ctl/src/utils.rs +++ b/src/tools/agent-ctl/src/utils.rs @@ -400,7 +400,7 @@ fn memory_oci_to_ttrpc( Swap: mem.swap.unwrap_or(0), Kernel: mem.kernel.unwrap_or(0), KernelTCP: mem.kernel_tcp.unwrap_or(0), - Swappiness: mem.swappiness.unwrap_or(0) as u64, + Swappiness: mem.swappiness.unwrap_or(0), DisableOOMKiller: mem.disable_oom_killer.unwrap_or(false), unknown_fields: protobuf::UnknownFields::new(), cached_size: protobuf::CachedSize::default(), From 96bc3ec2e984e3a62923a20b715867894c91706b Mon Sep 17 00:00:00 2001 From: Garrett Mahin Date: Fri, 22 Apr 2022 19:20:04 -0500 Subject: [PATCH 19/22] rustjail: Add tests for hooks_grpc_to_oci Add test coverage for hooks_grpc_to_oci in rustjail/src/lib.rs Fixes: #4142 Signed-off-by: Garrett Mahin --- src/agent/rustjail/src/lib.rs | 167 ++++++++++++++++++++++++++++++++++ 1 file changed, 167 insertions(+) diff --git a/src/agent/rustjail/src/lib.rs b/src/agent/rustjail/src/lib.rs index 3861bce48..631fdfc16 100644 --- a/src/agent/rustjail/src/lib.rs +++ b/src/agent/rustjail/src/lib.rs @@ -737,4 +737,171 @@ mod tests { assert_eq!(d.result, result, "{}", msg); } } + + #[test] + fn test_hooks_grpc_to_oci() { + #[derive(Debug)] + struct TestData { + grpchooks: grpc::Hooks, + result: oci::Hooks, + } + + let tests = &[ + TestData { + // Default fields + grpchooks: grpc::Hooks { + ..Default::default() + }, + result: oci::Hooks { + ..Default::default() + }, + }, + TestData { + // All specified + grpchooks: grpc::Hooks { + Prestart: protobuf::RepeatedField::from(Vec::from([ + grpc::Hook { + Path: String::from("prestartpath"), + Args: protobuf::RepeatedField::from(Vec::from([ + String::from("arg1"), + String::from("arg2"), + ])), + Env: protobuf::RepeatedField::from(Vec::from([ + String::from("env1"), + String::from("env2"), + ])), + Timeout: 10, + ..Default::default() + }, + grpc::Hook { + Path: String::from("prestartpath2"), + Args: protobuf::RepeatedField::from(Vec::from([ + String::from("arg3"), + String::from("arg4"), + ])), + Env: protobuf::RepeatedField::from(Vec::from([ + String::from("env3"), + String::from("env4"), + ])), + Timeout: 25, + ..Default::default() + }, + ])), + Poststart: protobuf::RepeatedField::from(Vec::from([grpc::Hook { + Path: String::from("poststartpath"), + Args: protobuf::RepeatedField::from(Vec::from([ + String::from("arg1"), + String::from("arg2"), + ])), + Env: protobuf::RepeatedField::from(Vec::from([ + String::from("env1"), + String::from("env2"), + ])), + Timeout: 10, + ..Default::default() + }])), + Poststop: protobuf::RepeatedField::from(Vec::from([grpc::Hook { + Path: String::from("poststoppath"), + Args: protobuf::RepeatedField::from(Vec::from([ + String::from("arg1"), + String::from("arg2"), + ])), + Env: protobuf::RepeatedField::from(Vec::from([ + String::from("env1"), + String::from("env2"), + ])), + Timeout: 10, + ..Default::default() + }])), + ..Default::default() + }, + result: oci::Hooks { + prestart: Vec::from([ + oci::Hook { + path: String::from("prestartpath"), + args: Vec::from([String::from("arg1"), String::from("arg2")]), + env: Vec::from([String::from("env1"), String::from("env2")]), + timeout: Some(10), + }, + oci::Hook { + path: String::from("prestartpath2"), + args: Vec::from([String::from("arg3"), String::from("arg4")]), + env: Vec::from([String::from("env3"), String::from("env4")]), + timeout: Some(25), + }, + ]), + poststart: Vec::from([oci::Hook { + path: String::from("poststartpath"), + args: Vec::from([String::from("arg1"), String::from("arg2")]), + env: Vec::from([String::from("env1"), String::from("env2")]), + timeout: Some(10), + }]), + poststop: Vec::from([oci::Hook { + path: String::from("poststoppath"), + args: Vec::from([String::from("arg1"), String::from("arg2")]), + env: Vec::from([String::from("env1"), String::from("env2")]), + timeout: Some(10), + }]), + }, + }, + TestData { + // Prestart empty + grpchooks: grpc::Hooks { + Prestart: protobuf::RepeatedField::from(Vec::from([])), + Poststart: protobuf::RepeatedField::from(Vec::from([grpc::Hook { + Path: String::from("poststartpath"), + Args: protobuf::RepeatedField::from(Vec::from([ + String::from("arg1"), + String::from("arg2"), + ])), + Env: protobuf::RepeatedField::from(Vec::from([ + String::from("env1"), + String::from("env2"), + ])), + Timeout: 10, + ..Default::default() + }])), + Poststop: protobuf::RepeatedField::from(Vec::from([grpc::Hook { + Path: String::from("poststoppath"), + Args: protobuf::RepeatedField::from(Vec::from([ + String::from("arg1"), + String::from("arg2"), + ])), + Env: protobuf::RepeatedField::from(Vec::from([ + String::from("env1"), + String::from("env2"), + ])), + Timeout: 10, + ..Default::default() + }])), + ..Default::default() + }, + result: oci::Hooks { + prestart: Vec::from([]), + poststart: Vec::from([oci::Hook { + path: String::from("poststartpath"), + args: Vec::from([String::from("arg1"), String::from("arg2")]), + env: Vec::from([String::from("env1"), String::from("env2")]), + timeout: Some(10), + }]), + poststop: Vec::from([oci::Hook { + path: String::from("poststoppath"), + args: Vec::from([String::from("arg1"), String::from("arg2")]), + env: Vec::from([String::from("env1"), String::from("env2")]), + timeout: Some(10), + }]), + }, + }, + ]; + + for (i, d) in tests.iter().enumerate() { + let msg = format!("test[{}]: {:?}", i, d); + + let result = hooks_grpc_to_oci(&d.grpchooks); + + let msg = format!("{}, result: {:?}", msg, result); + + assert_eq!(d.result, result, "{}", msg); + } + } } From 4b9e78b8373b10ea457b6080f83d27929e78d3cc Mon Sep 17 00:00:00 2001 From: Garrett Mahin Date: Mon, 18 Apr 2022 23:25:14 -0500 Subject: [PATCH 20/22] rustjail: Add tests for mount_grpc_to_oci Add test coverage for mount_grpc_to_oci in rustjail/src/lib.rs Fixes: #4106 Signed-off-by: Garrett Mahin --- src/agent/rustjail/src/lib.rs | 94 +++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/src/agent/rustjail/src/lib.rs b/src/agent/rustjail/src/lib.rs index d6f433096..4d1b06a27 100644 --- a/src/agent/rustjail/src/lib.rs +++ b/src/agent/rustjail/src/lib.rs @@ -904,4 +904,98 @@ mod tests { assert_eq!(d.result, result, "{}", msg); } } + + #[test] + fn test_mount_grpc_to_oci() { + #[derive(Debug)] + struct TestData { + grpcmount: grpc::Mount, + result: oci::Mount, + } + + let tests = &[ + TestData { + // Default fields + grpcmount: grpc::Mount { + ..Default::default() + }, + result: oci::Mount { + ..Default::default() + }, + }, + TestData { + grpcmount: grpc::Mount { + destination: String::from("destination"), + source: String::from("source"), + field_type: String::from("fieldtype"), + options: protobuf::RepeatedField::from(Vec::from([ + String::from("option1"), + String::from("option2"), + ])), + ..Default::default() + }, + result: oci::Mount { + destination: String::from("destination"), + source: String::from("source"), + r#type: String::from("fieldtype"), + options: Vec::from([String::from("option1"), String::from("option2")]), + }, + }, + TestData { + grpcmount: grpc::Mount { + destination: String::from("destination"), + source: String::from("source"), + field_type: String::from("fieldtype"), + options: protobuf::RepeatedField::from(Vec::new()), + ..Default::default() + }, + result: oci::Mount { + destination: String::from("destination"), + source: String::from("source"), + r#type: String::from("fieldtype"), + options: Vec::new(), + }, + }, + TestData { + grpcmount: grpc::Mount { + destination: String::new(), + source: String::from("source"), + field_type: String::from("fieldtype"), + options: protobuf::RepeatedField::from(Vec::from([String::from("option1")])), + ..Default::default() + }, + result: oci::Mount { + destination: String::new(), + source: String::from("source"), + r#type: String::from("fieldtype"), + options: Vec::from([String::from("option1")]), + }, + }, + TestData { + grpcmount: grpc::Mount { + destination: String::from("destination"), + source: String::from("source"), + field_type: String::new(), + options: protobuf::RepeatedField::from(Vec::from([String::from("option1")])), + ..Default::default() + }, + result: oci::Mount { + destination: String::from("destination"), + source: String::from("source"), + r#type: String::new(), + options: Vec::from([String::from("option1")]), + }, + }, + ]; + + for (i, d) in tests.iter().enumerate() { + let msg = format!("test[{}]: {:?}", i, d); + + let result = mount_grpc_to_oci(&d.grpcmount); + + let msg = format!("{}, result: {:?}", msg, result); + + assert_eq!(d.result, result, "{}", msg); + } + } } From afbd60da2787fc739c677633dc1c967d9dd08184 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 21 Apr 2022 16:47:41 +0200 Subject: [PATCH 21/22] packaging: Fix clh build from source fall-back If we fail to download the clh binary, we fall-back to build from source. Unfortunately, `pull_clh_released_binary()` leaves a `cloud_hypervisor` directory behind, which causes `build_clh_from_source()` not to clone the git repo: [ -d "${repo_dir}" ] || git clone "${cloud_hypervisor_repo}" When building from a kata-containers git repo, the subsequent calls to `git` in this function thus apply to the kata-containers repo and eventually fail, e.g.: + git checkout v23.0 error: pathspec 'v23.0' did not match any file(s) known to git It doesn't quite make sense actually to keep an existing directory the content of which is arbitrary when we want to it to contain a specific version of clh. Just remove it instead. Fixes: #4151 Signed-off-by: Greg Kurz --- .../static-build/cloud-hypervisor/build-static-clh.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/packaging/static-build/cloud-hypervisor/build-static-clh.sh b/tools/packaging/static-build/cloud-hypervisor/build-static-clh.sh index ff26822cc..0691c199d 100755 --- a/tools/packaging/static-build/cloud-hypervisor/build-static-clh.sh +++ b/tools/packaging/static-build/cloud-hypervisor/build-static-clh.sh @@ -55,7 +55,8 @@ build_clh_from_source() { info "Build ${cloud_hypervisor_repo} version: ${cloud_hypervisor_version}" repo_dir=$(basename "${cloud_hypervisor_repo}") repo_dir="${repo_dir//.git}" - [ -d "${repo_dir}" ] || git clone "${cloud_hypervisor_repo}" + rm -rf "${repo_dir}" + git clone "${cloud_hypervisor_repo}" pushd "${repo_dir}" if [ -n "${cloud_hypervisor_pr}" ]; then From b658dccc5f336da3c264a5944ce968ef6f03df9b Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 21 Apr 2022 15:39:16 +0200 Subject: [PATCH 22/22] tools: fix typo in clh directory name This allows to get released binaries again. Fixes: #4151 Signed-off-by: Greg Kurz --- .../packaging/static-build/cloud-hypervisor/build-static-clh.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/packaging/static-build/cloud-hypervisor/build-static-clh.sh b/tools/packaging/static-build/cloud-hypervisor/build-static-clh.sh index 0691c199d..f1883ace7 100755 --- a/tools/packaging/static-build/cloud-hypervisor/build-static-clh.sh +++ b/tools/packaging/static-build/cloud-hypervisor/build-static-clh.sh @@ -48,7 +48,7 @@ pull_clh_released_binary() { curl --fail -L ${cloud_hypervisor_binary} -o cloud-hypervisor-static || return 1 mkdir -p cloud-hypervisor mv -f cloud-hypervisor-static cloud-hypervisor/cloud-hypervisor - chmod +x cloud_hypervisor/cloud-hypervisor + chmod +x cloud-hypervisor/cloud-hypervisor } build_clh_from_source() {