From 2dae179bfafc8f918e805dfd11ba889ba1681186 Mon Sep 17 00:00:00 2001 From: jlanson Date: Mon, 9 Dec 2024 10:48:12 -0500 Subject: [PATCH] fix: get proper version from download manifest and spawn with canonical path Discovered a bug in the entry selection process from the plugin's download manifest, such that the desired version was not checked and the first entry with a matching arch was returned. Additionally, downloaded plugin.kdl files have entrypoints that are relative to the parent dir, but Hipcheck core does not change working directory for the process so we don't find the right plugin binary. This commit prepends the proper plugin cache dir to PATH when resolving the binary to execute so that the `git` plugin's `git` binary will be found before the actual version control software. This commit also passes entrypoint args properly to the plugin's Command object. Signed-off-by: jlanson --- config/local.Hipcheck.kdl | 54 +++++++++++++++++++++ hipcheck/src/engine.rs | 8 +++- hipcheck/src/main.rs | 8 ++-- hipcheck/src/plugin/manager.rs | 66 ++++++++++++++++++++------ hipcheck/src/plugin/plugin_manifest.rs | 4 +- hipcheck/src/plugin/retrieval.rs | 14 ++++-- hipcheck/src/plugin/types.rs | 2 + 7 files changed, 132 insertions(+), 24 deletions(-) create mode 100644 config/local.Hipcheck.kdl diff --git a/config/local.Hipcheck.kdl b/config/local.Hipcheck.kdl new file mode 100644 index 00000000..569a59b3 --- /dev/null +++ b/config/local.Hipcheck.kdl @@ -0,0 +1,54 @@ +plugins { + plugin "mitre/activity" version="0.1.0" manifest="plugins/activity/local-plugin.kdl" + plugin "mitre/affiliation" version="0.1.0" manifest="plugins/affiliation/local-plugin.kdl" + plugin "mitre/binary" version="0.1.0" manifest="plugins/binary/local-plugin.kdl" + plugin "mitre/churn" version="0.1.0" manifest="plugins/churn/local-plugin.kdl" + plugin "mitre/entropy" version="0.1.0" manifest="plugins/entropy/local-plugin.kdl" + plugin "mitre/fuzz" version="0.1.1" manifest="plugins/fuzz/local-plugin.kdl" + plugin "mitre/review" version="0.1.0" manifest="plugins/review/local-plugin.kdl" + plugin "mitre/typo" version="0.1.0" manifest="plugins/typo/local-plugin.kdl" +} + +patch { + plugin "mitre/github" { + api-token-var "HC_GITHUB_TOKEN" + } +} + +analyze { + investigate policy="(gt 0.5 $)" + investigate-if-fail "mitre/typo" "mitre/binary" + + category "practices" { + analysis "mitre/activity" policy="(lte $ P52w)" weight=3 + analysis "mitre/binary" { + binary-file "./config/Binary.toml" + binary-file-threshold 0 + } + analysis "mitre/fuzz" policy="(eq #t $)" + analysis "mitre/review" policy="(lte (divz (count (filter (eq #f) $)) (count $)) 0.05)" + } + + category "attacks" { + analysis "mitre/typo" { + typo-file "./config/Typos.toml" + count-threshold 0 + } + + category "commit" { + analysis "mitre/affiliation" { + orgs-file "./plugins/affiliation/test/example_orgs.kdl" + count-threshold 0 + } + + analysis "mitre/entropy" policy="(eq 0 (count (filter (gt 8.0) $)))" { + langs-file "./config/Langs.toml" + entropy-threshold 10.0 + commit-percentage 0.0 + } + analysis "mitre/churn" policy="(lte (divz (count (filter (gt 3) $)) (count $)) 0.02)" { + langs-file "./config/Langs.toml" + } + } + } +} diff --git a/hipcheck/src/engine.rs b/hipcheck/src/engine.rs index 51408f5c..d4514d98 100644 --- a/hipcheck/src/engine.rs +++ b/hipcheck/src/engine.rs @@ -239,7 +239,12 @@ pub fn start_plugins( let mut plugins = vec![]; for plugin_id in required_plugin_names.iter() { - let plugin_manifest = PluginManifest::from_file(plugin_cache.plugin_kdl(plugin_id))?; + let plugin_kdl = plugin_cache.plugin_kdl(plugin_id); + let working_dir = plugin_kdl + .parent() + .expect("The plugin.kdl is always in the plugin cache") + .to_owned(); + let plugin_manifest = PluginManifest::from_file(plugin_kdl)?; let entrypoint = plugin_manifest .get_entrypoint(¤t_arch) .ok_or_else(|| { @@ -252,6 +257,7 @@ pub fn start_plugins( let plugin = Plugin { name: plugin_id.to_policy_file_plugin_identifier(), + working_dir, entrypoint, }; diff --git a/hipcheck/src/main.rs b/hipcheck/src/main.rs index 1ef6c5be..b3304ed0 100644 --- a/hipcheck/src/main.rs +++ b/hipcheck/src/main.rs @@ -489,16 +489,18 @@ fn cmd_plugin(args: PluginArgs) { use std::sync::Arc; use tokio::task::JoinSet; - let tgt_dir = "./target/debug"; + let working_dir = PathBuf::from("./target/debug"); - let entrypoint1 = pathbuf![tgt_dir, "dummy_rand_data"]; - let entrypoint2 = pathbuf![tgt_dir, "dummy_sha256"]; + let entrypoint1 = pathbuf!["dummy_rand_data"]; + let entrypoint2 = pathbuf!["dummy_sha256"]; let plugin1 = Plugin { name: "dummy/rand_data".to_owned(), + working_dir: working_dir.clone(), entrypoint: entrypoint1.display().to_string(), }; let plugin2 = Plugin { name: "dummy/sha256".to_owned(), + working_dir: working_dir.clone(), entrypoint: entrypoint2.display().to_string(), }; let plugin_executor = PluginExecutor::new( diff --git a/hipcheck/src/plugin/manager.rs b/hipcheck/src/plugin/manager.rs index a4064ad8..142ea0ca 100644 --- a/hipcheck/src/plugin/manager.rs +++ b/hipcheck/src/plugin/manager.rs @@ -8,7 +8,7 @@ use crate::{ use futures::future::join_all; use hipcheck_common::proto::plugin_service_client::PluginServiceClient; use rand::Rng; -use std::{ops::Range, process::Command}; +use std::{ffi::OsString, ops::Range, path::Path, process::Command}; use tokio::time::{sleep_until, Duration, Instant}; #[derive(Clone, Debug)] @@ -76,31 +76,69 @@ impl PluginExecutor { // port to become unavailable between our check and the plugin's bind attempt. // Hence the need for subsequent attempts if we get unlucky - log::debug!("Starting plugin '{}'", plugin.name); - // `entrypoint` is a string that represents a CLI invocation which may contain // arguments, so we have to split off only the first token - if let Some(bin_path) = try_get_bin_for_entrypoint(&plugin.entrypoint).0 { - if which::which(bin_path).is_err() { - log::warn!( - "Binary '{}' used to spawn {} does not exist, spawn is unlikely to succeed", - bin_path, - plugin.name - ); - } - } + let (opt_bin_path_str, args) = try_get_bin_for_entrypoint(&plugin.entrypoint); + let Some(bin_path_str) = opt_bin_path_str else { + return Err(hc_error!( + "Unable to get bin path for plugin entrypoint '{}'", + &plugin.entrypoint + )); + }; + + // Entrypoints are often "" which can overlap with existing binaries on the + // system like "git", "npm", so we must search for from within the plugin + // cache subfolder. First, grab the existing path. + let Some(mut os_paths) = + std::env::var_os("PATH").map(|s| std::env::split_paths(&s).collect::>()) + else { + return Err(hc_error!("Unable to get system PATH var")); + }; + + let Ok(canon_working_dir) = plugin.working_dir.canonicalize() else { + return Err(hc_error!( + "Failed to canonicalize plugin working dir: {:?}", + &plugin.working_dir + )); + }; + + // Add canonicalized plugin cache dir to temp PATH + os_paths.insert(0, canon_working_dir.clone()); + let search_path = std::env::join_paths(os_paths).unwrap(); + + // Find the binary_str using temp PATH + let Ok(canon_bin_path) = which::which_in::<&str, &OsString, &Path>( + bin_path_str, + Some(&search_path), + canon_working_dir.as_ref(), + ) else { + return Err(hc_error!( + "Failed to find binary '{}' for plugin", + bin_path_str + )); + }; + + log::debug!( + "Starting plugin '{}' at '{:?}'", + plugin.name, + &canon_bin_path + ); let mut spawn_attempts: usize = 0; while spawn_attempts < self.max_spawn_attempts { + let mut spawn_args = args.clone(); + // Find free port for process. Don't retry if we fail since this means all // ports in the desired range are already bound let port = self.get_available_port()?; let port_str = port.to_string(); + spawn_args.push("--port"); + spawn_args.push(port_str.as_str()); // Spawn plugin process log::debug!("Spawning '{}' on port {}", &plugin.entrypoint, port_str); - let Ok(mut proc) = Command::new(&plugin.entrypoint) - .args(["--port", port_str.as_str()]) + let Ok(mut proc) = Command::new(&canon_bin_path) + .args(spawn_args) // @Temporary - directly forward stdout/stderr from plugin to shell .stdout(std::io::stdout()) .stderr(std::io::stderr()) diff --git a/hipcheck/src/plugin/plugin_manifest.rs b/hipcheck/src/plugin/plugin_manifest.rs index bae3d360..f15c9d5f 100644 --- a/hipcheck/src/plugin/plugin_manifest.rs +++ b/hipcheck/src/plugin/plugin_manifest.rs @@ -288,7 +288,7 @@ impl PluginManifest { let mut new_entrypoint = new_bin_path.to_string_lossy().to_string(); if args.is_empty().not() { new_entrypoint.push(' '); - new_entrypoint.push_str(&args); + new_entrypoint.push_str(&args.join(" ")); } self.set_entrypoint(arch.clone(), new_entrypoint); @@ -347,7 +347,7 @@ impl FromStr for PluginManifest { } } -pub fn try_get_bin_for_entrypoint(entrypoint: &str) -> (Option<&str>, String) { +pub fn try_get_bin_for_entrypoint(entrypoint: &str) -> (Option<&str>, Vec<&str>) { let mut split = entrypoint.split_whitespace(); (split.next(), split.collect()) } diff --git a/hipcheck/src/plugin/retrieval.rs b/hipcheck/src/plugin/retrieval.rs index 4d365900..99eb3ffa 100644 --- a/hipcheck/src/plugin/retrieval.rs +++ b/hipcheck/src/plugin/retrieval.rs @@ -59,7 +59,11 @@ fn retrieve_plugin( // TODO: if the plugin.kdl file for the plugin already exists, then should we skip the retrieval process? // if plugin_cache.plugin_kdl(&plugin_id).exists() - log::debug!("Retrieving Plugin ID: {:?}", plugin_id); + log::debug!( + "Retrieving Plugin ID {:?} from {:?}", + plugin_id, + manifest_location + ); let plugin_manifest = match manifest_location { Some(ManifestLocation::Url(plugin_url)) => { @@ -94,15 +98,17 @@ fn retrieve_plugin_from_network( plugin_cache: &HcPluginCache, ) -> Result { let current_arch = get_current_arch(); + let version = plugin_id.version(); let download_manifest = retrieve_download_manifest(plugin_url)?; for entry in &download_manifest.entries { - if entry.arch == current_arch { + if entry.arch == current_arch && version == &entry.version { return download_and_unpack_plugin(entry, plugin_id, plugin_cache); } } Err(hc_error!( - "Could not find download manifest entry for arch '{}'", - current_arch + "Could not find download manifest entry for arch '{}' with version '{}'", + current_arch, + version.0 )) } diff --git a/hipcheck/src/plugin/types.rs b/hipcheck/src/plugin/types.rs index ccf250d2..cc0e9b52 100644 --- a/hipcheck/src/plugin/types.rs +++ b/hipcheck/src/plugin/types.rs @@ -19,6 +19,7 @@ use std::{ convert::TryFrom, future::poll_fn, ops::Not as _, + path::PathBuf, pin::Pin, process::Child, result::Result as StdResult, @@ -32,6 +33,7 @@ pub type HcPluginClient = PluginServiceClient; #[derive(Clone, Debug)] pub struct Plugin { pub name: String, + pub working_dir: PathBuf, pub entrypoint: String, }