From 95cc69af97b104747d34fccbaaa2d2b5197546a6 Mon Sep 17 00:00:00 2001 From: hushen <190065939+918154429@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:03:52 +0800 Subject: [PATCH] fix(ufw): expose numbered rules before deletion --- CHANGELOG.md | 4 ++ .../src/planning_tools/propose_plan.rs | 4 +- .../src/planning_tools/query_tools.rs | 21 ++++++++- crates/sysknife-brain/src/prompt.rs | 10 ++-- crates/sysknife-daemon/src/actions/ufw.rs | 47 ++++++++++++++++++- crates/sysknife-daemon/src/executor.rs | 10 +++- docs/action-reference.md | 4 +- docs/typed-actions.md | 9 ++++ 8 files changed, 98 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d8d55fb..c074b399 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ Releases before `0.2.5` predate the public launch; their notes live in the ### Changed +- Let `UfwStatus` return numbered rules with `numbered: true`, retaining verbose + output by default. Add `query_ufw_rules` so the planner can read the indices + required by `UfwDeleteRule` instead of guessing them (#234). + - Separate Ubuntu identity requirements from Debian-family mechanisms and planner defaults. Canonical services, PPAs and the reboot sentinel require Ubuntu itself; portable tools are no longer refused merely for being another diff --git a/crates/sysknife-brain/src/planning_tools/propose_plan.rs b/crates/sysknife-brain/src/planning_tools/propose_plan.rs index a5859f20..5a30e4cf 100644 --- a/crates/sysknife-brain/src/planning_tools/propose_plan.rs +++ b/crates/sysknife-brain/src/planning_tools/propose_plan.rs @@ -357,7 +357,7 @@ this is runtime status, NOT the saved configuration; on Ubuntu the saved config ("UfwReset", "reset ufw to defaults, removing all rules — no params; Ubuntu only; High risk; irreversible"), ("UfwStatus", - "show current ufw status and rules — no params; Ubuntu only; read-only"), + "show current ufw status and rules — optional param: numbered (boolean, default false); true runs ufw status numbered and exposes rule_number values for UfwDeleteRule, false keeps verbose status; read-only"), // ── Ubuntu / distrobox — container environment ──────────────────────────── ("DistroboxList", "list distrobox containers — no params; Ubuntu only; read-only"), @@ -434,7 +434,7 @@ reports live interface state"), "regenerate netplan backend config without applying — no params; Ubuntu only; Medium risk; dry-run before NetplanApply"), // ── Ubuntu / Tier 3 — ufw extensions ───────────────────────────────────── ("UfwDeleteRule", - "delete a ufw rule by number — param: rule_number* (positive integer from 'ufw status numbered'); Ubuntu only; High risk"), + "delete a ufw rule by number — param: rule_number* (positive integer from query_ufw_rules or UfwStatus with numbered=true); never guess a rule number, and refresh after rule changes; High risk"), ("UfwLimit", "add rate-limiting rule on a port/service (>6 connections/30s blocked) — param: target* (e.g. '22' or 'ssh'); Ubuntu only; High risk; use for SSH brute-force mitigation"), // ── Ubuntu / Tier 3 — release upgrade ──────────────────────────────────── diff --git a/crates/sysknife-brain/src/planning_tools/query_tools.rs b/crates/sysknife-brain/src/planning_tools/query_tools.rs index 1aab020d..141d7bf2 100644 --- a/crates/sysknife-brain/src/planning_tools/query_tools.rs +++ b/crates/sysknife-brain/src/planning_tools/query_tools.rs @@ -8,6 +8,11 @@ use crate::provider::ToolDefinition; pub fn query_tools() -> Vec { let empty_schema = serde_json::json!({"type": "object", "properties": {}, "required": [], "additionalProperties": false}); vec![ + ToolDefinition { + name: "query_ufw_rules".into(), + description: "Read ufw status numbered to obtain current rule indices for UfwDeleteRule. Never guess an index; query again after any rule change because indices can shift.".into(), + input_schema: empty_schema.clone(), + }, ToolDefinition { name: "query_services".into(), description: "List all running systemd services. Returns one service name per line." @@ -225,6 +230,7 @@ pub fn query_tool_to_action( input: &serde_json::Value, ) -> Result, String> { match tool_name { + "query_ufw_rules" => Ok(Some(("UfwStatus", serde_json::json!({"numbered": true})))), "query_services" => Ok(Some(("ListServices", serde_json::json!({})))), "query_firewall" => Ok(Some(("GetFirewallState", serde_json::json!({})))), "query_deployments" => Ok(Some(("ListDeployments", serde_json::json!({})))), @@ -298,6 +304,10 @@ mod tests { #[test] fn known_query_tools_map_to_actions() { let empty = empty_input(); + assert_eq!( + query_tool_to_action("query_ufw_rules", &empty), + Ok(Some(("UfwStatus", serde_json::json!({"numbered": true})))) + ); assert_eq!( query_tool_to_action("query_services", &empty), Ok(Some(("ListServices", serde_json::json!({})))) @@ -449,9 +459,16 @@ mod tests { } #[test] - fn query_tools_returns_twenty_three_definitions() { + fn query_tools_returns_twenty_four_definitions() { let tools = query_tools(); - assert_eq!(tools.len(), 23); + assert_eq!(tools.len(), 24); + let ufw = tools + .iter() + .find(|tool| tool.name == "query_ufw_rules") + .unwrap(); + assert!(ufw.description.contains("UfwDeleteRule")); + assert!(ufw.description.contains("Never guess")); + assert_eq!(ufw.input_schema["additionalProperties"], false); for tool in &tools { assert!(tool.name.starts_with("query_")); assert!(!tool.description.is_empty()); diff --git a/crates/sysknife-brain/src/prompt.rs b/crates/sysknife-brain/src/prompt.rs index f5fdad9e..ced2a8ae 100644 --- a/crates/sysknife-brain/src/prompt.rs +++ b/crates/sysknife-brain/src/prompt.rs @@ -903,7 +903,7 @@ const DEBIAN_PARAMS: &str = r#" **No params** — use `{}`: AptUpdate, AptAutoremove, AptListInstalled, AptListUpgradable, AptHistoryList, CheckPendingReboot, GrubGetKargs, -UfwStatus, UfwEnable, UfwDisable, UfwReset, DistroboxList, NetplanGetConfig, +UfwEnable, UfwDisable, UfwReset, DistroboxList, NetplanGetConfig, NetplanApply, NetplanGenerate, ProStatus, ProDetach, LivepatchStatus, MultipassList, UbuntuReleaseUpgrade. @@ -928,8 +928,9 @@ ProStatus, ProDetach, LivepatchStatus, MultipassList, UbuntuReleaseUpgrade. - `GrubSetKargs`: `{"append":["quiet","nomodeset"],"delete":["splash"]}` — either list may be `[]` but at least one must be non-empty **UFW**: +- `UfwStatus`: `{}` for verbose status, or `{"numbered":true}` for current rule indices. `query_ufw_rules` reads the numbered form during planning. - `UfwAllow` / `UfwDeny`: `{"port_or_service":"22/tcp"}` or `{"port_or_service":"ssh"}` -- `UfwDeleteRule`: `{"rule_number":3}` — positive integer from `ufw status numbered` +- `UfwDeleteRule`: `{"rule_number":3}` — use a rule number explicitly supplied by the operator or obtained from `query_ufw_rules`; never guess. Query again after any rule change because indices shift. - `UfwLimit`: `{"target":"22"}` or `{"target":"ssh"}` **Netplan**: @@ -1507,9 +1508,12 @@ mod tests { /// that could only fail. The CLI now also refuses such a plan at plan time; /// this line is what stops the model proposing one in the first place. #[test] - fn debian_prompt_states_the_valid_port_range() { + fn debian_prompt_states_firewall_input_requirements() { let hint = debian_hint(); let p = build_system_prompt(None, Some(&hint)); + assert!(p.contains("query_ufw_rules")); + assert!(p.contains("{\"numbered\":true}")); + assert!(p.contains("indices shift")); assert!( p.contains("1-65535"), "Debian prompt must state the valid port range for ufw rules" diff --git a/crates/sysknife-daemon/src/actions/ufw.rs b/crates/sysknife-daemon/src/actions/ufw.rs index ab52eebe..6bd6f54f 100644 --- a/crates/sysknife-daemon/src/actions/ufw.rs +++ b/crates/sysknife-daemon/src/actions/ufw.rs @@ -122,9 +122,21 @@ pub fn ufw_reset() -> ActionSpec { /// /// Risk: Low / Observer. Read-only; no system changes. pub fn ufw_status() -> ActionSpec { + ufw_status_with_numbered(false) +} + +/// Show numbered rule indices for `UfwDeleteRule`, or verbose status otherwise. +pub fn ufw_status_with_numbered(numbered: bool) -> ActionSpec { ActionSpec { action_name: "UfwStatus", - mechanism: command_mechanism("sudo", ["ufw", "status", "verbose"]), + mechanism: command_mechanism( + "sudo", + [ + "ufw", + "status", + if numbered { "numbered" } else { "verbose" }, + ], + ), risk_level: RiskLevel::Low, reboot_required: false, rollback_available: false, @@ -326,6 +338,39 @@ mod tests { let (_, args) = extract_args(&spec); assert!(args.contains(&"status")); assert!(args.contains(&"verbose")); + for (numbered, rendering) in [(false, "verbose"), (true, "numbered")] { + let spec = ufw_status_with_numbered(numbered); + assert_eq!( + extract_args(&spec), + ("sudo", vec!["ufw", "status", rendering]) + ); + assert_eq!(spec.risk_level, RiskLevel::Low); + assert!(!spec.reboot_required && !spec.rollback_available); + } + for (params, rendering) in [ + (serde_json::json!({}), "verbose"), + (serde_json::json!({"numbered": false}), "verbose"), + (serde_json::json!({"numbered": true}), "numbered"), + ] { + let spec = crate::executor::build_action_spec("UfwStatus", ¶ms).unwrap(); + assert_eq!( + extract_args(&spec), + ("sudo", vec!["ufw", "status", rendering]) + ); + } + for invalid in [ + serde_json::json!("true"), + serde_json::json!(1), + serde_json::Value::Null, + ] { + assert!(matches!( + crate::executor::build_action_spec( + "UfwStatus", + &serde_json::json!({"numbered": invalid}) + ), + Err(crate::executor::ExecutorError::InvalidParam("numbered")) + )); + } } #[test] diff --git a/crates/sysknife-daemon/src/executor.rs b/crates/sysknife-daemon/src/executor.rs index 219eec4d..aed13b06 100644 --- a/crates/sysknife-daemon/src/executor.rs +++ b/crates/sysknife-daemon/src/executor.rs @@ -1608,7 +1608,15 @@ pub fn build_action_spec(action_name: &str, params: &Value) -> Result Ok(ufw::ufw_reset()), - "UfwStatus" => Ok(ufw::ufw_status()), + "UfwStatus" => { + let numbered = match params.get("numbered") { + None => false, + Some(value) => value + .as_bool() + .ok_or(ExecutorError::InvalidParam("numbered"))?, + }; + Ok(ufw::ufw_status_with_numbered(numbered)) + } // ── distrobox ──────────────────────────────────────────────────── "DistroboxList" => Ok(distrobox::distrobox_list()), diff --git a/docs/action-reference.md b/docs/action-reference.md index 2e9551d1..59df9200 100644 --- a/docs/action-reference.md +++ b/docs/action-reference.md @@ -338,8 +338,8 @@ Every row is derived from the live code: the command from each action's `ActionS | `UfwAllow` | `sudo ufw allow 22` | High | Ubuntu | – | – | allow inbound traffic on a port or service — param: port_or_service\* (e.g. 22, 22/tcp, OpenSSH); Ubuntu only; High risk | | `UfwDeny` | `sudo ufw deny 23` | High | Ubuntu | – | – | deny inbound traffic on a port or service — param: port_or_service\*; Ubuntu only; High risk | | `UfwReset` | `sudo ufw --force reset` | High | Ubuntu | – | – | reset ufw to defaults, removing all rules — no params; Ubuntu only; High risk; irreversible | -| `UfwStatus` | `sudo ufw status verbose` | Low | Ubuntu | – | – | show current ufw status and rules — no params; Ubuntu only; read-only | -| `UfwDeleteRule` | `sudo ufw --force delete 1` | High | Ubuntu | – | – | delete a ufw rule by number — param: rule_number\* (positive integer from 'ufw status numbered'); Ubuntu only; High risk | +| `UfwStatus` | `sudo ufw status verbose` | Low | Ubuntu | – | – | show current ufw status and rules — optional param: numbered (boolean, default false); true runs ufw status numbered and exposes rule_number values for UfwDeleteRule, false keeps verbose status; read-only | +| `UfwDeleteRule` | `sudo ufw --force delete 1` | High | Ubuntu | – | – | delete a ufw rule by number — param: rule_number\* (positive integer from query_ufw_rules or UfwStatus with numbered=true); never guess a rule number, and refresh after rule changes; High risk | | `UfwLimit` | `sudo ufw limit 22` | High | Ubuntu | – | – | add rate-limiting rule on a port/service (>6 connections/30s blocked) — param: target\* (e.g. '22' or 'ssh'); Ubuntu only; High risk; use for SSH brute-force mitigation | ## netplan diff --git a/docs/typed-actions.md b/docs/typed-actions.md index fa2d3cf7..d71ab95b 100644 --- a/docs/typed-actions.md +++ b/docs/typed-actions.md @@ -6,6 +6,15 @@ allowlist or a regex over a shell command string. SysKnife answers it differently — the model is never given a channel to produce a shell string in the first place. +## Reading ufw rule numbers + +For ufw rule deletion, read current indices with `UfwStatus` and +`{"numbered":true}` (`sysknife_ufw_status` in MCP). During planning, +`query_ufw_rules` performs that read. The default `{}` still returns verbose +status. Pass the selected index to `UfwDeleteRule`; refresh the listing after +rule changes, because indices shift. This does not make listing and deletion +atomic or change the deletion approval requirement. + ## Why string allowlists fail The independent security research known as **GuardFall** tested AI coding