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() |
ublue/skillet/crates/cli/src/main.rs
Outdated
| 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)?; |
ublue/skillet/crates/cli/src/main.rs
Outdated
| .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")?, |
ublue/skillet/crates/cli/src/main.rs
Outdated
| } 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.
ublue/skillet/crates/cli/src/main.rs
Outdated
| .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"))?, |
ublue/skillet/crates/cli/src/main.rs
Outdated
| } 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.
| @@ -0,0 +1,70 @@ | |||
| use super::*; | |||
There was a problem hiding this comment.
The existing tests cover file content changes and deletion well. However, there are no tests for ensuring file metadata (mode, owner, group) is correctly applied and idempotent. Since this is a core part of the ensure_file functionality, I recommend adding tests for these scenarios. For example, a test could:
- Create a file with default metadata.
- Call
ensure_filewith specificmode,owner, andgroup. - Assert that the metadata was changed correctly.
- Call
ensure_fileagain with the same parameters and assert that no change was reported (changed == false).
| 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.
ublue/skillet/Cargo.lock
Outdated
|
|
||
| [[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.
ublue/skillet/crates/cli/src/main.rs
Outdated
| 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.
ublue/skillet/crates/cli/src/main.rs
Outdated
| // 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 ops = recorder_system.get_ops(); | ||
| let yaml = serde_yml::to_string(&ops)?; | ||
| fs::write(&path, yaml)?; |
There was a problem hiding this comment.
fs::write will fail if the parent directory of the recording path does not exist. It is better to ensure the directory exists before attempting to write the file.
| fs::write(&path, yaml)?; | |
| if let Some(parent) = path.parent() { | |
| fs::create_dir_all(parent)?; | |
| } | |
| fs::write(&path, yaml)?; |
ublue/skillet/crates/cli/src/main.rs
Outdated
| 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.
| "--name", | ||
| container_name, | ||
| "-v", | ||
| &format!("{}:/usr/bin/skillet:ro", binary_path.display()), |
There was a problem hiding this comment.
Mounting the host-built binary directly into the container may lead to execution failures due to shared library mismatches (e.g., different glibc versions between the host and the fedora:latest image). For more robust integration testing, consider building a statically linked binary (e.g., using the MUSL target) or performing the build within a container environment that matches the test target.
| 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.
…ory type verification
|
/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 containing core primitives for file and system management, a hardening module for SSH and sysctl, and a CLI with an integrated containerized test runner. The review feedback identifies several robustness concerns in the core library, specifically regarding the destructive replacement of symlinks, the lack of validation for non-regular files, and logic errors in the systemd DBus fallback. Improvements are also suggested for the binary discovery logic and handling of group creation race conditions.
| let mut temp_file = NamedTempFile::new_in(parent)?; | ||
| temp_file.write_all(content)?; | ||
| temp_file | ||
| .persist(path) |
There was a problem hiding this comment.
The use of temp_file.persist(path) (which uses std::fs::rename) will atomically replace a symlink at path with a regular file. In many system configuration scenarios, if a file is a symlink (e.g., /etc/resolv.conf managed by a network manager), replacing it with a static file is destructive and breaks system expectations. Consider checking if the path is a symlink and either following it to the target or returning an error if symlink replacement is not intended.
ublue/skillet/crates/cli/src/main.rs
Outdated
| .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 | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The logic for locating binaries is brittle as it hardcodes target/release and target/debug relative to the current working directory. This will fail if the tool is executed from outside the workspace root or if a custom CARGO_TARGET_DIR is used. Additionally, it does not account for cross-compilation target subdirectories (e.g., target/x86_64-unknown-linux-gnu/debug). Consider using std::env::current_exe to find the current binary's directory or providing a way to specify the search path.
| .ok_or_else(|| FileError::InvalidPath(path.display().to_string()))?; | ||
|
|
||
| if !parent.exists() { | ||
| return Err(FileError::ParentMissing(path.display().to_string())); |
There was a problem hiding this comment.
The check !parent.exists() is insufficient because it doesn't verify that the parent is actually a directory. If the parent path exists but is a file, subsequent operations like NamedTempFile::new_in(parent) will fail with an error that might be harder to debug. It is safer to use parent.is_dir().
if !parent.is_dir() {| let mut changed = false; | ||
|
|
||
| // 2. Check content | ||
| let content_changed = if path.exists() { |
There was a problem hiding this comment.
In ensure_file, there is no explicit check to ensure that the existing path is a regular file. If path exists but is a directory, fs::File::open(path) might succeed on some Unix systems but read() operations (via std::io::copy) will fail with EISDIR. If path is a character device or FIFO, the hashing process could hang indefinitely. You should verify metadata.is_file() before attempting to read the content.
| let mut builder = fs::DirBuilder::new(); | ||
| builder.recursive(true); | ||
| if let Some(m) = mode { | ||
| builder.mode(m); | ||
| } | ||
| builder.create(path).map_err(FileError::Io)?; |
There was a problem hiding this comment.
When using fs::DirBuilder::new().recursive(true), the mode() setting only applies to the final directory in the path. Any parent directories created during the process will use default system permissions (respecting umask). If specific permissions are required for the entire directory tree, they must be managed explicitly for each level.
|
|
||
| if let Some(conn) = &self.conn { | ||
| info!("Running systemctl {action} {name_with_suffix} via DBus"); | ||
| let proxy = SystemdManagerProxyBlocking::new(conn)?; |
There was a problem hiding this comment.
If SystemdManagerProxyBlocking::new(conn) fails (e.g., if the systemd manager interface is not available on the bus despite a connection existing), the function returns an error immediately. This bypasses the CLI fallback logic implemented below. The proxy creation should be part of the block that falls back to the CLI on failure.
| let output = Command::new("groupadd").arg(name).output()?; | ||
|
|
||
| if !output.status.success() { | ||
| let stderr = String::from_utf8_lossy(&output.stderr); | ||
| return Err(SystemError::Command(format!("groupadd failed: {stderr}"))); | ||
| } |
There was a problem hiding this comment.
There is a potential TOCTOU (Time-of-Check to Time-of-Use) race condition between checking for the group's existence and calling groupadd. While unlikely to be an issue in sequential execution, it is more robust to handle the specific exit code of groupadd (typically 9 if the group already exists) rather than treating all non-zero exits as fatal errors.
…e group creation race
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the skillet project, a Rust-based idempotent host configuration tool, establishing a workspace with core primitives for file and system management, a hardening module for SSH and sysctl, and a CLI for configuration application and containerized integration testing. Feedback points out a compilation error in the file hashing implementation and suggests improvements for systemd unit suffix detection, binary search prioritization, directory tracking in test mocks, and error handling during container cleanup.
| std::io::copy(&mut reader, &mut hasher) | ||
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; |
There was a problem hiding this comment.
The sha2::Sha256 struct does not implement std::io::Write, which means std::io::copy will fail to compile here. You should instead read the file in a loop and call hasher.update() on the chunks to calculate the hash.
| std::io::copy(&mut reader, &mut hasher) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| let mut buffer = [0u8; 8192]; | |
| loop { | |
| let n = std::io::Read::read(&mut reader, &mut buffer) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| if n == 0 { break; } | |
| hasher.update(&buffer[..n]); | |
| } |
| let name_with_suffix = if name.contains('.') { | ||
| name.to_string() | ||
| } else { | ||
| format!("{name}.service") | ||
| }; |
There was a problem hiding this comment.
The logic name.contains('.') is insufficient for determining if a systemd unit name already includes a suffix. Service names can validly contain dots (e.g., my.app.service), and this check would incorrectly assume my.app is a complete unit name. It is safer to check for known systemd unit suffixes.
let name_with_suffix = if [
".service", ".socket", ".device", ".mount", ".automount", ".swap", ".target", ".path",
".timer", ".slice", ".scope",
]
.iter()
.any(|suffix| name.ends_with(suffix))
{
name.to_string()
} else {
format!("{name}.service")
};
ublue/skillet/crates/cli/src/main.rs
Outdated
| fn locate_binary(hostname: &str) -> Result<PathBuf> { | ||
| let host_binary_name = format!("skillet-{hostname}"); | ||
| let binary_path = ["target/release", "target/debug"] | ||
| .iter() |
There was a problem hiding this comment.
The search order for binaries prioritizes target/release over target/debug. During development, this can lead to running stale release binaries instead of the freshly built debug version. Consider reversing the priority or checking the modification times of the binaries to pick the most recent one.
| fn ensure_directory( | ||
| &self, | ||
| _path: &Path, | ||
| _mode: Option<u32>, | ||
| _owner: Option<&str>, | ||
| _group: Option<&str>, | ||
| ) -> Result<bool, FileError> { | ||
| // For mock, we don't really track directories separately for now, | ||
| // but we could if needed. Just return Ok(false) as if it exists. | ||
| Ok(false) | ||
| } |
There was a problem hiding this comment.
ensure_directory in MockFiles always returns Ok(false), which incorrectly indicates that no directory was ever created. This prevents unit tests from verifying the idempotency or correctness of directory creation logic. It should track created directories and return true when a new one is added.
ublue/skillet/crates/cli/src/main.rs
Outdated
| let _ = Command::new("podman") | ||
| .args(["kill", &container_name]) | ||
| .output(); |
There was a problem hiding this comment.
Errors from podman kill are silently ignored. If the command fails, the test container may leak, consuming system resources. It is better to log a warning or error if the container cannot be stopped.
if let Err(e) = Command::new("podman")
.args(["kill", &container_name])
.status()
{
error!("Failed to kill container {}: {}", container_name, e);
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces skillet, a Rust-based idempotent host configuration tool, establishing a workspace with core primitives for file and system management, security hardening modules, and a CLI for agent execution and integration testing. The architectural mandates emphasize safety, idempotency, and a clear separation between library and binary logic. Feedback identifies a critical compilation error in the file hashing implementation where Sha256 is used with std::io::copy without implementing the necessary trait, and suggests replacing a magic number in the system group management logic to improve code maintainability.
| let file = fs::File::open(path) | ||
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | ||
| let mut reader = std::io::BufReader::new(file); | ||
| let mut hasher = Sha256::new(); | ||
| std::io::copy(&mut reader, &mut hasher) |
There was a problem hiding this comment.
The use of std::io::copy with Sha256 will likely fail to compile because Sha256 (from the sha2 crate) does not implement the std::io::Write trait. Additionally, using BufReader here is redundant as std::io::copy already uses an internal buffer. It is more efficient and reliable to read the file in chunks and update the hasher manually.
| let file = fs::File::open(path) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| let mut reader = std::io::BufReader::new(file); | |
| let mut hasher = Sha256::new(); | |
| std::io::copy(&mut reader, &mut hasher) | |
| let mut file = fs::File::open(path) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| let mut hasher = Sha256::new(); | |
| let mut buffer = [0u8; 8192]; | |
| loop { | |
| let n = std::io::Read::read(&mut file, &mut buffer) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| if n == 0 { | |
| break; | |
| } | |
| hasher.update(&buffer[..n]); | |
| } |
|
|
||
| if !output.status.success() { | ||
| // Check if group was created by another process in the meantime (exit code 9 for groupadd) | ||
| if output.status.code() == Some(9) { |
There was a problem hiding this comment.
The exit code 9 is a magic number specific to the groupadd utility (indicating that the group already exists). It should be documented or replaced with a named constant to improve maintainability.
|
/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 including core primitives for file and system management, a hardening library for sysctl and SSH configurations, and a CLI that supports both agent execution and containerized integration testing using Podman. Feedback highlights a potential bug where I/O errors are silently ignored during file hashing in the core library, which could compromise idempotency, and suggests improving the robustness of the binary discovery logic in the CLI.
| while let Ok(n) = reader.read(&mut buffer) { | ||
| if n == 0 { | ||
| break; | ||
| } | ||
| hasher.update(&buffer[..n]); | ||
| } |
There was a problem hiding this comment.
The loop uses while let Ok(n) = reader.read(&mut buffer), which silently terminates if an I/O error occurs during reading. This results in an incomplete hash calculation, potentially leading to incorrect idempotency checks or unnecessary file overwrites. It should explicitly handle and propagate the error.
| while let Ok(n) = reader.read(&mut buffer) { | |
| if n == 0 { | |
| break; | |
| } | |
| hasher.update(&buffer[..n]); | |
| } | |
| loop { | |
| let n = reader.read(&mut buffer).map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| if n == 0 { | |
| break; | |
| } | |
| hasher.update(&buffer[..n]); | |
| } |
ublue/skillet/crates/cli/src/main.rs
Outdated
| 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"))?; |
There was a problem hiding this comment.
The locate_binary function relies on hardcoded relative paths (target/release, target/debug) from the current working directory. This makes the tool brittle if executed from a directory other than the project root. Consider using a more robust method to locate the workspace or the binary, such as checking for a Cargo.toml or using environment variables.
…nd improve binary discovery
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request establishes the foundation for skillet, a Rust-based idempotent host configuration tool. It introduces a workspace structure containing core primitives for file and system management, a hardening module for sysctl and SSH configurations, and a CLI that supports both agent-based application and containerized integration testing using Podman. The review feedback highlights opportunities to improve the efficiency of file metadata operations by reducing redundant system calls, simplifying hashing logic with more concise API usage, and enhancing the robustness of the CLI's binary discovery mechanism.
| fn check_metadata( | ||
| path: &Path, | ||
| mode: Option<u32>, | ||
| owner: Option<&str>, | ||
| group: Option<&str>, | ||
| ) -> Result<bool, FileError> { | ||
| let metadata = | ||
| fs::metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | ||
| let mut changed = false; | ||
|
|
||
| if let Some(desired_mode) = mode { | ||
| if (metadata.permissions().mode() & 0o7777) != desired_mode { | ||
| changed = true; | ||
| } | ||
| } | ||
|
|
||
| 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.uid() != user.uid() { | ||
| changed = true; | ||
| } | ||
| } | ||
|
|
||
| if let Some(desired_group) = group { | ||
| let grp = get_group_by_name(desired_group) | ||
| .ok_or_else(|| FileError::GroupNotFound(desired_group.to_string()))?; | ||
| if metadata.gid() != grp.gid() { | ||
| changed = true; | ||
| } | ||
| } | ||
|
|
||
| Ok(changed) | ||
| } | ||
|
|
||
| fn apply_metadata( | ||
| path: &Path, | ||
| mode: Option<u32>, | ||
| owner: Option<&str>, | ||
| group: Option<&str>, | ||
| ) -> Result<(), FileError> { | ||
| if let Some(desired_mode) = mode { | ||
| let mut perms = fs::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()))?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The check_metadata and apply_metadata functions perform redundant system calls and metadata lookups. Both functions independently resolve user and group names to IDs and fetch file metadata. This is inefficient as these operations (especially name resolution via NSS/LDAP) can be slow. Consider refactoring these into a single function that performs the check and apply logic in one pass, or at least resolve the Uid and Gid once and reuse them.
| let mut new_hasher = Sha256::new(); | ||
| new_hasher.update(content); | ||
| let new_hash = new_hasher.finalize(); |
There was a problem hiding this comment.
The hash of the provided content can be calculated more concisely using Sha256::digest(content) instead of manually managing a hasher instance.
| let mut new_hasher = Sha256::new(); | |
| new_hasher.update(content); | |
| let new_hash = new_hasher.finalize(); | |
| let new_hash = Sha256::digest(content); |
| fn locate_binary(hostname: &str) -> Result<PathBuf> { | ||
| let host_binary_name = format!("skillet-{hostname}"); | ||
| let dirs = ["target/release", "target/debug"]; | ||
|
|
||
| // First, try to find host-specific binary in release, then debug | ||
| let binary_path = dirs | ||
| .iter() | ||
| .find_map(|dir| { | ||
| let p = PathBuf::from(dir).join(&host_binary_name); | ||
| if p.exists() { | ||
| Some(p) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| // If not found, try generic skillet binary in release, then debug | ||
| .or_else(|| { | ||
| dirs.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"))?; | ||
|
|
||
| info!("Using binary: {}", binary_path.display()); | ||
| fs::canonicalize(&binary_path).context("Failed to canonicalize binary path") | ||
| } |
There was a problem hiding this comment.
The locate_binary function relies on hardcoded relative paths to the target/ directory. This makes the tool fragile and dependent on being executed from the workspace root. Consider using a more robust method to locate binaries, such as checking the directory of the current executable or using a configuration-defined path.
…ne binary discovery search
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request establishes the skillet project, a Rust-based tool for idempotent host configuration, organized into a workspace containing core primitives, hardening modules, and a CLI. The codebase implements idempotent file and system resource management, a recording system for integration tests, and initial hardening for SSH and sysctl. Feedback identifies opportunities to update the architectural documentation, support release builds in the test runner, simplify file hashing using std::io::copy, and refactor redundant directory creation logic in the hardening module.
| skillet/ | ||
| ├── Cargo.toml # Workspace configuration | ||
| ├── AGENTS.md # This file (Project mandates) | ||
| └── crates/ | ||
| ├── core/ # skillet_core: Low-level idempotent primitives | ||
| │ ├── src/ | ||
| │ │ ├── lib.rs | ||
| │ │ ├── files.rs # File management (Traits + Impl) | ||
| │ │ ├── files/ | ||
| │ │ │ └── tests.rs # Unit tests for files | ||
| │ │ ├── system.rs # User/Group management | ||
| │ │ └── system/ | ||
| │ │ └── tests.rs # Unit tests for system | ||
| │ └── tests/ # Integration tests | ||
| ├── hardening/ # skillet_hardening: Configuration logic (modules) | ||
| │ ├── src/ | ||
| │ │ ├── lib.rs # Hardening logic using core primitives | ||
| │ │ └── tests.rs # Unit tests for hardening logic | ||
| │ └── tests/ | ||
| └── cli/ # skillet: The binary executable | ||
| └── src/ | ||
| └── main.rs # CLI entry point (uses anyhow, clap) | ||
| ``` |
There was a problem hiding this comment.
The project structure diagram seems to be outdated and doesn't reflect the actual structure of the workspace. It's missing the cli-common and hosts/beezelbot crates. It would be beneficial to update the diagram to accurately represent all the crates in the workspace for better clarity and maintainability.
For example, the structure could be updated to include:
...
└── crates/
├── cli/ # skillet: The binary executable
├── cli-common/ # Shared CLI logic
├── core/ # skillet_core: Low-level idempotent primitives
├── hardening/ # skillet_hardening: Configuration logic (modules)
└── hosts/
└── beezelbot/ # Host-specific binary for beezelbot
| enum TestCommands { | ||
| Record { | ||
| hostname: String, | ||
| /// Container image to use | ||
| #[arg(long, default_value = "fedora:latest")] | ||
| image: String, | ||
| }, | ||
| Run { | ||
| hostname: String, | ||
| #[arg(long, default_value = "fedora:latest")] | ||
| image: String, | ||
| }, |
There was a problem hiding this comment.
The test runner always builds the project in debug mode because cargo build is called without --release. It would be beneficial to add a --release flag to the test record and test run commands to allow testing release builds. This would provide more flexibility and allow for performance testing or validation of optimized builds.
| let file = fs::File::open(path) | ||
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | ||
| let mut reader = std::io::BufReader::new(file); | ||
| let mut hasher = Sha256::new(); | ||
|
|
||
| let mut buffer = [0; 8192]; | ||
| loop { | ||
| let n = reader | ||
| .read(&mut buffer) | ||
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | ||
| if n == 0 { | ||
| break; | ||
| } | ||
| hasher.update(&buffer[..n]); | ||
| } |
There was a problem hiding this comment.
The manual loop for hashing the file content can be simplified by using std::io::copy. This function is more idiomatic and concise for streaming data from a reader to a writer, and Sha256 implements Write.
| let file = fs::File::open(path) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| let mut reader = std::io::BufReader::new(file); | |
| let mut hasher = Sha256::new(); | |
| let mut buffer = [0; 8192]; | |
| loop { | |
| let n = reader | |
| .read(&mut buffer) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| if n == 0 { | |
| break; | |
| } | |
| hasher.update(&buffer[..n]); | |
| } | |
| let mut file = fs::File::open(path) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| let mut hasher = Sha256::new(); | |
| io::copy(&mut file, &mut hasher) | |
| .map_err(|e| FileError::Read(path.display().to_string(), e))?; |
| pub fn apply<S, F>(system: &S, files: &F) -> Result<(), HardeningError> | ||
| where | ||
| S: SystemResource + ?Sized, | ||
| F: FileResource + ?Sized, | ||
| { | ||
| info!("Applying hardening..."); | ||
|
|
||
| // 1. Sysctl hardening | ||
| apply_sysctl_hardening(system, files)?; | ||
|
|
||
| // 2. Include 'os-hardening' | ||
| apply_os_hardening(system); | ||
|
|
||
| // 3. Include 'ssh-hardening::server' | ||
| apply_ssh_hardening_server(system, files)?; | ||
|
|
||
| // 4. Include 'ssh-hardening::client' | ||
| apply_ssh_hardening_client(system, files)?; | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
There's some repetition in ensuring the /etc/ssh directory exists in both apply_ssh_hardening_server and apply_ssh_hardening_client. You could consolidate this by ensuring the directory exists once before calling both functions. This would make the code a bit cleaner and avoid redundant checks.
Consider creating a wrapper function for SSH hardening that first ensures the directory exists, and then calls the server and client hardening functions.
1 participant
This PR introduces Skillet, a Rust-based idempotent host configuration tool with containerized integration tests. (Re-opened with target/ directory removed)