Skip to content

Commit 0b02832

Browse files
edimarlnxclaude
andcommitted
Manage, not view: persistence (Save), rule edit and move
The nftables backend could create rules, NAT and aliases but everything it did was runtime-only and immutable: a reboot reverted the ruleset, and the only way to change a rule was delete-and-re-add. This closes those gaps. Save (W): the tool's own table is captured with `nft list table inet tui` and installed with `install -m 644 /dev/stdin <path>` — the listing rides on stdin, never on a command line. The path is /etc/omarchy/router/tui-firewall.nft on an Omarchy router (the profile includes it on boot), /etc/nftables.d/tui-firewall.nft elsewhere, and TUI_FIREWALL_SAVE_PATH overrides it for tests. The confirm dialog shows a unified diff against the file as it stands. install(1) gets a runner of its own inside the backend, so the exec boundary stays where it was. Edit (E): the add-rule form opens pre-filled from the rule's modelled match and submits as `nft replace rule … handle H`, the same replace-by-handle rebuild the log toggle uses, with the same refusals — a rule shown only as text, a NAT rule, a source-port match or a log level the form cannot round-trip is refused before the form opens. Move (K/J): a copy written at the new position plus a delete of the old handle, always one `nft -f` transaction — staged into the atomic batch when staging is on, wrapped by the new staging.AtomicCommand when it is off — so the rule is never duplicated or missing in between. Per-rule enable/disable is deliberately left out; DESIGN.md records the persisted-spec format it waits for. The demo applies every new command in memory, so --demo behaves exactly like the real backend, and the new builders carry injection-guard and refusal tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCypLQF5AD8JD831Ea9S9q
1 parent 6fc9ddb commit 0b02832

21 files changed

Lines changed: 1939 additions & 31 deletions

‎DESIGN.md‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
# Design notes
2+
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.

‎README.md‎

Lines changed: 61 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -307,13 +307,16 @@ firewall nor `sudo` is available, it says so and points at `--demo`.
307307
| `pgup` / `pgdn` | Scroll a page |
308308
| `/` | Filter rules (matches any column; `esc` clears) |
309309
| `a` | Add a rule |
310+
| `E` | Edit the selected rule in place — same handle, same position (nftables) |
311+
| `K` / `J` | Move the selected rule up / down, as one atomic transaction (nftables) |
310312
| `d` | Delete the selected rule |
311313
| `e` | Enable or disable the firewall (ufw only) |
312314
| `r` | Reload the firewall |
313315
| `p` | Change a default policy (ufw) or a zone target (firewalld) |
314316
| `L` | Change the logging level (ufw) or the log-denied value (firewalld) |
315317
| `l` | Toggle logging on the selected rule (nftables) |
316318
| `w` | Watch the live firewall log of the logged rules (nftables) |
319+
| `W` | Save the tool's own table to a file loaded on boot, with a diff preview (nftables) |
317320
| `x` | Actions this backend offers beyond these keys |
318321
| `[` / `]` | Previous / next group — the firewalld zones and policy objects |
319322
| `R` | Re-read the firewall |
@@ -464,6 +467,44 @@ is restored automatically when the timer expires, and you are back on the rulese
464467
that was working. It is the standard `iptables-apply` idea, done as one nft
465468
transaction.
466469

470+
### Manage, not view: edit, move and save
471+
472+
The nftables backend does not stop at adding and deleting.
473+
474+
**Edit in place.** Press `E` on a rule and the add-rule form opens pre-filled
475+
with the rule as it stands — interfaces, connection state, alias source,
476+
comment, all of it read back from the modelled match. Submitting builds one
477+
`nft replace rule … handle H`, the same replace-by-handle the log toggle uses,
478+
so the rule keeps its handle and its position. A rule the tool cannot hold in
479+
full — one with a match shown only as text, a NAT translation, a source-port
480+
match, a log statement with a level or an nflog group — is refused before the
481+
form opens, with the reason, rather than silently rewritten with half of
482+
itself missing.
483+
484+
**Move.** `K` and `J` move a rule one position up or down. nftables has no
485+
move command, so a move is a copy written at the new position plus a delete of
486+
the old handle — two statements that only make sense together, which is why
487+
they always run as **one `nft -f` transaction**: wrapped as one when staging
488+
is off, or staged into the atomic batch when it is on. The rule is never
489+
duplicated or missing in between; the moved copy gets a fresh handle and a
490+
reset counter, and the preview says so.
491+
492+
**Save.** Everything above changes the *running* ruleset, which a reboot
493+
throws away. Press `W` and the tool serialises its own table — `nft list
494+
table inet tui`, nft's own text, which is a valid nft script — and installs it
495+
with `install -m 644 /dev/stdin <path>`. The confirm dialog shows a **unified
496+
diff** against the file as it currently stands, so "what does saving change"
497+
is answered before a byte is written. On an Omarchy router the file is
498+
`/etc/omarchy/router/tui-firewall.nft`, which the router profile includes on
499+
boot; on any other machine it is `/etc/nftables.d/tui-firewall.nft`, loaded by
500+
the usual include from `/etc/nftables.conf` (the dialog spells this out), and
501+
`TUI_FIREWALL_SAVE_PATH` overrides the path for tests and labs. Saving writes
502+
the running table: staged changes that were not applied yet are not in the
503+
file, and the dialog says so.
504+
505+
Per-rule enable/disable is deliberately absent for now; `DESIGN.md` describes
506+
the persisted-spec format it is waiting for.
507+
467508
### Per-rule logging and the live log
468509

469510
Press `l` on a rule to mark it for logging. nft rules are immutable, so the rule
@@ -535,6 +576,17 @@ are capped, so a firewall under a scan cannot grow it without bound.
535576
table, and say why when it will not. Delete by handle.
536577
- Stage changes and apply them as one atomic `nft -f` transaction, with an
537578
automatic connectivity-safe rollback if the apply is not kept in time.
579+
- Edit a rule in place with `E`: the form opens pre-filled from the rule's own
580+
modelled match and the submit is one `nft replace rule … handle H`, keeping
581+
the handle and the position; a rule the model does not hold in full is
582+
refused, never half-rewritten.
583+
- Move a rule up and down with `K` and `J`, as one atomic `nft -f` transaction
584+
(a copy at the new position plus a delete of the old handle), staged into
585+
the batch when staging is on.
586+
- Save the tool's own table to disk with `W` — `nft list table inet tui`
587+
installed to `/etc/omarchy/router/tui-firewall.nft` on an Omarchy router,
588+
`/etc/nftables.d/tui-firewall.nft` elsewhere — with a unified diff against
589+
the current file in the confirm dialog.
538590
- Mark a rule to be logged with `l`: the rule is replaced in place, keeping its
539591
handle and position, with a stable `log prefix "tui:<chain> <verdict> "` the
540592
live view greps for. Toggle it off the same way.
@@ -545,7 +597,8 @@ are capped, so a firewall under a scan cannot grow it without bound.
545597

546598
## What v0.1 cannot do
547599

548-
- **No rule editing.** Change a rule by deleting it and adding the new one.
600+
- **No rule editing on ufw or firewalld.** Change a rule by deleting it and
601+
adding the new one; only the nftables backend edits in place with `E`.
549602
- **No `ufw` interface qualifiers** (`ufw allow in on eth0 …`): they are parsed
550603
and displayed, but the form cannot create them.
551604
- **No `ufw` application profile management** (only using existing profiles).
@@ -556,10 +609,13 @@ are capped, so a firewall under a scan cannot grow it without bound.
556609
editable here.
557610
- **No live log tail on ufw or firewalld** — it is an nftables feature, since it
558611
reads the kernel log the nftables `log` statement writes.
559-
- **No nftables rule editing or reordering**, and no set-element editing: a rule
560-
is changed by deleting it and adding the new one, and a named set is created or
561-
used, not edited member by member.
562-
- Rules follow the backend's own order; there is no reordering key.
612+
- **No nftables set-element editing**: a named set is created or used, and its
613+
members are added and removed one at a time from the actions menu, not
614+
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.
617+
- On ufw and firewalld, rules follow the backend's own order; the move keys
618+
are an nftables feature.
563619

564620
## Compatibility
565621

‎cmd/tui-firewall/app.go‎

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,11 @@ type app struct {
9999
// the applied batch is on screen before its keep window begins.
100100
keepTickPending bool
101101

102+
// editing marks the open rule form as an edit: submitting replaces
103+
// editTarget in place rather than adding a rule.
104+
editing bool
105+
editTarget firewall.Rule
106+
102107
// live holds the live-log view: the open stream, the events retained, and
103108
// whether the feed is paused. It is only reachable on the nftables backend
104109
// and its demo, the ones that implement logStreamer.
@@ -273,6 +278,14 @@ func (a *app) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
273278
a.openApplyConfirm(msg.change)
274279
return a, nil
275280

281+
case saveReadyMsg:
282+
if msg.err != nil {
283+
a.setStatus(ui.StatusError, msg.err.Error())
284+
return a, nil
285+
}
286+
a.openSaveConfirm(msg)
287+
return a, nil
288+
276289
case logEventMsg:
277290
return a, a.handleLogEvent(msg)
278291

@@ -452,6 +465,7 @@ func (a *app) handleForm(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
452465
switch msg.String() {
453466
case "esc":
454467
a.mode = modeTable
468+
a.editing = false
455469
a.setStatus(ui.StatusInfo, "cancelled")
456470
return a, nil
457471
case "tab", "down":
@@ -484,18 +498,32 @@ func (a *app) handleForm(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
484498
return a, a.form.updateActive(msg)
485499
}
486500

487-
// submitForm builds the add command from the form and opens the confirm dialog.
501+
// submitForm builds the add — or, in edit mode, the replace-in-place — command
502+
// from the form and opens the confirm dialog.
488503
func (a *app) submitForm() tea.Cmd {
489504
spec, err := a.form.spec()
490505
if err != nil {
491506
a.setStatus(ui.StatusError, err.Error())
492507
return nil
493508
}
494-
change, err := a.backend.BuildAddRule(a.group, spec)
509+
var change firewall.Change
510+
if a.editing {
511+
editor, ok := a.backend.(ruleEditor)
512+
if !ok {
513+
// Unreachable: editing is only set by startEdit, which required the
514+
// interface. Refusing beats replacing a rule with an add.
515+
a.setStatus(ui.StatusError, "this backend cannot edit a rule in place")
516+
return nil
517+
}
518+
change, err = editor.BuildEditRule(a.group, a.editTarget, spec)
519+
} else {
520+
change, err = a.backend.BuildAddRule(a.group, spec)
521+
}
495522
if err != nil {
496523
a.setStatus(ui.StatusError, err.Error())
497524
return nil
498525
}
526+
a.editing = false
499527
a.stageOrConfirm(change.Description, "The firewall will be changed as follows.", change)
500528
return nil
501529
}
@@ -534,6 +562,14 @@ func (a *app) handleTableKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
534562
return a, a.load()
535563
case "a":
536564
return a, a.startAdd()
565+
case "E":
566+
return a, a.startEdit()
567+
case "K":
568+
return a, a.confirmMove(-1)
569+
case "J":
570+
return a, a.confirmMove(1)
571+
case "W":
572+
return a, a.beginSave()
537573
case "s":
538574
a.toggleStaging()
539575
case "S":
@@ -575,6 +611,7 @@ func (a *app) startAdd() tea.Cmd {
575611
return a.openExtrasMenu()
576612
default:
577613
a.form = newRuleForm(a.caps, a.model.Services)
614+
a.editing = false
578615
a.mode = modeForm
579616
return nil
580617
}

‎cmd/tui-firewall/edit.go‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
package main
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
7+
tea "github.com/charmbracelet/bubbletea"
8+
"github.com/tui-tools/tui-firewall/internal/firewall"
9+
"github.com/tui-tools/tui-firewall/internal/nftables/staging"
10+
"github.com/tui-tools/tui-kit/ui"
11+
)
12+
13+
// ruleEditor is the part of a backend the edit-in-place flow needs: the spec a
14+
// rule decomposes into, to pre-fill the form, and the replace-by-handle
15+
// command the submitted form builds. Only the nftables backend and its demo
16+
// implement it.
17+
type ruleEditor interface {
18+
RuleSpecFor(group string, rule firewall.Rule) (firewall.RuleSpec, error)
19+
BuildEditRule(group string, rule firewall.Rule,
20+
spec firewall.RuleSpec) (firewall.Change, error)
21+
}
22+
23+
// ruleMover is the part of a backend the move keys need. The Change it builds
24+
// is a copy plus a delete that only make sense as one transaction, which the
25+
// caller guarantees: staged into the atomic batch, or wrapped in a single
26+
// `nft -f` when staging is off.
27+
type ruleMover interface {
28+
BuildMoveRule(group string, rule firewall.Rule,
29+
delta int) (firewall.Change, error)
30+
}
31+
32+
// startEdit opens the add-rule form pre-filled with the selected rule, in edit
33+
// mode: submitting replaces the rule in place, keeping its handle and
34+
// position. A rule the backend cannot decompose into the form is refused here,
35+
// before the form opens, with the backend's own reason.
36+
func (a *app) startEdit() tea.Cmd {
37+
editor, ok := a.backend.(ruleEditor)
38+
if !ok {
39+
a.setStatus(ui.StatusWarn,
40+
"this backend does not edit a rule in place: delete it with d and "+
41+
"add the new one with a")
42+
return nil
43+
}
44+
rule, ok := a.selectedRule()
45+
if !ok {
46+
a.setStatus(ui.StatusWarn, "no rule selected")
47+
return nil
48+
}
49+
spec, err := editor.RuleSpecFor(a.group, rule)
50+
if err != nil {
51+
a.setStatus(ui.StatusError, err.Error())
52+
return nil
53+
}
54+
a.form = newRuleForm(a.caps, a.model.Services)
55+
a.form.title = fmt.Sprintf("Edit rule (replaces handle %s in place)", rule.ID)
56+
a.form.prefill(spec)
57+
a.editing = true
58+
a.editTarget = rule
59+
a.mode = modeForm
60+
return nil
61+
}
62+
63+
// confirmMove builds the up-or-down move for the selected rule. With staging
64+
// on it joins the pending batch, where the whole batch is already one nft
65+
// transaction; with staging off it is wrapped as a single `nft -f` so the copy
66+
// and the delete still apply together or not at all.
67+
func (a *app) confirmMove(delta int) tea.Cmd {
68+
mover, ok := a.backend.(ruleMover)
69+
if !ok {
70+
a.setStatus(ui.StatusWarn,
71+
"this backend does not reorder rules; rule order is only "+
72+
"meaningful on nftables")
73+
return nil
74+
}
75+
rule, ok := a.selectedRule()
76+
if !ok {
77+
a.setStatus(ui.StatusWarn, "no rule selected")
78+
return nil
79+
}
80+
change, err := mover.BuildMoveRule(a.group, rule, delta)
81+
if err != nil {
82+
a.setStatus(ui.StatusError, err.Error())
83+
return nil
84+
}
85+
if a.stagingOn && a.staging != nil {
86+
a.stageOrConfirm(change.Description, "", change)
87+
return nil
88+
}
89+
90+
atomic := staging.AtomicCommand(change)
91+
a.mode = modeConfirm
92+
a.confirm = ui.Confirm{
93+
Title: change.Description,
94+
Body: "The statements below apply as one nft -f transaction: both of " +
95+
"them, or neither.\n" + change.Note + ".",
96+
Command: strings.TrimRight(atomic.Stdin, "\n"),
97+
Danger: true,
98+
Payload: firewall.One(atomic),
99+
}
100+
return nil
101+
}

0 commit comments

Comments
 (0)