fix(daemon): replace shell grants with bounded action helpers

.github/workflows/ci.yml

@@ -113,6 +113,11 @@ jobs:
       - name: Check local gate discovery and required-check warnings
         run: bash tests/release/ci-local.test.sh
 
+      - name: Check bounded actions and removal of shell grants
+        # Root is used only in an isolated fixture subprocess to prove that the
+        # helper irreversibly drops credentials before touching user files.
+        run: sudo -n bash tests/release/action-steps.test.sh
+
       - name: Check the audit export states its confidentiality class
         run: bash tests/release/audit-export-confidentiality.test.sh
 

CHANGELOG.md

@@ -12,6 +12,14 @@ Releases before `0.2.5` predate the public launch; their notes live in the
 
 ## [Unreleased]
 
+### Changed
+
+- Remove whole-binary shell and runuser sudo grants. Firewall, group, Snap,
+  SSH-key and user-scoped Flatpak/Podman/Toolbox operations now use a bounded
+  helper with fixed command grammars. User operations refuse UID 0 and drop
+  groups/GID/UID before file access or execution; SSH edits reject symlinks.
+  Install the new helper together with the daemon and sudoers fragment (#417).
+
 ### Fixed
 
 - Align planner action descriptions with the Ubuntu execution fence, including

Makefile

@@ -118,6 +118,7 @@ daemon-install: daemon-install-preflight build
 	# sysknife-daemon --test helper_install_coverage` derives the required set
 	# from the daemon source and fails if one is missing.
 	install -Dm 755 packaging/sysknife-apt-pin-edit $(HELPERS)/apt-pin-edit
+	install -Dm 755 packaging/sysknife-action-steps $(HELPERS)/action-steps
 	install -Dm 755 packaging/sysknife-audit-edit $(HELPERS)/audit-edit
 	install -Dm 755 packaging/sysknife-fail2ban-jail-edit $(HELPERS)/fail2ban-jail-edit
 	install -Dm 755 packaging/sysknife-grub-kargs-edit $(HELPERS)/grub-kargs-edit
@@ -148,6 +149,7 @@ daemon-uninstall:
 	rm -f $(SYSUSERS)/sysknife.conf
 	rm -f $(TMPFILES)/sysknife.conf
 	rm -f $(HELPERS)/apt-pin-edit
+	rm -f $(HELPERS)/action-steps
 	rm -f $(HELPERS)/audit-edit
 	rm -f $(HELPERS)/fail2ban-jail-edit
 	rm -f $(HELPERS)/grub-kargs-edit

crates/sysknife-daemon/src/actions/containers.rs

@@ -1,4 +1,4 @@
-use super::{command_mechanism, ActionMechanism, ActionSpec};
+use super::{ActionMechanism, ActionSpec};
 use sysknife_types::RiskLevel;
 
 pub fn specs() -> Vec<ActionSpec> {
@@ -16,107 +16,82 @@ pub fn specs() -> Vec<ActionSpec> {
     ]
 }
 
-/// Run a Podman command as the target user via `sudo runuser -l`.
+/// Run fixed Podman argv after dropping to the target user.
 ///
 /// Rootless Podman operates against the user's container storage, not a shared
 /// system store. The daemon runs as the `sysknife` system user whose container
 /// namespace is empty; we must switch to `username` to reach their containers.
-/// `runuser -l` establishes a full login context including XDG_RUNTIME_DIR and
-/// the sub-UID/GID ranges required by the kernel namespace code — those env
-/// vars are populated by `pam_systemd` which only fires for a login shell.
-///
-/// **Shell-injection safety:** the `-c "<podman_cmd>"` form passes a string to
-/// `/bin/sh`, so any metacharacter in an interpolated value would be expanded
-/// by the shell. Defence-in-depth:
-///   1. `username` flows through `validated_username` (`[A-Za-z0-9._-]`, no
-///      leading dash, ≤32 bytes).
-///   2. Container `name` and `image` flow through `validated_safe_arg`, which
-///      enforces `[A-Za-z0-9._:/+@-]`, no leading dash, ≤254 bytes — every
-///      shell metacharacter (`;`, `&`, `|`, `$`, backtick, quotes, whitespace,
-///      glob, brace, `\`, etc.) is rejected at the boundary.
-///   3. The format!-interpolated values are wrapped in single quotes so even
-///      a future validator regression cannot escape the surrounding quotes.
-fn podman_as(username: &str, podman_cmd: &str) -> ActionMechanism {
+/// The helper sets HOME and XDG_RUNTIME_DIR for the resolved UID, resets
+/// supplementary groups, and drops GID/UID before invoking the fixed tool.
+/// Rootless Podman still requires the account's configured sub-UID/GID ranges
+/// and runtime directory. No login shell or user startup file runs as root.
+fn podman_as(username: &str, parameters: &[&str]) -> ActionMechanism {
+    let mut args = vec![
+        "/usr/lib/sysknife/action-steps".to_string(),
+        "podman".to_string(),
+        username.to_string(),
+    ];
+    args.extend(parameters.iter().map(|value| value.to_string()));
     ActionMechanism::Command {
         program: "sudo",
-        args: vec![
-            "runuser".to_string(),
-            "-l".to_string(),
-            username.to_string(),
-            "-c".to_string(),
-            podman_cmd.to_string(),
-        ],
+        args,
     }
 }
 
 pub fn list_containers(username: &str) -> ActionSpec {
     ActionSpec {
         action_name: "ListContainers",
-        mechanism: command_mechanism(
-            "sudo",
-            [
-                "runuser",
-                "-l",
-                username,
-                "-c",
-                "podman ps --all --format json",
-            ],
-        ),
+        mechanism: podman_as(username, &["ps", "--all", "--format", "json"]),
         risk_level: RiskLevel::Low,
         reboot_required: false,
         rollback_available: false,
     }
 }
 
 pub fn create_container(username: &str, name: &str, image: &str) -> ActionSpec {
-    let cmd = format!("podman create --name '{}' '{}'", name, image);
     ActionSpec {
         action_name: "CreateContainer",
-        mechanism: podman_as(username, &cmd),
+        mechanism: podman_as(username, &["create", "--name", name, image]),
         risk_level: RiskLevel::Medium,
         reboot_required: false,
         rollback_available: false,
     }
 }
 
 pub fn start_container(username: &str, name: &str) -> ActionSpec {
-    let cmd = format!("podman start '{}'", name);
     ActionSpec {
         action_name: "StartContainer",
-        mechanism: podman_as(username, &cmd),
+        mechanism: podman_as(username, &["start", name]),
         risk_level: RiskLevel::Medium,
         reboot_required: false,
         rollback_available: false,
     }
 }
 
 pub fn stop_container(username: &str, name: &str) -> ActionSpec {
-    let cmd = format!("podman stop '{}'", name);
     ActionSpec {
         action_name: "StopContainer",
-        mechanism: podman_as(username, &cmd),
+        mechanism: podman_as(username, &["stop", name]),
         risk_level: RiskLevel::Medium,
         reboot_required: false,
         rollback_available: false,
     }
 }
 
 pub fn remove_container(username: &str, name: &str) -> ActionSpec {
-    let cmd = format!("podman rm '{}'", name);
     ActionSpec {
         action_name: "RemoveContainer",
-        mechanism: podman_as(username, &cmd),
+        mechanism: podman_as(username, &["rm", name]),
         risk_level: RiskLevel::Medium,
         reboot_required: false,
         rollback_available: false,
     }
 }
 
 pub fn get_container_info(username: &str, name: &str) -> ActionSpec {
-    let cmd = format!("podman inspect '{}'", name);
     ActionSpec {
         action_name: "GetContainerInfo",
-        mechanism: podman_as(username, &cmd),
+        mechanism: podman_as(username, &["inspect", name]),
         risk_level: RiskLevel::Low,
         reboot_required: false,
         rollback_available: false,

crates/sysknife-daemon/src/actions/flatpak.rs

@@ -19,29 +19,22 @@ pub fn specs() -> Vec<ActionSpec> {
     ]
 }
 
-/// Run a Flatpak command as the target user via `sudo runuser -u user -- flatpak <argv>`.
+/// Run a fixed Flatpak operation after the helper drops to the target user.
 ///
 /// Flatpak user installations live under `~/.local/share/flatpak/` and are
 /// accessed through the user's D-Bus session. The daemon runs as `sysknife`
-/// (a system user) with no user installation; `runuser -u` switches to the
+/// (a system user) with no user installation; the helper switches to the
 /// correct user UID without spawning a login shell, so each argv element is
 /// passed to `flatpak` verbatim.
 ///
-/// **Shell-injection safety:** unlike `runuser -l user -c "<shell-string>"`,
-/// the `-u user -- argv` form bypasses the shell entirely. There is no string
-/// interpolation, no metacharacter expansion, and no quoting concern — every
-/// argument reaches `flatpak(1)` exactly as supplied. Callers must still pass
-/// arguments through `validated_safe_arg`/`validated_username` upstream so a
-/// hostile value cannot impersonate a flag (`-X`) or break out of the
-/// command's own option parser, but they no longer have to defend against
-/// shell metacharacters.
+/// The helper independently allowlists the complete Flatpak argv grammar and
+/// rejects option-shaped values before dropping credentials and executing the
+/// fixed binary. Callers also validate values upstream; no shell parses them.
 fn flatpak_as(username: &str, args: &[&str]) -> ActionMechanism {
     let mut argv: Vec<String> = vec![
-        "runuser".to_string(),
-        "-u".to_string(),
-        username.to_string(),
-        "--".to_string(),
+        "/usr/lib/sysknife/action-steps".to_string(),
         "flatpak".to_string(),
+        username.to_string(),
     ];
     argv.extend(args.iter().map(|s| s.to_string()));
     ActionMechanism::Command {
@@ -168,7 +161,7 @@ pub fn get_flatpak_app_info(username: &str, app_id: &str) -> ActionSpec {
 // every Ubuntu wrapper delegates directly to the shared `flatpak_as` helper.
 // ---------------------------------------------------------------------------
 
-/// Install a Flatpak app on Ubuntu (`sudo runuser -u <user> -- flatpak install --user -y <remote> <app>`).
+/// Install a Flatpak app on Ubuntu through the bounded user-operation helper.
 ///
 /// Identical argv to `InstallFlatpak` on Fedora. Distinct action name for
 /// Ubuntu-specific routing in the daemon and LLM prompt.
@@ -184,7 +177,7 @@ pub fn ubuntu_install_flatpak(username: &str, app_id: &str, remote: &str) -> Act
     }
 }
 
-/// Remove a Flatpak app on Ubuntu (`sudo runuser -u <user> -- flatpak uninstall --user -y <app>`).
+/// Remove a Flatpak app on Ubuntu through the bounded user-operation helper.
 ///
 /// Risk: Medium. Uninstalls a sandboxed Flatpak application.
 pub fn ubuntu_remove_flatpak(username: &str, app_id: &str) -> ActionSpec {

crates/sysknife-daemon/src/actions/mod.rs

@@ -152,7 +152,7 @@ pub fn exclusive_resource(spec: &ActionSpec) -> Option<ExclusiveResource> {
                 "apt-get" | "apt" | "apt-mark" | "aptitude" | "dpkg" | "dpkg-reconfigure"
                 | "add-apt-repository" | "do-release-upgrade" | "unattended-upgrade"
                 | "apt-pin-edit" => Some(ExclusiveResource::Dpkg),
-                "snap" => Some(ExclusiveResource::Snap),
+                "snap" | "snap-install-hold" => Some(ExclusiveResource::Snap),
                 "rpm-ostree" | "ostree" => Some(ExclusiveResource::RpmOstree),
                 "flatpak" => Some(ExclusiveResource::Flatpak),
                 _ => None,

crates/sysknife-daemon/src/actions/network.rs

@@ -55,37 +55,27 @@ pub fn set_dns_servers(interface: &str, servers: &[&str]) -> ActionSpec {
 
 /// Configure a firewalld rule and reload so it takes effect.
 ///
-/// Uses `sh -c` to chain `firewall-cmd --permanent ... && firewall-cmd --reload`
-/// atomically; firewalld has no single-call equivalent that updates the
-/// permanent rule and reloads runtime in one shot.
-///
-/// **Shell-injection safety:** `zone` and `service` are interpolated into the
-/// script via `format!`, so any shell metacharacter in either would be
-/// expanded by `/bin/sh`. Defence-in-depth:
-///   1. Both flow through `validated_safe_arg` upstream, which enforces a
-///      strict ASCII allowlist (`[A-Za-z0-9._:/+@-]`, no leading dash, ≤254
-///      bytes) and rejects every shell metacharacter at the boundary.
-///   2. The interpolated values are wrapped in single quotes so a future
-///      validator regression cannot escape the surrounding quotes.
-///   3. `verb` is selected from a fixed pair of literals (`add-service` /
-///      `remove-service`); it is never attacker-influenced.
+/// The bounded helper runs the permanent mutation then reloads only on success.
+/// These are sequential commands, not an atomic firewalld transaction. Arguments
+/// are validated independently by the helper and never interpreted as a shell.
 pub fn configure_firewall(zone: &str, service: &str, enabled: bool) -> ActionSpec {
     let verb = if enabled {
         "add-service"
     } else {
         "remove-service"
     };
-    let script = format!(
-        "firewall-cmd --permanent --zone='{}' --{}='{}' && firewall-cmd --reload",
-        zone, verb, service
-    );
-
     ActionSpec {
         action_name: "ConfigureFirewall",
-        mechanism: super::ActionMechanism::Command {
-            program: "sudo",
-            args: vec!["sh".to_string(), "-c".to_string(), script],
-        },
+        mechanism: command_mechanism(
+            "sudo",
+            [
+                "/usr/lib/sysknife/action-steps",
+                "firewall",
+                zone,
+                service,
+                verb,
+            ],
+        ),
         risk_level: RiskLevel::High,
         reboot_required: false,
         rollback_available: false,

crates/sysknife-daemon/src/actions/snap.rs

@@ -10,9 +10,8 @@
 //! `snap refresh --hold <name>` to pin the snap at the installed version.
 //! Set `auto_update: true` in the plan params to skip the hold.
 //!
-//! The hold is applied by building a two-command spec using a shell fragment
-//! via `sh -c "snap install … && snap refresh --hold …"`. `name` and
-//! `channel` are validated by [`snap_install`] itself before interpolation
+//! The bounded helper installs and then holds, stopping on either failure.
+//! `name` and `channel` are validated by [`snap_install`] and the helper
 //! (in addition to, not instead of, the executor's own `validated_safe_arg`
 //! check) — see the `SnapInstallError` doc below.
 
@@ -31,10 +30,8 @@ pub enum SnapInstallError {
     ///
     /// Defense in depth: the executor already validates both via
     /// `validated_safe_arg` before calling this constructor, but the
-    /// `auto_update: false` path interpolates `name`/`channel` into a
-    /// `sh -c "snap install … && snap refresh --hold …"` fragment — a future
-    /// internal Rust caller (fleet plan/execute path) that skipped the
-    /// executor could not otherwise be blocked from injecting through it.
+    /// helper also revalidates them. Keep constructor validation so internal
+    /// callers cannot produce an invalid spec by skipping the executor.
     InvalidArg { param: &'static str, value: String },
 }
 
@@ -132,16 +129,19 @@ pub fn snap_install(
             rollback_available: false,
         })
     } else {
-        // Install + hold in one shell fragment to avoid a window where the snap
-        // can be auto-refreshed between install and hold.
+        // Sequential install and hold: the helper does not hold a failed install.
         let channel_arg = channel.unwrap_or("stable");
-        let cmd = format!(
-            "snap install --channel={} {} && snap refresh --hold {}",
-            channel_arg, name, name
-        );
         Ok(ActionSpec {
             action_name: "SnapInstall",
-            mechanism: super::command_mechanism("sudo", ["sh", "-c", &cmd]),
+            mechanism: super::command_mechanism(
+                "sudo",
+                [
+                    "/usr/lib/sysknife/action-steps",
+                    "snap-install-hold",
+                    name,
+                    channel_arg,
+                ],
+            ),
             risk_level: RiskLevel::Medium,
             reboot_required: false,
             rollback_available: false,
@@ -299,15 +299,18 @@ mod tests {
         let spec = snap_install("firefox", None, false).unwrap();
         let (prog, args) = extract_args(&spec);
         assert_eq!(prog, "sudo");
-        // When auto_update=false the hold is embedded in a sh -c fragment.
-        let full = args.join(" ");
-        assert!(
-            full.contains("snap install"),
-            "missing 'snap install': {full}"
+        assert_eq!(
+            args,
+            [
+                "/usr/lib/sysknife/action-steps",
+                "snap-install-hold",
+                "firefox",
+                "stable"
+            ]
         );
-        assert!(
-            full.contains("snap refresh --hold firefox"),
-            "missing hold: {full}"
+        assert_eq!(
+            crate::actions::exclusive_resource(&spec),
+            Some(crate::actions::ExclusiveResource::Snap)
         );
     }