Skip to content

feat(network): report nftables and firewall backend observations - #413

Merged
vladimirrott merged 10 commits into
lacs-project:mainfrom
QinXi-ai:fix/239-nftables-state
Sep 15, 2026
Merged

vladimirrott merged 10 commits into
lacs-project:mainfrom
QinXi-ai:fix/239-nftables-state

Conversation

@QinXi-ai

@QinXi-ai QinXi-ai commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

An inactive or absent ufw frontend does not establish that a Debian host is unfiltered. This adds GetNftablesRuleset with fixed sudo nft list ruleset argv and routes query_firewall through a read-only backend reporter. The reporter preserves nftables/ufw/firewalld observations, permits coexistence, and reports unknown on empty or failed evidence. Existing GetFirewallState retains 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 nine 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

The follow-up at 2fd71f8 addresses output truncation: state, backend observations, nftables counts and the safety note precede diagnostics. Summaries use complete probe output, then each stdout/stderr excerpt is bounded to 1,024/512 JSON-encoded bytes, including escaping and an explicit truncation marker. The complete JSON fits the planner cap even when all six streams are oversized; short output is unchanged. Excerpts can still expose some firewall topology to the configured model. Full output can be inspected by running the fixed read-only commands locally.

Current local validation: all nine reporter fixtures pass, including 120-rule output, oversized Unicode/escaped streams, input preservation and reading the actual brain cap. The new cases fail on the previous helper. An isolated harness importing the actual Rust sanitizer also preserves parseable JSON and the complete caveat for large rulesets, Unicode, escaping and repeated prompt-envelope tags (the latter expands from 5,144 to 7,088 bytes, still below 8 KiB). Formatting/diff checks pass. Prior helper-install coverage passes with the forbidden-grant mutation failing as intended. No Rust test-count change. Live Debian firewall/LLM stories remain NOT_TESTED. Fresh CI: https://github.com/lacs-project/sysknife/actions/runs/34918740072

Codex assisted implementation and validation.

@QinXi-ai

Copy link
Copy Markdown
Contributor Author

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 UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh before merge. I have left those measured-baseline files untouched rather than editing a count by hand. The action catalogue figures and generated reference are updated independently.

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 vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@QinXi-ai

Copy link
Copy Markdown
Contributor Author

Addressed both blockers and the grant placement / secure_path notes in a43efb8:

  • The reporter carve-out now rejects every non-comment sudoers line referencing /usr/lib/sysknife/firewall-state. I appended the forbidden wildcard grant to the actual checkout: the helper-install guard failed; after removing it, all five tests passed. The helper remains unprivileged with empty argv.
  • Merged main at 61b3a87. Full Linux CI measured 1,853 passing tests (6 skipped), and the official recorder plus all three published count locations now reflect that result. Final-head CI, including the baseline guard, passes: https://github.com/lacs-project/sysknife/actions/runs/34867745479
  • Moved the two fixed nft grants into the network section and documented intentional fail-to-unknown behavior when sudo's secure_path cannot resolve nft.

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 vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_outputnormalise_free_texttruncate_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:

  1. Emit note (and keep state, backends_observed, nftables) before probes, so the interpretation survives any truncation.
  2. Bound each probe's stdout inside 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.

@QinXi-ai

Copy link
Copy Markdown
Contributor Author

Addressed both truncation requests in 2fd71f8.

  • state, backends_observed, nftables and note now precede probes.
  • The helper computes summaries from the complete original probe output, then bounds stdout/stderr to 1,024/512 JSON-encoded bytes per probe. Budgets include quotes, escapes and the explicit [truncated by firewall-state] marker, so Unicode/escape-heavy diagnostics cannot defeat the cap. Short output and probe failure/return codes are preserved.
  • Added three regression tests (nine reporter tests total): a 120-rule fixture retains its full rule count and caveat; all six oversized streams still produce complete JSON under the actual brain cap; short diagnostics remain unchanged. The new large-output cases failed against the previous helper and pass now.

I also ran the actual normalise_free_text implementation through an isolated Rust harness, with large rulesets, Unicode, escaped text and repeated prompt-envelope tags. All four outputs remain valid JSON with the full caveat. Even sanitizer expansion of repeated tags stays below the cap: 5,144 input bytes -> 7,088 sanitized bytes.

Docs and the changelog now describe bounded excerpts and local access to complete output. Fresh CI: https://github.com/lacs-project/sysknife/actions/runs/34918740072 . Live-host validation remains outside these fixture/sanitizer checks.

vladimirrott
vladimirrott previously approved these changes Sep 15, 2026

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix I was hoping for, and doing both halves matters: moving note ahead of probes keeps the caveat in the first bytes, and bounding each stream at source means it is never close to the edge in the first place.

I drove your own summarize with a synthetic ruleset rather than reading the diff:

tree rules nft -j stdout payload truncated at 8 KiB caveat survives
a43efb8e 120 31,858 B 41,293 B yes no
2fd71f8b 120 31,858 B 2,670 B no yes
2fd71f8b 2,000 536,949 B 2,671 B no yes

The payload stops growing at 2,671 bytes against MAX_OUTPUT_BYTES = 8192, so a host with a large ruleset now gets the same caveat as a quiet one. That was the whole point of the finding.

The part I checked hardest is that bounding changed no conclusion. I compared nftables, backends_observed and state between the two heads at 0, 5, 120 and 2,000 rules: identical at every size, because you parse the complete nft stdout and cap only the copy that goes on the wire. The self.assertEqual(json.loads(original["stdout"])["nftables"], entries) line in the new test pins that, which is better than my checking it once.

Then I broke it four ways to see whether the new tests bite:

mutation result
put probes back before note test_large_ruleset_keeps_summary_and_caveat_before_bounded_probes fails
drop bounded_probe, emit raw probes 3 failures, including both escaping subtests
raise STDOUT_JSON_BYTES to 1,000,000 3 failures
return the prefix without TRUNCATION_MARKER 3 failures

Clean run is 9 passed, restored tree is 9 passed. Reading MAX_OUTPUT_BYTES out of sanitize.rs with a regex instead of writing 8192 into the test is the right call: a future reduction of the brain's cap breaks this test instead of silently invalidating the budget. Same for the multibyte and backslash cases, since the budget is counted after JSON escaping and that is where a naive len() would be wrong.

Approving at 2fd71f8b. The workflow diff is one step, python tests/test_firewall_state.py, and I read it.

One thing that is not blocking. bounded_text charges len(json.dumps(char)) - 2 per character, which is exactly right while the final serialisation uses the default ensure_ascii=True. If anyone later passes ensure_ascii=False in main, every non-ASCII character costs fewer bytes than the budget charged it and the accounting over-reserves without saying so. One line next to STDOUT_JSON_BYTES saying the budget assumes ASCII-escaped output would keep that from being rediscovered.

On landing: tests/evidence/workspace-tests.json moves 1852 to 1853 here, and #424 and #426 carry a byte-identical hunk, so git merges them without a conflict and whichever lands second is stale. That is mine to regenerate at merge, not yours to chase.

@vladimirrott

Copy link
Copy Markdown
Member

Not blocking, and nothing for you to push. I wanted the worst-case wire size under the one transform in the brain that makes a payload larger, so I measured it at 2fd71f8b.

normalise_unbounded_text rewrites <untrusted_tool_output and <user_preferences into a BLOCKED_ form (crates/sysknife-brain/src/sanitize.rs:201-209), eight bytes per occurrence, and that runs before truncate_with_marker. An nft rule comment is free-form text that lands verbatim in nft -j list ruleset, so those bytes can reach your excerpts. I filled all six streams with each trigger, ran your summarize, and applied the same substitution the Rust does:

plain x                  payload= 5144  after tag rewrite= 5144  cap=8192  headroom= 3048
<user_preferences        payload= 5144  after tag rewrite= 7208  cap=8192  headroom=  984
<untrusted_tool_output   payload= 5144  after tag rewrite= 6728  cap=8192  headroom= 1464

<user_preferences is the worse of the two because it is the shorter trigger, so more rewrites fit inside the same 1,024 bytes. 7,208 against 8,192 holds. That sits close to the 7,088 you reported through the real sanitizer; mine models only the growing step and skips the whitespace collapse, so it reads as an upper bound rather than a disagreement.

What the number means for later: your budget of 3 × (1024 + 512) = 4608 excerpt bytes leaves about 12% of the cap once an adversarial ruleset is rewritten. Raising STDOUT_JSON_BYTES to 2,048 spends all of it. test_all_probe_streams_are_bounded_after_json_escaping covers backslashes and multibyte text, not this, so the day somebody raises that constant nothing goes red. One more entry in that loop's list, "<user_preferences" * 5000, closes it.

I am not asking you to push for that, and the approval stands as it is. If you would rather leave this branch alone I will carry it as a follow-up.

# Conflicts:
#	CHANGELOG.md
#	CONTRIBUTING.md
#	Makefile
#	README.md
#	docs/distro-support.md
#	docs/introduction.md
#	tests/evidence/workspace-tests.json

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving at 22fb8e3. Everything since your 2fd71f8 is mine, and none of it touches your code.

Eight pull requests landed here tonight and yours branched before all of them, so this needed main merged in. Seven files conflicted: CHANGELOG.md, CONTRIBUTING.md, Makefile, README.md, docs/distro-support.md, docs/introduction.md and tests/evidence/workspace-tests.json. Every one is churn from the order I merged things in.

The part worth telling you about is what did not conflict. packaging/sysknife-sudoers auto-merged, and #417 rewrote that file tonight to drop the blanket sh and runuser grants. I checked the result rather than trusting it: the merged file is main's plus exactly your four lines.

+# Fixed read-only nftables probes; the reporter itself has NO sudo grant.
+sysknife ALL=(root) NOPASSWD: /usr/sbin/nft list ruleset
+sysknife ALL=(root) NOPASSWD: /usr/sbin/nft -j list ruleset

Nine action-steps grants still present, zero shell or runuser grants reintroduced. crates/sysknife-daemon/src/actions/network.rs and executor.rs auto-merged too, and #417 had also edited both, so I compared my whole merge tree against git's own three-way result: the seven files above are the only paths that differ. Nothing of mine reached the rest.

Makefile now installs both helpers, yours and #417's. docs/action-reference.md is generated, so I regenerated rather than hand-merging it, and it came back byte-identical to the auto-merge.

Three derived figures moved, all measured rather than computed:

Summary [11.758s] 1861 tests run: 1861 passed, 6 skipped
test_baseline: recorded 1861 rust tests

The claim screen then caught two couplings I had missed, both caused by your new actions rather than by the merge: the catalogue went 190 to 192, and the uncovered-action count in CONTRIBUTING.md went 59 to 61 because your two new actions have no story yet. Both fixed, and it passes:

Published figures match the evidence artifacts.
Public claims contract passed.

ci-local: PASS locally. Nothing left for you. I will merge once the board is green and the gate has its receipt.

@vladimirrott
vladimirrott merged commit 08c08ef into lacs-project:main Sep 15, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants