diff --git a/src/agent/src/config.rs b/src/agent/src/config.rs index bec90d401d..7c1bf2f55e 100644 --- a/src/agent/src/config.rs +++ b/src/agent/src/config.rs @@ -23,6 +23,7 @@ const LOG_VPORT_OPTION: &str = "agent.log_vport"; const CONTAINER_PIPE_SIZE_OPTION: &str = "agent.container_pipe_size"; const UNIFIED_CGROUP_HIERARCHY_OPTION: &str = "agent.unified_cgroup_hierarchy"; const CONFIG_FILE: &str = "agent.config_file"; +const CONTAINER_POLICY_FILE: &str = "agent.container_policy_file"; const DEFAULT_LOG_LEVEL: slog::Level = slog::Level::Info; const DEFAULT_HOTPLUG_TIMEOUT: time::Duration = time::Duration::from_secs(3); @@ -51,6 +52,11 @@ const ERR_INVALID_CONTAINER_PIPE_SIZE_PARAM: &str = "unable to parse container p const ERR_INVALID_CONTAINER_PIPE_SIZE_KEY: &str = "invalid container pipe size key name"; const ERR_INVALID_CONTAINER_PIPE_NEGATIVE: &str = "container pipe size should not be negative"; +const ERR_INVALID_CONTAINER_POLICY_PATH_VALUE: &str = "invalid container_policy_file value"; +const ERR_INVALID_CONTAINER_POLICY_PATH_KEY: &str = "invalid container_policy_file key"; +const ERR_INVALID_CONTAINER_POLICY_ABSOLUTE: &str = + "container_policy_file path must be an absolute file path"; + #[derive(Debug, Default, Deserialize)] pub struct EndpointsConfig { pub allowed: Vec, @@ -76,6 +82,7 @@ pub struct AgentConfig { pub tracing: bool, pub endpoints: AgentEndpoints, pub supports_seccomp: bool, + pub container_policy_path: String, } #[derive(Debug, Deserialize)] @@ -91,6 +98,7 @@ pub struct AgentConfigBuilder { pub unified_cgroup_hierarchy: Option, pub tracing: Option, pub endpoints: Option, + pub container_policy_path: Option, } macro_rules! config_override { @@ -152,6 +160,7 @@ impl Default for AgentConfig { tracing: false, endpoints: Default::default(), supports_seccomp: rpc::have_seccomp(), + container_policy_path: String::from(""), } } } @@ -180,6 +189,7 @@ impl FromStr for AgentConfig { config_override!(agent_config_builder, agent_config, server_addr); config_override!(agent_config_builder, agent_config, unified_cgroup_hierarchy); config_override!(agent_config_builder, agent_config, tracing); + config_override!(agent_config_builder, agent_config, container_policy_path); // Populate the allowed endpoints hash set, if we got any from the config file. if let Some(endpoints) = agent_config_builder.endpoints { @@ -267,6 +277,13 @@ impl AgentConfig { config.unified_cgroup_hierarchy, get_bool_value ); + + parse_cmdline_param!( + param, + CONTAINER_POLICY_FILE, + config.container_policy_path, + get_container_policy_path_value + ); } if let Ok(addr) = env::var(SERVER_ADDR_ENV_VAR) { @@ -420,6 +437,29 @@ fn get_container_pipe_size(param: &str) -> Result { Ok(value) } +#[instrument] +fn get_container_policy_path_value(param: &str) -> Result { + let fields: Vec<&str> = param.split('=').collect(); + + ensure!(!fields[0].is_empty(), ERR_INVALID_CONTAINER_POLICY_PATH_KEY); + ensure!(fields.len() == 2, ERR_INVALID_CONTAINER_POLICY_PATH_VALUE); + + let key = fields[0]; + ensure!( + key == CONTAINER_POLICY_FILE, + ERR_INVALID_CONTAINER_POLICY_PATH_KEY + ); + + let value = String::from(fields[1]); + ensure!(!value.is_empty(), ERR_INVALID_CONTAINER_POLICY_PATH_VALUE); + ensure!( + value.starts_with('/'), + ERR_INVALID_CONTAINER_POLICY_ABSOLUTE + ); + ensure!(!value.contains(".."), ERR_INVALID_CONTAINER_POLICY_ABSOLUTE); + Ok(value) +} + #[cfg(test)] mod tests { use super::*; @@ -462,6 +502,7 @@ mod tests { assert!(!config.dev_mode); assert_eq!(config.log_level, DEFAULT_LOG_LEVEL); assert_eq!(config.hotplug_timeout, DEFAULT_HOTPLUG_TIMEOUT); + assert_eq!(config.container_policy_path, ""); } #[test] @@ -480,6 +521,7 @@ mod tests { server_addr: &'a str, unified_cgroup_hierarchy: bool, tracing: bool, + container_policy_path: &'a str, } impl Default for TestData<'_> { @@ -495,6 +537,7 @@ mod tests { server_addr: TEST_SERVER_ADDR, unified_cgroup_hierarchy: false, tracing: false, + container_policy_path: "", } } } @@ -864,6 +907,11 @@ mod tests { tracing: true, ..Default::default() }, + TestData { + contents: "agent.container_policy_file=/etc/containers/policy.json", + container_policy_path: "/etc/containers/policy.json", + ..Default::default() + }, ]; let dir = tempdir().expect("failed to create tmpdir"); @@ -1347,6 +1395,72 @@ Caused by: } } + #[test] + fn test_get_container_policy_path_value() { + #[derive(Debug)] + struct TestData<'a> { + param: &'a str, + result: Result, + } + + let tests = &[ + TestData { + param: "", + result: Err(anyhow!(ERR_INVALID_CONTAINER_POLICY_PATH_KEY)), + }, + TestData { + param: "agent.container_policy_file", + result: Err(anyhow!(ERR_INVALID_CONTAINER_POLICY_PATH_VALUE)), + }, + TestData { + param: "agent.container_policy_file=", + result: Err(anyhow!(ERR_INVALID_CONTAINER_POLICY_PATH_VALUE)), + }, + TestData { + param: "foo=bar", + result: Err(anyhow!(ERR_INVALID_CONTAINER_POLICY_PATH_KEY)), + }, + TestData { + param: "agent.policy_path=/another/absolute/path.json", + result: Err(anyhow!(ERR_INVALID_CONTAINER_POLICY_PATH_KEY)), + }, + TestData { + param: "agent.container_policy_file=/etc/container/policy.json", + result: Ok("/etc/container/policy.json".into()), + }, + TestData { + param: "agent.container_policy_file=/another/absolute/path.json", + result: Ok("/another/absolute/path.json".into()), + }, + TestData { + param: "agent.container_policy_file=./relative/path.json", + result: Err(anyhow!(ERR_INVALID_CONTAINER_POLICY_ABSOLUTE)), + }, + TestData { + param: "agent.container_policy_file=./relative/path.json", + result: Err(anyhow!(ERR_INVALID_CONTAINER_POLICY_ABSOLUTE)), + }, + TestData { + param: "agent.container_policy_file=../../relative/path.json", + result: Err(anyhow!(ERR_INVALID_CONTAINER_POLICY_ABSOLUTE)), + }, + TestData { + param: "agent.container_policy_file=junk_string", + result: Err(anyhow!(ERR_INVALID_CONTAINER_POLICY_ABSOLUTE)), + }, + ]; + + for (i, d) in tests.iter().enumerate() { + let msg = format!("test[{}]: {:?}", i, d); + + let result = get_container_policy_path_value(d.param); + + let msg = format!("{}: result: {:?}", msg, result); + + assert_result!(d.result, result, msg); + } + } + #[test] fn test_config_builder_from_string() { let config = AgentConfig::from_str( diff --git a/src/agent/src/image_rpc.rs b/src/agent/src/image_rpc.rs index 8fd35f1d7e..f7b345ecb7 100644 --- a/src/agent/src/image_rpc.rs +++ b/src/agent/src/image_rpc.rs @@ -4,7 +4,7 @@ // use std::fs; -use std::path::PathBuf; +use std::path::Path; use std::process::{Command, ExitStatus}; use std::sync::Arc; @@ -16,6 +16,7 @@ use ttrpc::{self, error::get_rpc_status as ttrpc_error}; use crate::rpc::{verify_cid, CONTAINER_BASE}; use crate::sandbox::Sandbox; +use crate::AGENT_CONFIG; const SKOPEO_PATH: &str = "/usr/bin/skopeo"; const UMOCI_PATH: &str = "/usr/local/bin/umoci"; @@ -37,73 +38,69 @@ impl ImageService { Self { sandbox } } - fn build_oci_path(cid: &str) -> PathBuf { - let mut oci_path = PathBuf::from("/tmp"); - oci_path.push(cid); - oci_path.push(IMAGE_OCI); - oci_path - } - - fn pull_image_from_registry(image: &str, cid: &str, source_creds: &Option<&str>) -> Result<()> { + fn pull_image_from_registry( + image: &str, + cid: &str, + source_creds: &Option<&str>, + policy_path: &Option<&String>, + ) -> Result<()> { let source_image = format!("{}{}", "docker://", image); - let mut manifest_path = PathBuf::from("/tmp"); - manifest_path.push(cid); - manifest_path.push("image_manifest"); - let target_path_manifest = format!("dir://{}", manifest_path.to_string_lossy()); - - // Define the target transport and path for the OCI image, without signature - let oci_path = Self::build_oci_path(cid); + let tmp_cid_path = Path::new("/tmp/").join(cid); + let oci_path = tmp_cid_path.join(IMAGE_OCI); let target_path_oci = format!("oci://{}", oci_path.to_string_lossy()); - fs::create_dir_all(&manifest_path)?; fs::create_dir_all(&oci_path)?; info!(sl!(), "Attempting to pull image {}...", &source_image); let mut pull_command = Command::new(SKOPEO_PATH); pull_command - // TODO: need to create a proper policy - .arg("--insecure-policy") .arg("copy") .arg(source_image) - .arg(&target_path_manifest); + .arg(&target_path_oci) + .arg("--remove-signatures"); //umoci requires signatures to be removed + // If source credentials were passed (so not using an anonymous registry), pass them through if let Some(source_creds) = source_creds { pull_command.arg("--src-creds").arg(source_creds); } + // If a policy_path provided, use it, otherwise fall back to allow all image registries + if let Some(policy_path) = policy_path { + pull_command.arg("--policy").arg(policy_path); + } else { + info!( + sl!(), + "No policy path was supplied, so revert to allow all images to be pulled." + ); + pull_command.arg("--insecure-policy"); + } + + debug!(sl!(), "skopeo command: {:?}", &pull_command); let status: ExitStatus = pull_command.status()?; - ensure!( - status.success(), - "failed to copy image manifest: {:?}", - status, - ); - // Copy image from one local file-system to another - // Resulting image is still stored in manifest format, but no longer includes the signature - // The image with a signature can then be unpacked into a bundle - let status: ExitStatus = Command::new(SKOPEO_PATH) - .arg("--insecure-policy") - .arg("copy") - .arg(&target_path_manifest) - .arg(&target_path_oci) - .arg("--remove-signatures") - .status()?; + if !status.success() { + let mut error_message = format!("failed to pull image: {:?}", status); - ensure!(status.success(), "failed to copy image oci: {:?}", status); - - // To save space delete the manifest. - // TODO LATER - when verify image is added, this will need moving the end of that, if required - fs::remove_dir_all(&manifest_path)?; + if let Err(e) = fs::remove_dir_all(&tmp_cid_path) { + error_message.push_str(&format!( + " and clean up of temporary container directory {:?} failed with error {:?}", + tmp_cid_path, e + )); + }; + return Err(anyhow!(error_message)); + } Ok(()) } fn unpack_image(cid: &str) -> Result<()> { - let source_path_oci = Self::build_oci_path(cid); - let target_path_bundle = format!("{}{}{}", CONTAINER_BASE, "/", cid); + let tmp_cid_path = Path::new("/tmp/").join(cid); + let source_path_oci = tmp_cid_path.join(IMAGE_OCI); - info!(sl!(), "unpack image"; "cid" => cid, "target_bundle_path" => &target_path_bundle); + let target_path_bundle = Path::new(CONTAINER_BASE).join(cid); + + info!(sl!(), "unpack image {:?} to {:?}", cid, target_path_bundle); // Unpack image let status: ExitStatus = Command::new(UMOCI_PATH) @@ -116,7 +113,7 @@ impl ImageService { ensure!(status.success(), "failed to unpack image: {:?}", status); // To save space delete the oci image after unpack - fs::remove_dir_all(&source_path_oci)?; + fs::remove_dir_all(&tmp_cid_path)?; Ok(()) } @@ -138,7 +135,12 @@ impl ImageService { let source_creds = (!req.get_source_creds().is_empty()).then(|| req.get_source_creds()); - Self::pull_image_from_registry(image, cid, &source_creds)?; + // Read the policy path from the agent config + let config_policy_path = &AGENT_CONFIG.read().await.container_policy_path; + let policy_path = (!config_policy_path.is_empty()).then(|| config_policy_path); + info!(sl!(), "Using container policy_path: {:?}...", &policy_path); + + Self::pull_image_from_registry(image, cid, &source_creds, &policy_path)?; Self::unpack_image(cid)?; let mut sandbox = self.sandbox.lock().await;