From 8b1d017463ac7ebcc4076326952c6ab37938be50 Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 21 Mar 2026 15:05:23 +0100 Subject: [PATCH 01/19] feat: implement skillet configuration tool with containerized integration 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. --- ublue/skillet/.gitignore | 1 + ublue/skillet/AGENTS.md | 61 ++ ublue/skillet/Cargo.lock | 862 ++++++++++++++++++ ublue/skillet/Cargo.toml | 23 + ublue/skillet/crates/cli/Cargo.toml | 16 + ublue/skillet/crates/cli/src/main.rs | 263 ++++++ ublue/skillet/crates/core/Cargo.toml | 17 + ublue/skillet/crates/core/src/files.rs | 217 +++++ ublue/skillet/crates/core/src/files/tests.rs | 70 ++ ublue/skillet/crates/core/src/lib.rs | 4 + ublue/skillet/crates/core/src/recorder.rs | 77 ++ ublue/skillet/crates/core/src/resource_op.rs | 18 + ublue/skillet/crates/core/src/system.rs | 63 ++ ublue/skillet/crates/core/src/system/tests.rs | 39 + ublue/skillet/crates/hardening/Cargo.toml | 12 + .../crates/hardening/files/sysctl.boxy.conf | 40 + ublue/skillet/crates/hardening/src/lib.rs | 68 ++ ublue/skillet/crates/hardening/src/tests.rs | 80 ++ .../skillet/crates/hosts/beezelbot/Cargo.toml | 15 + .../crates/hosts/beezelbot/src/main.rs | 73 ++ .../recordings/beezelbot.yaml | 6 + 21 files changed, 2025 insertions(+) create mode 100644 ublue/skillet/.gitignore create mode 100644 ublue/skillet/AGENTS.md create mode 100644 ublue/skillet/Cargo.lock create mode 100644 ublue/skillet/Cargo.toml create mode 100644 ublue/skillet/crates/cli/Cargo.toml create mode 100644 ublue/skillet/crates/cli/src/main.rs create mode 100644 ublue/skillet/crates/core/Cargo.toml create mode 100644 ublue/skillet/crates/core/src/files.rs create mode 100644 ublue/skillet/crates/core/src/files/tests.rs create mode 100644 ublue/skillet/crates/core/src/lib.rs create mode 100644 ublue/skillet/crates/core/src/recorder.rs create mode 100644 ublue/skillet/crates/core/src/resource_op.rs create mode 100644 ublue/skillet/crates/core/src/system.rs create mode 100644 ublue/skillet/crates/core/src/system/tests.rs create mode 100644 ublue/skillet/crates/hardening/Cargo.toml create mode 100644 ublue/skillet/crates/hardening/files/sysctl.boxy.conf create mode 100644 ublue/skillet/crates/hardening/src/lib.rs create mode 100644 ublue/skillet/crates/hardening/src/tests.rs create mode 100644 ublue/skillet/crates/hosts/beezelbot/Cargo.toml create mode 100644 ublue/skillet/crates/hosts/beezelbot/src/main.rs create mode 100644 ublue/skillet/integration_tests/recordings/beezelbot.yaml diff --git a/ublue/skillet/.gitignore b/ublue/skillet/.gitignore new file mode 100644 index 00000000..2f7896d1 --- /dev/null +++ b/ublue/skillet/.gitignore @@ -0,0 +1 @@ +target/ diff --git a/ublue/skillet/AGENTS.md b/ublue/skillet/AGENTS.md new file mode 100644 index 00000000..e18d8009 --- /dev/null +++ b/ublue/skillet/AGENTS.md @@ -0,0 +1,61 @@ +# Skillet Project Constraints & Structure + +This document defines the architectural mandates and project structure for `skillet`, a Rust-based idempotent host configuration tool. + +## Core Mandates + +### 1. Error Handling & Safety +- **Libraries MUST use `thiserror`** for custom error types. +- **Libraries MUST NOT use `anyhow`**. `anyhow` is reserved for the CLI binary only. +- **NEVER use `unwrap()` or `expect()`** in library code. All errors must be propagated and handled. +- **Prioritize Crates over Shell-out**: Use Rust crates (e.g., `users`, `nix`) for system interactions whenever possible instead of executing shell commands. + +### 2. Idempotency +- All resources (files, users, groups, etc.) must be **idempotent**. +- Before performing an action, check the current state (e.g., compare SHA256 hashes for files, check existence for users). +- Actions should only be taken if the system state does not match the desired state. + +### 3. Testing Strategy +- **Unit Tests**: Place unit tests in a `tests` submodule within each module's directory (e.g., `src/files/tests.rs`). +- **Separation**: Never put tests in the same `.rs` file as the implementation code. Reference them using `#[cfg(test)] #[path = "MODULE/tests.rs"] mod tests;`. +- **Abstractions**: Use Traits (e.g., `FileResource`, `SystemResource`) to allow for mocking in higher-level library tests. + +### 4. Quality Control & Validation +- **Formatting & Linting**: Always run `cargo fmt` and `cargo clippy` after making changes to ensure code quality and consistency. +- **Verification**: Always run both: + - **Unit Tests**: `cargo test` across the workspace. + - **Integration Tests**: `skillet test run ` for affected hosts to verify end-to-end correctness in a containerized environment. + +## Project Structure + +The project is organized as a Cargo workspace: + +```text +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) +``` + +## Module Design +- **Modules as Cookbooks**: Each library crate under `crates/` (besides `core`) represents a "module" or "cookbook" (e.g., `skillet_hardening`). +- **Binary per Host**: The idea is to have one binary per host type that picks up these modules and reuses core primitives. +- **Core Primitives**: Found in `skillet_core`, providing the building blocks for all modules. diff --git a/ublue/skillet/Cargo.lock b/ublue/skillet/Cargo.lock new file mode 100644 index 00000000..26e1d9d7 --- /dev/null +++ b/ublue/skillet/Cargo.lock @@ -0,0 +1,862 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 4 + +[[package]] +name = "anstream" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "824a212faf96e9acacdbd09febd34438f8f711fb84e09a8916013cd7815ca28d" +dependencies = [ + "anstyle", + "anstyle-parse", + "anstyle-query", + "anstyle-wincon", + "colorchoice", + "is_terminal_polyfill", + "utf8parse", +] + +[[package]] +name = "anstyle" +version = "1.0.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "940b3a0ca603d1eade50a4846a2afffd5ef57a9feac2c0e2ec2e14f9ead76000" + +[[package]] +name = "anstyle-parse" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "52ce7f38b242319f7cabaa6813055467063ecdc9d355bbb4ce0c68908cd8130e" +dependencies = [ + "utf8parse", +] + +[[package]] +name = "anstyle-query" +version = "1.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "40c48f72fd53cd289104fc64099abca73db4166ad86ea0b4341abe65af83dadc" +dependencies = [ + "windows-sys", +] + +[[package]] +name = "anstyle-wincon" +version = "3.0.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "291e6a250ff86cd4a820112fb8898808a366d8f9f58ce16d1f538353ad55747d" +dependencies = [ + "anstyle", + "once_cell_polyfill", + "windows-sys", +] + +[[package]] +name = "anyhow" +version = "1.0.102" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" + +[[package]] +name = "bitflags" +version = "2.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "843867be96c8daad0d758b57df9392b6d8d271134fce549de6ce169ff98a92af" + +[[package]] +name = "block-buffer" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" +dependencies = [ + "generic-array", +] + +[[package]] +name = "cfg-if" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" + +[[package]] +name = "clap" +version = "4.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b193af5b67834b676abd72466a96c1024e6a6ad978a1f484bd90b85c94041351" +dependencies = [ + "clap_builder", + "clap_derive", +] + +[[package]] +name = "clap_builder" +version = "4.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "714a53001bf66416adb0e2ef5ac857140e7dc3a0c48fb28b2f10762fc4b5069f" +dependencies = [ + "anstream", + "anstyle", + "clap_lex", + "strsim", +] + +[[package]] +name = "clap_derive" +version = "4.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1110bd8a634a1ab8cb04345d8d878267d57c3cf1b38d91b71af6686408bbca6a" +dependencies = [ + "heck", + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "clap_lex" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" + +[[package]] +name = "colorchoice" +version = "1.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d07550c9036bf2ae0c684c4297d503f838287c83c53686d05370d0e139ae570" + +[[package]] +name = "cpufeatures" +version = "0.2.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59ed5838eebb26a2bb2e58f6d5b5316989ae9d08bab10e0e6d103e656d1b0280" +dependencies = [ + "libc", +] + +[[package]] +name = "crypto-common" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "78c8292055d1c1df0cce5d180393dc8cce0abec0a7102adb6c7b1eef6016d60a" +dependencies = [ + "generic-array", + "typenum", +] + +[[package]] +name = "digest" +version = "0.10.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" +dependencies = [ + "block-buffer", + "crypto-common", +] + +[[package]] +name = "equivalent" +version = "1.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "877a4ace8713b0bcf2a4e7eec82529c029f1d0619886d18145fea96c3ffe5c0f" + +[[package]] +name = "errno" +version = "0.3.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" +dependencies = [ + "libc", + "windows-sys", +] + +[[package]] +name = "fastrand" +version = "2.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" + +[[package]] +name = "foldhash" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d9c4f5dac5e15c24eb999c26181a6ca40b39fe946cbe4c263c7209467bc83af2" + +[[package]] +name = "generic-array" +version = "0.14.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" +dependencies = [ + "typenum", + "version_check", +] + +[[package]] +name = "getrandom" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0de51e6874e94e7bf76d726fc5d13ba782deca734ff60d5bb2fb2607c7406555" +dependencies = [ + "cfg-if", + "libc", + "r-efi", + "wasip2", + "wasip3", +] + +[[package]] +name = "hashbrown" +version = "0.15.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9229cfe53dfd69f0609a49f65461bd93001ea1ef889cd5529dd176593f5338a1" +dependencies = [ + "foldhash", +] + +[[package]] +name = "hashbrown" +version = "0.16.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "841d1cc9bed7f9236f321df977030373f4a4163ae1a7dbfe1a51a2c1a51d9100" + +[[package]] +name = "heck" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" + +[[package]] +name = "hex" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" + +[[package]] +name = "id-arena" +version = "2.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3d3067d79b975e8844ca9eb072e16b31c3c1c36928edf9c6789548c524d0d954" + +[[package]] +name = "indexmap" +version = "2.13.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7714e70437a7dc3ac8eb7e6f8df75fd8eb422675fc7678aff7364301092b1017" +dependencies = [ + "equivalent", + "hashbrown 0.16.1", + "serde", + "serde_core", +] + +[[package]] +name = "is_terminal_polyfill" +version = "1.70.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a6cb138bb79a146c1bd460005623e142ef0181e3d0219cb493e02f7d08a35695" + +[[package]] +name = "itoa" +version = "1.0.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682" + +[[package]] +name = "lazy_static" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbd2bcb4c963f2ddae06a2efc7e9f3591312473c50c6685e1f298068316e66fe" + +[[package]] +name = "leb128fmt" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09edd9e8b54e49e587e4f6295a7d29c3ea94d469cb40ab8ca70b288248a81db2" + +[[package]] +name = "libc" +version = "0.2.183" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b5b646652bf6661599e1da8901b3b9522896f01e736bad5f723fe7a3a27f899d" + +[[package]] +name = "linux-raw-sys" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53" + +[[package]] +name = "log" +version = "0.4.29" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5e5032e24019045c762d3c0f28f5b6b8bbf38563a65908389bf7978758920897" + +[[package]] +name = "memchr" +version = "2.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" + +[[package]] +name = "nix" +version = "0.27.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2eb04e9c688eff1c89d72b407f168cf79bb9e867a9d3323ed6c01519eb9cc053" +dependencies = [ + "bitflags", + "cfg-if", + "libc", +] + +[[package]] +name = "nu-ansi-term" +version = "0.50.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" +dependencies = [ + "windows-sys", +] + +[[package]] +name = "once_cell" +version = "1.21.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" + +[[package]] +name = "once_cell_polyfill" +version = "1.70.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" + +[[package]] +name = "pin-project-lite" +version = "0.2.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" + +[[package]] +name = "prettyplease" +version = "0.2.37" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "479ca8adacdd7ce8f1fb39ce9ecccbfe93a3f1344b3d0d97f20bc0196208f62b" +dependencies = [ + "proc-macro2", + "syn", +] + +[[package]] +name = "proc-macro2" +version = "1.0.106" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8fd00f0bb2e90d81d1044c2b32617f68fcb9fa3bb7640c23e9c748e53fb30934" +dependencies = [ + "unicode-ident", +] + +[[package]] +name = "quote" +version = "1.0.45" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "41f2619966050689382d2b44f664f4bc593e129785a36d6ee376ddf37259b924" +dependencies = [ + "proc-macro2", +] + +[[package]] +name = "r-efi" +version = "6.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8dcc9c7d52a811697d2151c701e0d08956f92b0e24136cf4cf27b57a6a0d9bf" + +[[package]] +name = "rustix" +version = "1.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e9930ba190" +dependencies = [ + "bitflags", + "errno", + "libc", + "linux-raw-sys", + "windows-sys", +] + +[[package]] +name = "ryu" +version = "1.0.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9774ba4a74de5f7b1c1451ed6cd5285a32eddb5cccb8cc655a4e50009e06477f" + +[[package]] +name = "semver" +version = "1.0.27" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d767eb0aabc880b29956c35734170f26ed551a859dbd361d140cdbeca61ab1e2" + +[[package]] +name = "serde" +version = "1.0.228" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a8e94ea7f378bd32cbbd37198a4a91436180c5bb472411e48b5ec2e2124ae9e" +dependencies = [ + "serde_core", + "serde_derive", +] + +[[package]] +name = "serde_core" +version = "1.0.228" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "41d385c7d4ca58e59fc732af25c3983b67ac852c1a25000afe1175de458b67ad" +dependencies = [ + "serde_derive", +] + +[[package]] +name = "serde_derive" +version = "1.0.228" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d540f220d3187173da220f885ab66608367b6574e925011a9353e4badda91d79" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "serde_json" +version = "1.0.149" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "83fc039473c5595ace860d8c4fafa220ff474b3fc6bfdb4293327f1a37e94d86" +dependencies = [ + "itoa", + "memchr", + "serde", + "serde_core", + "zmij", +] + +[[package]] +name = "serde_yaml" +version = "0.9.34+deprecated" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6a8b1a1a2ebf674015cc02edccce75287f1a0130d394307b36743c2f5d504b47" +dependencies = [ + "indexmap", + "itoa", + "ryu", + "serde", + "unsafe-libyaml", +] + +[[package]] +name = "sha2" +version = "0.10.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7507d819769d01a365ab707794a4084392c824f54a7a6a7862f8c3d0892b283" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + +[[package]] +name = "sharded-slab" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f40ca3c46823713e0d4209592e8d6e826aa57e928f09752619fc696c499637f6" +dependencies = [ + "lazy_static", +] + +[[package]] +name = "skillet" +version = "0.1.0" +dependencies = [ + "anyhow", + "clap", + "hex", + "serde", + "serde_yaml", + "skillet_core", + "skillet_hardening", + "tempfile", + "tracing", + "tracing-subscriber", +] + +[[package]] +name = "skillet-beezelbot" +version = "0.1.0" +dependencies = [ + "anyhow", + "clap", + "hex", + "serde", + "serde_yaml", + "skillet_core", + "skillet_hardening", + "tracing", + "tracing-subscriber", +] + +[[package]] +name = "skillet_core" +version = "0.1.0" +dependencies = [ + "hex", + "nix", + "serde", + "sha2", + "tempfile", + "thiserror", + "tracing", + "users", +] + +[[package]] +name = "skillet_hardening" +version = "0.1.0" +dependencies = [ + "skillet_core", + "tempfile", + "thiserror", + "tracing", +] + +[[package]] +name = "smallvec" +version = "1.15.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "67b1b7a3b5fe4f1376887184045fcf45c69e92af734b7aaddc05fb777b6fbd03" + +[[package]] +name = "strsim" +version = "0.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" + +[[package]] +name = "syn" +version = "2.0.117" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e665b8803e7b1d2a727f4023456bbbbe74da67099c585258af0ad9c5013b9b99" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + +[[package]] +name = "tempfile" +version = "3.27.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" +dependencies = [ + "fastrand", + "getrandom", + "once_cell", + "rustix", + "windows-sys", +] + +[[package]] +name = "thiserror" +version = "1.0.69" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6aaf5339b578ea85b50e080feb250a3e8ae8cfcdff9a461c9ec2904bc923f52" +dependencies = [ + "thiserror-impl", +] + +[[package]] +name = "thiserror-impl" +version = "1.0.69" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4fee6c4efc90059e10f81e6d42c60a18f76588c3d74cb83a0b242a2b6c7504c1" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "thread_local" +version = "1.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f60246a4944f24f6e018aa17cdeffb7818b76356965d03b07d6a9886e8962185" +dependencies = [ + "cfg-if", +] + +[[package]] +name = "tracing" +version = "0.1.44" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "63e71662fa4b2a2c3a26f570f037eb95bb1f85397f3cd8076caed2f026a6d100" +dependencies = [ + "pin-project-lite", + "tracing-attributes", + "tracing-core", +] + +[[package]] +name = "tracing-attributes" +version = "0.1.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7490cfa5ec963746568740651ac6781f701c9c5ea257c58e057f3ba8cf69e8da" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "tracing-core" +version = "0.1.36" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "db97caf9d906fbde555dd62fa95ddba9eecfd14cb388e4f491a66d74cd5fb79a" +dependencies = [ + "once_cell", + "valuable", +] + +[[package]] +name = "tracing-log" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ee855f1f400bd0e5c02d150ae5de3840039a3f54b025156404e34c23c03f47c3" +dependencies = [ + "log", + "once_cell", + "tracing-core", +] + +[[package]] +name = "tracing-subscriber" +version = "0.3.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb7f578e5945fb242538965c2d0b04418d38ec25c79d160cd279bf0731c8d319" +dependencies = [ + "nu-ansi-term", + "sharded-slab", + "smallvec", + "thread_local", + "tracing-core", + "tracing-log", +] + +[[package]] +name = "typenum" +version = "1.19.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "562d481066bde0658276a35467c4af00bdc6ee726305698a55b86e61d7ad82bb" + +[[package]] +name = "unicode-ident" +version = "1.0.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" + +[[package]] +name = "unicode-xid" +version = "0.2.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ebc1c04c71510c7f702b52b7c350734c9ff1295c464a03335b00bb84fc54f853" + +[[package]] +name = "unsafe-libyaml" +version = "0.2.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "673aac59facbab8a9007c7f6108d11f63b603f7cabff99fabf650fea5c32b861" + +[[package]] +name = "users" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "24cc0f6d6f267b73e5a2cadf007ba8f9bc39c6a6f9666f8cf25ea809a153b032" +dependencies = [ + "libc", + "log", +] + +[[package]] +name = "utf8parse" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" + +[[package]] +name = "valuable" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65" + +[[package]] +name = "version_check" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" + +[[package]] +name = "wasip2" +version = "1.0.2+wasi-0.2.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9517f9239f02c069db75e65f174b3da828fe5f5b945c4dd26bd25d89c03ebcf5" +dependencies = [ + "wit-bindgen", +] + +[[package]] +name = "wasip3" +version = "0.4.0+wasi-0.3.0-rc-2026-01-06" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5428f8bf88ea5ddc08faddef2ac4a67e390b88186c703ce6dbd955e1c145aca5" +dependencies = [ + "wit-bindgen", +] + +[[package]] +name = "wasm-encoder" +version = "0.244.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "990065f2fe63003fe337b932cfb5e3b80e0b4d0f5ff650e6985b1048f62c8319" +dependencies = [ + "leb128fmt", + "wasmparser", +] + +[[package]] +name = "wasm-metadata" +version = "0.244.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bb0e353e6a2fbdc176932bbaab493762eb1255a7900fe0fea1a2f96c296cc909" +dependencies = [ + "anyhow", + "indexmap", + "wasm-encoder", + "wasmparser", +] + +[[package]] +name = "wasmparser" +version = "0.244.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47b807c72e1bac69382b3a6fb3dbe8ea4c0ed87ff5629b8685ae6b9a611028fe" +dependencies = [ + "bitflags", + "hashbrown 0.15.5", + "indexmap", + "semver", +] + +[[package]] +name = "windows-link" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" + +[[package]] +name = "windows-sys" +version = "0.61.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae137229bcbd6cdf0f7b80a31df61766145077ddf49416a728b02cb3921ff3fc" +dependencies = [ + "windows-link", +] + +[[package]] +name = "wit-bindgen" +version = "0.51.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d7249219f66ced02969388cf2bb044a09756a083d0fab1e566056b04d9fbcaa5" +dependencies = [ + "wit-bindgen-rust-macro", +] + +[[package]] +name = "wit-bindgen-core" +version = "0.51.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ea61de684c3ea68cb082b7a88508a8b27fcc8b797d738bfc99a82facf1d752dc" +dependencies = [ + "anyhow", + "heck", + "wit-parser", +] + +[[package]] +name = "wit-bindgen-rust" +version = "0.51.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b7c566e0f4b284dd6561c786d9cb0142da491f46a9fbed79ea69cdad5db17f21" +dependencies = [ + "anyhow", + "heck", + "indexmap", + "prettyplease", + "syn", + "wasm-metadata", + "wit-bindgen-core", + "wit-component", +] + +[[package]] +name = "wit-bindgen-rust-macro" +version = "0.51.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0c0f9bfd77e6a48eccf51359e3ae77140a7f50b1e2ebfe62422d8afdaffab17a" +dependencies = [ + "anyhow", + "prettyplease", + "proc-macro2", + "quote", + "syn", + "wit-bindgen-core", + "wit-bindgen-rust", +] + +[[package]] +name = "wit-component" +version = "0.244.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9d66ea20e9553b30172b5e831994e35fbde2d165325bec84fc43dbf6f4eb9cb2" +dependencies = [ + "anyhow", + "bitflags", + "indexmap", + "log", + "serde", + "serde_derive", + "serde_json", + "wasm-encoder", + "wasm-metadata", + "wasmparser", + "wit-parser", +] + +[[package]] +name = "wit-parser" +version = "0.244.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ecc8ac4bc1dc3381b7f59c34f00b67e18f910c2c0f50015669dde7def656a736" +dependencies = [ + "anyhow", + "id-arena", + "indexmap", + "log", + "semver", + "serde", + "serde_derive", + "serde_json", + "unicode-xid", + "wasmparser", +] + +[[package]] +name = "zmij" +version = "1.0.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b8848ee67ecc8aedbaf3e4122217aff892639231befc6a1b58d29fff4c2cabaa" diff --git a/ublue/skillet/Cargo.toml b/ublue/skillet/Cargo.toml new file mode 100644 index 00000000..ab1d3f99 --- /dev/null +++ b/ublue/skillet/Cargo.toml @@ -0,0 +1,23 @@ +[workspace] +resolver = "2" +members = [ + "crates/core", + "crates/hardening", + "crates/cli", + "crates/hosts/beezelbot", +] + +[workspace.dependencies] +skillet_core = { path = "crates/core" } +skillet_hardening = { path = "crates/hardening" } +thiserror = "1.0" +sha2 = "0.10" +users = "0.11" +nix = { version = "0.27", features = ["user", "fs"] } +clap = { version = "4.4", features = ["derive"] } +tracing = "0.1" +tracing-subscriber = "0.3" +tempfile = "3.8" +serde = { version = "1.0", features = ["derive"] } +serde_json = "1.0" +hex = "0.4" diff --git a/ublue/skillet/crates/cli/Cargo.toml b/ublue/skillet/crates/cli/Cargo.toml new file mode 100644 index 00000000..95f02f3e --- /dev/null +++ b/ublue/skillet/crates/cli/Cargo.toml @@ -0,0 +1,16 @@ +[package] +name = "skillet" +version = "0.1.0" +edition = "2021" + +[dependencies] +skillet_core.workspace = true +skillet_hardening.workspace = true +clap.workspace = true +tracing.workspace = true +tracing-subscriber.workspace = true +anyhow = "1.0" +serde.workspace = true +serde_yaml = "0.9" +hex.workspace = true +tempfile.workspace = true diff --git a/ublue/skillet/crates/cli/src/main.rs b/ublue/skillet/crates/cli/src/main.rs new file mode 100644 index 00000000..65bbee6e --- /dev/null +++ b/ublue/skillet/crates/cli/src/main.rs @@ -0,0 +1,263 @@ +use anyhow::{anyhow, Context, Result}; +use clap::{Parser, Subcommand}; +use skillet_core::files::LocalFileResource; +use skillet_core::recorder::Recorder; +use skillet_core::resource_op::ResourceOp; +use skillet_core::system::LinuxSystemResource; +use skillet_hardening::apply; +use std::fs; +use std::path::PathBuf; +use std::process::Command; +use tracing::{error, 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 (Agent Mode) + Apply { + /// Optional: Output recorded actions to this file path + #[arg(long)] + record: Option, + }, + /// Manage integration tests (Runner Mode) + Test { + #[command(subcommand)] + test_command: TestCommands, + }, +} + +#[derive(Subcommand, Debug)] +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, + }, +} + +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), + Commands::Test { test_command } => handle_test(test_command), + } +} + +fn handle_apply(record_path: Option) -> 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).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(()) +} + +fn handle_test(cmd: TestCommands) -> Result<()> { + match cmd { + TestCommands::Record { hostname, image } => { + info!("Recording integration test for host: {}", hostname); + run_container_test(&hostname, &image, true)?; + } + TestCommands::Run { hostname, image } => { + info!( + "Running integration test verification for host: {}", + hostname + ); + run_container_test(&hostname, &image, false)?; + } + } + Ok(()) +} + +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<()> { + info!("Executing skillet inside container..."); + // Use 'skillet apply' directly as it's the interface for all our binaries now + // We ensure /etc/sysctl.d exists because many minimal container images lack it. + let exec_status = Command::new("podman") + .args([ + "exec", + &container_name, + "sh", + "-c", + "mkdir -p /etc/sysctl.d && 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().unwrap(), + ]) + .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().unwrap(); + + 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 = serde_yaml::from_str(&recorded_content)?; + let new_ops: Vec = 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 +} diff --git a/ublue/skillet/crates/core/Cargo.toml b/ublue/skillet/crates/core/Cargo.toml new file mode 100644 index 00000000..76e82bf2 --- /dev/null +++ b/ublue/skillet/crates/core/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "skillet_core" +version = "0.1.0" +edition = "2021" + +[dependencies] +thiserror.workspace = true +sha2.workspace = true +users.workspace = true +nix.workspace = true +tempfile.workspace = true +hex.workspace = true +serde.workspace = true +tracing.workspace = true + +[dev-dependencies] +tempfile.workspace = true diff --git a/ublue/skillet/crates/core/src/files.rs b/ublue/skillet/crates/core/src/files.rs new file mode 100644 index 00000000..9d35caeb --- /dev/null +++ b/ublue/skillet/crates/core/src/files.rs @@ -0,0 +1,217 @@ +use nix::unistd::{chown, Gid, Uid}; +use sha2::{Digest, Sha256}; +use std::fs::{self}; +use std::io::{self, Write}; +use std::os::unix::fs::PermissionsExt; +use std::path::Path; +use tempfile::NamedTempFile; +use thiserror::Error; +use tracing::info; +use users::{get_group_by_name, get_user_by_name}; + +#[derive(Error, Debug)] +pub enum FileError { + #[error("IO error: {0}")] + Io(#[from] io::Error), + #[error("Failed to persist temporary file to {0}: {1}")] + Persist(String, io::Error), + #[error("Failed to read existing file {0}: {1}")] + Read(String, io::Error), + #[error("Invalid path: {0}")] + InvalidPath(String), + #[error("Parent directory for {0} does not exist")] + ParentMissing(String), + #[error("Failed to set permissions for {0}: {1}")] + SetPermissions(String, io::Error), + #[error("Failed to set ownership for {0}: {1}")] + SetOwnership(String, String), + #[error("User {0} not found")] + UserNotFound(String), + #[error("Group {0} not found")] + GroupNotFound(String), +} + +pub trait FileResource { + fn ensure_file( + &self, + path: &Path, + content: &[u8], + mode: Option, + owner: Option<&str>, + group: Option<&str>, + ) -> Result; + fn delete_file(&self, path: &Path) -> Result; +} + +pub struct LocalFileResource; + +impl LocalFileResource { + pub fn new() -> Self { + Self + } + + fn check_metadata( + &self, + path: &Path, + mode: Option, + owner: Option<&str>, + group: Option<&str>, + ) -> Result { + 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() & 0o777) != 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.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 + } + } + + // Ownership check is a bit more involved with std::fs::Metadata. + // We can use MetadataExt. + use std::os::unix::fs::MetadataExt; + + 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( + &self, + path: &Path, + mode: Option, + 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(()) + } +} + +impl Default for LocalFileResource { + fn default() -> Self { + Self::new() + } +} + +impl FileResource for LocalFileResource { + fn ensure_file( + &self, + path: &Path, + content: &[u8], + mode: Option, + owner: Option<&str>, + group: Option<&str>, + ) -> Result { + // 1. Check parent directory + let parent = path + .parent() + .ok_or_else(|| FileError::InvalidPath(path.display().to_string()))?; + + if !parent.exists() { + return Err(FileError::ParentMissing(path.display().to_string())); + } + + let mut changed = false; + + // 2. Check content + let content_changed = if path.exists() { + 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 + } else { + true + }; + + if content_changed { + // Write to temp file in same directory (for atomic rename) + 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; + info!("Updated file content for {}", path.display()); + } + + // 3. Check and apply metadata + if path.exists() && self.check_metadata(path, mode, owner, group)? { + self.apply_metadata(path, mode, owner, group)?; + changed = true; + info!("Updated file metadata for {}", path.display()); + } + + Ok(changed) + } + + fn delete_file(&self, path: &Path) -> Result { + if path.exists() { + fs::remove_file(path).map_err(FileError::Io)?; + info!("Deleted file {}", path.display()); + Ok(true) + } else { + Ok(false) + } + } +} + +#[cfg(test)] +#[path = "files/tests.rs"] +mod tests; diff --git a/ublue/skillet/crates/core/src/files/tests.rs b/ublue/skillet/crates/core/src/files/tests.rs new file mode 100644 index 00000000..232001dc --- /dev/null +++ b/ublue/skillet/crates/core/src/files/tests.rs @@ -0,0 +1,70 @@ +use super::*; +use std::fs; +use tempfile::tempdir; + +#[test] +fn test_ensure_file_creates_file() { + let dir = tempdir().unwrap(); + let file_path = dir.path().join("test.txt"); + let content = b"hello world"; + let resource = LocalFileResource::new(); + + let changed = resource + .ensure_file(&file_path, content, None, None, None) + .unwrap(); + assert!(changed); + assert!(file_path.exists()); + assert_eq!(fs::read(&file_path).unwrap(), content); +} + +#[test] +fn test_ensure_file_idempotent() { + let dir = tempdir().unwrap(); + let file_path = dir.path().join("test_idempotent.txt"); + let content = b"idempotent"; + let resource = LocalFileResource::new(); + + // First write + let changed = resource + .ensure_file(&file_path, content, None, None, None) + .unwrap(); + assert!(changed); + + // Second write (same content) + let changed_again = resource + .ensure_file(&file_path, content, None, None, None) + .unwrap(); + assert!(!changed_again); +} + +#[test] +fn test_ensure_file_updates_content() { + let dir = tempdir().unwrap(); + let file_path = dir.path().join("test_update.txt"); + let resource = LocalFileResource::new(); + + resource + .ensure_file(&file_path, b"initial", None, None, None) + .unwrap(); + + let changed = resource + .ensure_file(&file_path, b"updated", None, None, None) + .unwrap(); + assert!(changed); + assert_eq!(fs::read(&file_path).unwrap(), b"updated"); +} + +#[test] +fn test_delete_file() { + let dir = tempdir().unwrap(); + let file_path = dir.path().join("test_delete.txt"); + fs::write(&file_path, b"delete me").unwrap(); + let resource = LocalFileResource::new(); + + let changed = resource.delete_file(&file_path).unwrap(); + assert!(changed); + assert!(!file_path.exists()); + + let changed_again = resource.delete_file(&file_path).unwrap(); + assert!(!changed_again); +} diff --git a/ublue/skillet/crates/core/src/lib.rs b/ublue/skillet/crates/core/src/lib.rs new file mode 100644 index 00000000..7a18a869 --- /dev/null +++ b/ublue/skillet/crates/core/src/lib.rs @@ -0,0 +1,4 @@ +pub mod files; +pub mod recorder; +pub mod resource_op; +pub mod system; diff --git a/ublue/skillet/crates/core/src/recorder.rs b/ublue/skillet/crates/core/src/recorder.rs new file mode 100644 index 00000000..bffd6436 --- /dev/null +++ b/ublue/skillet/crates/core/src/recorder.rs @@ -0,0 +1,77 @@ +use crate::files::{FileError, FileResource}; +use crate::resource_op::ResourceOp; +use crate::system::{SystemError, SystemResource}; +use sha2::{Digest, Sha256}; +use std::path::Path; +use std::sync::{Arc, Mutex}; + +pub struct Recorder { + inner: T, + ops: Arc>>, +} + +impl Recorder { + pub fn new(inner: T) -> Self { + Self { + inner, + ops: Arc::new(Mutex::new(Vec::new())), + } + } + + pub fn with_ops(inner: T, ops: Arc>>) -> Self { + Self { inner, ops } + } + + pub fn get_ops(&self) -> Vec { + self.ops.lock().unwrap().clone() + } + + pub fn shared_ops(&self) -> Arc>> { + self.ops.clone() + } + + fn record(&self, op: ResourceOp) { + self.ops.lock().unwrap().push(op); + } +} + +impl FileResource for Recorder { + fn ensure_file( + &self, + path: &Path, + content: &[u8], + mode: Option, + owner: Option<&str>, + group: Option<&str>, + ) -> Result { + let mut hasher = Sha256::new(); + hasher.update(content); + let hash = hex::encode(hasher.finalize()); + + self.record(ResourceOp::EnsureFile { + path: path.display().to_string(), + content_hash: hash, + mode, + owner: owner.map(|s| s.to_string()), + group: group.map(|s| s.to_string()), + }); + + self.inner.ensure_file(path, content, mode, owner, group) + } + + fn delete_file(&self, path: &Path) -> Result { + self.record(ResourceOp::DeleteFile { + path: path.display().to_string(), + }); + self.inner.delete_file(path) + } +} + +impl SystemResource for Recorder { + fn ensure_group(&self, name: &str) -> Result { + self.record(ResourceOp::EnsureGroup { + name: name.to_string(), + }); + self.inner.ensure_group(name) + } +} diff --git a/ublue/skillet/crates/core/src/resource_op.rs b/ublue/skillet/crates/core/src/resource_op.rs new file mode 100644 index 00000000..6008a2a4 --- /dev/null +++ b/ublue/skillet/crates/core/src/resource_op.rs @@ -0,0 +1,18 @@ +use serde::{Deserialize, Serialize}; + +#[derive(Serialize, Deserialize, PartialEq, Debug, Clone)] +pub enum ResourceOp { + EnsureFile { + path: String, + content_hash: String, + mode: Option, + owner: Option, + group: Option, + }, + DeleteFile { + path: String, + }, + EnsureGroup { + name: String, + }, +} diff --git a/ublue/skillet/crates/core/src/system.rs b/ublue/skillet/crates/core/src/system.rs new file mode 100644 index 00000000..baf98e7e --- /dev/null +++ b/ublue/skillet/crates/core/src/system.rs @@ -0,0 +1,63 @@ +use std::process::Command; +use thiserror::Error; +use tracing::{debug, info}; +use users::get_group_by_name; + +#[derive(Error, Debug)] +pub enum SystemError { + #[error("Group check error: {0}")] + GroupCheck(String), + #[error("Command failed: {0}")] + Command(String), + #[error("IO error: {0}")] + Io(#[from] std::io::Error), +} + +pub trait SystemResource { + fn ensure_group(&self, name: &str) -> Result; +} + +pub struct LinuxSystemResource; + +impl LinuxSystemResource { + pub fn new() -> Self { + Self + } +} + +impl Default for LinuxSystemResource { + fn default() -> Self { + Self::new() + } +} + +impl SystemResource for LinuxSystemResource { + fn ensure_group(&self, name: &str) -> Result { + // 1. Check if group exists using `users` crate + if get_group_by_name(name).is_some() { + debug!("Group {} already exists", name); + return Ok(false); + } + + // 2. Create group using `groupadd` + // Note: Creating groups requires root privileges usually. + info!("Creating group {}", name); + let output = Command::new("groupadd") + .arg(name) + // .arg("-r") // System group? Maybe make it an option? + // For now, simple group creation. + .output()?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + return Err(SystemError::Command(format!("groupadd failed: {}", stderr))); + } + + info!("Created group {}", name); + Ok(true) + } +} + +#[cfg(test)] +#[path = "system/tests.rs"] +mod tests; diff --git a/ublue/skillet/crates/core/src/system/tests.rs b/ublue/skillet/crates/core/src/system/tests.rs new file mode 100644 index 00000000..5e100091 --- /dev/null +++ b/ublue/skillet/crates/core/src/system/tests.rs @@ -0,0 +1,39 @@ +use super::*; +use std::collections::HashSet; +use std::sync::{Arc, Mutex}; + +// Mock implementation for testing consumers +pub struct MockSystemResource { + pub groups: Arc>>, +} + +impl MockSystemResource { + pub fn new() -> Self { + Self { + groups: Arc::new(Mutex::new(HashSet::new())), + } + } +} + +impl SystemResource for MockSystemResource { + fn ensure_group(&self, name: &str) -> Result { + let mut groups = self.groups.lock().unwrap(); + if groups.contains(name) { + Ok(false) + } else { + groups.insert(name.to_string()); + Ok(true) + } + } +} + +#[test] +fn test_mock_system_resource() { + let system = MockSystemResource::new(); + let changed = system.ensure_group("syslog").unwrap(); + assert!(changed); + assert!(system.groups.lock().unwrap().contains("syslog")); + + let changed_again = system.ensure_group("syslog").unwrap(); + assert!(!changed_again); +} diff --git a/ublue/skillet/crates/hardening/Cargo.toml b/ublue/skillet/crates/hardening/Cargo.toml new file mode 100644 index 00000000..f9a9f432 --- /dev/null +++ b/ublue/skillet/crates/hardening/Cargo.toml @@ -0,0 +1,12 @@ +[package] +name = "skillet_hardening" +version = "0.1.0" +edition = "2021" + +[dependencies] +skillet_core.workspace = true +thiserror.workspace = true +tracing.workspace = true + +[dev-dependencies] +tempfile.workspace = true diff --git a/ublue/skillet/crates/hardening/files/sysctl.boxy.conf b/ublue/skillet/crates/hardening/files/sysctl.boxy.conf new file mode 100644 index 00000000..1b29f8bb --- /dev/null +++ b/ublue/skillet/crates/hardening/files/sysctl.boxy.conf @@ -0,0 +1,40 @@ +fs.suid_dumpable = 0 +kernel.randomize_va_space = 2 +kernel.sysrq = 0 +net.ipv4.conf.all.accept_redirects = 0 +net.ipv4.conf.all.accept_source_route = 0 +net.ipv4.conf.all.arp_announce = 2 +net.ipv4.conf.all.arp_ignore = 1 +net.ipv4.conf.all.log_martians = 0 +net.ipv4.conf.all.rp_filter = 1 +net.ipv4.conf.all.secure_redirects = 0 +net.ipv4.conf.all.send_redirects = 0 +net.ipv4.conf.all.shared_media = 1 +net.ipv4.conf.default.accept_redirects = 0 +net.ipv4.conf.default.accept_source_route = 0 +net.ipv4.conf.default.log_martians = 0 +net.ipv4.conf.default.rp_filter = 1 +net.ipv4.conf.default.secure_redirects = 0 +net.ipv4.conf.default.send_redirects = 0 +net.ipv4.conf.default.shared_media = 1 +net.ipv4.icmp_echo_ignore_broadcasts = 1 +net.ipv4.icmp_ignore_bogus_error_responses = 1 +net.ipv4.icmp_ratelimit = 100 +net.ipv4.icmp_ratemask = 88089 +net.ipv4.ip_forward = 1 +net.ipv4.tcp_rfc1337 = 1 +net.ipv4.tcp_syncookies = 1 +net.ipv4.tcp_timestamps = 0 +net.ipv6.conf.all.accept_ra = 0 +net.ipv6.conf.all.accept_redirects = 0 +net.ipv6.conf.all.disable_ipv6 = 0 +net.ipv6.conf.all.forwarding = 1 +net.ipv6.conf.default.accept_ra = 0 +net.ipv6.conf.default.accept_ra_defrtr = 0 +net.ipv6.conf.default.accept_ra_pinfo = 0 +net.ipv6.conf.default.accept_ra_rtr_pref = 0 +net.ipv6.conf.default.accept_redirects = 0 +net.ipv6.conf.default.autoconf = 0 +net.ipv6.conf.default.dad_transmits = 0 +net.ipv6.conf.default.max_addresses = 1 +net.ipv6.conf.default.router_solicitations = 0 diff --git a/ublue/skillet/crates/hardening/src/lib.rs b/ublue/skillet/crates/hardening/src/lib.rs new file mode 100644 index 00000000..b372bfec --- /dev/null +++ b/ublue/skillet/crates/hardening/src/lib.rs @@ -0,0 +1,68 @@ +use skillet_core::files::{FileError, FileResource}; +use skillet_core::system::{SystemError, SystemResource}; +use std::path::Path; +use thiserror::Error; +use tracing::info; + +#[derive(Error, Debug)] +pub enum HardeningError { + #[error("System error: {0}")] + System(#[from] SystemError), + #[error("File error: {0}")] + File(#[from] FileError), +} + +pub fn apply(system: &S, files: &F) -> Result<(), HardeningError> +where + S: SystemResource + ?Sized, + F: FileResource + ?Sized, +{ + info!("Applying hardening..."); + + // 1. Sysctl hardening + apply_sysctl_hardening(files)?; + + // 2. Include 'os-hardening' + apply_os_hardening(system)?; + + // 3. Include 'ssh-hardening::server' + apply_ssh_hardening_server(system)?; + + // 4. Include 'ssh-hardening::client' + apply_ssh_hardening_client(system)?; + + Ok(()) +} + +fn apply_sysctl_hardening(files: &F) -> Result<(), HardeningError> { + info!("Applying sysctl hardening..."); + let content = include_bytes!("../files/sysctl.boxy.conf"); + let path = Path::new("/etc/sysctl.d/99-hardening.conf"); + + files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; + + Ok(()) +} + +fn apply_os_hardening(_system: &S) -> Result<(), HardeningError> { + info!("(Placeholder) Applying os-hardening"); + Ok(()) +} + +fn apply_ssh_hardening_server( + _system: &S, +) -> Result<(), HardeningError> { + info!("(Placeholder) Applying ssh-hardening::server"); + Ok(()) +} + +fn apply_ssh_hardening_client( + _system: &S, +) -> Result<(), HardeningError> { + info!("(Placeholder) Applying ssh-hardening::client"); + Ok(()) +} + +#[cfg(test)] +#[path = "tests.rs"] +mod tests; diff --git a/ublue/skillet/crates/hardening/src/tests.rs b/ublue/skillet/crates/hardening/src/tests.rs new file mode 100644 index 00000000..81f26dcb --- /dev/null +++ b/ublue/skillet/crates/hardening/src/tests.rs @@ -0,0 +1,80 @@ +use super::*; +use skillet_core::files::{FileError, FileResource}; +use skillet_core::system::{SystemError, SystemResource}; +use std::collections::{HashMap, HashSet}; +use std::path::Path; +use std::sync::{Arc, Mutex}; + +struct MockSystem { + groups: Arc>>, +} + +impl MockSystem { + fn new() -> Self { + Self { + groups: Arc::new(Mutex::new(HashSet::new())), + } + } +} + +impl SystemResource for MockSystem { + fn ensure_group(&self, name: &str) -> Result { + let mut groups = self.groups.lock().unwrap(); + if groups.contains(name) { + Ok(false) + } else { + groups.insert(name.to_string()); + Ok(true) + } + } +} + +struct MockFiles { + files: Arc>>>, +} + +impl MockFiles { + fn new() -> Self { + Self { + files: Arc::new(Mutex::new(HashMap::new())), + } + } +} + +impl FileResource for MockFiles { + fn ensure_file( + &self, + path: &Path, + content: &[u8], + _mode: Option, + _owner: Option<&str>, + _group: Option<&str>, + ) -> Result { + let mut files = self.files.lock().unwrap(); + let path_str = path.display().to_string(); + if let Some(existing) = files.get(&path_str) { + if existing == content { + return Ok(false); + } + } + files.insert(path_str, content.to_vec()); + Ok(true) + } + + fn delete_file(&self, path: &Path) -> Result { + let mut files = self.files.lock().unwrap(); + Ok(files.remove(&path.display().to_string()).is_some()) + } +} + +#[test] +fn test_hardening_applies_sysctl() { + let system = MockSystem::new(); + let files = MockFiles::new(); + apply(&system, &files).unwrap(); + assert!(files + .files + .lock() + .unwrap() + .contains_key("/etc/sysctl.d/99-hardening.conf")); +} diff --git a/ublue/skillet/crates/hosts/beezelbot/Cargo.toml b/ublue/skillet/crates/hosts/beezelbot/Cargo.toml new file mode 100644 index 00000000..99bb502e --- /dev/null +++ b/ublue/skillet/crates/hosts/beezelbot/Cargo.toml @@ -0,0 +1,15 @@ +[package] +name = "skillet-beezelbot" +version = "0.1.0" +edition = "2021" + +[dependencies] +skillet_core.workspace = true +skillet_hardening.workspace = true +clap.workspace = true +tracing.workspace = true +tracing-subscriber.workspace = true +anyhow = "1.0" +serde.workspace = true +serde_yaml = "0.9" +hex.workspace = true diff --git a/ublue/skillet/crates/hosts/beezelbot/src/main.rs b/ublue/skillet/crates/hosts/beezelbot/src/main.rs new file mode 100644 index 00000000..03817940 --- /dev/null +++ b/ublue/skillet/crates/hosts/beezelbot/src/main.rs @@ -0,0 +1,73 @@ +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, + }, +} + +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) -> 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(()) +} diff --git a/ublue/skillet/integration_tests/recordings/beezelbot.yaml b/ublue/skillet/integration_tests/recordings/beezelbot.yaml new file mode 100644 index 00000000..3ea1f5b7 --- /dev/null +++ b/ublue/skillet/integration_tests/recordings/beezelbot.yaml @@ -0,0 +1,6 @@ +- !EnsureFile + path: /etc/sysctl.d/99-hardening.conf + content_hash: c71e2f0edb84c44cfb601a2dc3d35df3b46afbbe9d28e02283a12d4b5f55b89d + mode: 420 + owner: root + group: root From 1db75db920da3776731f72e45f64838284b8c5bc Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 21 Mar 2026 22:02:22 +0100 Subject: [PATCH 02/19] refactor: address PR review comments - 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. --- ublue/skillet/Cargo.lock | 9 +- ublue/skillet/Cargo.toml | 2 + ublue/skillet/crates/cli-common/Cargo.toml | 14 +++ ublue/skillet/crates/cli-common/src/lib.rs | 73 ++++++++++++ ublue/skillet/crates/cli/src/main.rs | 13 ++- ublue/skillet/crates/core/Cargo.toml | 3 + ublue/skillet/crates/core/src/files.rs | 15 +-- ublue/skillet/crates/core/src/files/tests.rs | 31 +++++ ublue/skillet/crates/core/src/lib.rs | 2 + ublue/skillet/crates/core/src/recorder.rs | 6 +- ublue/skillet/crates/core/src/resource_op.rs | 2 +- ublue/skillet/crates/core/src/test_utils.rs | 110 ++++++++++++++++++ ublue/skillet/crates/hardening/Cargo.toml | 2 +- ublue/skillet/crates/hardening/src/tests.rs | 70 +---------- .../skillet/crates/hosts/beezelbot/Cargo.toml | 9 +- .../crates/hosts/beezelbot/src/main.rs | 73 +----------- .../recordings/beezelbot.yaml | 2 +- 17 files changed, 265 insertions(+), 171 deletions(-) create mode 100644 ublue/skillet/crates/cli-common/Cargo.toml create mode 100644 ublue/skillet/crates/cli-common/src/lib.rs create mode 100644 ublue/skillet/crates/core/src/test_utils.rs diff --git a/ublue/skillet/Cargo.lock b/ublue/skillet/Cargo.lock index 26e1d9d7..f305e365 100644 --- a/ublue/skillet/Cargo.lock +++ b/ublue/skillet/Cargo.lock @@ -490,10 +490,17 @@ dependencies = [ [[package]] name = "skillet-beezelbot" version = "0.1.0" +dependencies = [ + "anyhow", + "skillet_cli_common", +] + +[[package]] +name = "skillet_cli_common" +version = "0.1.0" dependencies = [ "anyhow", "clap", - "hex", "serde", "serde_yaml", "skillet_core", diff --git a/ublue/skillet/Cargo.toml b/ublue/skillet/Cargo.toml index ab1d3f99..5c7bd3c4 100644 --- a/ublue/skillet/Cargo.toml +++ b/ublue/skillet/Cargo.toml @@ -5,11 +5,13 @@ members = [ "crates/hardening", "crates/cli", "crates/hosts/beezelbot", + "crates/cli-common", ] [workspace.dependencies] skillet_core = { path = "crates/core" } skillet_hardening = { path = "crates/hardening" } +skillet_cli_common = { path = "crates/cli-common" } thiserror = "1.0" sha2 = "0.10" users = "0.11" diff --git a/ublue/skillet/crates/cli-common/Cargo.toml b/ublue/skillet/crates/cli-common/Cargo.toml new file mode 100644 index 00000000..27a0db7c --- /dev/null +++ b/ublue/skillet/crates/cli-common/Cargo.toml @@ -0,0 +1,14 @@ +[package] +name = "skillet_cli_common" +version = "0.1.0" +edition = "2021" + +[dependencies] +skillet_core.workspace = true +skillet_hardening.workspace = true +clap.workspace = true +tracing.workspace = true +tracing-subscriber.workspace = true +anyhow = "1.0" +serde.workspace = true +serde_yaml = "0.9" diff --git a/ublue/skillet/crates/cli-common/src/lib.rs b/ublue/skillet/crates/cli-common/src/lib.rs new file mode 100644 index 00000000..fc5bbfed --- /dev/null +++ b/ublue/skillet/crates/cli-common/src/lib.rs @@ -0,0 +1,73 @@ +use anyhow::{Context, Result}; +use clap::Parser; +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)] +pub struct HostArgs { + #[command(subcommand)] + pub command: HostCommands, + + /// Enable verbose logging + #[arg(short, long, global = true)] + pub verbose: bool, +} + +#[derive(clap::Subcommand, Debug)] +pub enum HostCommands { + /// Apply configuration + Apply { + /// Optional: Output recorded actions to this file path + #[arg(long)] + record: Option, + }, +} + +pub fn run_host(hostname: &str) -> Result<()> { + let args = HostArgs::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 { + HostCommands::Apply { record } => handle_apply(hostname, record), + } +} + +fn handle_apply(hostname: &str, record_path: Option) -> Result<()> { + info!("Starting Skillet configuration for {}...", hostname); + + 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(()) +} diff --git a/ublue/skillet/crates/cli/src/main.rs b/ublue/skillet/crates/cli/src/main.rs index 65bbee6e..ea9bbc00 100644 --- a/ublue/skillet/crates/cli/src/main.rs +++ b/ublue/skillet/crates/cli/src/main.rs @@ -81,14 +81,14 @@ fn handle_apply(record_path: Option) -> Result<()> { 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))?; + 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).map_err(|e| anyhow!(e))?; + apply(&system, &files)?; } info!("Configuration applied successfully."); @@ -207,7 +207,9 @@ fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<() .args([ "cp", &format!("{}:/tmp/ops.yaml", container_name), - dest_file.to_str().unwrap(), + dest_file + .to_str() + .ok_or_else(|| anyhow!("Destination path is not valid UTF-8"))?, ]) .status()?; @@ -217,7 +219,10 @@ fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<() } else { info!("Verifying recording..."); let temp_dest = tempfile::Builder::new().suffix(".yaml").tempfile()?; - 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"))?; let cp_status = Command::new("podman") .args([ diff --git a/ublue/skillet/crates/core/Cargo.toml b/ublue/skillet/crates/core/Cargo.toml index 76e82bf2..cd8f2250 100644 --- a/ublue/skillet/crates/core/Cargo.toml +++ b/ublue/skillet/crates/core/Cargo.toml @@ -15,3 +15,6 @@ tracing.workspace = true [dev-dependencies] tempfile.workspace = true + +[features] +test-utils = [] diff --git a/ublue/skillet/crates/core/src/files.rs b/ublue/skillet/crates/core/src/files.rs index 9d35caeb..85891237 100644 --- a/ublue/skillet/crates/core/src/files.rs +++ b/ublue/skillet/crates/core/src/files.rs @@ -2,7 +2,7 @@ use nix::unistd::{chown, Gid, Uid}; use sha2::{Digest, Sha256}; use std::fs::{self}; use std::io::{self, Write}; -use std::os::unix::fs::PermissionsExt; +use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::Path; use tempfile::NamedTempFile; use thiserror::Error; @@ -67,19 +67,6 @@ impl LocalFileResource { } } - 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 - } - } - - // Ownership check is a bit more involved with std::fs::Metadata. - // We can use MetadataExt. - use std::os::unix::fs::MetadataExt; - if let Some(desired_user) = owner { let user = get_user_by_name(desired_user) .ok_or_else(|| FileError::UserNotFound(desired_user.to_string()))?; diff --git a/ublue/skillet/crates/core/src/files/tests.rs b/ublue/skillet/crates/core/src/files/tests.rs index 232001dc..bfe75580 100644 --- a/ublue/skillet/crates/core/src/files/tests.rs +++ b/ublue/skillet/crates/core/src/files/tests.rs @@ -1,5 +1,6 @@ use super::*; use std::fs; +use std::os::unix::fs::PermissionsExt; use tempfile::tempdir; #[test] @@ -54,6 +55,36 @@ fn test_ensure_file_updates_content() { assert_eq!(fs::read(&file_path).unwrap(), b"updated"); } +#[test] +fn test_ensure_file_metadata() { + let dir = tempdir().unwrap(); + let file_path = dir.path().join("test_meta.txt"); + let resource = LocalFileResource::new(); + let content = b"metadata test"; + + // 1. Create with default meta + resource + .ensure_file(&file_path, content, None, None, None) + .unwrap(); + + // 2. Change mode + let changed = resource + .ensure_file(&file_path, content, Some(0o644), None, None) + .unwrap(); + assert!(changed); + let meta = fs::metadata(&file_path).unwrap(); + assert_eq!(meta.permissions().mode() & 0o777, 0o644); + + // 3. Idempotent mode change + let changed_again = resource + .ensure_file(&file_path, content, Some(0o644), None, None) + .unwrap(); + assert!(!changed_again); + + // Note: Testing owner/group change typically requires root, so we skip it in unit tests + // or we would need to mock the underlying chown call. +} + #[test] fn test_delete_file() { let dir = tempdir().unwrap(); diff --git a/ublue/skillet/crates/core/src/lib.rs b/ublue/skillet/crates/core/src/lib.rs index 7a18a869..59285565 100644 --- a/ublue/skillet/crates/core/src/lib.rs +++ b/ublue/skillet/crates/core/src/lib.rs @@ -2,3 +2,5 @@ pub mod files; pub mod recorder; pub mod resource_op; pub mod system; +#[cfg(feature = "test-utils")] +pub mod test_utils; diff --git a/ublue/skillet/crates/core/src/recorder.rs b/ublue/skillet/crates/core/src/recorder.rs index bffd6436..e8af59a2 100644 --- a/ublue/skillet/crates/core/src/recorder.rs +++ b/ublue/skillet/crates/core/src/recorder.rs @@ -23,7 +23,7 @@ impl Recorder { } pub fn get_ops(&self) -> Vec { - self.ops.lock().unwrap().clone() + self.ops.lock().unwrap_or_else(|e| e.into_inner()).clone() } pub fn shared_ops(&self) -> Arc>> { @@ -31,7 +31,7 @@ impl Recorder { } fn record(&self, op: ResourceOp) { - self.ops.lock().unwrap().push(op); + self.ops.lock().unwrap_or_else(|e| e.into_inner()).push(op); } } @@ -51,7 +51,7 @@ impl FileResource for Recorder { self.record(ResourceOp::EnsureFile { path: path.display().to_string(), content_hash: hash, - mode, + mode: mode.map(|m| format!("0o{:o}", m)), owner: owner.map(|s| s.to_string()), group: group.map(|s| s.to_string()), }); diff --git a/ublue/skillet/crates/core/src/resource_op.rs b/ublue/skillet/crates/core/src/resource_op.rs index 6008a2a4..da251c3f 100644 --- a/ublue/skillet/crates/core/src/resource_op.rs +++ b/ublue/skillet/crates/core/src/resource_op.rs @@ -5,7 +5,7 @@ pub enum ResourceOp { EnsureFile { path: String, content_hash: String, - mode: Option, + mode: Option, owner: Option, group: Option, }, diff --git a/ublue/skillet/crates/core/src/test_utils.rs b/ublue/skillet/crates/core/src/test_utils.rs new file mode 100644 index 00000000..81d4466d --- /dev/null +++ b/ublue/skillet/crates/core/src/test_utils.rs @@ -0,0 +1,110 @@ +use crate::files::{FileError, FileResource}; +use crate::system::{SystemError, SystemResource}; +use std::collections::{HashMap, HashSet}; +use std::path::Path; +use std::sync::{Arc, Mutex}; + +pub struct MockSystem { + pub groups: Arc>>, +} + +impl MockSystem { + pub fn new() -> Self { + Self { + groups: Arc::new(Mutex::new(HashSet::new())), + } + } +} + +impl Default for MockSystem { + fn default() -> Self { + Self::new() + } +} + +impl SystemResource for MockSystem { + fn ensure_group(&self, name: &str) -> Result { + let mut groups = self.groups.lock().unwrap_or_else(|e| e.into_inner()); + if groups.contains(name) { + Ok(false) + } else { + groups.insert(name.to_string()); + Ok(true) + } + } +} + +pub struct MockFiles { + pub files: Arc>>>, + pub metadata: Arc, Option, Option)>>>, +} + +impl MockFiles { + pub fn new() -> Self { + Self { + files: Arc::new(Mutex::new(HashMap::new())), + metadata: Arc::new(Mutex::new(HashMap::new())), + } + } +} + +impl Default for MockFiles { + fn default() -> Self { + Self::new() + } +} + +impl FileResource for MockFiles { + fn ensure_file( + &self, + path: &Path, + content: &[u8], + mode: Option, + owner: Option<&str>, + group: Option<&str>, + ) -> Result { + let path_str = path.display().to_string(); + let mut files = self.files.lock().unwrap_or_else(|e| e.into_inner()); + let mut metadata = self.metadata.lock().unwrap_or_else(|e| e.into_inner()); + + let mut changed = false; + + if let Some(existing) = files.get(&path_str) { + if existing != content { + files.insert(path_str.clone(), content.to_vec()); + changed = true; + } + } else { + files.insert(path_str.clone(), content.to_vec()); + changed = true; + } + + let new_meta = ( + mode, + owner.map(|s| s.to_string()), + group.map(|s| s.to_string()), + ); + if let Some(existing_meta) = metadata.get(&path_str) { + if existing_meta != &new_meta { + metadata.insert(path_str, new_meta); + changed = true; + } + } else { + metadata.insert(path_str, new_meta); + changed = true; + } + + Ok(changed) + } + + fn delete_file(&self, path: &Path) -> Result { + let path_str = path.display().to_string(); + let mut files = self.files.lock().unwrap_or_else(|e| e.into_inner()); + let mut metadata = self.metadata.lock().unwrap_or_else(|e| e.into_inner()); + + let f_removed = files.remove(&path_str).is_some(); + let m_removed = metadata.remove(&path_str).is_some(); + + Ok(f_removed || m_removed) + } +} diff --git a/ublue/skillet/crates/hardening/Cargo.toml b/ublue/skillet/crates/hardening/Cargo.toml index f9a9f432..ea4c8203 100644 --- a/ublue/skillet/crates/hardening/Cargo.toml +++ b/ublue/skillet/crates/hardening/Cargo.toml @@ -4,7 +4,7 @@ version = "0.1.0" edition = "2021" [dependencies] -skillet_core.workspace = true +skillet_core = { workspace = true, features = ["test-utils"] } thiserror.workspace = true tracing.workspace = true diff --git a/ublue/skillet/crates/hardening/src/tests.rs b/ublue/skillet/crates/hardening/src/tests.rs index 81f26dcb..3d35e61b 100644 --- a/ublue/skillet/crates/hardening/src/tests.rs +++ b/ublue/skillet/crates/hardening/src/tests.rs @@ -1,71 +1,5 @@ use super::*; -use skillet_core::files::{FileError, FileResource}; -use skillet_core::system::{SystemError, SystemResource}; -use std::collections::{HashMap, HashSet}; -use std::path::Path; -use std::sync::{Arc, Mutex}; - -struct MockSystem { - groups: Arc>>, -} - -impl MockSystem { - fn new() -> Self { - Self { - groups: Arc::new(Mutex::new(HashSet::new())), - } - } -} - -impl SystemResource for MockSystem { - fn ensure_group(&self, name: &str) -> Result { - let mut groups = self.groups.lock().unwrap(); - if groups.contains(name) { - Ok(false) - } else { - groups.insert(name.to_string()); - Ok(true) - } - } -} - -struct MockFiles { - files: Arc>>>, -} - -impl MockFiles { - fn new() -> Self { - Self { - files: Arc::new(Mutex::new(HashMap::new())), - } - } -} - -impl FileResource for MockFiles { - fn ensure_file( - &self, - path: &Path, - content: &[u8], - _mode: Option, - _owner: Option<&str>, - _group: Option<&str>, - ) -> Result { - let mut files = self.files.lock().unwrap(); - let path_str = path.display().to_string(); - if let Some(existing) = files.get(&path_str) { - if existing == content { - return Ok(false); - } - } - files.insert(path_str, content.to_vec()); - Ok(true) - } - - fn delete_file(&self, path: &Path) -> Result { - let mut files = self.files.lock().unwrap(); - Ok(files.remove(&path.display().to_string()).is_some()) - } -} +use skillet_core::test_utils::{MockFiles, MockSystem}; #[test] fn test_hardening_applies_sysctl() { @@ -75,6 +9,6 @@ fn test_hardening_applies_sysctl() { assert!(files .files .lock() - .unwrap() + .unwrap_or_else(|e| e.into_inner()) .contains_key("/etc/sysctl.d/99-hardening.conf")); } diff --git a/ublue/skillet/crates/hosts/beezelbot/Cargo.toml b/ublue/skillet/crates/hosts/beezelbot/Cargo.toml index 99bb502e..666fe7f9 100644 --- a/ublue/skillet/crates/hosts/beezelbot/Cargo.toml +++ b/ublue/skillet/crates/hosts/beezelbot/Cargo.toml @@ -4,12 +4,5 @@ version = "0.1.0" edition = "2021" [dependencies] -skillet_core.workspace = true -skillet_hardening.workspace = true -clap.workspace = true -tracing.workspace = true -tracing-subscriber.workspace = true +skillet_cli_common.workspace = true anyhow = "1.0" -serde.workspace = true -serde_yaml = "0.9" -hex.workspace = true diff --git a/ublue/skillet/crates/hosts/beezelbot/src/main.rs b/ublue/skillet/crates/hosts/beezelbot/src/main.rs index 03817940..c6e4eb17 100644 --- a/ublue/skillet/crates/hosts/beezelbot/src/main.rs +++ b/ublue/skillet/crates/hosts/beezelbot/src/main.rs @@ -1,73 +1,6 @@ -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, - }, -} +use anyhow::Result; +use skillet_cli_common::run_host; 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) -> 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(()) + run_host("beezelbot") } diff --git a/ublue/skillet/integration_tests/recordings/beezelbot.yaml b/ublue/skillet/integration_tests/recordings/beezelbot.yaml index 3ea1f5b7..572f8ba3 100644 --- a/ublue/skillet/integration_tests/recordings/beezelbot.yaml +++ b/ublue/skillet/integration_tests/recordings/beezelbot.yaml @@ -1,6 +1,6 @@ - !EnsureFile path: /etc/sysctl.d/99-hardening.conf content_hash: c71e2f0edb84c44cfb601a2dc3d35df3b46afbbe9d28e02283a12d4b5f55b89d - mode: 420 + mode: '0o644' owner: root group: root From 005d85bfe8a3bc2a9f2465cbfe6a00d3e9ce6e0d Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 21 Mar 2026 22:11:45 +0100 Subject: [PATCH 03/19] refactor: replace expect with proper error propagation in cli crates - 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. --- ublue/skillet/Cargo.lock | 2 +- ublue/skillet/crates/cli-common/Cargo.toml | 2 +- ublue/skillet/crates/cli-common/src/lib.rs | 22 ++++++++++++++----- ublue/skillet/crates/cli/src/main.rs | 2 +- .../crates/hosts/beezelbot/src/main.rs | 3 ++- 5 files changed, 22 insertions(+), 9 deletions(-) diff --git a/ublue/skillet/Cargo.lock b/ublue/skillet/Cargo.lock index f305e365..036f3c07 100644 --- a/ublue/skillet/Cargo.lock +++ b/ublue/skillet/Cargo.lock @@ -499,12 +499,12 @@ dependencies = [ name = "skillet_cli_common" version = "0.1.0" dependencies = [ - "anyhow", "clap", "serde", "serde_yaml", "skillet_core", "skillet_hardening", + "thiserror", "tracing", "tracing-subscriber", ] diff --git a/ublue/skillet/crates/cli-common/Cargo.toml b/ublue/skillet/crates/cli-common/Cargo.toml index 27a0db7c..2babad52 100644 --- a/ublue/skillet/crates/cli-common/Cargo.toml +++ b/ublue/skillet/crates/cli-common/Cargo.toml @@ -9,6 +9,6 @@ skillet_hardening.workspace = true clap.workspace = true tracing.workspace = true tracing-subscriber.workspace = true -anyhow = "1.0" +thiserror.workspace = true serde.workspace = true serde_yaml = "0.9" diff --git a/ublue/skillet/crates/cli-common/src/lib.rs b/ublue/skillet/crates/cli-common/src/lib.rs index fc5bbfed..4b8fd280 100644 --- a/ublue/skillet/crates/cli-common/src/lib.rs +++ b/ublue/skillet/crates/cli-common/src/lib.rs @@ -1,4 +1,3 @@ -use anyhow::{Context, Result}; use clap::Parser; use skillet_core::files::LocalFileResource; use skillet_core::recorder::Recorder; @@ -6,9 +5,22 @@ use skillet_core::system::LinuxSystemResource; use skillet_hardening::apply; use std::fs; use std::path::PathBuf; +use thiserror::Error; use tracing::{info, Level}; use tracing_subscriber::FmtSubscriber; +#[derive(Error, Debug)] +pub enum CliCommonError { + #[error("Failed to apply hardening: {0}")] + Hardening(#[from] skillet_hardening::HardeningError), + #[error("Failed to set default tracing subscriber: {0}")] + SetLogger(#[from] tracing::subscriber::SetGlobalDefaultError), + #[error("IO error: {0}")] + Io(#[from] std::io::Error), + #[error("Serialization error: {0}")] + Yaml(#[from] serde_yaml::Error), +} + #[derive(Parser, Debug)] #[command(author, version, about, long_about = None)] pub struct HostArgs { @@ -30,7 +42,7 @@ pub enum HostCommands { }, } -pub fn run_host(hostname: &str) -> Result<()> { +pub fn run_host(hostname: &str) -> Result<(), CliCommonError> { let args = HostArgs::parse(); let subscriber = FmtSubscriber::builder() @@ -41,14 +53,14 @@ pub fn run_host(hostname: &str) -> Result<()> { }) .finish(); - tracing::subscriber::set_global_default(subscriber).expect("setting default subscriber failed"); + tracing::subscriber::set_global_default(subscriber)?; match args.command { HostCommands::Apply { record } => handle_apply(hostname, record), } } -fn handle_apply(hostname: &str, record_path: Option) -> Result<()> { +fn handle_apply(hostname: &str, record_path: Option) -> Result<(), CliCommonError> { info!("Starting Skillet configuration for {}...", hostname); let system = LinuxSystemResource::new(); @@ -62,7 +74,7 @@ fn handle_apply(hostname: &str, record_path: Option) -> Result<()> { let ops = recorder_system.get_ops(); let yaml = serde_yaml::to_string(&ops)?; - fs::write(&path, yaml).context("Failed to write recording")?; + fs::write(&path, yaml)?; info!("Recording saved to {}", path.display()); } else { apply(&system, &files)?; diff --git a/ublue/skillet/crates/cli/src/main.rs b/ublue/skillet/crates/cli/src/main.rs index ea9bbc00..fca62aa9 100644 --- a/ublue/skillet/crates/cli/src/main.rs +++ b/ublue/skillet/crates/cli/src/main.rs @@ -63,7 +63,7 @@ fn main() -> Result<()> { }) .finish(); - tracing::subscriber::set_global_default(subscriber).expect("setting default subscriber failed"); + tracing::subscriber::set_global_default(subscriber).context("setting default subscriber failed")?; match args.command { Commands::Apply { record } => handle_apply(record), diff --git a/ublue/skillet/crates/hosts/beezelbot/src/main.rs b/ublue/skillet/crates/hosts/beezelbot/src/main.rs index c6e4eb17..fdc19c19 100644 --- a/ublue/skillet/crates/hosts/beezelbot/src/main.rs +++ b/ublue/skillet/crates/hosts/beezelbot/src/main.rs @@ -2,5 +2,6 @@ use anyhow::Result; use skillet_cli_common::run_host; fn main() -> Result<()> { - run_host("beezelbot") + run_host("beezelbot")?; + Ok(()) } From 9cc457d2ee19521996daaa757048dee963521434 Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 21 Mar 2026 22:16:33 +0100 Subject: [PATCH 04/19] ci: add skillet quality checks to GitHub workflow - Runs clippy, unit tests, and integration tests. - Uses path filtering to run only when ublue/skillet/ is changed. --- .github/workflows/ci.yml | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index eac3b627..0f5621f5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,3 +49,42 @@ jobs: run: | find . -name "*.sh" -not -path "./.venv/*" -not -path "./berks-cookbooks/*" -exec shellcheck -x {} + shellcheck -x run + + skillet-checks: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dorny/paths-filter@v3 + id: filter + with: + filters: | + rust: + - 'ublue/skillet/**' + + - name: Set up Rust + if: steps.filter.outputs.rust == 'true' + uses: dtolnay/rust-toolchain@stable + with: + components: clippy, rustfmt + + - name: Rust Cache + if: steps.filter.outputs.rust == 'true' + uses: Swatinem/rust-cache@v2 + with: + workspaces: ublue/skillet + + - name: Run Clippy + if: steps.filter.outputs.rust == 'true' + run: cd ublue/skillet && cargo clippy -- -D warnings + + - name: Run Unit Tests + if: steps.filter.outputs.rust == 'true' + run: cd ublue/skillet && cargo test + + - name: Run Integration Tests + if: steps.filter.outputs.rust == 'true' + run: | + cd ublue/skillet + # Build binary explicitly for the test runner to find it + cargo build + ./target/debug/skillet test run beezelbot --image fedora:latest From c90f5f862a2e1249431509ccd05f624adc66ac17 Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 21 Mar 2026 22:18:01 +0100 Subject: [PATCH 05/19] fix: resolve clippy type complexity warning in test_utils --- ublue/skillet/crates/core/src/test_utils.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ublue/skillet/crates/core/src/test_utils.rs b/ublue/skillet/crates/core/src/test_utils.rs index 81d4466d..b5731c82 100644 --- a/ublue/skillet/crates/core/src/test_utils.rs +++ b/ublue/skillet/crates/core/src/test_utils.rs @@ -34,9 +34,11 @@ impl SystemResource for MockSystem { } } +pub type FileMetadata = (Option, Option, Option); + pub struct MockFiles { pub files: Arc>>>, - pub metadata: Arc, Option, Option)>>>, + pub metadata: Arc>>, } impl MockFiles { From 42b7e4b4a2c7e380fc40bcfb47c076c835fc0935 Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Mon, 23 Mar 2026 19:22:17 +0100 Subject: [PATCH 06/19] refactor: simplify integration test container setup - Moved complex shell logic to 'test_entrypoint.sh'. - Embedded entrypoint script into the binary. - Updated test runner to use the embedded script. --- ublue/skillet/crates/cli/src/main.rs | 54 ++++++++++++++++--- .../skillet/crates/cli/src/test_entrypoint.sh | 19 +++++++ 2 files changed, 67 insertions(+), 6 deletions(-) create mode 100644 ublue/skillet/crates/cli/src/test_entrypoint.sh diff --git a/ublue/skillet/crates/cli/src/main.rs b/ublue/skillet/crates/cli/src/main.rs index fca62aa9..62872ded 100644 --- a/ublue/skillet/crates/cli/src/main.rs +++ b/ublue/skillet/crates/cli/src/main.rs @@ -63,7 +63,8 @@ fn main() -> Result<()> { }) .finish(); - tracing::subscriber::set_global_default(subscriber).context("setting default subscriber failed")?; + tracing::subscriber::set_global_default(subscriber) + .context("setting default subscriber failed")?; match args.command { Commands::Apply { record } => handle_apply(record), @@ -179,16 +180,57 @@ fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<() } 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..."); - // Use 'skillet apply' directly as it's the interface for all our binaries now - // We ensure /etc/sysctl.d exists because many minimal container images lack it. let exec_status = Command::new("podman") .args([ "exec", &container_name, - "sh", - "-c", - "mkdir -p /etc/sysctl.d && skillet apply --record /tmp/ops.yaml", + "/tmp/test_entrypoint.sh", + "skillet", + "apply", + "--record", + "/tmp/ops.yaml", ]) .status() .context("Failed to exec skillet")?; diff --git a/ublue/skillet/crates/cli/src/test_entrypoint.sh b/ublue/skillet/crates/cli/src/test_entrypoint.sh new file mode 100644 index 00000000..285d649d --- /dev/null +++ b/ublue/skillet/crates/cli/src/test_entrypoint.sh @@ -0,0 +1,19 @@ +#!/bin/sh +set -e + +# Ensure /etc/sysctl.d exists (often missing in minimal containers) +mkdir -p /etc/sysctl.d + +# Mock systemctl if it doesn't exist +if [ ! -x /usr/bin/systemctl ]; then + echo "Mocking systemctl..." + cat < /usr/bin/systemctl +#!/bin/sh +echo "Mock systemctl: \$@" +exit 0 +EOF + chmod +x /usr/bin/systemctl +fi + +# Execute the passed command (skillet apply) +exec "$@" From a21ff6d915b302a35f5e5a5f8526ce867747af49 Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Mon, 23 Mar 2026 19:34:41 +0100 Subject: [PATCH 07/19] feat: implement service management and systemctl restart - 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. --- ublue/skillet/crates/core/src/recorder.rs | 21 ++++++ ublue/skillet/crates/core/src/resource_op.rs | 9 +++ ublue/skillet/crates/core/src/system.rs | 30 ++++++++- ublue/skillet/crates/core/src/system/tests.rs | 65 ++++++++++--------- ublue/skillet/crates/core/src/test_utils.rs | 26 ++++++++ ublue/skillet/crates/hardening/src/lib.rs | 15 ++++- ublue/skillet/crates/hardening/src/tests.rs | 9 +++ .../recordings/beezelbot.yaml | 2 + 8 files changed, 144 insertions(+), 33 deletions(-) diff --git a/ublue/skillet/crates/core/src/recorder.rs b/ublue/skillet/crates/core/src/recorder.rs index e8af59a2..89aa6b96 100644 --- a/ublue/skillet/crates/core/src/recorder.rs +++ b/ublue/skillet/crates/core/src/recorder.rs @@ -74,4 +74,25 @@ impl SystemResource for Recorder { }); self.inner.ensure_group(name) } + + fn service_start(&self, name: &str) -> Result<(), SystemError> { + self.record(ResourceOp::ServiceStart { + name: name.to_string(), + }); + self.inner.service_start(name) + } + + fn service_stop(&self, name: &str) -> Result<(), SystemError> { + self.record(ResourceOp::ServiceStop { + name: name.to_string(), + }); + self.inner.service_stop(name) + } + + fn service_restart(&self, name: &str) -> Result<(), SystemError> { + self.record(ResourceOp::ServiceRestart { + name: name.to_string(), + }); + self.inner.service_restart(name) + } } diff --git a/ublue/skillet/crates/core/src/resource_op.rs b/ublue/skillet/crates/core/src/resource_op.rs index da251c3f..f83845e5 100644 --- a/ublue/skillet/crates/core/src/resource_op.rs +++ b/ublue/skillet/crates/core/src/resource_op.rs @@ -15,4 +15,13 @@ pub enum ResourceOp { EnsureGroup { name: String, }, + ServiceStart { + name: String, + }, + ServiceStop { + name: String, + }, + ServiceRestart { + name: String, + }, } diff --git a/ublue/skillet/crates/core/src/system.rs b/ublue/skillet/crates/core/src/system.rs index baf98e7e..a4ed3715 100644 --- a/ublue/skillet/crates/core/src/system.rs +++ b/ublue/skillet/crates/core/src/system.rs @@ -15,6 +15,9 @@ pub enum SystemError { pub trait SystemResource { fn ensure_group(&self, name: &str) -> Result; + fn service_start(&self, name: &str) -> Result<(), SystemError>; + fn service_stop(&self, name: &str) -> Result<(), SystemError>; + fn service_restart(&self, name: &str) -> Result<(), SystemError>; } pub struct LinuxSystemResource; @@ -23,6 +26,20 @@ impl LinuxSystemResource { pub fn new() -> Self { Self } + + fn run_systemctl(&self, action: &str, name: &str) -> Result<(), SystemError> { + info!("Running systemctl {} {}", action, name); + let output = Command::new("systemctl").arg(action).arg(name).output()?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + return Err(SystemError::Command(format!( + "systemctl {} {} failed: {}", + action, name, stderr + ))); + } + Ok(()) + } } impl Default for LinuxSystemResource { @@ -56,8 +73,19 @@ impl SystemResource for LinuxSystemResource { info!("Created group {}", name); Ok(true) } -} + fn service_start(&self, name: &str) -> Result<(), SystemError> { + self.run_systemctl("start", name) + } + + fn service_stop(&self, name: &str) -> Result<(), SystemError> { + self.run_systemctl("stop", name) + } + + fn service_restart(&self, name: &str) -> Result<(), SystemError> { + self.run_systemctl("restart", name) + } +} #[cfg(test)] #[path = "system/tests.rs"] mod tests; diff --git a/ublue/skillet/crates/core/src/system/tests.rs b/ublue/skillet/crates/core/src/system/tests.rs index 5e100091..a34a9d79 100644 --- a/ublue/skillet/crates/core/src/system/tests.rs +++ b/ublue/skillet/crates/core/src/system/tests.rs @@ -1,39 +1,46 @@ use super::*; -use std::collections::HashSet; -use std::sync::{Arc, Mutex}; - -// Mock implementation for testing consumers -pub struct MockSystemResource { - pub groups: Arc>>, -} - -impl MockSystemResource { - pub fn new() -> Self { - Self { - groups: Arc::new(Mutex::new(HashSet::new())), - } - } -} - -impl SystemResource for MockSystemResource { - fn ensure_group(&self, name: &str) -> Result { - let mut groups = self.groups.lock().unwrap(); - if groups.contains(name) { - Ok(false) - } else { - groups.insert(name.to_string()); - Ok(true) - } - } -} +#[cfg(feature = "test-utils")] +use crate::test_utils::MockSystem; #[test] +#[cfg(feature = "test-utils")] fn test_mock_system_resource() { - let system = MockSystemResource::new(); + let system = MockSystem::new(); let changed = system.ensure_group("syslog").unwrap(); assert!(changed); - assert!(system.groups.lock().unwrap().contains("syslog")); + assert!(system + .groups + .lock() + .unwrap_or_else(|e| e.into_inner()) + .contains("syslog")); let changed_again = system.ensure_group("syslog").unwrap(); assert!(!changed_again); } + +#[test] +#[cfg(feature = "test-utils")] +fn test_mock_system_services() { + let system = MockSystem::new(); + system.service_start("test-service").unwrap(); + assert_eq!( + system + .services + .lock() + .unwrap_or_else(|e| e.into_inner()) + .get("test-service") + .unwrap(), + "started" + ); + + system.service_restart("test-service").unwrap(); + assert_eq!( + system + .services + .lock() + .unwrap_or_else(|e| e.into_inner()) + .get("test-service") + .unwrap(), + "restarted" + ); +} diff --git a/ublue/skillet/crates/core/src/test_utils.rs b/ublue/skillet/crates/core/src/test_utils.rs index b5731c82..724d42c7 100644 --- a/ublue/skillet/crates/core/src/test_utils.rs +++ b/ublue/skillet/crates/core/src/test_utils.rs @@ -6,12 +6,14 @@ use std::sync::{Arc, Mutex}; pub struct MockSystem { pub groups: Arc>>, + pub services: Arc>>, // name -> state (started, stopped, restarted) } impl MockSystem { pub fn new() -> Self { Self { groups: Arc::new(Mutex::new(HashSet::new())), + services: Arc::new(Mutex::new(HashMap::new())), } } } @@ -32,6 +34,30 @@ impl SystemResource for MockSystem { Ok(true) } } + + fn service_start(&self, name: &str) -> Result<(), SystemError> { + self.services + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(name.to_string(), "started".to_string()); + Ok(()) + } + + fn service_stop(&self, name: &str) -> Result<(), SystemError> { + self.services + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(name.to_string(), "stopped".to_string()); + Ok(()) + } + + fn service_restart(&self, name: &str) -> Result<(), SystemError> { + self.services + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(name.to_string(), "restarted".to_string()); + Ok(()) + } } pub type FileMetadata = (Option, Option, Option); diff --git a/ublue/skillet/crates/hardening/src/lib.rs b/ublue/skillet/crates/hardening/src/lib.rs index b372bfec..c8f6e0a3 100644 --- a/ublue/skillet/crates/hardening/src/lib.rs +++ b/ublue/skillet/crates/hardening/src/lib.rs @@ -20,7 +20,7 @@ where info!("Applying hardening..."); // 1. Sysctl hardening - apply_sysctl_hardening(files)?; + apply_sysctl_hardening(system, files)?; // 2. Include 'os-hardening' apply_os_hardening(system)?; @@ -34,12 +34,21 @@ where Ok(()) } -fn apply_sysctl_hardening(files: &F) -> Result<(), HardeningError> { +fn apply_sysctl_hardening(system: &S, files: &F) -> Result<(), HardeningError> +where + S: SystemResource + ?Sized, + F: FileResource + ?Sized, +{ info!("Applying sysctl hardening..."); let content = include_bytes!("../files/sysctl.boxy.conf"); let path = Path::new("/etc/sysctl.d/99-hardening.conf"); - files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; + let changed = files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; + + if changed { + info!("Sysctl configuration changed, restarting systemd-sysctl..."); + system.service_restart("systemd-sysctl")?; + } Ok(()) } diff --git a/ublue/skillet/crates/hardening/src/tests.rs b/ublue/skillet/crates/hardening/src/tests.rs index 3d35e61b..eed864d8 100644 --- a/ublue/skillet/crates/hardening/src/tests.rs +++ b/ublue/skillet/crates/hardening/src/tests.rs @@ -11,4 +11,13 @@ fn test_hardening_applies_sysctl() { .lock() .unwrap_or_else(|e| e.into_inner()) .contains_key("/etc/sysctl.d/99-hardening.conf")); + assert_eq!( + system + .services + .lock() + .unwrap_or_else(|e| e.into_inner()) + .get("systemd-sysctl") + .unwrap(), + "restarted" + ); } diff --git a/ublue/skillet/integration_tests/recordings/beezelbot.yaml b/ublue/skillet/integration_tests/recordings/beezelbot.yaml index 572f8ba3..090c8dd3 100644 --- a/ublue/skillet/integration_tests/recordings/beezelbot.yaml +++ b/ublue/skillet/integration_tests/recordings/beezelbot.yaml @@ -4,3 +4,5 @@ mode: '0o644' owner: root group: root +- !ServiceRestart + name: systemd-sysctl From dbefdc6781e6dcdef1fd22883b2150ff8468a06a Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Mon, 23 Mar 2026 20:20:11 +0100 Subject: [PATCH 08/19] refactor(skillet): address code review comments - 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 --- ublue/skillet/Cargo.lock | 31 ++- ublue/skillet/Cargo.toml | 1 + ublue/skillet/crates/cli-common/Cargo.toml | 2 +- ublue/skillet/crates/cli-common/src/lib.rs | 6 +- ublue/skillet/crates/cli/Cargo.toml | 3 +- ublue/skillet/crates/cli/src/main.rs | 297 ++++++++++----------- ublue/skillet/crates/hardening/Cargo.toml | 3 +- 7 files changed, 174 insertions(+), 169 deletions(-) diff --git a/ublue/skillet/Cargo.lock b/ublue/skillet/Cargo.lock index 036f3c07..ebb97afe 100644 --- a/ublue/skillet/Cargo.lock +++ b/ublue/skillet/Cargo.lock @@ -280,6 +280,16 @@ version = "0.2.183" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b5b646652bf6661599e1da8901b3b9522896f01e736bad5f723fe7a3a27f899d" +[[package]] +name = "libyml" +version = "0.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3302702afa434ffa30847a83305f0a69d6abd74293b6554c18ec85c7ef30c980" +dependencies = [ + "anyhow", + "version_check", +] + [[package]] name = "linux-raw-sys" version = "0.12.1" @@ -439,16 +449,18 @@ dependencies = [ ] [[package]] -name = "serde_yaml" -version = "0.9.34+deprecated" +name = "serde_yml" +version = "0.0.12" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6a8b1a1a2ebf674015cc02edccce75287f1a0130d394307b36743c2f5d504b47" +checksum = "59e2dd588bf1597a252c3b920e0143eb99b0f76e4e082f4c92ce34fbc9e71ddd" dependencies = [ "indexmap", "itoa", + "libyml", + "memchr", "ryu", "serde", - "unsafe-libyaml", + "version_check", ] [[package]] @@ -479,7 +491,8 @@ dependencies = [ "clap", "hex", "serde", - "serde_yaml", + "serde_yml", + "skillet_cli_common", "skillet_core", "skillet_hardening", "tempfile", @@ -501,7 +514,7 @@ version = "0.1.0" dependencies = [ "clap", "serde", - "serde_yaml", + "serde_yml", "skillet_core", "skillet_hardening", "thiserror", @@ -673,12 +686,6 @@ version = "0.2.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ebc1c04c71510c7f702b52b7c350734c9ff1295c464a03335b00bb84fc54f853" -[[package]] -name = "unsafe-libyaml" -version = "0.2.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "673aac59facbab8a9007c7f6108d11f63b603f7cabff99fabf650fea5c32b861" - [[package]] name = "users" version = "0.11.0" diff --git a/ublue/skillet/Cargo.toml b/ublue/skillet/Cargo.toml index 5c7bd3c4..0f4fd941 100644 --- a/ublue/skillet/Cargo.toml +++ b/ublue/skillet/Cargo.toml @@ -22,4 +22,5 @@ tracing-subscriber = "0.3" tempfile = "3.8" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" +serde_yml = "0.0.12" hex = "0.4" diff --git a/ublue/skillet/crates/cli-common/Cargo.toml b/ublue/skillet/crates/cli-common/Cargo.toml index 2babad52..3818197a 100644 --- a/ublue/skillet/crates/cli-common/Cargo.toml +++ b/ublue/skillet/crates/cli-common/Cargo.toml @@ -11,4 +11,4 @@ tracing.workspace = true tracing-subscriber.workspace = true thiserror.workspace = true serde.workspace = true -serde_yaml = "0.9" +serde_yml.workspace = true diff --git a/ublue/skillet/crates/cli-common/src/lib.rs b/ublue/skillet/crates/cli-common/src/lib.rs index 4b8fd280..86aa2dc7 100644 --- a/ublue/skillet/crates/cli-common/src/lib.rs +++ b/ublue/skillet/crates/cli-common/src/lib.rs @@ -18,7 +18,7 @@ pub enum CliCommonError { #[error("IO error: {0}")] Io(#[from] std::io::Error), #[error("Serialization error: {0}")] - Yaml(#[from] serde_yaml::Error), + Yaml(#[from] serde_yml::Error), } #[derive(Parser, Debug)] @@ -60,7 +60,7 @@ pub fn run_host(hostname: &str) -> Result<(), CliCommonError> { } } -fn handle_apply(hostname: &str, record_path: Option) -> Result<(), CliCommonError> { +pub fn handle_apply(hostname: &str, record_path: Option) -> Result<(), CliCommonError> { info!("Starting Skillet configuration for {}...", hostname); let system = LinuxSystemResource::new(); @@ -73,7 +73,7 @@ fn handle_apply(hostname: &str, record_path: Option) -> Result<(), CliC apply(&recorder_system, &recorder_files)?; let ops = recorder_system.get_ops(); - let yaml = serde_yaml::to_string(&ops)?; + let yaml = serde_yml::to_string(&ops)?; fs::write(&path, yaml)?; info!("Recording saved to {}", path.display()); } else { diff --git a/ublue/skillet/crates/cli/Cargo.toml b/ublue/skillet/crates/cli/Cargo.toml index 95f02f3e..4cccda54 100644 --- a/ublue/skillet/crates/cli/Cargo.toml +++ b/ublue/skillet/crates/cli/Cargo.toml @@ -6,11 +6,12 @@ edition = "2021" [dependencies] skillet_core.workspace = true skillet_hardening.workspace = true +skillet_cli_common.workspace = true clap.workspace = true tracing.workspace = true tracing-subscriber.workspace = true anyhow = "1.0" serde.workspace = true -serde_yaml = "0.9" +serde_yml.workspace = true hex.workspace = true tempfile.workspace = true diff --git a/ublue/skillet/crates/cli/src/main.rs b/ublue/skillet/crates/cli/src/main.rs index 62872ded..da445010 100644 --- a/ublue/skillet/crates/cli/src/main.rs +++ b/ublue/skillet/crates/cli/src/main.rs @@ -1,12 +1,9 @@ use anyhow::{anyhow, Context, Result}; use clap::{Parser, Subcommand}; -use skillet_core::files::LocalFileResource; -use skillet_core::recorder::Recorder; use skillet_core::resource_op::ResourceOp; -use skillet_core::system::LinuxSystemResource; -use skillet_hardening::apply; use std::fs; -use std::path::PathBuf; +use std::io::Write; +use std::path::{Path, PathBuf}; use std::process::Command; use tracing::{error, info, Level}; use tracing_subscriber::FmtSubscriber; @@ -67,32 +64,12 @@ fn main() -> Result<()> { .context("setting default subscriber failed")?; match args.command { - Commands::Apply { record } => handle_apply(record), - Commands::Test { test_command } => handle_test(test_command), - } -} - -fn handle_apply(record_path: Option) -> 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)?; + Commands::Apply { record } => { + skillet_cli_common::handle_apply("(Agent Mode)", record) + .map_err(|e| anyhow!("Failed to apply configuration: {}", e))?; + } + Commands::Test { test_command } => handle_test(test_command)?, } - - info!("Configuration applied successfully."); Ok(()) } @@ -114,7 +91,28 @@ fn handle_test(cmd: TestCommands) -> Result<()> { } fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<()> { - // 1. Build binary + build_workspace()?; + + let binary_path = locate_binary(hostname)?; + let container_name = format!("skillet-test-{}", hostname); + + setup_container(&container_name, image, &binary_path)?; + + let result = (|| -> Result<()> { + prepare_and_run_skillet(&container_name)?; + verify_or_record(hostname, &container_name, is_record)?; + Ok(()) + })(); + + info!("Stopping container..."); + let _ = Command::new("podman") + .args(["kill", &container_name]) + .output(); + + result +} + +fn build_workspace() -> Result<()> { info!("Building skillet workspace..."); let build_status = Command::new("cargo") .args(["build"]) @@ -124,8 +122,10 @@ fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<() if !build_status.success() { return Err(anyhow!("Build failed")); } + Ok(()) +} - // 2. Locate binary (with fallback) +fn locate_binary(hostname: &str) -> Result { let host_binary_name = format!("skillet-{}", hostname); let target_debug = PathBuf::from("target/debug"); @@ -146,17 +146,17 @@ fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<() binary_path.display() )); } - let abs_binary_path = fs::canonicalize(&binary_path)?; + fs::canonicalize(&binary_path).context("Failed to canonicalize binary path") +} - // 3. Start Container - let container_name = format!("skillet-test-{}", hostname); +fn setup_container(container_name: &str, image: &str, binary_path: &Path) -> Result<()> { info!( "Starting container {} from image {}...", container_name, image ); let _ = Command::new("podman") - .args(["rm", "-f", &container_name]) + .args(["rm", "-f", container_name]) .output(); let run_status = Command::new("podman") @@ -165,9 +165,9 @@ fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<() "-d", "--rm", "--name", - &container_name, + container_name, "-v", - &format!("{}:/usr/bin/skillet:ro", abs_binary_path.display()), + &format!("{}:/usr/bin/skillet:ro", binary_path.display()), image, "sleep", "infinity", @@ -178,133 +178,128 @@ fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<() if !run_status.success() { return Err(anyhow!("Failed to start container")); } + Ok(()) +} - 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"))?; +fn prepare_and_run_skillet(container_name: &str) -> Result<()> { + // Prepare entrypoint script + let entrypoint_content = include_str!("test_entrypoint.sh"); + let mut temp_entrypoint = tempfile::Builder::new().suffix(".sh").tempfile()?; + 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")); + } - // Copy entrypoint to container - info!("Copying test 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")); + } + Ok(()) +} + +fn verify_or_record(hostname: &str, container_name: &str, is_record: bool) -> Result<()> { + 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", - temp_entrypoint_path, - &format!("{}:/tmp/test_entrypoint.sh", container_name), + &format!("{}:/tmp/ops.yaml", container_name), + dest_file + .to_str() + .ok_or_else(|| anyhow!("Destination path is not valid UTF-8"))?, ]) - .status() - .context("Failed to copy entrypoint")?; + .status()?; 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")); + 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"))?; - info!("Executing skillet inside container..."); - let exec_status = Command::new("podman") + let cp_status = Command::new("podman") .args([ - "exec", - &container_name, - "/tmp/test_entrypoint.sh", - "skillet", - "apply", - "--record", - "/tmp/ops.yaml", + "cp", + &format!("{}:/tmp/ops.yaml", container_name), + temp_path, ]) - .status() - .context("Failed to exec skillet")?; - - if !exec_status.success() { - return Err(anyhow!("skillet apply failed inside container")); + .status()?; + if !cp_status.success() { + return Err(anyhow!("Failed to copy recording from 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")); - } + 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 = serde_yml::from_str(&recorded_content)?; + let new_ops: Vec = serde_yml::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!("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 = serde_yaml::from_str(&recorded_content)?; - let new_ops: Vec = 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!"); - } + info!("Integration test passed!"); } - - Ok(()) - })(); - - info!("Stopping container..."); - let _ = Command::new("podman") - .args(["kill", &container_name]) - .output(); - - result + } + Ok(()) } diff --git a/ublue/skillet/crates/hardening/Cargo.toml b/ublue/skillet/crates/hardening/Cargo.toml index ea4c8203..7e2dac0f 100644 --- a/ublue/skillet/crates/hardening/Cargo.toml +++ b/ublue/skillet/crates/hardening/Cargo.toml @@ -4,9 +4,10 @@ version = "0.1.0" edition = "2021" [dependencies] -skillet_core = { workspace = true, features = ["test-utils"] } +skillet_core.workspace = true thiserror.workspace = true tracing.workspace = true [dev-dependencies] +skillet_core = { workspace = true, features = ["test-utils"] } tempfile.workspace = true From 75a61550ae1f53ffb7b67ee6b0ca5b40f5ac3afd Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Fri, 3 Apr 2026 08:22:51 +0200 Subject: [PATCH 09/19] feat(skillet): implement ssh hardening and enforce pedantic clippy lints --- ublue/skillet/AGENTS.md | 2 +- ublue/skillet/Cargo.toml | 9 + ublue/skillet/crates/cli-common/Cargo.toml | 3 + ublue/skillet/crates/cli/Cargo.toml | 3 + ublue/skillet/crates/cli/src/main.rs | 28 ++- ublue/skillet/crates/core/Cargo.toml | 3 + ublue/skillet/crates/core/src/files.rs | 37 +++- ublue/skillet/crates/core/src/recorder.rs | 27 ++- ublue/skillet/crates/core/src/resource_op.rs | 8 +- ublue/skillet/crates/core/src/system.rs | 21 +-- ublue/skillet/crates/core/src/system/tests.rs | 6 +- ublue/skillet/crates/core/src/test_utils.rs | 32 +++- ublue/skillet/crates/hardening/Cargo.toml | 3 + .../skillet/crates/hardening/files/ssh_config | 100 ++++++++++ .../crates/hardening/files/sshd_config | 173 ++++++++++++++++++ ublue/skillet/crates/hardening/src/lib.rs | 50 +++-- ublue/skillet/crates/hardening/src/tests.rs | 39 +++- .../skillet/crates/hosts/beezelbot/Cargo.toml | 3 + 18 files changed, 482 insertions(+), 65 deletions(-) create mode 100644 ublue/skillet/crates/hardening/files/ssh_config create mode 100644 ublue/skillet/crates/hardening/files/sshd_config diff --git a/ublue/skillet/AGENTS.md b/ublue/skillet/AGENTS.md index e18d8009..3db8fdc1 100644 --- a/ublue/skillet/AGENTS.md +++ b/ublue/skillet/AGENTS.md @@ -21,7 +21,7 @@ This document defines the architectural mandates and project structure for `skil - **Abstractions**: Use Traits (e.g., `FileResource`, `SystemResource`) to allow for mocking in higher-level library tests. ### 4. Quality Control & Validation -- **Formatting & Linting**: Always run `cargo fmt` and `cargo clippy` after making changes to ensure code quality and consistency. +- **Formatting & Linting**: Always run `cargo fmt` and `cargo clippy` after making changes to ensure code quality and consistency. **Clippy MUST be run with `pedantic` lints enabled (configured in `Cargo.toml`).** - **Verification**: Always run both: - **Unit Tests**: `cargo test` across the workspace. - **Integration Tests**: `skillet test run ` for affected hosts to verify end-to-end correctness in a containerized environment. diff --git a/ublue/skillet/Cargo.toml b/ublue/skillet/Cargo.toml index 0f4fd941..4eb3947a 100644 --- a/ublue/skillet/Cargo.toml +++ b/ublue/skillet/Cargo.toml @@ -24,3 +24,12 @@ serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" serde_yml = "0.0.12" hex = "0.4" + +[workspace.lints.rust] +unsafe_code = "forbid" + +[workspace.lints.clippy] +pedantic = { level = "warn", priority = -1 } +missing_errors_doc = "allow" +must_use_candidate = "allow" +# Optional: explicitly allow some pedantic lints if they are too noisy diff --git a/ublue/skillet/crates/cli-common/Cargo.toml b/ublue/skillet/crates/cli-common/Cargo.toml index 3818197a..e91d6281 100644 --- a/ublue/skillet/crates/cli-common/Cargo.toml +++ b/ublue/skillet/crates/cli-common/Cargo.toml @@ -3,6 +3,9 @@ name = "skillet_cli_common" version = "0.1.0" edition = "2021" +[lints] +workspace = true + [dependencies] skillet_core.workspace = true skillet_hardening.workspace = true diff --git a/ublue/skillet/crates/cli/Cargo.toml b/ublue/skillet/crates/cli/Cargo.toml index 4cccda54..5cc13aaf 100644 --- a/ublue/skillet/crates/cli/Cargo.toml +++ b/ublue/skillet/crates/cli/Cargo.toml @@ -3,6 +3,9 @@ name = "skillet" version = "0.1.0" edition = "2021" +[lints] +workspace = true + [dependencies] skillet_core.workspace = true skillet_hardening.workspace = true diff --git a/ublue/skillet/crates/cli/src/main.rs b/ublue/skillet/crates/cli/src/main.rs index da445010..f505cfe3 100644 --- a/ublue/skillet/crates/cli/src/main.rs +++ b/ublue/skillet/crates/cli/src/main.rs @@ -66,7 +66,7 @@ fn main() -> Result<()> { match args.command { Commands::Apply { record } => { skillet_cli_common::handle_apply("(Agent Mode)", record) - .map_err(|e| anyhow!("Failed to apply configuration: {}", e))?; + .map_err(|e| anyhow!("Failed to apply configuration: {e}"))?; } Commands::Test { test_command } => handle_test(test_command)?, } @@ -94,7 +94,7 @@ fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<() build_workspace()?; let binary_path = locate_binary(hostname)?; - let container_name = format!("skillet-test-{}", hostname); + let container_name = format!("skillet-test-{hostname}"); setup_container(&container_name, image, &binary_path)?; @@ -126,16 +126,15 @@ fn build_workspace() -> Result<()> { } fn locate_binary(hostname: &str) -> Result { - let host_binary_name = format!("skillet-{}", hostname); + 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); + 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 + "Using generic skillet binary (host binary {host_binary_name} not found)" ); target_debug.join("skillet") }; @@ -151,8 +150,7 @@ fn locate_binary(hostname: &str) -> Result { fn setup_container(container_name: &str, image: &str, binary_path: &Path) -> Result<()> { info!( - "Starting container {} from image {}...", - container_name, image + "Starting container {container_name} from image {image}..." ); let _ = Command::new("podman") @@ -197,7 +195,7 @@ fn prepare_and_run_skillet(container_name: &str) -> Result<()> { .args([ "cp", temp_entrypoint_path, - &format!("{}:/tmp/test_entrypoint.sh", container_name), + &format!("{container_name}:/tmp/test_entrypoint.sh"), ]) .status() .context("Failed to copy entrypoint")?; @@ -245,14 +243,14 @@ fn prepare_and_run_skillet(container_name: &str) -> Result<()> { fn verify_or_record(hostname: &str, container_name: &str, is_record: bool) -> Result<()> { let dest_dir = PathBuf::from("integration_tests/recordings"); fs::create_dir_all(&dest_dir)?; - let dest_file = dest_dir.join(format!("{}.yaml", hostname)); + let dest_file = dest_dir.join(format!("{hostname}.yaml")); if is_record { info!("Copying recording to {}", dest_file.display()); let cp_status = Command::new("podman") .args([ "cp", - &format!("{}:/tmp/ops.yaml", container_name), + &format!("{container_name}:/tmp/ops.yaml"), dest_file .to_str() .ok_or_else(|| anyhow!("Destination path is not valid UTF-8"))?, @@ -273,7 +271,7 @@ fn verify_or_record(hostname: &str, container_name: &str, is_record: bool) -> Re let cp_status = Command::new("podman") .args([ "cp", - &format!("{}:/tmp/ops.yaml", container_name), + &format!("{container_name}:/tmp/ops.yaml"), temp_path, ]) .status()?; @@ -290,15 +288,15 @@ fn verify_or_record(hostname: &str, container_name: &str, is_record: bool) -> Re let recorded_ops: Vec = serde_yml::from_str(&recorded_content)?; let new_ops: Vec = serde_yml::from_str(&new_content)?; - if recorded_ops != new_ops { + if recorded_ops == new_ops { + info!("Integration test passed!"); + } else { 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(()) diff --git a/ublue/skillet/crates/core/Cargo.toml b/ublue/skillet/crates/core/Cargo.toml index cd8f2250..240af0c6 100644 --- a/ublue/skillet/crates/core/Cargo.toml +++ b/ublue/skillet/crates/core/Cargo.toml @@ -3,6 +3,9 @@ name = "skillet_core" version = "0.1.0" edition = "2021" +[lints] +workspace = true + [dependencies] thiserror.workspace = true sha2.workspace = true diff --git a/ublue/skillet/crates/core/src/files.rs b/ublue/skillet/crates/core/src/files.rs index 85891237..41e1ef78 100644 --- a/ublue/skillet/crates/core/src/files.rs +++ b/ublue/skillet/crates/core/src/files.rs @@ -40,6 +40,13 @@ pub trait FileResource { owner: Option<&str>, group: Option<&str>, ) -> Result; + fn ensure_directory( + &self, + path: &Path, + mode: Option, + owner: Option<&str>, + group: Option<&str>, + ) -> Result; fn delete_file(&self, path: &Path) -> Result; } @@ -51,7 +58,6 @@ impl LocalFileResource { } fn check_metadata( - &self, path: &Path, mode: Option, owner: Option<&str>, @@ -87,7 +93,6 @@ impl LocalFileResource { } fn apply_metadata( - &self, path: &Path, mode: Option, owner: Option<&str>, @@ -179,8 +184,8 @@ impl FileResource for LocalFileResource { } // 3. Check and apply metadata - if path.exists() && self.check_metadata(path, mode, owner, group)? { - self.apply_metadata(path, mode, owner, group)?; + if path.exists() && Self::check_metadata(path, mode, owner, group)? { + Self::apply_metadata(path, mode, owner, group)?; changed = true; info!("Updated file metadata for {}", path.display()); } @@ -188,6 +193,30 @@ impl FileResource for LocalFileResource { Ok(changed) } + fn ensure_directory( + &self, + path: &Path, + mode: Option, + owner: Option<&str>, + group: Option<&str>, + ) -> Result { + let mut changed = false; + + if !path.exists() { + fs::create_dir_all(path).map_err(FileError::Io)?; + changed = true; + info!("Created directory {}", path.display()); + } + + if path.exists() && Self::check_metadata(path, mode, owner, group)? { + Self::apply_metadata(path, mode, owner, group)?; + changed = true; + info!("Updated directory metadata for {}", path.display()); + } + + Ok(changed) + } + fn delete_file(&self, path: &Path) -> Result { if path.exists() { fs::remove_file(path).map_err(FileError::Io)?; diff --git a/ublue/skillet/crates/core/src/recorder.rs b/ublue/skillet/crates/core/src/recorder.rs index 89aa6b96..18cb9127 100644 --- a/ublue/skillet/crates/core/src/recorder.rs +++ b/ublue/skillet/crates/core/src/recorder.rs @@ -23,7 +23,7 @@ impl Recorder { } pub fn get_ops(&self) -> Vec { - self.ops.lock().unwrap_or_else(|e| e.into_inner()).clone() + self.ops.lock().unwrap_or_else(std::sync::PoisonError::into_inner).clone() } pub fn shared_ops(&self) -> Arc>> { @@ -31,7 +31,7 @@ impl Recorder { } fn record(&self, op: ResourceOp) { - self.ops.lock().unwrap_or_else(|e| e.into_inner()).push(op); + self.ops.lock().unwrap_or_else(std::sync::PoisonError::into_inner).push(op); } } @@ -51,14 +51,31 @@ impl FileResource for Recorder { self.record(ResourceOp::EnsureFile { path: path.display().to_string(), content_hash: hash, - mode: mode.map(|m| format!("0o{:o}", m)), - owner: owner.map(|s| s.to_string()), - group: group.map(|s| s.to_string()), + mode: mode.map(|m| format!("0o{m:o}")), + owner: owner.map(ToString::to_string), + group: group.map(ToString::to_string), }); self.inner.ensure_file(path, content, mode, owner, group) } + fn ensure_directory( + &self, + path: &Path, + mode: Option, + owner: Option<&str>, + group: Option<&str>, + ) -> Result { + self.record(ResourceOp::EnsureDirectory { + path: path.display().to_string(), + mode: mode.map(|m| format!("0o{m:o}")), + owner: owner.map(ToString::to_string), + group: group.map(ToString::to_string), + }); + + self.inner.ensure_directory(path, mode, owner, group) + } + fn delete_file(&self, path: &Path) -> Result { self.record(ResourceOp::DeleteFile { path: path.display().to_string(), diff --git a/ublue/skillet/crates/core/src/resource_op.rs b/ublue/skillet/crates/core/src/resource_op.rs index f83845e5..1896bcdb 100644 --- a/ublue/skillet/crates/core/src/resource_op.rs +++ b/ublue/skillet/crates/core/src/resource_op.rs @@ -1,6 +1,6 @@ use serde::{Deserialize, Serialize}; -#[derive(Serialize, Deserialize, PartialEq, Debug, Clone)] +#[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone)] pub enum ResourceOp { EnsureFile { path: String, @@ -12,6 +12,12 @@ pub enum ResourceOp { DeleteFile { path: String, }, + EnsureDirectory { + path: String, + mode: Option, + owner: Option, + group: Option, + }, EnsureGroup { name: String, }, diff --git a/ublue/skillet/crates/core/src/system.rs b/ublue/skillet/crates/core/src/system.rs index a4ed3715..d4df4569 100644 --- a/ublue/skillet/crates/core/src/system.rs +++ b/ublue/skillet/crates/core/src/system.rs @@ -27,15 +27,14 @@ impl LinuxSystemResource { Self } - fn run_systemctl(&self, action: &str, name: &str) -> Result<(), SystemError> { - info!("Running systemctl {} {}", action, name); + fn run_systemctl(action: &str, name: &str) -> Result<(), SystemError> { + info!("Running systemctl {action} {name}"); let output = Command::new("systemctl").arg(action).arg(name).output()?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); return Err(SystemError::Command(format!( - "systemctl {} {} failed: {}", - action, name, stderr + "systemctl {action} {name} failed: {stderr}" ))); } Ok(()) @@ -52,13 +51,13 @@ impl SystemResource for LinuxSystemResource { fn ensure_group(&self, name: &str) -> Result { // 1. Check if group exists using `users` crate if get_group_by_name(name).is_some() { - debug!("Group {} already exists", name); + debug!("Group {name} already exists"); return Ok(false); } // 2. Create group using `groupadd` // Note: Creating groups requires root privileges usually. - info!("Creating group {}", name); + info!("Creating group {name}"); let output = Command::new("groupadd") .arg(name) // .arg("-r") // System group? Maybe make it an option? @@ -67,23 +66,23 @@ impl SystemResource for LinuxSystemResource { if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); - return Err(SystemError::Command(format!("groupadd failed: {}", stderr))); + return Err(SystemError::Command(format!("groupadd failed: {stderr}"))); } - info!("Created group {}", name); + info!("Created group {name}"); Ok(true) } fn service_start(&self, name: &str) -> Result<(), SystemError> { - self.run_systemctl("start", name) + Self::run_systemctl("start", name) } fn service_stop(&self, name: &str) -> Result<(), SystemError> { - self.run_systemctl("stop", name) + Self::run_systemctl("stop", name) } fn service_restart(&self, name: &str) -> Result<(), SystemError> { - self.run_systemctl("restart", name) + Self::run_systemctl("restart", name) } } #[cfg(test)] diff --git a/ublue/skillet/crates/core/src/system/tests.rs b/ublue/skillet/crates/core/src/system/tests.rs index a34a9d79..8c50d2ae 100644 --- a/ublue/skillet/crates/core/src/system/tests.rs +++ b/ublue/skillet/crates/core/src/system/tests.rs @@ -11,7 +11,7 @@ fn test_mock_system_resource() { assert!(system .groups .lock() - .unwrap_or_else(|e| e.into_inner()) + .unwrap_or_else(std::sync::PoisonError::into_inner) .contains("syslog")); let changed_again = system.ensure_group("syslog").unwrap(); @@ -27,7 +27,7 @@ fn test_mock_system_services() { system .services .lock() - .unwrap_or_else(|e| e.into_inner()) + .unwrap_or_else(std::sync::PoisonError::into_inner) .get("test-service") .unwrap(), "started" @@ -38,7 +38,7 @@ fn test_mock_system_services() { system .services .lock() - .unwrap_or_else(|e| e.into_inner()) + .unwrap_or_else(std::sync::PoisonError::into_inner) .get("test-service") .unwrap(), "restarted" diff --git a/ublue/skillet/crates/core/src/test_utils.rs b/ublue/skillet/crates/core/src/test_utils.rs index 724d42c7..f4c5b1c4 100644 --- a/ublue/skillet/crates/core/src/test_utils.rs +++ b/ublue/skillet/crates/core/src/test_utils.rs @@ -26,7 +26,7 @@ impl Default for MockSystem { impl SystemResource for MockSystem { fn ensure_group(&self, name: &str) -> Result { - let mut groups = self.groups.lock().unwrap_or_else(|e| e.into_inner()); + let mut groups = self.groups.lock().unwrap_or_else(std::sync::PoisonError::into_inner); if groups.contains(name) { Ok(false) } else { @@ -38,7 +38,7 @@ impl SystemResource for MockSystem { fn service_start(&self, name: &str) -> Result<(), SystemError> { self.services .lock() - .unwrap_or_else(|e| e.into_inner()) + .unwrap_or_else(std::sync::PoisonError::into_inner) .insert(name.to_string(), "started".to_string()); Ok(()) } @@ -46,7 +46,7 @@ impl SystemResource for MockSystem { fn service_stop(&self, name: &str) -> Result<(), SystemError> { self.services .lock() - .unwrap_or_else(|e| e.into_inner()) + .unwrap_or_else(std::sync::PoisonError::into_inner) .insert(name.to_string(), "stopped".to_string()); Ok(()) } @@ -54,7 +54,7 @@ impl SystemResource for MockSystem { fn service_restart(&self, name: &str) -> Result<(), SystemError> { self.services .lock() - .unwrap_or_else(|e| e.into_inner()) + .unwrap_or_else(std::sync::PoisonError::into_inner) .insert(name.to_string(), "restarted".to_string()); Ok(()) } @@ -92,8 +92,8 @@ impl FileResource for MockFiles { group: Option<&str>, ) -> Result { let path_str = path.display().to_string(); - let mut files = self.files.lock().unwrap_or_else(|e| e.into_inner()); - let mut metadata = self.metadata.lock().unwrap_or_else(|e| e.into_inner()); + let mut files = self.files.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let mut metadata = self.metadata.lock().unwrap_or_else(std::sync::PoisonError::into_inner); let mut changed = false; @@ -109,8 +109,8 @@ impl FileResource for MockFiles { let new_meta = ( mode, - owner.map(|s| s.to_string()), - group.map(|s| s.to_string()), + owner.map(ToString::to_string), + group.map(ToString::to_string), ); if let Some(existing_meta) = metadata.get(&path_str) { if existing_meta != &new_meta { @@ -125,10 +125,22 @@ impl FileResource for MockFiles { Ok(changed) } + fn ensure_directory( + &self, + _path: &Path, + _mode: Option, + _owner: Option<&str>, + _group: Option<&str>, + ) -> Result { + // 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) + } + fn delete_file(&self, path: &Path) -> Result { let path_str = path.display().to_string(); - let mut files = self.files.lock().unwrap_or_else(|e| e.into_inner()); - let mut metadata = self.metadata.lock().unwrap_or_else(|e| e.into_inner()); + let mut files = self.files.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let mut metadata = self.metadata.lock().unwrap_or_else(std::sync::PoisonError::into_inner); let f_removed = files.remove(&path_str).is_some(); let m_removed = metadata.remove(&path_str).is_some(); diff --git a/ublue/skillet/crates/hardening/Cargo.toml b/ublue/skillet/crates/hardening/Cargo.toml index 7e2dac0f..6d818c18 100644 --- a/ublue/skillet/crates/hardening/Cargo.toml +++ b/ublue/skillet/crates/hardening/Cargo.toml @@ -3,6 +3,9 @@ name = "skillet_hardening" version = "0.1.0" edition = "2021" +[lints] +workspace = true + [dependencies] skillet_core.workspace = true thiserror.workspace = true diff --git a/ublue/skillet/crates/hardening/files/ssh_config b/ublue/skillet/crates/hardening/files/ssh_config new file mode 100644 index 00000000..b9e0a37f --- /dev/null +++ b/ublue/skillet/crates/hardening/files/ssh_config @@ -0,0 +1,100 @@ +# **Note:** This file was automatically created by Hardening Framework (dev-sec.io) configuration. If you use its automated setup, do not edit this file directly, but adjust the automation instead. +#--- + +# This is the ssh client system-wide configuration file. +# See ssh_config(5) for more information on any settings used. Comments will be added only to clarify why a configuration was chosen. +# +# Created for OpenSSH v5.9 up to 6.8 + +# Basic configuration +# =================== + +# Address family should always be limited to the active network configuration. +AddressFamily any + + +# The port at the destination should be defined +Port 22 + +# Identity file configuration. You may restrict available identity files. Otherwise ssh will search for a pattern and use any that matches. +#IdentityFile ~/.ssh/identity +#IdentityFile ~/.ssh/id_rsa +#IdentityFile ~/.ssh/id_dsa + + +# Security configuration +# ====================== + +# Set the protocol version to 2 for security reasons. Disables legacy support. +Protocol 2 + +# Make sure passphrase querying is enabled +BatchMode no + +# Prevent IP spoofing by checking to host IP against the `known_hosts` file. +CheckHostIP yes + +# Always ask before adding keys to the `known_hosts` file. Do not set to `yes`. +StrictHostKeyChecking ask + +# **Ciphers** -- If your clients don't support CTR (eg older versions), cbc will be added +# CBC: is true if you want to connect with OpenSSL-base libraries +# eg ruby Net::SSH::Transport::CipherFactory requires cbc-versions of the given openssh ciphers to work +# -- see: (http://net-ssh.github.com/net-ssh/classes/Net/SSH/Transport/CipherFactory.html) +# +Ciphers chacha20-poly1305@openssh.com,aes256-gcm@openssh.com,aes128-gcm@openssh.com,aes256-ctr,aes192-ctr,aes128-ctr + +# **Hash algorithms** -- Make sure not to use SHA1 for hashing, unless it is really necessary. +# Weak HMAC is sometimes required if older package versions are used +# eg Ruby's Net::SSH at around 2.2.* doesn't support sha2 for hmac, so this will have to be set true in this case. +# +MACs hmac-sha2-512-etm@openssh.com,hmac-sha2-256-etm@openssh.com,umac-128-etm@openssh.com,hmac-sha2-512,hmac-sha2-256 + +# Alternative setting, if OpenSSH version is below v5.9 +#MACs hmac-ripemd160 + +# **Key Exchange Algorithms** -- Make sure not to use SHA1 for kex, unless it is really necessary +# Weak kex is sometimes required if older package versions are used +# eg ruby's Net::SSH at around 2.2.* doesn't support sha2 for kex, so this will have to be set true in this case. +# +KexAlgorithms curve25519-sha256@libssh.org,diffie-hellman-group-exchange-sha256 + + +# Disable agent formwarding, since local agent could be accessed through forwarded connection. +ForwardAgent no + +# Disable X11 forwarding, since local X11 display could be accessed through forwarded connection. +ForwardX11 no + +# Never use host-based authentication. It can be exploited. +HostbasedAuthentication no + + +# Disable password-based authentication, it can allow for potentially easier brute-force attacks. +PasswordAuthentication no + +# Only use GSSAPIAuthentication if implemented on the network. +GSSAPIAuthentication no +GSSAPIDelegateCredentials no + +# Disable tunneling +Tunnel no + +# Disable local command execution. +PermitLocalCommand no + + +# Misc. configuration +# =================== + +# Enable compression. More pressure on the CPU, less on the network. +Compression yes + +#EscapeChar ~ +#VisualHostKey yes + +# http://undeadly.org/cgi?action=article&sid=20160114142733 +UseRoaming no + +# Send locale environment variables +SendEnv LANG LC_* LANGUAGE diff --git a/ublue/skillet/crates/hardening/files/sshd_config b/ublue/skillet/crates/hardening/files/sshd_config new file mode 100644 index 00000000..ec954166 --- /dev/null +++ b/ublue/skillet/crates/hardening/files/sshd_config @@ -0,0 +1,173 @@ +# **Note:** This file was automatically created by Hardening Framework (dev-sec.io) configuration. If you use its automated setup, do not edit this file directly, but adjust the automation instead. +#--- + +# This is the ssh client system-wide configuration file. +# See sshd_config(5) for more information on any settings used. Comments will be added only to clarify why a configuration was chosen. +# +# Created for OpenSSH v5.9 up to 6.8 + +# Basic configuration +# =================== + +# Either disable or only allow root login via certificates. +PermitRootLogin without-password + +# Define which port sshd should listen to. Default to `22`. +Port 22 + +# Address family should always be limited to the active network configuration. +AddressFamily any + +# Define which addresses sshd should listen to. Default to `0.0.0.0`, ie make sure you put your desired address in here, since otherwise sshd will listen to everyone. +ListenAddress 0.0.0.0 +ListenAddress :: + +# List HostKeys here. +HostKey /etc/ssh/ssh_host_rsa_key # Req 20 +HostKey /etc/ssh/ssh_host_ecdsa_key # Req 20 +HostKey /etc/ssh/ssh_host_ed25519_key # Req 20 + + +# Security configuration +# ====================== + +# Set the protocol version to 2 for security reasons. Disables legacy support. +Protocol 2 + +# Make sure sshd checks file modes and ownership before accepting logins. This prevents accidental misconfiguration. +StrictModes yes + +# Logging, obsoletes QuietMode and FascistLogging +SyslogFacility AUTH +LogLevel VERBOSE + +# Cryptography +# ------------ + +# **Ciphers** -- If your clients don't support CTR (eg older versions), cbc will be added +# CBC: is true if you want to connect with OpenSSL-base libraries +# eg ruby Net::SSH::Transport::CipherFactory requires cbc-versions of the given openssh ciphers to work +# -- see: (http://net-ssh.github.com/net-ssh/classes/Net/SSH/Transport/CipherFactory.html) +# +Ciphers chacha20-poly1305@openssh.com,aes256-gcm@openssh.com,aes128-gcm@openssh.com,aes256-ctr,aes192-ctr,aes128-ctr + +# **Hash algorithms** -- Make sure not to use SHA1 for hashing, unless it is really necessary. +# Weak HMAC is sometimes required if older package versions are used +# eg Ruby's Net::SSH at around 2.2.* doesn't support sha2 for hmac, so this will have to be set true in this case. +# +MACs hmac-sha2-512-etm@openssh.com,hmac-sha2-256-etm@openssh.com,umac-128-etm@openssh.com,hmac-sha2-512,hmac-sha2-256 + +# Alternative setting, if OpenSSH version is below v5.9 +#MACs hmac-ripemd160 + +# **Key Exchange Algorithms** -- Make sure not to use SHA1 for kex, unless it is really necessary +# Weak kex is sometimes required if older package versions are used +# eg ruby's Net::SSH at around 2.2.* doesn't support sha2 for kex, so this will have to be set true in this case. +# based on: https://bettercrypto.org/static/applied-crypto-hardening.pdf +KexAlgorithms curve25519-sha256@libssh.org,diffie-hellman-group-exchange-sha256 + +# Authentication +# -------------- + +# Secure Login directives. +PermitUserEnvironment no +LoginGraceTime 30s +MaxAuthTries 2 +MaxSessions 10 +MaxStartups 10:30:100 + +# Enable public key authentication +PubkeyAuthentication yes + + +# Never use host-based authentication. It can be exploited. +IgnoreRhosts yes +IgnoreUserKnownHosts yes +HostbasedAuthentication no + +# Enable PAM to enforce system wide rules +UsePAM yes +# Disable password-based authentication, it can allow for potentially easier brute-force attacks. +PasswordAuthentication no +PermitEmptyPasswords no +ChallengeResponseAuthentication no + +# Only enable Kerberos authentication if it is configured. +KerberosAuthentication no +KerberosOrLocalPasswd no +KerberosTicketCleanup yes +#KerberosGetAFSToken no + +# Only enable GSSAPI authentication if it is configured. +GSSAPIAuthentication no +GSSAPICleanupCredentials yes + +#DenyUsers * +#AllowUsers user1 +#DenyGroups * +#AllowGroups group1 + + +# Network +# ------- + +# Disable TCP keep alive since it is spoofable. Use ClientAlive messages instead, they use the encrypted channel +TCPKeepAlive no + +# Manage `ClientAlive..` signals via interval and maximum count. This will periodically check up to a `..CountMax` number of times within `..Interval` timeframe, and abort the connection once these fail. +ClientAliveInterval 300 +ClientAliveCountMax 3 + +# Disable tunneling +PermitTunnel no + +# Disable forwarding tcp connections. +# no real advantage without denied shell access +AllowTcpForwarding yes + +# Disable agent formwarding, since local agent could be accessed through forwarded connection. +# no real advantage without denied shell access +AllowAgentForwarding no + +# Do not allow remote port forwardings to bind to non-loopback addresses. +GatewayPorts no + +# Disable X11 forwarding, since local X11 display could be accessed through forwarded connection. +X11Forwarding no +X11UseLocalhost yes + + +# Misc. configuration +# =================== + + +PrintMotd no +PrintLastLog no +Banner none + + +# Since OpenSSH 6.8, this value defaults to 'no' +#UseDNS no +#PidFile /var/run/sshd.pid +#MaxStartups 10 +#ChrootDirectory none +#ChrootDirectory /home/%u + +# Accept locale environment variables +AcceptEnv LANG LC_* LANGUAGE + + +# Configuration, in case SFTP is used +## override default of no subsystems +## Subsystem sftp /opt/app/openssh5/libexec/sftp-server +Subsystem sftp internal-sftp -l VERBOSE + +## These lines must appear at the *end* of sshd_config +Match Group sftponly + ForceCommand internal-sftp -l VERBOSE + ChrootDirectory /home/%u + AllowTcpForwarding no + AllowAgentForwarding no + PasswordAuthentication no + PermitRootLogin no + X11Forwarding no diff --git a/ublue/skillet/crates/hardening/src/lib.rs b/ublue/skillet/crates/hardening/src/lib.rs index c8f6e0a3..260df8f3 100644 --- a/ublue/skillet/crates/hardening/src/lib.rs +++ b/ublue/skillet/crates/hardening/src/lib.rs @@ -23,13 +23,13 @@ where apply_sysctl_hardening(system, files)?; // 2. Include 'os-hardening' - apply_os_hardening(system)?; + apply_os_hardening(system); // 3. Include 'ssh-hardening::server' - apply_ssh_hardening_server(system)?; + apply_ssh_hardening_server(system, files)?; // 4. Include 'ssh-hardening::client' - apply_ssh_hardening_client(system)?; + apply_ssh_hardening_client(system, files)?; Ok(()) } @@ -53,22 +53,46 @@ where Ok(()) } -fn apply_os_hardening(_system: &S) -> Result<(), HardeningError> { +fn apply_os_hardening(_system: &S) { info!("(Placeholder) Applying os-hardening"); - Ok(()) } -fn apply_ssh_hardening_server( - _system: &S, -) -> Result<(), HardeningError> { - info!("(Placeholder) Applying ssh-hardening::server"); +fn apply_ssh_hardening_server(system: &S, files: &F) -> Result<(), HardeningError> +where + S: SystemResource + ?Sized, + F: FileResource + ?Sized, +{ + info!("Applying ssh-hardening::server"); + let ssh_dir = Path::new("/etc/ssh"); + files.ensure_directory(ssh_dir, Some(0o755), Some("root"), Some("root"))?; + + let content = include_bytes!("../files/sshd_config"); + let path = Path::new("/etc/ssh/sshd_config"); + + let changed = files.ensure_file(path, content, Some(0o600), Some("root"), Some("root"))?; + + if changed { + info!("SSH server configuration changed, restarting sshd..."); + system.service_restart("sshd")?; + } + Ok(()) } -fn apply_ssh_hardening_client( - _system: &S, -) -> Result<(), HardeningError> { - info!("(Placeholder) Applying ssh-hardening::client"); +fn apply_ssh_hardening_client(_system: &S, files: &F) -> Result<(), HardeningError> +where + S: SystemResource + ?Sized, + F: FileResource + ?Sized, +{ + info!("Applying ssh-hardening::client"); + let ssh_dir = Path::new("/etc/ssh"); + files.ensure_directory(ssh_dir, Some(0o755), Some("root"), Some("root"))?; + + let content = include_bytes!("../files/ssh_config"); + let path = Path::new("/etc/ssh/ssh_config"); + + files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; + Ok(()) } diff --git a/ublue/skillet/crates/hardening/src/tests.rs b/ublue/skillet/crates/hardening/src/tests.rs index eed864d8..6ef71625 100644 --- a/ublue/skillet/crates/hardening/src/tests.rs +++ b/ublue/skillet/crates/hardening/src/tests.rs @@ -9,15 +9,50 @@ fn test_hardening_applies_sysctl() { assert!(files .files .lock() - .unwrap_or_else(|e| e.into_inner()) + .unwrap_or_else(std::sync::PoisonError::into_inner) .contains_key("/etc/sysctl.d/99-hardening.conf")); assert_eq!( system .services .lock() - .unwrap_or_else(|e| e.into_inner()) + .unwrap_or_else(std::sync::PoisonError::into_inner) .get("systemd-sysctl") .unwrap(), "restarted" ); } + +#[test] +fn test_hardening_applies_ssh_server() { + let system = MockSystem::new(); + let files = MockFiles::new(); + apply(&system, &files).unwrap(); + let files_map = files.files.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + assert!(files_map.contains_key("/etc/ssh/sshd_config")); + + let content = String::from_utf8(files_map.get("/etc/ssh/sshd_config").unwrap().clone()).unwrap(); + assert!(content.contains("PermitRootLogin without-password")); + assert!(content.contains("Ciphers chacha20-poly1305@openssh.com")); + + assert_eq!( + system + .services + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .get("sshd") + .unwrap(), + "restarted" + ); +} + +#[test] +fn test_hardening_applies_ssh_client() { + let system = MockSystem::new(); + let files = MockFiles::new(); + apply(&system, &files).unwrap(); + let files_map = files.files.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + assert!(files_map.contains_key("/etc/ssh/ssh_config")); + + let content = String::from_utf8(files_map.get("/etc/ssh/ssh_config").unwrap().clone()).unwrap(); + assert!(content.contains("StrictHostKeyChecking ask")); +} diff --git a/ublue/skillet/crates/hosts/beezelbot/Cargo.toml b/ublue/skillet/crates/hosts/beezelbot/Cargo.toml index 666fe7f9..4292cd45 100644 --- a/ublue/skillet/crates/hosts/beezelbot/Cargo.toml +++ b/ublue/skillet/crates/hosts/beezelbot/Cargo.toml @@ -3,6 +3,9 @@ name = "skillet-beezelbot" version = "0.1.0" edition = "2021" +[lints] +workspace = true + [dependencies] skillet_cli_common.workspace = true anyhow = "1.0" From 7dc9b5e19f74cf02479a054df8395304526c30da Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Fri, 3 Apr 2026 08:40:39 +0200 Subject: [PATCH 10/19] test(skillet): update beezelbot integration test recording --- .../recordings/beezelbot.yaml | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/ublue/skillet/integration_tests/recordings/beezelbot.yaml b/ublue/skillet/integration_tests/recordings/beezelbot.yaml index 090c8dd3..09b6b627 100644 --- a/ublue/skillet/integration_tests/recordings/beezelbot.yaml +++ b/ublue/skillet/integration_tests/recordings/beezelbot.yaml @@ -6,3 +6,27 @@ group: root - !ServiceRestart name: systemd-sysctl +- !EnsureDirectory + path: /etc/ssh + mode: '0o755' + owner: root + group: root +- !EnsureFile + path: /etc/ssh/sshd_config + content_hash: '1355f199c4b2ed28c09c1cc2c7fc6fa44690f9b77d01412013f08118faa7b42b' + mode: '0o600' + owner: root + group: root +- !ServiceRestart + name: sshd +- !EnsureDirectory + path: /etc/ssh + mode: '0o755' + owner: root + group: root +- !EnsureFile + path: /etc/ssh/ssh_config + content_hash: b1c686c7da8fcea74e83f6a2dbd5552f2fb16a58601f347058b5ba4529e6d602 + mode: '0o644' + owner: root + group: root From 2afcbbda077155cd5399ea6000e16b327b474808 Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Fri, 3 Apr 2026 08:47:17 +0200 Subject: [PATCH 11/19] refactor(skillet): address second round of review comments --- ublue/skillet/crates/cli-common/src/lib.rs | 3 ++ ublue/skillet/crates/cli/src/main.rs | 39 ++++++++++++---------- ublue/skillet/crates/core/src/files.rs | 24 ++++++------- 3 files changed, 35 insertions(+), 31 deletions(-) diff --git a/ublue/skillet/crates/cli-common/src/lib.rs b/ublue/skillet/crates/cli-common/src/lib.rs index 86aa2dc7..9c6c7d6a 100644 --- a/ublue/skillet/crates/cli-common/src/lib.rs +++ b/ublue/skillet/crates/cli-common/src/lib.rs @@ -74,6 +74,9 @@ pub fn handle_apply(hostname: &str, record_path: Option) -> Result<(), let ops = recorder_system.get_ops(); let yaml = serde_yml::to_string(&ops)?; + if let Some(parent) = path.parent() { + fs::create_dir_all(parent)?; + } fs::write(&path, yaml)?; info!("Recording saved to {}", path.display()); } else { diff --git a/ublue/skillet/crates/cli/src/main.rs b/ublue/skillet/crates/cli/src/main.rs index f505cfe3..c75653bd 100644 --- a/ublue/skillet/crates/cli/src/main.rs +++ b/ublue/skillet/crates/cli/src/main.rs @@ -127,24 +127,29 @@ fn build_workspace() -> Result<()> { fn locate_binary(hostname: &str) -> Result { let host_binary_name = format!("skillet-{hostname}"); - let target_debug = PathBuf::from("target/debug"); + 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 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() - )); - } + info!("Using binary: {}", binary_path.display()); fs::canonicalize(&binary_path).context("Failed to canonicalize binary path") } diff --git a/ublue/skillet/crates/core/src/files.rs b/ublue/skillet/crates/core/src/files.rs index 41e1ef78..a1eb3c5d 100644 --- a/ublue/skillet/crates/core/src/files.rs +++ b/ublue/skillet/crates/core/src/files.rs @@ -1,5 +1,4 @@ use nix::unistd::{chown, Gid, Uid}; -use sha2::{Digest, Sha256}; use std::fs::{self}; use std::io::{self, Write}; use std::os::unix::fs::{MetadataExt, PermissionsExt}; @@ -68,7 +67,7 @@ impl LocalFileResource { let mut changed = false; if let Some(desired_mode) = mode { - if (metadata.permissions().mode() & 0o777) != desired_mode { + if (metadata.permissions().mode() & 0o7777) != desired_mode { changed = true; } } @@ -156,18 +155,15 @@ impl FileResource for LocalFileResource { // 2. Check content let content_changed = if path.exists() { - 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 { + let existing_content = + fs::read(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; + existing_content != content + } else { + true + } } else { true }; From db991753c83ced154602ffc012d2d4c60c1558c8 Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 4 Apr 2026 14:54:42 +0200 Subject: [PATCH 12/19] refactor(skillet): address review comments including systemd DBus interaction --- ublue/skillet/Cargo.toml | 1 + ublue/skillet/crates/cli-common/src/lib.rs | 2 + ublue/skillet/crates/core/Cargo.toml | 1 + ublue/skillet/crates/core/src/files.rs | 8 +- ublue/skillet/crates/core/src/system.rs | 79 +++++++++++++++---- ublue/skillet/crates/hardening/src/lib.rs | 7 +- .../recordings/beezelbot.yaml | 5 ++ 7 files changed, 84 insertions(+), 19 deletions(-) diff --git a/ublue/skillet/Cargo.toml b/ublue/skillet/Cargo.toml index 4eb3947a..c7b8661b 100644 --- a/ublue/skillet/Cargo.toml +++ b/ublue/skillet/Cargo.toml @@ -24,6 +24,7 @@ serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" serde_yml = "0.0.12" hex = "0.4" +zbus = { version = "4.3", features = ["blocking"] } [workspace.lints.rust] unsafe_code = "forbid" diff --git a/ublue/skillet/crates/cli-common/src/lib.rs b/ublue/skillet/crates/cli-common/src/lib.rs index 9c6c7d6a..8ab6118d 100644 --- a/ublue/skillet/crates/cli-common/src/lib.rs +++ b/ublue/skillet/crates/cli-common/src/lib.rs @@ -13,6 +13,8 @@ use tracing_subscriber::FmtSubscriber; pub enum CliCommonError { #[error("Failed to apply hardening: {0}")] Hardening(#[from] skillet_hardening::HardeningError), + #[error("System error: {0}")] + System(#[from] skillet_core::system::SystemError), #[error("Failed to set default tracing subscriber: {0}")] SetLogger(#[from] tracing::subscriber::SetGlobalDefaultError), #[error("IO error: {0}")] diff --git a/ublue/skillet/crates/core/Cargo.toml b/ublue/skillet/crates/core/Cargo.toml index 240af0c6..1c80a9d2 100644 --- a/ublue/skillet/crates/core/Cargo.toml +++ b/ublue/skillet/crates/core/Cargo.toml @@ -15,6 +15,7 @@ tempfile.workspace = true hex.workspace = true serde.workspace = true tracing.workspace = true +zbus.workspace = true [dev-dependencies] tempfile.workspace = true diff --git a/ublue/skillet/crates/core/src/files.rs b/ublue/skillet/crates/core/src/files.rs index a1eb3c5d..dc2de967 100644 --- a/ublue/skillet/crates/core/src/files.rs +++ b/ublue/skillet/crates/core/src/files.rs @@ -199,7 +199,13 @@ impl FileResource for LocalFileResource { let mut changed = false; if !path.exists() { - 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)?; changed = true; info!("Created directory {}", path.display()); } diff --git a/ublue/skillet/crates/core/src/system.rs b/ublue/skillet/crates/core/src/system.rs index d4df4569..4d5aed54 100644 --- a/ublue/skillet/crates/core/src/system.rs +++ b/ublue/skillet/crates/core/src/system.rs @@ -1,7 +1,19 @@ use std::process::Command; use thiserror::Error; -use tracing::{debug, info}; +use tracing::{debug, info, warn}; use users::get_group_by_name; +use zbus::proxy; + +#[proxy( + interface = "org.freedesktop.systemd1.Manager", + default_service = "org.freedesktop.systemd1", + default_path = "/org/freedesktop/systemd1" +)] +trait SystemdManager { + fn start_unit(&self, name: &str, mode: &str) -> zbus::Result; + fn stop_unit(&self, name: &str, mode: &str) -> zbus::Result; + fn restart_unit(&self, name: &str, mode: &str) -> zbus::Result; +} #[derive(Error, Debug)] pub enum SystemError { @@ -9,6 +21,8 @@ pub enum SystemError { GroupCheck(String), #[error("Command failed: {0}")] Command(String), + #[error("DBus error: {0}")] + DBus(#[from] zbus::Error), #[error("IO error: {0}")] Io(#[from] std::io::Error), } @@ -20,21 +34,59 @@ pub trait SystemResource { fn service_restart(&self, name: &str) -> Result<(), SystemError>; } -pub struct LinuxSystemResource; +pub struct LinuxSystemResource { + conn: Option, +} impl LinuxSystemResource { pub fn new() -> Self { - Self + let conn = match zbus::blocking::Connection::system() { + Ok(c) => Some(c), + Err(e) => { + warn!("Failed to connect to system DBus, will fallback to CLI: {e}"); + None + } + }; + Self { conn } } - fn run_systemctl(action: &str, name: &str) -> Result<(), SystemError> { - info!("Running systemctl {action} {name}"); - let output = Command::new("systemctl").arg(action).arg(name).output()?; + fn run_systemctl(&self, action: &str, name: &str) -> Result<(), SystemError> { + let name_with_suffix = if name.contains('.') { + name.to_string() + } else { + format!("{name}.service") + }; + + if let Some(conn) = &self.conn { + info!("Running systemctl {action} {name_with_suffix} via DBus"); + let proxy = SystemdManagerProxyBlocking::new(conn)?; + let res = match action { + "start" => proxy.start_unit(&name_with_suffix, "replace"), + "stop" => proxy.stop_unit(&name_with_suffix, "replace"), + "restart" => proxy.restart_unit(&name_with_suffix, "replace"), + _ => { + return Err(SystemError::Command(format!("Unsupported action: {action}"))); + } + }; + + match res { + Ok(_) => return Ok(()), + Err(e) => { + warn!("DBus call failed, falling back to CLI: {e}"); + } + } + } + + info!("Running systemctl {action} {name_with_suffix} via CLI"); + let output = Command::new("systemctl") + .arg(action) + .arg(&name_with_suffix) + .output()?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); return Err(SystemError::Command(format!( - "systemctl {action} {name} failed: {stderr}" + "systemctl {action} {name_with_suffix} failed: {stderr}" ))); } Ok(()) @@ -56,13 +108,8 @@ impl SystemResource for LinuxSystemResource { } // 2. Create group using `groupadd` - // Note: Creating groups requires root privileges usually. info!("Creating group {name}"); - let output = Command::new("groupadd") - .arg(name) - // .arg("-r") // System group? Maybe make it an option? - // For now, simple group creation. - .output()?; + let output = Command::new("groupadd").arg(name).output()?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); @@ -74,15 +121,15 @@ impl SystemResource for LinuxSystemResource { } fn service_start(&self, name: &str) -> Result<(), SystemError> { - Self::run_systemctl("start", name) + self.run_systemctl("start", name) } fn service_stop(&self, name: &str) -> Result<(), SystemError> { - Self::run_systemctl("stop", name) + self.run_systemctl("stop", name) } fn service_restart(&self, name: &str) -> Result<(), SystemError> { - Self::run_systemctl("restart", name) + self.run_systemctl("restart", name) } } #[cfg(test)] diff --git a/ublue/skillet/crates/hardening/src/lib.rs b/ublue/skillet/crates/hardening/src/lib.rs index 260df8f3..55ea9eee 100644 --- a/ublue/skillet/crates/hardening/src/lib.rs +++ b/ublue/skillet/crates/hardening/src/lib.rs @@ -40,10 +40,13 @@ where F: FileResource + ?Sized, { info!("Applying sysctl hardening..."); + let sysctl_dir = Path::new("/etc/sysctl.d"); + files.ensure_directory(sysctl_dir, Some(0o755), Some("root"), Some("root"))?; + let content = include_bytes!("../files/sysctl.boxy.conf"); - let path = Path::new("/etc/sysctl.d/99-hardening.conf"); + let path = sysctl_dir.join("99-hardening.conf"); - let changed = files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; + let changed = files.ensure_file(&path, content, Some(0o644), Some("root"), Some("root"))?; if changed { info!("Sysctl configuration changed, restarting systemd-sysctl..."); diff --git a/ublue/skillet/integration_tests/recordings/beezelbot.yaml b/ublue/skillet/integration_tests/recordings/beezelbot.yaml index 09b6b627..c123e6ba 100644 --- a/ublue/skillet/integration_tests/recordings/beezelbot.yaml +++ b/ublue/skillet/integration_tests/recordings/beezelbot.yaml @@ -1,3 +1,8 @@ +- !EnsureDirectory + path: /etc/sysctl.d + mode: '0o755' + owner: root + group: root - !EnsureFile path: /etc/sysctl.d/99-hardening.conf content_hash: c71e2f0edb84c44cfb601a2dc3d35df3b46afbbe9d28e02283a12d4b5f55b89d From c20b696a65df3a0924000b6335397b1732866289 Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 4 Apr 2026 18:26:02 +0200 Subject: [PATCH 13/19] refactor(skillet): optimize file hashing and verify directory types --- ublue/skillet/crates/core/src/files.rs | 27 ++++++++++++++--- ublue/skillet/crates/core/src/files/tests.rs | 29 +++++++++++++++++++ ublue/skillet/crates/core/src/system/tests.rs | 1 - 3 files changed, 52 insertions(+), 5 deletions(-) diff --git a/ublue/skillet/crates/core/src/files.rs b/ublue/skillet/crates/core/src/files.rs index dc2de967..0f302ef8 100644 --- a/ublue/skillet/crates/core/src/files.rs +++ b/ublue/skillet/crates/core/src/files.rs @@ -1,4 +1,5 @@ use nix::unistd::{chown, Gid, Uid}; +use sha2::{Digest, Sha256}; use std::fs::{self}; use std::io::{self, Write}; use std::os::unix::fs::{MetadataExt, PermissionsExt}; @@ -28,6 +29,8 @@ pub enum FileError { UserNotFound(String), #[error("Group {0} not found")] GroupNotFound(String), + #[error("Path {0} exists but is not a directory")] + NotADirectory(String), } pub trait FileResource { @@ -158,9 +161,19 @@ impl FileResource for LocalFileResource { let metadata = fs::metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; if metadata.len() == content.len() as u64 { - let existing_content = - fs::read(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; - existing_content != content + 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) + .map_err(|e| FileError::Read(path.display().to_string(), e))?; + 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 } else { true } @@ -198,7 +211,13 @@ impl FileResource for LocalFileResource { ) -> Result { let mut changed = false; - if !path.exists() { + if path.exists() { + let metadata = + fs::metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; + if !metadata.is_dir() { + return Err(FileError::NotADirectory(path.display().to_string())); + } + } else { use std::os::unix::fs::DirBuilderExt; let mut builder = fs::DirBuilder::new(); builder.recursive(true); diff --git a/ublue/skillet/crates/core/src/files/tests.rs b/ublue/skillet/crates/core/src/files/tests.rs index bfe75580..51426bb5 100644 --- a/ublue/skillet/crates/core/src/files/tests.rs +++ b/ublue/skillet/crates/core/src/files/tests.rs @@ -85,6 +85,35 @@ fn test_ensure_file_metadata() { // or we would need to mock the underlying chown call. } +#[test] +fn test_ensure_directory_creates_dir() { + let dir = tempdir().unwrap(); + let sub_dir = dir.path().join("subdir"); + let resource = LocalFileResource::new(); + + let changed = resource + .ensure_directory(&sub_dir, Some(0o755), None, None) + .unwrap(); + assert!(changed); + assert!(sub_dir.exists()); + assert!(sub_dir.is_dir()); +} + +#[test] +fn test_ensure_directory_fails_if_file() { + let dir = tempdir().unwrap(); + let file_path = dir.path().join("file.txt"); + fs::write(&file_path, b"not a dir").unwrap(); + let resource = LocalFileResource::new(); + + let result = resource.ensure_directory(&file_path, None, None, None); + assert!(result.is_err()); + match result { + Err(FileError::NotADirectory(p)) => assert_eq!(p, file_path.display().to_string()), + _ => panic!("Expected NotADirectory error, got {result:?}"), + } +} + #[test] fn test_delete_file() { let dir = tempdir().unwrap(); diff --git a/ublue/skillet/crates/core/src/system/tests.rs b/ublue/skillet/crates/core/src/system/tests.rs index 8c50d2ae..65a038c1 100644 --- a/ublue/skillet/crates/core/src/system/tests.rs +++ b/ublue/skillet/crates/core/src/system/tests.rs @@ -1,4 +1,3 @@ -use super::*; #[cfg(feature = "test-utils")] use crate::test_utils::MockSystem; From 3d4d98aa04f1453a9102671a0bbfe83c6e6b255e Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 4 Apr 2026 18:29:25 +0200 Subject: [PATCH 14/19] refactor(skillet): address review comments on file hashing and directory type verification --- ublue/skillet/crates/core/src/system/tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/ublue/skillet/crates/core/src/system/tests.rs b/ublue/skillet/crates/core/src/system/tests.rs index 65a038c1..8c50d2ae 100644 --- a/ublue/skillet/crates/core/src/system/tests.rs +++ b/ublue/skillet/crates/core/src/system/tests.rs @@ -1,3 +1,4 @@ +use super::*; #[cfg(feature = "test-utils")] use crate::test_utils::MockSystem; From 92cfb0689bc9fc51227fd909993439ac0f4d82cb Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 4 Apr 2026 18:54:26 +0200 Subject: [PATCH 15/19] refactor(skillet): improve file/directory type verification and handle group creation race --- ublue/skillet/crates/core/src/files.rs | 19 ++++++---- ublue/skillet/crates/core/src/files/tests.rs | 38 ++++++++++++++++++++ ublue/skillet/crates/core/src/system.rs | 6 ++++ 3 files changed, 57 insertions(+), 6 deletions(-) diff --git a/ublue/skillet/crates/core/src/files.rs b/ublue/skillet/crates/core/src/files.rs index 0f302ef8..b0c24b7b 100644 --- a/ublue/skillet/crates/core/src/files.rs +++ b/ublue/skillet/crates/core/src/files.rs @@ -31,6 +31,8 @@ pub enum FileError { GroupNotFound(String), #[error("Path {0} exists but is not a directory")] NotADirectory(String), + #[error("Path {0} exists but is not a regular file")] + NotARegularFile(String), } pub trait FileResource { @@ -158,11 +160,16 @@ impl FileResource for LocalFileResource { // 2. Check content let content_changed = if path.exists() { - let metadata = - fs::metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; + let metadata = fs::symlink_metadata(path) + .map_err(|e| FileError::Read(path.display().to_string(), e))?; + + if !metadata.is_file() { + return Err(FileError::NotARegularFile(path.display().to_string())); + } + if metadata.len() == content.len() as u64 { - let file = - fs::File::open(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; + 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) @@ -212,8 +219,8 @@ impl FileResource for LocalFileResource { let mut changed = false; if path.exists() { - let metadata = - fs::metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; + let metadata = fs::symlink_metadata(path) + .map_err(|e| FileError::Read(path.display().to_string(), e))?; if !metadata.is_dir() { return Err(FileError::NotADirectory(path.display().to_string())); } diff --git a/ublue/skillet/crates/core/src/files/tests.rs b/ublue/skillet/crates/core/src/files/tests.rs index 51426bb5..f38c97d0 100644 --- a/ublue/skillet/crates/core/src/files/tests.rs +++ b/ublue/skillet/crates/core/src/files/tests.rs @@ -85,6 +85,25 @@ fn test_ensure_file_metadata() { // or we would need to mock the underlying chown call. } +#[test] +fn test_ensure_file_fails_if_symlink() { + let dir = tempdir().unwrap(); + let target_path = dir.path().join("target.txt"); + let link_path = dir.path().join("link.txt"); + fs::write(&target_path, b"target").unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(&target_path, &link_path).unwrap(); + + let resource = LocalFileResource::new(); + let result = resource.ensure_file(&link_path, b"new content", None, None, None); + + assert!(result.is_err()); + match result { + Err(FileError::NotARegularFile(p)) => assert_eq!(p, link_path.display().to_string()), + _ => panic!("Expected NotARegularFile error, got {result:?}"), + } +} + #[test] fn test_ensure_directory_creates_dir() { let dir = tempdir().unwrap(); @@ -114,6 +133,25 @@ fn test_ensure_directory_fails_if_file() { } } +#[test] +fn test_ensure_directory_fails_if_symlink() { + let dir = tempdir().unwrap(); + let target_dir = dir.path().join("target_dir"); + let link_path = dir.path().join("link_dir"); + fs::create_dir(&target_dir).unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(&target_dir, &link_path).unwrap(); + + let resource = LocalFileResource::new(); + let result = resource.ensure_directory(&link_path, None, None, None); + + assert!(result.is_err()); + match result { + Err(FileError::NotADirectory(p)) => assert_eq!(p, link_path.display().to_string()), + _ => panic!("Expected NotADirectory error, got {result:?}"), + } +} + #[test] fn test_delete_file() { let dir = tempdir().unwrap(); diff --git a/ublue/skillet/crates/core/src/system.rs b/ublue/skillet/crates/core/src/system.rs index 4d5aed54..a986aca6 100644 --- a/ublue/skillet/crates/core/src/system.rs +++ b/ublue/skillet/crates/core/src/system.rs @@ -112,6 +112,12 @@ impl SystemResource for LinuxSystemResource { let output = Command::new("groupadd").arg(name).output()?; 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) { + debug!("Group {name} was created by another process"); + return Ok(false); + } + let stderr = String::from_utf8_lossy(&output.stderr); return Err(SystemError::Command(format!("groupadd failed: {stderr}"))); } From 1f8dea656a0f47e301346ddfa03a73f084b1536d Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 4 Apr 2026 19:19:05 +0200 Subject: [PATCH 16/19] refactor(skillet): optimize systemd unit handling and improve test runner cleanup --- ublue/skillet/crates/cli/src/main.rs | 12 +++++--- ublue/skillet/crates/core/src/system.rs | 34 ++++++++++++++++++--- ublue/skillet/crates/core/src/test_utils.rs | 18 ++++++++--- 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/ublue/skillet/crates/cli/src/main.rs b/ublue/skillet/crates/cli/src/main.rs index c75653bd..5c1ff35a 100644 --- a/ublue/skillet/crates/cli/src/main.rs +++ b/ublue/skillet/crates/cli/src/main.rs @@ -104,14 +104,18 @@ fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<() Ok(()) })(); - info!("Stopping container..."); - let _ = Command::new("podman") - .args(["kill", &container_name]) - .output(); + stop_container(&container_name); result } +fn stop_container(container_name: &str) { + info!("Stopping container {container_name}..."); + let _ = Command::new("podman") + .args(["rm", "-f", container_name]) + .output(); +} + fn build_workspace() -> Result<()> { info!("Building skillet workspace..."); let build_status = Command::new("cargo") diff --git a/ublue/skillet/crates/core/src/system.rs b/ublue/skillet/crates/core/src/system.rs index a986aca6..be0e5c5e 100644 --- a/ublue/skillet/crates/core/src/system.rs +++ b/ublue/skillet/crates/core/src/system.rs @@ -1,9 +1,37 @@ use std::process::Command; +use std::sync::LazyLock; use thiserror::Error; use tracing::{debug, info, warn}; use users::get_group_by_name; use zbus::proxy; +static SYSTEMD_UNIT_SUFFIXES: LazyLock> = LazyLock::new(|| { + vec![ + ".service", + ".socket", + ".device", + ".mount", + ".automount", + ".swap", + ".target", + ".path", + ".timer", + ".slice", + ".scope", + ] +}); + +fn ensure_systemd_suffix(name: &str) -> String { + if SYSTEMD_UNIT_SUFFIXES + .iter() + .any(|suffix| name.ends_with(suffix)) + { + name.to_string() + } else { + format!("{name}.service") + } +} + #[proxy( interface = "org.freedesktop.systemd1.Manager", default_service = "org.freedesktop.systemd1", @@ -51,11 +79,7 @@ impl LinuxSystemResource { } fn run_systemctl(&self, action: &str, name: &str) -> Result<(), SystemError> { - let name_with_suffix = if name.contains('.') { - name.to_string() - } else { - format!("{name}.service") - }; + let name_with_suffix = ensure_systemd_suffix(name); if let Some(conn) = &self.conn { info!("Running systemctl {action} {name_with_suffix} via DBus"); diff --git a/ublue/skillet/crates/core/src/test_utils.rs b/ublue/skillet/crates/core/src/test_utils.rs index f4c5b1c4..c5fafb51 100644 --- a/ublue/skillet/crates/core/src/test_utils.rs +++ b/ublue/skillet/crates/core/src/test_utils.rs @@ -65,6 +65,7 @@ pub type FileMetadata = (Option, Option, Option); pub struct MockFiles { pub files: Arc>>>, pub metadata: Arc>>, + pub directories: Arc>>, } impl MockFiles { @@ -72,6 +73,7 @@ impl MockFiles { Self { files: Arc::new(Mutex::new(HashMap::new())), metadata: Arc::new(Mutex::new(HashMap::new())), + directories: Arc::new(Mutex::new(HashSet::new())), } } } @@ -127,14 +129,22 @@ impl FileResource for MockFiles { fn ensure_directory( &self, - _path: &Path, + path: &Path, _mode: Option, _owner: Option<&str>, _group: Option<&str>, ) -> Result { - // 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) + let path_str = path.display().to_string(); + let mut directories = self + .directories + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + if directories.contains(&path_str) { + Ok(false) + } else { + directories.insert(path_str); + Ok(true) + } } fn delete_file(&self, path: &Path) -> Result { From a1f0701d632c885ac7bde292b09cbd3430cbc36a Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sat, 4 Apr 2026 19:37:08 +0200 Subject: [PATCH 17/19] refactor(skillet): fix Sha256 usage and use constant for group existence exit code --- ublue/skillet/crates/core/src/files.rs | 12 +++++++++--- ublue/skillet/crates/core/src/system.rs | 6 ++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/ublue/skillet/crates/core/src/files.rs b/ublue/skillet/crates/core/src/files.rs index b0c24b7b..bac226e8 100644 --- a/ublue/skillet/crates/core/src/files.rs +++ b/ublue/skillet/crates/core/src/files.rs @@ -1,7 +1,7 @@ use nix::unistd::{chown, Gid, Uid}; use sha2::{Digest, Sha256}; use std::fs::{self}; -use std::io::{self, Write}; +use std::io::{self, Read, Write}; use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::Path; use tempfile::NamedTempFile; @@ -172,8 +172,14 @@ impl FileResource for LocalFileResource { .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) - .map_err(|e| FileError::Read(path.display().to_string(), e))?; + + let mut buffer = [0; 8192]; + while let Ok(n) = reader.read(&mut buffer) { + if n == 0 { + break; + } + hasher.update(&buffer[..n]); + } let existing_hash = hasher.finalize(); let mut new_hasher = Sha256::new(); diff --git a/ublue/skillet/crates/core/src/system.rs b/ublue/skillet/crates/core/src/system.rs index be0e5c5e..1783df78 100644 --- a/ublue/skillet/crates/core/src/system.rs +++ b/ublue/skillet/crates/core/src/system.rs @@ -123,6 +123,8 @@ impl Default for LinuxSystemResource { } } +const EXIT_CODE_GROUP_EXISTS: i32 = 9; + impl SystemResource for LinuxSystemResource { fn ensure_group(&self, name: &str) -> Result { // 1. Check if group exists using `users` crate @@ -136,8 +138,8 @@ impl SystemResource for LinuxSystemResource { let output = Command::new("groupadd").arg(name).output()?; 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) { + // Check if group was created by another process in the meantime + if output.status.code() == Some(EXIT_CODE_GROUP_EXISTS) { debug!("Group {name} was created by another process"); return Ok(false); } From ea08f62d9c9f45c30c4c64dd44dc25dc25715556 Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sun, 5 Apr 2026 09:55:28 +0200 Subject: [PATCH 18/19] refactor(skillet): ensure I/O errors are not ignored during hashing and improve binary discovery --- ublue/skillet/crates/cli/src/main.rs | 8 ++++++-- ublue/skillet/crates/core/src/files.rs | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/ublue/skillet/crates/cli/src/main.rs b/ublue/skillet/crates/cli/src/main.rs index 5c1ff35a..3fe9a295 100644 --- a/ublue/skillet/crates/cli/src/main.rs +++ b/ublue/skillet/crates/cli/src/main.rs @@ -131,7 +131,10 @@ fn build_workspace() -> Result<()> { fn locate_binary(hostname: &str) -> Result { let host_binary_name = format!("skillet-{hostname}"); - let binary_path = ["target/release", "target/debug"] + 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); @@ -141,8 +144,9 @@ fn locate_binary(hostname: &str) -> Result { None } }) + // If not found, try generic skillet binary in release, then debug .or_else(|| { - ["target/release", "target/debug"].iter().find_map(|dir| { + dirs.iter().find_map(|dir| { let p = PathBuf::from(dir).join("skillet"); if p.exists() { Some(p) diff --git a/ublue/skillet/crates/core/src/files.rs b/ublue/skillet/crates/core/src/files.rs index bac226e8..cfb0b30b 100644 --- a/ublue/skillet/crates/core/src/files.rs +++ b/ublue/skillet/crates/core/src/files.rs @@ -174,7 +174,9 @@ impl FileResource for LocalFileResource { let mut hasher = Sha256::new(); let mut buffer = [0; 8192]; - while let Ok(n) = reader.read(&mut buffer) { + loop { + let n = reader.read(&mut buffer) + .map_err(|e| FileError::Read(path.display().to_string(), e))?; if n == 0 { break; } From f05638c75e67113275514e5106829cb60b3e6d56 Mon Sep 17 00:00:00 2001 From: Giacomo Bagnoli Date: Sun, 5 Apr 2026 10:51:51 +0200 Subject: [PATCH 19/19] refactor(skillet): optimize file metadata calls and hashing, and refine binary discovery search --- ublue/skillet/crates/cli/src/main.rs | 41 +++++++--------- ublue/skillet/crates/core/src/files.rs | 65 ++++++++++++++------------ 2 files changed, 51 insertions(+), 55 deletions(-) diff --git a/ublue/skillet/crates/cli/src/main.rs b/ublue/skillet/crates/cli/src/main.rs index 3fe9a295..d352e92d 100644 --- a/ublue/skillet/crates/cli/src/main.rs +++ b/ublue/skillet/crates/cli/src/main.rs @@ -131,31 +131,22 @@ fn build_workspace() -> Result<()> { fn locate_binary(hostname: &str) -> Result { 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"))?; + + // Ordered search: + // 1. host-specific release + // 2. host-specific debug + // 3. generic skillet release + // 4. generic skillet debug + + let binary_path = [ + PathBuf::from("target/release").join(&host_binary_name), + PathBuf::from("target/debug").join(&host_binary_name), + PathBuf::from("target/release").join("skillet"), + PathBuf::from("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") diff --git a/ublue/skillet/crates/core/src/files.rs b/ublue/skillet/crates/core/src/files.rs index cfb0b30b..20eef2da 100644 --- a/ublue/skillet/crates/core/src/files.rs +++ b/ublue/skillet/crates/core/src/files.rs @@ -62,13 +62,12 @@ impl LocalFileResource { } fn check_metadata( - path: &Path, + _path: &Path, + metadata: &fs::Metadata, mode: Option, owner: Option<&str>, group: Option<&str>, ) -> Result { - 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 { @@ -98,14 +97,13 @@ impl LocalFileResource { fn apply_metadata( path: &Path, + metadata: &fs::Metadata, mode: Option, 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(); + let mut perms = metadata.permissions(); perms.set_mode(desired_mode); fs::set_permissions(path, perms) .map_err(|e| FileError::SetPermissions(path.display().to_string(), e))?; @@ -159,23 +157,22 @@ impl FileResource for LocalFileResource { let mut changed = false; // 2. Check content - let content_changed = if path.exists() { - let metadata = fs::symlink_metadata(path) - .map_err(|e| FileError::Read(path.display().to_string(), e))?; - - if !metadata.is_file() { + let mut metadata = fs::symlink_metadata(path).ok(); + let content_changed = if let Some(meta) = &metadata { + if !meta.is_file() { return Err(FileError::NotARegularFile(path.display().to_string())); } - if metadata.len() == content.len() as u64 { + if meta.len() == content.len() as u64 { 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) + let n = reader + .read(&mut buffer) .map_err(|e| FileError::Read(path.display().to_string(), e))?; if n == 0 { break; @@ -183,10 +180,7 @@ impl FileResource for LocalFileResource { hasher.update(&buffer[..n]); } let existing_hash = hasher.finalize(); - - let mut new_hasher = Sha256::new(); - new_hasher.update(content); - let new_hash = new_hasher.finalize(); + let new_hash = Sha256::digest(content); existing_hash != new_hash } else { @@ -205,13 +199,19 @@ impl FileResource for LocalFileResource { .map_err(|e| FileError::Persist(path.display().to_string(), e.error))?; changed = true; info!("Updated file content for {}", path.display()); + // Fetch metadata for the newly created file + metadata = Some( + fs::metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?, + ); } // 3. Check and apply metadata - if path.exists() && Self::check_metadata(path, mode, owner, group)? { - Self::apply_metadata(path, mode, owner, group)?; - changed = true; - info!("Updated file metadata for {}", path.display()); + if let Some(meta) = metadata { + if Self::check_metadata(path, &meta, mode, owner, group)? { + Self::apply_metadata(path, &meta, mode, owner, group)?; + changed = true; + info!("Updated file metadata for {}", path.display()); + } } Ok(changed) @@ -226,10 +226,9 @@ impl FileResource for LocalFileResource { ) -> Result { let mut changed = false; - if path.exists() { - let metadata = fs::symlink_metadata(path) - .map_err(|e| FileError::Read(path.display().to_string(), e))?; - if !metadata.is_dir() { + let mut metadata = fs::symlink_metadata(path).ok(); + if let Some(meta) = &metadata { + if !meta.is_dir() { return Err(FileError::NotADirectory(path.display().to_string())); } } else { @@ -242,12 +241,18 @@ impl FileResource for LocalFileResource { builder.create(path).map_err(FileError::Io)?; changed = true; info!("Created directory {}", path.display()); + // Fetch metadata for the newly created directory + metadata = Some( + fs::metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?, + ); } - if path.exists() && Self::check_metadata(path, mode, owner, group)? { - Self::apply_metadata(path, mode, owner, group)?; - changed = true; - info!("Updated directory metadata for {}", path.display()); + if let Some(meta) = metadata { + if Self::check_metadata(path, &meta, mode, owner, group)? { + Self::apply_metadata(path, &meta, mode, owner, group)?; + changed = true; + info!("Updated directory metadata for {}", path.display()); + } } Ok(changed)