From d1482c1b737ea5d6f6a63c668f18d644a7124b06 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 20 Feb 2026 11:47:52 +0000 Subject: [PATCH] Fix Windows command injection in validate_command_args - Remove `%` from safe characters list in `src/modules/mod.rs` to ensure it undergoes dangerous pattern check. - Add `%` (variable expansion) and `^` (shell escape) to `dangerous_patterns` list in `src/modules/mod.rs` to prevent Windows-specific injection and info disclosure. - Add regression tests in `tests/security_windows_command_injection.rs`. - Update security journal in `.jules/sentinel.md`. Co-authored-by: dolagoartur <146357947+dolagoartur@users.noreply.github.com> --- .jules/sentinel.md | 5 + src/modules/cloud/aws/ec2.rs | 1 - src/modules/cloud/aws/s3.rs | 6 +- src/modules/cloud/azure/vm.rs | 1 - src/modules/cloud/gcp/compute.rs | 1 - src/modules/cloud/kubernetes/secret.rs | 4 +- src/modules/docker/docker_container.rs | 4 +- src/modules/docker/docker_image.rs | 5 +- src/modules/docker/docker_network.rs | 4 +- src/modules/get_url.rs | 15 +-- src/modules/hpc/dcgm.rs | 68 +++--------- src/modules/hpc/fabric_manager.rs | 25 ++--- src/modules/hpc/gdrcopy.rs | 17 ++- src/modules/hpc/gpu.rs | 41 ++----- src/modules/hpc/ib_diagnostics.rs | 12 +- src/modules/hpc/ib_partition.rs | 115 +++++++++++--------- src/modules/hpc/ipmi.rs | 41 +++++-- src/modules/hpc/lmod.rs | 66 +++++------ src/modules/hpc/lustre_mount.rs | 59 +++++----- src/modules/hpc/mig_config.rs | 34 +++--- src/modules/hpc/mod.rs | 4 +- src/modules/hpc/nccl.rs | 35 +++--- src/modules/hpc/nvidia_container_toolkit.rs | 47 ++++---- src/modules/hpc/nvidia_driver.rs | 14 +-- src/modules/hpc/nvidia_peermem.rs | 30 ++--- src/modules/hpc/redfish.rs | 10 +- src/modules/hpc/sssd.rs | 35 ++---- src/modules/mod.rs | 17 ++- src/modules/shell.rs | 5 +- tests/security_windows_command_injection.rs | 47 ++++++++ 30 files changed, 365 insertions(+), 403 deletions(-) create mode 100644 tests/security_windows_command_injection.rs diff --git a/.jules/sentinel.md b/.jules/sentinel.md index bd250ada..4019e174 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -90,3 +90,8 @@ **Vulnerability:** The `CronModule` in `src/modules/cron.rs` was vulnerable to CRLF injection because it did not validate that the `name` (and other parameters like `job`) of a cron job were free of newline characters. This allowed an attacker to inject arbitrary lines into the crontab file, potentially creating malicious cron jobs running as the target user. **Learning:** When generating configuration files that are line-based (like crontabs), always validate user input for newline characters to prevent injection of new entries. **Prevention:** Implement strict validation for all parameters that are written to line-based configuration files. Use helper functions like `validate_no_newlines` to enforce this constraint consistently. + +## 2025-06-03 - Windows Command Injection via % and ^ +**Vulnerability:** The `validate_command_args` utility was too permissive for Windows environments. It allowed `%` (variable expansion) and `^` (shell escape). This enabled Information Disclosure (reading environment variables) and command obfuscation/filter bypass on Windows systems where commands are executed via `cmd.exe`. +**Learning:** Shell metacharacters vary significantly by platform. A validation logic that works for POSIX shells is insufficient for Windows `cmd.exe`, which has its own set of special characters (`%`, `^`). +**Prevention:** Explicitly block Windows-specific shell metacharacters (`%`, `^`) in validation routines intended to be cross-platform or Windows-compatible. diff --git a/src/modules/cloud/aws/ec2.rs b/src/modules/cloud/aws/ec2.rs index 0c5fc8bb..e9be6b58 100644 --- a/src/modules/cloud/aws/ec2.rs +++ b/src/modules/cloud/aws/ec2.rs @@ -113,7 +113,6 @@ pub enum InstanceState { Rebooted, } - impl InstanceState { fn from_str(s: &str) -> ModuleResult { match s.to_lowercase().as_str() { diff --git a/src/modules/cloud/aws/s3.rs b/src/modules/cloud/aws/s3.rs index 17972703..80d72900 100644 --- a/src/modules/cloud/aws/s3.rs +++ b/src/modules/cloud/aws/s3.rs @@ -282,11 +282,7 @@ pub enum ServerSideEncryption { impl ServerSideEncryption { fn from_str(s: &str) -> ModuleResult { - match s - .to_uppercase() - .replace(['-', ':'], "_") - .as_str() - { + match s.to_uppercase().replace(['-', ':'], "_").as_str() { "AES256" | "AES_256" | "SSE_S3" => Ok(ServerSideEncryption::Aes256), "AWS_KMS" | "AWSKMS" | "KMS" | "SSE_KMS" => Ok(ServerSideEncryption::AwsKms), "SSE_C" | "CUSTOMER" | "CUSTOMER_PROVIDED" => { diff --git a/src/modules/cloud/azure/vm.rs b/src/modules/cloud/azure/vm.rs index 29cc0a02..7ef362ed 100644 --- a/src/modules/cloud/azure/vm.rs +++ b/src/modules/cloud/azure/vm.rs @@ -127,7 +127,6 @@ pub enum VmState { Restarted, } - impl VmState { fn from_str(s: &str) -> ModuleResult { match s.to_lowercase().as_str() { diff --git a/src/modules/cloud/gcp/compute.rs b/src/modules/cloud/gcp/compute.rs index 75f60b38..f90c5f90 100644 --- a/src/modules/cloud/gcp/compute.rs +++ b/src/modules/cloud/gcp/compute.rs @@ -129,7 +129,6 @@ pub enum InstanceState { Reset, } - impl InstanceState { fn from_str(s: &str) -> ModuleResult { match s.to_lowercase().as_str() { diff --git a/src/modules/cloud/kubernetes/secret.rs b/src/modules/cloud/kubernetes/secret.rs index d6a0fa91..ea361c70 100644 --- a/src/modules/cloud/kubernetes/secret.rs +++ b/src/modules/cloud/kubernetes/secret.rs @@ -43,8 +43,8 @@ //! ``` use crate::modules::{ - Diff, Module, ModuleClassification, ModuleContext, ModuleOutput, ModuleParams, - ModuleResult, ParallelizationHint, ParamExt, + Diff, Module, ModuleClassification, ModuleContext, ModuleOutput, ModuleParams, ModuleResult, + ParallelizationHint, ParamExt, }; use serde_json::json; use std::collections::BTreeMap; diff --git a/src/modules/docker/docker_container.rs b/src/modules/docker/docker_container.rs index 18fc51a8..1f7910f2 100644 --- a/src/modules/docker/docker_container.rs +++ b/src/modules/docker/docker_container.rs @@ -33,8 +33,8 @@ #[cfg(feature = "docker")] use bollard::container::{ - Config, CreateContainerOptions, RemoveContainerOptions, - RestartContainerOptions, StartContainerOptions, StopContainerOptions, + Config, CreateContainerOptions, RemoveContainerOptions, RestartContainerOptions, + StartContainerOptions, StopContainerOptions, }; #[cfg(feature = "docker")] use bollard::models::{ diff --git a/src/modules/docker/docker_image.rs b/src/modules/docker/docker_image.rs index 9cff187c..36144b3a 100644 --- a/src/modules/docker/docker_image.rs +++ b/src/modules/docker/docker_image.rs @@ -22,9 +22,7 @@ //! - `repository`: Registry repository for push #[cfg(feature = "docker")] -use bollard::image::{ - BuildImageOptions, CreateImageOptions, RemoveImageOptions, TagImageOptions, -}; +use bollard::image::{BuildImageOptions, CreateImageOptions, RemoveImageOptions, TagImageOptions}; #[cfg(feature = "docker")] use bollard::Docker; #[cfg(feature = "docker")] @@ -273,7 +271,6 @@ impl DockerImageModule { /// Build image from Dockerfile async fn build_image(docker: &Docker, config: &ImageConfig) -> ModuleResult<()> { - use tar::Builder; let build_path = config.build.path.as_ref().ok_or_else(|| { diff --git a/src/modules/docker/docker_network.rs b/src/modules/docker/docker_network.rs index dca8ab74..f8da794d 100644 --- a/src/modules/docker/docker_network.rs +++ b/src/modules/docker/docker_network.rs @@ -23,9 +23,7 @@ #[cfg(feature = "docker")] use bollard::models::{Ipam, IpamConfig}; #[cfg(feature = "docker")] -use bollard::network::{ - ConnectNetworkOptions, CreateNetworkOptions, DisconnectNetworkOptions, -}; +use bollard::network::{ConnectNetworkOptions, CreateNetworkOptions, DisconnectNetworkOptions}; #[cfg(feature = "docker")] use bollard::Docker; diff --git a/src/modules/get_url.rs b/src/modules/get_url.rs index 60851a62..2349bb6b 100644 --- a/src/modules/get_url.rs +++ b/src/modules/get_url.rs @@ -183,11 +183,9 @@ impl Module for GetUrlModule { } } - let bytes = rt - .block_on(async { response.bytes().await }) - .map_err(|e| { - ModuleError::ExecutionFailed(format!("Failed to read response body: {}", e)) - })?; + let bytes = rt.block_on(async { response.bytes().await }).map_err(|e| { + ModuleError::ExecutionFailed(format!("Failed to read response body: {}", e)) + })?; // Verify checksum if provided if let Some(ref cksum) = checksum { @@ -220,10 +218,9 @@ impl Module for GetUrlModule { dest, bytes.len() )); - output.data.insert( - "dest".to_string(), - serde_json::Value::String(dest.clone()), - ); + output + .data + .insert("dest".to_string(), serde_json::Value::String(dest.clone())); output .data .insert("url".to_string(), serde_json::Value::String(url)); diff --git a/src/modules/hpc/dcgm.rs b/src/modules/hpc/dcgm.rs index ced5cc6f..8cdc5778 100644 --- a/src/modules/hpc/dcgm.rs +++ b/src/modules/hpc/dcgm.rs @@ -189,10 +189,7 @@ fn parse_dcgmi_diag(output: &str, level: u32) -> DcgmDiagResult { if parts.len() >= 3 { let test_name = parts[1].trim(); let result = parts[2].trim(); - if test_name.is_empty() - || test_name.starts_with('-') - || test_name.starts_with('=') - { + if test_name.is_empty() || test_name.starts_with('-') || test_name.starts_with('=') { continue; } if result.to_lowercase().contains("fail") { @@ -250,9 +247,7 @@ impl Module for DcgmModule { let health_watches = params.get_bool_or("health_watches", false); let diag_level = params.get_u32("diag_level")?; let exporter = params.get_bool_or("exporter", false); - let exporter_port = params - .get_u32("exporter_port")? - .unwrap_or(9400); + let exporter_port = params.get_u32("exporter_port")?.unwrap_or(9400); let exporter_counters = params.get_string("exporter_counters")?; validate_operation_mode(&operation_mode)?; @@ -262,11 +257,8 @@ impl Module for DcgmModule { // -- state=absent -- if state == "absent" { - let (installed, _, _) = run_cmd( - connection, - "command -v dcgmi >/dev/null 2>&1", - context, - )?; + let (installed, _, _) = + run_cmd(connection, "command -v dcgmi >/dev/null 2>&1", context)?; if !installed { return Ok(ModuleOutput::ok("DCGM is not installed")); @@ -276,16 +268,8 @@ impl Module for DcgmModule { return Ok(ModuleOutput::changed("Would remove DCGM packages")); } - let _ = run_cmd( - connection, - "systemctl stop nvidia-dcgm.service", - context, - ); - let _ = run_cmd( - connection, - "systemctl disable nvidia-dcgm.service", - context, - ); + let _ = run_cmd(connection, "systemctl stop nvidia-dcgm.service", context); + let _ = run_cmd(connection, "systemctl disable nvidia-dcgm.service", context); let remove_cmd = match os_family { "rhel" => "dnf remove -y datacenter-gpu-manager dcgm-exporter", @@ -299,11 +283,8 @@ impl Module for DcgmModule { // -- state=present -- // Step 1: Install DCGM - let (dcgm_installed, _, _) = run_cmd( - connection, - "command -v dcgmi >/dev/null 2>&1", - context, - )?; + let (dcgm_installed, _, _) = + run_cmd(connection, "command -v dcgmi >/dev/null 2>&1", context)?; if !dcgm_installed { if context.check_mode { @@ -355,11 +336,7 @@ impl Module for DcgmModule { context, )?; if svc_active { - run_cmd_ok( - connection, - "systemctl stop nvidia-dcgm.service", - context, - )?; + run_cmd_ok(connection, "systemctl stop nvidia-dcgm.service", context)?; changed = true; changes.push("Stopped nvidia-dcgm.service for embedded mode".to_string()); } @@ -386,11 +363,8 @@ impl Module for DcgmModule { changes.push(format!("Would run DCGM diagnostics level {}", level)); None } else { - let diag_stdout = run_cmd_ok( - connection, - &format!("dcgmi diag -r {}", level), - context, - )?; + let diag_stdout = + run_cmd_ok(connection, &format!("dcgmi diag -r {}", level), context)?; let result = parse_dcgmi_diag(&diag_stdout, level); if !result.passed { changes.push(format!( @@ -430,17 +404,11 @@ impl Module for DcgmModule { // Configure exporter port via environment override if !context.check_mode { - let env_line = format!( - "DCGM_EXPORTER_LISTEN=:{}", - exporter_port - ); + let env_line = format!("DCGM_EXPORTER_LISTEN=:{}", exporter_port); let env_file = "/etc/default/dcgm-exporter"; let mut env_content = env_line.clone(); if let Some(ref counters) = exporter_counters { - env_content.push_str(&format!( - "\nDCGM_EXPORTER_COLLECTORS={}", - counters - )); + env_content.push_str(&format!("\nDCGM_EXPORTER_COLLECTORS={}", counters)); } run_cmd_ok( connection, @@ -630,7 +598,10 @@ mod tests { #[test] fn test_detect_os_family() { - assert_eq!(detect_os_family("ID_LIKE=\"rhel centos fedora\""), Some("rhel")); + assert_eq!( + detect_os_family("ID_LIKE=\"rhel centos fedora\""), + Some("rhel") + ); assert_eq!(detect_os_family("ID=ubuntu\nVERSION=22.04"), Some("debian")); assert_eq!(detect_os_family("ID=freebsd"), None); } @@ -655,9 +626,6 @@ mod tests { }; let json = serde_json::to_value(&config).unwrap(); assert_eq!(json["port"], 9400); - assert_eq!( - json["counters_file"], - "/etc/dcgm-exporter/counters.csv" - ); + assert_eq!(json["counters_file"], "/etc/dcgm-exporter/counters.csv"); } } diff --git a/src/modules/hpc/fabric_manager.rs b/src/modules/hpc/fabric_manager.rs index 3ba8cbe0..3b9f8e7f 100644 --- a/src/modules/hpc/fabric_manager.rs +++ b/src/modules/hpc/fabric_manager.rs @@ -107,14 +107,7 @@ fn parse_driver_version(nvidia_smi_output: &str) -> Option { return None; } // Take only the first line in case of multi-GPU output - Some( - version - .lines() - .next() - .unwrap_or("") - .trim() - .to_string(), - ) + Some(version.lines().next().unwrap_or("").trim().to_string()) } /// Validate fabric mode parameter. @@ -297,11 +290,7 @@ impl Module for FabricManagerModule { let config_content = generate_fm_config(&fabric_mode, fault_tolerance, log_level); if !context.check_mode { - run_cmd_ok( - connection, - "mkdir -p /etc/nvidia-fabricmanager", - context, - )?; + run_cmd_ok(connection, "mkdir -p /etc/nvidia-fabricmanager", context)?; // Check if config differs let (exists, current_content, _) = run_cmd( @@ -375,10 +364,7 @@ impl Module for FabricManagerModule { }; let mut output = if changed { - ModuleOutput::changed(format!( - "Applied {} Fabric Manager changes", - changes.len() - )) + ModuleOutput::changed(format!("Applied {} Fabric Manager changes", changes.len())) } else { ModuleOutput::ok("Fabric Manager is installed and configured") }; @@ -467,7 +453,10 @@ mod tests { #[test] fn test_detect_os_family() { - assert_eq!(detect_os_family("ID_LIKE=\"rhel centos fedora\""), Some("rhel")); + assert_eq!( + detect_os_family("ID_LIKE=\"rhel centos fedora\""), + Some("rhel") + ); assert_eq!(detect_os_family("ID=ubuntu\nVERSION=22.04"), Some("debian")); assert_eq!(detect_os_family("ID=freebsd"), None); } diff --git a/src/modules/hpc/gdrcopy.rs b/src/modules/hpc/gdrcopy.rs index d311c69f..224c2df1 100644 --- a/src/modules/hpc/gdrcopy.rs +++ b/src/modules/hpc/gdrcopy.rs @@ -208,11 +208,7 @@ impl Module for GdrcopyModule { if context.check_mode { changes.push("Would remove /etc/modules-load.d/gdrdrv.conf".to_string()); } else { - run_cmd_ok( - connection, - "rm -f /etc/modules-load.d/gdrdrv.conf", - context, - )?; + run_cmd_ok(connection, "rm -f /etc/modules-load.d/gdrdrv.conf", context)?; changes.push("Removed /etc/modules-load.d/gdrdrv.conf".to_string()); } } @@ -237,10 +233,8 @@ impl Module for GdrcopyModule { .with_data("changes", serde_json::json!(changes))); } - return Ok( - ModuleOutput::changed("Removed GDRCopy") - .with_data("changes", serde_json::json!(changes)), - ); + return Ok(ModuleOutput::changed("Removed GDRCopy") + .with_data("changes", serde_json::json!(changes))); } // -- state=present -- @@ -443,7 +437,10 @@ gdrcopy_sanity: PASSED #[test] fn test_detect_os_family() { - assert_eq!(detect_os_family("ID_LIKE=\"rhel centos fedora\""), Some("rhel")); + assert_eq!( + detect_os_family("ID_LIKE=\"rhel centos fedora\""), + Some("rhel") + ); assert_eq!(detect_os_family("ID=ubuntu\nVERSION=22.04"), Some("debian")); assert_eq!(detect_os_family("ID=freebsd"), None); } diff --git a/src/modules/hpc/gpu.rs b/src/modules/hpc/gpu.rs index 9129edb1..d5636144 100644 --- a/src/modules/hpc/gpu.rs +++ b/src/modules/hpc/gpu.rs @@ -136,10 +136,7 @@ fn gpu_selector_arg(gpu_id: &Option) -> String { } fn parse_power_limit(value: &str) -> Option { - let token = value - .split_whitespace() - .next() - .unwrap_or(""); + let token = value.split_whitespace().next().unwrap_or(""); token.parse::().ok() } @@ -386,8 +383,7 @@ impl Module for NvidiaGpuModule { let current = self .query_single_value(connection, context, "persistence_mode", &gpu_id) .unwrap_or_default(); - let current_enabled = current.first() - .and_then(|value| parse_enabled_flag(value)); + let current_enabled = current.first().and_then(|value| parse_enabled_flag(value)); if current_enabled != Some(persistence_mode) { if context.check_mode { @@ -427,8 +423,8 @@ impl Module for NvidiaGpuModule { if !svc_active || !svc_enabled { if context.check_mode { - changes.push("Would enable and start nvidia-persistenced.service" - .to_string()); + changes + .push("Would enable and start nvidia-persistenced.service".to_string()); } else { run_cmd_ok( connection, @@ -436,9 +432,7 @@ impl Module for NvidiaGpuModule { context, )?; changed = true; - changes.push( - "Enabled and started nvidia-persistenced.service".to_string(), - ); + changes.push("Enabled and started nvidia-persistenced.service".to_string()); } } } else { @@ -471,8 +465,7 @@ impl Module for NvidiaGpuModule { let current = self .query_single_value(connection, context, "compute_mode", &gpu_id) .unwrap_or_default(); - let current_mode = current.first() - .map(|value| value.trim().to_lowercase()); + let current_mode = current.first().map(|value| value.trim().to_lowercase()); if current_mode.as_deref() != Some(desired_label.as_str()) { if context.check_mode { @@ -495,8 +488,7 @@ impl Module for NvidiaGpuModule { let current = self .query_single_value(connection, context, "ecc.mode.current", &gpu_id) .unwrap_or_default(); - let current_enabled = current.first() - .and_then(|value| parse_enabled_flag(value)); + let current_enabled = current.first().and_then(|value| parse_enabled_flag(value)); if current_enabled != Some(ecc_mode) { if context.check_mode { @@ -504,11 +496,7 @@ impl Module for NvidiaGpuModule { } else { run_cmd_ok( connection, - &format!( - "nvidia-smi{} -e {}", - selector, - if ecc_mode { 1 } else { 0 } - ), + &format!("nvidia-smi{} -e {}", selector, if ecc_mode { 1 } else { 0 }), context, )?; changed = true; @@ -523,8 +511,7 @@ impl Module for NvidiaGpuModule { let current = self .query_single_value(connection, context, "power.limit", &gpu_id) .unwrap_or_default(); - let current_limit = current.first() - .and_then(|value| parse_power_limit(value)); + let current_limit = current.first().and_then(|value| parse_power_limit(value)); let needs_update = current_limit .map(|limit| (limit - power_limit as f64).abs() > 0.1) @@ -532,10 +519,7 @@ impl Module for NvidiaGpuModule { if needs_update { if context.check_mode { - changes.push(format!( - "Would set power_limit={}{}", - power_limit, selector - )); + changes.push(format!("Would set power_limit={}{}", power_limit, selector)); } else { run_cmd_ok( connection, @@ -543,10 +527,7 @@ impl Module for NvidiaGpuModule { context, )?; changed = true; - changes.push(format!( - "Set power_limit={}{}", - power_limit, selector - )); + changes.push(format!("Set power_limit={}{}", power_limit, selector)); } } } diff --git a/src/modules/hpc/ib_diagnostics.rs b/src/modules/hpc/ib_diagnostics.rs index d86c62df..57c1d7b1 100644 --- a/src/modules/hpc/ib_diagnostics.rs +++ b/src/modules/hpc/ib_diagnostics.rs @@ -266,12 +266,9 @@ impl Module for IbDiagnosticsModule { .get_string("output_dir")? .unwrap_or_else(|| "/tmp/ib_diagnostics".to_string()); let auto_drain = params.get_bool_or("auto_drain", false); - let threshold_symbol_errors = params - .get_u32("threshold_symbol_errors")? - .unwrap_or(10) as u64; - let threshold_link_downed = params - .get_u32("threshold_link_downed")? - .unwrap_or(5) as u64; + let threshold_symbol_errors = + params.get_u32("threshold_symbol_errors")?.unwrap_or(10) as u64; + let threshold_link_downed = params.get_u32("threshold_link_downed")?.unwrap_or(5) as u64; let drain_reason = params .get_string("drain_reason")? .unwrap_or_else(|| "IB diagnostics threshold exceeded".to_string()); @@ -348,8 +345,7 @@ impl Module for IbDiagnosticsModule { .map(|output| parse_counter_values(output)) .unwrap_or_default(); - let (overall_verdict, counter_verdicts) = - generate_verdict(&counter_values, &thresholds); + let (overall_verdict, counter_verdicts) = generate_verdict(&counter_values, &thresholds); // Optionally trigger drain let drain_cmd = if auto_drain && overall_verdict == "fail" { diff --git a/src/modules/hpc/ib_partition.rs b/src/modules/hpc/ib_partition.rs index 2535aeeb..57e2b988 100644 --- a/src/modules/hpc/ib_partition.rs +++ b/src/modules/hpc/ib_partition.rs @@ -250,10 +250,7 @@ fn detect_pkey_conflicts(entries: &[PartitionEntry]) -> PreflightResult { } for (pkey, count) in &pkey_counts { if *count > 1 { - errors.push(format!( - "Duplicate pkey {} appears {} times", - pkey, count - )); + errors.push(format!("Duplicate pkey {} appears {} times", pkey, count)); } } @@ -514,9 +511,12 @@ impl Module for IbPartitionModule { return Ok( ModuleOutput::ok(format!("Partition key {} not present", pkey)) .with_data("pkey", serde_json::json!(pkey)) - .with_data("diagnostics", serde_json::json!({ - "preflight_warnings": preflight_warnings, - })), + .with_data( + "diagnostics", + serde_json::json!({ + "preflight_warnings": preflight_warnings, + }), + ), ); } @@ -524,9 +524,12 @@ impl Module for IbPartitionModule { return Ok( ModuleOutput::changed(format!("Would remove partition key {}", pkey)) .with_data("pkey", serde_json::json!(pkey)) - .with_data("diagnostics", serde_json::json!({ - "preflight_warnings": preflight_warnings, - })), + .with_data( + "diagnostics", + serde_json::json!({ + "preflight_warnings": preflight_warnings, + }), + ), ); } @@ -553,9 +556,12 @@ impl Module for IbPartitionModule { return Ok( ModuleOutput::changed(format!("Removed partition key {}", pkey)) .with_data("pkey", serde_json::json!(pkey)) - .with_data("diagnostics", serde_json::json!({ - "preflight_warnings": preflight_warnings, - })), + .with_data( + "diagnostics", + serde_json::json!({ + "preflight_warnings": preflight_warnings, + }), + ), ); } @@ -589,27 +595,31 @@ impl Module for IbPartitionModule { return Ok( ModuleOutput::ok(format!("Partition key {} already configured", pkey)) .with_data("pkey", serde_json::json!(pkey)) - .with_data("diagnostics", serde_json::json!({ - "preflight_warnings": full_preflight.warnings, - "drift": serde_json::json!([]), - })), + .with_data( + "diagnostics", + serde_json::json!({ + "preflight_warnings": full_preflight.warnings, + "drift": serde_json::json!([]), + }), + ), ); } // There is drift -- update the entry. if context.check_mode { - return Ok( - ModuleOutput::changed(format!( - "Would update partition key {} ({} change(s))", - pkey, - drift.len() - )) - .with_data("pkey", serde_json::json!(pkey)) - .with_data("diagnostics", serde_json::json!({ + return Ok(ModuleOutput::changed(format!( + "Would update partition key {} ({} change(s))", + pkey, + drift.len() + )) + .with_data("pkey", serde_json::json!(pkey)) + .with_data( + "diagnostics", + serde_json::json!({ "preflight_warnings": full_preflight.warnings, "drift": drift, - })), - ); + }), + )); } // Rewrite the config with the desired entry replacing the current one. @@ -636,19 +646,20 @@ impl Module for IbPartitionModule { context, )?; - return Ok( - ModuleOutput::changed(format!( - "Updated partition key {} ({} change(s))", - pkey, - drift.len() - )) - .with_data("pkey", serde_json::json!(pkey)) - .with_data("members", serde_json::json!(members)) - .with_data("diagnostics", serde_json::json!({ + return Ok(ModuleOutput::changed(format!( + "Updated partition key {} ({} change(s))", + pkey, + drift.len() + )) + .with_data("pkey", serde_json::json!(pkey)) + .with_data("members", serde_json::json!(members)) + .with_data( + "diagnostics", + serde_json::json!({ "preflight_warnings": full_preflight.warnings, "drift": drift, - })), - ); + }), + )); } // Entry does not exist -- add it. @@ -656,9 +667,12 @@ impl Module for IbPartitionModule { return Ok( ModuleOutput::changed(format!("Would add partition key {}", pkey)) .with_data("pkey", serde_json::json!(pkey)) - .with_data("diagnostics", serde_json::json!({ - "preflight_warnings": full_preflight.warnings, - })), + .with_data( + "diagnostics", + serde_json::json!({ + "preflight_warnings": full_preflight.warnings, + }), + ), ); } @@ -674,9 +688,12 @@ impl Module for IbPartitionModule { ModuleOutput::changed(format!("Added partition key {}", pkey)) .with_data("pkey", serde_json::json!(pkey)) .with_data("members", serde_json::json!(members)) - .with_data("diagnostics", serde_json::json!({ - "preflight_warnings": full_preflight.warnings, - })), + .with_data( + "diagnostics", + serde_json::json!({ + "preflight_warnings": full_preflight.warnings, + }), + ), ) } @@ -1002,18 +1019,12 @@ MyPartition=0x8001, ipoib : guid1=full ; let current = PartitionEntry { pkey: "0x8001".to_string(), ipoib: false, - members: vec![ - "guid1=full".to_string(), - "guid2=limited".to_string(), - ], + members: vec!["guid1=full".to_string(), "guid2=limited".to_string()], }; let desired = PartitionEntry { pkey: "0x8001".to_string(), ipoib: true, - members: vec![ - "guid1=limited".to_string(), - "guid3=full".to_string(), - ], + members: vec!["guid1=limited".to_string(), "guid3=full".to_string()], }; let drift = reconcile_members(¤t, &desired); // ipoib change + guid1 access change + guid3 add + guid2 remove = 4 diff --git a/src/modules/hpc/ipmi.rs b/src/modules/hpc/ipmi.rs index 77372e8b..97366e08 100644 --- a/src/modules/hpc/ipmi.rs +++ b/src/modules/hpc/ipmi.rs @@ -154,7 +154,10 @@ fn check_bmc_reachability( PreflightResult { passed: false, warnings: Vec::new(), - errors: vec![format!("BMC network unreachable (transient): {}", stderr.trim())], + errors: vec![format!( + "BMC network unreachable (transient): {}", + stderr.trim() + )], } } else if lower.contains("password") || lower.contains("auth") { PreflightResult { @@ -168,7 +171,10 @@ fn check_bmc_reachability( } else { PreflightResult { passed: false, - warnings: vec![format!("BMC probe failed with unknown error: {}", stderr.trim())], + warnings: vec![format!( + "BMC probe failed with unknown error: {}", + stderr.trim() + )], errors: vec![format!("BMC unreachable: {}", stderr.trim())], } } @@ -362,13 +368,21 @@ impl Module for IpmiPowerModule { // Execute power action with retry let cmd = format!("{} chassis power {}", base, action); - let (success, _stdout, stderr, retries_needed) = - run_cmd_with_retry(connection, &cmd, context, "power action", max_retries, initial_delay_ms)?; + let (success, _stdout, stderr, retries_needed) = run_cmd_with_retry( + connection, + &cmd, + context, + "power action", + max_retries, + initial_delay_ms, + )?; if !success { return Err(ModuleError::ExecutionFailed(format!( "Power {} failed after {} retries: {}", - action, retries_needed, stderr.trim() + action, + retries_needed, + stderr.trim() ))); } @@ -512,13 +526,20 @@ impl Module for IpmiBootModule { } // Execute bootdev command with retry - let (success, _stdout, stderr, retries_needed) = - run_cmd_with_retry(connection, &cmd, context, "bootdev", max_retries, initial_delay_ms)?; + let (success, _stdout, stderr, retries_needed) = run_cmd_with_retry( + connection, + &cmd, + context, + "bootdev", + max_retries, + initial_delay_ms, + )?; if !success { return Err(ModuleError::ExecutionFailed(format!( "Boot device set failed after {} retries: {}", - retries_needed, stderr.trim() + retries_needed, + stderr.trim() ))); } @@ -669,8 +690,8 @@ mod tests { ]; for msg in &network_errors { let lower = msg.to_lowercase(); - let is_transient = lower.contains("unable to establish") - || lower.contains("connection timed out"); + let is_transient = + lower.contains("unable to establish") || lower.contains("connection timed out"); assert!( is_transient, "Expected transient classification for: {}", diff --git a/src/modules/hpc/lmod.rs b/src/modules/hpc/lmod.rs index 7a39af1d..b6bce378 100644 --- a/src/modules/hpc/lmod.rs +++ b/src/modules/hpc/lmod.rs @@ -139,10 +139,7 @@ fn configure_hierarchical_modulepath( let escaped = lmodrc_content.replace('\'', "'\\''"); run_cmd_ok( connection, - &format!( - "printf '%s\\n' '{}' > /etc/lmod/lmodrc.lua", - escaped - ), + &format!("printf '%s\\n' '{}' > /etc/lmod/lmodrc.lua", escaped), context, )?; } @@ -152,8 +149,7 @@ fn configure_hierarchical_modulepath( for base in modulepaths { for level in hierarchy { let dir = format!("{}/{}", base, level); - let (exists, _, _) = - run_cmd(connection, &format!("test -d '{}'", dir), context)?; + let (exists, _, _) = run_cmd(connection, &format!("test -d '{}'", dir), context)?; if !exists { if !context.check_mode { run_cmd_ok(connection, &format!("mkdir -p '{}'", dir), context)?; @@ -183,10 +179,7 @@ fn enforce_version_policies( Some(v) => v, None => continue, }; - let version_content = format!( - "#%Module\nset ModulesVersion \"{}\"", - version - ); + let version_content = format!("#%Module\nset ModulesVersion \"{}\"", version); // Write .version in the first modulepath that contains the module dir, // or the first modulepath by default. @@ -199,11 +192,7 @@ fn enforce_version_policies( let version_file = format!("{}/.version", module_dir); if !context.check_mode { - run_cmd_ok( - connection, - &format!("mkdir -p '{}'", module_dir), - context, - )?; + run_cmd_ok(connection, &format!("mkdir -p '{}'", module_dir), context)?; let escaped = version_content.replace('\'', "'\\''"); run_cmd_ok( connection, @@ -233,8 +222,7 @@ fn detect_modulepath_drift( // Check each configured directory exists for dir in modulepaths { - let (exists, _, _) = - run_cmd(connection, &format!("test -d '{}'", dir), context)?; + let (exists, _, _) = run_cmd(connection, &format!("test -d '{}'", dir), context)?; if !exists { drift.push(DriftItem { field: format!("modulepath_dir:{}", dir), @@ -505,12 +493,8 @@ impl Module for LmodModule { if let Some(dv_value) = params.get("default_versions") { if let Some(dv_map) = dv_value.as_object() { if !dv_map.is_empty() { - let ver_changes = enforce_version_policies( - connection, - context, - &effective_paths, - dv_map, - )?; + let ver_changes = + enforce_version_policies(connection, context, &effective_paths, dv_map)?; if !ver_changes.is_empty() { changed = true; changes.extend(ver_changes); @@ -552,8 +536,14 @@ impl Module for LmodModule { .with_data("changes", serde_json::json!(changes)) .with_data("os_family", serde_json::json!(os_family)) .with_data("modulepath_drift", serde_json::json!(drift)) - .with_data("hierarchy_configured", serde_json::json!(hierarchy_configured)) - .with_data("default_versions_set", serde_json::json!(default_versions_set))); + .with_data( + "hierarchy_configured", + serde_json::json!(hierarchy_configured), + ) + .with_data( + "default_versions_set", + serde_json::json!(default_versions_set), + )); } if changed { @@ -562,15 +552,27 @@ impl Module for LmodModule { .with_data("changes", serde_json::json!(changes)) .with_data("os_family", serde_json::json!(os_family)) .with_data("modulepath_drift", serde_json::json!(drift)) - .with_data("hierarchy_configured", serde_json::json!(hierarchy_configured)) - .with_data("default_versions_set", serde_json::json!(default_versions_set)), + .with_data( + "hierarchy_configured", + serde_json::json!(hierarchy_configured), + ) + .with_data( + "default_versions_set", + serde_json::json!(default_versions_set), + ), ) } else { Ok(ModuleOutput::ok("Lmod is configured and up to date") .with_data("os_family", serde_json::json!(os_family)) .with_data("modulepath_drift", serde_json::json!(drift)) - .with_data("hierarchy_configured", serde_json::json!(hierarchy_configured)) - .with_data("default_versions_set", serde_json::json!(default_versions_set))) + .with_data( + "hierarchy_configured", + serde_json::json!(hierarchy_configured), + ) + .with_data( + "default_versions_set", + serde_json::json!(default_versions_set), + )) } } @@ -688,9 +690,11 @@ mod tests { #[test] fn test_drift_detection_missing_dir() { // Simulate drift items for directories that would be missing - let configured_paths = ["/opt/modulefiles".to_string(), + let configured_paths = [ + "/opt/modulefiles".to_string(), "/missing/path".to_string(), - "/also/missing".to_string()]; + "/also/missing".to_string(), + ]; // Simulate checking: first exists, second and third do not let dir_exists = [true, false, false]; diff --git a/src/modules/hpc/lustre_mount.rs b/src/modules/hpc/lustre_mount.rs index f402db34..60abc63c 100644 --- a/src/modules/hpc/lustre_mount.rs +++ b/src/modules/hpc/lustre_mount.rs @@ -96,10 +96,7 @@ fn run_cmd_ok( /// Valid formats: `@` where network is `tcp`, `tcp1`, `o2ib`, `o2ib0`, etc. /// Examples: `10.0.0.1@tcp`, `192.168.1.100@o2ib`, `10.0.0.1@tcp0` fn is_valid_nid(nid: &str) -> bool { - let re = Regex::new( - r"^(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})@(tcp|o2ib)\d*$", - ) - .unwrap(); + let re = Regex::new(r"^(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})@(tcp|o2ib)\d*$").unwrap(); if !re.is_match(nid) { return false; } @@ -320,15 +317,9 @@ fn remount_with_rollback( let rollback_cmd = format!("mount -o remount,'{}' '{}'", prev_opts, mount_point); let (rollback_ok, _, rollback_stderr) = run_cmd(connection, &rollback_cmd, context)?; if rollback_ok { - details.push(format!( - "Rolled back to previous options: {}", - prev_opts - )); + details.push(format!("Rolled back to previous options: {}", prev_opts)); } else { - details.push(format!( - "Rollback also failed: {}", - rollback_stderr.trim() - )); + details.push(format!("Rollback also failed: {}", rollback_stderr.trim())); } } @@ -375,10 +366,7 @@ fn mount_health_output( for line in stat_stdout.lines() { let trimmed = line.trim(); if trimmed.contains("Type:") { - health.insert( - "stat_type".to_string(), - serde_json::json!(trimmed), - ); + health.insert("stat_type".to_string(), serde_json::json!(trimmed)); break; } } @@ -667,8 +655,13 @@ impl LustreMountModule { // Fstab convergence check and management if manage_fstab { - let drift = - fstab_convergence_check(connection, context, lustre_source, mount_point, mount_options)?; + let drift = fstab_convergence_check( + connection, + context, + lustre_source, + mount_point, + mount_options, + )?; if !drift.is_empty() { output_data.insert("fstab_drift".to_string(), serde_json::json!(drift)); } @@ -707,11 +700,14 @@ impl LustreMountModule { } } else if mount_options != "defaults" { // Already mounted -- check if options need a remount - let drift_items = - fstab_convergence_check(connection, context, lustre_source, mount_point, mount_options)?; - let options_drifted = drift_items - .iter() - .any(|d| d.field == "mount_options"); + let drift_items = fstab_convergence_check( + connection, + context, + lustre_source, + mount_point, + mount_options, + )?; + let options_drifted = drift_items.iter().any(|d| d.field == "mount_options"); if options_drifted && !context.check_mode { let (remount_ok, remount_details) = remount_with_rollback(connection, context, mount_point, mount_options)?; @@ -796,13 +792,17 @@ impl LustreMountModule { if mp_in_fstab { // Entry exists for mount point -- check for drift - let drift = - fstab_convergence_check(connection, context, lustre_source, mount_point, mount_options)?; + let drift = fstab_convergence_check( + connection, + context, + lustre_source, + mount_point, + mount_options, + )?; if !drift.is_empty() { if context.check_mode { - let drift_fields: Vec = - drift.iter().map(|d| d.field.clone()).collect(); + let drift_fields: Vec = drift.iter().map(|d| d.field.clone()).collect(); changes.push(format!( "Would update fstab entry for {} (drifted fields: {})", mount_point, @@ -973,10 +973,7 @@ mod tests { health.insert("use_percent".to_string(), serde_json::json!("50%")); health.insert("mounted_on".to_string(), serde_json::json!("/mnt/scratch")); health.insert("accessible".to_string(), serde_json::json!(true)); - health.insert( - "mount_point".to_string(), - serde_json::json!("/mnt/scratch"), - ); + health.insert("mount_point".to_string(), serde_json::json!("/mnt/scratch")); let value = serde_json::Value::Object(health); assert!(value.is_object()); diff --git a/src/modules/hpc/mig_config.rs b/src/modules/hpc/mig_config.rs index e7d5b773..7105d3a6 100644 --- a/src/modules/hpc/mig_config.rs +++ b/src/modules/hpc/mig_config.rs @@ -110,12 +110,20 @@ fn parse_gpu_instances(output: &str) -> Vec { let mut instances = Vec::new(); for line in output.lines() { let line = line.trim(); - if !line.starts_with('|') || line.contains("GPU instances") || line.contains("GPU Name") || line.contains("ID") { + if !line.starts_with('|') + || line.contains("GPU instances") + || line.contains("GPU Name") + || line.contains("ID") + { continue; } // Strip leading/trailing '|' and parse columns let inner = line.trim_matches('|').trim(); - if inner.starts_with('=') || inner.starts_with('-') || inner.starts_with('+') || inner.is_empty() { + if inner.starts_with('=') + || inner.starts_with('-') + || inner.starts_with('+') + || inner.is_empty() + { continue; } let parts: Vec<&str> = inner.split_whitespace().collect(); @@ -179,16 +187,8 @@ fn validate_profile(profile: &str) -> bool { if parts.len() != 2 { return false; } - let slice_ok = parts[0].ends_with('g') - && parts[0] - .trim_end_matches('g') - .parse::() - .is_ok(); - let mem_ok = parts[1].ends_with("gb") - && parts[1] - .trim_end_matches("gb") - .parse::() - .is_ok(); + let slice_ok = parts[0].ends_with('g') && parts[0].trim_end_matches('g').parse::().is_ok(); + let mem_ok = parts[1].ends_with("gb") && parts[1].trim_end_matches("gb").parse::().is_ok(); slice_ok && mem_ok } @@ -354,15 +354,9 @@ impl Module for MigConfigModule { // Create new instances let profiles_arg = profile_list.join(","); let create_cmd = if auto_create_compute { - format!( - "nvidia-smi mig -cgi {} -C -i {}", - profiles_arg, gpu_id - ) + format!("nvidia-smi mig -cgi {} -C -i {}", profiles_arg, gpu_id) } else { - format!( - "nvidia-smi mig -cgi {} -i {}", - profiles_arg, gpu_id - ) + format!("nvidia-smi mig -cgi {} -i {}", profiles_arg, gpu_id) }; run_cmd_ok(connection, &create_cmd, context)?; diff --git a/src/modules/hpc/mod.rs b/src/modules/hpc/mod.rs index 8fb730ca..1cf82eaf 100644 --- a/src/modules/hpc/mod.rs +++ b/src/modules/hpc/mod.rs @@ -154,12 +154,12 @@ pub use cuda::CudaToolkitModule; #[cfg(feature = "gpu")] pub use dcgm::DcgmModule; pub use discovery::HpcDiscoveryModule; +#[cfg(feature = "gpu")] +pub use fabric_manager::FabricManagerModule; pub use facts::HpcFactsModule; #[cfg(feature = "parallel_fs")] pub use fs::{BeegfsClientModule, LustreClientModule}; #[cfg(feature = "gpu")] -pub use fabric_manager::FabricManagerModule; -#[cfg(feature = "gpu")] pub use gdrcopy::GdrcopyModule; #[cfg(feature = "gpu")] pub use gpu::NvidiaGpuModule; diff --git a/src/modules/hpc/nccl.rs b/src/modules/hpc/nccl.rs index d7de18cf..82d48ecf 100644 --- a/src/modules/hpc/nccl.rs +++ b/src/modules/hpc/nccl.rs @@ -174,14 +174,7 @@ fn parse_nccl_version(output: &str) -> Option { return None; } // Take the first line only - Some( - version - .lines() - .next() - .unwrap_or("") - .trim() - .to_string(), - ) + Some(version.lines().next().unwrap_or("").trim().to_string()) } // ---- NCCL Module ---- @@ -234,14 +227,18 @@ impl Module for NcclModule { let remove_cmd = match os_family { "rhel" => "dnf remove -y libnccl libnccl-devel", - _ => "DEBIAN_FRONTEND=noninteractive apt-get remove --purge -y libnccl2 libnccl-dev", + _ => { + "DEBIAN_FRONTEND=noninteractive apt-get remove --purge -y libnccl2 libnccl-dev" + } }; let _ = run_cmd(connection, remove_cmd, context); let _ = run_cmd(connection, "rm -f /etc/nccl.conf", context); return Ok( - ModuleOutput::changed("Removed NCCL packages and configuration") - .with_data("changes", serde_json::json!(["Removed NCCL packages", "Removed /etc/nccl.conf"])), + ModuleOutput::changed("Removed NCCL packages and configuration").with_data( + "changes", + serde_json::json!(["Removed NCCL packages", "Removed /etc/nccl.conf"]), + ), ); } @@ -320,7 +317,11 @@ impl Module for NcclModule { "all_reduce_perf -b 8 -e 128M -f 2 -g 1 2>&1", context, )?; - let combined = if ok { stdout } else { format!("{}\n{}", stdout, stderr) }; + let combined = if ok { + stdout + } else { + format!("{}\n{}", stdout, stderr) + }; let result = parse_nccl_test_output(&combined); if result.passed { if let Some(bw) = result.bandwidth_gb_s { @@ -479,10 +480,7 @@ mod tests { parse_nccl_version("2.29.3-1+cuda12.9"), Some("2.29.3-1+cuda12.9".to_string()) ); - assert_eq!( - parse_nccl_version("2.29.3\n"), - Some("2.29.3".to_string()) - ); + assert_eq!(parse_nccl_version("2.29.3\n"), Some("2.29.3".to_string())); assert_eq!(parse_nccl_version(""), None); assert_eq!( parse_nccl_version(" 2.29.3 \n"), @@ -492,7 +490,10 @@ mod tests { #[test] fn test_detect_os_family() { - assert_eq!(detect_os_family("ID_LIKE=\"rhel centos fedora\""), Some("rhel")); + assert_eq!( + detect_os_family("ID_LIKE=\"rhel centos fedora\""), + Some("rhel") + ); assert_eq!(detect_os_family("ID=ubuntu\nVERSION=22.04"), Some("debian")); assert_eq!(detect_os_family("ID=freebsd"), None); } diff --git a/src/modules/hpc/nvidia_container_toolkit.rs b/src/modules/hpc/nvidia_container_toolkit.rs index f44f332e..103f753f 100644 --- a/src/modules/hpc/nvidia_container_toolkit.rs +++ b/src/modules/hpc/nvidia_container_toolkit.rs @@ -201,14 +201,13 @@ impl Module for NvidiaContainerToolkitModule { // -- state=absent -- if state == "absent" { - let (installed, _, _) = run_cmd( - connection, - "command -v nvidia-ctk >/dev/null 2>&1", - context, - )?; + let (installed, _, _) = + run_cmd(connection, "command -v nvidia-ctk >/dev/null 2>&1", context)?; if !installed { - return Ok(ModuleOutput::ok("NVIDIA Container Toolkit is not installed")); + return Ok(ModuleOutput::ok( + "NVIDIA Container Toolkit is not installed", + )); } if context.check_mode { @@ -274,11 +273,8 @@ impl Module for NvidiaContainerToolkitModule { } // Step 2: Install toolkit - let (toolkit_installed, _, _) = run_cmd( - connection, - "command -v nvidia-ctk >/dev/null 2>&1", - context, - )?; + let (toolkit_installed, _, _) = + run_cmd(connection, "command -v nvidia-ctk >/dev/null 2>&1", context)?; if !toolkit_installed { if context.check_mode { @@ -286,7 +282,9 @@ impl Module for NvidiaContainerToolkitModule { } else { let install_cmd = match os_family { "rhel" => "dnf install -y nvidia-container-toolkit", - _ => "DEBIAN_FRONTEND=noninteractive apt-get install -y nvidia-container-toolkit", + _ => { + "DEBIAN_FRONTEND=noninteractive apt-get install -y nvidia-container-toolkit" + } }; run_cmd_ok(connection, install_cmd, context)?; changed = true; @@ -296,10 +294,7 @@ impl Module for NvidiaContainerToolkitModule { // Step 3: Configure runtime if !context.check_mode { - let configure_cmd = format!( - "nvidia-ctk runtime configure --runtime={}", - runtime - ); + let configure_cmd = format!("nvidia-ctk runtime configure --runtime={}", runtime); let (ok, _, _) = run_cmd(connection, &configure_cmd, context)?; if ok { changes.push(format!("Configured {} runtime for NVIDIA", runtime)); @@ -316,7 +311,10 @@ impl Module for NvidiaContainerToolkitModule { context, )?; changed = true; - changes.push(format!("Applied custom toolkit config from {}", config_path)); + changes.push(format!( + "Applied custom toolkit config from {}", + config_path + )); } // Restart runtime service @@ -345,7 +343,13 @@ impl Module for NvidiaContainerToolkitModule { } else { run_cmd_ok( connection, - &format!("mkdir -p {}", cdi_output.rsplit_once('/').map(|(d, _)| d).unwrap_or("/etc/cdi")), + &format!( + "mkdir -p {}", + cdi_output + .rsplit_once('/') + .map(|(d, _)| d) + .unwrap_or("/etc/cdi") + ), context, )?; let (ok, _, _) = run_cmd( @@ -362,11 +366,8 @@ impl Module for NvidiaContainerToolkitModule { // Step 5: Collect status info let status = if !context.check_mode { - let (ok, stdout, _) = run_cmd( - connection, - "nvidia-container-cli info 2>/dev/null", - context, - )?; + let (ok, stdout, _) = + run_cmd(connection, "nvidia-container-cli info 2>/dev/null", context)?; if ok { let mut s = parse_container_cli_info(&stdout); s.runtime = runtime.clone(); diff --git a/src/modules/hpc/nvidia_driver.rs b/src/modules/hpc/nvidia_driver.rs index 9ce7605b..fbf3e52d 100644 --- a/src/modules/hpc/nvidia_driver.rs +++ b/src/modules/hpc/nvidia_driver.rs @@ -253,8 +253,7 @@ fn orchestrate_dkms_rebuild( let (install_ok, install_stdout, install_stderr) = run_cmd(connection, &install_cmd, context)?; if install_ok { - details - .push(format!("DKMS install succeeded for nvidia/{}", version)); + details.push(format!("DKMS install succeeded for nvidia/{}", version)); } else { verified = false; warnings.push(format!( @@ -849,7 +848,9 @@ impl Module for NvidiaDriverModule { changes.len() )))) } else { - Ok(build_output(ModuleOutput::ok("NVIDIA driver is configured"))) + Ok(build_output(ModuleOutput::ok( + "NVIDIA driver is configured", + ))) } } @@ -950,10 +951,7 @@ ID=freebsd // Minimal version string assert_eq!(parse_kernel_version("5.4"), Some((5, 4))); // With trailing whitespace from uname output - assert_eq!( - parse_kernel_version(" 5.15.0-91-generic\n"), - Some((5, 15)) - ); + assert_eq!(parse_kernel_version(" 5.15.0-91-generic\n"), Some((5, 15))); // Empty / invalid input assert_eq!(parse_kernel_version(""), None); assert_eq!(parse_kernel_version("not-a-version"), None); @@ -1010,7 +1008,7 @@ ID=freebsd gpu_count: 4, ecc_errors: vec!["GPU 0 'A100': 3 corrected ECC errors detected".to_string()], temperature_warnings: vec![ - "GPU 1 'A100': temperature 92C exceeds 85C threshold".to_string(), + "GPU 1 'A100': temperature 92C exceeds 85C threshold".to_string() ], power_issues: vec!["GPU 2 'A100': abnormal power draw 0.0W".to_string()], }; diff --git a/src/modules/hpc/nvidia_peermem.rs b/src/modules/hpc/nvidia_peermem.rs index 3b21899d..bb058b43 100644 --- a/src/modules/hpc/nvidia_peermem.rs +++ b/src/modules/hpc/nvidia_peermem.rs @@ -79,14 +79,7 @@ fn parse_driver_version(nvidia_smi_output: &str) -> Option { if version.is_empty() { return None; } - Some( - version - .lines() - .next() - .unwrap_or("") - .trim() - .to_string(), - ) + Some(version.lines().next().unwrap_or("").trim().to_string()) } /// Parse the major version number from a driver version string like "535.183.01". @@ -172,9 +165,8 @@ impl Module for NvidiaPeermemModule { )?; if autoload_exists { if context.check_mode { - changes.push( - "Would remove /etc/modules-load.d/nvidia-peermem.conf".to_string(), - ); + changes + .push("Would remove /etc/modules-load.d/nvidia-peermem.conf".to_string()); } else { run_cmd_ok( connection, @@ -217,10 +209,8 @@ impl Module for NvidiaPeermemModule { } if changed { - return Ok( - ModuleOutput::changed("Removed nvidia-peermem") - .with_data("changes", serde_json::json!(changes)), - ); + return Ok(ModuleOutput::changed("Removed nvidia-peermem") + .with_data("changes", serde_json::json!(changes))); } return Ok(ModuleOutput::ok("nvidia-peermem is not loaded")); @@ -323,9 +313,8 @@ impl Module for NvidiaPeermemModule { if !autoload_exists { if context.check_mode { - changes.push( - "Would create /etc/modules-load.d/nvidia-peermem.conf".to_string(), - ); + changes + .push("Would create /etc/modules-load.d/nvidia-peermem.conf".to_string()); } else { run_cmd_ok( connection, @@ -363,10 +352,7 @@ impl Module for NvidiaPeermemModule { } let mut output = if changed { - ModuleOutput::changed(format!( - "Applied {} nvidia-peermem changes", - changes.len() - )) + ModuleOutput::changed(format!("Applied {} nvidia-peermem changes", changes.len())) } else { ModuleOutput::ok("nvidia-peermem is loaded and configured") }; diff --git a/src/modules/hpc/redfish.rs b/src/modules/hpc/redfish.rs index fe719201..cea69cc6 100644 --- a/src/modules/hpc/redfish.rs +++ b/src/modules/hpc/redfish.rs @@ -309,10 +309,7 @@ fn firmware_precheck(firmware_json: &str) -> PreflightResult { for member in &members { let name = member.get("Name").and_then(|v| v.as_str()).unwrap_or(""); - let version = member - .get("Version") - .and_then(|v| v.as_str()) - .unwrap_or(""); + let version = member.get("Version").and_then(|v| v.as_str()).unwrap_or(""); let name_lower = name.to_lowercase(); if name_lower.contains("bmc") @@ -1143,10 +1140,7 @@ mod tests { assert_eq!(detect_vendor(r#"{"Manufacturer": ""}"#), "unknown"); // Other vendor falls back to lowercased name - assert_eq!( - detect_vendor(r#"{"Manufacturer": "Fujitsu"}"#), - "fujitsu" - ); + assert_eq!(detect_vendor(r#"{"Manufacturer": "Fujitsu"}"#), "fujitsu"); } #[test] diff --git a/src/modules/hpc/sssd.rs b/src/modules/hpc/sssd.rs index 4ab3278a..2968581b 100644 --- a/src/modules/hpc/sssd.rs +++ b/src/modules/hpc/sssd.rs @@ -141,9 +141,7 @@ fn parse_sssd_conf(content: &str) -> HashMap> { // Section header if trimmed.starts_with('[') && trimmed.ends_with(']') { current_section = trimmed[1..trimmed.len() - 1].to_string(); - sections - .entry(current_section.clone()) - .or_default(); + sections.entry(current_section.clone()).or_default(); continue; } @@ -229,17 +227,11 @@ fn validate_tls_certs( let mut errors = Vec::new(); // Check that the certificate file exists - let (cert_exists, _, _) = run_cmd( - connection, - &format!("test -f '{}'", tls_cert_path), - context, - )?; + let (cert_exists, _, _) = + run_cmd(connection, &format!("test -f '{}'", tls_cert_path), context)?; if !cert_exists { - errors.push(format!( - "TLS certificate file not found: {}", - tls_cert_path - )); + errors.push(format!("TLS certificate file not found: {}", tls_cert_path)); return Ok(PreflightResult { passed: false, warnings, @@ -305,11 +297,9 @@ fn parse_cert_expiry_warning(enddate_output: &str) -> Option { _ => None, }; - if let (Some(_month_num), Ok(year), Ok(day)) = ( - month, - year_str.parse::(), - day_str.parse::(), - ) { + if let (Some(_month_num), Ok(year), Ok(day)) = + (month, year_str.parse::(), day_str.parse::()) + { return Some(format!( "TLS certificate expires on {} {} {} (year {}). \ Verify it is not within 30 days of expiry.", @@ -841,8 +831,7 @@ impl Module for SssdDomainModule { check_nss_pam_health(connection, context, Some(user.as_str()))?; diagnostics.insert( "nss_pam_health".to_string(), - serde_json::to_value(&nss_result) - .unwrap_or(serde_json::json!(null)), + serde_json::to_value(&nss_result).unwrap_or(serde_json::json!(null)), ); } } @@ -899,8 +888,7 @@ impl Module for SssdDomainModule { // NSS/PAM health check if let Some(ref user) = test_user { - let nss_result = - check_nss_pam_health(connection, context, Some(user.as_str()))?; + let nss_result = check_nss_pam_health(connection, context, Some(user.as_str()))?; diagnostics.insert( "nss_pam_health".to_string(), serde_json::to_value(&nss_result).unwrap_or(serde_json::json!(null)), @@ -1069,10 +1057,7 @@ krb5_realm = CORP.LOCAL assert!(parsed.contains_key("domain/EXAMPLE.COM")); let domain1 = &parsed["domain/EXAMPLE.COM"]; assert_eq!(domain1.get("id_provider").unwrap(), "ldap"); - assert_eq!( - domain1.get("ldap_uri").unwrap(), - "ldaps://ldap.example.com" - ); + assert_eq!(domain1.get("ldap_uri").unwrap(), "ldaps://ldap.example.com"); assert_eq!(domain1.get("krb5_realm").unwrap(), "EXAMPLE.COM"); assert_eq!( domain1.get("ldap_search_base").unwrap(), diff --git a/src/modules/mod.rs b/src/modules/mod.rs index b73cf0bd..90023cbc 100644 --- a/src/modules/mod.rs +++ b/src/modules/mod.rs @@ -393,12 +393,15 @@ pub fn validate_command_args(args: &str) -> ModuleResult<()> { // If the string contains only safe characters, we can skip the detailed check. // This avoids checking 24 patterns for every safe string (O(N) vs O(M*N)). // - // Safe characters: alphanumeric, space, _, -, ., /, :, +, =, ,, @, % - let is_safe = args.bytes().all(|b| matches!(b, - b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | - b' ' | b'_' | b'-' | b'.' | b'/' | b':' | - b'+' | b'=' | b',' | b'@' | b'%' - )); + // Safe characters: alphanumeric, space, _, -, ., /, :, +, =, ,, @ + // Removed % because it can be used for variable expansion on Windows + let is_safe = args.bytes().all(|b| { + matches!(b, + b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | + b' ' | b'_' | b'-' | b'.' | b'/' | b':' | + b'+' | b'=' | b',' | b'@' + ) + }); if is_safe { return Ok(()); @@ -430,6 +433,8 @@ pub fn validate_command_args(args: &str) -> ModuleResult<()> { ("\\", "shell escaping \\"), ("$", "variable expansion $"), ("#", "shell comment #"), + ("%", "variable expansion %"), + ("^", "shell escape ^"), ]; for (pattern, description) in dangerous_patterns { diff --git a/src/modules/shell.rs b/src/modules/shell.rs index af4a8ee5..a339327d 100644 --- a/src/modules/shell.rs +++ b/src/modules/shell.rs @@ -712,10 +712,7 @@ mod tests { let module = ShellModule; let mut params: ModuleParams = HashMap::new(); // Use /bin/sh as executable and echo as the command - params.insert( - "executable".to_string(), - serde_json::json!("/bin/sh"), - ); + params.insert("executable".to_string(), serde_json::json!("/bin/sh")); params.insert("cmd".to_string(), serde_json::json!("echo arg1")); let context = ModuleContext::default(); diff --git a/tests/security_windows_command_injection.rs b/tests/security_windows_command_injection.rs new file mode 100644 index 00000000..931a0f47 --- /dev/null +++ b/tests/security_windows_command_injection.rs @@ -0,0 +1,47 @@ +use rustible::modules::{validate_command_args, ModuleError}; + +#[test] +fn test_validate_command_args_blocks_windows_caret() { + // ^ is the escape character in cmd.exe. + // "echo h^ello" -> prints "hello" + // "who^ami" -> runs "whoami" + // + // validate_command_args should now reject this. + + let payload = "echo h^ello"; + + let result = validate_command_args(payload); + + assert!(result.is_err(), "Expected payload with ^ to be rejected"); + match result { + Err(ModuleError::InvalidParameter(msg)) => { + assert!( + msg.contains("shell escape ^"), + "Error message should mention shell escape ^" + ); + } + _ => panic!("Expected InvalidParameter error"), + } +} + +#[test] +fn test_validate_command_args_blocks_variable_expansion() { + // %VAR% is expanded by cmd.exe. + // "echo %OS%" + + let payload = "echo %OS%"; + + // validate_command_args should now reject this. + let result = validate_command_args(payload); + + assert!(result.is_err(), "Expected payload with % to be rejected"); + match result { + Err(ModuleError::InvalidParameter(msg)) => { + assert!( + msg.contains("variable expansion %"), + "Error message should mention variable expansion %" + ); + } + _ => panic!("Expected InvalidParameter error"), + } +}