From ba555802569d7f7a95f6264eda13353970c15082 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Mon, 11 May 2026 11:55:00 -0700 Subject: [PATCH 01/20] Port osmodifier from Go binary to native Rust crate Replace the external Go osmodifier binary from azure-linux-image-tools with a native Rust library crate (crates/osmodifier). Trident now calls osmodifier functions directly instead of serializing config to YAML, writing a temp file, and exec'ing the Go binary. The new crate implements: - User management (useradd, password hashing, SSH keys, groups) - Hostname configuration (/etc/hostname) - Service management (systemctl enable/disable) - Kernel module configuration (modules-load.d, modprobe.d) - SELinux configuration (/etc/selinux/config and kernel cmdline) - /etc/default/grub parsing and writing - grub.cfg parsing for update-default-grub flow - grub2-mkconfig execution Public API: - modify_os() - replaces osmodifier --config-file for OS modifications - update_default_grub() - replaces osmodifier --update-grub - modify_boot() - replaces osmodifier --config-file for boot config This eliminates: - External Go binary build dependency (azure-linux-image-tools clone) - Binary bind-mounting into newroot - YAML serialization round-trip overhead - OS_MODIFIER_BINARY_PATH and OS_MODIFIER_NEWROOT_PATH constants - Makefile, Dockerfile, RPM spec, and pipeline osmodifier references Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Cargo.toml | 1 + Makefile | 21 +- crates/osmodifier/Cargo.toml | 20 + crates/osmodifier/src/config.rs | 137 ++++++ crates/osmodifier/src/default_grub.rs | 225 ++++++++++ crates/osmodifier/src/grub_cfg.rs | 225 ++++++++++ crates/osmodifier/src/hostname.rs | 21 + crates/osmodifier/src/lib.rs | 241 ++++++++++ crates/osmodifier/src/modules.rs | 138 ++++++ crates/osmodifier/src/selinux.rs | 73 ++++ crates/osmodifier/src/services.rs | 71 +++ crates/osmodifier/src/users.rs | 410 ++++++++++++++++++ crates/osutils/Cargo.toml | 1 + crates/osutils/src/osmodifier.rs | 197 +-------- crates/trident/Cargo.toml | 1 + crates/trident/src/engine/boot/grub.rs | 46 +- crates/trident/src/engine/boot/mod.rs | 4 +- crates/trident/src/engine/newroot.rs | 18 +- crates/trident/src/lib.rs | 6 - crates/trident/src/subsystems/osconfig/mod.rs | 71 +-- packaging/docker/Dockerfile.azl3 | 1 - packaging/docker/Dockerfile.full | 1 - packaging/docker/Dockerfile.full.public | 1 - packaging/docker/Dockerfile.runtime | 3 - packaging/rpm/trident.spec | 14 +- 25 files changed, 1611 insertions(+), 336 deletions(-) create mode 100644 crates/osmodifier/Cargo.toml create mode 100644 crates/osmodifier/src/config.rs create mode 100644 crates/osmodifier/src/default_grub.rs create mode 100644 crates/osmodifier/src/grub_cfg.rs create mode 100644 crates/osmodifier/src/hostname.rs create mode 100644 crates/osmodifier/src/lib.rs create mode 100644 crates/osmodifier/src/modules.rs create mode 100644 crates/osmodifier/src/selinux.rs create mode 100644 crates/osmodifier/src/services.rs create mode 100644 crates/osmodifier/src/users.rs diff --git a/Cargo.toml b/Cargo.toml index 34df5dfbb..f96d75320 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,6 +5,7 @@ default-members = ["crates/trident"] members = [ "crates/docbuilder", "crates/trident-acl-agent", + "crates/osmodifier", "crates/osutils", "crates/pytest_gen", "crates/pytest", diff --git a/Makefile b/Makefile index 65c0aa904..d9e374528 100644 --- a/Makefile +++ b/Makefile @@ -142,25 +142,8 @@ target/release/trident target/release/trident-acl-agent: .cargo/config | version TRIDENT_VERSION="$(LOCAL_BUILD_TRIDENT_VERSION)" \ cargo build --release --features dangerous-options,grpc-preview -p trident -p trident-acl-agent -TOOLKIT_DIR="azure-linux-image-tools/toolkit" -AZL_TOOLS_OUT_DIR="$(TOOLKIT_DIR)/out/tools" ARTIFACTS_DIR="artifacts" -# Build OSModifier from a local clone of azure-linux-image-tools. -# Make sure the repo has been cloned manually, via: -# -# git clone https://github.com/microsoft/azure-linux-image-tools - -artifacts/osmodifier: packaging/docker/Dockerfile-osmodifier.azl3 - @docker build -t trident/osmodifier-build:latest \ - -f packaging/docker/Dockerfile-osmodifier.azl3 \ - . - @mkdir -p "$(ARTIFACTS_DIR)" - @id=$$(docker create trident/osmodifier-build:latest) && \ - docker cp -q $$id:/work/azure-linux-image-tools/toolkit/out/tools/osmodifier $@ || \ - docker rm -v $$id - @touch $@ - .PHONY: azl3-builder-image clean-azl3-builder-image build-azl3 azl3-builder-image: @echo "Checking for local image $(AZL3_BUILDER_IMAGE)..." @@ -185,7 +168,7 @@ target/azl3/release/trident target/azl3/release/trident-acl-agent: version-vars cargo build --color always --target-dir target/azl3 --release --features dangerous-options,grpc-preview -p trident -p trident-acl-agent # This will do a proper build on azl3, exactly as the pipelines would, with the custom registry and all. -bin/trident-rpms-azl3.tar.gz: packaging/docker/Dockerfile.full packaging/systemd/*.service packaging/rpm/trident.spec artifacts/osmodifier packaging/selinux-policy-trident/* version-vars +bin/trident-rpms-azl3.tar.gz: packaging/docker/Dockerfile.full packaging/systemd/*.service packaging/rpm/trident.spec packaging/selinux-policy-trident/* version-vars $(eval CARGO_REGISTRIES_BMP_PUBLICPACKAGES_TOKEN := $(shell az account get-access-token --query "join(' ', ['Bearer', accessToken])" --output tsv)) @mkdir -p bin/ @@ -207,7 +190,7 @@ bin/trident-rpms-azl3.tar.gz: packaging/docker/Dockerfile.full packaging/systemd @tar xf $@ -C bin/ # This one does a fast trick-build where we build locally and inject the binary into the container to add it to the RPM. -bin/trident-rpms.tar.gz: packaging/docker/Dockerfile.azl3 packaging/systemd/*.service packaging/rpm/trident.spec artifacts/osmodifier target/release/trident target/release/trident-acl-agent packaging/selinux-policy-trident/* +bin/trident-rpms.tar.gz: packaging/docker/Dockerfile.azl3 packaging/systemd/*.service packaging/rpm/trident.spec target/release/trident target/release/trident-acl-agent packaging/selinux-policy-trident/* @mkdir -p bin/ @if [ ! -f bin/trident ] || ! cmp -s target/release/trident bin/trident; then \ cp target/release/trident bin/trident; \ diff --git a/crates/osmodifier/Cargo.toml b/crates/osmodifier/Cargo.toml new file mode 100644 index 000000000..8f565e224 --- /dev/null +++ b/crates/osmodifier/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "osmodifier" +version = "0.1.0" +edition = "2021" +publish = false +license = "MIT" +description = "OS modifier library - applies OS configuration changes (users, hostname, services, modules, boot config, SELinux)" + +[dependencies] +anyhow = { workspace = true } +log = { workspace = true } +regex = { workspace = true } +serde = { workspace = true } +serde_yaml = { workspace = true } +tempfile = { workspace = true } + +trident_api = { path = "../trident_api" } + +[dev-dependencies] +indoc = { workspace = true } diff --git a/crates/osmodifier/src/config.rs b/crates/osmodifier/src/config.rs new file mode 100644 index 000000000..c054f97a6 --- /dev/null +++ b/crates/osmodifier/src/config.rs @@ -0,0 +1,137 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Configuration types for OS modifier operations. +//! +//! These types were originally in `osutils::osmodifier` and are the Rust +//! equivalents of the Go `osmodifierapi` types. + +use serde::{Deserialize, Serialize}; +use trident_api::config::{KernelCommandLine, Module, Selinux, Services}; + +/// OS modification configuration. +/// +/// Covers users, hostname, modules, services, kernel command line, and SELinux. +#[derive(Serialize, Deserialize, Default, Debug)] +#[serde(rename_all = "camelCase")] +pub struct OSModifierConfig { + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub users: Vec, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub hostname: Option, + + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub modules: Vec, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub services: Option, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub kernel_command_line: Option, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub selinux: Option, +} + +/// Password type for user configuration. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(rename_all = "kebab-case")] +pub enum PasswordType { + Locked, + PlainText, + Hashed, +} + +/// User password configuration. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct MICPassword { + #[serde(rename = "type")] + pub password_type: PasswordType, + pub value: String, +} + +/// User configuration in the MIC (Microsoft Image Customizer) format. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct MICUser { + pub name: String, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub uid: Option, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub password: Option, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub password_expires_days: Option, + + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub ssh_public_keys: Vec, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub primary_group: Option, + + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub secondary_groups: Vec, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub startup_command: Option, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub home_directory: Option, +} + +/// Overlay filesystem configuration. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct Overlay { + pub lower_dir: String, + pub upper_dir: String, + pub work_dir: String, + pub partition: IdentifiedPartition, +} + +/// A partition identified by an ID string. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Default)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct IdentifiedPartition { + pub id: String, +} + +/// dm-verity configuration. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Default)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct Verity { + pub id: String, + pub name: String, + pub data_device: String, + pub hash_device: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub corruption_option: Option, +} + +/// Corruption handling behavior for dm-verity. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub enum CorruptionOption { + IoError, + Ignore, + Panic, + Restart, +} + +/// Boot-specific configuration (overlays, verity, SELinux, root device). +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct BootConfig { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub selinux: Option, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub overlays: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub verity: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub root_device: Option, +} diff --git a/crates/osmodifier/src/default_grub.rs b/crates/osmodifier/src/default_grub.rs new file mode 100644 index 000000000..88214a54f --- /dev/null +++ b/crates/osmodifier/src/default_grub.rs @@ -0,0 +1,225 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! /etc/default/grub parser and writer. +//! +//! Parses the shell-variable format used by GRUB's default configuration file. +//! Supports reading, modifying, and writing back while preserving comments and +//! ordering. + +use std::{fs, path::PathBuf}; + +use anyhow::{Context, Error}; +use log::{debug, trace}; + +use crate::OsModifierContext; + +const DEFAULT_GRUB_PATH: &str = "/etc/default/grub"; + +/// Represents a parsed /etc/default/grub file. +pub struct DefaultGrub { + /// Original lines of the file, with modifications applied in-place. + lines: Vec, + /// Path to the file on disk. + path: PathBuf, +} + +impl DefaultGrub { + /// Read and parse /etc/default/grub. + pub fn read(ctx: &OsModifierContext) -> Result { + let path = ctx.path(DEFAULT_GRUB_PATH); + debug!("Reading default grub from '{}'", path.display()); + + let content = fs::read_to_string(&path) + .with_context(|| format!("Failed to read '{}'", path.display()))?; + + trace!("Default grub content:\n{content}"); + + let lines = content.lines().map(String::from).collect(); + Ok(Self { lines, path }) + } + + /// Write the (possibly modified) config back to disk. + pub fn write(&self) -> Result<(), Error> { + let mut content = self.lines.join("\n"); + content.push('\n'); + + debug!("Writing default grub to '{}'", self.path.display()); + trace!("Default grub content to write:\n{content}"); + + fs::write(&self.path, &content) + .with_context(|| format!("Failed to write '{}'", self.path.display())) + } + + /// Get the value of a variable (e.g., "GRUB_CMDLINE_LINUX"). + /// Returns the unquoted value. + pub fn get_variable(&self, name: &str) -> Option { + let prefix = format!("{name}="); + for line in &self.lines { + let trimmed = line.trim(); + if trimmed.starts_with(&prefix) { + let value = &trimmed[prefix.len()..]; + return Some(unquote(value)); + } + } + None + } + + /// Set a variable value. If the variable exists, update it in place. + /// If not, append it. + pub fn set_variable(&mut self, name: &str, value: &str) { + let prefix = format!("{name}="); + let new_line = format!("{name}=\"{value}\""); + + for line in &mut self.lines { + if line.trim().starts_with(&prefix) { + *line = new_line; + return; + } + } + + // Not found — append + self.lines.push(new_line); + } + + /// Update kernel command line args in GRUB_CMDLINE_LINUX. + /// + /// `old_keys` specifies which arg names to remove (matched by prefix + /// before `=`). `new_args` are the replacement args to insert. + /// + /// This matches the Go `UpdateKernelCommandLineArgs` behavior. + pub fn update_cmdline_args( + &mut self, + old_keys: &[&str], + new_args: &[String], + ) -> Result<(), Error> { + let current = self + .get_variable("GRUB_CMDLINE_LINUX") + .unwrap_or_default(); + + let mut args: Vec = current + .split_whitespace() + .filter(|arg| { + let arg_name = arg.split('=').next().unwrap_or(arg); + !old_keys.contains(&arg_name) + }) + .map(String::from) + .collect(); + + args.extend(new_args.iter().cloned()); + + let new_value = args.join(" "); + self.set_variable("GRUB_CMDLINE_LINUX", &new_value); + + Ok(()) + } + + /// Add extra command line arguments to GRUB_CMDLINE_LINUX without + /// removing any existing ones. + pub fn add_extra_cmdline(&mut self, extra: &[String]) { + let current = self + .get_variable("GRUB_CMDLINE_LINUX") + .unwrap_or_default(); + + let mut args: Vec = if current.is_empty() { + Vec::new() + } else { + current + .split_whitespace() + .map(String::from) + .collect() + }; + + args.extend(extra.iter().cloned()); + + let new_value = args.join(" "); + self.set_variable("GRUB_CMDLINE_LINUX", &new_value); + } +} + +/// Remove surrounding quotes (single or double) from a value string. +fn unquote(s: &str) -> String { + let s = s.trim(); + if (s.starts_with('"') && s.ends_with('"')) || (s.starts_with('\'') && s.ends_with('\'')) { + s[1..s.len() - 1].to_string() + } else { + s.to_string() + } +} + +/// Add extra kernel command line args to /etc/default/grub. +pub fn add_extra_cmdline(ctx: &OsModifierContext, extra: &[String]) -> Result<(), Error> { + let mut grub = DefaultGrub::read(ctx)?; + grub.add_extra_cmdline(extra); + grub.write() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_unquote() { + assert_eq!(unquote(r#""hello world""#), "hello world"); + assert_eq!(unquote("'hello'"), "hello"); + assert_eq!(unquote("noquotes"), "noquotes"); + assert_eq!(unquote(""), ""); + } + + #[test] + fn test_get_set_variable() { + let mut grub = DefaultGrub { + lines: vec![ + "# Comment".to_string(), + r#"GRUB_CMDLINE_LINUX="selinux=1 enforcing=1""#.to_string(), + r#"GRUB_DEVICE="/dev/sda2""#.to_string(), + ], + path: PathBuf::from("/etc/default/grub"), + }; + + assert_eq!( + grub.get_variable("GRUB_CMDLINE_LINUX"), + Some("selinux=1 enforcing=1".to_string()) + ); + assert_eq!( + grub.get_variable("GRUB_DEVICE"), + Some("/dev/sda2".to_string()) + ); + assert_eq!(grub.get_variable("NONEXISTENT"), None); + + grub.set_variable("GRUB_DEVICE", "/dev/sdb1"); + assert_eq!( + grub.get_variable("GRUB_DEVICE"), + Some("/dev/sdb1".to_string()) + ); + + grub.set_variable("NEW_VAR", "new_value"); + assert_eq!( + grub.get_variable("NEW_VAR"), + Some("new_value".to_string()) + ); + } + + #[test] + fn test_update_cmdline_args() { + let mut grub = DefaultGrub { + lines: vec![ + r#"GRUB_CMDLINE_LINUX="quiet selinux=1 enforcing=1 rd.overlayfs=old""#.to_string(), + ], + path: PathBuf::from("/etc/default/grub"), + }; + + grub.update_cmdline_args( + &["selinux", "enforcing"], + &["selinux=0".to_string()], + ) + .unwrap(); + + let result = grub.get_variable("GRUB_CMDLINE_LINUX").unwrap(); + assert!(result.contains("quiet")); + assert!(result.contains("rd.overlayfs=old")); + assert!(result.contains("selinux=0")); + assert!(!result.contains("enforcing=1")); + assert!(!result.contains("selinux=1")); + } +} diff --git a/crates/osmodifier/src/grub_cfg.rs b/crates/osmodifier/src/grub_cfg.rs new file mode 100644 index 000000000..5206fd7f4 --- /dev/null +++ b/crates/osmodifier/src/grub_cfg.rs @@ -0,0 +1,225 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! grub.cfg parsing and grub2-mkconfig execution. +//! +//! Used by the `update_default_grub` flow to extract boot args from the +//! generated grub.cfg and sync them back to /etc/default/grub. + +use std::{fs, path::Path, process::Command}; + +use anyhow::{bail, Context, Error}; +use log::{debug, info, trace}; +use regex::Regex; + +use crate::OsModifierContext; + +/// Possible grub.cfg locations, tried in order. +const GRUB_CFG_PATHS: &[&str] = &["/boot/grub2/grub.cfg", "/boot/grub/grub.cfg"]; + +/// The grub.cfg args we want to extract for syncing to /etc/default/grub. +const SYNC_ARG_NAMES: &[&str] = &[ + "rd.overlayfs", + "roothash", + "root", + "selinux", + "enforcing", +]; + +/// Extract boot arguments from the generated grub.cfg. +/// +/// Returns a tuple of (args_to_sync, optional_root_device). +/// `args_to_sync` contains entries like `["selinux=1", "rd.overlayfs=..."]`. +/// `root_device` is extracted separately because it maps to GRUB_DEVICE +/// rather than GRUB_CMDLINE_LINUX. +pub fn extract_boot_args_from_grub_cfg( + ctx: &OsModifierContext, +) -> Result<(Vec, Option), Error> { + let grub_cfg_path = find_grub_cfg(ctx)?; + let content = fs::read_to_string(&grub_cfg_path) + .with_context(|| format!("Failed to read '{}'", grub_cfg_path.display()))?; + + trace!("grub.cfg content:\n{content}"); + + // Find the non-recovery linux command line + let linux_line = find_non_recovery_linux_line(&content)?; + debug!("Found linux line: {linux_line}"); + + // Parse args from the linux line + let mut args = Vec::new(); + let mut root_device = None; + + for token in linux_line.split_whitespace() { + let (name, value) = match token.split_once('=') { + Some((n, v)) => (n, Some(v)), + None => (token, None), + }; + + if SYNC_ARG_NAMES.contains(&name) { + if name == "root" { + if let Some(v) = value { + root_device = Some(v.to_string()); + } + } else if let Some(v) = value { + args.push(format!("{name}={v}")); + } + } + } + + Ok((args, root_device)) +} + +/// Find the grub.cfg file on the filesystem. +fn find_grub_cfg(ctx: &OsModifierContext) -> Result { + for path in GRUB_CFG_PATHS { + let full = ctx.path(path); + if full.exists() { + return Ok(full); + } + } + bail!( + "Could not find grub.cfg at any of: {:?}", + GRUB_CFG_PATHS + ) +} + +/// Find the linux command line from a non-recovery menuentry in grub.cfg. +/// +/// This matches the Go `FindNonRecoveryLinuxLine` behavior: +/// - Scans for `menuentry` blocks +/// - Skips entries whose title contains "recovery" +/// - Returns the `linux` line from the first non-recovery entry +/// - Expects exactly one match +fn find_non_recovery_linux_line(content: &str) -> Result { + // Simple state-machine approach: track whether we're inside a menuentry + // block, skip recovery entries, find the linux line. + let menuentry_re = Regex::new(r#"^\s*menuentry\s+['"](.*?)['"]\s"#) + .context("Failed to compile menuentry regex")?; + let linux_re = Regex::new(r"^\s*linux\s+(.+)$") + .context("Failed to compile linux regex")?; + + let mut in_menuentry = false; + let mut is_recovery = false; + let mut brace_depth: i32 = 0; + let mut linux_lines = Vec::new(); + + for line in content.lines() { + let trimmed = line.trim(); + + // Track menuentry blocks + if let Some(caps) = menuentry_re.captures(line) { + let title = caps.get(1).map(|m| m.as_str()).unwrap_or(""); + is_recovery = title.to_lowercase().contains("recovery"); + in_menuentry = true; + brace_depth = 0; + } + + // Track braces + for ch in trimmed.chars() { + match ch { + '{' => brace_depth += 1, + '}' => { + brace_depth -= 1; + if brace_depth <= 0 { + in_menuentry = false; + is_recovery = false; + } + } + _ => {} + } + } + + // Look for linux lines in non-recovery menuentries + if in_menuentry && !is_recovery { + if let Some(caps) = linux_re.captures(line) { + if let Some(args) = caps.get(1) { + linux_lines.push(args.as_str().to_string()); + } + } + } + } + + if linux_lines.is_empty() { + bail!("No non-recovery linux command line found in grub.cfg"); + } + + if linux_lines.len() > 1 { + // The Go code expects exactly one. We use the first one with a warning. + debug!( + "Found {} non-recovery linux lines, using the first one", + linux_lines.len() + ); + } + + Ok(linux_lines.into_iter().next().unwrap()) +} + +/// Run grub2-mkconfig to regenerate the GRUB configuration. +pub fn run_grub_mkconfig(ctx: &OsModifierContext) -> Result<(), Error> { + let grub_cfg_path = find_grub_cfg(ctx)?; + + info!( + "Running grub2-mkconfig -o '{}'", + grub_cfg_path.display() + ); + + let output = Command::new("grub2-mkconfig") + .arg("-o") + .arg(&grub_cfg_path) + .output() + .context("Failed to execute grub2-mkconfig")?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + let stdout = String::from_utf8_lossy(&output.stdout); + bail!( + "grub2-mkconfig failed:\nstdout: {stdout}\nstderr: {stderr}" + ); + } + + debug!("grub2-mkconfig completed successfully"); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_find_non_recovery_linux_line() { + let grub_cfg = indoc::indoc! {r#" + set timeout=5 + menuentry 'Azure Linux' --class azurelinux { + linux /boot/vmlinuz root=/dev/sda2 selinux=1 enforcing=1 rd.overlayfs=/a,/b,/c,/dev/sda3 + initrd /boot/initrd.img + } + menuentry 'Azure Linux (recovery)' --class azurelinux { + linux /boot/vmlinuz root=/dev/sda2 single + initrd /boot/initrd.img + } + "#}; + + let result = find_non_recovery_linux_line(grub_cfg).unwrap(); + assert!(result.contains("root=/dev/sda2")); + assert!(result.contains("selinux=1")); + assert!(result.contains("rd.overlayfs=")); + } + + #[test] + fn test_find_non_recovery_linux_line_no_recovery() { + let grub_cfg = indoc::indoc! {r#" + menuentry 'Linux' { + linux /boot/vmlinuz root=/dev/sda1 + } + "#}; + + let result = find_non_recovery_linux_line(grub_cfg).unwrap(); + assert!(result.contains("root=/dev/sda1")); + } + + #[test] + fn test_no_linux_line() { + let grub_cfg = "set timeout=5\n"; + assert!(find_non_recovery_linux_line(grub_cfg).is_err()); + } +} diff --git a/crates/osmodifier/src/hostname.rs b/crates/osmodifier/src/hostname.rs new file mode 100644 index 000000000..89e2640e8 --- /dev/null +++ b/crates/osmodifier/src/hostname.rs @@ -0,0 +1,21 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Hostname management — writes /etc/hostname. + +use std::fs; + +use anyhow::{Context, Error}; +use log::debug; + +use crate::OsModifierContext; + +const HOSTNAME_PATH: &str = "/etc/hostname"; + +/// Write the hostname to /etc/hostname. +pub fn update(ctx: &OsModifierContext, hostname: &str) -> Result<(), Error> { + let path = ctx.path(HOSTNAME_PATH); + debug!("Writing hostname '{}' to '{}'", hostname, path.display()); + fs::write(&path, format!("{hostname}\n")) + .with_context(|| format!("Failed to write hostname to '{}'", path.display())) +} diff --git a/crates/osmodifier/src/lib.rs b/crates/osmodifier/src/lib.rs new file mode 100644 index 000000000..6ef669639 --- /dev/null +++ b/crates/osmodifier/src/lib.rs @@ -0,0 +1,241 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! OS modifier library — applies OS configuration changes to the filesystem. +//! +//! This crate replaces the external Go `osmodifier` binary with native Rust +//! implementations. All operations target paths under a configurable root +//! directory (defaulting to `/`). + +pub mod config; +mod default_grub; +mod grub_cfg; +mod hostname; +mod modules; +mod selinux; +mod services; +mod users; + +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Error}; +use log::{debug, info}; + +pub use config::*; + +/// Execution context for OS modifier operations. +/// +/// All filesystem paths are resolved relative to `root`. When trident has +/// chrooted into newroot, `root` should be `/`. When operating on an offline +/// image mounted at a specific path, set `root` accordingly. +pub struct OsModifierContext { + /// Root directory for all filesystem operations. + pub root: PathBuf, +} + +impl Default for OsModifierContext { + fn default() -> Self { + Self { + root: PathBuf::from("/"), + } + } +} + +impl OsModifierContext { + /// Resolve a path relative to the context root. + pub fn path(&self, p: impl AsRef) -> PathBuf { + if self.root == Path::new("/") { + p.as_ref().to_path_buf() + } else { + let p = p.as_ref(); + let stripped = p.strip_prefix("/").unwrap_or(p); + self.root.join(stripped) + } + } +} + +/// Apply OS modifications: users, hostname, services, modules, kernel command +/// line, and SELinux. +/// +/// This replaces the Go `osmodifier --config-file` codepath for +/// [`OSModifierConfig`]. +pub fn modify_os(ctx: &OsModifierContext, config: &OSModifierConfig) -> Result<(), Error> { + debug!("Applying OS modifications"); + + if !config.users.is_empty() { + info!("Configuring users"); + users::add_or_update_users(ctx, &config.users) + .context("Failed to configure users")?; + } + + if let Some(ref name) = config.hostname { + if !name.is_empty() { + info!("Setting hostname to '{name}'"); + hostname::update(ctx, name) + .context("Failed to update hostname")?; + } + } + + if let Some(ref services) = config.services { + if !services.enable.is_empty() || !services.disable.is_empty() { + info!("Configuring services"); + services::configure(ctx, services) + .context("Failed to configure services")?; + } + } + + if !config.modules.is_empty() { + info!("Configuring kernel modules"); + modules::configure(ctx, &config.modules) + .context("Failed to configure kernel modules")?; + } + + // Kernel command line and SELinux are handled via boot config, not here. + // The Go code uses BootCustomizer for these, which requires detecting the + // bootloader type and working with /etc/default/grub. In trident, these + // are only set through the boot subsystem's modify_boot() path. + // + // However, for UKI images, SELinux mode is set via the config file directly + // (not via kernel cmdline). The osconfig subsystem handles this case by + // including selinux in the OSModifierConfig. + if let Some(ref selinux_cfg) = config.selinux { + if let Some(ref mode) = selinux_cfg.mode { + info!("Updating SELinux config file to mode '{mode:?}'"); + selinux::update_config_file(ctx, mode) + .context("Failed to update SELinux config file")?; + } + } + + if let Some(ref kcl) = config.kernel_command_line { + if !kcl.extra_command_line.is_empty() { + info!("Adding extra kernel command line arguments"); + default_grub::add_extra_cmdline(ctx, &kcl.extra_command_line) + .context("Failed to add extra kernel command line")?; + grub_cfg::run_grub_mkconfig(ctx) + .context("Failed to regenerate GRUB config")?; + } + } + + Ok(()) +} + +/// Sync current grub.cfg values back to /etc/default/grub and regenerate. +/// +/// This replaces the Go `osmodifier --update-grub` codepath: +/// 1. Reads the generated grub.cfg +/// 2. Extracts overlayfs, verity, root, selinux, enforcing args +/// 3. Stamps those values into /etc/default/grub +/// 4. Runs grub2-mkconfig to regenerate +pub fn update_default_grub(ctx: &OsModifierContext) -> Result<(), Error> { + info!("Syncing grub.cfg values to /etc/default/grub"); + + let (args, root_device) = grub_cfg::extract_boot_args_from_grub_cfg(ctx) + .context("Failed to extract boot args from grub.cfg")?; + + let mut default_grub = default_grub::DefaultGrub::read(ctx)?; + + default_grub.update_cmdline_args( + &["rd.overlayfs", "roothash", "root", "selinux", "enforcing"], + &args, + )?; + + if let Some(root) = root_device { + default_grub.set_variable("GRUB_DEVICE", &root); + } + + default_grub.write()?; + + grub_cfg::run_grub_mkconfig(ctx) + .context("Failed to regenerate GRUB config after updating defaults")?; + + info!("Successfully updated default grub"); + Ok(()) +} + +/// Apply boot-specific modifications: SELinux, overlays, verity, root device. +/// +/// This replaces the Go `osmodifier --config-file` codepath for +/// [`BootConfig`]. Updates /etc/default/grub and regenerates via +/// grub2-mkconfig. +pub fn modify_boot(ctx: &OsModifierContext, config: &BootConfig) -> Result<(), Error> { + info!("Applying boot configuration modifications"); + + let mut default_grub = default_grub::DefaultGrub::read(ctx)?; + let mut changed = false; + + if let Some(ref selinux_cfg) = config.selinux { + if let Some(ref mode) = selinux_cfg.mode { + debug!("Updating SELinux in boot config"); + selinux::update_grub_cmdline(ctx, &mut default_grub, mode)?; + selinux::update_config_file(ctx, mode) + .context("Failed to update SELinux config file")?; + changed = true; + } + } + + if !config.overlays.is_empty() { + debug!("Updating overlays in boot config"); + let mut overlay_configs = Vec::new(); + for overlay in &config.overlays { + overlay_configs.push(format!( + "{},{},{},{}", + overlay.lower_dir, overlay.upper_dir, overlay.work_dir, overlay.partition.id, + )); + } + let concatenated = overlay_configs.join(" "); + default_grub.update_cmdline_args( + &["rd.overlayfs"], + &[format!("rd.overlayfs={concatenated}")], + )?; + changed = true; + } + + if let Some(ref verity) = config.verity { + debug!("Updating verity in boot config"); + let corruption_opt = verity + .corruption_option + .as_ref() + .map(|c| format_corruption_option(c)) + .unwrap_or_default(); + + let new_args = vec![ + "rd.systemd.verity=1".to_string(), + format!("systemd.verity_root_data={}", verity.data_device), + format!("systemd.verity_root_hash={}", verity.hash_device), + format!("systemd.verity_root_options={corruption_opt}"), + ]; + default_grub.update_cmdline_args( + &[ + "rd.systemd.verity", + "systemd.verity_root_data", + "systemd.verity_root_hash", + "systemd.verity_root_options", + ], + &new_args, + )?; + changed = true; + } + + if let Some(ref root_device) = config.root_device { + debug!("Setting root device to '{root_device}'"); + default_grub.set_variable("GRUB_DEVICE", root_device); + changed = true; + } + + if changed { + default_grub.write()?; + grub_cfg::run_grub_mkconfig(ctx) + .context("Failed to regenerate GRUB config after boot modifications")?; + } + + Ok(()) +} + +fn format_corruption_option(opt: &CorruptionOption) -> String { + match opt { + CorruptionOption::IoError => String::new(), + CorruptionOption::Ignore => "ignore-corruption".to_string(), + CorruptionOption::Panic => "panic-on-corruption".to_string(), + CorruptionOption::Restart => "restart-on-corruption".to_string(), + } +} diff --git a/crates/osmodifier/src/modules.rs b/crates/osmodifier/src/modules.rs new file mode 100644 index 000000000..8b3a3a7e1 --- /dev/null +++ b/crates/osmodifier/src/modules.rs @@ -0,0 +1,138 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Kernel module configuration — write modules-load.d and modprobe.d configs. + +use std::fs; + +use anyhow::{bail, Context, Error}; +use log::debug; + +use trident_api::config::{Module, LoadMode}; + +use crate::OsModifierContext; + +const MODULES_LOAD_DIR: &str = "/etc/modules-load.d"; +const MODULES_LOAD_CONF: &str = "/etc/modules-load.d/modules-load.conf"; +const MODPROBE_DIR: &str = "/etc/modprobe.d"; +const MODPROBE_DISABLED_CONF: &str = "/etc/modprobe.d/modules-disabled.conf"; +const MODPROBE_OPTIONS_CONF: &str = "/etc/modprobe.d/module-options.conf"; + +/// Configure kernel modules by writing modules-load.d and modprobe.d files. +pub fn configure(ctx: &OsModifierContext, modules: &[Module]) -> Result<(), Error> { + // Read existing configs (or start fresh) + let load_path = ctx.path(MODULES_LOAD_CONF); + let disabled_path = ctx.path(MODPROBE_DISABLED_CONF); + let options_path = ctx.path(MODPROBE_OPTIONS_CONF); + + let mut load_lines = read_config_lines(ctx, MODULES_LOAD_CONF); + let mut disabled_lines = read_config_lines(ctx, MODPROBE_DISABLED_CONF); + let mut options_lines = read_config_lines(ctx, MODPROBE_OPTIONS_CONF); + + for module in modules { + match module.load_mode { + LoadMode::Always => { + debug!("Module '{}': set to always load", module.name); + // Remove from blacklist if present + remove_blacklist(&mut disabled_lines, &module.name); + // Add to auto-load list if not present + if !load_lines.iter().any(|l| l.trim() == module.name) { + load_lines.push(module.name.clone()); + } + // Set options if provided + if let Some(ref opts) = module.options { + update_options(&mut options_lines, &module.name, opts); + } + } + LoadMode::Auto => { + debug!("Module '{}': set to auto", module.name); + // Remove from blacklist if present + remove_blacklist(&mut disabled_lines, &module.name); + // Set options if provided + if let Some(ref opts) = module.options { + update_options(&mut options_lines, &module.name, opts); + } + } + LoadMode::Disable => { + debug!("Module '{}': set to disabled", module.name); + if module.options.is_some() { + bail!( + "Module '{}' is disabled but has options set — this is not allowed", + module.name + ); + } + // Remove from auto-load list + load_lines.retain(|l| l.trim() != module.name); + // Add to blacklist if not present + let blacklist_entry = format!("blacklist {}", module.name); + if !disabled_lines.iter().any(|l| l.trim() == blacklist_entry) { + disabled_lines.push(blacklist_entry); + } + } + LoadMode::Inherit => { + debug!("Module '{}': inherit (update options only)", module.name); + // Only update options if module is not disabled + let is_disabled = disabled_lines + .iter() + .any(|l| l.trim() == format!("blacklist {}", module.name)); + if !is_disabled { + if let Some(ref opts) = module.options { + update_options(&mut options_lines, &module.name, opts); + } + } + } + } + } + + // Write out the config files + ensure_dir(ctx, MODULES_LOAD_DIR)?; + ensure_dir(ctx, MODPROBE_DIR)?; + + write_config(&load_path, &load_lines)?; + write_config(&disabled_path, &disabled_lines)?; + write_config(&options_path, &options_lines)?; + + Ok(()) +} + +fn read_config_lines(ctx: &OsModifierContext, path: &str) -> Vec { + let full = ctx.path(path); + fs::read_to_string(&full) + .map(|s| s.lines().map(String::from).collect()) + .unwrap_or_default() +} + +fn remove_blacklist(lines: &mut Vec, module_name: &str) { + let entry = format!("blacklist {module_name}"); + lines.retain(|l| l.trim() != entry); +} + +fn update_options(lines: &mut Vec, module_name: &str, options: &std::collections::HashMap) { + // Remove any existing options line for this module + let prefix = format!("options {module_name} "); + lines.retain(|l| !l.starts_with(&prefix) && l.trim() != format!("options {module_name}")); + + // Build new options line + if !options.is_empty() { + let opts_str: Vec = options.iter().map(|(k, v)| format!("{k}={v}")).collect(); + lines.push(format!("options {module_name} {}", opts_str.join(" "))); + } +} + +fn ensure_dir(ctx: &OsModifierContext, path: &str) -> Result<(), Error> { + let full = ctx.path(path); + fs::create_dir_all(&full) + .with_context(|| format!("Failed to create directory '{}'", full.display())) +} + +fn write_config(path: &std::path::Path, lines: &[String]) -> Result<(), Error> { + let content = if lines.is_empty() { + String::new() + } else { + let mut s = lines.join("\n"); + s.push('\n'); + s + }; + fs::write(path, &content) + .with_context(|| format!("Failed to write config to '{}'", path.display())) +} diff --git a/crates/osmodifier/src/selinux.rs b/crates/osmodifier/src/selinux.rs new file mode 100644 index 000000000..b5de36d7b --- /dev/null +++ b/crates/osmodifier/src/selinux.rs @@ -0,0 +1,73 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! SELinux management — update /etc/selinux/config and GRUB cmdline args. + +use std::fs; + +use anyhow::{bail, Context, Error}; +use log::debug; +use trident_api::config::SelinuxMode; + +use crate::{default_grub::DefaultGrub, OsModifierContext}; + +const SELINUX_CONFIG_PATH: &str = "/etc/selinux/config"; + +/// Update the SELinux mode in /etc/selinux/config. +pub fn update_config_file(ctx: &OsModifierContext, mode: &SelinuxMode) -> Result<(), Error> { + let path = ctx.path(SELINUX_CONFIG_PATH); + + if !path.exists() { + bail!( + "SELinux config file not found at '{}'. \ + Ensure the selinux-policy package is installed.", + path.display() + ); + } + + let content = fs::read_to_string(&path) + .with_context(|| format!("Failed to read '{}'", path.display()))?; + + let selinux_value = match mode { + SelinuxMode::Enforcing => "enforcing", + SelinuxMode::Permissive => "permissive", + SelinuxMode::Disabled => "disabled", + }; + + // Replace the SELINUX= line + let re = regex::Regex::new(r"(?m)^SELINUX=.*$") + .context("Failed to compile SELinux regex")?; + + let new_content = if re.is_match(&content) { + re.replace(&content, &format!("SELINUX={selinux_value}")) + .to_string() + } else { + // Append if not present + format!("{content}\nSELINUX={selinux_value}\n") + }; + + debug!( + "Updating SELinux config at '{}' to '{selinux_value}'", + path.display() + ); + fs::write(&path, new_content) + .with_context(|| format!("Failed to write '{}'", path.display())) +} + +/// Update SELinux kernel command line args in the default GRUB config. +/// +/// This sets the `selinux` and `enforcing` args in GRUB_CMDLINE_LINUX, +/// matching the Go `UpdateSELinuxCommandLineForEMU` behavior. +pub fn update_grub_cmdline( + _ctx: &OsModifierContext, + default_grub: &mut DefaultGrub, + mode: &SelinuxMode, +) -> Result<(), Error> { + let new_args = match mode { + SelinuxMode::Enforcing => vec!["selinux=1".to_string(), "enforcing=1".to_string()], + SelinuxMode::Permissive => vec!["selinux=1".to_string(), "enforcing=0".to_string()], + SelinuxMode::Disabled => vec!["selinux=0".to_string()], + }; + + default_grub.update_cmdline_args(&["selinux", "enforcing"], &new_args) +} diff --git a/crates/osmodifier/src/services.rs b/crates/osmodifier/src/services.rs new file mode 100644 index 000000000..134af873d --- /dev/null +++ b/crates/osmodifier/src/services.rs @@ -0,0 +1,71 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Service management — enable and disable systemd services. + +use std::process::Command; + +use anyhow::{Context, Error}; +use log::{debug, warn}; + +use trident_api::config::Services; + +use crate::OsModifierContext; + +/// Enable and disable the requested systemd services. +pub fn configure(ctx: &OsModifierContext, services: &Services) -> Result<(), Error> { + for service in &services.enable { + enable_service(ctx, service)?; + } + + for service in &services.disable { + disable_service(ctx, service)?; + } + + Ok(()) +} + +fn enable_service(ctx: &OsModifierContext, service: &str) -> Result<(), Error> { + debug!("Enabling service '{service}'"); + let root = ctx.root.to_str().unwrap_or("/"); + + let output = Command::new("systemctl") + .args(["--root", root, "enable", service]) + .output() + .with_context(|| format!("Failed to execute systemctl enable {service}"))?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!("Failed to enable service '{service}': {stderr}"); + } + + Ok(()) +} + +fn disable_service(ctx: &OsModifierContext, service: &str) -> Result<(), Error> { + // Check if the service is enabled first + let root = ctx.root.to_str().unwrap_or("/"); + + let check = Command::new("systemctl") + .args(["--root", root, "is-enabled", service]) + .output() + .with_context(|| format!("Failed to check if service '{service}' is enabled"))?; + + if !check.status.success() { + warn!("Service '{service}' is not enabled, skipping disable"); + return Ok(()); + } + + debug!("Disabling service '{service}'"); + let output = Command::new("systemctl") + .args(["--root", root, "disable", service]) + .output() + .with_context(|| format!("Failed to execute systemctl disable {service}"))?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!("Failed to disable service '{service}': {stderr}"); + } + + Ok(()) +} diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs new file mode 100644 index 000000000..5cf89aca6 --- /dev/null +++ b/crates/osmodifier/src/users.rs @@ -0,0 +1,410 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! User management — create/update users, passwords, SSH keys, groups. + +use std::{ + fs, + io::Write, + os::unix::fs::PermissionsExt, + path::Path, + process::Command, +}; + +use anyhow::{bail, Context, Error}; +use log::{debug, info}; + +use crate::{ + config::{MICUser, PasswordType}, + OsModifierContext, +}; + +/// Add or update all configured users. +pub fn add_or_update_users(ctx: &OsModifierContext, users: &[MICUser]) -> Result<(), Error> { + for user in users { + add_or_update_user(ctx, user) + .with_context(|| format!("Failed to configure user '{}'", user.name))?; + } + Ok(()) +} + +fn add_or_update_user(ctx: &OsModifierContext, user: &MICUser) -> Result<(), Error> { + let root = ctx.root.to_str().unwrap_or("/"); + + // Hash the password if needed + let hashed_password = match &user.password { + Some(pwd) => match pwd.password_type { + PasswordType::PlainText => Some(hash_password(&pwd.value)?), + PasswordType::Hashed => Some(pwd.value.clone()), + PasswordType::Locked => None, + }, + None => None, + }; + + let user_exists = check_user_exists(root, &user.name)?; + + if user_exists { + debug!("User '{}' already exists, updating", user.name); + if user.uid.is_some() { + bail!( + "Cannot change UID for existing user '{}'. \ + Remove the UID field or delete the user first.", + user.name + ); + } + if user.home_directory.is_some() { + bail!( + "Cannot change home directory for existing user '{}'. \ + Remove the home directory field or delete the user first.", + user.name + ); + } + + // Update password if provided + if let Some(ref hash) = hashed_password { + update_user_password(ctx, &user.name, hash)?; + } + } else { + info!("Creating user '{}'", user.name); + create_user(root, user, hashed_password.as_deref())?; + } + + // Set password expiry + if let Some(days) = user.password_expires_days { + set_password_expiry(ctx, &user.name, days)?; + } + + // Update groups + if let Some(ref primary) = user.primary_group { + set_primary_group(root, &user.name, primary)?; + } + if !user.secondary_groups.is_empty() { + set_secondary_groups(root, &user.name, &user.secondary_groups)?; + } + + // SSH keys + if !user.ssh_public_keys.is_empty() { + write_ssh_keys(ctx, &user.name, &user.ssh_public_keys)?; + } + + // Startup command + if let Some(ref cmd) = user.startup_command { + set_startup_command(ctx, &user.name, cmd)?; + } + + Ok(()) +} + +fn check_user_exists(root: &str, username: &str) -> Result { + let status = if root == "/" { + Command::new("id").arg("-u").arg(username).status() + } else { + Command::new("chroot") + .arg(root) + .args(["id", "-u", username]) + .status() + } + .with_context(|| format!("Failed to check if user '{username}' exists"))?; + + Ok(status.success()) +} + +fn hash_password(plaintext: &str) -> Result { + // Use openssl to hash the password, matching the Go implementation + let mut child = Command::new("openssl") + .args(["passwd", "-6", "-stdin"]) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .context("Failed to start openssl passwd")?; + + if let Some(ref mut stdin) = child.stdin { + stdin + .write_all(plaintext.as_bytes()) + .context("Failed to write password to openssl stdin")?; + } + + let output = child + .wait_with_output() + .context("Failed to wait for openssl passwd")?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("openssl passwd failed: {stderr}"); + } + + Ok(String::from_utf8(output.stdout) + .context("openssl passwd produced non-UTF-8 output")? + .trim() + .to_string()) +} + +fn create_user(root: &str, user: &MICUser, hashed_password: Option<&str>) -> Result<(), Error> { + let mut cmd = if root == "/" { + Command::new("useradd") + } else { + let mut c = Command::new("chroot"); + c.arg(root).arg("useradd"); + c + }; + + cmd.arg("-m"); // Create home directory + + if let Some(ref hash) = hashed_password { + cmd.arg("-p").arg(hash); + } + + if let Some(uid) = user.uid { + cmd.arg("-u").arg(uid.to_string()); + } + + if let Some(ref home) = user.home_directory { + cmd.arg("-d").arg(home); + } + + if let Some(ref primary_group) = user.primary_group { + cmd.arg("-g").arg(primary_group); + } + + cmd.arg(&user.name); + + let output = cmd + .output() + .with_context(|| format!("Failed to execute useradd for '{}'", user.name))?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("useradd failed for '{}': {stderr}", user.name); + } + + Ok(()) +} + +fn update_user_password(ctx: &OsModifierContext, username: &str, hash: &str) -> Result<(), Error> { + debug!("Updating password for user '{username}'"); + let shadow_path = ctx.path("/etc/shadow"); + + let content = fs::read_to_string(&shadow_path) + .with_context(|| format!("Failed to read '{}'", shadow_path.display()))?; + + let mut found = false; + let updated: Vec = content + .lines() + .map(|line| { + let fields: Vec<&str> = line.split(':').collect(); + if fields.len() >= 2 && fields[0] == username { + found = true; + let mut new_fields: Vec = fields.iter().map(|f| f.to_string()).collect(); + new_fields[1] = hash.to_string(); + new_fields.join(":") + } else { + line.to_string() + } + }) + .collect(); + + if !found { + bail!("User '{username}' not found in shadow file"); + } + + let mut result = updated.join("\n"); + if content.ends_with('\n') { + result.push('\n'); + } + + fs::write(&shadow_path, &result) + .with_context(|| format!("Failed to write '{}'", shadow_path.display())) +} + +fn set_password_expiry(ctx: &OsModifierContext, username: &str, days: u64) -> Result<(), Error> { + debug!("Setting password expiry for '{username}' to {days} days"); + let shadow_path = ctx.path("/etc/shadow"); + + let content = fs::read_to_string(&shadow_path) + .with_context(|| format!("Failed to read '{}'", shadow_path.display()))?; + + let updated: Vec = content + .lines() + .map(|line| { + let fields: Vec<&str> = line.split(':').collect(); + if fields.len() >= 5 && fields[0] == username { + let mut new_fields: Vec = fields.iter().map(|f| f.to_string()).collect(); + // Field index 4 is the maximum password age + while new_fields.len() < 5 { + new_fields.push(String::new()); + } + new_fields[4] = days.to_string(); + new_fields.join(":") + } else { + line.to_string() + } + }) + .collect(); + + let mut result = updated.join("\n"); + if content.ends_with('\n') { + result.push('\n'); + } + + fs::write(&shadow_path, &result) + .with_context(|| format!("Failed to write '{}'", shadow_path.display())) +} + +fn set_primary_group(root: &str, username: &str, group: &str) -> Result<(), Error> { + debug!("Setting primary group for '{username}' to '{group}'"); + let output = if root == "/" { + Command::new("usermod") + .args(["-g", group, username]) + .output() + } else { + Command::new("chroot") + .arg(root) + .args(["usermod", "-g", group, username]) + .output() + } + .with_context(|| format!("Failed to set primary group for '{username}'"))?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("usermod -g failed for '{username}': {stderr}"); + } + Ok(()) +} + +fn set_secondary_groups(root: &str, username: &str, groups: &[String]) -> Result<(), Error> { + let groups_str = groups.join(","); + debug!("Setting secondary groups for '{username}' to '{groups_str}'"); + let output = if root == "/" { + Command::new("usermod") + .args(["-a", "-G", &groups_str, username]) + .output() + } else { + Command::new("chroot") + .arg(root) + .args(["usermod", "-a", "-G", &groups_str, username]) + .output() + } + .with_context(|| format!("Failed to set secondary groups for '{username}'"))?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("usermod -a -G failed for '{username}': {stderr}"); + } + Ok(()) +} + +fn write_ssh_keys(ctx: &OsModifierContext, username: &str, keys: &[String]) -> Result<(), Error> { + // Determine home directory + let home = get_home_dir(ctx, username)?; + let ssh_dir = home.join(".ssh"); + let auth_keys_path = ssh_dir.join("authorized_keys"); + + debug!( + "Writing {} SSH key(s) for '{username}' to '{}'", + keys.len(), + auth_keys_path.display() + ); + + // Create .ssh directory + fs::create_dir_all(&ssh_dir) + .with_context(|| format!("Failed to create '{}'", ssh_dir.display()))?; + + // Set directory permissions to 0700 + fs::set_permissions(&ssh_dir, fs::Permissions::from_mode(0o700)) + .with_context(|| format!("Failed to set permissions on '{}'", ssh_dir.display()))?; + + // Write authorized_keys + let content = keys.join("\n") + "\n"; + fs::write(&auth_keys_path, &content) + .with_context(|| format!("Failed to write '{}'", auth_keys_path.display()))?; + + // Set file permissions to 0600 + fs::set_permissions(&auth_keys_path, fs::Permissions::from_mode(0o600)) + .with_context(|| format!("Failed to set permissions on '{}'", auth_keys_path.display()))?; + + // Set ownership to the user + set_ownership(ctx, username, &ssh_dir)?; + set_ownership(ctx, username, &auth_keys_path)?; + + Ok(()) +} + +fn get_home_dir(ctx: &OsModifierContext, username: &str) -> Result { + let passwd_path = ctx.path("/etc/passwd"); + let content = fs::read_to_string(&passwd_path) + .with_context(|| format!("Failed to read '{}'", passwd_path.display()))?; + + for line in content.lines() { + let fields: Vec<&str> = line.split(':').collect(); + if fields.len() >= 6 && fields[0] == username { + return Ok(ctx.path(fields[5])); + } + } + + bail!("Could not find home directory for user '{username}' in /etc/passwd") +} + +fn set_ownership(ctx: &OsModifierContext, username: &str, path: &Path) -> Result<(), Error> { + let root = ctx.root.to_str().unwrap_or("/"); + let path_str = path + .to_str() + .context("Failed to convert path to string")?; + + let output = if root == "/" { + Command::new("chown") + .args([&format!("{username}:{username}"), path_str]) + .output() + } else { + // For non-root context, strip the root prefix for chroot + let relative = path.strip_prefix(&ctx.root).unwrap_or(path); + let rel_str = relative.to_str().context("path to string")?; + Command::new("chroot") + .arg(root) + .args(["chown", &format!("{username}:{username}"), rel_str]) + .output() + } + .with_context(|| format!("Failed to chown '{}'", path.display()))?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("chown failed for '{}': {stderr}", path.display()); + } + Ok(()) +} + +fn set_startup_command(ctx: &OsModifierContext, username: &str, cmd: &str) -> Result<(), Error> { + debug!("Setting startup command for '{username}' to '{cmd}'"); + let passwd_path = ctx.path("/etc/passwd"); + + let content = fs::read_to_string(&passwd_path) + .with_context(|| format!("Failed to read '{}'", passwd_path.display()))?; + + let mut found = false; + let updated: Vec = content + .lines() + .map(|line| { + let fields: Vec<&str> = line.split(':').collect(); + if fields.len() >= 7 && fields[0] == username { + found = true; + let mut new_fields: Vec = fields.iter().map(|f| f.to_string()).collect(); + new_fields[6] = cmd.to_string(); + new_fields.join(":") + } else { + line.to_string() + } + }) + .collect(); + + if !found { + bail!("User '{username}' not found in /etc/passwd"); + } + + let mut result = updated.join("\n"); + if content.ends_with('\n') { + result.push('\n'); + } + + fs::write(&passwd_path, &result) + .with_context(|| format!("Failed to write '{}'", passwd_path.display())) +} diff --git a/crates/osutils/Cargo.toml b/crates/osutils/Cargo.toml index 839eac14b..42e0085e6 100644 --- a/crates/osutils/Cargo.toml +++ b/crates/osutils/Cargo.toml @@ -39,6 +39,7 @@ rand = {workspace = true, optional = true} pytest_gen = { path = "../pytest_gen" } pytest = { path = "../pytest", optional = true } +osmodifier = { path = "../osmodifier" } trident_api = { path = "../trident_api" } sysdefs = { path = "../sysdefs" } diff --git a/crates/osutils/src/osmodifier.rs b/crates/osutils/src/osmodifier.rs index 79f9fcdf4..cbcd2f62b 100644 --- a/crates/osutils/src/osmodifier.rs +++ b/crates/osutils/src/osmodifier.rs @@ -1,193 +1,22 @@ -use std::{io::Write, path::Path, process::Command}; +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. -use anyhow::{Context, Error}; -use log::{debug, trace, warn}; -use serde::{Deserialize, Serialize}; -use tempfile::NamedTempFile; +//! Re-exports from the `osmodifier` crate for backwards compatibility. +//! +//! The types and functions have moved to the standalone `osmodifier` crate. +//! This module re-exports them so that existing `osutils::osmodifier::*` +//! imports continue to work during the migration. -use trident_api::config::{KernelCommandLine, Module, Selinux, Services}; +pub use osmodifier::{ + BootConfig, CorruptionOption, IdentifiedPartition, MICPassword, MICUser, OSModifierConfig, + Overlay, PasswordType, Verity, +}; -use crate::{exe::RunAndCheck, osmodifier}; - -#[derive(Serialize, Deserialize, Default)] -#[serde(rename_all = "camelCase")] -pub struct OSModifierConfig { - #[serde(skip_serializing_if = "Vec::is_empty")] - pub users: Vec, - - #[serde(skip_serializing_if = "Option::is_none")] - pub hostname: Option, - - #[serde(skip_serializing_if = "Vec::is_empty")] - pub modules: Vec, - - #[serde(skip_serializing_if = "Option::is_none")] - pub services: Option, - - #[serde(skip_serializing_if = "Option::is_none")] - pub kernel_command_line: Option, - - #[serde(skip_serializing_if = "Option::is_none")] - pub selinux: Option, -} - -impl OSModifierConfig { - pub fn call_os_modifier(&self, os_modifier_path: &Path) -> Result<(), Error> { - let os_modifier_config_yaml = - serde_yaml::to_string(&self).context("Failed to serialize OS modifier config")?; - - if os_modifier_config_yaml.is_empty() { - // Should never get here, but in case the OS modifier config is empty, return early - // without calling binary - warn!("OS modifier config is empty. OS modifier will not be called."); - return Ok(()); - } - - debug!("Calling OS modifier"); - trace!( - "Calling OS modifier with the following config:\n{}", - os_modifier_config_yaml - ); - let mut config_file = NamedTempFile::new().context("Failed to create a temporary file")?; - config_file - .write_all(os_modifier_config_yaml.as_bytes()) - .and_then(|_| config_file.flush()) - .context("Failed to write OS modifier config to temporary file and flush")?; - osmodifier::run(os_modifier_path, config_file.path()) - .context("Failed to run OS modifier")?; - Ok(()) - } -} +use serde::Serialize; +use trident_api::config::Services; #[derive(Serialize)] #[serde(rename_all = "camelCase")] pub struct MICServices { pub services: Services, } - -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] -#[serde(rename_all = "kebab-case")] -pub enum PasswordType { - Locked, - PlainText, - Hashed, -} - -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] -#[serde(rename_all = "camelCase")] -pub struct MICPassword { - #[serde(rename = "type")] - pub password_type: PasswordType, - pub value: String, -} - -/// A helper struct to convert user into MIC's user format -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] -#[serde(rename_all = "camelCase")] -pub struct MICUser { - pub name: String, - - #[serde(skip_serializing_if = "Option::is_none")] - pub uid: Option, - - #[serde(skip_serializing_if = "Option::is_none")] - pub password: Option, - - #[serde(default, skip_serializing_if = "Option::is_none")] - pub password_expires_days: Option, - - #[serde(skip_serializing_if = "Vec::is_empty")] - pub ssh_public_keys: Vec, - - #[serde(skip_serializing_if = "Option::is_none")] - pub primary_group: Option, - - #[serde(skip_serializing_if = "Vec::is_empty")] - pub secondary_groups: Vec, - - #[serde(skip_serializing_if = "Option::is_none")] - pub startup_command: Option, - - #[serde(skip_serializing_if = "Option::is_none")] - pub home_directory: Option, -} - -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] -#[serde(rename_all = "camelCase", deny_unknown_fields)] -pub struct Overlay { - pub lower_dir: String, - pub upper_dir: String, - pub work_dir: String, - pub partition: IdentifiedPartition, -} - -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Default)] -#[serde(rename_all = "camelCase", deny_unknown_fields)] -pub struct IdentifiedPartition { - pub id: String, -} - -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Default)] -#[serde(rename_all = "camelCase", deny_unknown_fields)] -pub struct Verity { - pub id: String, - pub name: String, - pub data_device: String, - pub hash_device: String, - #[serde(skip_serializing_if = "Option::is_none")] - pub corruption_option: Option, -} - -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] -#[serde(rename_all = "camelCase", deny_unknown_fields)] -/// Specifies the behavior in case of detected corruption. -pub enum CorruptionOption { - /// Default setting. Fails the I/O operation with an I/O error. - IoError, - - /// Ignores the corruption and continues operation. - Ignore, - - /// Causes the system to panic. This will print errors and try restarting the system - /// upon detecting corruption. - Panic, - - /// Attempts to restart the system upon detecting corruption. - Restart, -} - -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] -#[serde(rename_all = "camelCase", deny_unknown_fields)] -pub struct BootConfig { - #[serde(skip_serializing_if = "Option::is_none")] - pub selinux: Option, - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub overlays: Vec, - #[serde(skip_serializing_if = "Option::is_none")] - pub verity: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub root_device: Option, -} - -pub fn run(os_modifier_path: &Path, config_file: &Path) -> Result<(), Error> { - // Run the OS modifier with the configuration file - Command::new(os_modifier_path) - .arg("--config-file") - .arg(config_file) - .arg("--log-level=debug") - .run_and_check() - .context(format!( - "Failed to run OS modifier with config file {}", - config_file.display() - ))?; - - Ok(()) -} - -pub fn update_grub(os_modifier_path: &Path) -> Result<(), Error> { - Command::new(os_modifier_path.to_str().unwrap()) - .arg("--update-grub") - .arg("--log-level=debug") - .run_and_check() - .context("Failed to run OS modifier to update GRUB config") -} diff --git a/crates/trident/Cargo.toml b/crates/trident/Cargo.toml index 86582d537..f3ed56ce3 100644 --- a/crates/trident/Cargo.toml +++ b/crates/trident/Cargo.toml @@ -70,6 +70,7 @@ tonic-middleware = { workspace = true } # Local Crate Dependencies sysdefs = { path = "../sysdefs" } +osmodifier = { path = "../osmodifier" } osutils = { path = "../osutils" } trident_api = { path = "../trident_api" } trident-proto = { path = "../trident-proto", features = ["server", "log"] } diff --git a/crates/trident/src/engine/boot/grub.rs b/crates/trident/src/engine/boot/grub.rs index 879eac1ad..176ac67ff 100644 --- a/crates/trident/src/engine/boot/grub.rs +++ b/crates/trident/src/engine/boot/grub.rs @@ -1,15 +1,14 @@ -use std::{fs, io::Write, path::Path}; +use std::{fs, path::Path}; use anyhow::{bail, Context, Error}; use log::{debug, info, trace}; -use tempfile::NamedTempFile; use uuid::Uuid; +use osmodifier::{BootConfig, IdentifiedPartition, Overlay, OsModifierContext, Verity}; use osutils::{ blkid, grub::GrubConfig, grub_mkconfig::GrubMkConfigScript, - osmodifier::{self, BootConfig, IdentifiedPartition, Overlay, Verity}, osrelease::{AzureLinuxRelease, Distro}, }; use trident_api::{ @@ -36,7 +35,7 @@ fn update_grub_config_esp(grub_config_path: &Path, boot_fs_uuid: &Uuid) -> Resul grub_config.write() } -pub(super) fn update_configs(ctx: &EngineContext, os_modifier_path: &Path) -> Result<(), Error> { +pub(super) fn update_configs(ctx: &EngineContext) -> Result<(), Error> { // Get the root block device path let root_device_path = ctx .get_root_block_device_path() @@ -68,7 +67,6 @@ pub(super) fn update_configs(ctx: &EngineContext, os_modifier_path: &Path) -> Re Distro::AzureLinux(AzureLinuxRelease::AzL3) => { update_grub_config_azl3( ctx, - os_modifier_path, &root_device_path, &boot_grub_config_path, )?; @@ -94,7 +92,6 @@ pub(super) fn update_configs(ctx: &EngineContext, os_modifier_path: &Path) -> Re /// Updates the GRUB config for Azure Linux 3.0 using OS modifier. fn update_grub_config_azl3( ctx: &EngineContext, - os_modifier_path: &Path, root_device_path: &Path, boot_grub_config_path: &Path, ) -> Result<(), Error> { @@ -122,7 +119,8 @@ fn update_grub_config_azl3( grub_config ); - osmodifier::update_grub(os_modifier_path)?; + let osmod_ctx = OsModifierContext::default(); + osmodifier::update_default_grub(&osmod_ctx)?; let updated_grub_config = fs::read_to_string(boot_grub_config_path)?; trace!( @@ -225,27 +223,8 @@ fn update_grub_config_azl3( root_device: Some(root_device_str.to_string()), }; - let boot_config_yaml = serde_yaml::to_string(&config).context("Failed to serialize to YAML")?; - - // Create a temporary file and write the config to it - let mut tmpfile = NamedTempFile::new().context("Failed to create a temporary file")?; - tmpfile - .write_all(boot_config_yaml.as_bytes()) - .context(format!( - "Failed to write boot config to temporary file at {:?}", - tmpfile.path() - ))?; - tmpfile.flush().context(format!( - "Failed to flush temporary file at {:?}", - tmpfile.path() - ))?; - - osmodifier::run(os_modifier_path, tmpfile.path()).with_context(|| { - format!( - "Failed to run OS modifier to update GRUB config with temporary config file at {:?}", - tmpfile.path() - ) - })?; + osmodifier::modify_boot(&osmod_ctx, &config) + .context("Failed to apply boot configuration modifications")?; debug!("Finished updating GRUB config for Azure Linux 3.0 with OS modifier"); @@ -262,10 +241,7 @@ pub(crate) mod functional_test { use const_format::formatcp; use maplit::btreemap; - use crate::{ - engine::{boot::get_update_esp_dir_name, storage::raid}, - OS_MODIFIER_BINARY_PATH, - }; + use crate::engine::{boot::get_update_esp_dir_name, storage::raid}; use osutils::{ block_devices, @@ -601,7 +577,7 @@ pub(crate) mod functional_test { let _a = setup_mock_grub_configs(ctx); - update_configs(ctx, Path::new(OS_MODIFIER_BINARY_PATH)) + update_configs(ctx) } #[functional_test(feature = "helpers")] @@ -673,7 +649,7 @@ pub(crate) mod functional_test { let _a = setup_mock_grub_configs(&ctx); - update_configs(&ctx, Path::new(OS_MODIFIER_BINARY_PATH)).unwrap(); + update_configs(&ctx).unwrap(); } #[functional_test(feature = "helpers")] @@ -760,7 +736,7 @@ pub(crate) mod functional_test { let _a = setup_mock_grub_configs(&ctx); - update_configs(&ctx, Path::new(OS_MODIFIER_BINARY_PATH)).unwrap(); + update_configs(&ctx).unwrap(); } #[functional_test(feature = "helpers")] diff --git a/crates/trident/src/engine/boot/mod.rs b/crates/trident/src/engine/boot/mod.rs index b076b2324..b7b4d3f54 100644 --- a/crates/trident/src/engine/boot/mod.rs +++ b/crates/trident/src/engine/boot/mod.rs @@ -12,7 +12,7 @@ use trident_api::{ status::AbVolumeSelection, }; -use crate::{engine::Subsystem, OS_MODIFIER_NEWROOT_PATH}; +use crate::engine::Subsystem; use super::EngineContext; @@ -40,7 +40,7 @@ impl Subsystem for BootSubsystem { return Ok(()); } - grub::update_configs(ctx, Path::new(OS_MODIFIER_NEWROOT_PATH)) + grub::update_configs(ctx) .structured(ServicingError::UpdateGrubConfigs)?; Ok(()) diff --git a/crates/trident/src/engine/newroot.rs b/crates/trident/src/engine/newroot.rs index d039e45de..e08777e09 100644 --- a/crates/trident/src/engine/newroot.rs +++ b/crates/trident/src/engine/newroot.rs @@ -22,7 +22,7 @@ use trident_api::{ BlockDeviceId, }; -use crate::{OS_MODIFIER_BINARY_PATH, OS_MODIFIER_NEWROOT_PATH}; +use crate::engine::EngineContext; /// NewrootMount represents all the necessary mounting points for newroot and /// the nested execmount to exit the chroot jail. It is also responsible for @@ -82,21 +82,7 @@ impl NewrootMount { newroot_mount.mount_tmpfs("/tmp")?; newroot_mount.mount_tmpfs("/run")?; - if Path::new(OS_MODIFIER_BINARY_PATH).exists() { - // Bind mount the execroot binary to the newroot - debug!("Bind mounting osmodifier binary into newroot"); - let mount_path = path::join_relative(newroot_mount.path(), OS_MODIFIER_NEWROOT_PATH); - - fs::write(&mount_path, b"").structured(ServicingError::MountExecrootBinary)?; - - MountBuilder::default() - .flags(MountFlags::BIND) - .mount(OS_MODIFIER_BINARY_PATH, &mount_path) - .structured(ServicingError::MountExecrootBinary)?; - newroot_mount.mounts.push(mount_path); - } else { - debug!("Skipping bind mount of osmodifier binary into newroot"); - } + // OS modifier is now a library — no binary bind mount needed. Ok(newroot_mount) } diff --git a/crates/trident/src/lib.rs b/crates/trident/src/lib.rs index 1b9596b7f..23d07a2fa 100644 --- a/crates/trident/src/lib.rs +++ b/crates/trident/src/lib.rs @@ -89,12 +89,6 @@ const TRIDENT_BINARY_PATH: &str = "/usr/bin/trident"; /// Launcher binary path. const LAUNCHER_BINARY_PATH: &str = "/usr/bin/launcher"; -/// OS Modifier (EMU) binary path. -const OS_MODIFIER_BINARY_PATH: &str = "/usr/bin/osmodifier"; - -/// Path to OS Modifier on the newroot. -const OS_MODIFIER_NEWROOT_PATH: &str = "/tmp/osmodifier"; - /// Path to the Trident background log for the current servicing. pub const TRIDENT_BACKGROUND_LOG_PATH: &str = "/var/log/trident-full.log"; diff --git a/crates/trident/src/subsystems/osconfig/mod.rs b/crates/trident/src/subsystems/osconfig/mod.rs index 823c34cf3..752d2b179 100644 --- a/crates/trident/src/subsystems/osconfig/mod.rs +++ b/crates/trident/src/subsystems/osconfig/mod.rs @@ -1,21 +1,19 @@ -use std::{fs, path::Path}; +use std::fs; use anyhow::Context; use log::{debug, error, info, warn}; -use osutils::{osmodifier::OSModifierConfig, path}; +use osmodifier::{OSModifierConfig, OsModifierContext}; +use osutils::path; use trident_api::{ config::{ManagementOs, Services, SshMode}, constants::internal_params::DISABLE_HOSTNAME_CARRY_OVER, - error::{ExecutionEnvironmentMisconfigurationError, ReportError, ServicingError, TridentError}, + error::{ReportError, ServicingError, TridentError}, is_default, status::ServicingType, }; -use crate::{ - engine::{EngineContext, Subsystem, RUNS_ON_ALL}, - OS_MODIFIER_BINARY_PATH, OS_MODIFIER_NEWROOT_PATH, -}; +use crate::engine::{EngineContext, Subsystem, RUNS_ON_ALL}; mod users; @@ -125,17 +123,8 @@ impl Subsystem for OsConfigSubsystem { Ok(ServicingType::NoActiveServicing) } - fn validate_host_config(&self, ctx: &EngineContext) -> Result<(), TridentError> { - // If the os-modifier binary is required but not present, return an error. - if os_changes_required(ctx) && !Path::new(OS_MODIFIER_BINARY_PATH).exists() { - return Err(TridentError::new( - ExecutionEnvironmentMisconfigurationError::FindOSModifierBinary { - binary_path: OS_MODIFIER_BINARY_PATH.to_string(), - config: self.name().to_string(), - }, - )); - } - + fn validate_host_config(&self, _ctx: &EngineContext) -> Result<(), TridentError> { + // OS modifier is now a library crate, no external binary needed. Ok(()) } @@ -262,8 +251,7 @@ impl OsConfigSubsystem { os_modifier_config.selinux = Some(ctx.spec.os.selinux.clone()); } - os_modifier_config - .call_os_modifier(Path::new(OS_MODIFIER_NEWROOT_PATH)) + osmodifier::modify_os(&OsModifierContext::default(), &os_modifier_config) .structured(ServicingError::RunOsModifier) } @@ -289,8 +277,7 @@ impl OsConfigSubsystem { services: Some(services), ..Default::default() }; - return os_modifier_config - .call_os_modifier(Path::new(OS_MODIFIER_BINARY_PATH)) + return osmodifier::modify_os(&OsModifierContext::default(), &os_modifier_config) .structured(ServicingError::RunOsModifier); } Ok(()) @@ -314,16 +301,10 @@ impl Subsystem for MosConfigSubsystem { return Ok(()); } - // If the os-modifier binary is required but not present, return an error. - if mos_config_requires_os_modifier(&ctx.spec.management_os) - && !Path::new(OS_MODIFIER_BINARY_PATH).exists() - { - return Err(TridentError::new( - ExecutionEnvironmentMisconfigurationError::FindOSModifierBinary { - binary_path: OS_MODIFIER_BINARY_PATH.to_string(), - config: self.name().to_string(), - }, - )); + // OS modifier is now a library crate, no external binary needed. + if mos_config_requires_os_modifier(&ctx.spec.management_os) { + // Validation still passes — we just don't need to check for a binary. + debug!("MOS config requires OS modifications (now handled by library)"); } Ok(()) @@ -339,10 +320,6 @@ impl Subsystem for MosConfigSubsystem { return Ok(()); } - // Get the path to the os-modifier binary. We've already validated that - // it exists when required in 'validate_host_config'. - let os_modifier_path = Path::new(OS_MODIFIER_BINARY_PATH); - if !ctx.spec.management_os.users.is_empty() { info!("Setting up users for management OS"); let os_modifier_config = OSModifierConfig { @@ -350,8 +327,7 @@ impl Subsystem for MosConfigSubsystem { .structured(ServicingError::SetUpUsers)?, ..Default::default() }; - os_modifier_config - .call_os_modifier(os_modifier_path) + osmodifier::modify_os(&OsModifierContext::default(), &os_modifier_config) .structured(ServicingError::RunOsModifier)?; // If the config enables SSH for any MOS user, then we changed the @@ -700,8 +676,6 @@ mod tests { mod functional_test { use super::*; - use sys_mount::{MountBuilder, MountFlags, Unmount, UnmountFlags}; - use pytest_gen::functional_test; use trident_api::config::{HostConfiguration, Os}; @@ -728,14 +702,7 @@ mod functional_test { }; assert!(os_changes_required(&ctx)); - fs::write(OS_MODIFIER_NEWROOT_PATH, b"").unwrap(); - let _mount = MountBuilder::default() - .flags(MountFlags::BIND) - .mount(OS_MODIFIER_BINARY_PATH, OS_MODIFIER_NEWROOT_PATH) - .unwrap() - .into_unmount_drop(UnmountFlags::empty()); - - // Configure OsConfig subsystem + // Configure OsConfig subsystem (osmodifier is now a library, no binary mount needed) let mut os_config_subsystem = OsConfigSubsystem::default(); let _ = os_config_subsystem.configure(&ctx); @@ -769,14 +736,8 @@ mod functional_test { }; assert!(os_changes_required(&ctx)); - fs::write(OS_MODIFIER_NEWROOT_PATH, b"").unwrap(); - let _mount = MountBuilder::default() - .flags(MountFlags::BIND) - .mount(OS_MODIFIER_BINARY_PATH, OS_MODIFIER_NEWROOT_PATH) - .unwrap() - .into_unmount_drop(UnmountFlags::empty()); - // Configure OsConfig subsystem and set prev_hostname parameter + // (osmodifier is now a library, no binary mount needed) let mut os_config_subsystem = OsConfigSubsystem { prev_hostname: Some("carry-over-hostname".into()), }; diff --git a/packaging/docker/Dockerfile.azl3 b/packaging/docker/Dockerfile.azl3 index bca35ff22..d2356694f 100644 --- a/packaging/docker/Dockerfile.azl3 +++ b/packaging/docker/Dockerfile.azl3 @@ -10,7 +10,6 @@ COPY packaging/rpm/trident.spec . COPY packaging ./packaging COPY bin/trident ./target/release/trident COPY bin/trident-acl-agent ./target/release/trident-acl-agent -COPY artifacts/osmodifier /usr/src/azl/SOURCES/osmodifier ARG TRIDENT_VERSION=dev-build ARG RPM_VER=0.1.0 diff --git a/packaging/docker/Dockerfile.full b/packaging/docker/Dockerfile.full index 078ed812d..beef29797 100644 --- a/packaging/docker/Dockerfile.full +++ b/packaging/docker/Dockerfile.full @@ -8,7 +8,6 @@ WORKDIR /work COPY packaging/rpm/trident.spec . COPY packaging ./packaging -COPY artifacts/osmodifier /usr/src/azl/SOURCES/osmodifier COPY .cargo/config.toml ./.cargo/config.toml COPY Cargo.toml . diff --git a/packaging/docker/Dockerfile.full.public b/packaging/docker/Dockerfile.full.public index c95141552..c4a469bc4 100644 --- a/packaging/docker/Dockerfile.full.public +++ b/packaging/docker/Dockerfile.full.public @@ -8,7 +8,6 @@ WORKDIR /work COPY trident.spec . COPY packaging ./packaging -COPY artifacts/osmodifier /usr/src/azl/SOURCES/osmodifier COPY .cargo/config ./.cargo/config COPY Cargo.toml . diff --git a/packaging/docker/Dockerfile.runtime b/packaging/docker/Dockerfile.runtime index 514cc2104..7cd1fd3c1 100644 --- a/packaging/docker/Dockerfile.runtime +++ b/packaging/docker/Dockerfile.runtime @@ -22,9 +22,6 @@ RUN tdnf -y install \ RUN \ --mount=type=bind,source=./bin/RPMS,target=/trident \ - if [ -n "$(find /trident/x86_64 -name 'azurelinux-image-tools-osmodifier-*.rpm')" ]; then \ - tdnf install -y /trident/x86_64/azurelinux-image-tools-osmodifier-*.rpm; \ - fi && \ tdnf install -y \ /trident/x86_64/trident-0*.rpm && \ tdnf install -y \ diff --git a/packaging/rpm/trident.spec b/packaging/rpm/trident.spec index eec9158e3..cf3b8807d 100644 --- a/packaging/rpm/trident.spec +++ b/packaging/rpm/trident.spec @@ -34,8 +34,6 @@ Source0: https://github.com/microsoft/trident/archive/refs/tags/v%{versio # tar -czf %%{name}-%%{version}-cargo.tar.gz vendor/ # Source1: %{name}-%{version}-cargo.tar.gz -%else -Source1: osmodifier %endif BuildRequires: openssl-devel @@ -45,9 +43,8 @@ BuildRequires: systemd-units BuildRequires: rust %if %{undefined rpm_ver} -# For distro build, require cargo to build and osmodifier +# For distro build, require cargo to build BuildRequires: cargo -Requires: azurelinux-image-tools-osmodifier %endif Requires: e2fsprogs @@ -83,10 +80,6 @@ and its dependencies for managing the lifecycle of Azure Linux hosts. %files %{_bindir}/%{name} %dir /etc/%{name} -%if %{defined rpm_ver} -# For Trident repo build, package osmodifier included via `Source1` -%{_bindir}/osmodifier -%endif %{_unitdir}/%{name}d.service %{_unitdir}/%{name}d.socket @@ -283,11 +276,6 @@ cargo test --all --no-fail-fast -- --skip test_run_systemd_check --skip test_pre %endif %install -%if %{defined rpm_ver} -# For Trident repo build, package osmodifier included via `Source1`. -# Distro RPM will use distro osmodifier RPM via Requires directive. -install -D -m 755 %{SOURCE1} %{buildroot}%{_bindir}/osmodifier -%endif install -D -m 755 target/release/%{name} %{buildroot}/%{_bindir}/%{name} install -D -m 755 target/release/%{name}-acl-agent %{buildroot}/%{_bindir}/%{name}-acl-agent From 9684c93b066c392f42ab16bf6687079efb407d28 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Mon, 11 May 2026 12:14:30 -0700 Subject: [PATCH 02/20] fix: correct module.options type (HashMap not Option) and remove unused import Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/src/grub_cfg.rs | 2 +- crates/osmodifier/src/modules.rs | 16 +++++++--------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/osmodifier/src/grub_cfg.rs b/crates/osmodifier/src/grub_cfg.rs index 5206fd7f4..97ee333ad 100644 --- a/crates/osmodifier/src/grub_cfg.rs +++ b/crates/osmodifier/src/grub_cfg.rs @@ -6,7 +6,7 @@ //! Used by the `update_default_grub` flow to extract boot args from the //! generated grub.cfg and sync them back to /etc/default/grub. -use std::{fs, path::Path, process::Command}; +use std::{fs, process::Command}; use anyhow::{bail, Context, Error}; use log::{debug, info, trace}; diff --git a/crates/osmodifier/src/modules.rs b/crates/osmodifier/src/modules.rs index 8b3a3a7e1..6fcec8f39 100644 --- a/crates/osmodifier/src/modules.rs +++ b/crates/osmodifier/src/modules.rs @@ -40,8 +40,8 @@ pub fn configure(ctx: &OsModifierContext, modules: &[Module]) -> Result<(), Erro load_lines.push(module.name.clone()); } // Set options if provided - if let Some(ref opts) = module.options { - update_options(&mut options_lines, &module.name, opts); + if !module.options.is_empty() { + update_options(&mut options_lines, &module.name, &module.options); } } LoadMode::Auto => { @@ -49,13 +49,13 @@ pub fn configure(ctx: &OsModifierContext, modules: &[Module]) -> Result<(), Erro // Remove from blacklist if present remove_blacklist(&mut disabled_lines, &module.name); // Set options if provided - if let Some(ref opts) = module.options { - update_options(&mut options_lines, &module.name, opts); + if !module.options.is_empty() { + update_options(&mut options_lines, &module.name, &module.options); } } LoadMode::Disable => { debug!("Module '{}': set to disabled", module.name); - if module.options.is_some() { + if !module.options.is_empty() { bail!( "Module '{}' is disabled but has options set — this is not allowed", module.name @@ -75,10 +75,8 @@ pub fn configure(ctx: &OsModifierContext, modules: &[Module]) -> Result<(), Erro let is_disabled = disabled_lines .iter() .any(|l| l.trim() == format!("blacklist {}", module.name)); - if !is_disabled { - if let Some(ref opts) = module.options { - update_options(&mut options_lines, &module.name, opts); - } + if !is_disabled && !module.options.is_empty() { + update_options(&mut options_lines, &module.name, &module.options); } } } From accd65e8a8006d69c1fa0bea2f5f4fd46f98a357 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Mon, 11 May 2026 12:22:44 -0700 Subject: [PATCH 03/20] fix: remove osmodifier binary from pipelines, Makefile, and functional tests Remove all download-osmodifier.yml template invocations, make artifacts/osmodifier targets, and functional test binary upload steps. The osmodifier is now compiled into the trident binary as a library crate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../testing_functional/functional-testing.yml | 6 ------ .../templates/stages/trident_rpms/build-source.yml | 12 ------------ .../stages/validate_makefile/dev-build.yml | 13 ------------- Makefile | 12 ++++-------- tests/functional_tests/conftest.py | 7 +------ 5 files changed, 5 insertions(+), 45 deletions(-) diff --git a/.pipelines/templates/stages/testing_functional/functional-testing.yml b/.pipelines/templates/stages/testing_functional/functional-testing.yml index 45a06b8d0..2c847909b 100644 --- a/.pipelines/templates/stages/testing_functional/functional-testing.yml +++ b/.pipelines/templates/stages/testing_functional/functional-testing.yml @@ -100,12 +100,6 @@ stages: - template: ../common_tasks/update-protoc.yml - - template: ../common_tasks/download-osmodifier.yml - parameters: - tridentSourceDirectory: $(TRIDENT_SOURCE_DIR) - osModifierBranch: ${{ parameters.osModifierBranch }} - targetArchitecture: amd64 - - bash: | set -eux diff --git a/.pipelines/templates/stages/trident_rpms/build-source.yml b/.pipelines/templates/stages/trident_rpms/build-source.yml index 305504299..f8e2e407b 100644 --- a/.pipelines/templates/stages/trident_rpms/build-source.yml +++ b/.pipelines/templates/stages/trident_rpms/build-source.yml @@ -101,12 +101,6 @@ stages: - template: ../common_tasks/cargo-auth.yml parameters: cargoConfigPath: $(TRIDENT_SOURCE_DIR)/.cargo/config.toml - - template: ../common_tasks/download-osmodifier.yml - parameters: - tridentSourceDirectory: $(TRIDENT_SOURCE_DIR) - targetArchitecture: ${{ parameters.targetArchitecture }} - osModifierBranch: ${{ parameters.osModifierBranch }} - osModifierBuildType: ${{ parameters.osModifierBuildType }} - template: release.yml parameters: targetArchitecture: ${{ parameters.targetArchitecture }} @@ -144,12 +138,6 @@ stages: set -eux sudo systemctl start docker displayName: Start Docker - - template: ../common_tasks/download-osmodifier.yml - parameters: - tridentSourceDirectory: $(TRIDENT_SOURCE_DIR) - targetArchitecture: ${{ parameters.targetArchitecture }} - osModifierBranch: ${{ parameters.osModifierBranch }} - osModifierBuildType: ${{ parameters.osModifierBuildType }} - template: release.yml parameters: targetArchitecture: ${{ parameters.targetArchitecture }} diff --git a/.pipelines/templates/stages/validate_makefile/dev-build.yml b/.pipelines/templates/stages/validate_makefile/dev-build.yml index 20244d87d..21420c9bb 100644 --- a/.pipelines/templates/stages/validate_makefile/dev-build.yml +++ b/.pipelines/templates/stages/validate_makefile/dev-build.yml @@ -85,19 +85,6 @@ stages: steps: - template: ../common_tasks/checkout_trident.yml - template: ../common_tasks/avoid-pypi-usage.yml - - bash: | - set -eux - make artifacts/osmodifier - rm -rf artifacts/osmodifier - displayName: Invoke make artifacts/osmodifier - workingDirectory: $(TRIDENT_SOURCE_DIR) - - - template: ../common_tasks/download-osmodifier.yml - parameters: - tridentSourceDirectory: $(TRIDENT_SOURCE_DIR) - osModifierBuildType: dev - osModifierBranch: ${{ parameters.osModifierBranch }} - targetArchitecture: amd64 - script: | set -eux diff --git a/Makefile b/Makefile index d9e374528..96734bf1e 100644 --- a/Makefile +++ b/Makefile @@ -373,7 +373,7 @@ functional-test: artifacts/trident-functest.qcow2 # A target for pipelines that skips all setup and building steps that are not # required in the pipeline environment. .PHONY: functional-test-core -functional-test-core: artifacts/osmodifier build-functional-test-cc generate-functional-test-manifest artifacts/trident-functest.qcow2 bin/virtdeploy +functional-test-core: build-functional-test-cc generate-functional-test-manifest artifacts/trident-functest.qcow2 bin/virtdeploy python3 -u -m \ pytest --color=yes \ --log-level=INFO \ @@ -390,7 +390,7 @@ functional-test-core: artifacts/osmodifier build-functional-test-cc generate-fun --build-output $(BUILD_OUTPUT) .PHONY: patch-functional-test -patch-functional-test: artifacts/osmodifier build-functional-test-cc generate-functional-test-manifest +patch-functional-test: build-functional-test-cc generate-functional-test-manifest python3 -u -m \ pytest --color=yes \ --log-level=INFO \ @@ -549,16 +549,14 @@ RUN_NETLAUNCH_TRIDENT_BIN ?= $(if $(filter yes,$(IS_UBUNTU_24_OR_NEWER)),target/ RUN_NETLAUNCH_LAUNCHER_BIN ?= $(if $(filter yes,$(IS_UBUNTU_24_OR_NEWER)),target/azl3/release/trident-acl-agent,target/release/trident-acl-agent) .PHONY: run-netlaunch run-netlaunch-stream -run-netlaunch: $(NETLAUNCH_CONFIG) $(TRIDENT_CONFIG) $(NETLAUNCH_ISO) bin/netlaunch validate artifacts/osmodifier $(RUN_NETLAUNCH_TRIDENT_BIN) $(RUN_NETLAUNCH_LAUNCHER_BIN) +run-netlaunch: $(NETLAUNCH_CONFIG) $(TRIDENT_CONFIG) $(NETLAUNCH_ISO) bin/netlaunch validate $(RUN_NETLAUNCH_TRIDENT_BIN) $(RUN_NETLAUNCH_LAUNCHER_BIN) @echo "Using trident binary: $(RUN_NETLAUNCH_TRIDENT_BIN)" @mkdir -p artifacts/test-image @cp $(RUN_NETLAUNCH_TRIDENT_BIN) artifacts/test-image/trident @cp $(RUN_NETLAUNCH_LAUNCHER_BIN) artifacts/test-image/trident-acl-agent - @cp artifacts/osmodifier artifacts/test-image/ @bin/netlaunch \ --trident-binary $(RUN_NETLAUNCH_TRIDENT_BIN) \ --launcher-binary $(RUN_NETLAUNCH_LAUNCHER_BIN) \ - --osmodifier-binary artifacts/osmodifier \ --rcp-agent-mode cli \ --iso $(NETLAUNCH_ISO) \ $(if $(NETLAUNCH_PORT),--port $(NETLAUNCH_PORT)) \ @@ -570,15 +568,13 @@ run-netlaunch: $(NETLAUNCH_CONFIG) $(TRIDENT_CONFIG) $(NETLAUNCH_ISO) bin/netlau --trace-file trident-metrics.jsonl \ $(if $(LOG_TRACE),--log-trace) -run-netlaunch-stream: $(NETLAUNCH_CONFIG) $(TRIDENT_CONFIG) $(NETLAUNCH_ISO) bin/netlaunch artifacts/osmodifier $(RUN_NETLAUNCH_TRIDENT_BIN) +run-netlaunch-stream: $(NETLAUNCH_CONFIG) $(TRIDENT_CONFIG) $(NETLAUNCH_ISO) bin/netlaunch $(RUN_NETLAUNCH_TRIDENT_BIN) @echo "Using trident binary: $(RUN_NETLAUNCH_TRIDENT_BIN)" @mkdir -p artifacts/test-image @cp $(RUN_NETLAUNCH_TRIDENT_BIN) artifacts/test-image/trident - @cp artifacts/osmodifier artifacts/test-image/ @bin/netlaunch \ --stream-image \ --trident-binary $(RUN_NETLAUNCH_TRIDENT_BIN) \ - --osmodifier-binary artifacts/osmodifier \ --rcp-agent-mode cli \ --iso $(NETLAUNCH_ISO) \ $(if $(NETLAUNCH_PORT),--port $(NETLAUNCH_PORT)) \ diff --git a/tests/functional_tests/conftest.py b/tests/functional_tests/conftest.py index bfaba0b0b..8627ef448 100644 --- a/tests/functional_tests/conftest.py +++ b/tests/functional_tests/conftest.py @@ -355,12 +355,7 @@ def vm(request, ssh_key_path, known_hosts_path) -> SshNode: known_hosts_path=known_hosts_path, ) - # Upload OS modifier binary to the VM. - osmodifier_path = request.config.getoption("--osmodifier") - logging.info(f"Copying osmodifier from {osmodifier_path} to VM") - ssh_node.copy(osmodifier_path, Path("osmodifier")) - ssh_node.execute("chmod +x osmodifier") - ssh_node.execute(f"sudo mv osmodifier {OS_MODIFIER_BIN_TARGET_PATH}") + # OS modifier is now compiled into the trident binary — no separate upload needed. if build_output: upload_test_binaries(build_output, force_upload, ssh_node) From 30afe99a5db7d6e487189806d68d43bc71d57425 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Mon, 11 May 2026 12:40:36 -0700 Subject: [PATCH 04/20] fix: restore Path import in osconfig and remove unused imports - Add back std::path::Path import in osconfig/mod.rs (needed for provision() signature) - Remove unused crate::engine::EngineContext import in newroot.rs - Remove unused std::path::Path import in boot/mod.rs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/boot/mod.rs | 2 -- crates/trident/src/engine/newroot.rs | 1 - crates/trident/src/subsystems/osconfig/mod.rs | 2 +- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/trident/src/engine/boot/mod.rs b/crates/trident/src/engine/boot/mod.rs index b7b4d3f54..3b336f525 100644 --- a/crates/trident/src/engine/boot/mod.rs +++ b/crates/trident/src/engine/boot/mod.rs @@ -1,5 +1,3 @@ -use std::path::Path; - use log::debug; use strum::IntoEnumIterator; diff --git a/crates/trident/src/engine/newroot.rs b/crates/trident/src/engine/newroot.rs index e08777e09..bfa0df86d 100644 --- a/crates/trident/src/engine/newroot.rs +++ b/crates/trident/src/engine/newroot.rs @@ -22,7 +22,6 @@ use trident_api::{ BlockDeviceId, }; -use crate::engine::EngineContext; /// NewrootMount represents all the necessary mounting points for newroot and /// the nested execmount to exit the chroot jail. It is also responsible for diff --git a/crates/trident/src/subsystems/osconfig/mod.rs b/crates/trident/src/subsystems/osconfig/mod.rs index 752d2b179..aa5205e14 100644 --- a/crates/trident/src/subsystems/osconfig/mod.rs +++ b/crates/trident/src/subsystems/osconfig/mod.rs @@ -1,4 +1,4 @@ -use std::fs; +use std::{fs, path::Path}; use anyhow::Context; use log::{debug, error, info, warn}; From 6303de6bc80b3eafe220db0fe000f9f56759d0a5 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Mon, 11 May 2026 20:32:38 +0000 Subject: [PATCH 05/20] fix: apply cargo fmt formatting Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 16 ++++++++++++++++ crates/osmodifier/src/default_grub.rs | 25 ++++++------------------- crates/osmodifier/src/grub_cfg.rs | 25 +++++-------------------- crates/osmodifier/src/lib.rs | 21 +++++++-------------- crates/osmodifier/src/modules.rs | 8 ++++++-- crates/osmodifier/src/selinux.rs | 6 ++---- crates/osmodifier/src/users.rs | 20 ++++++++------------ crates/trident/src/engine/boot/grub.rs | 8 ++------ crates/trident/src/engine/boot/mod.rs | 3 +-- crates/trident/src/engine/newroot.rs | 1 - 10 files changed, 53 insertions(+), 80 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2814f53ba..937896728 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1862,6 +1862,20 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "osmodifier" +version = "0.1.0" +dependencies = [ + "anyhow", + "indoc", + "log", + "regex", + "serde", + "serde_yaml", + "tempfile", + "trident_api", +] + [[package]] name = "osutils" version = "0.1.0" @@ -1881,6 +1895,7 @@ dependencies = [ "nix", "once_cell", "openssl", + "osmodifier", "pytest", "pytest_gen", "rand 0.9.0", @@ -3433,6 +3448,7 @@ dependencies = [ "netplan-types", "nix", "oci-client", + "osmodifier", "osutils", "procfs", "prost-types", diff --git a/crates/osmodifier/src/default_grub.rs b/crates/osmodifier/src/default_grub.rs index 88214a54f..b73664e10 100644 --- a/crates/osmodifier/src/default_grub.rs +++ b/crates/osmodifier/src/default_grub.rs @@ -93,9 +93,7 @@ impl DefaultGrub { old_keys: &[&str], new_args: &[String], ) -> Result<(), Error> { - let current = self - .get_variable("GRUB_CMDLINE_LINUX") - .unwrap_or_default(); + let current = self.get_variable("GRUB_CMDLINE_LINUX").unwrap_or_default(); let mut args: Vec = current .split_whitespace() @@ -117,17 +115,12 @@ impl DefaultGrub { /// Add extra command line arguments to GRUB_CMDLINE_LINUX without /// removing any existing ones. pub fn add_extra_cmdline(&mut self, extra: &[String]) { - let current = self - .get_variable("GRUB_CMDLINE_LINUX") - .unwrap_or_default(); + let current = self.get_variable("GRUB_CMDLINE_LINUX").unwrap_or_default(); let mut args: Vec = if current.is_empty() { Vec::new() } else { - current - .split_whitespace() - .map(String::from) - .collect() + current.split_whitespace().map(String::from).collect() }; args.extend(extra.iter().cloned()); @@ -194,10 +187,7 @@ mod tests { ); grub.set_variable("NEW_VAR", "new_value"); - assert_eq!( - grub.get_variable("NEW_VAR"), - Some("new_value".to_string()) - ); + assert_eq!(grub.get_variable("NEW_VAR"), Some("new_value".to_string())); } #[test] @@ -209,11 +199,8 @@ mod tests { path: PathBuf::from("/etc/default/grub"), }; - grub.update_cmdline_args( - &["selinux", "enforcing"], - &["selinux=0".to_string()], - ) - .unwrap(); + grub.update_cmdline_args(&["selinux", "enforcing"], &["selinux=0".to_string()]) + .unwrap(); let result = grub.get_variable("GRUB_CMDLINE_LINUX").unwrap(); assert!(result.contains("quiet")); diff --git a/crates/osmodifier/src/grub_cfg.rs b/crates/osmodifier/src/grub_cfg.rs index 97ee333ad..2044f8b43 100644 --- a/crates/osmodifier/src/grub_cfg.rs +++ b/crates/osmodifier/src/grub_cfg.rs @@ -18,13 +18,7 @@ use crate::OsModifierContext; const GRUB_CFG_PATHS: &[&str] = &["/boot/grub2/grub.cfg", "/boot/grub/grub.cfg"]; /// The grub.cfg args we want to extract for syncing to /etc/default/grub. -const SYNC_ARG_NAMES: &[&str] = &[ - "rd.overlayfs", - "roothash", - "root", - "selinux", - "enforcing", -]; +const SYNC_ARG_NAMES: &[&str] = &["rd.overlayfs", "roothash", "root", "selinux", "enforcing"]; /// Extract boot arguments from the generated grub.cfg. /// @@ -77,10 +71,7 @@ fn find_grub_cfg(ctx: &OsModifierContext) -> Result { return Ok(full); } } - bail!( - "Could not find grub.cfg at any of: {:?}", - GRUB_CFG_PATHS - ) + bail!("Could not find grub.cfg at any of: {:?}", GRUB_CFG_PATHS) } /// Find the linux command line from a non-recovery menuentry in grub.cfg. @@ -95,8 +86,7 @@ fn find_non_recovery_linux_line(content: &str) -> Result { // block, skip recovery entries, find the linux line. let menuentry_re = Regex::new(r#"^\s*menuentry\s+['"](.*?)['"]\s"#) .context("Failed to compile menuentry regex")?; - let linux_re = Regex::new(r"^\s*linux\s+(.+)$") - .context("Failed to compile linux regex")?; + let linux_re = Regex::new(r"^\s*linux\s+(.+)$").context("Failed to compile linux regex")?; let mut in_menuentry = false; let mut is_recovery = false; @@ -158,10 +148,7 @@ fn find_non_recovery_linux_line(content: &str) -> Result { pub fn run_grub_mkconfig(ctx: &OsModifierContext) -> Result<(), Error> { let grub_cfg_path = find_grub_cfg(ctx)?; - info!( - "Running grub2-mkconfig -o '{}'", - grub_cfg_path.display() - ); + info!("Running grub2-mkconfig -o '{}'", grub_cfg_path.display()); let output = Command::new("grub2-mkconfig") .arg("-o") @@ -172,9 +159,7 @@ pub fn run_grub_mkconfig(ctx: &OsModifierContext) -> Result<(), Error> { if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); - bail!( - "grub2-mkconfig failed:\nstdout: {stdout}\nstderr: {stderr}" - ); + bail!("grub2-mkconfig failed:\nstdout: {stdout}\nstderr: {stderr}"); } debug!("grub2-mkconfig completed successfully"); diff --git a/crates/osmodifier/src/lib.rs b/crates/osmodifier/src/lib.rs index 6ef669639..f8caf062e 100644 --- a/crates/osmodifier/src/lib.rs +++ b/crates/osmodifier/src/lib.rs @@ -64,30 +64,26 @@ pub fn modify_os(ctx: &OsModifierContext, config: &OSModifierConfig) -> Result<( if !config.users.is_empty() { info!("Configuring users"); - users::add_or_update_users(ctx, &config.users) - .context("Failed to configure users")?; + users::add_or_update_users(ctx, &config.users).context("Failed to configure users")?; } if let Some(ref name) = config.hostname { if !name.is_empty() { info!("Setting hostname to '{name}'"); - hostname::update(ctx, name) - .context("Failed to update hostname")?; + hostname::update(ctx, name).context("Failed to update hostname")?; } } if let Some(ref services) = config.services { if !services.enable.is_empty() || !services.disable.is_empty() { info!("Configuring services"); - services::configure(ctx, services) - .context("Failed to configure services")?; + services::configure(ctx, services).context("Failed to configure services")?; } } if !config.modules.is_empty() { info!("Configuring kernel modules"); - modules::configure(ctx, &config.modules) - .context("Failed to configure kernel modules")?; + modules::configure(ctx, &config.modules).context("Failed to configure kernel modules")?; } // Kernel command line and SELinux are handled via boot config, not here. @@ -111,8 +107,7 @@ pub fn modify_os(ctx: &OsModifierContext, config: &OSModifierConfig) -> Result<( info!("Adding extra kernel command line arguments"); default_grub::add_extra_cmdline(ctx, &kcl.extra_command_line) .context("Failed to add extra kernel command line")?; - grub_cfg::run_grub_mkconfig(ctx) - .context("Failed to regenerate GRUB config")?; + grub_cfg::run_grub_mkconfig(ctx).context("Failed to regenerate GRUB config")?; } } @@ -183,10 +178,8 @@ pub fn modify_boot(ctx: &OsModifierContext, config: &BootConfig) -> Result<(), E )); } let concatenated = overlay_configs.join(" "); - default_grub.update_cmdline_args( - &["rd.overlayfs"], - &[format!("rd.overlayfs={concatenated}")], - )?; + default_grub + .update_cmdline_args(&["rd.overlayfs"], &[format!("rd.overlayfs={concatenated}")])?; changed = true; } diff --git a/crates/osmodifier/src/modules.rs b/crates/osmodifier/src/modules.rs index 6fcec8f39..40c1897c6 100644 --- a/crates/osmodifier/src/modules.rs +++ b/crates/osmodifier/src/modules.rs @@ -8,7 +8,7 @@ use std::fs; use anyhow::{bail, Context, Error}; use log::debug; -use trident_api::config::{Module, LoadMode}; +use trident_api::config::{LoadMode, Module}; use crate::OsModifierContext; @@ -105,7 +105,11 @@ fn remove_blacklist(lines: &mut Vec, module_name: &str) { lines.retain(|l| l.trim() != entry); } -fn update_options(lines: &mut Vec, module_name: &str, options: &std::collections::HashMap) { +fn update_options( + lines: &mut Vec, + module_name: &str, + options: &std::collections::HashMap, +) { // Remove any existing options line for this module let prefix = format!("options {module_name} "); lines.retain(|l| !l.starts_with(&prefix) && l.trim() != format!("options {module_name}")); diff --git a/crates/osmodifier/src/selinux.rs b/crates/osmodifier/src/selinux.rs index b5de36d7b..7916d93b3 100644 --- a/crates/osmodifier/src/selinux.rs +++ b/crates/osmodifier/src/selinux.rs @@ -35,8 +35,7 @@ pub fn update_config_file(ctx: &OsModifierContext, mode: &SelinuxMode) -> Result }; // Replace the SELINUX= line - let re = regex::Regex::new(r"(?m)^SELINUX=.*$") - .context("Failed to compile SELinux regex")?; + let re = regex::Regex::new(r"(?m)^SELINUX=.*$").context("Failed to compile SELinux regex")?; let new_content = if re.is_match(&content) { re.replace(&content, &format!("SELINUX={selinux_value}")) @@ -50,8 +49,7 @@ pub fn update_config_file(ctx: &OsModifierContext, mode: &SelinuxMode) -> Result "Updating SELinux config at '{}' to '{selinux_value}'", path.display() ); - fs::write(&path, new_content) - .with_context(|| format!("Failed to write '{}'", path.display())) + fs::write(&path, new_content).with_context(|| format!("Failed to write '{}'", path.display())) } /// Update SELinux kernel command line args in the default GRUB config. diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs index 5cf89aca6..6c58b7cd4 100644 --- a/crates/osmodifier/src/users.rs +++ b/crates/osmodifier/src/users.rs @@ -3,13 +3,7 @@ //! User management — create/update users, passwords, SSH keys, groups. -use std::{ - fs, - io::Write, - os::unix::fs::PermissionsExt, - path::Path, - process::Command, -}; +use std::{fs, io::Write, os::unix::fs::PermissionsExt, path::Path, process::Command}; use anyhow::{bail, Context, Error}; use log::{debug, info}; @@ -320,8 +314,12 @@ fn write_ssh_keys(ctx: &OsModifierContext, username: &str, keys: &[String]) -> R .with_context(|| format!("Failed to write '{}'", auth_keys_path.display()))?; // Set file permissions to 0600 - fs::set_permissions(&auth_keys_path, fs::Permissions::from_mode(0o600)) - .with_context(|| format!("Failed to set permissions on '{}'", auth_keys_path.display()))?; + fs::set_permissions(&auth_keys_path, fs::Permissions::from_mode(0o600)).with_context(|| { + format!( + "Failed to set permissions on '{}'", + auth_keys_path.display() + ) + })?; // Set ownership to the user set_ownership(ctx, username, &ssh_dir)?; @@ -347,9 +345,7 @@ fn get_home_dir(ctx: &OsModifierContext, username: &str) -> Result Result<(), Error> { let root = ctx.root.to_str().unwrap_or("/"); - let path_str = path - .to_str() - .context("Failed to convert path to string")?; + let path_str = path.to_str().context("Failed to convert path to string")?; let output = if root == "/" { Command::new("chown") diff --git a/crates/trident/src/engine/boot/grub.rs b/crates/trident/src/engine/boot/grub.rs index 176ac67ff..01fc1be15 100644 --- a/crates/trident/src/engine/boot/grub.rs +++ b/crates/trident/src/engine/boot/grub.rs @@ -4,7 +4,7 @@ use anyhow::{bail, Context, Error}; use log::{debug, info, trace}; use uuid::Uuid; -use osmodifier::{BootConfig, IdentifiedPartition, Overlay, OsModifierContext, Verity}; +use osmodifier::{BootConfig, IdentifiedPartition, OsModifierContext, Overlay, Verity}; use osutils::{ blkid, grub::GrubConfig, @@ -65,11 +65,7 @@ pub(super) fn update_configs(ctx: &EngineContext) -> Result<(), Error> { // Update GRUB config on the boot device (volume holding /boot) match ctx.host_os_release.get_distro() { Distro::AzureLinux(AzureLinuxRelease::AzL3) => { - update_grub_config_azl3( - ctx, - &root_device_path, - &boot_grub_config_path, - )?; + update_grub_config_azl3(ctx, &root_device_path, &boot_grub_config_path)?; } d => bail!("Unsupported distro for GRUB config update: {d:?}"), diff --git a/crates/trident/src/engine/boot/mod.rs b/crates/trident/src/engine/boot/mod.rs index 3b336f525..097abc38b 100644 --- a/crates/trident/src/engine/boot/mod.rs +++ b/crates/trident/src/engine/boot/mod.rs @@ -38,8 +38,7 @@ impl Subsystem for BootSubsystem { return Ok(()); } - grub::update_configs(ctx) - .structured(ServicingError::UpdateGrubConfigs)?; + grub::update_configs(ctx).structured(ServicingError::UpdateGrubConfigs)?; Ok(()) } diff --git a/crates/trident/src/engine/newroot.rs b/crates/trident/src/engine/newroot.rs index bfa0df86d..e21d1fa96 100644 --- a/crates/trident/src/engine/newroot.rs +++ b/crates/trident/src/engine/newroot.rs @@ -22,7 +22,6 @@ use trident_api::{ BlockDeviceId, }; - /// NewrootMount represents all the necessary mounting points for newroot and /// the nested execmount to exit the chroot jail. It is also responsible for /// unmounting them in the correct order. NewrootMount provides information for: From e64322fc4b23343d33a8d761972ee2758c55a8e5 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Mon, 11 May 2026 20:53:53 +0000 Subject: [PATCH 06/20] fix: resolve clippy redundant_closure warning Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/src/lib.rs | 2 +- crates/trident/src/engine/boot/grub.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/osmodifier/src/lib.rs b/crates/osmodifier/src/lib.rs index f8caf062e..5ee2b2c8f 100644 --- a/crates/osmodifier/src/lib.rs +++ b/crates/osmodifier/src/lib.rs @@ -188,7 +188,7 @@ pub fn modify_boot(ctx: &OsModifierContext, config: &BootConfig) -> Result<(), E let corruption_opt = verity .corruption_option .as_ref() - .map(|c| format_corruption_option(c)) + .map(format_corruption_option) .unwrap_or_default(); let new_args = vec![ diff --git a/crates/trident/src/engine/boot/grub.rs b/crates/trident/src/engine/boot/grub.rs index 01fc1be15..b345f5c31 100644 --- a/crates/trident/src/engine/boot/grub.rs +++ b/crates/trident/src/engine/boot/grub.rs @@ -789,7 +789,7 @@ pub(crate) mod functional_test { let _a = setup_mock_grub_configs(&ctx); - let result = update_configs(&ctx, Path::new(ROOT_MOUNT_POINT_PATH)); + let result = update_configs(&ctx); assert_eq!( result.unwrap_err().to_string(), "Failed to get UUID for path '/dev/sdb2', received ''" @@ -850,7 +850,7 @@ pub(crate) mod functional_test { let _a = setup_mock_grub_configs(&ctx); - let result = update_configs(&ctx, Path::new(ROOT_MOUNT_POINT_PATH)); + let result = update_configs(&ctx); assert_eq!(result.unwrap_err().to_string(), "Root device path is none"); } From 0c7e5adf56bf22431e10aa53359a2d992e74ce21 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Mon, 11 May 2026 19:45:28 -0700 Subject: [PATCH 07/20] fix: remove stale osmodifier option and constant from conftest.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/functional_tests/conftest.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/functional_tests/conftest.py b/tests/functional_tests/conftest.py index 8627ef448..400b5ebec 100644 --- a/tests/functional_tests/conftest.py +++ b/tests/functional_tests/conftest.py @@ -33,9 +33,6 @@ FT_BASE_IMAGE = TRIDENT_REPO_DIR_PATH / "artifacts" / "trident-functest.qcow2" -"""Target location of the osmodifier binary in the test host.""" -OS_MODIFIER_BIN_TARGET_PATH = Path("/usr/bin/osmodifier") - def pytest_addoption(parser): """Defines additional command line options for the tests.""" @@ -72,13 +69,6 @@ def pytest_addoption(parser): help="Force upload of tests even if no change was detected.", ) - parser.addoption( - "--osmodifier", - help="Path to the osmodifier binary to copy into the test host.", - default=TRIDENT_REPO_DIR_PATH / "artifacts" / "osmodifier", - type=Path, - ) - def pytest_collect_file(file_path: Path, parent: Collector) -> Optional[Collector]: """Creates a custom collector for ft.json.""" From dff2cc24d2c4fa1b8fecb84d20eac3a63c8433d6 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Tue, 12 May 2026 10:07:41 -0700 Subject: [PATCH 08/20] fix: address deep review findings - atomic writes, security, correctness Fixes based on 9-agent adversarial code review: Security: - Use chpasswd -e via stdin instead of useradd -p to avoid leaking password hashes through /proc/cmdline - Validate startup_command for colons/newlines to prevent /etc/passwd corruption - Implement PasswordType::Locked for existing users (write ! marker) Atomicity: - All /etc/shadow and /etc/passwd edits now use atomic write-temp-rename pattern via tempfile::NamedTempFile::persist() Correctness: - Fix set_password_expiry: add missing found-check (was silent no-op) - Fix add_extra_cmdline: deduplicate by key to be idempotent on re-run - Fix stale comment in modify_os about not touching grub Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/src/default_grub.rs | 12 ++- crates/osmodifier/src/lib.rs | 12 +-- crates/osmodifier/src/users.rs | 131 +++++++++++++++++++++++--- 3 files changed, 132 insertions(+), 23 deletions(-) diff --git a/crates/osmodifier/src/default_grub.rs b/crates/osmodifier/src/default_grub.rs index b73664e10..c1e1788cc 100644 --- a/crates/osmodifier/src/default_grub.rs +++ b/crates/osmodifier/src/default_grub.rs @@ -112,8 +112,8 @@ impl DefaultGrub { Ok(()) } - /// Add extra command line arguments to GRUB_CMDLINE_LINUX without - /// removing any existing ones. + /// Add extra command line arguments to GRUB_CMDLINE_LINUX, skipping + /// any that are already present (idempotent). pub fn add_extra_cmdline(&mut self, extra: &[String]) { let current = self.get_variable("GRUB_CMDLINE_LINUX").unwrap_or_default(); @@ -123,7 +123,13 @@ impl DefaultGrub { current.split_whitespace().map(String::from).collect() }; - args.extend(extra.iter().cloned()); + for item in extra { + let key = item.split('=').next().unwrap_or(item); + // Skip if an arg with the same key already exists + if !args.iter().any(|a| a.split('=').next().unwrap_or(a) == key) { + args.push(item.clone()); + } + } let new_value = args.join(" "); self.set_variable("GRUB_CMDLINE_LINUX", &new_value); diff --git a/crates/osmodifier/src/lib.rs b/crates/osmodifier/src/lib.rs index 5ee2b2c8f..6adb73d58 100644 --- a/crates/osmodifier/src/lib.rs +++ b/crates/osmodifier/src/lib.rs @@ -86,13 +86,8 @@ pub fn modify_os(ctx: &OsModifierContext, config: &OSModifierConfig) -> Result<( modules::configure(ctx, &config.modules).context("Failed to configure kernel modules")?; } - // Kernel command line and SELinux are handled via boot config, not here. - // The Go code uses BootCustomizer for these, which requires detecting the - // bootloader type and working with /etc/default/grub. In trident, these - // are only set through the boot subsystem's modify_boot() path. - // - // However, for UKI images, SELinux mode is set via the config file directly - // (not via kernel cmdline). The osconfig subsystem handles this case by + // For UKI images, SELinux mode is set via the config file directly (not + // via kernel cmdline). The osconfig subsystem handles this case by // including selinux in the OSModifierConfig. if let Some(ref selinux_cfg) = config.selinux { if let Some(ref mode) = selinux_cfg.mode { @@ -102,6 +97,9 @@ pub fn modify_os(ctx: &OsModifierContext, config: &OSModifierConfig) -> Result<( } } + // Extra kernel command line args are appended to /etc/default/grub and + // grub.cfg is regenerated. Note: modify_boot() also writes to + // /etc/default/grub for boot-specific config (overlays, verity, etc.). if let Some(ref kcl) = config.kernel_command_line { if !kcl.extra_command_line.is_empty() { info!("Adding extra kernel command line arguments"); diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs index 6c58b7cd4..76314ef3b 100644 --- a/crates/osmodifier/src/users.rs +++ b/crates/osmodifier/src/users.rs @@ -35,6 +35,11 @@ fn add_or_update_user(ctx: &OsModifierContext, user: &MICUser) -> Result<(), Err None => None, }; + let is_locked = user + .password + .as_ref() + .is_some_and(|p| p.password_type == PasswordType::Locked); + let user_exists = check_user_exists(root, &user.name)?; if user_exists { @@ -57,10 +62,19 @@ fn add_or_update_user(ctx: &OsModifierContext, user: &MICUser) -> Result<(), Err // Update password if provided if let Some(ref hash) = hashed_password { update_user_password(ctx, &user.name, hash)?; + } else if is_locked { + // Lock the account by writing a locked marker to /etc/shadow + lock_user_password(ctx, &user.name)?; } } else { info!("Creating user '{}'", user.name); - create_user(root, user, hashed_password.as_deref())?; + create_user(root, user)?; + + // Set password after creation via chpasswd (avoids leaking hash in + // /proc/cmdline that useradd -p would cause). + if let Some(ref hash) = hashed_password { + set_password_via_chpasswd(root, &user.name, hash)?; + } } // Set password expiry @@ -134,7 +148,7 @@ fn hash_password(plaintext: &str) -> Result { .to_string()) } -fn create_user(root: &str, user: &MICUser, hashed_password: Option<&str>) -> Result<(), Error> { +fn create_user(root: &str, user: &MICUser) -> Result<(), Error> { let mut cmd = if root == "/" { Command::new("useradd") } else { @@ -145,9 +159,8 @@ fn create_user(root: &str, user: &MICUser, hashed_password: Option<&str>) -> Res cmd.arg("-m"); // Create home directory - if let Some(ref hash) = hashed_password { - cmd.arg("-p").arg(hash); - } + // Password is set separately via chpasswd to avoid leaking the hash + // through /proc/cmdline (useradd -p is world-readable). if let Some(uid) = user.uid { cmd.arg("-u").arg(uid.to_string()); @@ -207,8 +220,54 @@ fn update_user_password(ctx: &OsModifierContext, username: &str, hash: &str) -> result.push('\n'); } - fs::write(&shadow_path, &result) - .with_context(|| format!("Failed to write '{}'", shadow_path.display())) + atomic_write_file(&shadow_path, &result) +} + +/// Set password on a newly created user via chpasswd -e (stdin), avoiding +/// leaking the hash through /proc/cmdline. +fn set_password_via_chpasswd(root: &str, username: &str, hash: &str) -> Result<(), Error> { + debug!("Setting password for new user '{username}' via chpasswd"); + let input = format!("{username}:{hash}\n"); + + let mut child = if root == "/" { + Command::new("chpasswd") + .arg("-e") + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + } else { + Command::new("chroot") + .arg(root) + .args(["chpasswd", "-e"]) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + } + .context("Failed to start chpasswd")?; + + if let Some(ref mut stdin) = child.stdin { + stdin + .write_all(input.as_bytes()) + .context("Failed to write to chpasswd stdin")?; + } + + let output = child + .wait_with_output() + .context("Failed to wait for chpasswd")?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("chpasswd failed for '{username}': {stderr}"); + } + Ok(()) +} + +/// Lock a user's password by writing the locked marker '!' into /etc/shadow. +fn lock_user_password(ctx: &OsModifierContext, username: &str) -> Result<(), Error> { + debug!("Locking password for user '{username}'"); + update_user_password(ctx, username, "!") } fn set_password_expiry(ctx: &OsModifierContext, username: &str, days: u64) -> Result<(), Error> { @@ -218,13 +277,16 @@ fn set_password_expiry(ctx: &OsModifierContext, username: &str, days: u64) -> Re let content = fs::read_to_string(&shadow_path) .with_context(|| format!("Failed to read '{}'", shadow_path.display()))?; + let mut found = false; let updated: Vec = content .lines() .map(|line| { let fields: Vec<&str> = line.split(':').collect(); - if fields.len() >= 5 && fields[0] == username { + if fields.len() >= 2 && fields[0] == username { + found = true; let mut new_fields: Vec = fields.iter().map(|f| f.to_string()).collect(); - // Field index 4 is the maximum password age + // Shadow fields: login:password:lastChange:minAge:maxAge:warn:inactive:expire:reserved + // Field index 4 (0-based) is the maximum password age. while new_fields.len() < 5 { new_fields.push(String::new()); } @@ -236,13 +298,16 @@ fn set_password_expiry(ctx: &OsModifierContext, username: &str, days: u64) -> Re }) .collect(); + if !found { + bail!("User '{username}' not found in shadow file for password expiry"); + } + let mut result = updated.join("\n"); if content.ends_with('\n') { result.push('\n'); } - fs::write(&shadow_path, &result) - .with_context(|| format!("Failed to write '{}'", shadow_path.display())) + atomic_write_file(&shadow_path, &result) } fn set_primary_group(root: &str, username: &str, group: &str) -> Result<(), Error> { @@ -371,6 +436,15 @@ fn set_ownership(ctx: &OsModifierContext, username: &str, path: &Path) -> Result fn set_startup_command(ctx: &OsModifierContext, username: &str, cmd: &str) -> Result<(), Error> { debug!("Setting startup command for '{username}' to '{cmd}'"); + + // Validate: colons would corrupt the colon-delimited /etc/passwd format + if cmd.contains(':') { + bail!("Startup command for user '{username}' contains ':' which would corrupt /etc/passwd"); + } + if cmd.contains('\n') { + bail!("Startup command for user '{username}' contains a newline"); + } + let passwd_path = ctx.path("/etc/passwd"); let content = fs::read_to_string(&passwd_path) @@ -401,6 +475,37 @@ fn set_startup_command(ctx: &OsModifierContext, username: &str, cmd: &str) -> Re result.push('\n'); } - fs::write(&passwd_path, &result) - .with_context(|| format!("Failed to write '{}'", passwd_path.display())) + atomic_write_file(&passwd_path, &result) +} + +/// Atomically write a file by writing to a temp file and renaming. +/// This prevents corruption from crashes mid-write. +fn atomic_write_file(path: &std::path::Path, content: &str) -> Result<(), Error> { + use std::io::Write as IoWrite; + + let parent = path.parent().context("Cannot determine parent directory")?; + + let mut tmp = tempfile::NamedTempFile::new_in(parent) + .with_context(|| format!("Failed to create temp file in '{}'", parent.display()))?; + + tmp.write_all(content.as_bytes()) + .with_context(|| format!("Failed to write temp file for '{}'", path.display()))?; + + tmp.flush() + .with_context(|| format!("Failed to flush temp file for '{}'", path.display()))?; + + // Preserve permissions from the original file if it exists + if let Ok(metadata) = fs::metadata(path) { + fs::set_permissions(tmp.path(), metadata.permissions()).with_context(|| { + format!( + "Failed to set permissions on temp file for '{}'", + path.display() + ) + })?; + } + + tmp.persist(path) + .with_context(|| format!("Failed to atomically replace '{}'", path.display()))?; + + Ok(()) } From 7a25b620c5c233de0c3626d90d63425f837b410c Mon Sep 17 00:00:00 2001 From: bfjelds Date: Tue, 12 May 2026 10:24:29 -0700 Subject: [PATCH 09/20] fix: remove trailing newline from hostname write to match Go behavior The Go osmodifier wrote hostname without trailing newline. The Rust port added one, causing the functional test assertion to fail: left: 'carry-over-hostname\n' right: 'carry-over-hostname' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/src/hostname.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/osmodifier/src/hostname.rs b/crates/osmodifier/src/hostname.rs index 89e2640e8..ace7b8bb5 100644 --- a/crates/osmodifier/src/hostname.rs +++ b/crates/osmodifier/src/hostname.rs @@ -16,6 +16,6 @@ const HOSTNAME_PATH: &str = "/etc/hostname"; pub fn update(ctx: &OsModifierContext, hostname: &str) -> Result<(), Error> { let path = ctx.path(HOSTNAME_PATH); debug!("Writing hostname '{}' to '{}'", hostname, path.display()); - fs::write(&path, format!("{hostname}\n")) + fs::write(&path, hostname) .with_context(|| format!("Failed to write hostname to '{}'", path.display())) } From 23ca4c82d12afe0741f8cc14cf396fae087530ee Mon Sep 17 00:00:00 2001 From: bfjelds Date: Thu, 14 May 2026 09:19:28 -0700 Subject: [PATCH 10/20] Add functional tests for osmodifier crate Add Rust functional tests for the osmodifier modules that run inside the test VM. Tests cover: - hostname: write and overwrite /etc/hostname - modules: always-load, disable, options, idempotency, disable-removes-load - selinux: config file update (enforcing/disabled), missing file error, GRUB cmdline SELinux args - services: enable/disable with synthetic systemd units, already-disabled - lib.rs integration: modify_os with hostname+modules, empty config no-op, hostname+services combined All tests use tempdir-rooted OsModifierContext to avoid modifying the real system. Service tests use synthetic unit files rather than depending on installed services. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/Cargo.toml | 6 ++ crates/osmodifier/src/hostname.rs | 39 +++++++ crates/osmodifier/src/lib.rs | 104 ++++++++++++++++++ crates/osmodifier/src/modules.rs | 170 ++++++++++++++++++++++++++++++ crates/osmodifier/src/selinux.rs | 101 ++++++++++++++++++ crates/osmodifier/src/services.rs | 108 +++++++++++++++++++ 6 files changed, 528 insertions(+) diff --git a/crates/osmodifier/Cargo.toml b/crates/osmodifier/Cargo.toml index 8f565e224..14729ddcc 100644 --- a/crates/osmodifier/Cargo.toml +++ b/crates/osmodifier/Cargo.toml @@ -8,13 +8,19 @@ description = "OS modifier library - applies OS configuration changes (users, ho [dependencies] anyhow = { workspace = true } +inventory = { workspace = true } log = { workspace = true } regex = { workspace = true } serde = { workspace = true } serde_yaml = { workspace = true } tempfile = { workspace = true } +pytest = { path = "../pytest" } +pytest_gen = { path = "../pytest_gen" } trident_api = { path = "../trident_api" } [dev-dependencies] indoc = { workspace = true } + +[features] +functional-test = [] diff --git a/crates/osmodifier/src/hostname.rs b/crates/osmodifier/src/hostname.rs index ace7b8bb5..4a1d3b2d2 100644 --- a/crates/osmodifier/src/hostname.rs +++ b/crates/osmodifier/src/hostname.rs @@ -19,3 +19,42 @@ pub fn update(ctx: &OsModifierContext, hostname: &str) -> Result<(), Error> { fs::write(&path, hostname) .with_context(|| format!("Failed to write hostname to '{}'", path.display())) } + +#[cfg_attr(not(test), allow(unused_imports, dead_code))] +mod functional_test { + use super::*; + use tempfile::tempdir; + + use pytest_gen::functional_test; + + use crate::OsModifierContext; + + #[functional_test(feature = "core")] + fn test_update_hostname() { + let tmp = tempdir().unwrap(); + fs::create_dir_all(tmp.path().join("etc")).unwrap(); + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + update(&ctx, "my-test-host").unwrap(); + + let content = fs::read_to_string(tmp.path().join("etc/hostname")).unwrap(); + assert_eq!(content.trim(), "my-test-host"); + } + + #[functional_test(feature = "core")] + fn test_update_hostname_overwrites() { + let tmp = tempdir().unwrap(); + fs::create_dir_all(tmp.path().join("etc")).unwrap(); + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + update(&ctx, "first-host").unwrap(); + update(&ctx, "second-host").unwrap(); + + let content = fs::read_to_string(tmp.path().join("etc/hostname")).unwrap(); + assert_eq!(content.trim(), "second-host"); + } +} diff --git a/crates/osmodifier/src/lib.rs b/crates/osmodifier/src/lib.rs index 6adb73d58..59ccad56d 100644 --- a/crates/osmodifier/src/lib.rs +++ b/crates/osmodifier/src/lib.rs @@ -230,3 +230,107 @@ fn format_corruption_option(opt: &CorruptionOption) -> String { CorruptionOption::Restart => "restart-on-corruption".to_string(), } } + +#[cfg_attr(not(test), allow(unused_imports, dead_code))] +mod functional_test { + use super::*; + use std::collections::HashMap; + use std::fs; + use tempfile::tempdir; + + use pytest_gen::functional_test; + use trident_api::config::{LoadMode, Module, Services}; + + #[functional_test(feature = "core")] + fn test_modify_os_hostname_and_modules() { + let tmp = tempdir().unwrap(); + fs::create_dir_all(tmp.path().join("etc")).unwrap(); + + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + let config = OSModifierConfig { + hostname: Some("integration-test-host".to_string()), + modules: vec![Module { + name: "vfio_pci".to_string(), + load_mode: LoadMode::Always, + options: HashMap::new(), + }], + ..Default::default() + }; + + modify_os(&ctx, &config).unwrap(); + + // Verify hostname + let hostname = fs::read_to_string(tmp.path().join("etc/hostname")).unwrap(); + assert_eq!(hostname.trim(), "integration-test-host"); + + // Verify module loaded + let load_conf = + fs::read_to_string(tmp.path().join("etc/modules-load.d/modules-load.conf")).unwrap(); + assert!( + load_conf.contains("vfio_pci"), + "Expected vfio_pci in modules-load.conf" + ); + } + + #[functional_test(feature = "core")] + fn test_modify_os_empty_config() { + let tmp = tempdir().unwrap(); + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + let config = OSModifierConfig::default(); + + // Empty config should be a no-op + modify_os(&ctx, &config).unwrap(); + } + + #[functional_test(feature = "core")] + fn test_modify_os_with_services() { + let tmp = tempdir().unwrap(); + fs::create_dir_all(tmp.path().join("etc")).unwrap(); + + // Create a minimal systemd tree with a synthetic service + let unit_dir = tmp.path().join("usr/lib/systemd/system"); + fs::create_dir_all(&unit_dir).unwrap(); + fs::create_dir_all(tmp.path().join("etc/systemd/system/multi-user.target.wants")).unwrap(); + fs::write( + unit_dir.join("test-integration.service"), + "[Unit]\nDescription=Test\n\n[Service]\nType=oneshot\nExecStart=/bin/true\n\n[Install]\nWantedBy=multi-user.target\n", + ) + .unwrap(); + + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + let config = OSModifierConfig { + hostname: Some("svc-test-host".to_string()), + services: Some(Services { + enable: vec!["test-integration.service".to_string()], + disable: vec![], + }), + ..Default::default() + }; + + modify_os(&ctx, &config).unwrap(); + + // Verify hostname + let hostname = fs::read_to_string(tmp.path().join("etc/hostname")).unwrap(); + assert_eq!(hostname.trim(), "svc-test-host"); + + // Verify service enabled — symlink may be dangling (target is absolute /usr/... + // but only exists under the temp root), so check is_symlink() not exists() + let symlink = tmp + .path() + .join("etc/systemd/system/multi-user.target.wants/test-integration.service"); + assert!( + symlink.is_symlink(), + "Service should be enabled (symlink at {})", + symlink.display() + ); + } +} diff --git a/crates/osmodifier/src/modules.rs b/crates/osmodifier/src/modules.rs index 40c1897c6..a4abd7ab9 100644 --- a/crates/osmodifier/src/modules.rs +++ b/crates/osmodifier/src/modules.rs @@ -138,3 +138,173 @@ fn write_config(path: &std::path::Path, lines: &[String]) -> Result<(), Error> { fs::write(path, &content) .with_context(|| format!("Failed to write config to '{}'", path.display())) } + +#[cfg_attr(not(test), allow(unused_imports, dead_code))] +mod functional_test { + use super::*; + use std::collections::HashMap; + use tempfile::tempdir; + + use pytest_gen::functional_test; + use trident_api::config::LoadMode; + + use crate::OsModifierContext; + + fn make_ctx(tmp: &tempfile::TempDir) -> OsModifierContext { + OsModifierContext { + root: tmp.path().to_path_buf(), + } + } + + #[functional_test(feature = "core")] + fn test_configure_modules_always_load() { + let tmp = tempdir().unwrap(); + let ctx = make_ctx(&tmp); + + let modules = vec![Module { + name: "br_netfilter".to_string(), + load_mode: LoadMode::Always, + options: HashMap::new(), + }]; + + configure(&ctx, &modules).unwrap(); + + let load_conf = + fs::read_to_string(tmp.path().join("etc/modules-load.d/modules-load.conf")).unwrap(); + assert!( + load_conf.contains("br_netfilter"), + "Expected br_netfilter in modules-load.conf, got: {load_conf}" + ); + } + + #[functional_test(feature = "core")] + fn test_configure_modules_disable() { + let tmp = tempdir().unwrap(); + let ctx = make_ctx(&tmp); + + let modules = vec![Module { + name: "floppy".to_string(), + load_mode: LoadMode::Disable, + options: HashMap::new(), + }]; + + configure(&ctx, &modules).unwrap(); + + let disabled_conf = + fs::read_to_string(tmp.path().join("etc/modprobe.d/modules-disabled.conf")).unwrap(); + assert!( + disabled_conf.contains("blacklist floppy"), + "Expected 'blacklist floppy' in modules-disabled.conf, got: {disabled_conf}" + ); + + // Should NOT appear in modules-load.conf + let load_conf = + fs::read_to_string(tmp.path().join("etc/modules-load.d/modules-load.conf")).unwrap(); + assert!( + !load_conf.contains("floppy"), + "floppy should not be in modules-load.conf" + ); + } + + #[functional_test(feature = "core")] + fn test_configure_modules_with_options() { + let tmp = tempdir().unwrap(); + let ctx = make_ctx(&tmp); + + let mut opts = HashMap::new(); + opts.insert("num_vfs".to_string(), "4".to_string()); + + let modules = vec![Module { + name: "ixgbevf".to_string(), + load_mode: LoadMode::Always, + options: opts, + }]; + + configure(&ctx, &modules).unwrap(); + + let options_conf = + fs::read_to_string(tmp.path().join("etc/modprobe.d/module-options.conf")).unwrap(); + assert!( + options_conf.contains("options ixgbevf num_vfs=4"), + "Expected module options line, got: {options_conf}" + ); + } + + #[functional_test(feature = "core", negative = true)] + fn test_configure_modules_disable_with_options_fails() { + let tmp = tempdir().unwrap(); + let ctx = make_ctx(&tmp); + + let mut opts = HashMap::new(); + opts.insert("bad".to_string(), "option".to_string()); + + let modules = vec![Module { + name: "floppy".to_string(), + load_mode: LoadMode::Disable, + options: opts, + }]; + + let result = configure(&ctx, &modules); + assert!( + result.is_err(), + "Disabling a module with options should fail" + ); + } + + #[functional_test(feature = "core")] + fn test_configure_modules_idempotent() { + let tmp = tempdir().unwrap(); + let ctx = make_ctx(&tmp); + + let modules = vec![Module { + name: "br_netfilter".to_string(), + load_mode: LoadMode::Always, + options: HashMap::new(), + }]; + + // Apply twice + configure(&ctx, &modules).unwrap(); + configure(&ctx, &modules).unwrap(); + + let load_conf = + fs::read_to_string(tmp.path().join("etc/modules-load.d/modules-load.conf")).unwrap(); + let count = load_conf.matches("br_netfilter").count(); + assert_eq!(count, 1, "Module should appear exactly once, got {count}"); + } + + #[functional_test(feature = "core")] + fn test_configure_modules_disable_removes_from_load() { + let tmp = tempdir().unwrap(); + let ctx = make_ctx(&tmp); + + // First enable + let enable = vec![Module { + name: "br_netfilter".to_string(), + load_mode: LoadMode::Always, + options: HashMap::new(), + }]; + configure(&ctx, &enable).unwrap(); + + // Then disable + let disable = vec![Module { + name: "br_netfilter".to_string(), + load_mode: LoadMode::Disable, + options: HashMap::new(), + }]; + configure(&ctx, &disable).unwrap(); + + let load_conf = + fs::read_to_string(tmp.path().join("etc/modules-load.d/modules-load.conf")).unwrap(); + assert!( + !load_conf.contains("br_netfilter"), + "Disabled module should be removed from modules-load.conf" + ); + + let disabled_conf = + fs::read_to_string(tmp.path().join("etc/modprobe.d/modules-disabled.conf")).unwrap(); + assert!( + disabled_conf.contains("blacklist br_netfilter"), + "Disabled module should appear in blacklist" + ); + } +} diff --git a/crates/osmodifier/src/selinux.rs b/crates/osmodifier/src/selinux.rs index 7916d93b3..7d3b116a1 100644 --- a/crates/osmodifier/src/selinux.rs +++ b/crates/osmodifier/src/selinux.rs @@ -69,3 +69,104 @@ pub fn update_grub_cmdline( default_grub.update_cmdline_args(&["selinux", "enforcing"], &new_args) } + +#[cfg_attr(not(test), allow(unused_imports, dead_code))] +mod functional_test { + use super::*; + use std::fs; + use tempfile::tempdir; + + use pytest_gen::functional_test; + + use crate::OsModifierContext; + + #[functional_test(feature = "core")] + fn test_update_selinux_config_enforcing() { + let tmp = tempdir().unwrap(); + let etc = tmp.path().join("etc/selinux"); + fs::create_dir_all(&etc).unwrap(); + fs::write(etc.join("config"), "SELINUX=permissive\nSELINUXTYPE=targeted\n").unwrap(); + + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + update_config_file(&ctx, &SelinuxMode::Enforcing).unwrap(); + + let content = fs::read_to_string(etc.join("config")).unwrap(); + assert!( + content.contains("SELINUX=enforcing"), + "Expected SELINUX=enforcing, got: {content}" + ); + // Original SELINUXTYPE should be preserved + assert!( + content.contains("SELINUXTYPE=targeted"), + "SELINUXTYPE should be preserved" + ); + } + + #[functional_test(feature = "core")] + fn test_update_selinux_config_disabled() { + let tmp = tempdir().unwrap(); + let etc = tmp.path().join("etc/selinux"); + fs::create_dir_all(&etc).unwrap(); + fs::write(etc.join("config"), "SELINUX=enforcing\n").unwrap(); + + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + update_config_file(&ctx, &SelinuxMode::Disabled).unwrap(); + + let content = fs::read_to_string(etc.join("config")).unwrap(); + assert!( + content.contains("SELINUX=disabled"), + "Expected SELINUX=disabled, got: {content}" + ); + } + + #[functional_test(feature = "core", negative = true)] + fn test_update_selinux_config_missing_file() { + let tmp = tempdir().unwrap(); + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + let result = update_config_file(&ctx, &SelinuxMode::Enforcing); + assert!(result.is_err(), "Should fail when SELinux config is missing"); + } + + #[functional_test(feature = "core")] + fn test_update_selinux_grub_cmdline_enforcing() { + let tmp = tempdir().unwrap(); + let etc = tmp.path().join("etc/default"); + fs::create_dir_all(&etc).unwrap(); + fs::write( + etc.join("grub"), + "GRUB_CMDLINE_LINUX=\"quiet selinux=0\"\n", + ) + .unwrap(); + + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + let mut grub = DefaultGrub::read(&ctx).unwrap(); + update_grub_cmdline(&ctx, &mut grub, &SelinuxMode::Enforcing).unwrap(); + grub.write().unwrap(); + + let content = fs::read_to_string(etc.join("grub")).unwrap(); + assert!( + content.contains("selinux=1"), + "Expected selinux=1 in grub, got: {content}" + ); + assert!( + content.contains("enforcing=1"), + "Expected enforcing=1 in grub, got: {content}" + ); + assert!( + !content.contains("selinux=0"), + "Old selinux=0 should be removed" + ); + } +} diff --git a/crates/osmodifier/src/services.rs b/crates/osmodifier/src/services.rs index 134af873d..cd09d119a 100644 --- a/crates/osmodifier/src/services.rs +++ b/crates/osmodifier/src/services.rs @@ -69,3 +69,111 @@ fn disable_service(ctx: &OsModifierContext, service: &str) -> Result<(), Error> Ok(()) } + +#[cfg_attr(not(test), allow(unused_imports, dead_code))] +mod functional_test { + use super::*; + use std::fs; + use tempfile::tempdir; + + use pytest_gen::functional_test; + use trident_api::config::Services; + + use crate::OsModifierContext; + + /// Create a minimal systemd tree with a synthetic service unit. + fn setup_systemd_root(tmp: &std::path::Path) { + let unit_dir = tmp.join("usr/lib/systemd/system"); + fs::create_dir_all(&unit_dir).unwrap(); + + // systemctl --root needs these directories + fs::create_dir_all(tmp.join("etc/systemd/system/multi-user.target.wants")).unwrap(); + + fs::write( + unit_dir.join("test-osmodifier.service"), + "[Unit]\nDescription=Test Service\n\n[Service]\nType=oneshot\nExecStart=/bin/true\n\n[Install]\nWantedBy=multi-user.target\n", + ) + .unwrap(); + } + + #[functional_test(feature = "core")] + fn test_enable_service() { + let tmp = tempdir().unwrap(); + setup_systemd_root(tmp.path()); + + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + let services = Services { + enable: vec!["test-osmodifier.service".to_string()], + disable: vec![], + }; + + configure(&ctx, &services).unwrap(); + + // Verify the symlink was created — may be dangling since target is absolute + let wants_dir = tmp.path().join("etc/systemd/system/multi-user.target.wants"); + let service_link = wants_dir.join("test-osmodifier.service"); + assert!( + service_link.is_symlink(), + "Expected service symlink at {}", + service_link.display(), + ); + } + + #[functional_test(feature = "core")] + fn test_disable_service() { + let tmp = tempdir().unwrap(); + setup_systemd_root(tmp.path()); + + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + // Enable first + let enable = Services { + enable: vec!["test-osmodifier.service".to_string()], + disable: vec![], + }; + configure(&ctx, &enable).unwrap(); + + // Then disable + let disable = Services { + enable: vec![], + disable: vec!["test-osmodifier.service".to_string()], + }; + configure(&ctx, &disable).unwrap(); + + // Verify the symlink was removed + let symlink_path = tmp + .path() + .join("etc/systemd/system/multi-user.target.wants/test-osmodifier.service"); + assert!( + !symlink_path.is_symlink(), + "Symlink should be removed after disable" + ); + } + + #[functional_test(feature = "core")] + fn test_disable_already_disabled_service() { + let tmp = tempdir().unwrap(); + setup_systemd_root(tmp.path()); + + let ctx = OsModifierContext { + root: tmp.path().to_path_buf(), + }; + + // Disable without enabling first — should succeed (warn and skip) + let services = Services { + enable: vec![], + disable: vec!["test-osmodifier.service".to_string()], + }; + + let result = configure(&ctx, &services); + assert!( + result.is_ok(), + "Disabling an already-disabled service should succeed" + ); + } +} From ee3bb10a189cf276822865743c28e84f8696cb0e Mon Sep 17 00:00:00 2001 From: bfjelds Date: Thu, 14 May 2026 11:37:19 -0700 Subject: [PATCH 11/20] fix: apply cargo fmt formatting corrections Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/src/lib.rs | 6 +++++- crates/osmodifier/src/selinux.rs | 17 ++++++++++------- crates/osmodifier/src/services.rs | 4 +++- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/crates/osmodifier/src/lib.rs b/crates/osmodifier/src/lib.rs index 59ccad56d..624bfe5e1 100644 --- a/crates/osmodifier/src/lib.rs +++ b/crates/osmodifier/src/lib.rs @@ -296,7 +296,11 @@ mod functional_test { // Create a minimal systemd tree with a synthetic service let unit_dir = tmp.path().join("usr/lib/systemd/system"); fs::create_dir_all(&unit_dir).unwrap(); - fs::create_dir_all(tmp.path().join("etc/systemd/system/multi-user.target.wants")).unwrap(); + fs::create_dir_all( + tmp.path() + .join("etc/systemd/system/multi-user.target.wants"), + ) + .unwrap(); fs::write( unit_dir.join("test-integration.service"), "[Unit]\nDescription=Test\n\n[Service]\nType=oneshot\nExecStart=/bin/true\n\n[Install]\nWantedBy=multi-user.target\n", diff --git a/crates/osmodifier/src/selinux.rs b/crates/osmodifier/src/selinux.rs index 7d3b116a1..7f0b2ebb2 100644 --- a/crates/osmodifier/src/selinux.rs +++ b/crates/osmodifier/src/selinux.rs @@ -85,7 +85,11 @@ mod functional_test { let tmp = tempdir().unwrap(); let etc = tmp.path().join("etc/selinux"); fs::create_dir_all(&etc).unwrap(); - fs::write(etc.join("config"), "SELINUX=permissive\nSELINUXTYPE=targeted\n").unwrap(); + fs::write( + etc.join("config"), + "SELINUX=permissive\nSELINUXTYPE=targeted\n", + ) + .unwrap(); let ctx = OsModifierContext { root: tmp.path().to_path_buf(), @@ -133,7 +137,10 @@ mod functional_test { }; let result = update_config_file(&ctx, &SelinuxMode::Enforcing); - assert!(result.is_err(), "Should fail when SELinux config is missing"); + assert!( + result.is_err(), + "Should fail when SELinux config is missing" + ); } #[functional_test(feature = "core")] @@ -141,11 +148,7 @@ mod functional_test { let tmp = tempdir().unwrap(); let etc = tmp.path().join("etc/default"); fs::create_dir_all(&etc).unwrap(); - fs::write( - etc.join("grub"), - "GRUB_CMDLINE_LINUX=\"quiet selinux=0\"\n", - ) - .unwrap(); + fs::write(etc.join("grub"), "GRUB_CMDLINE_LINUX=\"quiet selinux=0\"\n").unwrap(); let ctx = OsModifierContext { root: tmp.path().to_path_buf(), diff --git a/crates/osmodifier/src/services.rs b/crates/osmodifier/src/services.rs index cd09d119a..e94055e4a 100644 --- a/crates/osmodifier/src/services.rs +++ b/crates/osmodifier/src/services.rs @@ -113,7 +113,9 @@ mod functional_test { configure(&ctx, &services).unwrap(); // Verify the symlink was created — may be dangling since target is absolute - let wants_dir = tmp.path().join("etc/systemd/system/multi-user.target.wants"); + let wants_dir = tmp + .path() + .join("etc/systemd/system/multi-user.target.wants"); let service_link = wants_dir.join("test-osmodifier.service"); assert!( service_link.is_symlink(), From 9399a1e61b82f70c204648e85f57e97b957cf23d Mon Sep 17 00:00:00 2001 From: bfjelds Date: Thu, 14 May 2026 12:02:00 -0700 Subject: [PATCH 12/20] fix: update Cargo.lock with osmodifier test dependencies Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 937896728..69ca6c2e3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1868,7 +1868,10 @@ version = "0.1.0" dependencies = [ "anyhow", "indoc", + "inventory", "log", + "pytest", + "pytest_gen", "regex", "serde", "serde_yaml", From d7e563b16774c9817bdb476ad6c7b30fdce9c61b Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Sat, 16 May 2026 08:23:45 -0700 Subject: [PATCH 13/20] docs: add README mapping Rust port to Go source files Document the origin of each Rust source file in the osmodifier crate, including the Go source file it was ported from, the commit hash, and date. Include key differences between the Rust and Go implementations with reasoning for each divergence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/README.md | 129 ++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 crates/osmodifier/README.md diff --git a/crates/osmodifier/README.md b/crates/osmodifier/README.md new file mode 100644 index 000000000..a007e31d7 --- /dev/null +++ b/crates/osmodifier/README.md @@ -0,0 +1,129 @@ +# osmodifier + +Native Rust port of the OS modifier functionality from +[azure-linux-image-tools](https://github.com/microsoft/azure-linux-image-tools). + +Trident calls osmodifier functions directly as a library crate instead of +serializing config to YAML, writing a temp file, and exec'ing the Go binary. + +## Port Origin + +The initial port was made on **2026-05-11** (commit `ba55580`) from the +azure-linux-image-tools repository. The Go code spans three packages under +`toolkit/tools/`: + +| Go package | Purpose | +|------------|---------| +| `osmodifier/` | CLI entry point | +| `osmodifierapi/` | Configuration types and validation | +| `pkg/osmodifierlib/` | Core modification logic | +| `pkg/imagecustomizerlib/` | Shared helpers (users, hostname, services, modules) | + +## File Mapping + +Each Rust source file and the Go file(s) it was ported from: + +| Rust file | Go source(s) | Go commit | Date | +|-----------|--------------|-----------|------| +| `lib.rs` | `pkg/osmodifierlib/osmodifier.go`, `pkg/osmodifierlib/modifierutils.go` | `f4de1a0` | 2026-03-17 | +| `config.rs` | `osmodifierapi/os.go`, `osmodifierapi/overlay.go`, `osmodifierapi/verity.go`, `osmodifierapi/identifiedpartition.go` | `8bd4ef3` | 2025-09-02 | +| `users.rs` | `pkg/imagecustomizerlib/customizeusers.go` | `8bd4ef3` | 2025-09-02 | +| `hostname.rs` | `pkg/imagecustomizerlib/customizehostname.go` | `8bd4ef3` | 2025-09-02 | +| `modules.rs` | `pkg/imagecustomizerlib/kernelmoduleutils.go` | `8bd4ef3` | 2025-09-02 | +| `services.rs` | `pkg/imagecustomizerlib/customizeservices.go` | `dc90945` | 2026-03-31 | +| `selinux.rs` | `pkg/osmodifierlib/modifierutils.go` (SELinux functions) | `f4de1a0` | 2026-03-17 | +| `default_grub.rs` | `pkg/osmodifierlib/modifydefaultgrub.go` | `f4de1a0` | 2026-03-17 | +| `grub_cfg.rs` | `pkg/osmodifierlib/modifydefaultgrub.go`, `pkg/osmodifierlib/modifierutils.go` | `f4de1a0` | 2026-03-17 | + +All Go paths are relative to `toolkit/tools/` in the azure-linux-image-tools +repository. The Go commit column is the latest commit touching that file at the +time of the port. + +## Key Differences from the Go Implementation + +### Library instead of binary + +The Go osmodifier is a standalone CLI binary invoked via `exec`. The Rust +version is a library crate exposing three public functions: + +```rust +osmodifier::modify_os(&ctx, &config)?; // replaces: osmodifier --config-file +osmodifier::modify_boot(&ctx, &boot_config)?; // replaces: osmodifier --config-file (boot subset) +osmodifier::update_default_grub(&ctx)?; // replaces: osmodifier --update-grub +``` + +**Reasoning:** Eliminates YAML serialization round-trips, temp file I/O, and +process spawning overhead. Errors propagate as native Rust `Result` types +instead of being parsed from stderr. + +### No chroot / safechroot + +The Go code uses `safechroot` to enter a chroot environment before making +modifications. The Rust version operates on a mounted root directory via +`OsModifierContext`, prefixing all paths with the root directory. + +**Reasoning:** Trident already manages the chroot lifecycle at a higher level. +Duplicating chroot enter/exit in osmodifier would conflict with the outer +chroot management. Path-prefixing achieves the same isolation without the +complexity. + +### Inlined imagecustomizerlib logic + +The Go osmodifier delegates user, hostname, service, and module management to +`imagecustomizerlib`, a shared library also used by the image customizer tool. +The Rust port inlines this logic into dedicated modules (`users.rs`, +`hostname.rs`, `services.rs`, `modules.rs`). + +**Reasoning:** Trident only needs the osmodifier subset of imagecustomizerlib. +Porting the full shared library would pull in unnecessary dependencies. Inlining +keeps the crate self-contained and avoids coupling to Go-side refactors in the +shared library. + +### Secure password handling + +The Go code sets passwords via `useradd -p `, which exposes the password +hash in `/proc//cmdline`. The Rust version uses `chpasswd -e` with the +hash passed via stdin. + +**Reasoning:** Defense in depth. Any process on the system can read +`/proc/cmdline`, making the hash visible during user creation. Passing it via +stdin keeps the hash out of the process argument list. + +### Atomic file writes + +The Rust code uses `tempfile::NamedTempFile::persist()` for all writes to +sensitive files (`/etc/shadow`, `/etc/passwd`). The Go code writes directly. + +**Reasoning:** Atomic rename prevents partial writes from corrupting critical +auth files if the process is interrupted mid-write. + +### Startup command validation + +The Rust code validates that startup commands do not contain colons or newlines +before writing to `/etc/passwd`. The Go code does not perform this validation. + +**Reasoning:** `/etc/passwd` is colon-delimited and newline-separated. A +malicious or malformed startup command containing these characters could corrupt +the passwd file or inject additional entries. + +### Split boot configuration API + +The Go binary handles OS and boot modifications in a single `--config-file` +invocation. The Rust version splits this into `modify_os()` and `modify_boot()` +with separate config types (`OSModifierConfig` and `BootConfig`). + +**Reasoning:** OS modifications (users, hostname, services) and boot +modifications (SELinux, overlays, verity) happen at different stages of the +Trident image build pipeline. Separating them avoids passing unused +configuration and makes the call sites clearer. + +## Keeping the Port in Sync + +When the Go osmodifier code changes upstream, compare the diff against the +corresponding Rust module using the file mapping table above. Pay special +attention to: + +- New fields added to config structs in `osmodifierapi/` +- New modification steps in `modifierutils.go` +- Changes to GRUB parsing logic in `modifydefaultgrub.go` +- Changes to user/service/module handling in `imagecustomizerlib/` From afa8641a4037aa78ec5f8d72854f6917fdbe6c99 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Sat, 16 May 2026 08:39:51 -0700 Subject: [PATCH 14/20] refactor: use Dependency enum for system tool invocations Replace direct std::process::Command calls with osutils::Dependency variants for systemctl, grub2-mkconfig, id, useradd, usermod, chown, and chroot. This aligns osmodifier with trident's pattern for tracking and resolving runtime binary dependencies. Two tools (openssl, chpasswd) remain as direct Command calls because they require stdin piping, which the Dependency Command wrapper does not yet support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/Cargo.toml | 1 + crates/osmodifier/README.md | 23 +++++++++ crates/osmodifier/src/grub_cfg.rs | 14 ++---- crates/osmodifier/src/services.rs | 32 +++++------- crates/osmodifier/src/users.rs | 80 ++++++++++++++---------------- crates/osutils/src/dependencies.rs | 7 +++ 6 files changed, 85 insertions(+), 72 deletions(-) diff --git a/crates/osmodifier/Cargo.toml b/crates/osmodifier/Cargo.toml index 14729ddcc..c83502f8a 100644 --- a/crates/osmodifier/Cargo.toml +++ b/crates/osmodifier/Cargo.toml @@ -18,6 +18,7 @@ tempfile = { workspace = true } pytest = { path = "../pytest" } pytest_gen = { path = "../pytest_gen" } trident_api = { path = "../trident_api" } +osutils = { path = "../osutils" } [dev-dependencies] indoc = { workspace = true } diff --git a/crates/osmodifier/README.md b/crates/osmodifier/README.md index a007e31d7..3f3d45261 100644 --- a/crates/osmodifier/README.md +++ b/crates/osmodifier/README.md @@ -117,6 +117,29 @@ modifications (SELinux, overlays, verity) happen at different stages of the Trident image build pipeline. Separating them avoids passing unused configuration and makes the call sites clearer. +### System tool access via Dependency enum + +External tool invocations use the trident `osutils::Dependency` enum instead +of calling `std::process::Command` directly. This provides consistent binary +resolution (via `which`), structured error reporting, and a centralized +inventory of runtime dependencies. + +| Dependency variant | Used in | +|--------------------|---------| +| `Systemctl` | `services.rs` — enable/disable services | +| `Grub2Mkconfig` | `grub_cfg.rs` — regenerate GRUB config | +| `Chroot` | `users.rs` — run tools inside a mounted root | +| `Id` | `users.rs` — check if a user exists | +| `Useradd` | `users.rs` — create new users | +| `Usermod` | `users.rs` — modify groups | +| `Chown` | `users.rs` — set file ownership | + +Two tools still use `std::process::Command` directly because the Dependency +`Command` wrapper does not yet support stdin piping: + +- **`openssl passwd`** (`hash_password`) — reads plaintext from stdin +- **`chpasswd -e`** (`set_password_via_chpasswd`) — reads `user:hash` from stdin + ## Keeping the Port in Sync When the Go osmodifier code changes upstream, compare the diff against the diff --git a/crates/osmodifier/src/grub_cfg.rs b/crates/osmodifier/src/grub_cfg.rs index 2044f8b43..80cd641f5 100644 --- a/crates/osmodifier/src/grub_cfg.rs +++ b/crates/osmodifier/src/grub_cfg.rs @@ -6,10 +6,11 @@ //! Used by the `update_default_grub` flow to extract boot args from the //! generated grub.cfg and sync them back to /etc/default/grub. -use std::{fs, process::Command}; +use std::fs; use anyhow::{bail, Context, Error}; use log::{debug, info, trace}; +use osutils::Dependency; use regex::Regex; use crate::OsModifierContext; @@ -150,18 +151,13 @@ pub fn run_grub_mkconfig(ctx: &OsModifierContext) -> Result<(), Error> { info!("Running grub2-mkconfig -o '{}'", grub_cfg_path.display()); - let output = Command::new("grub2-mkconfig") + Dependency::Grub2Mkconfig + .cmd() .arg("-o") .arg(&grub_cfg_path) - .output() + .run_and_check() .context("Failed to execute grub2-mkconfig")?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - let stdout = String::from_utf8_lossy(&output.stdout); - bail!("grub2-mkconfig failed:\nstdout: {stdout}\nstderr: {stderr}"); - } - debug!("grub2-mkconfig completed successfully"); Ok(()) } diff --git a/crates/osmodifier/src/services.rs b/crates/osmodifier/src/services.rs index e94055e4a..ebe774f34 100644 --- a/crates/osmodifier/src/services.rs +++ b/crates/osmodifier/src/services.rs @@ -3,10 +3,9 @@ //! Service management — enable and disable systemd services. -use std::process::Command; - use anyhow::{Context, Error}; use log::{debug, warn}; +use osutils::Dependency; use trident_api::config::Services; @@ -29,15 +28,11 @@ fn enable_service(ctx: &OsModifierContext, service: &str) -> Result<(), Error> { debug!("Enabling service '{service}'"); let root = ctx.root.to_str().unwrap_or("/"); - let output = Command::new("systemctl") + Dependency::Systemctl + .cmd() .args(["--root", root, "enable", service]) - .output() - .with_context(|| format!("Failed to execute systemctl enable {service}"))?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - anyhow::bail!("Failed to enable service '{service}': {stderr}"); - } + .run_and_check() + .with_context(|| format!("Failed to enable service '{service}'"))?; Ok(()) } @@ -46,26 +41,23 @@ fn disable_service(ctx: &OsModifierContext, service: &str) -> Result<(), Error> // Check if the service is enabled first let root = ctx.root.to_str().unwrap_or("/"); - let check = Command::new("systemctl") + let check = Dependency::Systemctl + .cmd() .args(["--root", root, "is-enabled", service]) .output() .with_context(|| format!("Failed to check if service '{service}' is enabled"))?; - if !check.status.success() { + if !check.success() { warn!("Service '{service}' is not enabled, skipping disable"); return Ok(()); } debug!("Disabling service '{service}'"); - let output = Command::new("systemctl") + Dependency::Systemctl + .cmd() .args(["--root", root, "disable", service]) - .output() - .with_context(|| format!("Failed to execute systemctl disable {service}"))?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - anyhow::bail!("Failed to disable service '{service}': {stderr}"); - } + .run_and_check() + .with_context(|| format!("Failed to disable service '{service}'"))?; Ok(()) } diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs index 76314ef3b..ca043ba48 100644 --- a/crates/osmodifier/src/users.rs +++ b/crates/osmodifier/src/users.rs @@ -7,6 +7,7 @@ use std::{fs, io::Write, os::unix::fs::PermissionsExt, path::Path, process::Comm use anyhow::{bail, Context, Error}; use log::{debug, info}; +use osutils::Dependency; use crate::{ config::{MICUser, PasswordType}, @@ -104,21 +105,24 @@ fn add_or_update_user(ctx: &OsModifierContext, user: &MICUser) -> Result<(), Err } fn check_user_exists(root: &str, username: &str) -> Result { - let status = if root == "/" { - Command::new("id").arg("-u").arg(username).status() + let output = if root == "/" { + Dependency::Id.cmd().args(["-u", username]).output() } else { - Command::new("chroot") + Dependency::Chroot + .cmd() .arg(root) .args(["id", "-u", username]) - .status() + .output() } .with_context(|| format!("Failed to check if user '{username}' exists"))?; - Ok(status.success()) + Ok(output.success()) } fn hash_password(plaintext: &str) -> Result { - // Use openssl to hash the password, matching the Go implementation + // TODO: Convert to Dependency::Openssl once the Command wrapper supports + // stdin piping. Currently uses std::process::Command directly because + // openssl passwd reads the password from stdin. let mut child = Command::new("openssl") .args(["passwd", "-6", "-stdin"]) .stdin(std::process::Stdio::piped()) @@ -150,9 +154,9 @@ fn hash_password(plaintext: &str) -> Result { fn create_user(root: &str, user: &MICUser) -> Result<(), Error> { let mut cmd = if root == "/" { - Command::new("useradd") + Dependency::Useradd.cmd() } else { - let mut c = Command::new("chroot"); + let mut c = Dependency::Chroot.cmd(); c.arg(root).arg("useradd"); c }; @@ -176,14 +180,8 @@ fn create_user(root: &str, user: &MICUser) -> Result<(), Error> { cmd.arg(&user.name); - let output = cmd - .output() - .with_context(|| format!("Failed to execute useradd for '{}'", user.name))?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("useradd failed for '{}': {stderr}", user.name); - } + cmd.run_and_check() + .with_context(|| format!("Failed to create user '{}'", user.name))?; Ok(()) } @@ -226,6 +224,8 @@ fn update_user_password(ctx: &OsModifierContext, username: &str, hash: &str) -> /// Set password on a newly created user via chpasswd -e (stdin), avoiding /// leaking the hash through /proc/cmdline. fn set_password_via_chpasswd(root: &str, username: &str, hash: &str) -> Result<(), Error> { + // TODO: Convert to Dependency::{Chpasswd,Chroot} once the Command wrapper + // supports stdin piping. chpasswd reads username:hash from stdin. debug!("Setting password for new user '{username}' via chpasswd"); let input = format!("{username}:{hash}\n"); @@ -312,44 +312,40 @@ fn set_password_expiry(ctx: &OsModifierContext, username: &str, days: u64) -> Re fn set_primary_group(root: &str, username: &str, group: &str) -> Result<(), Error> { debug!("Setting primary group for '{username}' to '{group}'"); - let output = if root == "/" { - Command::new("usermod") + if root == "/" { + Dependency::Usermod + .cmd() .args(["-g", group, username]) - .output() + .run_and_check() } else { - Command::new("chroot") + Dependency::Chroot + .cmd() .arg(root) .args(["usermod", "-g", group, username]) - .output() + .run_and_check() } .with_context(|| format!("Failed to set primary group for '{username}'"))?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("usermod -g failed for '{username}': {stderr}"); - } Ok(()) } fn set_secondary_groups(root: &str, username: &str, groups: &[String]) -> Result<(), Error> { let groups_str = groups.join(","); debug!("Setting secondary groups for '{username}' to '{groups_str}'"); - let output = if root == "/" { - Command::new("usermod") + if root == "/" { + Dependency::Usermod + .cmd() .args(["-a", "-G", &groups_str, username]) - .output() + .run_and_check() } else { - Command::new("chroot") + Dependency::Chroot + .cmd() .arg(root) .args(["usermod", "-a", "-G", &groups_str, username]) - .output() + .run_and_check() } .with_context(|| format!("Failed to set secondary groups for '{username}'"))?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("usermod -a -G failed for '{username}': {stderr}"); - } Ok(()) } @@ -412,25 +408,23 @@ fn set_ownership(ctx: &OsModifierContext, username: &str, path: &Path) -> Result let root = ctx.root.to_str().unwrap_or("/"); let path_str = path.to_str().context("Failed to convert path to string")?; - let output = if root == "/" { - Command::new("chown") + if root == "/" { + Dependency::Chown + .cmd() .args([&format!("{username}:{username}"), path_str]) - .output() + .run_and_check() } else { // For non-root context, strip the root prefix for chroot let relative = path.strip_prefix(&ctx.root).unwrap_or(path); let rel_str = relative.to_str().context("path to string")?; - Command::new("chroot") + Dependency::Chroot + .cmd() .arg(root) .args(["chown", &format!("{username}:{username}"), rel_str]) - .output() + .run_and_check() } .with_context(|| format!("Failed to chown '{}'", path.display()))?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("chown failed for '{}': {stderr}", path.display()); - } Ok(()) } diff --git a/crates/osutils/src/dependencies.rs b/crates/osutils/src/dependencies.rs index f9cec1b64..5f21aa6aa 100644 --- a/crates/osutils/src/dependencies.rs +++ b/crates/osutils/src/dependencies.rs @@ -89,6 +89,8 @@ impl DependencyResultExt for Result> { #[strum(serialize_all = "lowercase")] pub enum Dependency { Blkid, + Chown, + Chroot, Cryptsetup, Dd, Df, @@ -98,6 +100,9 @@ pub enum Dependency { Efibootmgr, Eject, Findmnt, + #[strum(serialize = "grub2-mkconfig")] + Grub2Mkconfig, + Id, Iptables, Journalctl, Losetup, @@ -118,6 +123,8 @@ pub enum Dependency { Swapoff, Swapon, Systemctl, + Useradd, + Usermod, #[strum(serialize = "systemd-confext")] SystemdConfext, #[strum(serialize = "systemd-cryptenroll")] From a97e604ec918d27ee58df754b3661c9d6ba4b455 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Sat, 16 May 2026 08:55:45 -0700 Subject: [PATCH 15/20] simplify: remove chroot codepath from osmodifier Trident always chroots into newroot before calling osmodifier, so root is always '/'. Remove the if root=='/' / else chroot branches, drop the Chroot dependency variant, and simplify function signatures by removing the unused root parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/README.md | 14 +-- crates/osmodifier/src/lib.rs | 7 +- crates/osmodifier/src/users.rs | 131 +++++++++-------------------- crates/osutils/src/dependencies.rs | 1 - 4 files changed, 51 insertions(+), 102 deletions(-) diff --git a/crates/osmodifier/README.md b/crates/osmodifier/README.md index 3f3d45261..09d0c404e 100644 --- a/crates/osmodifier/README.md +++ b/crates/osmodifier/README.md @@ -59,13 +59,14 @@ instead of being parsed from stderr. ### No chroot / safechroot The Go code uses `safechroot` to enter a chroot environment before making -modifications. The Rust version operates on a mounted root directory via -`OsModifierContext`, prefixing all paths with the root directory. +modifications. The Rust version assumes it is already running inside the +chroot (trident manages the chroot lifecycle at a higher level). File +operations use `OsModifierContext` for path resolution; system tool +invocations (`useradd`, `usermod`, etc.) run directly against `/`. -**Reasoning:** Trident already manages the chroot lifecycle at a higher level. -Duplicating chroot enter/exit in osmodifier would conflict with the outer -chroot management. Path-prefixing achieves the same isolation without the -complexity. +**Reasoning:** Trident always chroots into newroot before calling osmodifier. +Duplicating chroot enter/exit here would conflict with the outer chroot +management and add unnecessary complexity. ### Inlined imagecustomizerlib logic @@ -128,7 +129,6 @@ inventory of runtime dependencies. |--------------------|---------| | `Systemctl` | `services.rs` — enable/disable services | | `Grub2Mkconfig` | `grub_cfg.rs` — regenerate GRUB config | -| `Chroot` | `users.rs` — run tools inside a mounted root | | `Id` | `users.rs` — check if a user exists | | `Useradd` | `users.rs` — create new users | | `Usermod` | `users.rs` — modify groups | diff --git a/crates/osmodifier/src/lib.rs b/crates/osmodifier/src/lib.rs index 624bfe5e1..2f5f9141d 100644 --- a/crates/osmodifier/src/lib.rs +++ b/crates/osmodifier/src/lib.rs @@ -25,9 +25,10 @@ pub use config::*; /// Execution context for OS modifier operations. /// -/// All filesystem paths are resolved relative to `root`. When trident has -/// chrooted into newroot, `root` should be `/`. When operating on an offline -/// image mounted at a specific path, set `root` accordingly. +/// All filesystem paths are resolved relative to `root`. Trident always +/// runs osmodifier after chrooting into newroot, so `root` is `/` in +/// production. The non-`/` option exists only for unit tests that +/// operate on a temp directory. pub struct OsModifierContext { /// Root directory for all filesystem operations. pub root: PathBuf, diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs index ca043ba48..dcbc23904 100644 --- a/crates/osmodifier/src/users.rs +++ b/crates/osmodifier/src/users.rs @@ -24,8 +24,6 @@ pub fn add_or_update_users(ctx: &OsModifierContext, users: &[MICUser]) -> Result } fn add_or_update_user(ctx: &OsModifierContext, user: &MICUser) -> Result<(), Error> { - let root = ctx.root.to_str().unwrap_or("/"); - // Hash the password if needed let hashed_password = match &user.password { Some(pwd) => match pwd.password_type { @@ -41,7 +39,7 @@ fn add_or_update_user(ctx: &OsModifierContext, user: &MICUser) -> Result<(), Err .as_ref() .is_some_and(|p| p.password_type == PasswordType::Locked); - let user_exists = check_user_exists(root, &user.name)?; + let user_exists = check_user_exists(&user.name)?; if user_exists { debug!("User '{}' already exists, updating", user.name); @@ -69,12 +67,12 @@ fn add_or_update_user(ctx: &OsModifierContext, user: &MICUser) -> Result<(), Err } } else { info!("Creating user '{}'", user.name); - create_user(root, user)?; + create_user(user)?; // Set password after creation via chpasswd (avoids leaking hash in // /proc/cmdline that useradd -p would cause). if let Some(ref hash) = hashed_password { - set_password_via_chpasswd(root, &user.name, hash)?; + set_password_via_chpasswd(&user.name, hash)?; } } @@ -85,10 +83,10 @@ fn add_or_update_user(ctx: &OsModifierContext, user: &MICUser) -> Result<(), Err // Update groups if let Some(ref primary) = user.primary_group { - set_primary_group(root, &user.name, primary)?; + set_primary_group(&user.name, primary)?; } if !user.secondary_groups.is_empty() { - set_secondary_groups(root, &user.name, &user.secondary_groups)?; + set_secondary_groups(&user.name, &user.secondary_groups)?; } // SSH keys @@ -104,17 +102,12 @@ fn add_or_update_user(ctx: &OsModifierContext, user: &MICUser) -> Result<(), Err Ok(()) } -fn check_user_exists(root: &str, username: &str) -> Result { - let output = if root == "/" { - Dependency::Id.cmd().args(["-u", username]).output() - } else { - Dependency::Chroot - .cmd() - .arg(root) - .args(["id", "-u", username]) - .output() - } - .with_context(|| format!("Failed to check if user '{username}' exists"))?; +fn check_user_exists(username: &str) -> Result { + let output = Dependency::Id + .cmd() + .args(["-u", username]) + .output() + .with_context(|| format!("Failed to check if user '{username}' exists"))?; Ok(output.success()) } @@ -152,14 +145,8 @@ fn hash_password(plaintext: &str) -> Result { .to_string()) } -fn create_user(root: &str, user: &MICUser) -> Result<(), Error> { - let mut cmd = if root == "/" { - Dependency::Useradd.cmd() - } else { - let mut c = Dependency::Chroot.cmd(); - c.arg(root).arg("useradd"); - c - }; +fn create_user(user: &MICUser) -> Result<(), Error> { + let mut cmd = Dependency::Useradd.cmd(); cmd.arg("-m"); // Create home directory @@ -223,29 +210,19 @@ fn update_user_password(ctx: &OsModifierContext, username: &str, hash: &str) -> /// Set password on a newly created user via chpasswd -e (stdin), avoiding /// leaking the hash through /proc/cmdline. -fn set_password_via_chpasswd(root: &str, username: &str, hash: &str) -> Result<(), Error> { - // TODO: Convert to Dependency::{Chpasswd,Chroot} once the Command wrapper - // supports stdin piping. chpasswd reads username:hash from stdin. +fn set_password_via_chpasswd(username: &str, hash: &str) -> Result<(), Error> { + // TODO: Convert to Dependency::Chpasswd once the Command wrapper supports + // stdin piping. chpasswd reads username:hash from stdin. debug!("Setting password for new user '{username}' via chpasswd"); let input = format!("{username}:{hash}\n"); - let mut child = if root == "/" { - Command::new("chpasswd") - .arg("-e") - .stdin(std::process::Stdio::piped()) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() - } else { - Command::new("chroot") - .arg(root) - .args(["chpasswd", "-e"]) - .stdin(std::process::Stdio::piped()) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() - } - .context("Failed to start chpasswd")?; + let mut child = Command::new("chpasswd") + .arg("-e") + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .context("Failed to start chpasswd")?; if let Some(ref mut stdin) = child.stdin { stdin @@ -310,41 +287,25 @@ fn set_password_expiry(ctx: &OsModifierContext, username: &str, days: u64) -> Re atomic_write_file(&shadow_path, &result) } -fn set_primary_group(root: &str, username: &str, group: &str) -> Result<(), Error> { +fn set_primary_group(username: &str, group: &str) -> Result<(), Error> { debug!("Setting primary group for '{username}' to '{group}'"); - if root == "/" { - Dependency::Usermod - .cmd() - .args(["-g", group, username]) - .run_and_check() - } else { - Dependency::Chroot - .cmd() - .arg(root) - .args(["usermod", "-g", group, username]) - .run_and_check() - } - .with_context(|| format!("Failed to set primary group for '{username}'"))?; + Dependency::Usermod + .cmd() + .args(["-g", group, username]) + .run_and_check() + .with_context(|| format!("Failed to set primary group for '{username}'"))?; Ok(()) } -fn set_secondary_groups(root: &str, username: &str, groups: &[String]) -> Result<(), Error> { +fn set_secondary_groups(username: &str, groups: &[String]) -> Result<(), Error> { let groups_str = groups.join(","); debug!("Setting secondary groups for '{username}' to '{groups_str}'"); - if root == "/" { - Dependency::Usermod - .cmd() - .args(["-a", "-G", &groups_str, username]) - .run_and_check() - } else { - Dependency::Chroot - .cmd() - .arg(root) - .args(["usermod", "-a", "-G", &groups_str, username]) - .run_and_check() - } - .with_context(|| format!("Failed to set secondary groups for '{username}'"))?; + Dependency::Usermod + .cmd() + .args(["-a", "-G", &groups_str, username]) + .run_and_check() + .with_context(|| format!("Failed to set secondary groups for '{username}'"))?; Ok(()) } @@ -405,25 +366,13 @@ fn get_home_dir(ctx: &OsModifierContext, username: &str) -> Result Result<(), Error> { - let root = ctx.root.to_str().unwrap_or("/"); let path_str = path.to_str().context("Failed to convert path to string")?; - if root == "/" { - Dependency::Chown - .cmd() - .args([&format!("{username}:{username}"), path_str]) - .run_and_check() - } else { - // For non-root context, strip the root prefix for chroot - let relative = path.strip_prefix(&ctx.root).unwrap_or(path); - let rel_str = relative.to_str().context("path to string")?; - Dependency::Chroot - .cmd() - .arg(root) - .args(["chown", &format!("{username}:{username}"), rel_str]) - .run_and_check() - } - .with_context(|| format!("Failed to chown '{}'", path.display()))?; + Dependency::Chown + .cmd() + .args([&format!("{username}:{username}"), path_str]) + .run_and_check() + .with_context(|| format!("Failed to chown '{}'", path.display()))?; Ok(()) } diff --git a/crates/osutils/src/dependencies.rs b/crates/osutils/src/dependencies.rs index 5f21aa6aa..81649266e 100644 --- a/crates/osutils/src/dependencies.rs +++ b/crates/osutils/src/dependencies.rs @@ -90,7 +90,6 @@ impl DependencyResultExt for Result> { pub enum Dependency { Blkid, Chown, - Chroot, Cryptsetup, Dd, Df, From 691b57777bf73e311fbbb2d21f0531a6f0038435 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Sat, 16 May 2026 09:11:38 -0700 Subject: [PATCH 16/20] fix: break cyclic dependency between osutils and osmodifier Remove the osutils::osmodifier re-export shim and its dependency on the osmodifier crate. The single consumer (trident/subsystems/osconfig/ users.rs) now imports from osmodifier directly, which it already had as a dependency. This breaks the cycle: osutils -> osmodifier -> osutils. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osutils/Cargo.toml | 1 - crates/osutils/src/lib.rs | 1 - crates/osutils/src/osmodifier.rs | 22 ------------------- .../trident/src/subsystems/osconfig/users.rs | 2 +- 4 files changed, 1 insertion(+), 25 deletions(-) delete mode 100644 crates/osutils/src/osmodifier.rs diff --git a/crates/osutils/Cargo.toml b/crates/osutils/Cargo.toml index 42e0085e6..839eac14b 100644 --- a/crates/osutils/Cargo.toml +++ b/crates/osutils/Cargo.toml @@ -39,7 +39,6 @@ rand = {workspace = true, optional = true} pytest_gen = { path = "../pytest_gen" } pytest = { path = "../pytest", optional = true } -osmodifier = { path = "../osmodifier" } trident_api = { path = "../trident_api" } sysdefs = { path = "../sysdefs" } diff --git a/crates/osutils/src/lib.rs b/crates/osutils/src/lib.rs index e2a11dc82..2ebb435f8 100644 --- a/crates/osutils/src/lib.rs +++ b/crates/osutils/src/lib.rs @@ -26,7 +26,6 @@ pub mod mkinitrd; pub mod mount; pub mod mountpoint; pub mod netplan; -pub mod osmodifier; pub mod osrelease; pub mod overlay; pub mod path; diff --git a/crates/osutils/src/osmodifier.rs b/crates/osutils/src/osmodifier.rs deleted file mode 100644 index cbcd2f62b..000000000 --- a/crates/osutils/src/osmodifier.rs +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -//! Re-exports from the `osmodifier` crate for backwards compatibility. -//! -//! The types and functions have moved to the standalone `osmodifier` crate. -//! This module re-exports them so that existing `osutils::osmodifier::*` -//! imports continue to work during the migration. - -pub use osmodifier::{ - BootConfig, CorruptionOption, IdentifiedPartition, MICPassword, MICUser, OSModifierConfig, - Overlay, PasswordType, Verity, -}; - -use serde::Serialize; -use trident_api::config::Services; - -#[derive(Serialize)] -#[serde(rename_all = "camelCase")] -pub struct MICServices { - pub services: Services, -} diff --git a/crates/trident/src/subsystems/osconfig/users.rs b/crates/trident/src/subsystems/osconfig/users.rs index c0d4d1046..4bba5247e 100644 --- a/crates/trident/src/subsystems/osconfig/users.rs +++ b/crates/trident/src/subsystems/osconfig/users.rs @@ -6,7 +6,7 @@ use std::{ use anyhow::{bail, Context, Error}; use log::{debug, warn}; -use osutils::osmodifier::{MICPassword, MICUser, PasswordType}; +use osmodifier::{MICPassword, MICUser, PasswordType}; use trident_api::config::{Password, SshMode, User}; const SSHD_CONFIG_FILE: &str = "/etc/ssh/sshd_config"; From 33fb451a0a8ce1d9f4a020feeb1deb0320713c75 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Sat, 16 May 2026 09:23:20 -0700 Subject: [PATCH 17/20] =?UTF-8?q?fix:=20update=20Cargo.lock=20after=20remo?= =?UTF-8?q?ving=20osutils=E2=86=92osmodifier=20dependency?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 69ca6c2e3..1bf18b951 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1872,7 +1872,8 @@ dependencies = [ "log", "pytest", "pytest_gen", - "regex", + "osutils", + "regex", "serde", "serde_yaml", "tempfile", @@ -1898,8 +1899,7 @@ dependencies = [ "nix", "once_cell", "openssl", - "osmodifier", - "pytest", + "pytest", "pytest_gen", "rand 0.9.0", "regex", @@ -4372,4 +4372,4 @@ checksum = "91e19ebc2adc8f83e43039e79776e3fda8ca919132d68a1fed6a5faca2683748" dependencies = [ "cc", "pkg-config", -] +] \ No newline at end of file From af8c03b9ead13f1e87cf4961c5182f8f4b574d75 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Sat, 16 May 2026 09:31:10 -0700 Subject: [PATCH 18/20] fix: use full import path osutils::dependencies::Dependency Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/src/grub_cfg.rs | 3 ++- crates/osmodifier/src/services.rs | 3 ++- crates/osmodifier/src/users.rs | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/osmodifier/src/grub_cfg.rs b/crates/osmodifier/src/grub_cfg.rs index 80cd641f5..5ec78ae8c 100644 --- a/crates/osmodifier/src/grub_cfg.rs +++ b/crates/osmodifier/src/grub_cfg.rs @@ -10,7 +10,7 @@ use std::fs; use anyhow::{bail, Context, Error}; use log::{debug, info, trace}; -use osutils::Dependency; +use osutils::dependencies::Dependency; use regex::Regex; use crate::OsModifierContext; @@ -204,3 +204,4 @@ mod tests { assert!(find_non_recovery_linux_line(grub_cfg).is_err()); } } + diff --git a/crates/osmodifier/src/services.rs b/crates/osmodifier/src/services.rs index ebe774f34..9f0775085 100644 --- a/crates/osmodifier/src/services.rs +++ b/crates/osmodifier/src/services.rs @@ -5,7 +5,7 @@ use anyhow::{Context, Error}; use log::{debug, warn}; -use osutils::Dependency; +use osutils::dependencies::Dependency; use trident_api::config::Services; @@ -171,3 +171,4 @@ mod functional_test { ); } } + diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs index dcbc23904..c8d7d222f 100644 --- a/crates/osmodifier/src/users.rs +++ b/crates/osmodifier/src/users.rs @@ -7,7 +7,7 @@ use std::{fs, io::Write, os::unix::fs::PermissionsExt, path::Path, process::Comm use anyhow::{bail, Context, Error}; use log::{debug, info}; -use osutils::Dependency; +use osutils::dependencies::Dependency; use crate::{ config::{MICUser, PasswordType}, @@ -452,3 +452,4 @@ fn atomic_write_file(path: &std::path::Path, content: &str) -> Result<(), Error> Ok(()) } + From cba55c5fb0a9570b6339de8e86b67f0bdb1a2702 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Sat, 16 May 2026 09:38:15 -0700 Subject: [PATCH 19/20] fix: remove trailing blank lines (cargo fmt) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/src/grub_cfg.rs | 3 +++ crates/osmodifier/src/services.rs | 3 +++ crates/osmodifier/src/users.rs | 3 +++ 3 files changed, 9 insertions(+) diff --git a/crates/osmodifier/src/grub_cfg.rs b/crates/osmodifier/src/grub_cfg.rs index 5ec78ae8c..3e4400c76 100644 --- a/crates/osmodifier/src/grub_cfg.rs +++ b/crates/osmodifier/src/grub_cfg.rs @@ -205,3 +205,6 @@ mod tests { } } + + + diff --git a/crates/osmodifier/src/services.rs b/crates/osmodifier/src/services.rs index 9f0775085..67febb6e1 100644 --- a/crates/osmodifier/src/services.rs +++ b/crates/osmodifier/src/services.rs @@ -172,3 +172,6 @@ mod functional_test { } } + + + diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs index c8d7d222f..e9ab6f08d 100644 --- a/crates/osmodifier/src/users.rs +++ b/crates/osmodifier/src/users.rs @@ -453,3 +453,6 @@ fn atomic_write_file(path: &std::path::Path, content: &str) -> Result<(), Error> Ok(()) } + + + From 0e2ab384631584c60d2af4e77934425f4ac43bcb Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Sat, 16 May 2026 09:45:45 -0700 Subject: [PATCH 20/20] fix: cargo fmt and unused variable warning Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 8 ++++---- crates/osmodifier/src/grub_cfg.rs | 4 ---- crates/osmodifier/src/services.rs | 4 ---- crates/osmodifier/src/users.rs | 6 +----- 4 files changed, 5 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1bf18b951..e85f1714c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1870,10 +1870,10 @@ dependencies = [ "indoc", "inventory", "log", + "osutils", "pytest", "pytest_gen", - "osutils", - "regex", + "regex", "serde", "serde_yaml", "tempfile", @@ -1899,7 +1899,7 @@ dependencies = [ "nix", "once_cell", "openssl", - "pytest", + "pytest", "pytest_gen", "rand 0.9.0", "regex", @@ -4372,4 +4372,4 @@ checksum = "91e19ebc2adc8f83e43039e79776e3fda8ca919132d68a1fed6a5faca2683748" dependencies = [ "cc", "pkg-config", -] \ No newline at end of file +] diff --git a/crates/osmodifier/src/grub_cfg.rs b/crates/osmodifier/src/grub_cfg.rs index 3e4400c76..b1b32769d 100644 --- a/crates/osmodifier/src/grub_cfg.rs +++ b/crates/osmodifier/src/grub_cfg.rs @@ -204,7 +204,3 @@ mod tests { assert!(find_non_recovery_linux_line(grub_cfg).is_err()); } } - - - - diff --git a/crates/osmodifier/src/services.rs b/crates/osmodifier/src/services.rs index 67febb6e1..3a2a4a492 100644 --- a/crates/osmodifier/src/services.rs +++ b/crates/osmodifier/src/services.rs @@ -171,7 +171,3 @@ mod functional_test { ); } } - - - - diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs index e9ab6f08d..6569af3f4 100644 --- a/crates/osmodifier/src/users.rs +++ b/crates/osmodifier/src/users.rs @@ -365,7 +365,7 @@ fn get_home_dir(ctx: &OsModifierContext, username: &str) -> Result Result<(), Error> { +fn set_ownership(_ctx: &OsModifierContext, username: &str, path: &Path) -> Result<(), Error> { let path_str = path.to_str().context("Failed to convert path to string")?; Dependency::Chown @@ -452,7 +452,3 @@ fn atomic_write_file(path: &std::path::Path, content: &str) -> Result<(), Error> Ok(()) } - - - -