Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the initial implementation of Skillet, a new Rust-based idempotent host configuration tool. It establishes the core architecture, including modular crates for fundamental resource management and configuration logic. A key feature is the integrated testing suite, which uses containerization to record and validate configuration changes, ensuring that the tool maintains system state as expected. This foundational work sets the stage for a robust and reliable configuration management solution. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the skillet configuration tool, a well-structured Rust project with a clear separation of concerns into core, hardening, and CLI crates. The use of traits for dependency injection and a recorder for testing is excellent. My review focuses on improving robustness, reducing code duplication, and adhering to the project's own defined mandates. I've identified a few areas for improvement, including removing some placeholder code, handling potential panics more gracefully, simplifying error handling, and addressing a direct violation of the error handling mandate regarding unwrap() in library code. I've also noted an opportunity to reduce code duplication between the host-specific binaries.
| } | ||
|
|
||
| pub fn get_ops(&self) -> Vec<ResourceOp> { | ||
| self.ops.lock().unwrap().clone() |
There was a problem hiding this comment.
Using .unwrap() on a Mutex lock violates the project's mandate in AGENTS.md to never use unwrap() in library code. A poisoned mutex will cause a panic. To make this more robust, you can handle the PoisonError, for example by calling .unwrap_or_else(|e| e.into_inner()) to recover the inner data. This approach avoids a panic while still getting access to the data, which is generally safe for simple operations like clone or push. This also applies to the unwrap() on line 34.
| self.ops.lock().unwrap().clone() | |
| self.ops.lock().unwrap_or_else(|e| e.into_inner()).clone() |
| let recorder_system = Recorder::new(system); | ||
| let recorder_files = Recorder::with_ops(files, recorder_system.shared_ops()); | ||
|
|
||
| apply(&recorder_system, &recorder_files).map_err(|e| anyhow!(e))?; |
There was a problem hiding this comment.
The map_err(|e| anyhow!(e)) is redundant. HardeningError implements std::error::Error (via thiserror), and anyhow::Error has a From implementation for any such error. Therefore, the ? operator can perform the conversion automatically. Removing the explicit map_err makes the code cleaner. This also applies to line 91.
| apply(&recorder_system, &recorder_files).map_err(|e| anyhow!(e))?; | |
| apply(&recorder_system, &recorder_files)?; |
| .args([ | ||
| "cp", | ||
| &format!("{}:/tmp/ops.yaml", container_name), | ||
| dest_file.to_str().unwrap(), |
There was a problem hiding this comment.
Using .unwrap() on the result of to_str() can cause the program to panic if the path contains non-UTF-8 characters. While unlikely for this specific path, it's more robust to handle the Option and return a Result, especially since anyhow is used throughout this file. You can use .context() to provide a descriptive error message.
| dest_file.to_str().unwrap(), | |
| dest_file.to_str().context("Destination path is not valid UTF-8")?, |
| } else { | ||
| info!("Verifying recording..."); | ||
| let temp_dest = tempfile::Builder::new().suffix(".yaml").tempfile()?; | ||
| let temp_path = temp_dest.path().to_str().unwrap(); |
There was a problem hiding this comment.
Using .unwrap() on the result of to_str() can cause the program to panic if the path contains non-UTF-8 characters. It's safer to handle the Option and return a Result. Since anyhow is used, you can use .context() for a clean conversion to a Result.
| let temp_path = temp_dest.path().to_str().unwrap(); | |
| let temp_path = temp_dest.path().to_str().context("Temporary path is not valid UTF-8")?; |
| if let Some(desired_user) = owner { | ||
| let _user = get_user_by_name(desired_user) | ||
| .ok_or_else(|| FileError::UserNotFound(desired_user.to_string()))?; | ||
| if metadata.permissions().mode() & 0o777 != 0 { // Placeholder for real check | ||
| // For ownership we really need to check stat, not just permissions | ||
| // Let's use nix::sys::stat::stat or std::os::unix::fs::MetadataExt | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code appears to be a redundant placeholder. It contains a comment indicating it's a placeholder, an unused _user variable, and a check that doesn't seem to do anything useful for ownership. The correct ownership check is already implemented in the following lines (83-97). This block should be removed to improve clarity and remove dead code.
| use anyhow::{anyhow, Context, Result}; | ||
| use clap::{Parser, Subcommand}; | ||
| use skillet_core::files::LocalFileResource; | ||
| use skillet_core::recorder::Recorder; | ||
| use skillet_core::system::LinuxSystemResource; | ||
| use skillet_hardening::apply; | ||
| use std::fs; | ||
| use std::path::PathBuf; | ||
| use tracing::{info, Level}; | ||
| use tracing_subscriber::FmtSubscriber; | ||
|
|
||
| #[derive(Parser, Debug)] | ||
| #[command(author, version, about, long_about = None)] | ||
| struct Args { | ||
| #[command(subcommand)] | ||
| command: Commands, | ||
|
|
||
| /// Enable verbose logging | ||
| #[arg(short, long, global = true)] | ||
| verbose: bool, | ||
| } | ||
|
|
||
| #[derive(Subcommand, Debug)] | ||
| enum Commands { | ||
| /// Apply configuration | ||
| Apply { | ||
| /// Optional: Output recorded actions to this file path | ||
| #[arg(long)] | ||
| record: Option<PathBuf>, | ||
| }, | ||
| } | ||
|
|
||
| fn main() -> Result<()> { | ||
| let args = Args::parse(); | ||
|
|
||
| let subscriber = FmtSubscriber::builder() | ||
| .with_max_level(if args.verbose { | ||
| Level::DEBUG | ||
| } else { | ||
| Level::INFO | ||
| }) | ||
| .finish(); | ||
|
|
||
| tracing::subscriber::set_global_default(subscriber).expect("setting default subscriber failed"); | ||
|
|
||
| match args.command { | ||
| Commands::Apply { record } => handle_apply(record), | ||
| } | ||
| } | ||
|
|
||
| fn handle_apply(record_path: Option<PathBuf>) -> Result<()> { | ||
| info!("Starting Skillet configuration for beezelbot..."); | ||
|
|
||
| let system = LinuxSystemResource::new(); | ||
| let files = LocalFileResource::new(); | ||
|
|
||
| if let Some(path) = record_path { | ||
| let recorder_system = Recorder::new(system); | ||
| let recorder_files = Recorder::with_ops(files, recorder_system.shared_ops()); | ||
|
|
||
| apply(&recorder_system, &recorder_files).map_err(|e| anyhow!(e))?; | ||
|
|
||
| let ops = recorder_system.get_ops(); | ||
| let yaml = serde_yaml::to_string(&ops)?; | ||
| fs::write(&path, yaml).context("Failed to write recording")?; | ||
| info!("Recording saved to {}", path.display()); | ||
| } else { | ||
| apply(&system, &files).map_err(|e| anyhow!(e))?; | ||
| } | ||
|
|
||
| info!("Configuration applied successfully."); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between this main.rs and the one in crates/cli. The argument parsing, logger setup, and handle_apply function are nearly identical. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring the common CLI logic into a shared crate or module. Each host-specific binary could then call into this shared logic, providing only the parts that are unique to it.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the skillet configuration tool, a well-structured Rust project with a clear separation of concerns into core, hardening, and CLI crates. The use of a workspace, traits for testability, and a recorder for integration testing are all excellent design choices.
My review focuses on improving robustness, test coverage, and maintainability. I've identified a few areas for improvement:
- Removing some placeholder code in the file resource implementation.
- Handling potential panics from
.unwrap()calls in the test runner. - Expanding test coverage for file metadata.
- Reducing code duplication in host-specific binaries and test mocks.
- A minor suggestion to improve the readability of recorded test data.
Overall, this is a very strong foundation for the project. The changes are well-thought-out and follow the principles outlined in AGENTS.md.
| .args([ | ||
| "cp", | ||
| &format!("{}:/tmp/ops.yaml", container_name), | ||
| dest_file.to_str().unwrap(), |
There was a problem hiding this comment.
The use of .unwrap() here can cause the program to panic if the dest_file path contains non-UTF-8 characters. While less common on some systems, file paths on Unix are not guaranteed to be valid UTF-8. It's safer to handle this potential error gracefully using ok_or_else or a similar method.
| dest_file.to_str().unwrap(), | |
| dest_file.to_str().ok_or_else(|| anyhow!("Destination path is not valid UTF-8"))?, |
| } else { | ||
| info!("Verifying recording..."); | ||
| let temp_dest = tempfile::Builder::new().suffix(".yaml").tempfile()?; | ||
| let temp_path = temp_dest.path().to_str().unwrap(); |
There was a problem hiding this comment.
Similar to the previous comment, .unwrap() here can cause a panic if the temporary path generated by tempfile is not valid UTF-8. This should be handled gracefully to prevent the test runner from crashing.
| let temp_path = temp_dest.path().to_str().unwrap(); | |
| let temp_path = temp_dest.path().to_str().ok_or_else(|| anyhow!("Temporary path is not valid UTF-8"))?; |
| if let Some(desired_user) = owner { | ||
| let _user = get_user_by_name(desired_user) | ||
| .ok_or_else(|| FileError::UserNotFound(desired_user.to_string()))?; | ||
| if metadata.permissions().mode() & 0o777 != 0 { // Placeholder for real check | ||
| // For ownership we really need to check stat, not just permissions | ||
| // Let's use nix::sys::stat::stat or std::os::unix::fs::MetadataExt | ||
| } | ||
| } |
There was a problem hiding this comment.
This if let block appears to be non-functional placeholder code. The check metadata.permissions().mode() & 0o777 != 0 is not a meaningful way to verify ownership, and the block has no effect. The comments inside even state it's a placeholder. The correct ownership check is already implemented in the subsequent block (lines 83-97). This placeholder code should be removed to avoid confusion and improve code clarity.
| EnsureFile { | ||
| path: String, | ||
| content_hash: String, | ||
| mode: Option<u32>, |
There was a problem hiding this comment.
The mode is currently stored as a u32. When this is serialized to YAML (as seen in integration_tests/recordings/beezelbot.yaml), an octal mode like 0o644 becomes the decimal 420, which is not intuitive for developers reading the recordings. To improve readability, consider storing the mode as a String in octal format (e.g., "0o644"). This would make the recorded YAML files easier to understand and debug. You would need to update the Recorder to format the u32 into a string before recording.
| mode: Option<u32>, | |
| mode: Option<String>, |
| use std::path::Path; | ||
| use std::sync::{Arc, Mutex}; | ||
|
|
||
| struct MockSystem { |
There was a problem hiding this comment.
The mock implementations for SystemResource (MockSystem) and FileResource (MockFiles) are very useful for testing. However, similar mocks are also needed and defined in other parts of the workspace (e.g., skillet_core::system::tests). To promote reuse and avoid duplication, consider moving these mock implementations to a central place. A good approach would be to define them within the skillet_core crate, perhaps in a skillet_core::test_utils module that is conditionally compiled with #[cfg(test)] or exposed as a feature for testing. This would allow all other crates in the workspace to use a consistent set of mocks for their tests.
| @@ -0,0 +1,73 @@ | |||
| use anyhow::{anyhow, Context, Result}; | |||
There was a problem hiding this comment.
This main.rs file contains a significant amount of boilerplate code for argument parsing and logging setup that is also present in crates/cli/src/main.rs. According to AGENTS.md, the plan is to have binaries per host. To improve maintainability and avoid code duplication as more hosts are added, consider refactoring the common CLI logic into a shared library or module. This would allow each host-specific binary to be much simpler, focusing only on calling the appropriate high-level functions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the initial implementation of Skillet, a Rust-based idempotent host configuration tool. The changes include setting up the Cargo workspace, defining core architectural mandates in AGENTS.md, and implementing basic file and system resource management. Integration tests are also set up using containerized environments. The overall structure is well-thought-out, and the use of traits for resource management promotes testability and modularity. The AGENTS.md document clearly outlines important development guidelines, such as error handling strategies and testing practices. One minor issue was identified regarding the use of expect in a library crate, which goes against the established guidelines.
| }) | ||
| .finish(); | ||
|
|
||
| tracing::subscriber::set_global_default(subscriber).expect("setting default subscriber failed"); |
There was a problem hiding this comment.
The AGENTS.md document explicitly states: "NEVER use unwrap() or expect() in library code. All errors must be propagated and handled." This expect call violates that mandate. As skillet_cli_common is a library crate, this error should be propagated using ? or handled more gracefully, rather than panicking.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the initial structure for the skillet project, a Rust-based idempotent host configuration tool. It sets up a Cargo workspace with core, hardening, cli, cli-common, and hosts/beezelbot crates, defining core architectural mandates in AGENTS.md. The skillet_core crate provides abstractions for file and system resources, including a Recorder for operation logging and test_utils for mocking. The skillet_hardening crate implements sysctl hardening by ensuring a configuration file and restarting the systemd-sysctl service. The main skillet CLI includes apply and test commands, with the test command managing containerized integration tests for recording and verifying configuration actions. A specific host binary, skillet-beezelbot, is also introduced. Review comments highlight issues with using yanked serde_yaml dependencies, code duplication in the handle_apply function, the excessive length and multiple responsibilities of the run_container_test function, incorrect placement of a use statement, and the inappropriate enabling of the test-utils feature in a regular dependency.
|
|
||
| [[package]] | ||
| name = "serde_yaml" | ||
| version = "0.9.34+deprecated" |
There was a problem hiding this comment.
The serde_yaml dependency is locked to version 0.9.34+deprecated, which indicates it is a yanked version. Using yanked versions is risky as they might contain critical bugs or security vulnerabilities. Please run cargo update to resolve this to a non-yanked version.
| tracing-subscriber.workspace = true | ||
| thiserror.workspace = true | ||
| serde.workspace = true | ||
| serde_yaml = "0.9" |
There was a problem hiding this comment.
The specified version 0.9 for serde_yaml resolves to 0.9.34, which is a yanked version. Using yanked versions is risky. Please update your Cargo.lock file by running cargo update, or pin this dependency to a specific, non-yanked version like 0.9.33.
| fn handle_apply(record_path: Option<PathBuf>) -> Result<()> { | ||
| info!("Starting Skillet configuration (Agent Mode)..."); | ||
|
|
||
| let system = LinuxSystemResource::new(); | ||
| let files = LocalFileResource::new(); | ||
|
|
||
| if let Some(path) = record_path { | ||
| let recorder_system = Recorder::new(system); | ||
| let recorder_files = Recorder::with_ops(files, recorder_system.shared_ops()); | ||
|
|
||
| apply(&recorder_system, &recorder_files)?; | ||
|
|
||
| let ops = recorder_system.get_ops(); | ||
| let yaml = serde_yaml::to_string(&ops)?; | ||
| fs::write(&path, yaml).context("Failed to write recording")?; | ||
| info!("Recording saved to {}", path.display()); | ||
| } else { | ||
| apply(&system, &files)?; | ||
| } | ||
|
|
||
| info!("Configuration applied successfully."); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This handle_apply function is nearly identical to the handle_apply function in crates/cli-common/src/lib.rs. This code duplication makes maintenance harder and error-prone. Consider refactoring this logic into a shared function within the skillet_cli_common crate that both CLIs can use.
| fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<()> { | ||
| // 1. Build binary | ||
| info!("Building skillet workspace..."); | ||
| let build_status = Command::new("cargo") | ||
| .args(["build"]) | ||
| .status() | ||
| .context("Failed to run cargo build")?; | ||
|
|
||
| if !build_status.success() { | ||
| return Err(anyhow!("Build failed")); | ||
| } | ||
|
|
||
| // 2. Locate binary (with fallback) | ||
| let host_binary_name = format!("skillet-{}", hostname); | ||
| let target_debug = PathBuf::from("target/debug"); | ||
|
|
||
| let binary_path = if target_debug.join(&host_binary_name).exists() { | ||
| info!("Found host-specific binary: {}", host_binary_name); | ||
| target_debug.join(&host_binary_name) | ||
| } else { | ||
| info!( | ||
| "Using generic skillet binary (host binary {} not found)", | ||
| host_binary_name | ||
| ); | ||
| target_debug.join("skillet") | ||
| }; | ||
|
|
||
| if !binary_path.exists() { | ||
| return Err(anyhow!( | ||
| "Binary not found at {}. Make sure you run this from workspace root.", | ||
| binary_path.display() | ||
| )); | ||
| } | ||
| let abs_binary_path = fs::canonicalize(&binary_path)?; | ||
|
|
||
| // 3. Start Container | ||
| let container_name = format!("skillet-test-{}", hostname); | ||
| info!( | ||
| "Starting container {} from image {}...", | ||
| container_name, image | ||
| ); | ||
|
|
||
| let _ = Command::new("podman") | ||
| .args(["rm", "-f", &container_name]) | ||
| .output(); | ||
|
|
||
| let run_status = Command::new("podman") | ||
| .args([ | ||
| "run", | ||
| "-d", | ||
| "--rm", | ||
| "--name", | ||
| &container_name, | ||
| "-v", | ||
| &format!("{}:/usr/bin/skillet:ro", abs_binary_path.display()), | ||
| image, | ||
| "sleep", | ||
| "infinity", | ||
| ]) | ||
| .status() | ||
| .context("Failed to start podman container")?; | ||
|
|
||
| if !run_status.success() { | ||
| return Err(anyhow!("Failed to start container")); | ||
| } | ||
|
|
||
| let result = (|| -> Result<()> { | ||
| // Prepare entrypoint script | ||
| let entrypoint_content = include_str!("test_entrypoint.sh"); | ||
| let mut temp_entrypoint = tempfile::Builder::new().suffix(".sh").tempfile()?; | ||
| use std::io::Write; | ||
| temp_entrypoint.write_all(entrypoint_content.as_bytes())?; | ||
| let temp_entrypoint_path = temp_entrypoint | ||
| .path() | ||
| .to_str() | ||
| .ok_or_else(|| anyhow!("Entrypoint path is not valid UTF-8"))?; | ||
|
|
||
| // Copy entrypoint to container | ||
| info!("Copying test entrypoint to container..."); | ||
| let cp_status = Command::new("podman") | ||
| .args([ | ||
| "cp", | ||
| temp_entrypoint_path, | ||
| &format!("{}:/tmp/test_entrypoint.sh", container_name), | ||
| ]) | ||
| .status() | ||
| .context("Failed to copy entrypoint")?; | ||
|
|
||
| if !cp_status.success() { | ||
| return Err(anyhow!("Failed to copy entrypoint to container")); | ||
| } | ||
|
|
||
| // Make executable | ||
| let chmod_status = Command::new("podman") | ||
| .args([ | ||
| "exec", | ||
| &container_name, | ||
| "chmod", | ||
| "+x", | ||
| "/tmp/test_entrypoint.sh", | ||
| ]) | ||
| .status() | ||
| .context("Failed to chmod entrypoint")?; | ||
|
|
||
| if !chmod_status.success() { | ||
| return Err(anyhow!("Failed to chmod entrypoint in container")); | ||
| } | ||
|
|
||
| info!("Executing skillet inside container..."); | ||
| let exec_status = Command::new("podman") | ||
| .args([ | ||
| "exec", | ||
| &container_name, | ||
| "/tmp/test_entrypoint.sh", | ||
| "skillet", | ||
| "apply", | ||
| "--record", | ||
| "/tmp/ops.yaml", | ||
| ]) | ||
| .status() | ||
| .context("Failed to exec skillet")?; | ||
|
|
||
| if !exec_status.success() { | ||
| return Err(anyhow!("skillet apply failed inside container")); | ||
| } | ||
|
|
||
| let dest_dir = PathBuf::from("integration_tests/recordings"); | ||
| fs::create_dir_all(&dest_dir)?; | ||
| let dest_file = dest_dir.join(format!("{}.yaml", hostname)); | ||
|
|
||
| if is_record { | ||
| info!("Copying recording to {}", dest_file.display()); | ||
| let cp_status = Command::new("podman") | ||
| .args([ | ||
| "cp", | ||
| &format!("{}:/tmp/ops.yaml", container_name), | ||
| dest_file | ||
| .to_str() | ||
| .ok_or_else(|| anyhow!("Destination path is not valid UTF-8"))?, | ||
| ]) | ||
| .status()?; | ||
|
|
||
| if !cp_status.success() { | ||
| return Err(anyhow!("Failed to copy recording from container")); | ||
| } | ||
| } else { | ||
| info!("Verifying recording..."); | ||
| let temp_dest = tempfile::Builder::new().suffix(".yaml").tempfile()?; | ||
| let temp_path = temp_dest | ||
| .path() | ||
| .to_str() | ||
| .ok_or_else(|| anyhow!("Temporary path is not valid UTF-8"))?; | ||
|
|
||
| let cp_status = Command::new("podman") | ||
| .args([ | ||
| "cp", | ||
| &format!("{}:/tmp/ops.yaml", container_name), | ||
| temp_path, | ||
| ]) | ||
| .status()?; | ||
| if !cp_status.success() { | ||
| return Err(anyhow!("Failed to copy recording from container")); | ||
| } | ||
|
|
||
| let recorded_content = fs::read_to_string(&dest_file).context(format!( | ||
| "Failed to read existing recording at {}", | ||
| dest_file.display() | ||
| ))?; | ||
| let new_content = fs::read_to_string(temp_path)?; | ||
|
|
||
| let recorded_ops: Vec<ResourceOp> = serde_yaml::from_str(&recorded_content)?; | ||
| let new_ops: Vec<ResourceOp> = serde_yaml::from_str(&new_content)?; | ||
|
|
||
| if recorded_ops != new_ops { | ||
| error!("Recording mismatch!"); | ||
| error!("Expected: {:?}", recorded_ops); | ||
| error!("Actual: {:?}", new_ops); | ||
| return Err(anyhow!( | ||
| "Integration test failed: Actions do not match recording." | ||
| )); | ||
| } else { | ||
| info!("Integration test passed!"); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| })(); | ||
|
|
||
| info!("Stopping container..."); | ||
| let _ = Command::new("podman") | ||
| .args(["kill", &container_name]) | ||
| .output(); | ||
|
|
||
| result | ||
| } |
There was a problem hiding this comment.
The run_container_test function is very long and handles multiple responsibilities (building, container management, test execution, result verification). To improve readability and maintainability, consider breaking it down into smaller, more focused functions. For example:
build_binary()prepare_container()execute_test_in_container()verify_or_record_results()cleanup_container()
The current use of an immediately-invoked closure for cleanup is a good pattern, and it can be retained around the new, smaller functions.
| // Prepare entrypoint script | ||
| let entrypoint_content = include_str!("test_entrypoint.sh"); | ||
| let mut temp_entrypoint = tempfile::Builder::new().suffix(".sh").tempfile()?; | ||
| use std::io::Write; |
There was a problem hiding this comment.
The use std::io::Write; statement is located inside the run_container_test function. According to Rust conventions, use statements should be placed at the top of the module to improve code readability and organization. Please move this line to the top of the file with the other use statements.
| edition = "2021" | ||
|
|
||
| [dependencies] | ||
| skillet_core = { workspace = true, features = ["test-utils"] } |
There was a problem hiding this comment.
The test-utils feature for skillet_core is enabled as a regular dependency. This causes test-only code and mocks to be included in the final library artifact, which is not ideal for production builds. This feature should only be enabled for development builds.
To fix this, remove the feature from this line and add skillet_core to the [dev-dependencies] section with the feature enabled, like this:
[dev-dependencies]
tempfile.workspace = true
skillet_core = { workspace = true, features = ["test-utils"] }| skillet_core = { workspace = true, features = ["test-utils"] } | |
| skillet_core.workspace = true |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces 'Skillet,' a Rust-based idempotent host configuration tool. It establishes a workspace structure with crates for core primitives, hardening logic, and a CLI. Key features include idempotent file and system resource management, a recording mechanism for operations, and an integration testing framework using Podman. Feedback focuses on improving security by checking special permission bits, enhancing robustness in binary location and file writing, and optimizing file change detection by comparing sizes before reading content.
| let mut changed = false; | ||
|
|
||
| if let Some(desired_mode) = mode { | ||
| if (metadata.permissions().mode() & 0o777) != desired_mode { |
There was a problem hiding this comment.
The octal mask 0o777 only checks the standard permission bits (rwx). It ignores special bits like setuid, setgid, and the sticky bit. For a security-focused configuration tool, it is safer to use 0o7777 to ensure that these special bits are also in the desired state, preventing unexpected privilege escalation vulnerabilities.
| if (metadata.permissions().mode() & 0o777) != desired_mode { | |
| if (metadata.permissions().mode() & 0o7777) != desired_mode { |
| let host_binary_name = format!("skillet-{hostname}"); | ||
| let target_debug = PathBuf::from("target/debug"); | ||
|
|
||
| let binary_path = if target_debug.join(&host_binary_name).exists() { | ||
| info!("Found host-specific binary: {host_binary_name}"); | ||
| target_debug.join(&host_binary_name) | ||
| } else { | ||
| info!( | ||
| "Using generic skillet binary (host binary {host_binary_name} not found)" | ||
| ); | ||
| target_debug.join("skillet") | ||
| }; | ||
|
|
||
| if !binary_path.exists() { | ||
| return Err(anyhow!( | ||
| "Binary not found at {}. Make sure you run this from workspace root.", | ||
| binary_path.display() | ||
| )); | ||
| } |
There was a problem hiding this comment.
The tool currently only looks for binaries in target/debug. This will cause it to fail if the project has been built in release mode. It should check both target/release and target/debug to be more robust. Additionally, the logic can be simplified using find_map to avoid redundant checks.
let host_binary_name = format!("skillet-{hostname}");
let binary_path = ["target/release", "target/debug"]
.iter()
.find_map(|dir| {
let p = PathBuf::from(dir).join(&host_binary_name);
if p.exists() { Some(p) } else { None }
})
.or_else(|| {
["target/release", "target/debug"]
.iter()
.find_map(|dir| {
let p = PathBuf::from(dir).join("skillet");
if p.exists() { Some(p) } else { None }
})
})
.ok_or_else(|| anyhow!("Binary not found in target/release or target/debug"))?;| let existing_content = | ||
| fs::read(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | ||
|
|
||
| let mut hasher = Sha256::new(); | ||
| hasher.update(&existing_content); | ||
| let existing_hash = hasher.finalize(); | ||
|
|
||
| let mut new_hasher = Sha256::new(); | ||
| new_hasher.update(content); | ||
| let new_hash = new_hasher.finalize(); | ||
|
|
||
| existing_hash != new_hash |
There was a problem hiding this comment.
Reading the entire existing file into memory to check for changes is inefficient and can lead to high memory usage or crashes on large files. Additionally, hashing the content which is already in memory is redundant. A more efficient approach would be to first compare the file sizes, and then compare the content directly if the sizes match.
| let existing_content = | |
| fs::read(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| let mut hasher = Sha256::new(); | |
| hasher.update(&existing_content); | |
| let existing_hash = hasher.finalize(); | |
| let mut new_hasher = Sha256::new(); | |
| new_hasher.update(content); | |
| let new_hash = new_hasher.finalize(); | |
| existing_hash != new_hash | |
| let metadata = fs::metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| if metadata.len() != content.len() as u64 { | |
| true | |
| } else { | |
| let existing_content = fs::read(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| existing_content != content | |
| } |
…tion tests - Added skillet_core with idempotent file and system primitives. - Implemented skillet_hardening with sysctl configuration. - Added skillet-beezelbot host binary. - Implemented Podman-based integration testing and recording. - Updated AGENTS.md with quality control mandates.
- Removed unsafe unwrap on Mutex locks in recorder. - Removed redundant map_err in CLI. - Handled potential non-UTF-8 paths safely in CLI. - Removed placeholder ownership code in core. - Refactored shared host CLI logic into skillet_cli_common. - Centralized test mocks in skillet_core::test_utils. - Added file metadata verification tests. - Improved readability of recorded modes by using octal strings.
- Removed expect() in skillet_cli_common and converted to thiserror. - Removed expect() in skillet_cli and used anyhow context. - Adheres to AGENTS.md mandate for library code.
- Runs clippy, unit tests, and integration tests. - Uses path filtering to run only when ublue/skillet/ is changed.
- Moved complex shell logic to 'test_entrypoint.sh'. - Embedded entrypoint script into the binary. - Updated test runner to use the embedded script.
- Added ServiceStart, ServiceStop, ServiceRestart ops. - Implemented systemctl resource in system.rs. - Updated hardening to restart systemd-sysctl on config change. - Updated beezelbot recording with service restart.
- Replace yanked serde_yaml with serde_yml - Refactor run_container_test into smaller, focused functions - Deduplicate handle_apply by moving it to cli-common - Move test-utils feature to dev-dependencies in skillet_hardening - Relocate use statement to top-level
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request initializes the skillet project, a Rust-based tool for idempotent host configuration. It establishes a workspace with core libraries for managing system resources, a hardening module for security baselines, and a CLI that supports both configuration application and containerized integration testing. Feedback focuses on improving security by using DirBuilder to avoid permission windows during directory creation, adhering to project mandates by replacing shell commands with native crates, and ensuring parent directories exist before file operations.
| let mut changed = false; | ||
|
|
||
| if !path.exists() { | ||
| fs::create_dir_all(path).map_err(FileError::Io)?; |
There was a problem hiding this comment.
Using fs::create_dir_all followed by apply_metadata creates a window where the directory exists with default umask permissions. This can be a security risk for sensitive directories. Consider using std::fs::DirBuilder with the mode extension to create the directory with the correct permissions from the start.
| fs::create_dir_all(path).map_err(FileError::Io)?; | |
| use std::os::unix::fs::DirBuilderExt; | |
| let mut builder = fs::DirBuilder::new(); | |
| builder.recursive(true); | |
| if let Some(m) = mode { | |
| builder.mode(m); | |
| } | |
| builder.create(path).map_err(FileError::Io)?; |
|
|
||
| fn run_systemctl(action: &str, name: &str) -> Result<(), SystemError> { | ||
| info!("Running systemctl {action} {name}"); | ||
| let output = Command::new("systemctl").arg(action).arg(name).output()?; |
There was a problem hiding this comment.
This implementation uses Command to shell out to systemctl, which violates the project mandate to 'Prioritize Crates over Shell-out' (AGENTS.md, line 11). Consider using a Rust crate to interact with systemd (e.g., via DBus) to improve safety and portability.
| let content = include_bytes!("../files/sysctl.boxy.conf"); | ||
| let path = Path::new("/etc/sysctl.d/99-hardening.conf"); | ||
|
|
||
| let changed = files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; |
There was a problem hiding this comment.
The /etc/sysctl.d directory may not exist on all systems (e.g., minimal containers). Since ensure_file fails if the parent directory is missing (as implemented in skillet_core), you should ensure the directory exists before attempting to write the configuration file.
| let changed = files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; | |
| files.ensure_directory(Path::new("/etc/sysctl.d"), Some(0o755), Some("root"), Some("root"))?; | |
| let changed = files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces skillet, a Rust-based idempotent host configuration tool organized as a Cargo workspace. The implementation includes core primitives for file and system management, a hardening module for sysctl and SSH configurations, and a CLI supporting both agent execution and containerized integration testing. Feedback focuses on improving the robustness and efficiency of the implementation, specifically regarding potential library mismatches when running host-built binaries in containers, adhering to the project's SHA256 hashing mandate for file idempotency checks, and ensuring that existing paths are correctly validated as directories during resource management.
| let existing_content = | ||
| fs::read(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | ||
| existing_content != content |
There was a problem hiding this comment.
Reading the entire file into memory with fs::read to check for changes is inefficient for large files and violates the project mandate in AGENTS.md (line 15), which specifies using SHA256 hashes for idempotency checks. Consider streaming the file through a hasher to minimize memory usage and adhere to the project's architectural guidelines.
| ) -> Result<bool, FileError> { | ||
| let mut changed = false; | ||
|
|
||
| if !path.exists() { |
There was a problem hiding this comment.
The ensure_directory implementation does not verify that the path is a directory if it already exists. If the path exists as a regular file, the function will proceed to check and apply metadata to that file instead of ensuring the path is a directory. This results in an incorrect system state. You should verify metadata.is_dir() when the path exists.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces 'Skillet', a Rust-based idempotent host configuration tool, including core primitives, hardening modules, and CLI support for containerized integration testing. The review identified critical compilation errors in the CLI test runner due to missing function parameters and unsafe path handling, as well as opportunities to improve robustness in system user resolution and systemctl action handling.
| setup_container(&container_name, image, &binary_path)?; | ||
|
|
||
| let result = (|| -> Result<()> { | ||
| prepare_and_run_skillet(&container_name)?; |
There was a problem hiding this comment.
The binary_path variable is defined in run_container_test but is not passed to prepare_and_run_skillet. This will cause a compilation error as binary_path is used inside that function.
| prepare_and_run_skillet(&container_name)?; | |
| prepare_and_run_skillet(&container_name, &binary_path)?; |
| Ok(()) | ||
| } | ||
|
|
||
| fn prepare_and_run_skillet(container_name: &str) -> Result<()> { |
There was a problem hiding this comment.
The function signature for prepare_and_run_skillet is missing the binary_path parameter, which is required for the implementation on line 291.
| fn prepare_and_run_skillet(container_name: &str) -> Result<()> { | |
| fn prepare_and_run_skillet(container_name: &str, binary_path: &Path) -> Result<()> { |
| let cp_bin_status = Command::new("podman") | ||
| .args([ | ||
| "cp", | ||
| binary_path.to_str().unwrap(), |
There was a problem hiding this comment.
Using unwrap() on a Path conversion to string can cause a panic if the path contains invalid UTF-8 characters. It is safer to handle this as an error, especially since this is a CLI tool where paths might be user-controlled or environment-dependent.
| binary_path.to_str().unwrap(), | |
| binary_path.to_str().ok_or_else(|| anyhow!("Binary path is not valid UTF-8"))?, |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Skillet, a modular Rust-based tool for idempotent host configuration management, featuring core primitives, hardening modules, and a containerized integration testing framework. Feedback highlights critical improvements for idempotency in user and group management, where existing UIDs and GIDs should be verified against desired states. Other suggestions include using the correct primary GID for host users, making Podman subuid mappings configurable for better portability, and removing a redundant volume mount in the test runner.
| if get_group_by_name(name).is_some() { | ||
| debug!("Group {name} already exists"); | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
The ensure_group implementation only checks if a group with the given name exists, but it ignores the gid parameter if one is provided. If a group with the same name exists but has a different GID than requested, the function will return Ok(false) (no change), which violates idempotency as the system state does not match the desired configuration.
| if get_group_by_name(name).is_some() { | |
| debug!("Group {name} already exists"); | |
| return Ok(false); | |
| } | |
| if let Some(group) = get_group_by_name(name) { | |
| if let Some(desired_gid) = gid { | |
| if group.gid() != desired_gid { | |
| return Err(SystemError::GroupCheck(format!( | |
| "Group {name} exists but has GID {}, expected {desired_gid}", | |
| group.gid() | |
| ))); | |
| } | |
| } | |
| debug!("Group {name} already exists with correct GID"); | |
| return Ok(false); | |
| } |
| if let Some(_user) = get_user_by_name(name) { | ||
| debug!("User {name} already exists"); | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
Similar to ensure_group, ensure_user only verifies the existence of the username. It should also verify that the existing user's UID and primary GID match the requested uid and gid parameters to ensure the system matches the desired state.
| if let Some(_user) = get_user_by_name(name) { | |
| debug!("User {name} already exists"); | |
| return Ok(false); | |
| } | |
| if let Some(user) = get_user_by_name(name) { | |
| if let Some(desired_uid) = uid { | |
| if user.uid() != desired_uid { | |
| return Err(SystemError::Command(format!( | |
| "User {name} exists but has UID {}, expected {desired_uid}", | |
| user.uid() | |
| ))); | |
| } | |
| } | |
| debug!("User {name} already exists with correct UID"); | |
| return Ok(false); | |
| } |
| } | ||
| }; | ||
| // For simplicity, assuming gid = uid for now | ||
| Ok(Some((uid, uid, username))) |
There was a problem hiding this comment.
This line assumes the host user's primary GID is identical to their UID. While often true, it is not guaranteed and can lead to incorrect ownership of host volumes or container processes. You should use u.primary_group_id() from the users crate to retrieve the actual GID.
| Ok(Some((uid, uid, username))) | |
| Ok(Some((uid, u.primary_group_id(), username))) |
| } | ||
|
|
||
| // 2. Ensure group exists | ||
| if let Some(gid_val) = gid { |
There was a problem hiding this comment.
This logic assumes that the primary group for a user must always have the same name as the user. This is a common pattern (User Private Groups), but it is too restrictive for cases where a user should belong to an existing group with a different name (e.g., adding a service user to a shared group). The SystemResource trait should probably be updated to allow specifying a primary group name.
| let sub_base = 100_000; | ||
| let sub_size = 65_536; |
There was a problem hiding this comment.
Hardcoding the sub_base and sub_size for UID/GID mapping limits the tool's portability. These values should ideally be configurable or detected from the host's /etc/subuid and /etc/subgid files to avoid conflicts with other users or containers on the system.
| &format!("{}:/usr/bin/skillet:ro", binary_path.display()), | ||
| image, |
There was a problem hiding this comment.
The binary is mounted into the container here as a volume, but it is also explicitly copied via podman cp at line 285. The execution at line 320 uses the copied version at /tmp/skillet, making this volume mount redundant. Removing it simplifies the container setup and avoids potential issues with host path permissions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces 'Skillet', a modular, Rust-based tool for idempotent host configuration management. It includes core primitives for file and system management, hardening modules for SSH and sysctl, and a podman-based container management system. The implementation includes a testing framework for recording and verifying configuration states. I have identified two areas for improvement: the use of magic numbers for exit codes in the system module and hardcoded configuration values in the podman library, both of which should be refactored into named constants to improve maintainability.
| const EXIT_CODE_GROUP_EXISTS: i32 = 9; | ||
| const EXIT_CODE_USER_EXISTS: i32 = 9; |
There was a problem hiding this comment.
The exit codes 9 for both groupadd and useradd are used as magic numbers. Define these as named constants or use a centralized error handling approach to improve maintainability.
| let sub_base = 100_000; | ||
| let sub_size = 65_536; |
There was a problem hiding this comment.
The sub_base and sub_size values are hardcoded magic numbers. These should be defined as constants at the top of the module or in a configuration file to improve readability and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Skillet, a modular Rust-based tool for idempotent host configuration management, featuring core primitives, a recording system for integration tests, and modules for hardening and container management. The review feedback highlights several critical improvements, including correctly handling systemd's daemon-reload command, ensuring the release build profile is respected during binary location for tests, and refining user/group mapping logic. Additionally, suggestions were provided to enhance error handling consistency across the workspace using boxed error trait objects and to adopt more secure systemd specifiers and SSH configurations.
| fn service_reload(&self, name: &str) -> Result<(), SystemError> { | ||
| self.run_systemctl("reload", name) | ||
| } |
There was a problem hiding this comment.
daemon-reload is a special systemd command that reloads the systemd manager configuration, not a specific service. It should be handled separately from regular service reloads. The current implementation incorrectly treats it as a service name, which will fail.
This can be fixed by adding a special case in service_reload to call systemctl daemon-reload or the corresponding Reload method on the D-Bus Manager interface.
fn service_reload(&self, name: &str) -> Result<(), SystemError> {
if name == "daemon-reload" {
info!("Running systemctl daemon-reload");
if let Some(conn) = &self.conn {
let proxy = SystemdManagerProxyBlocking::new(conn)?;
return proxy.reload().map_err(SystemError::DBus);
}
let output = Command::new("systemctl").arg("daemon-reload").output()?;
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
return Err(SystemError::Command(format!(
"systemctl daemon-reload failed: {stderr}"
)));
}
return Ok(());
}
self.run_systemctl("reload", name)
}| ) -> Result<()> { | ||
| build_workspace(release)?; | ||
|
|
||
| let binary_path = locate_binary(hostname)?; |
There was a problem hiding this comment.
The locate_binary function doesn't respect the release flag from the test command. It searches for binaries in both release and debug directories, which could lead to running a debug binary for a release test, or vice-versa. The release flag should be passed to locate_binary to ensure the correct binary is used.
| let binary_path = locate_binary(hostname)?; | |
| let binary_path = locate_binary(hostname, release)?; |
| let (username, uid) = match hu { | ||
| HostUser::Name(ref n) => { | ||
| if create { | ||
| system.ensure_user(n, None, None)?; | ||
| } | ||
| let u = get_user_by_name(n).ok_or_else(|| { | ||
| PodmanError::UserMapping(format!("User {n} not found on host")) | ||
| })?; | ||
| (n.clone(), u.uid()) | ||
| } | ||
| HostUser::Uid(u) => { | ||
| let u_info = get_user_by_uid(*u).ok_or_else(|| { | ||
| PodmanError::UserMapping(format!("UID {u} not found on host")) | ||
| })?; | ||
| (u_info.name().to_string_lossy().to_string(), *u) | ||
| } | ||
| }; | ||
| // For simplicity, assuming gid = uid for now | ||
| Ok(Some((uid, uid, username))) |
There was a problem hiding this comment.
The assumption that GID equals UID is not always correct. A user's primary group ID can be different from their user ID. You should use the primary_group_id() method on the User object to get the correct GID. This will make the user mapping more robust.
| let (username, uid) = match hu { | |
| HostUser::Name(ref n) => { | |
| if create { | |
| system.ensure_user(n, None, None)?; | |
| } | |
| let u = get_user_by_name(n).ok_or_else(|| { | |
| PodmanError::UserMapping(format!("User {n} not found on host")) | |
| })?; | |
| (n.clone(), u.uid()) | |
| } | |
| HostUser::Uid(u) => { | |
| let u_info = get_user_by_uid(*u).ok_or_else(|| { | |
| PodmanError::UserMapping(format!("UID {u} not found on host")) | |
| })?; | |
| (u_info.name().to_string_lossy().to_string(), *u) | |
| } | |
| }; | |
| // For simplicity, assuming gid = uid for now | |
| Ok(Some((uid, uid, username))) | |
| let (username, uid, gid) = match hu { | |
| HostUser::Name(ref n) => { | |
| if create { | |
| system.ensure_user(n, None, None)?; | |
| } | |
| let u = get_user_by_name(n).ok_or_else(|| { | |
| PodmanError::UserMapping(format!("User {n} not found on host")) | |
| })?; | |
| (n.clone(), u.uid(), u.primary_group_id()) | |
| } | |
| HostUser::Uid(u) => { | |
| let u_info = get_user_by_uid(*u).ok_or_else(|| { | |
| PodmanError::UserMapping(format!("UID {u} not found on host")) | |
| })?; | |
| (u_info.name().to_string_lossy().to_string(), *u, u_info.primary_group_id()) | |
| } | |
| }; | |
| Ok(Some((uid, gid.as_raw(), username))) |
| [Service] | ||
| Type=oneshot | ||
| # Use the generic skillet binary and pass the hostname from /etc/hostname | ||
| ExecStart=/usr/bin/sh -c "/usr/bin/skillet apply --host $(cat /etc/hostname)" |
There was a problem hiding this comment.
Using sh -c with command substitution $(cat /etc/hostname) can be fragile and less secure. If the hostname contains spaces or other special characters, it could lead to unexpected behavior. A more robust and secure approach is to use systemd's built-in specifiers. The %H specifier will be replaced by the system's hostname.
This avoids an unnecessary shell invocation and potential parsing issues.
ExecStart=/usr/bin/skillet apply --host %H
| fn locate_binary(hostname: &str) -> Result<PathBuf> { | ||
| let host_binary_name = format!("skillet-{hostname}"); | ||
| let root = find_workspace_root()?; | ||
|
|
||
| // Ordered search: | ||
| // 1. host-specific release | ||
| // 2. host-specific debug | ||
| // 3. generic skillet release | ||
| // 4. generic skillet debug | ||
|
|
||
| let binary_path = [ | ||
| root.join("target/release").join(&host_binary_name), | ||
| root.join("target/debug").join(&host_binary_name), | ||
| root.join("target/release").join("skillet"), | ||
| root.join("target/debug").join("skillet"), | ||
| ] | ||
| .into_iter() | ||
| .find(|p| p.exists()) | ||
| .ok_or_else(|| { | ||
| anyhow!("No suitable skillet binary found in target/release or target/debug") | ||
| })?; | ||
|
|
||
| info!("Using binary: {}", binary_path.display()); | ||
| fs::canonicalize(&binary_path).context("Failed to canonicalize binary path") | ||
| } |
There was a problem hiding this comment.
This function should accept a release flag to determine whether to look in the target/release or target/debug directory. This ensures that the correct build profile's binary is used for testing.
fn locate_binary(hostname: &str, release: bool) -> Result<PathBuf> {
let host_binary_name = format!("skillet-{hostname}");
let root = find_workspace_root()?;
let build_profile = if release { "release" } else { "debug" };
let target_dir = root.join("target").join(build_profile);
// Ordered search:
// 1. host-specific
// 2. generic skillet
let binary_path = [
target_dir.join(&host_binary_name),
target_dir.join("skillet"),
]
.into_iter()
.find(|p| p.exists())
.ok_or_else(|| {
anyhow!("No suitable skillet binary found in target/{build_profile}")
})?;
info!("Using binary: {}", binary_path.display());
fs::canonicalize(&binary_path).context("Failed to canonicalize binary path")
}| pub fn handle_apply<F>( | ||
| hostname: &str, | ||
| record_path: Option<PathBuf>, | ||
| apply_fn: F, | ||
| ) -> Result<(), CliCommonError> | ||
| where | ||
| F: Fn(&dyn SystemResource, &dyn FileResource) -> Result<(), String>, |
There was a problem hiding this comment.
To align with the proposed change in run_host, the apply_fn signature in handle_apply should also be updated to accept a boxed error trait object. This allows for consistent and improved error handling.
| pub fn handle_apply<F>( | |
| hostname: &str, | |
| record_path: Option<PathBuf>, | |
| apply_fn: F, | |
| ) -> Result<(), CliCommonError> | |
| where | |
| F: Fn(&dyn SystemResource, &dyn FileResource) -> Result<(), String>, | |
| pub fn handle_apply<F>( | |
| hostname: &str, | |
| record_path: Option<PathBuf>, | |
| apply_fn: F, | |
| ) -> Result<(), CliCommonError> | |
| where | |
| F: Fn(&dyn SystemResource, &dyn FileResource) -> Result<(), Box<dyn std::error::Error + Send + Sync>>, |
|
|
||
| # Disable forwarding tcp connections. | ||
| # no real advantage without denied shell access | ||
| AllowTcpForwarding yes |
There was a problem hiding this comment.
For security hardening, it is recommended to disable TCP forwarding unless it is explicitly required. Setting AllowTcpForwarding to no reduces the attack surface by preventing users from creating tunnels through the SSH server. This is consistent with other hardening settings in this file, such as disabling agent forwarding.
AllowTcpForwarding no
| run_host("beezelbot", |system, files| { | ||
| skillet_hardening::apply(system, files).map_err(|e| e.to_string()) | ||
| })?; |
There was a problem hiding this comment.
Following the proposed changes in skillet_cli_common to improve error handling, this closure can be simplified by using the ? operator and returning a boxed error.
| run_host("beezelbot", |system, files| { | |
| skillet_hardening::apply(system, files).map_err(|e| e.to_string()) | |
| })?; | |
| run_host("beezelbot", |system, files| -> Result<(), Box<dyn std::error::Error + Send + Sync>> { | |
| skillet_hardening::apply(system, files)?; | |
| Ok(()) | |
| })?; |
| run_host("clamps", |system, files| { | ||
| skillet_hardening::apply(system, files).map_err(|e| e.to_string())?; | ||
|
|
||
| // 1. Ingest secret from systemd | ||
| let cred_manager = CredentialManager::new().map_err(|e: skillet_core::credentials::CredentialError| e.to_string())?; | ||
| let secret_payload = cred_manager | ||
| .read_secret("test_secret") | ||
| .map_err(|e: skillet_core::credentials::CredentialError| e.to_string())?; | ||
|
|
||
| // 2. Provision to Podman | ||
| system | ||
| .ensure_podman_secret("pihole_web_password", &secret_payload) | ||
| .map_err(|e| e.to_string())?; | ||
|
|
||
| // 3. Apply pihole with the secret | ||
| let secrets = vec![QuadletSecret { | ||
| secret_name: "pihole_web_password".to_string(), | ||
| target: SecretTarget::File { | ||
| target_path: "/etc/pihole/webpassword".to_string(), | ||
| mode: Some("0400".to_string()), | ||
| uid: Some(40000), | ||
| gid: Some(40000), | ||
| }, | ||
| }]; | ||
|
|
||
| skillet_pihole::apply( | ||
| system, | ||
| files, | ||
| skillet_pihole::PiholeUser { | ||
| uid: 40000, | ||
| gid: 40000, | ||
| name: "pihole".to_string(), | ||
| group_name: "pihole".to_string(), | ||
| }, | ||
| secrets, | ||
| ) | ||
| .map_err(|e| e.to_string())?; | ||
|
|
||
| Ok(()) | ||
| })?; |
There was a problem hiding this comment.
Following the proposed changes in skillet_cli_common to improve error handling, this closure can be greatly simplified. By returning a boxed error, you can use the ? operator directly, making the code more readable and idiomatic.
run_host("clamps", |system, files| -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
skillet_hardening::apply(system, files)?;
// 1. Ingest secret from systemd
let cred_manager = CredentialManager::new()?;
let secret_payload = cred_manager.read_secret("test_secret")?;
// 2. Provision to Podman
system.ensure_podman_secret("pihole_web_password", &secret_payload)?;
// 3. Apply pihole with the secret
let secrets = vec![QuadletSecret {
secret_name: "pihole_web_password".to_string(),
target: SecretTarget::File {
target_path: "/etc/pihole/webpassword".to_string(),
mode: Some("0400".to_string()),
uid: Some(40000),
gid: Some(40000),
},
}];
skillet_pihole::apply(
system,
files,
skillet_pihole::PiholeUser {
uid: 40000,
gid: 40000,
name: "pihole".to_string(),
group_name: "pihole".to_string(),
},
secrets,
)?;
Ok(())
})?;| pub fn run_host<F>(hostname: &str, apply_fn: F) -> Result<(), CliCommonError> | ||
| where | ||
| F: Fn(&dyn SystemResource, &dyn FileResource) -> Result<(), String>, |
There was a problem hiding this comment.
The apply_fn closure is constrained to return Result<(), String>, which forces callers to manually convert specific errors into strings using map_err(|e| e.to_string()). This is verbose and loses structured error information.
Consider changing the closure's return type to use a boxed error trait object, like Result<(), Box<dyn std::error::Error + Send + Sync>>. This would allow idiomatic use of the ? operator within the closure for better error handling and more readable code in the host-specific crates.
| pub fn run_host<F>(hostname: &str, apply_fn: F) -> Result<(), CliCommonError> | |
| where | |
| F: Fn(&dyn SystemResource, &dyn FileResource) -> Result<(), String>, | |
| pub fn run_host<F>(hostname: &str, apply_fn: F) -> Result<(), CliCommonError> | |
| where | |
| F: Fn(&dyn SystemResource, &dyn FileResource) -> Result<(), Box<dyn std::error::Error + Send + Sync>>, |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Skillet, a modular Rust-based tool for idempotent host configuration management. It includes a core library for managing system resources and files, a hardening module for sysctl and SSH configurations, and a Quadlet-based Podman integration for deploying services like Pi-hole. The PR also establishes a recording-based integration testing framework. Feedback focuses on critical idempotency gaps where the tool checks for resource existence but ignores content or ID mismatches in secrets, users, and groups. Additionally, there is a bug in the Butane configuration regarding unsupported CLI arguments, a potential infinite loop in symlink metadata handling, and a need for newline trimming in the credential manager.
| fn ensure_podman_secret(&self, name: &str, payload: &str) -> Result<bool, SystemError> { | ||
| let inspect_output = Command::new("podman") | ||
| .args(["secret", "inspect", name]) | ||
| .output()?; | ||
|
|
||
| if inspect_output.status.success() { | ||
| debug!("Podman secret {name} already exists"); | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
The ensure_podman_secret implementation only checks for the existence of a secret with the specified name. It does not verify if the existing secret's payload matches the desired payload. Consequently, if the configuration is updated with a new secret value, the tool will skip the update, leaving the system in an inconsistent state. Since Podman secrets are immutable, the tool should ideally detect content mismatches (e.g., via labels or external tracking) and recreate the secret if necessary.
| let mut perms = fs::symlink_metadata(path) | ||
| .map_err(|e| FileError::Read(path.display().to_string(), e))? | ||
| .permissions(); | ||
| perms.set_mode(desired_mode); | ||
| fs::set_permissions(path, perms) | ||
| .map_err(|e| FileError::SetPermissions(path.display().to_string(), e))?; | ||
| } | ||
|
|
||
| if owner.is_some() || group.is_some() { | ||
| let uid = owner | ||
| .map(|u| get_user_by_name(u).ok_or_else(|| FileError::UserNotFound(u.to_string()))) | ||
| .transpose()? | ||
| .map(|u| Uid::from_raw(u.uid())); | ||
|
|
||
| let gid = group | ||
| .map(|g| { | ||
| get_group_by_name(g).ok_or_else(|| FileError::GroupNotFound(g.to_string())) | ||
| }) | ||
| .transpose()? | ||
| .map(|g| Gid::from_raw(g.gid())); | ||
|
|
||
| chown(path, uid, gid) | ||
| .map_err(|e| FileError::SetOwnership(path.display().to_string(), e.to_string()))?; |
There was a problem hiding this comment.
There is a discrepancy between how metadata is checked and how it is applied. check_metadata (line 71) uses fs::symlink_metadata, which targets the symlink itself. However, apply_metadata uses fs::set_permissions (line 110) and nix::unistd::chown (line 127), both of which follow symlinks to their targets.
If path is a symlink, the tool will check the link's ownership/mode, find a mismatch, and then apply the change to the target file. On subsequent runs, the link's metadata will still not match, leading to an infinite loop of redundant updates to the target. To fix this, use lchown and avoid calling set_permissions on symlinks (as link permissions are generally not manageable on Linux), or use fchownat with AT_SYMLINK_NOFOLLOW.
| [Service] | ||
| Type=oneshot | ||
| # Use the generic skillet binary and pass the hostname from /etc/hostname | ||
| ExecStart=/usr/bin/sh -c "/usr/bin/skillet apply --host $(cat /etc/hostname)" |
There was a problem hiding this comment.
The ExecStart command passes a --host argument which is not defined in the CLI's Apply subcommand in crates/cli/src/main.rs. This will cause the systemd service to fail with an 'unexpected argument' error. Either add the --host argument to the CLI parser or remove it from the service definition.
| let mut content = String::new(); | ||
| file.read_to_string(&mut content) | ||
| .map_err(|e| CredentialError::ReadError(name.to_string(), e))?; |
There was a problem hiding this comment.
Secret files often contain a trailing newline added by text editors. read_secret currently includes this newline in the returned string, which can break authentication if the secret is used as a password or token. It is safer to trim trailing whitespace from the secret content.
| let mut content = String::new(); | |
| file.read_to_string(&mut content) | |
| .map_err(|e| CredentialError::ReadError(name.to_string(), e))?; | |
| file.read_to_string(&mut content) | |
| .map_err(|e| CredentialError::ReadError(name.to_string(), e))?; | |
| Ok(content.trim_end().to_string()) |
| let sub_base = 100_000; | ||
| let sub_size = 65_536; |
There was a problem hiding this comment.
The sub_base and sub_size values are hardcoded to 100,000 and 65,536 respectively. While these are common defaults, they vary across distributions and host configurations (defined in /etc/subuid). Hardcoding these values makes the tool fragile on systems with non-standard user namespace configurations. Consider discovering these ranges from the host system.
| fn ensure_group(&self, name: &str, gid: Option<u32>) -> Result<bool, SystemError> { | ||
| if get_group_by_name(name).is_some() { | ||
| debug!("Group {name} already exists"); | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
The ensure_group implementation (and similarly ensure_user at line 168) only checks if a group with the given name exists. It does not verify if the existing group's GID matches the desired gid provided in the arguments. This violates idempotency as the system might remain in a state where the group exists but with an incorrect ID.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces 'Skillet', a Rust-based idempotent host configuration tool, along with its core primitives, hardening modules, and host-specific binaries. The feedback highlights critical issues in file metadata handling where using symlink_metadata instead of metadata can cause infinite loops or incorrect permission applications on symlinks. Additionally, there are concerns regarding the atomicity of file creation, hardcoded user/group ID assumptions in Podman mappings, and the asynchronous nature of DBus service calls which could lead to race conditions.
| group: Option<&str>, | ||
| ) -> Result<bool, FileError> { | ||
| let metadata = | ||
| fs::symlink_metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; |
There was a problem hiding this comment.
Using fs::symlink_metadata here causes issues when path is a symlink to a directory. On Linux, symlinks typically have a mode of 0777. If the desired mode is different (e.g., 0755), this check will always return true (changed), but the subsequent apply_metadata call uses fs::set_permissions which follows the symlink and modifies the target directory. This results in an infinite loop where every run reports a change. Using fs::metadata instead will check the target's metadata, which is consistent with how permissions and ownership are applied.
| fs::symlink_metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| fs::metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; |
| let mut perms = fs::symlink_metadata(path) | ||
| .map_err(|e| FileError::Read(path.display().to_string(), e))? | ||
| .permissions(); |
There was a problem hiding this comment.
Similar to the issue in check_metadata, using fs::symlink_metadata here to get permissions that are then passed to fs::set_permissions is problematic if path is a symlink. fs::set_permissions follows the link, so you would be applying the symlink's own permissions (usually 0777) to the target object, which is likely not what is intended. Switching to fs::metadata ensures you are modifying the permissions of the actual file or directory.
| let mut perms = fs::symlink_metadata(path) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))? | |
| .permissions(); | |
| let mut perms = fs::metadata(path) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))? | |
| .permissions(); |
| let mut temp_file = NamedTempFile::new_in(parent)?; | ||
| temp_file.write_all(content)?; | ||
| temp_file | ||
| .persist(path) | ||
| .map_err(|e| FileError::Persist(path.display().to_string(), e.error))?; | ||
| changed = true; |
There was a problem hiding this comment.
When creating a new file, NamedTempFile creates it with restrictive 0600 permissions. There is a window between persist(path) and the subsequent apply_metadata call where the file exists at the target location with these permissions. If the tool is interrupted before apply_metadata runs, the file is left in this state. It is better to set the desired permissions on the temporary file before persisting it to ensure atomicity of both content and metadata application.
| } | ||
| }; | ||
| // For simplicity, assuming gid = uid for now | ||
| Ok(Some((uid, uid, username))) |
There was a problem hiding this comment.
This assumes the host user's primary GID is equal to their UID. While common in some distributions (User Private Groups), it is not universal. If the user belongs to a different primary group (e.g., GID 100), this mapping will be incorrect and may cause permission issues for volumes mounted into the container. You should retrieve the actual GID using u.primary_group_id() in the branches above.
| let sub_base = 100_000; | ||
| let sub_size = 65_536; |
There was a problem hiding this comment.
Hardcoding sub_base to 100,000 and sub_size to 65,536 is risky. These values depend on the host's /etc/subuid and /etc/subgid configuration for the user. If the host range does not match these hardcoded values, Podman will fail to start the container due to invalid mappings. These should be dynamically discovered or made configurable.
| "start" => proxy.start_unit(&name_with_suffix, "replace"), | ||
| "stop" => proxy.stop_unit(&name_with_suffix, "replace"), | ||
| "restart" => proxy.restart_unit(&name_with_suffix, "replace"), | ||
| "reload" => proxy.reload_unit(&name_with_suffix, "replace"), |
There was a problem hiding this comment.
The DBus methods StartUnit, StopUnit, etc., return immediately after the job is queued. The current implementation does not wait for the job to finish or check if the service actually started successfully. This can lead to race conditions if subsequent configuration steps depend on the service being operational. Consider waiting for the job completion signal or checking the unit state after the call.
| if let Some(grp) = get_group_by_name(name) { | ||
| debug!("Group {name} already exists"); | ||
| if let Some(desired_gid) = gid { | ||
| if grp.gid() != desired_gid { | ||
| warn!( | ||
| "Group {name} exists but GID {} does not match desired {desired_gid}", | ||
| grp.gid() | ||
| ); | ||
| } | ||
| } | ||
| return Ok(false); |
There was a problem hiding this comment.
If the group already exists but with a different GID than requested, the tool only issues a warning and proceeds. This means the system state does not match the desired configuration, which could lead to permission issues later (e.g., in Podman volume mappings). Idempotent tools should generally error if a resource exists in a state that cannot be automatically corrected to the desired state.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Skillet, a modular Rust-based tool for idempotent host configuration management, featuring core primitives for file and system resource handling, hardening modules, and a containerized integration testing framework. The feedback identifies several critical improvements: hardcoded rootless user mapping values in the Podman crate should be dynamically discovered, and the workspace root discovery logic needs to be more robust for production builds. Security and correctness concerns were raised regarding the automatic trimming of secrets, the non-atomic application of file metadata after persistence, and the lenient handling of UID/GID mismatches during user creation. Additionally, the Podman secret inspection logic should be made more resilient to missing labels to avoid command failures.
| let sub_base = 100_000; | ||
| let sub_size = 65_536; |
There was a problem hiding this comment.
The sub_base and sub_size values for rootless user mappings are hardcoded. These values must match the host's /etc/subuid and /etc/subgid configuration for the container to function correctly. Hardcoding them makes the tool brittle across different host configurations; these should ideally be discovered from the system.
| Ok(()) | ||
| } | ||
|
|
||
| fn find_workspace_root() -> Result<PathBuf> { |
There was a problem hiding this comment.
The find_workspace_root function relies on the presence of a Cargo.toml file in the directory hierarchy. This logic is suitable for development but will fail if the binary is installed in standard system paths (e.g., /usr/bin). A more robust approach for locating resources or test data should be used for production builds.
| let mut content = String::new(); | ||
| file.read_to_string(&mut content) | ||
| .map_err(|e| CredentialError::ReadError(name.to_string(), e))?; | ||
| Ok(content.trim().to_string()) |
There was a problem hiding this comment.
Trimming the secret content automatically can be problematic for secrets where leading or trailing whitespace is significant (e.g., certain private key formats or passwords). It is safer to return the raw content and let the caller decide whether to trim it.
| Ok(content.trim().to_string()) | |
| Ok(content) |
|
|
||
| // 3. Check and apply metadata | ||
| if path.exists() && Self::check_metadata(path, mode, owner, group)? { | ||
| Self::apply_metadata(path, mode, owner, group)?; |
There was a problem hiding this comment.
Applying metadata (permissions and ownership) after the file has been persisted to the target path creates a small window where the file exists with default permissions (usually 0o600 from NamedTempFile). For improved security and atomicity, consider applying these settings to the temporary file handle (using fchmod/fchown) before calling persist.
| warn!( | ||
| "User {name} exists but UID {} does not match desired {desired_uid}", | ||
| user.uid() | ||
| ); | ||
| } |
There was a problem hiding this comment.
The ensure_user method only issues a warning if an existing user has a different UID or GID than requested. In a configuration management context, this usually indicates a discrepancy that should either be corrected or treated as an error to ensure the system reaches the desired state.
| "secret", | ||
| "inspect", | ||
| "--format", | ||
| "{{ index .Spec.Labels \"skillet.payload_hash\" }}", |
There was a problem hiding this comment.
Using index in the Podman format string will cause the command to fail if the skillet.payload_hash label is missing from an existing secret. This prevents the tool from identifying and potentially correcting secrets created outside of Skillet. Consider handling the case where the label might be absent or using a more resilient template.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces 'Skillet', a modular, Rust-based tool for idempotent host configuration management, featuring a core library for system primitives, hardening modules, and containerized integration testing. The review identified several areas for improvement: the secret reading logic should be more precise to avoid stripping legitimate whitespace, the recording file write operation should be made atomic using temporary files, the CLI entry point requires refactoring to remove hardcoded host-specific logic, systemctl commands should be executed with non-blocking flags to prevent potential hangs, and UID/GID mapping constants should be made configurable or dynamically discovered.
| let mut content = String::new(); | ||
| file.read_to_string(&mut content) | ||
| .map_err(|e| CredentialError::ReadError(name.to_string(), e))?; | ||
| Ok(content.trim().to_string()) |
There was a problem hiding this comment.
Using trim() on secret content is potentially destructive as it removes all leading and trailing whitespace, which might be a legitimate part of the secret (e.g., in binary keys or specific formats). It is safer to only remove trailing newline characters, which are often added by text editors or echo commands.
| Ok(content.trim().to_string()) | |
| Ok(content.trim_end_matches(['\n', '\r']).to_string()) |
| if let Some(parent) = path.parent() { | ||
| fs::create_dir_all(parent)?; | ||
| } | ||
| fs::write(&path, yaml)?; |
There was a problem hiding this comment.
Writing the recording file using fs::write is not atomic. If the process is interrupted during the write operation, the recording file will be left in a corrupted state. Since the project already utilizes tempfile for atomic operations in the core crate, it should be applied here as well by writing to a temporary file and then persisting it to the final destination.
| match hostname { | ||
| "beezelbot" => { | ||
| skillet_hardening::apply(system, files).map_err(|e| e.to_string())?; | ||
| } | ||
| "clamps" => { | ||
| skillet_hardening::apply(system, files).map_err(|e| e.to_string())?; | ||
|
|
||
| // 1. Ingest secret from systemd | ||
| let cred_manager = CredentialManager::new().map_err(|e| e.to_string())?; | ||
| let secret_payload = cred_manager | ||
| .read_secret("test_secret") | ||
| .map_err(|e| e.to_string())?; | ||
|
|
||
| // 2. Provision to Podman | ||
| system | ||
| .ensure_podman_secret("pihole_web_password", &secret_payload) | ||
| .map_err(|e| e.to_string())?; | ||
|
|
||
| // 3. Apply pihole with the secret | ||
| let secrets = vec![QuadletSecret { | ||
| secret_name: "pihole_web_password".to_string(), | ||
| target: SecretTarget::File { | ||
| target_path: "/etc/pihole/webpassword".to_string(), | ||
| mode: Some("0400".to_string()), | ||
| uid: Some(40000), | ||
| gid: Some(40000), | ||
| }, | ||
| }]; | ||
|
|
||
| skillet_pihole::apply( | ||
| system, | ||
| files, | ||
| skillet_pihole::PiholeUser { | ||
| uid: 40000, | ||
| gid: 40000, | ||
| name: "pihole".to_string(), | ||
| group_name: "pihole".to_string(), | ||
| }, | ||
| secrets, | ||
| ) | ||
| .map_err(|e: skillet_pihole::PiholeError| e.to_string())?; | ||
| } | ||
| _ => { | ||
| // Default fallback: just hardening | ||
| skillet_hardening::apply(system, files).map_err(|e| e.to_string())?; | ||
| } | ||
| } |
There was a problem hiding this comment.
The CLI entry point contains hardcoded configuration logic for specific hostnames (beezelbot, clamps). This approach violates the principle of modularity and will cause the skillet binary to grow indefinitely as more hosts are added. Host-specific logic should be encapsulated within the respective host crates (e.g., skillet-beezelbot), or a dynamic discovery/plugin mechanism should be implemented to load configurations based on the hostname.
| } | ||
|
|
||
| info!("Running systemctl {action} {name_with_suffix} via CLI"); | ||
| let output = Command::new("systemctl") |
There was a problem hiding this comment.
Calling systemctl synchronously without a timeout or the --no-block flag can cause the configuration tool to hang indefinitely if a service fails to start or enters an inconsistent state. Consider using --no-block if immediate completion isn't required, or wrap the command execution with a timeout.
| let output = Command::new("systemctl") | |
| let output = Command::new("systemctl") | |
| .arg(action) | |
| .arg("--no-block") | |
| .arg(&name_with_suffix) | |
| .output()?; |
| let sub_base = 100_000; | ||
| let sub_size = 65_536; |
There was a problem hiding this comment.
The sub_base (100,000) and sub_size (65,536) for UID/GID mapping are hardcoded. This assumes a specific system configuration for subuids/subgids which may not be present on all hosts or may conflict with other users. These values should ideally be discovered from /etc/subuid and /etc/subgid for the target user or made configurable.
- Refactor CLI entry point to move host-specific logic to 'host_applies' module - Implement atomic recording file writes using temporary files and rename - Improve secret reading logic to preserve whitespace except for trailing newline - Fix 'daemon-reload' handling and ensure systemctl commands use non-blocking flags - Implement dynamic discovery of subuid/subgid ranges for Podman user mapping - Enable dynamic UID/GID assignment for Pi-hole user and group management
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces 'Skillet', a Rust-based idempotent host configuration tool. The changes include the core library, hardening modules, and CLI utilities for managing host configurations and containerized integration tests. I have provided feedback on several areas, including security improvements for command execution, workspace configuration, error handling, and code robustness. Specifically, I identified a command injection vulnerability in the Butane configuration, a missing workspace member in Cargo.toml, and hardcoded security risks in the Clamps host configuration. Other suggestions focus on improving error propagation, simplifying path resolution, and refining system resource management.
| [Service] | ||
| Type=oneshot | ||
| # Use the generic skillet binary and pass the hostname from /etc/hostname | ||
| ExecStart=/usr/bin/sh -c "/usr/bin/skillet apply --host $(cat /etc/hostname)" |
There was a problem hiding this comment.
The current ExecStart command uses sh -c with command substitution $(...), which creates a command injection vulnerability. If an attacker can write to /etc/hostname, they can execute arbitrary commands as root.
To fix this, I recommend avoiding the shell and making the skillet binary handle reading the hostname from a file. This requires changes in two places:
- In
ublue/skillet/crates/cli/src/main.rs, modify theApplycommand to accept a--host-fileargument and use it to determine the hostname. - In this file, update
ExecStartto use the new argument.
Here's the suggested change for this file:
ExecStart=/usr/bin/skillet apply --host-file /etc/hostname
| members = [ | ||
| "crates/core", | ||
| "crates/hardening", | ||
| "crates/podman", | ||
| "crates/cli", | ||
| "crates/hosts/beezelbot", | ||
| "crates/hosts/clamps", | ||
| "crates/cli-common", | ||
| ] |
There was a problem hiding this comment.
The skillet_pihole crate is used as a workspace dependency (line 16) but is not included in the members list. This can cause issues with workspace-wide cargo commands like cargo check --workspace.
Please add it to the list of members.
| members = [ | |
| "crates/core", | |
| "crates/hardening", | |
| "crates/podman", | |
| "crates/cli", | |
| "crates/hosts/beezelbot", | |
| "crates/hosts/clamps", | |
| "crates/cli-common", | |
| ] | |
| members = [ | |
| "crates/core", | |
| "crates/hardening", | |
| "crates/pihole", | |
| "crates/podman", | |
| "crates/cli", | |
| "crates/hosts/beezelbot", | |
| "crates/hosts/clamps", | |
| "crates/cli-common", | |
| ] |
| uid: Some(0), | ||
| gid: Some(0), |
There was a problem hiding this comment.
Using hardcoded UID/GID 0 for the PiholeUser is a security risk as it will attempt to run container processes as root. It's also inconsistent with the logic in host_applies.rs which dynamically looks up the pihole user.
To improve security and consistency, you should use None to let the system dynamically assign a UID/GID.
| uid: Some(0), | |
| gid: Some(0), | |
| uid: None, | |
| gid: None, |
| ) -> Result<(), String> { | ||
| match hostname { | ||
| "beezelbot" => { | ||
| skillet_hardening::apply(system, files).map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
Throughout this function, errors are converted to strings using map_err(|e| e.to_string()). This practice loses valuable type information and context, making debugging more difficult. It would be better to define a custom error enum for this function that can wrap the various error types from skillet_hardening, CredentialError, etc., to preserve the original error.
| fn find_workspace_root() -> Result<PathBuf> { | ||
| let mut current = std::env::current_exe()? | ||
| .parent() | ||
| .ok_or_else(|| anyhow!("Failed to get executable directory"))? | ||
| .to_path_buf(); | ||
|
|
||
| loop { | ||
| if current.join("Cargo.toml").exists() { | ||
| return Ok(current); | ||
| } | ||
| if !current.pop() { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Fallback to CWD if not found relative to exe | ||
| let cwd = std::env::current_dir()?; | ||
| if cwd.join("Cargo.toml").exists() { | ||
| return Ok(cwd); | ||
| } | ||
|
|
||
| Err(anyhow!( | ||
| "Failed to locate workspace root (looking for Cargo.toml)" | ||
| )) | ||
| } |
There was a problem hiding this comment.
The find_workspace_root function is implemented manually by traversing parent directories. This can be simplified and made more robust by using the cargo_metadata crate.
Add cargo_metadata to your dependencies and you can replace this function with a much simpler one:
fn find_workspace_root() -> Result<PathBuf> {
let metadata = cargo_metadata::MetadataCommand::new().exec()?;
Ok(metadata.workspace_root.into_std_path_buf())
}| if content.ends_with('\n') { | ||
| content.pop(); | ||
| if content.ends_with('\r') { | ||
| content.pop(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current logic for removing the trailing newline is a bit complex and only handles \n and \r\n. A simpler and more robust approach is to use trim_end() to remove all trailing whitespace characters, which can be done in-place without a new allocation.
let new_len = content.trim_end().len();
content.truncate(new_len);| if name == "daemon-reload" && action == "reload" { | ||
| return self.daemon_reload(); | ||
| } |
There was a problem hiding this comment.
The run_systemctl function has a special case to handle daemon-reload. This is confusing because daemon-reload is a global systemd action, not an action on a specific unit. This is highlighted by its misuse in skillet_podman where it's called via service_reload("daemon-reload").
To improve clarity and correctness, I suggest creating a dedicated daemon_reload() method on the SystemResource trait and calling that directly.
| fn discover_subid_range(path: &str) -> Option<(u32, u32)> { | ||
| use std::fs::File; | ||
| use std::io::{BufRead, BufReader}; | ||
|
|
||
| let user = users::get_current_username()?.to_string_lossy().to_string(); | ||
| let file = File::open(path).ok()?; | ||
| let reader = BufReader::new(file); | ||
|
|
||
| for line in reader.lines().map_while(Result::ok) { | ||
| let parts: Vec<&str> = line.split(':').collect(); | ||
| if parts.len() == 3 && parts[0] == user { | ||
| let start = parts[1].parse().ok()?; | ||
| let size = parts[2].parse().ok()?; | ||
| return Some((start, size)); | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
The discover_subid_range function manually parses /etc/subuid and /etc/subgid. This approach is fragile and has a couple of issues:
- It only checks for the current user (
users::get_current_username()), which may not be the correct user ifskilletis run as root to configure a container for a different user. - Manual parsing can fail on different formats or edge cases.
Consider passing the target host user to this function to ensure you're looking up the correct subuid/subgid range.
…eview - Refactor SystemResource to include a dedicated 'daemon_reload' method - Implement atomic metadata application during file creation to eliminate security race conditions - Enforce strict UID/GID matching in user/group management (error on mismatch) - Improve recording file atomicity in cli-common - Secure systemd unit command execution using --host-file and --no-block - Dynamic discovery of subuid/subgid ranges for specific host users in Podman - Use cargo_metadata for robust workspace root discovery - Refine secret trimming and error handling in host application logic
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces 'Skillet', a Rust-based idempotent host configuration tool, including core primitives, hardening modules, and CLI support. The review identified two areas for improvement: the need to handle potential errors during container cleanup in the CLI and the need to ensure robust error handling when writing to stdin for Podman secret creation.
| let _ = Command::new("podman") | ||
| .args(["rm", "-f", &container_name]) | ||
| .output(); |
There was a problem hiding this comment.
Using Command::new("podman").args(["rm", "-f", &container_name]).output() without checking the result or handling potential errors can lead to silent failures in test cleanup. Consider checking the status of the command.
| if let Some(mut stdin) = child.stdin.take() { | ||
| use std::io::Write as _; | ||
| stdin.write_all(payload.as_bytes())?; | ||
| } |
There was a problem hiding this comment.
The stdin.write_all(payload.as_bytes())? call is inside a block that ignores potential errors if the stdin pipe is closed unexpectedly. Ensure proper error handling for the write operation.
1 participant
This PR introduces Skillet, a Rust-based idempotent host configuration tool with containerized integration tests. (Re-opened with target/ directory removed)