Skip to content

Commit 8ebbc93

Browse files
edimarlnxclaude
andauthored
Per-rule disable/enable with a persisted spec (#17)
* Per-rule disable/enable with a persisted spec nftables has no disabled state: a rule is in the ruleset or it is not. So disabling can only mean deleting the rule while remembering enough to put it back exactly where it was, and that memory has to survive a reboot or the feature is a delete with a friendly name. The memory goes in the file the Save action already writes, as comment lines `nft -f` ignores and this tool parses. The live half stays nft's own listing, byte for byte, so the boot path is untouched and a machine that loses this tool still boots the rules it had; a file written before this existed still loads, and a ruleset with nothing disabled still writes that same v1 file, so nobody who does not press D sees the format at all. D disables the selected rule and D again puts it back at the recorded index. The statement replayed on the way back is captured from the live match when the rule was disabled, by the same rebuild a move uses, so the refusals are the ones E already makes: a rule this tool does not hold in full is refused before anything is deleted. The disabled rule stays on screen as a greyed-out row drawn from the spec rather than from the kernel. Disabling does not join a staged batch. The ruleset change and the record of it have to agree, and a batch defers the one past the other; so it applies on its own and then offers the save, with the header saying `1 disabled, unsaved` until that save happens. Everything read off disk is guarded before it becomes an nft statement or a table cell, with two fuzz targets on the invariants. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCypLQF5AD8JD831Ea9S9q * Bound the LCS table the save diff allocates The Save preview diffs the capture against the file at the save path, and only one of those two is this tool's own output. The other is whatever is on disk, and the diff is a quadratic longest-common-subsequence over lines: a file that is not a table listing — a log somebody redirected there, a ruleset a hundred times this tool's own — asks for a table quadratic in its size before a character is drawn. CodeQL flags the allocation, and it is right to. Past the bound the diff degrades to "the whole file is replaced", which is what installing over it does anyway. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCypLQF5AD8JD831Ea9S9q * Bound each side of the save diff, not the product CodeQL follows the line count of a file this tool did not write into the allocation and does not read the product of two int64s as a bound on either. A plain per-side limit says the same thing and is the same fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCypLQF5AD8JD831Ea9S9q --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 1afcf1e commit 8ebbc93

22 files changed

Lines changed: 2096 additions & 73 deletions

‎DESIGN.md‎

Lines changed: 117 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,119 @@
11
# Design notes
22

3-
## Per-rule enable/disable (not implemented)
4-
5-
The manage feature set (Save, edit in place, move) deliberately leaves
6-
per-rule disable out. nftables has no disabled state: a rule either is in the
7-
ruleset or it is not, so "disable" means deleting the rule while remembering
8-
enough to put it back exactly where it was.
9-
10-
Doing that honestly needs a second persistence format — call it v2 of the
11-
saved file — that stores the tool's own view of the ruleset rather than nft's
12-
listing: a list of rule *specs* (the same `RuleSpec` the add and edit forms
13-
produce), each with an enabled flag and its position. Disabling would then be
14-
"delete the rule, keep its spec"; enabling, "insert the spec at its recorded
15-
position"; and Save would write both the active ruleset (as the `.nft` file it
16-
writes today, still loadable by plain `nft -f` on boot) and the spec sidecar
17-
that carries the disabled entries.
18-
19-
That sidecar is a real format with compatibility duties, and it only pays for
20-
itself once the tool also reconciles it against a ruleset that changed behind
21-
its back. Until that reconciliation exists, a fake disable would silently drop
22-
rules on the floor, so the feature waits for persistence v2 instead of
23-
shipping as a trap.
3+
## Per-rule enable/disable, and the saved-file format
4+
5+
nftables has no disabled state. A rule is in the ruleset or it is not, so
6+
"disable" can only mean deleting the rule while remembering enough to put it
7+
back exactly where it was. That memory is a file format, and this is what it
8+
is.
9+
10+
### The file is still one file, and still an nft script
11+
12+
The Save action (`W`) writes `nft list table inet tui` — nft's own text, which
13+
is a valid nft script — to `/etc/omarchy/router/tui-firewall.nft` on an
14+
Omarchy router or `/etc/nftables.d/tui-firewall.nft` elsewhere. The router
15+
profile loads that file with `nft -f` on boot, and nothing about that may
16+
change: a machine that loses this tool still has to boot the rules it had.
17+
18+
So the disabled rules ride in the same file, in comment lines. `nft -f`
19+
ignores a line starting with `#`; this tool parses those lines and nobody else
20+
has to know they exist.
21+
22+
```
23+
# tui-firewall:spec v2
24+
table inet tui {
25+
chain input {
26+
type filter hook input priority 0; policy drop;
27+
iifname "wan0" tcp dport 22 counter drop comment "no ssh from the wan"
28+
}
29+
}
30+
# tui-firewall:disabled {"index":2,"expr":["iifname","\"wan0\"","udp","dport","53","counter","drop"],"family":"v4","rule":{…}}
31+
```
32+
33+
- `# tui-firewall:spec v2` is the version marker, and it is written **only
34+
when something is actually disabled**. A ruleset with nothing disabled
35+
produces the byte-identical v1 file earlier versions wrote, so a user who
36+
never presses `D` never sees the format change and never sees a diff for it.
37+
- `# tui-firewall:disabled <json>` is one disabled rule, one line.
38+
- A file with no marker is version 1 and loads as "nothing disabled". A marker
39+
naming a version this build does not read is an error, not a guess, and the
40+
save is refused while it stands: overwriting a file whose disabled rules
41+
could not be read would delete them.
42+
43+
A sidecar file was the other option and was rejected. Two files drift, the
44+
boot path would have to learn about the second one, and the operator who
45+
copies `tui-firewall.nft` to another machine would silently leave half the
46+
configuration behind.
47+
48+
### What one disabled entry holds
49+
50+
Two renderings of the same rule, for two different jobs:
51+
52+
- `expr` — the nft statement, one argv word per element, captured from the
53+
live match at the moment the rule was disabled. This is what the re-add
54+
replays. It is a snapshot rather than something recomputed later, so a
55+
ruleset that has been re-read in between cannot change what comes back.
56+
- `rule` — the modelled rule (`Match` and all), because a disabled rule still
57+
has to be a row in the table and the kernel no longer holds it. Plus
58+
`family`, which `Match` derives from the expressions it decoded rather than
59+
from a field nft printed, so it does not survive a JSON round trip on its
60+
own.
61+
- `index` — the 0-based position in its chain the rule goes back to.
62+
63+
`counter` is dropped on the way in: a disabled rule is not counting anything,
64+
and a stale number is a number the row invites the reader to believe.
65+
66+
### Disable and enable
67+
68+
- **Disable** is `nft delete rule … handle H`, plus the entry recorded in the
69+
spec once that command has actually run. The statement is rebuilt from the
70+
rule's own modelled match by the same machinery a move uses
71+
(`rebuildRuleExpr`), so a rule this package does not hold in full — an
72+
unmodelled match, a NAT translation, a verdict other than accept/drop/reject,
73+
a log statement carrying a level or an nflog group — is refused before
74+
anything is deleted. That is the same refusal set `E` makes.
75+
- **Enable** replays `expr` with `nft insert rule … index N`, or `nft add rule`
76+
when the chain has since become shorter than the recorded position. The rule
77+
comes back with a fresh handle and a counter at zero, and the preview says
78+
so.
79+
80+
Both are previewed and confirmed like every other change, and both go through
81+
`internal/nftables` — the only place in this tool that starts a process.
82+
83+
### Why disable does not join a staged batch
84+
85+
Staging collects changes and applies them later as one `nft -f` transaction.
86+
The record of what is disabled is written the moment its command succeeds. If
87+
the ruleset change were staged and the record were not, the file would claim a
88+
rule is disabled while the kernel still enforces it — the exact lie the
89+
preview contract exists to prevent — and a batch cannot carry the record
90+
either, because the record is a file write and an `nft -f` transaction is not.
91+
92+
So `D` applies on its own, as one nft command, and is refused while staging is
93+
collecting or while an applied batch is waiting to be kept. It then **offers
94+
the save**, once the reload has put the new list on screen, because until that
95+
save happens the disabled rule lives only in this process. The header says the
96+
same thing: `rules: 1 disabled, unsaved (W)`.
97+
98+
### Reading a file this tool did not write
99+
100+
Everything that comes off disk is guarded before it becomes an nft statement
101+
or a table cell. A statement word is either a bare operand from a fixed
102+
alphabet or one closed double-quoted string; a table, family or chain name is
103+
a short name with no nft syntax in it; every string the row draws goes through
104+
the same one-line pass nft's own output goes through. A line that does not
105+
parse is an error rather than a skip, because dropping the ones that did not
106+
parse would lose exactly the rules the file exists to preserve.
107+
108+
`FuzzParseSpec` asserts those invariants on arbitrary input, and
109+
`FuzzSaveFileRoundTrip` asserts that anything this package writes, it reads
110+
back.
111+
112+
### What is still open
113+
114+
The spec is not reconciled against a ruleset that changed behind this tool's
115+
back. If something else adds rules to `inet tui` while a rule is disabled, the
116+
recorded position is a position in a chain that has moved; enabling then puts
117+
the rule back at that index, which may not be the neighbour it had. The
118+
position past the end of a shorter chain is handled (it appends); the rest is
119+
a reconciliation pass this tool does not do yet.

‎README.md‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ Upgrades then arrive with the rest of your system updates.
132132
### Any distribution, static binary
133133

134134
```sh
135-
curl -fsSL https://github.com/tui-tools/tui-firewall/releases/download/v0.2.2/tui-firewall_0.2.2_linux_amd64.tar.gz | tar -xz tui-firewall
135+
curl -fsSL https://github.com/tui-tools/tui-firewall/releases/download/v0.3.0/tui-firewall_0.3.0_linux_amd64.tar.gz | tar -xz tui-firewall
136136
sudo install -m0755 tui-firewall /usr/local/bin/tui-firewall
137137
```
138138

@@ -309,6 +309,7 @@ firewall nor `sudo` is available, it says so and points at `--demo`.
309309
| `a` | Add a rule |
310310
| `E` | Edit the selected rule in place — same handle, same position (nftables) |
311311
| `K` / `J` | Move the selected rule up / down, as one atomic transaction (nftables) |
312+
| `D` | Disable or enable the selected rule, keeping its spec and its position (nftables) |
312313
| `d` | Delete the selected rule |
313314
| `e` | Enable or disable the firewall (ufw only) |
314315
| `r` | Reload the firewall |
@@ -467,7 +468,7 @@ is restored automatically when the timer expires, and you are back on the rulese
467468
that was working. It is the standard `iptables-apply` idea, done as one nft
468469
transaction.
469470

470-
### Manage, not view: edit, move and save
471+
### Manage, not view: edit, move, disable and save
471472

472473
The nftables backend does not stop at adding and deleting.
473474

@@ -502,8 +503,36 @@ the usual include from `/etc/nftables.conf` (the dialog spells this out), and
502503
the running table: staged changes that were not applied yet are not in the
503504
file, and the dialog says so.
504505

505-
Per-rule enable/disable is deliberately absent for now; `DESIGN.md` describes
506-
the persisted-spec format it is waiting for.
506+
**Disable.** Press `D` and the selected rule stops being enforced without
507+
being lost. nftables has no per-rule off switch — a rule is in the ruleset or
508+
it is not — so disabling is `nft delete rule … handle H` plus a record of the
509+
statement the rule was and the position it sat at. The rule stays in the table
510+
as a greyed-out row marked `disabled`, drawn from that record rather than from
511+
the kernel, and `D` again puts it back where it was with `nft insert rule …
512+
index N` (or appends it, if the chain has become shorter since). Only rules
513+
the tool holds in full can be disabled: the same refusals `E` makes, for the
514+
same reason.
515+
516+
That record is written to the same file `W` saves, as comment lines `nft -f`
517+
ignores and this tool reads back:
518+
519+
```
520+
# tui-firewall:spec v2
521+
table inet tui {
522+
…
523+
}
524+
# tui-firewall:disabled {"index":2,"expr":["iifname","\"wan0\"","udp","dport","53","counter","drop"],…}
525+
```
526+
527+
The live half of the file is still nft's own listing, byte for byte, so the
528+
boot path is exactly as it was — and a file written before this feature
529+
existed still loads, as a ruleset with nothing disabled. A ruleset with
530+
nothing disabled still *writes* that same v1 file, so nobody who does not use
531+
`D` sees the format at all. Disabling never joins a staged batch (the ruleset
532+
and the record have to change together), and it offers the save straight
533+
afterwards, because until it is saved the disabled rule lives only in the
534+
running process — which is what the `rules: 1 disabled, unsaved (W)` fact in
535+
the header is telling you. `DESIGN.md` has the format in full.
507536

508537
### Per-rule logging and the live log
509538

@@ -583,10 +612,14 @@ are capped, so a firewall under a scan cannot grow it without bound.
583612
- Move a rule up and down with `K` and `J`, as one atomic `nft -f` transaction
584613
(a copy at the new position plus a delete of the old handle), staged into
585614
the batch when staging is on.
615+
- Disable a rule with `D` and enable it again with `D`: the rule leaves the
616+
ruleset and stays on screen as a greyed-out row, rebuilt from the tool's own
617+
persisted spec and put back at the position it had.
586618
- Save the tool's own table to disk with `W` — `nft list table inet tui`
587619
installed to `/etc/omarchy/router/tui-firewall.nft` on an Omarchy router,
588620
`/etc/nftables.d/tui-firewall.nft` elsewhere — with a unified diff against
589-
the current file in the confirm dialog.
621+
the current file in the confirm dialog. The disabled rules ride in the same
622+
file as comments `nft -f` ignores, so it stays loadable on boot as it is.
590623
- Mark a rule to be logged with `l`: the rule is replaced in place, keeping its
591624
handle and position, with a stable `log prefix "tui:<chain> <verdict> "` the
592625
live view greps for. Toggle it off the same way.
@@ -612,8 +645,11 @@ are capped, so a firewall under a scan cannot grow it without bound.
612645
- **No nftables set-element editing**: a named set is created or used, and its
613646
members are added and removed one at a time from the actions menu, not
614647
edited in bulk.
615-
- **No per-rule enable/disable**: nftables has no disabled state, and faking
616-
one honestly needs the persisted-spec format `DESIGN.md` describes.
648+
- **No per-rule enable/disable on ufw or firewalld**: it needs the tool's own
649+
persisted spec, which only the nftables backend has. On nftables the spec is
650+
not reconciled against a ruleset something else changed behind this tool's
651+
back, so a rule enabled after another tool has reshuffled the chain lands at
652+
the recorded index rather than beside the neighbour it had.
617653
- On ufw and firewalld, rules follow the backend's own order; the move keys
618654
are an nftables feature.
619655

‎cmd/tui-firewall/app.go‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88

99
tea "github.com/charmbracelet/bubbletea"
1010
"github.com/tui-tools/tui-firewall/internal/firewall"
11+
"github.com/tui-tools/tui-firewall/internal/nftables"
1112
"github.com/tui-tools/tui-firewall/internal/nftables/staging"
1213
"github.com/tui-tools/tui-kit/compat"
1314
"github.com/tui-tools/tui-kit/theme"
@@ -99,6 +100,17 @@ type app struct {
99100
// the applied batch is on screen before its keep window begins.
100101
keepTickPending bool
101102

103+
// pendingDisable is the toggle the change now at the confirm dialog
104+
// applies, so a yes records it in the backend's spec. It is nil when the
105+
// pending change is something else.
106+
pendingDisable *nftables.Toggle
107+
// pendingSave marks the change at the confirm dialog as the save, so a yes
108+
// clears the "not written yet" state of the spec.
109+
pendingSave bool
110+
// saveOfferPending asks the next load to offer the save, so the disabled
111+
// rule is on screen before the dialog that writes it to disk opens.
112+
saveOfferPending bool
113+
102114
// editing marks the open rule form as an edit: submitting replaces
103115
// editTarget in place rather than adding a rule.
104116
editing bool
@@ -230,6 +242,13 @@ func (a *app) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
230242
a.applyFilter()
231243
// A batch that just applied starts its keep window now, with the applied
232244
// ruleset already on screen, so the operator sees what they are keeping.
245+
// A disable or enable that just applied offers the save that puts the
246+
// tool's record of it on disk; until that happens the record lives only
247+
// in this process.
248+
if a.saveOfferPending {
249+
a.saveOfferPending = false
250+
return a, a.beginSave()
251+
}
233252
if a.keepTickPending && a.staging != nil {
234253
a.keepTickPending = false
235254
token := a.keepToken
@@ -243,7 +262,11 @@ func (a *app) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
243262
case ranMsg:
244263
a.busy = false
245264
wasApply := a.pendingApply
265+
wasDisable := a.pendingDisable
266+
wasSave := a.pendingSave
246267
a.pendingApply = false
268+
a.pendingDisable = nil
269+
a.pendingSave = false
247270
if msg.err != nil {
248271
if wasApply {
249272
// The batch was atomic: nft rejected it, so nothing changed and
@@ -265,6 +288,18 @@ func (a *app) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
265288
}
266289
a.setStatusf(ui.StatusOK, "%s: %s", msg.change.Description, firstLine(summary))
267290
a.loading = true
291+
if wasSave {
292+
if saver, ok := a.backend.(tableSaver); ok {
293+
saver.SpecSaved()
294+
}
295+
}
296+
if wasDisable != nil {
297+
// The rule is out of the ruleset (or back in it): record that now,
298+
// and offer the save once the reload has put the new list on
299+
// screen, so the file being written is the state the user can see.
300+
a.commitToggleDisabled(*wasDisable)
301+
a.saveOfferPending = true
302+
}
268303
return a, a.load()
269304

270305
case keepExpiredMsg:
@@ -347,8 +382,10 @@ func (a *app) handleConfirm(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
347382
change, ok := a.confirm.Payload.(firewall.Change)
348383
a.confirm = ui.Confirm{}
349384
if !confirmed || !ok {
350-
// A cancelled apply must not leave the next run looking like the batch.
385+
// A cancelled change must not leave the next run looking like this one.
351386
a.pendingApply = false
387+
a.pendingDisable = nil
388+
a.pendingSave = false
352389
a.setStatus(ui.StatusInfo, "cancelled")
353390
return a, nil
354391
}
@@ -562,6 +599,8 @@ func (a *app) handleTableKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
562599
return a, a.load()
563600
case "a":
564601
return a, a.startAdd()
602+
case "D":
603+
return a, a.confirmToggleDisabled()
565604
case "E":
566605
return a, a.startEdit()
567606
case "K":

0 commit comments

Comments
 (0)