Skip to content

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

Open
QinXi-ai wants to merge 8 commits into
lacs-project:mainfrom
QinXi-ai:fix/239-nftables-state
Open

QinXi-ai wants to merge 8 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 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.

@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.

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