fix: get proper version from download manifest and spawn with canonic…

…al 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 <[email protected]>

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"
+			}
+        }
+    }
+}

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(&current_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,
 		};
 

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(

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 "<BIN_NAME>" which can overlap with existing binaries on the
+		// system like "git", "npm", so we must search for <BIN_NAME> 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::<Vec<_>>())
+		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())

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())
 }

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<PluginManifest, Error> {
 	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
 	))
 }
 

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<Channel>;
 #[derive(Clone, Debug)]
 pub struct Plugin {
 	pub name: String,
+	pub working_dir: PathBuf,
 	pub entrypoint: String,
 }