Conversation
|
Linux CI on 85702d7 now passes all 1,852 Rust tests (6 skipped), with Clippy and rustdoc also passing. The Rust job is red only because the checked-in baseline records 1,851: https://github.com/lacs-project/sysknife/actions/runs/34441868054/job/102758408360 This PR adds one Rust test. Following CONTRIBUTING's supported fallback for contributors without a full local Linux workspace, please regenerate the evidence artifact and accompanying published Rust-test figures with The six firewall-reporter fixture tests pass. Live Debian/LLM validation is still outstanding as discussed in #238; this PR covers the read-only scope of #239. |
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at 85702d79 by reading, which is what this one gets: it touches .github/workflows/, packaging/, a sudoers grant and a new privileged helper, so nothing from it was executed on the review host.
This is the strongest design of the four and I want to say why before the two things that block it.
The helper is not sudo-granted, and that is the whole point. Every other packaging/sysknife-*-edit helper carries a wildcard NOPASSWD: /usr/lib/sysknife/<name> *, which makes it root-callable with arbitrary argv, skipping the preview, the approval receipt and the signed chain. Its own screen becomes the only guard. You did not do that. firewall-state runs as the daemon account, and only its fixed probe commands hold grants:
sysknife ALL=(root) NOPASSWD: /usr/sbin/nft list ruleset
sysknife ALL=(root) NOPASSWD: /usr/sbin/nft -j list ruleset
Argument-restricted, read-only, no wildcard. nft flush ruleset is not permitted by those lines. Compare the existing /usr/sbin/ufw grant, which is the whole binary.
main() refuses every argument (len(sys.argv) != 1), the three probes are fixed argv with no interpolation, sudo -n fails rather than prompting, and the JSON parse validates shape before trusting it and falls back to unknown with a reason. The summary is conservative in the correct direction: absence of evidence is reported as unknown, never as "unfiltered", and the note says so in words that will reach the operator. That is exactly the defect #239 describes.
Blocking: the carve-out tolerates the grant instead of forbidding it
every_referenced_helper_has_a_sudoers_grant exists to make sure a referenced helper is reachable without a password prompt. You correctly needed an exception, because this helper must not be granted. The exception asserts the reporter's mechanism is the bare helper with empty args, then continues.
It never asserts the grant is absent. I added one and the guard stayed green:
$ # append to packaging/sysknife-sudoers:
$ # sysknife ALL=(root) NOPASSWD: /usr/lib/sysknife/firewall-state *
$ cargo test --offline --locked -p sysknife-daemon --test helper_install_coverage
test result: ok. 5 passed; 0 failed
So the property your design depends on is the one thing nothing checks. Today the helper refuses arguments, so the immediate exposure is small; the guard's job is to stop the next person, and it would not. One line fixes it, and I ran it both ways:
assert!(
!sudoers.contains(&expected),
"{expected} must have NO sudoers grant: a wildcard entry would make \
this reporter root-callable with arbitrary argv"
);with the offending grant present: FAILED. 4 passed; 1 failed
panicked at helper_install_coverage.rs:214:
/usr/lib/sysknife/firewall-state must have NO sudoers grant: ...
on your tree unmodified: ok. 5 passed; 0 failed
Blocking: the test baseline
rust is the one red check and it is bookkeeping, not your code. 1852 tests run, 1852 passed, baseline says 1851:
UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh
Yours is the only one of your four that moves the count, so if this merges last it is the only baseline bump anyone has to do. I checked every pair of your four: the only other conflict is CHANGELOG.md, one hunk each, and I will resolve those at merge. Suggested order: #412, #414, #415, then this.
Two non-blocking notes
The two grants landed inside the file's header comment block, between # IMPORTANT: test with visudo and the # Note: Defaults:sysknife !requiretty paragraph, which splits that paragraph in half. Syntactically fine, and this is the one file where somebody skimming the header should not find live grants in the middle of it. Move them down with the others.
GetFirewallBackendState returns the full stdout and stderr of all three probes in probes, alongside the summary. A complete nft -j list ruleset on a real host includes every table and chain, docker and kubernetes NAT rules included, and that payload goes to the model. GetFirewallState already sends ufw rules, so this is more of the same rather than new in kind, but it is a lot more of it. Worth deciding deliberately whether the raw dumps need to travel with the summary or whether the summary is the product.
One thing I liked and nearly missed: the sudo -n nft calls resolve nft through sudo's secure_path while the grants name /usr/sbin/nft. On a host with a narrower secure_path the probe fails and the reporter says unknown, which is the safe direction. Worth a comment saying that is intentional.
Everything else here I would merge as is.
|
Addressed both blockers and the grant placement / secure_path notes in a43efb8:
For the raw-probe note, this revision deliberately retains stdout/stderr for auditability of mixed backends and unknown results. That means full firewall rules can reach the configured model; I made that tradeoff explicit in the PR description. No rule mutation was added. The new CI also found today's RUSTSEC-2026-0285 in main's inherited rustls lockfile. A separate commit updates rustls to 0.23.45 and its required TLS dependencies; security audit now passes. Live Debian/LLM validation remains outstanding. |
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at a43efb8eaf176f48ed83beb230098016cef7d733.
This is a careful piece of work and the security shape is right. Three things I checked and liked before I get to the one item I want changed.
The reporter has no sudo grant, and you did not merely assert that, you made the test enforce it:
if helper == "firewall-state" {
assert!(
!sudoers.lines().filter(|line| !line.trim_start().starts_with('#'))
.any(|line| line.contains(&expected)),
"{expected} must have NO sudoers grant: the reporter must not be root-callable"
);Every other helper in that loop is checked for the presence of a grant. Yours is the one entry checked for absence, with the reason written down. That is the correct inversion and it will stop a future contributor from "fixing" the missing grant.
The two new grants are fixed argv (nft list ruleset and nft -j list ruleset), not a bare nft grant that would also permit nft flush ruleset. And the fail-to-unknown behavior is real: every exception path in probe returns a status that summarize maps to unknown, so a failed probe never becomes an "unfiltered" claim. That is the actual thesis of #239 and you implemented it rather than describing it.
I also confirmed the Python fixtures reach a gate. tests/test_firewall_state.py is wired into ci.yml and ci-local.sh, and the file ends in unittest.main(), so it is not a script that exits 0 having run nothing.
The one blocking item
The payload loses its safety caveat on exactly the hosts that need it most.
query_firewall now routes to GetFirewallBackendState, and the result reaches the model through sanitize_tool_output → normalise_free_text → truncate_with_marker(s, MAX_OUTPUT_BYTES), where MAX_OUTPUT_BYTES = 8 * 1024 (crates/sysknife-brain/src/sanitize.rs:45). Your summarize returns note last, after probes, and probes carries the complete nft -j list ruleset stdout.
I measured it with your own summarize, against a host with one base chain and 120 forward rules, which is modest for anything running Docker:
ruleset json bytes: 21596
total payload bytes: 26587
MAX_OUTPUT_BYTES = 8192 -> truncated at: True
note field survives truncation: False
backends_observed survives: True
nftables summary survives: True
So on a busy host the model receives JSON cut off mid-string, and the field it loses is this one:
"Empty/failed probes do not prove the host is unfiltered; legacy iptables and other mechanisms may exist."
That sentence is the point of the PR. Dropping it on the hosts with the largest rulesets inverts the intent: the more complex the firewall, the less likely the model is to be told that absence of evidence is not evidence of absence. The summary fields survive, which is the saving grace, but the caveat does not.
Two small changes fix it, and I would take both:
- Emit
note(and keepstate,backends_observed,nftables) beforeprobes, so the interpretation survives any truncation. - Bound each probe's
stdoutinside the helper with an explicit marker, rather than relying on a downstream cap that was sized for command output and knows nothing about your JSON. A few KB of ruleset is plenty for the model to reason about, and the full text is still available locally.
That also happens to reduce how much of a host's complete firewall topology gets shipped to the configured model provider, which you flagged honestly in the PR body. I would rather bound it at the source than have it bounded by accident at 8 KiB.
Not blocking
nft_state["status"] reads "rules_present" when hooks is non-empty but rules is 0. A host with base chains and no rules gets a status that says rules are present next to "rule_count": 0. "chains_present" or similar would read better. Cosmetic.
For the record, security-audit red on this branch is RUSTSEC-2026-0285 against rustls, which also affects main at 61b3a878. Your lockfile bump is the correct fix and I verified it clears cargo audit (rc=0). That part is not a problem.
Requesting changes only on the truncation ordering. Everything else here is ready, and this is the most careful helper anyone has added to packaging/ so far.
An inactive or absent ufw frontend does not establish that a Debian host is unfiltered. This adds
GetNftablesRulesetwith fixedsudo nft list rulesetargv and routesquery_firewallthrough a read-only backend reporter. The reporter preserves nftables/ufw/firewalld observations, permits coexistence, and reports unknown on empty or failed evidence. ExistingGetFirewallStateretains its firewalld-specific meaning.Addresses the read-only scope of #239. Includes exact nft sudoers grants, helper installation, catalogue/MCP/preview registration, generated action reference, and six Python fixture tests. No rule mutation or automatic ufw refusal is introduced. Details:
docs/firewall-observations.md.The reporter itself must have no sudoers grant. The helper-install regression now explicitly rejects any non-comment sudoers line referencing its path; adding the forbidden wildcard grant makes it fail. The two fixed nft grants are in the network section, and the helper documents intentional fail-to-unknown behavior when sudo's secure_path cannot resolve nft.
Merged main at 61b3a87 and regenerated the catalogue reference. The full Linux workspace on c396703 passed 1,853 tests with 6 skipped; the official recorder and published counts now reflect that observed result. Evidence: https://github.com/lacs-project/sysknife/actions/runs/34866753371/job/104052386147
CI subsequently exposed the newly published RUSTSEC-2026-0285 in the inherited rustls lockfile. A separate commit upgrades rustls to 0.23.45 and required TLS dependencies. Final head a43efb8 passes all 1,853 Linux workspace tests (6 skipped), Clippy, rustdoc, frontend, Postgres contract, hygiene and security audit: https://github.com/lacs-project/sysknife/actions/runs/34867745479
Local validation: six reporter fixtures and five production-source helper-install tests pass; the forbidden-grant mutation fails as intended; formatting/diff checks pass. Live Debian firewall/LLM stories remain NOT_TESTED. Probe stdout/stderr is deliberately retained to make mixed-backend and unknown results auditable; it can contain complete host firewall rules and is sent to the configured model.
Codex assisted implementation and validation.