Merge pull request 'Add boot unit applying a local config' (#34) from benvin/boot-unit into main

Reviewed-on: #34

.woodpecker/release.yaml

@@ -59,7 +59,7 @@ steps:
             cpu: 2
 
   - name: package
-    image: git.unkin.net/unkin/almalinux9-rpmbuilder:latest
+    image: artifactapi.k8s.syd1.au.unkin.net/docker-internal/rpmbuilder:0.1.0-alma9
     commands:
       - ./scripts/build-rpm.sh ${CI_COMMIT_TAG}
     depends_on: [build]

DESIGN.md

@@ -430,6 +430,13 @@ report the generation applied, giving a fleet-wide "converged / N behind" view.
   source/dest disables its rule loudly, never opens it.
 - **Adds fail closed, the control plane fails open.** Partial rollout blocks new
   flows until every hop converges; a dead API leaves the last-good posture running.
+- **A generation that severs the API is reverted.** The agent applies as a
+  `tomswall try` does (on-disk snapshot, systemd revert timer, shared lock), then
+  reports `applied` over a fresh connection. If that fails at the transport level,
+  or the apply errors, it records the generation in
+  `/var/lib/tomswall/reverted.json` (skipped until a newer one arrives), restores
+  the snapshot and reports `reverted`/`failed`. A failed restore leaves the timer
+  to retry it and is reported `failed`.
 
 ---
 

cmd/tomswall/agent.go

@@ -29,7 +29,8 @@ func agentCmd() *cobra.Command {
 		Long: `Agent runs the control-plane pull loop: it fetches this device's compiled
 config from tomswallapi, differentially applies it, and reports the applied
 generation back. It caches the last known-good config and, if the control plane
-is unreachable, keeps applying that cache — it never fails closed.
+is unreachable, keeps applying that cache — it never fails closed. A new
+generation that cuts the agent off from the API is reverted and reported as such.
 
 The agent token defaults to the TOMSWALL_AGENT_TOKEN environment variable, and
 the device name defaults to the system hostname.`,

cmd/tomswall/try.go

@@ -89,10 +89,10 @@ func tryApply(cfg *config.Config, fallback time.Duration) (string, error) {
 		return "", err
 	}
 	if err := engine.Apply(changes); err != nil {
-		if derr := tryapply.Discard(); derr != nil {
+		if aerr := tryapply.Abort(); aerr != nil {
-			err = fmt.Errorf("%w (discarding snapshot: %v)", err, derr)
+			return "", fmt.Errorf("applying changes: %w; %v; the revert timer restores the previous ruleset within %s", err, aerr, fallback)
 		}
-		return "", fmt.Errorf("applying changes: %w", err)
+		return "", fmt.Errorf("applying changes: %w: previous ruleset restored", err)
 	}
 	return id, nil
 }

internal/agent/agent.go

@@ -2,8 +2,13 @@ package agent
 
 import (
 	"context"
+	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
+	"net/url"
+	"os"
+	"path/filepath"
 	"time"
 
 	"git.unkin.net/unkin/tomswall/internal/config"
@@ -12,11 +17,17 @@ import (
 )
 
 // Applier applies a translated config to the firewall. Abstracted so the run
-// loop is testable without touching the kernel.
+// loop is testable without touching the kernel. With safe, a change is applied
+// as a pending try: revert restores the previous ruleset (a failed revert leaves
+// the revert timer armed) and keep drops the snapshot. Both are nil when nothing
+// changed or safe is false.
 type Applier interface {
-	Apply(ctx context.Context, cfg *config.Config) error
+	Apply(ctx context.Context, cfg *config.Config, safe bool) (revert, keep func() error, err error)
 }
 
+// revertDelay is when the revert timer fires if the agent dies mid-apply.
+const revertDelay = time.Minute
+
 // Agent runs the pull-apply-report loop for one device.
 type Agent struct {
 	Client   *Client
@@ -25,6 +36,9 @@ type Agent struct {
 	Applier  Applier
 	// Resolver overrides the DNS resolver (tests); nil derives it per-config.
 	Resolver *Resolver
+
+	// lastReverted covers a reverted generation whose persistence failed.
+	lastReverted *reverted
 }
 
 // Run loops until ctx is cancelled, applying one cycle per Interval (and once
@@ -62,16 +76,31 @@ func (a *Agent) RunOnce(ctx context.Context) error {
 			return fmt.Errorf("control plane unreachable and no cached config: %w", err)
 		}
 		// Re-apply last known-good; do not report a generation we didn't fetch.
-		return a.applyConfig(ctx, cached, false)
+		return a.applyConfig(ctx, cached, nil)
 	}
 
-	if err := a.Cache.Write(raw); err != nil {
+	rv, err := a.readReverted()
-		slog.Warn("agent: caching config failed", "err", err)
+	if err != nil {
+		return err
 	}
-	return a.applyConfig(ctx, rc, true)
+	if a.lastReverted != nil && (rv == nil || a.lastReverted.Generation > rv.Generation) {
+		rv = a.lastReverted
+	}
+	if rv != nil {
+		a.reportReverted(ctx, rv)
+		if rc.Generation <= rv.Generation {
+			slog.Info("agent: generation was reverted, waiting for a newer one", "generation", rc.Generation)
+			return nil
+		}
+	}
+	return a.applyConfig(ctx, rc, raw)
 }
 
-func (a *Agent) applyConfig(ctx context.Context, rc *RenderedConfig, report bool) error {
+// applyConfig applies rc. A fetched config (raw != nil) is applied as a pending
+// try, verified by reaching the API through the new ruleset and reverted if that
+// fails; only then is it cached. The cached config is the last verified-good one,
+// so it is applied plainly: there is nothing to verify it against.
+func (a *Agent) applyConfig(ctx context.Context, rc *RenderedConfig, raw []byte) error {
 	resolver := a.Resolver
 	if resolver == nil {
 		resolver = NewResolver(rc.Resolver)
@@ -82,46 +111,219 @@ func (a *Agent) applyConfig(ctx context.Context, rc *RenderedConfig, report bool
 	if err != nil {
 		return fmt.Errorf("translate: %w", err)
 	}
-	if err := a.Applier.Apply(ctx, cfg); err != nil {
+
-		return fmt.Errorf("apply: %w", err)
+	unlock, err := tryapply.Acquire()
+	if errors.Is(err, tryapply.ErrPending) {
+		slog.Warn("agent: a 'tomswall try' is pending, skipping cycle")
+		return nil
+	}
+	if err != nil {
+		return err
+	}
+	defer unlock()
+
+	revert, keep, err := a.Applier.Apply(ctx, cfg, raw != nil)
+	if err != nil {
+		err = fmt.Errorf("apply: %w", err)
+		if raw != nil && revert != nil {
+			return a.revertGeneration(ctx, rc.Generation, StatusFailed, err, revert)
+		}
+		if revert != nil {
+			if rerr := revert(); rerr != nil {
+				err = fmt.Errorf("%w; restore: %v; revert timer pending", err, rerr)
+			}
+		}
+		if raw != nil {
+			if rerr := a.Client.ReportStatus(ctx, Status{Status: StatusFailed, Generation: rc.Generation, Error: err.Error()}); rerr != nil {
+				slog.Warn("agent: reporting status failed", "err", rerr)
+			}
+		}
+		return err
+	}
+	if raw == nil {
+		slog.Info("agent: applied cached config", "generation", rc.Generation, "rules", len(cfg.Rules))
+		return nil
+	}
+
+	if err := a.confirm(ctx, rc.Generation); err != nil {
+		if keep != nil && ctx.Err() == nil {
+			return a.revertGeneration(ctx, rc.Generation, StatusReverted, err, revert)
+		}
+		// Shutdown is not a verdict on the generation: keep it.
+		if keep != nil {
+			if kerr := keep(); kerr != nil {
+				slog.Warn("agent: dropping snapshot failed", "err", kerr)
+			}
+		}
+		return err
+	}
+	if keep != nil {
+		if err := keep(); err != nil {
+			return fmt.Errorf("dropping snapshot: %w", err)
+		}
 	}
 	slog.Info("agent: applied config", "generation", rc.Generation, "rules", len(cfg.Rules))
 
-	if report {
+	if err := a.Cache.Write(raw); err != nil {
-		if err := a.Client.ReportStatus(ctx, rc.Generation); err != nil {
+		slog.Warn("agent: caching config failed", "err", err)
-			slog.Warn("agent: reporting status failed", "err", err)
+	}
-		}
+	a.lastReverted = nil
-		// Report the FIB so the control plane can scope router enforcement.
+	if err := os.Remove(a.revertedPath()); err != nil && !os.IsNotExist(err) {
-		if fib := CollectFIB(ctx); len(fib) > 0 {
+		slog.Warn("agent: clearing reverted generation failed", "err", err)
-			if err := a.Client.ReportRoutes(ctx, fib); err != nil {
+	}
-				slog.Warn("agent: reporting routes failed", "err", err)
+	// Report the FIB so the control plane can scope router enforcement.
-			}
+	if fib := CollectFIB(ctx); len(fib) > 0 {
+		if err := a.Client.ReportRoutes(ctx, fib); err != nil {
+			slog.Warn("agent: reporting routes failed", "err", err)
 		}
 	}
 	return nil
 }
 
-// EngineApplier applies via the real nftables differential engine.
+// revertGeneration marks generation as reverted before restoring, so a failed
-type EngineApplier struct{}
+// restore can never lead to re-applying it, then reports status. It is also kept
+// in memory in case persisting fails. A failed
+// restore leaves the snapshot and timer armed and is reported as failed.
+func (a *Agent) revertGeneration(ctx context.Context, generation int64, status string, cause error, revert func() error) error {
+	rv := &reverted{Generation: generation, Status: status, Error: cause.Error()}
+	a.lastReverted = rv
+	if err := a.writeReverted(rv); err != nil {
+		slog.Error("agent: persisting reverted generation failed", "err", err)
+	}
+	suffix := ""
+	if rerr := revert(); rerr != nil {
+		suffix = fmt.Sprintf("; restore: %v; revert timer pending", rerr)
+		rv.Status = StatusFailed
+		rv.Error += suffix
+		if werr := a.writeReverted(rv); werr != nil {
+			slog.Error("agent: persisting reverted generation failed", "err", werr)
+		}
+	}
+	a.reportReverted(ctx, rv)
+	return fmt.Errorf("generation %d %s: %w%s", generation, rv.Status, cause, suffix)
+}
 
-// Apply computes and applies the differential change set for cfg. It refuses
+var (
-// while a 'tomswall try' awaits confirmation.
+	verifyAttempts = 3
-func (EngineApplier) Apply(_ context.Context, cfg *config.Config) error {
+	verifyDelay    = 2 * time.Second
-	unlock, err := tryapply.Acquire()
+	verifyTimeout  = 5 * time.Second
+)
+
+// errUnreachable means the API could not be reached through the new ruleset.
+var errUnreachable = errors.New("control plane unreachable after apply")
+
+// confirm reports generation as applied over a fresh connection, which proves
+// the API is reachable through the new ruleset. Any HTTP response counts as
+// reachable; only repeated transport failures return errUnreachable.
+func (a *Agent) confirm(ctx context.Context, generation int64) error {
+	var err error
+	for i := 0; i < verifyAttempts; i++ {
+		if i > 0 {
+			select {
+			case <-ctx.Done():
+				return ctx.Err()
+			case <-time.After(verifyDelay):
+			}
+		}
+		actx, cancel := context.WithTimeout(ctx, verifyTimeout)
+		err = a.Client.ReportStatus(actx, Status{Status: StatusApplied, Generation: generation})
+		cancel()
+		if ctx.Err() != nil {
+			return ctx.Err()
+		}
+		var uerr *url.Error
+		if !errors.As(err, &uerr) {
+			if err != nil {
+				slog.Warn("agent: reporting status failed", "err", err)
+			}
+			return nil
+		}
+	}
+	return fmt.Errorf("%w: %v", errUnreachable, err)
+}
+
+// reverted is a generation rolled back after a failed apply or for severing the
+// API; persisted so it is not re-applied until a newer generation arrives.
+type reverted struct {
+	Generation int64  `json:"generation"`
+	Status     string `json:"status,omitempty"`
+	Error      string `json:"error,omitempty"`
+	Reported   bool   `json:"reported"`
+}
+
+func (a *Agent) revertedPath() string {
+	return filepath.Join(filepath.Dir(a.Cache.Path), "reverted.json")
+}
+
+func (a *Agent) readReverted() (*reverted, error) {
+	b, err := os.ReadFile(a.revertedPath())
+	if os.IsNotExist(err) {
+		return nil, nil
+	}
+	if err != nil {
+		return nil, err
+	}
+	var rv reverted
+	if err := json.Unmarshal(b, &rv); err != nil {
+		return nil, fmt.Errorf("parsing %s: %w", a.revertedPath(), err)
+	}
+	return &rv, nil
+}
+
+func (a *Agent) writeReverted(rv *reverted) error {
+	b, err := json.Marshal(rv)
 	if err != nil {
 		return err
 	}
-	defer unlock()
+	return tryapply.WriteFile(a.revertedPath(), b)
+}
+
+// reportReverted reports rv until the control plane accepts it.
+func (a *Agent) reportReverted(ctx context.Context, rv *reverted) {
+	if rv.Reported {
+		return
+	}
+	status := rv.Status
+	if status == "" {
+		status = StatusReverted
+	}
+	if err := a.Client.ReportStatus(ctx, Status{Status: status, Generation: rv.Generation, Error: rv.Error}); err != nil {
+		slog.Warn("agent: reporting reverted generation failed, retrying next cycle", "generation", rv.Generation, "err", err)
+		return
+	}
+	rv.Reported = true
+	if err := a.writeReverted(rv); err != nil {
+		slog.Warn("agent: persisting reverted generation failed", "err", err)
+	}
+}
+
+// EngineApplier applies via the real nftables differential engine.
+type EngineApplier struct{}
+
+// Apply computes and applies the differential change set for cfg, with safe
+// under a pending try as 'tomswall try' does. The caller holds the try lock.
+func (EngineApplier) Apply(_ context.Context, cfg *config.Config, safe bool) (revert, keep func() error, err error) {
 	engine, err := nftables.NewEngine(cfg)
 	if err != nil {
-		return fmt.Errorf("initializing nftables: %w", err)
+		return nil, nil, fmt.Errorf("initializing nftables: %w", err)
 	}
 	changes, err := engine.Plan()
 	if err != nil {
-		return fmt.Errorf("computing changes: %w", err)
+		return nil, nil, fmt.Errorf("computing changes: %w", err)
 	}
 	if changes.Empty() {
-		return nil
+		return nil, nil, nil
 	}
-	return engine.Apply(changes)
+	if !safe {
+		return nil, nil, engine.Apply(changes)
+	}
+	snap, err := engine.Snapshot()
+	if err != nil {
+		return nil, nil, fmt.Errorf("snapshotting ruleset: %w", err)
+	}
+	// PID 0: 'tomswall confirm' must not signal the agent.
+	if _, err := tryapply.Arm(snap, 0, revertDelay); err != nil {
+		return nil, nil, err
+	}
+	return tryapply.Abort, tryapply.Discard, engine.Apply(changes)
 }

internal/agent/agent_test.go

@@ -132,10 +132,10 @@ type fakeApplier struct {
 	lastGen int
 }
 
-func (f *fakeApplier) Apply(_ context.Context, cfg *config.Config) error {
+func (f *fakeApplier) Apply(_ context.Context, cfg *config.Config, _ bool) (func() error, func() error, error) {
 	atomic.AddInt32(&f.count, 1)
 	f.lastGen = len(cfg.Rules)
-	return nil
+	return nil, nil, nil
 }
 
 const renderedYAML = `generation: 7

internal/agent/cache.go

@@ -2,7 +2,8 @@ package agent
 
 import (
 	"os"
-	"path/filepath"
+
+	"git.unkin.net/unkin/tomswall/internal/tryapply"
 )
 
 // Cache persists the last known-good rendered config to disk so the agent can
@@ -11,16 +12,9 @@ type Cache struct {
 	Path string
 }
 
-// Write atomically stores the raw config bytes.
+// Write durably stores the raw config bytes.
 func (c Cache) Write(raw []byte) error {
-	if err := os.MkdirAll(filepath.Dir(c.Path), 0o755); err != nil {
+	return tryapply.WriteFile(c.Path, raw)
-		return err
-	}
-	tmp := c.Path + ".tmp"
-	if err := os.WriteFile(tmp, raw, 0o600); err != nil {
-		return err
-	}
-	return os.Rename(tmp, c.Path)
 }
 
 // Read returns the cached config, or (nil, nil) when no cache exists yet.

internal/agent/client.go

@@ -26,7 +26,9 @@ func NewClient(baseURL, device, token string) *Client {
 		BaseURL: baseURL,
 		Device:  device,
 		Token:   token,
-		HTTP:    &http.Client{Timeout: 30 * time.Second},
+		// No keep-alives: every request, the post-apply check included, opens a
+		// fresh connection that must pass the current ruleset.
+		HTTP: &http.Client{Timeout: 30 * time.Second, Transport: noKeepAlive()},
 	}
 }
 
@@ -94,10 +96,24 @@ func (c *Client) ReportRoutes(ctx context.Context, prefixes []string) error {
 	return nil
 }
 
-// ReportStatus tells the control plane which generation this device has applied.
+// Status values reported to POST /api/v1/devices/{name}/status.
-func (c *Client) ReportStatus(ctx context.Context, generation int64) error {
+const (
+	StatusApplied  = "applied"
+	StatusReverted = "reverted"
+	StatusFailed   = "failed"
+)
+
+// Status is the outcome of applying one generation.
+type Status struct {
+	Status     string `json:"status"`
+	Generation int64  `json:"generation"`
+	Error      string `json:"error,omitempty"`
+}
+
+// ReportStatus tells the control plane the outcome of applying a generation.
+func (c *Client) ReportStatus(ctx context.Context, st Status) error {
 	url := fmt.Sprintf("%s/api/v1/devices/%s/status", c.BaseURL, c.Device)
-	payload, _ := json.Marshal(map[string]int64{"generation": generation})
+	payload, _ := json.Marshal(st)
 	req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(payload))
 	if err != nil {
 		return err
@@ -116,3 +132,9 @@ func (c *Client) ReportStatus(ctx context.Context, generation int64) error {
 	}
 	return nil
 }
+
+func noKeepAlive() http.RoundTripper {
+	t := http.DefaultTransport.(*http.Transport).Clone()
+	t.DisableKeepAlives = true
+	return t
+}

internal/agent/safeapply_test.go

@@ -0,0 +1,396 @@
+package agent
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"git.unkin.net/unkin/tomswall/internal/config"
+	"git.unkin.net/unkin/tomswall/internal/nftables"
+	"git.unkin.net/unkin/tomswall/internal/tryapply"
+)
+
+func TestMain(m *testing.M) {
+	dir, err := os.MkdirTemp("", "tomswall-agent-test")
+	if err != nil {
+		panic(err)
+	}
+	tryapply.Dir = dir
+	tryapply.Run = func(name string, args ...string) error {
+		timerCmds = append(timerCmds, name)
+		return nil
+	}
+	verifyDelay = time.Millisecond
+	verifyTimeout = time.Second
+	code := m.Run()
+	os.RemoveAll(dir)
+	os.Exit(code)
+}
+
+// fakeAPI serves a config generation and records status reports; while cut it
+// drops connections to the status endpoint, as a severing ruleset would.
+type fakeAPI struct {
+	*httptest.Server
+	gen     atomic.Int64
+	cut     atomic.Bool
+	code    atomic.Int32
+	mu      sync.Mutex
+	reports []Status
+}
+
+func newFakeAPI(t *testing.T, gen int64) *fakeAPI {
+	f := &fakeAPI{}
+	f.gen.Store(gen)
+	f.code.Store(http.StatusNoContent)
+	f.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/devices/fw-a/config":
+			_, _ = w.Write([]byte(strings.Replace(renderedYAML, "generation: 7", "generation: "+itoa(f.gen.Load()), 1)))
+		case "/api/v1/devices/fw-a/status":
+			if f.cut.Load() {
+				conn, _, _ := w.(http.Hijacker).Hijack()
+				conn.Close()
+				return
+			}
+			var st Status
+			_ = json.NewDecoder(r.Body).Decode(&st)
+			f.mu.Lock()
+			f.reports = append(f.reports, st)
+			f.mu.Unlock()
+			w.WriteHeader(int(f.code.Load()))
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	t.Cleanup(f.Close)
+	return f
+}
+
+// timerCmds records the systemd commands tryapply runs.
+var timerCmds []string
+
+func itoa(n int64) string { b, _ := json.Marshal(n); return string(b) }
+
+func (f *fakeAPI) last() Status {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	if len(f.reports) == 0 {
+		return Status{}
+	}
+	return f.reports[len(f.reports)-1]
+}
+
+// fakeEngine always changes the ruleset, when safe under a real tryapply pending
+// try; onApply simulates its effect and restoreErr fails the restore.
+type fakeEngine struct {
+	applies, plain, restores int
+	err, restoreErr          error
+	onApply                  func()
+	onRestore                func()
+}
+
+func (f *fakeEngine) Apply(_ context.Context, _ *config.Config, safe bool) (func() error, func() error, error) {
+	if !safe {
+		f.plain++
+		return nil, nil, f.err
+	}
+	if _, err := tryapply.Arm(&nftables.Snapshot{Table: "tomswall"}, 0, time.Minute); err != nil {
+		return nil, nil, err
+	}
+	tryapply.Restore = func(*nftables.Snapshot) error {
+		f.restores++
+		if f.onRestore != nil {
+			f.onRestore()
+		}
+		return f.restoreErr
+	}
+	f.applies++
+	if f.onApply != nil {
+		f.onApply()
+	}
+	return tryapply.Abort, tryapply.Discard, f.err
+}
+
+// pending reports whether a snapshot is still armed and its timer not stopped since.
+func pending(t *testing.T) bool {
+	t.Helper()
+	_, err := os.Stat(filepath.Join(tryapply.Dir, "try-snapshot.json"))
+	armed := len(timerCmds) > 0 && timerCmds[len(timerCmds)-1] == "systemd-run"
+	if (err == nil) != armed {
+		t.Fatalf("snapshot present=%v but timer armed=%v", err == nil, armed)
+	}
+	return armed
+}
+
+func newAgent(t *testing.T, api *fakeAPI, eng *fakeEngine) *Agent {
+	return &Agent{
+		Client:  NewClient(api.URL, "fw-a", "tok"),
+		Cache:   Cache{Path: filepath.Join(t.TempDir(), "rendered.yaml")},
+		Applier: eng,
+	}
+}
+
+func cachedGen(t *testing.T, a *Agent) int64 {
+	rc, err := a.Cache.Read()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if rc == nil {
+		return 0
+	}
+	return rc.Generation
+}
+
+func TestSafeApplyReachableApplies(t *testing.T) {
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{}
+	a := newAgent(t, api, eng)
+	if err := a.RunOnce(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	if eng.restores != 0 || api.last() != (Status{Status: StatusApplied, Generation: 7}) || cachedGen(t, a) != 7 || pending(t) {
+		t.Fatalf("restores=%d last=%+v cache=%d", eng.restores, api.last(), cachedGen(t, a))
+	}
+}
+
+func TestSafeApplyUnreachableRevertsAndReports(t *testing.T) {
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{onApply: func() { api.cut.Store(true) }, onRestore: func() { api.cut.Store(false) }}
+	a := newAgent(t, api, eng)
+	if err := a.RunOnce(context.Background()); !errors.Is(err, errUnreachable) {
+		t.Fatalf("want errUnreachable, got %v", err)
+	}
+	if eng.restores != 1 || cachedGen(t, a) != 0 || pending(t) {
+		t.Fatalf("restores=%d cache=%d", eng.restores, cachedGen(t, a))
+	}
+	if st := api.last(); st.Status != StatusReverted || st.Generation != 7 || st.Error == "" {
+		t.Fatalf("last report %+v", st)
+	}
+	rv, _ := a.readReverted()
+	if rv == nil || rv.Generation != 7 || !rv.Reported {
+		t.Fatalf("persisted %+v", rv)
+	}
+}
+
+func TestSafeApplyRevertReportedOnceReachable(t *testing.T) {
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{onApply: func() { api.cut.Store(true) }}
+	a := newAgent(t, api, eng)
+	_ = a.RunOnce(context.Background())
+	if eng.restores != 1 || api.last().Status != "" {
+		t.Fatalf("restores=%d last=%+v", eng.restores, api.last())
+	}
+	api.cut.Store(false)
+	if err := a.RunOnce(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	if eng.applies != 1 || api.last() != (Status{Status: StatusReverted, Generation: 7, Error: api.last().Error}) {
+		t.Fatalf("applies=%d last=%+v", eng.applies, api.last())
+	}
+}
+
+func TestSafeApplyShutdownDoesNotRevert(t *testing.T) {
+	api := newFakeAPI(t, 7)
+	ctx, cancel := context.WithCancel(context.Background())
+	eng := &fakeEngine{onApply: func() { api.cut.Store(true); cancel() }}
+	a := newAgent(t, api, eng)
+	if err := a.RunOnce(ctx); !errors.Is(err, context.Canceled) {
+		t.Fatalf("want context.Canceled, got %v", err)
+	}
+	if rv, _ := a.readReverted(); eng.restores != 0 || rv != nil || cachedGen(t, a) != 0 {
+		t.Fatalf("restores=%d reverted=%+v cache=%d", eng.restores, rv, cachedGen(t, a))
+	}
+}
+
+func TestSafeApplyHTTPErrorDoesNotRevert(t *testing.T) {
+	api := newFakeAPI(t, 7)
+	api.code.Store(http.StatusInternalServerError)
+	eng := &fakeEngine{}
+	a := newAgent(t, api, eng)
+	if err := a.RunOnce(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	if eng.restores != 0 || cachedGen(t, a) != 7 {
+		t.Fatalf("restores=%d cache=%d", eng.restores, cachedGen(t, a))
+	}
+}
+
+func TestSafeApplyApplyErrorRestoresAndReportsFailed(t *testing.T) {
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{err: errors.New("netlink: boom")}
+	a := newAgent(t, api, eng)
+	if err := a.RunOnce(context.Background()); err == nil {
+		t.Fatal("want error")
+	}
+	if st := api.last(); eng.restores != 1 || st.Status != StatusFailed || !strings.Contains(st.Error, "boom") || pending(t) {
+		t.Fatalf("restores=%d last=%+v", eng.restores, st)
+	}
+	if rv, _ := a.readReverted(); rv == nil || rv.Generation != 7 || !rv.Reported {
+		t.Fatalf("persisted %+v", rv)
+	}
+}
+
+func TestSafeApplyApplyErrorRestoreFailsKeepsTimer(t *testing.T) {
+	t.Cleanup(func() { _ = tryapply.Discard() })
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{err: errors.New("netlink: boom"), restoreErr: errors.New("netlink: stuck")}
+	a := newAgent(t, api, eng)
+	if err := a.RunOnce(context.Background()); err == nil || !strings.Contains(err.Error(), "restore: restoring snapshot: netlink: stuck") {
+		t.Fatalf("got %v", err)
+	}
+	want := "apply: netlink: boom; restore: restoring snapshot: netlink: stuck; revert timer pending"
+	if st := api.last(); st != (Status{Status: StatusFailed, Generation: 7, Error: want}) || !pending(t) {
+		t.Fatalf("last=%+v", st)
+	}
+	if rv, _ := a.readReverted(); rv == nil || rv.Generation != 7 || rv.Status != StatusFailed {
+		t.Fatalf("persisted %+v", rv)
+	}
+	// The next cycle waits for the timer instead of re-applying.
+	if err := a.RunOnce(context.Background()); err != nil || eng.applies != 1 {
+		t.Fatalf("err=%v applies=%d", err, eng.applies)
+	}
+}
+
+func TestSafeApplyUnreachableRestoreFailsKeepsTimer(t *testing.T) {
+	t.Cleanup(func() { _ = tryapply.Discard() })
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{onApply: func() { api.cut.Store(true) }, restoreErr: errors.New("netlink: stuck")}
+	eng.onRestore = func() { api.cut.Store(false) }
+	a := newAgent(t, api, eng)
+	if err := a.RunOnce(context.Background()); !errors.Is(err, errUnreachable) || !strings.Contains(err.Error(), "revert timer pending") {
+		t.Fatalf("got %v", err)
+	}
+	st := api.last()
+	if st.Status != StatusFailed || st.Generation != 7 || !strings.HasPrefix(st.Error, errUnreachable.Error()) ||
+		!strings.HasSuffix(st.Error, "; restore: restoring snapshot: netlink: stuck; revert timer pending") || !pending(t) {
+		t.Fatalf("last=%+v", st)
+	}
+	if rv, _ := a.readReverted(); rv == nil || rv.Generation != 7 || !rv.Reported || cachedGen(t, a) != 0 {
+		t.Fatalf("persisted %+v cache=%d", rv, cachedGen(t, a))
+	}
+}
+
+func TestSafeApplyRevertedGenerationSkippedAfterRestart(t *testing.T) {
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{}
+	a := newAgent(t, api, eng)
+	if err := a.writeReverted(&reverted{Generation: 7, Reported: true}); err != nil {
+		t.Fatal(err)
+	}
+	if err := a.RunOnce(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	if eng.applies != 0 {
+		t.Fatalf("reverted generation re-applied")
+	}
+
+	api.gen.Store(8)
+	if err := a.RunOnce(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	if rv, _ := a.readReverted(); eng.applies != 1 || api.last().Generation != 8 || rv != nil {
+		t.Fatalf("applies=%d last=%+v reverted=%+v", eng.applies, api.last(), rv)
+	}
+}
+
+func TestSafeApplySkipsWhileTryPending(t *testing.T) {
+	marker := filepath.Join(tryapply.Dir, "try-snapshot.json")
+	if err := os.WriteFile(marker, []byte("{}"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(marker)
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{}
+	a := newAgent(t, api, eng)
+	if err := a.RunOnce(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	if eng.applies != 0 || api.last().Status != "" {
+		t.Fatalf("applies=%d last=%+v", eng.applies, api.last())
+	}
+}
+
+// failArm makes arming the revert timer fail, as without systemd.
+func failArm(t *testing.T) {
+	orig := tryapply.Run
+	tryapply.Run = func(name string, args ...string) error {
+		timerCmds = append(timerCmds, name)
+		if name == "systemd-run" {
+			return errors.New("no systemd")
+		}
+		return nil
+	}
+	t.Cleanup(func() { tryapply.Run = orig })
+}
+
+func TestSafeApplyArmFailureReportsFailedAndRetries(t *testing.T) {
+	failArm(t)
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{}
+	a := newAgent(t, api, eng)
+	if err := a.RunOnce(context.Background()); err == nil || !strings.Contains(err.Error(), "no systemd") {
+		t.Fatalf("got %v", err)
+	}
+	if st := api.last(); eng.applies != 0 || st.Status != StatusFailed || st.Generation != 7 || pending(t) || cachedGen(t, a) != 0 {
+		t.Fatalf("applies=%d last=%+v", eng.applies, st)
+	}
+	if rv, _ := a.readReverted(); rv != nil || a.lastReverted != nil {
+		t.Fatalf("arm failure marked generation reverted: %+v", rv)
+	}
+	tryapply.Run = func(name string, args ...string) error {
+		timerCmds = append(timerCmds, name)
+		return nil
+	}
+	if err := a.RunOnce(context.Background()); err != nil || eng.applies != 1 || cachedGen(t, a) != 7 {
+		t.Fatalf("retry err=%v applies=%d", err, eng.applies)
+	}
+}
+
+func TestCachedConfigAppliesWithoutArm(t *testing.T) {
+	failArm(t)
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{}
+	a := newAgent(t, api, eng)
+	if err := a.Cache.Write([]byte(renderedYAML)); err != nil {
+		t.Fatal(err)
+	}
+	api.Close()
+	if err := a.RunOnce(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	if eng.plain != 1 || eng.applies != 0 || pending(t) {
+		t.Fatalf("plain=%d safe=%d", eng.plain, eng.applies)
+	}
+}
+
+func TestSafeApplyRevertedKeptInMemoryWhenPersistFails(t *testing.T) {
+	api := newFakeAPI(t, 7)
+	eng := &fakeEngine{onRestore: func() { api.cut.Store(false) }}
+	a := newAgent(t, api, eng)
+	// A non-empty directory in its place makes persisting reverted.json fail.
+	eng.onApply = func() {
+		api.cut.Store(true)
+		_ = os.MkdirAll(filepath.Join(a.revertedPath(), "x"), 0o755)
+	}
+	if err := a.RunOnce(context.Background()); !errors.Is(err, errUnreachable) {
+		t.Fatalf("want errUnreachable, got %v", err)
+	}
+	if err := os.RemoveAll(a.revertedPath()); err != nil {
+		t.Fatal(err)
+	}
+	if eng.restores != 1 || api.last().Status != StatusReverted || pending(t) {
+		t.Fatalf("restores=%d last=%+v", eng.restores, api.last())
+	}
+	if err := a.RunOnce(context.Background()); err != nil || eng.applies != 1 {
+		t.Fatalf("reverted generation re-applied: err=%v applies=%d", err, eng.applies)
+	}
+}

internal/config/config.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 
 	"gopkg.in/yaml.v3"
@@ -55,10 +56,17 @@ type Settings struct {
 	AddressFamily AddressFamily `yaml:"address_family,omitempty"`
 	IPForwarding  bool          `yaml:"ip_forwarding"`
 	LogLevel      string        `yaml:"log_level"`
-	TableName     string        `yaml:"table_name"`
+	// LogLimit rate-limits every log site, shorewall LOGLIMIT syntax rate/unit[:burst]; unset logs every hit.
+	LogLimit  string `yaml:"log_limit,omitempty"`
+	TableName string `yaml:"table_name"`
 
 	// When true, auto-generate CONTINUE policies for sub-zones to their parent zones.
 	ImplicitContinue bool `yaml:"implicit_continue,omitempty"`
+
+	// Verdict for ct state invalid/untracked packets; continue passes them to the rules.
+	// Unset: invalid drops, untracked continues.
+	InvalidDisposition   PolicyAction `yaml:"invalid_disposition,omitempty"`
+	UntrackedDisposition PolicyAction `yaml:"untracked_disposition,omitempty"`
 }
 
 // Load reads a config file in YAML or JSON format (detected by extension).
@@ -107,6 +115,8 @@ func (c *Config) applyDefaults() {
 	}
 }
 
+var logLimitRe = regexp.MustCompile(`^[1-9][0-9]*/(sec|second|min|minute|hour|day)(:[1-9][0-9]*)?$`)
+
 var validAddressFamilies = map[AddressFamily]bool{
 	FamilyINET: true, FamilyIP: true, FamilyIP6: true,
 }
@@ -115,6 +125,19 @@ func (c *Config) validateSettings() error {
 	if !validAddressFamilies[c.Settings.AddressFamily] {
 		return fmt.Errorf("unknown address_family %q (use inet, ip, or ip6)", c.Settings.AddressFamily)
 	}
+	if l := c.Settings.LogLimit; l != "" && !logLimitRe.MatchString(l) {
+		return fmt.Errorf("invalid log_limit %q (use rate/{sec|min|hour|day}[:burst]; per-source s:/d: is not supported)", l)
+	}
+	for name, d := range map[string]PolicyAction{
+		"invalid_disposition":   c.Settings.InvalidDisposition,
+		"untracked_disposition": c.Settings.UntrackedDisposition,
+	} {
+		switch d {
+		case "", PolicyAccept, PolicyDrop, PolicyReject, PolicyContinue:
+		default:
+			return fmt.Errorf("unknown %s %q (use accept, drop, reject, or continue)", name, d)
+		}
+	}
 	return nil
 }
 

internal/config/config_test.go

@@ -1052,3 +1052,32 @@ func TestSplitZoneList(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateDispositions(t *testing.T) {
+	for _, tc := range []struct {
+		invalid, untracked PolicyAction
+		wantErr            string
+	}{
+		{"", "", ""},
+		{PolicyContinue, PolicyDrop, ""},
+		{"bogus", "", `unknown invalid_disposition "bogus"`},
+		{PolicyAccept, "log", `unknown untracked_disposition "log"`},
+	} {
+		c := baseConfig()
+		c.Settings.InvalidDisposition = tc.invalid
+		c.Settings.UntrackedDisposition = tc.untracked
+		checkErr(t, c.Validate(), tc.wantErr)
+	}
+}
+
+func TestValidateLogLimit(t *testing.T) {
+	for v, ok := range map[string]bool{
+		"": true, "1/sec": true, "1/sec:10": true, "30/minute:5": true, "2/hour": true, "1/day:1": true,
+		"s:1/sec:10": false, "d:1/sec": false, "1": false, "1/week": false, "0/sec": false, "1/sec:": false,
+	} {
+		c := &Config{Settings: Settings{AddressFamily: FamilyINET, LogLimit: v}}
+		if err := c.validateSettings(); (err == nil) != ok {
+			t.Errorf("log_limit %q: err = %v, want ok=%v", v, err, ok)
+		}
+	}
+}

internal/nftables/cleanup.go

@@ -28,7 +28,7 @@ func (e *Engine) FindForeignRules() ([]ForeignRule, error) {
 
 	var ourTable *nftables.Table
 	for _, t := range tables {
-		if t.Name == e.cfg.Settings.TableName && t.Family == nftables.TableFamilyINet {
+		if t.Name == e.cfg.Settings.TableName && t.Family == e.family() {
 			ourTable = t
 			break
 		}
@@ -52,7 +52,7 @@ func (e *Engine) FindForeignRules() ([]ForeignRule, error) {
 
 	var foreign []ForeignRule
 
-	chains, err := e.conn.ListChainsOfTableFamily(nftables.TableFamilyINet)
+	chains, err := e.conn.ListChainsOfTableFamily(e.family())
 	if err != nil {
 		return nil, fmt.Errorf("listing chains: %w", err)
 	}

internal/nftables/compiler.go

@@ -65,26 +65,68 @@ func (c *Compiler) Compile() (*FirewallState, error) {
 		return nil, fmt.Errorf("static-nat: %w", err)
 	}
 	c.compileMSSClamp(state)
+	limitLogs(state, c.cfg.Settings.LogLimit)
 
 	return state, nil
 }
 
+// limitLogs puts a limit in front of every log expression. A limit stops the
+// whole rule, so like shorewall's separate LOG rule, a log followed by an action
+// splits into a limited log-only rule and the same rule without the log. A
+// LOG-action rule (nothing but rate limits after the log) keeps only the log rule.
+func limitLogs(state *FirewallState, spec string) {
+	if spec == "" {
+		return
+	}
+	for chain, rules := range state.Rules {
+		var out []ManagedRule
+		for _, r := range rules {
+			i := slices.IndexFunc(r.Exprs, func(e expr.Any) bool { _, ok := e.(*expr.Log); return ok })
+			if i < 0 {
+				out = append(out, r)
+				continue
+			}
+			logRule := r
+			logRule.Exprs = slices.Concat(r.Exprs[:i], parseRateLimit(spec), r.Exprs[i:i+1])
+			out = append(out, logRule)
+			if slices.ContainsFunc(r.Exprs[i+1:], func(e expr.Any) bool { _, ok := e.(*expr.Limit); return !ok }) {
+				r.Exprs = slices.Concat(r.Exprs[:i], r.Exprs[i+1:])
+				out = append(out, r)
+			}
+		}
+		state.Rules[chain] = out
+	}
+}
+
 func (c *Compiler) compileConntrackFastPath(state *FirewallState) error {
+	invalid := c.cfg.Settings.InvalidDisposition
+	if invalid == "" {
+		invalid = config.PolicyDrop
+	}
 	for _, chain := range []string{"input", "forward", "output"} {
-		state.Rules[chain] = append(state.Rules[chain],
+		state.Rules[chain] = append(state.Rules[chain], ManagedRule{
-			ManagedRule{
+			Chain: chain,
+			Exprs: append(matchCtState(ctStateEstablished|ctStateRelated),
+				&expr.Verdict{Kind: expr.VerdictAccept}),
+			Tag: "ct:fastpath:" + chain,
+		})
+		for _, d := range []struct {
+			name   string
+			state  uint32
+			action config.PolicyAction
+		}{
+			{"invalid", ctStateInvalid, invalid},
+			{"untracked", ctStateUntracked, c.cfg.Settings.UntrackedDisposition},
+		} {
+			if d.action == "" || d.action == config.PolicyContinue {
+				continue
+			}
+			state.Rules[chain] = append(state.Rules[chain], ManagedRule{
 				Chain: chain,
-				Exprs: append(matchCtState(ctStateEstablished|ctStateRelated),
+				Exprs: append(matchCtState(d.state), policyVerdict(d.action, c.cfg.Settings.AddressFamily)...),
-					&expr.Verdict{Kind: expr.VerdictAccept}),
+				Tag:   "ct:" + d.name + ":" + chain,
-				Tag: "ct:fastpath:" + chain,
+			})
-			},
+		}
-			ManagedRule{
-				Chain: chain,
-				Exprs: append(matchCtState(ctStateInvalid),
-					&expr.Verdict{Kind: expr.VerdictDrop}),
-				Tag: "ct:invalid:" + chain,
-			},
-		)
 	}
 	return nil
 }
@@ -126,40 +168,23 @@ func (c *Compiler) compileDHCP(state *FirewallState) {
 			continue
 		}
 		name := iface.PhysicalName()
-		// Allow DHCPv4 client traffic (bootpc:68 → bootps:67)
+		dhcp := func(chain, dir string, ifaceMatch []expr.Any) {
-		state.Rules["input"] = append(state.Rules["input"], ManagedRule{
+			state.Rules[chain] = append(state.Rules[chain], ManagedRule{
-			Chain: "input",
+				Chain: chain,
-			Exprs: append(append(append(
+				Exprs: append(append(append(append(ifaceMatch,
-				matchIfaceName(true, name),
+					matchNFProto(unix.NFPROTO_IPV4)...),
-				matchProtoNum(unix.IPPROTO_UDP)...),
+					matchProtoNum(unix.IPPROTO_UDP)...),
-				matchSPort(68)...),
+					matchDPortRange(67, 68)...),
-				matchDPort(67)...,
+					&expr.Verdict{Kind: expr.VerdictAccept}),
-			),
+				Tag: fmt.Sprintf("dhcp:%s:%s", dir, iface.Interface),
-			Tag: fmt.Sprintf("dhcp:in:%s", iface.Interface),
+			})
-		})
+		}
-		// Allow DHCPv4 server → client replies
+		// shorewall: udp dport 67:68 both ways between fw and iface, forwarded back out a bridge
-		state.Rules["input"] = append(state.Rules["input"], ManagedRule{
+		dhcp("input", "in", matchIfaceName(true, name))
-			Chain: "input",
+		dhcp("output", "out", matchIfaceName(false, name))
-			Exprs: append(append(append(append(
+		if iface.Options.Bridge {
-				matchIfaceName(true, name),
+			dhcp("forward", "fwd", append(matchIfaceName(true, name), matchIfaceName(false, name)...))
-				matchProtoNum(unix.IPPROTO_UDP)...),
+		}
-				matchSPort(67)...),
-				matchDPort(68)...),
-				&expr.Verdict{Kind: expr.VerdictAccept},
-			),
-			Tag: fmt.Sprintf("dhcp:reply:%s", iface.Interface),
-		})
-		state.Rules["output"] = append(state.Rules["output"], ManagedRule{
-			Chain: "output",
-			Exprs: append(append(append(append(
-				matchIfaceName(false, name),
-				matchProtoNum(unix.IPPROTO_UDP)...),
-				matchSPort(68)...),
-				matchDPort(67)...),
-				&expr.Verdict{Kind: expr.VerdictAccept},
-			),
-			Tag: fmt.Sprintf("dhcp:out:%s", iface.Interface),
-		})
 	}
 }
 
@@ -381,7 +406,7 @@ func (c *Compiler) compileRules(state *FirewallState) error {
 			if err != nil {
 				return fmt.Errorf("rule[%d]: %w", i, err)
 			}
-			if len(matches)*c.specCount(rule.Source, rule.Dest, rule.OrigDest, rule.Action) > 1 {
+			if len(matches)*c.specCount(rule.Source, rule.Dest, rule.OrigDest, fwZone, rule.Action) > 1 {
 				return fmt.Errorf("rule[%d]: ratelimit/connlimit cannot be combined with proto, port, zone or address lists (each expanded rule would get its own limiter)", i)
 			}
 		}
@@ -418,24 +443,24 @@ func (c *Compiler) applyChainExtras(state *FirewallState, chain, tag string, rul
 			break
 		}
 
-		var extra []expr.Any
+		var match, extra []expr.Any
 		var replaceVerdict []expr.Any
 
 		if rule.User != "" {
-			extra = append(extra, matchUID(rule.User)...)
+			match = append(match, matchUID(rule.User)...)
 		}
 		if rule.Mark != "" {
-			extra = append(extra, matchMark(rule.Mark)...)
+			match = append(match, matchMark(rule.Mark)...)
+		}
+		if rule.ConnLimit != "" {
+			match = append(match, matchConnLimit(rule.ConnLimit)...)
+		}
+		if rule.Time != nil {
+			match = append(match, matchTime(rule.Time)...)
 		}
 		if rule.RateLimit != "" {
 			extra = append(extra, parseRateLimit(rule.RateLimit)...)
 		}
-		if rule.ConnLimit != "" {
-			extra = append(extra, matchConnLimit(rule.ConnLimit)...)
-		}
-		if rule.Time != nil {
-			extra = append(extra, matchTime(rule.Time)...)
-		}
 		if rule.SetMark != "" {
 			extra = append(extra, setMarkExprs(rule.SetMark)...)
 		}
@@ -443,21 +468,27 @@ func (c *Compiler) applyChainExtras(state *FirewallState, chain, tag string, rul
 			replaceVerdict = []expr.Any{&expr.Queue{Num: uint16(rule.NFQueue), Total: 1}}
 		}
 
-		if len(extra) > 0 || len(replaceVerdict) > 0 {
+		if len(match) > 0 || len(extra) > 0 || len(replaceVerdict) > 0 {
-			existingExprs := rules[idx].Exprs
+			var pre, post, verdict []expr.Any
-			var verdict []expr.Any
+			for _, e := range rules[idx].Exprs {
-			var nonVerdict []expr.Any
+				switch e.(type) {
-			for _, e := range existingExprs {
+				case *expr.Verdict:
-				if _, ok := e.(*expr.Verdict); ok {
 					verdict = append(verdict, e)
-				} else {
+				case *expr.Log:
-					nonVerdict = append(nonVerdict, e)
+					post = append(post, e)
+				default:
+					if len(post) > 0 {
+						post = append(post, e)
+					} else {
+						pre = append(pre, e)
+					}
 				}
 			}
 			if len(replaceVerdict) > 0 {
 				verdict = replaceVerdict
 			}
-			rules[idx].Exprs = append(append(nonVerdict, extra...), verdict...)
+			// matches go before the log so it only fires for packets the rule matches
+			rules[idx].Exprs = slices.Concat(pre, match, post, extra, verdict)
 		}
 	}
 	state.Rules[chain] = rules
@@ -467,26 +498,32 @@ func (c *Compiler) compileOneRule(state *FirewallState, tag, srcSpec, dstSpec, p
 	dports, sports config.PortSpec, action config.RuleAction, logLevel string,
 	dnatDest, origDest string, fwZone string, section config.RuleSection) error {
 
-	for _, src := range c.zoneSpecs(srcSpec) {
+	if action == config.RuleDNAT || action == config.RuleRedirect {
-		for _, srcAddr := range splitAddrs(src.Addr) {
+		for _, src := range c.dnatSourceSpecs(srcSpec, fwZone) {
-			for _, od := range splitAddrs(origDest) {
+			if dnatSkipsIntrazone(srcSpec, src.Zone, dstSpec) {
-				if action == config.RuleDNAT || action == config.RuleRedirect {
+				continue
-					if err := c.compileDNATRule(state, tag, src.Zone, srcAddr, od, dstSpec, proto, dports, action, logLevel); err != nil {
+			}
+			for _, srcAddr := range splitAddrs(src.Addr) {
+				if err := c.compileDNATAccept(state, tag+":accept", src.Zone, srcAddr, dstSpec, proto, dports, sports, action, fwZone, section); err != nil {
+					return err
+				}
+				for _, od := range splitAddrs(origDest) {
+					if err := c.compileDNATRule(state, tag, src.Zone, srcAddr, od, dstSpec, proto, dports, sports, action, logLevel); err != nil {
 						return err
 					}
-					continue
 				}
-				for _, dst := range c.zoneSpecs(dstSpec) {
+			}
-					// Exclusion expansion never pairs fw with itself, and pairs a zone with itself only for "all+".
+		}
-					if src.Zone == dst.Zone && (isZoneExclusion(srcSpec) || isZoneExclusion(dstSpec)) &&
+		return nil
-						(src.Zone == fwZone || !strings.Contains(srcSpec, "+!") && !strings.Contains(dstSpec, "+!")) {
+	}
-						continue
+	for _, p := range c.zonePairs(srcSpec, dstSpec, fwZone) {
-					}
+		src, dst := p[0], p[1]
-					for _, dstAddr := range splitAddrs(dst.Addr) {
+		for _, srcAddr := range splitAddrs(src.Addr) {
-						if err := c.compileZonePair(state, tag, src.Zone, srcAddr, dst.Zone, dstAddr, od, proto,
+			for _, od := range splitAddrs(origDest) {
-							dports, sports, action, logLevel, fwZone, section); err != nil {
+				for _, dstAddr := range splitAddrs(dst.Addr) {
-							return err
+					if err := c.compileZonePair(state, tag, src.Zone, srcAddr, dst.Zone, dstAddr, od, proto,
-						}
+						dports, sports, action, logLevel, fwZone, section); err != nil {
+						return err
 					}
 				}
 			}
@@ -495,19 +532,93 @@ func (c *Compiler) compileOneRule(state *FirewallState, tag, srcSpec, dstSpec, p
 	return nil
 }
 
-// specCount is how many zone/address combinations compileOneRule expands src and dst into.
+// dnatSourceSpecs hooks DNAT per source zone like shorewall: all/any expand to every zone but fw (prerouting never sees fw traffic).
-func (c *Compiler) specCount(srcSpec, dstSpec, origDest string, action config.RuleAction) int {
+func (c *Compiler) dnatSourceSpecs(spec, fwZone string) []config.ZoneSpec {
-	count := func(spec string) (n int) {
+	zone, addr := splitZoneSpec(spec)
-		for _, z := range c.zoneSpecs(spec) {
+	base, _, _ := strings.Cut(zone, "!")
-			n += len(splitAddrs(z.Addr))
+	if base = strings.TrimSuffix(base, "+"); base != "all" && base != "any" {
+		return c.zoneSpecs(spec)
+	}
+	var out []config.ZoneSpec
+	for _, z := range c.expandZoneRef(zone) {
+		if z != fwZone {
+			out = append(out, config.ZoneSpec{Zone: z, Addr: addr})
 		}
-		return n
 	}
-	n := count(srcSpec) * len(splitAddrs(origDest))
+	return out
+}
+
+// dnatSkipsIntrazone mirrors shorewall: a zone list or all/any source never pairs a zone with itself unless marked "+".
+func dnatSkipsIntrazone(srcSpec, srcZone, dstSpec string) bool {
+	zones, _, _ := strings.Cut(srcSpec, ":")
+	base, _, _ := strings.Cut(zones, "!")
+	wild := base == "all" || base == "any" || strings.Contains(base, ",")
+	dstZone, _, _ := strings.Cut(dstSpec, ":")
+	return wild && srcZone == dstZone
+}
+
+// compileDNATAccept emits the filter ACCEPT implied by DNAT/REDIRECT (shorewall's DNAT-/REDIRECT- omit it) for the translated flow.
+func (c *Compiler) compileDNATAccept(state *FirewallState, tag, srcZone, srcAddr, dstSpec, proto string,
+	dports, sports config.PortSpec, action config.RuleAction, fwZone string, section config.RuleSection) error {
+	parts := strings.SplitN(dstSpec, ":", 3)
+	if len(parts) < 2 {
+		return fmt.Errorf("DNAT dest must be zone:address or zone:address:port")
+	}
+	dstZone, dstAddr := parts[0], parts[1]
+	if action == config.RuleRedirect {
+		dstZone, dstAddr = fwZone, ""
+	}
+	if len(parts) == 3 {
+		dports = config.PortSpec{parts[2]}
+	}
+	chain := c.selectChain(srcZone, dstZone, fwZone)
+	n := len(state.Rules[chain])
+	if err := c.compileZonePair(state, tag, srcZone, srcAddr, dstZone, dstAddr, "", proto,
+		dports, sports, config.RuleAccept, "", fwZone, section); err != nil {
+		return err
+	}
+	for i := n; i < len(state.Rules[chain]); i++ {
+		e := state.Rules[chain][i].Exprs
+		last := len(e) - 1
+		state.Rules[chain][i].Exprs = append(append(e[:last:last], matchCtBits(expr.CtKeySTATUS, ctStatusDNAT)...), e[last])
+	}
+	return nil
+}
+
+// specCount is how many zone/address combinations compileOneRule expands src and dst into.
+func (c *Compiler) specCount(srcSpec, dstSpec, origDest, fwZone string, action config.RuleAction) int {
+	n := 0
 	if action == config.RuleDNAT || action == config.RuleRedirect {
-		return n
+		for _, src := range c.dnatSourceSpecs(srcSpec, fwZone) {
+			n += len(splitAddrs(src.Addr))
+		}
+		return n * len(splitAddrs(origDest))
 	}
-	return n * count(dstSpec)
+	for _, p := range c.zonePairs(srcSpec, dstSpec, fwZone) {
+		n += len(splitAddrs(p[0].Addr)) * len(splitAddrs(p[1].Addr))
+	}
+	return n * len(splitAddrs(origDest))
+}
+
+// zonePairs is the src/dst zone expansion of a non-DNAT rule, with fw added beside all/any.
+func (c *Compiler) zonePairs(srcSpec, dstSpec, fwZone string) [][2]config.ZoneSpec {
+	srcs, srcGlobal := withFirewall(c.zoneSpecs(srcSpec), fwZone)
+	dsts, dstGlobal := withFirewall(c.zoneSpecs(dstSpec), fwZone)
+	var out [][2]config.ZoneSpec
+	for _, src := range srcs {
+		for _, dst := range dsts {
+			if src.Zone == fwZone && dst.Zone == fwZone && (srcGlobal || dstGlobal) {
+				continue
+			}
+			// Exclusion expansion never pairs fw with itself, and pairs a zone with itself only for "all+".
+			if src.Zone == dst.Zone && (isZoneExclusion(srcSpec) || isZoneExclusion(dstSpec)) &&
+				(src.Zone == fwZone || !strings.Contains(srcSpec, "+!") && !strings.Contains(dstSpec, "+!")) {
+				continue
+			}
+			out = append(out, [2]config.ZoneSpec{src, dst})
+		}
+	}
+	return out
 }
 
 // zoneSpecs expands a comma zone list; "all"/"any" stay global and "all!x,y" becomes every zone but x and y.
@@ -526,6 +637,26 @@ func (c *Compiler) zoneSpecs(spec string) []config.ZoneSpec {
 	return out
 }
 
+// withFirewall adds the firewall zone beside a global all/any spec, which otherwise only reaches forward.
+func withFirewall(specs []config.ZoneSpec, fwZone string) ([]config.ZoneSpec, bool) {
+	out, global := specs, false
+	for _, s := range specs {
+		if fwZone != "" && isGlobalZone(s.Zone) {
+			global = true
+			if fw := (config.ZoneSpec{Zone: fwZone, Addr: s.Addr}); !slices.Contains(out, fw) {
+				out = append(out, fw)
+			}
+		}
+	}
+	return out, global
+}
+
+func isGlobalZone(spec string) bool {
+	zone, _ := splitZoneSpec(spec)
+	base := strings.TrimSuffix(zone, "+")
+	return base == "all" || base == "any"
+}
+
 func isZoneExclusion(spec string) bool {
 	base, _, ok := strings.Cut(spec, "!")
 	base = strings.TrimSuffix(base, "+")
@@ -595,7 +726,7 @@ func (c *Compiler) compileZonePair(state *FirewallState, tag, srcZone, srcAddr,
 }
 
 func (c *Compiler) compileDNATRule(state *FirewallState, tag, srcZone, srcAddr, origDest, dstSpec, proto string,
-	dports config.PortSpec, action config.RuleAction, logLevel string) error {
+	dports, sports config.PortSpec, action config.RuleAction, logLevel string) error {
 	chain := "prerouting"
 
 	parts := strings.SplitN(dstSpec, ":", 3)
@@ -623,7 +754,7 @@ func (c *Compiler) compileDNATRule(state *FirewallState, tag, srcZone, srcAddr,
 		}
 	}
 
-	matches, err := l4Matches(proto, dports, nil)
+	matches, err := l4Matches(proto, dports, sports)
 	if err != nil {
 		return err
 	}
@@ -1554,10 +1685,14 @@ func matchOrigDest(addr string) ([]expr.Any, error) {
 	if err != nil {
 		return nil, err
 	}
-	return append([]expr.Any{
+	return append(matchNFProto(proto), dst...), nil
+}
+
+func matchNFProto(proto byte) []expr.Any {
+	return []expr.Any{
 		&expr.Meta{Key: expr.MetaKeyNFPROTO, Register: 1},
 		&expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{proto}},
-	}, dst...), nil
+	}
 }
 
 func matchAddrCIDR(cidr string, isSrc bool) ([]expr.Any, error) {
@@ -1645,13 +1780,18 @@ const (
 	ctStateRelated     = 4
 	ctStateNew         = 8
 	ctStateUntracked   = 64
+	ctStatusDNAT       = 32
 )
 
 func matchCtState(stateMask uint32) []expr.Any {
+	return matchCtBits(expr.CtKeySTATE, stateMask)
+}
+
+func matchCtBits(key expr.CtKey, stateMask uint32) []expr.Any {
 	stateBytes := make([]byte, 4)
 	binary.NativeEndian.PutUint32(stateBytes, stateMask)
 	return []expr.Any{
-		&expr.Ct{Key: expr.CtKeySTATE, Register: 1},
+		&expr.Ct{Key: key, Register: 1},
 		&expr.Bitwise{
 			SourceRegister: 1,
 			DestRegister:   1,
@@ -1957,6 +2097,13 @@ func rejectExprs(proto byte, family config.AddressFamily) []expr.Any {
 			Code: 0,
 		}}
 	}
+	// icmpx is inet-only; ip and ip6 tables silently drop on it.
+	switch family {
+	case config.FamilyIP:
+		return []expr.Any{&expr.Reject{Type: unix.NFT_REJECT_ICMP_UNREACH, Code: 3}} // port-unreachable
+	case config.FamilyIP6:
+		return []expr.Any{&expr.Reject{Type: unix.NFT_REJECT_ICMP_UNREACH, Code: 4}} // port-unreachable
+	}
 	return []expr.Any{&expr.Reject{
 		Type: unix.NFT_REJECT_ICMPX_UNREACH,
 		Code: unix.NFT_REJECT_ICMPX_PORT_UNREACH,

internal/nftables/compiler_test.go

@@ -1013,38 +1013,84 @@ func TestCompile_DHCP(t *testing.T) {
 		Zones: map[string]config.Zone{
 			"fw":  {Type: config.ZoneFirewall},
 			"net": {Type: config.ZoneIP},
+			"loc": {Type: config.ZoneIP},
 		},
 		Interfaces: []config.Interface{
 			{Zone: "net", Interface: "eth0", Options: config.InterfaceOptions{DHCP: true}},
+			{Zone: "loc", Interface: "br0", Options: config.InterfaceOptions{DHCP: true, Bridge: true}},
 		},
 		Policy: []config.Policy{
 			{Source: "all", Dest: "all", Action: config.PolicyDrop},
 		},
 		PortGroups: make(map[string]config.PortGroup),
 	}
-	c := NewCompiler(cfg)
+	state, err := NewCompiler(cfg).Compile()
-	state, err := c.Compile()
 	if err != nil {
 		t.Fatalf("Compile() error: %v", err)
 	}
 
-	foundIn := false
+	find := func(chain, tag string) *ManagedRule {
-	foundOut := false
+		for i, r := range state.Rules[chain] {
-	for _, r := range state.Rules["input"] {
+			if r.Tag == tag {
-		if r.Tag == "dhcp:in:eth0" || r.Tag == "dhcp:reply:eth0" {
+				return &state.Rules[chain][i]
-			foundIn = true
+			}
+		}
+		return nil
+	}
+	for _, want := range []struct{ chain, tag, iif, oif string }{
+		{"input", "dhcp:in:eth0", "eth0", ""},
+		{"output", "dhcp:out:eth0", "", "eth0"},
+		{"input", "dhcp:in:br0", "br0", ""},
+		{"output", "dhcp:out:br0", "", "br0"},
+		{"forward", "dhcp:fwd:br0", "br0", "br0"},
+	} {
+		r := find(want.chain, want.tag)
+		if r == nil {
+			t.Errorf("%s: no rule %s", want.chain, want.tag)
+			continue
+		}
+		metas := map[expr.MetaKey][]byte{}
+		for i := 0; i+1 < len(r.Exprs); i++ {
+			if m, ok := r.Exprs[i].(*expr.Meta); ok {
+				if c, ok := r.Exprs[i+1].(*expr.Cmp); ok && c.Op == expr.CmpOpEq {
+					metas[m.Key] = c.Data
+				}
+			}
+		}
+		if got := metas[expr.MetaKeyNFPROTO]; !bytes.Equal(got, []byte{unix.NFPROTO_IPV4}) {
+			t.Errorf("%s: nfproto %v, want ipv4 guard", want.tag, got)
+		}
+		if got := metas[expr.MetaKeyL4PROTO]; !bytes.Equal(got, []byte{unix.IPPROTO_UDP}) {
+			t.Errorf("%s: l4proto %v, want udp", want.tag, got)
+		}
+		for key, name := range map[expr.MetaKey]string{expr.MetaKeyIIFNAME: want.iif, expr.MetaKeyOIFNAME: want.oif} {
+			got, ok := metas[key]
+			if name == "" {
+				if ok {
+					t.Errorf("%s: unexpected meta %v match %q", want.tag, key, got)
+				}
+			} else if string(got) != name+"\x00" {
+				t.Errorf("%s: meta %v %q, want %q", want.tag, key, got, name)
+			}
+		}
+		v, ok := r.Exprs[len(r.Exprs)-1].(*expr.Verdict)
+		if !ok || v.Kind != expr.VerdictAccept {
+			t.Errorf("%s: last expr %#v, want accept verdict", want.tag, r.Exprs[len(r.Exprs)-1])
+		}
+		var lo, hi []byte
+		for _, e := range r.Exprs {
+			if c, ok := e.(*expr.Cmp); ok && c.Op == expr.CmpOpGte {
+				lo = c.Data
+			} else if ok && c.Op == expr.CmpOpLte {
+				hi = c.Data
+			}
+		}
+		if !bytes.Equal(lo, []byte{0, 67}) || !bytes.Equal(hi, []byte{0, 68}) {
+			t.Errorf("%s: dport range %v-%v, want 67-68", want.tag, lo, hi)
 		}
 	}
-	for _, r := range state.Rules["output"] {
+	if find("forward", "dhcp:fwd:eth0") != nil {
-		if r.Tag == "dhcp:out:eth0" {
+		t.Error("non-bridge eth0 must not forward DHCP")
-			foundOut = true
-		}
-	}
-	if !foundIn {
-		t.Error("no DHCP input rule found for eth0")
-	}
-	if !foundOut {
-		t.Error("no DHCP output rule found for eth0")
 	}
 }
 
@@ -1918,12 +1964,58 @@ func TestCompile_CommaZoneLists(t *testing.T) {
 		{
 			name: "dnat source list",
 			rule: config.Rule{Action: config.RuleDNAT, Source: "net,lan", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
-			want: map[string][]string{"prerouting": {"iif=eth0", "iif=eth1"}},
+			want: map[string][]string{"prerouting": {"iif=eth0", "iif=eth1"},
+				"forward": {"iif=eth0 oif=eth2 daddr=192.0.2.10", "iif=eth1 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat source list skips the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "net,svr", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth0"}, "forward": {"iif=eth0 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat lone source zone may equal the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "svr", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth2"}, "forward": {"iif=eth2 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat exclusion source skips the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "all!fw,anycast", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth1", "iif=eth0"},
+				"forward": {"iif=eth1 oif=eth2 daddr=192.0.2.10", "iif=eth0 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat intrazone exclusion source keeps the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "all+!fw,anycast,lan", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth0", "iif=eth2"},
+				"forward": {"iif=eth0 oif=eth2 daddr=192.0.2.10", "iif=eth2 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat all source expands per zone and skips fw and the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "all", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth3", "iif=eth1", "iif=eth0"},
+				"forward": {"iif=eth3 oif=eth2 daddr=192.0.2.10", "iif=eth1 oif=eth2 daddr=192.0.2.10", "iif=eth0 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat any+ source keeps the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "any+", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth3", "iif=eth1", "iif=eth0", "iif=eth2"},
+				"forward": {"iif=eth3 oif=eth2 daddr=192.0.2.10", "iif=eth1 oif=eth2 daddr=192.0.2.10", "iif=eth0 oif=eth2 daddr=192.0.2.10", "iif=eth2 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat to fw accepts in input",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "fw:192.0.2.1", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth0"}, "input": {"iif=eth0 daddr=192.0.2.1"}},
+		},
+		{
+			name: "redirect accepts in input without daddr",
+			rule: config.Rule{Action: config.RuleRedirect, Source: "lan", Dest: "fw:192.0.2.1:3128", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth1"}, "input": {"iif=eth1"}},
 		},
 		{
 			name: "dnat source address list",
 			rule: config.Rule{Action: config.RuleDNAT, Source: "net:192.0.2.5,198.51.100.5", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
-			want: map[string][]string{"prerouting": {"iif=eth0 saddr=192.0.2.5", "iif=eth0 saddr=198.51.100.5"}},
+			want: map[string][]string{"prerouting": {"iif=eth0 saddr=192.0.2.5", "iif=eth0 saddr=198.51.100.5"},
+				"forward": {"iif=eth0 oif=eth2 saddr=192.0.2.5 daddr=192.0.2.10", "iif=eth0 oif=eth2 saddr=198.51.100.5 daddr=192.0.2.10"}},
 		},
 		{
 			name: "negated address list stays one AND-ed rule",
@@ -1958,17 +2050,20 @@ func TestCompile_CommaZoneLists(t *testing.T) {
 		{
 			name: "dnat origdest",
 			rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}, OrigDest: "203.0.113.5"},
-			want: map[string][]string{"prerouting": {"iif=eth0 daddr=203.0.113.5"}},
+			want: map[string][]string{"prerouting": {"iif=eth0 daddr=203.0.113.5"},
+				"forward": {"iif=eth0 oif=eth2 daddr=192.0.2.17"}},
 		},
 		{
 			name: "dnat origdest list",
 			rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}, OrigDest: "203.0.113.5,203.0.113.6"},
-			want: map[string][]string{"prerouting": {"iif=eth0 daddr=203.0.113.5", "iif=eth0 daddr=203.0.113.6"}},
+			want: map[string][]string{"prerouting": {"iif=eth0 daddr=203.0.113.5", "iif=eth0 daddr=203.0.113.6"},
+				"forward": {"iif=eth0 oif=eth2 daddr=192.0.2.17"}},
 		},
 		{
 			name: "dnat negated origdest list",
 			rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}, OrigDest: "!203.0.113.5,203.0.113.6"},
-			want: map[string][]string{"prerouting": {"iif=eth0 !daddr=203.0.113.5 !daddr=203.0.113.6"}},
+			want: map[string][]string{"prerouting": {"iif=eth0 !daddr=203.0.113.5 !daddr=203.0.113.6"},
+				"forward": {"iif=eth0 oif=eth2 daddr=192.0.2.17"}},
 		},
 		{
 			name: "accept origdest",
@@ -2019,7 +2114,7 @@ func TestCompile_CommaZoneLists(t *testing.T) {
 			got := map[string][]string{}
 			for chain, rules := range state.Rules {
 				for _, r := range rules {
-					if r.Tag == tag {
+					if r.Tag == tag || r.Tag == tag+":accept" {
 						got[chain] = append(got[chain], describeRule(r))
 					}
 				}
@@ -2217,11 +2312,116 @@ func TestCompile_DNATGetsNoRuleExtras(t *testing.T) {
 	}
 }
 
+func TestCompile_DNATImpliedAccept(t *testing.T) {
+	state, err := NewCompiler(listCfg(func(c *config.Config) {
+		c.Zones["svr"] = config.Zone{Type: config.ZoneIP}
+		c.Interfaces = append(c.Interfaces, config.Interface{Zone: "svr", Interface: "eth2"})
+		c.Rules = []config.Rule{{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17:8080", Proto: "tcp", DPort: config.PortSpec{"80"}}}
+	})).Compile()
+	if err != nil {
+		t.Fatalf("Compile() error: %v", err)
+	}
+	fwd := taggedRules(state, "forward", "rule:0:accept")
+	if len(fwd) != 1 {
+		t.Fatalf("got %d forward accepts, want 1", len(fwd))
+	}
+	want := append(append(append(append(append(matchIfaceName(true, "eth0"), matchIfaceName(false, "eth2")...),
+		mustExprs(t)(matchDestCIDR("192.0.2.17"))...), mustExprs(t)(l4Exprs("tcp", "8080"))...),
+		dnatStatusExprs...), &expr.Verdict{Kind: expr.VerdictAccept})
+	if !reflect.DeepEqual(fwd[0].Exprs, want) {
+		t.Errorf("forward accept = %#v, want %#v", fwd[0].Exprs, want)
+	}
+}
+
+// IPS_DST_NAT = 1<<5, hard-coded so a wrong ctStatusDNAT or ct key fails here.
+var dnatStatusExprs = []expr.Any{
+	&expr.Ct{Key: expr.CtKeySTATUS, Register: 1},
+	&expr.Bitwise{SourceRegister: 1, DestRegister: 1, Len: 4, Mask: binary.NativeEndian.AppendUint32(nil, 32), Xor: []byte{0, 0, 0, 0}},
+	&expr.Cmp{Op: expr.CmpOpNeq, Register: 1, Data: []byte{0, 0, 0, 0}},
+}
+
+func TestCompile_DNATImpliedAcceptGetsNoRuleExtras(t *testing.T) {
+	compile := func(r config.Rule) *FirewallState {
+		state, err := NewCompiler(listCfg(func(c *config.Config) {
+			c.Zones["svr"] = config.Zone{Type: config.ZoneIP}
+			c.Interfaces = append(c.Interfaces, config.Interface{Zone: "svr", Interface: "eth2"})
+			c.Rules = []config.Rule{r}
+		})).Compile()
+		if err != nil {
+			t.Fatalf("Compile() error: %v", err)
+		}
+		return state
+	}
+	plain := config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}}
+	extras := plain
+	extras.RateLimit, extras.Mark, extras.User = "10/sec:5", "0x1", "root"
+	want, got := compile(plain), compile(extras)
+	if len(taggedRules(got, "forward", "rule:0:accept")) != 1 {
+		t.Fatalf("want one forward accept, got %v", got.Rules["forward"])
+	}
+	if !reflect.DeepEqual(got.Rules, want.Rules) {
+		t.Errorf("ratelimit/mark/user changed the DNAT rules:\ngot  %#v\nwant %#v", got.Rules, want.Rules)
+	}
+}
+
+func TestCompile_DNATMatchesSport(t *testing.T) {
+	state, err := NewCompiler(listCfg(func(c *config.Config) {
+		c.Zones["svr"] = config.Zone{Type: config.ZoneIP}
+		c.Interfaces = append(c.Interfaces, config.Interface{Zone: "svr", Interface: "eth2"})
+		c.Rules = []config.Rule{{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp",
+			DPort: config.PortSpec{"80"}, SPort: config.PortSpec{"1024"}}}
+	})).Compile()
+	if err != nil {
+		t.Fatalf("Compile() error: %v", err)
+	}
+	m, err := l4Matches("tcp", config.PortSpec{"80"}, config.PortSpec{"1024"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, r := range append(taggedRules(state, "prerouting", "rule:0"), taggedRules(state, "forward", "rule:0:accept")...) {
+		if !containsExprs(r.Exprs, m[0].exprs) {
+			t.Errorf("%s rule lacks the sport match: %#v", r.Chain, r.Exprs)
+		}
+	}
+}
+
+func containsExprs(haystack, needle []expr.Any) bool {
+	for i := 0; i+len(needle) <= len(haystack); i++ {
+		if reflect.DeepEqual(haystack[i:i+len(needle)], needle) {
+			return true
+		}
+	}
+	return false
+}
+
+func mustExprs(t *testing.T) func([]expr.Any, error) []expr.Any {
+	return func(e []expr.Any, err error) []expr.Any {
+		t.Helper()
+		if err != nil {
+			t.Fatal(err)
+		}
+		return e
+	}
+}
+
+func l4Exprs(proto, port string) ([]expr.Any, error) {
+	m, err := l4Matches(proto, config.PortSpec{port}, nil)
+	if err != nil {
+		return nil, err
+	}
+	return m[0].exprs, nil
+}
+
 func TestCompile_CommaZoneListLimitErrors(t *testing.T) {
 	for _, r := range []config.Rule{
 		{Action: config.RuleAccept, Source: "net", Dest: "fw,lan", RateLimit: "10/sec:5"},
 		{Action: config.RuleAccept, Source: "net,lan", Dest: "fw", ConnLimit: "10"},
 		{Action: config.RuleAccept, Source: "net", Dest: "fw:192.0.2.1,198.51.100.1", RateLimit: "10/sec"},
+		{Action: config.RuleDNAT, Source: "net,lan", Dest: "fw:192.0.2.1", RateLimit: "10/sec"},
+		{Action: config.RuleAccept, Source: "all", Dest: "all", RateLimit: "10/sec"},
+		{Action: config.RuleAccept, Source: "net", Dest: "all", ConnLimit: "10"},
+		{Action: config.RuleAccept, Source: "all", Dest: "net", RateLimit: "10/sec"},
+		{Action: config.RuleAccept, Source: "net,all", Dest: "fw", RateLimit: "10/sec"},
 	} {
 		t.Run(r.Source+">"+r.Dest, func(t *testing.T) {
 			cfg := &config.Config{
@@ -2799,3 +2999,324 @@ func TestCompile_ConntrackHelperZones(t *testing.T) {
 		})
 	}
 }
+
+func TestCompile_AllIncludesFirewallMatches(t *testing.T) {
+	cases := []struct {
+		name string
+		rule config.Rule
+		want map[string][]string
+	}{
+		{
+			name: "all address kept on added fw rules",
+			rule: config.Rule{Action: config.RuleAccept, Source: "all:192.0.2.5", Dest: "all"},
+			want: map[string][]string{"input": {"saddr=192.0.2.5"}, "output": {"saddr=192.0.2.5"}, "forward": {"saddr=192.0.2.5"}},
+		},
+		{
+			name: "dnat with all source skips fw",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "all", Dest: "fw:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth0"}},
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			cfg := &config.Config{
+				Settings:   config.Settings{TableName: "test", AddressFamily: config.FamilyINET},
+				Zones:      map[string]config.Zone{"fw": {Type: config.ZoneFirewall}, "net": {Type: config.ZoneIP}},
+				Interfaces: []config.Interface{{Zone: "net", Interface: "eth0"}},
+				Rules:      []config.Rule{tc.rule},
+				PortGroups: map[string]config.PortGroup{},
+			}
+			state := mustCompile(t, cfg)
+			got := map[string][]string{}
+			for _, chain := range []string{"prerouting", "input", "output", "forward"} {
+				for _, r := range taggedRules(state, chain, "rule:0") {
+					got[chain] = append(got[chain], describeRule(r))
+				}
+			}
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Errorf("rules = %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestCompile_AllIncludesFirewall(t *testing.T) {
+	cases := []struct {
+		src, dst string
+		want     map[string]int
+	}{
+		{"all", "all", map[string]int{"input": 1, "output": 1, "forward": 1}},
+		{"net", "all", map[string]int{"input": 1, "output": 0, "forward": 1}},
+		{"all", "net", map[string]int{"input": 0, "output": 1, "forward": 1}},
+		{"all", "fw", map[string]int{"input": 1, "output": 0, "forward": 0}},
+		{"all:192.0.2.0/24", "fw", map[string]int{"input": 1, "output": 0, "forward": 0}},
+		{"all!fw", "all!fw", map[string]int{"input": 0, "output": 0, "forward": 2}},
+		{"all+", "all", map[string]int{"input": 1, "output": 1, "forward": 1}},
+		{"net,all", "fw", map[string]int{"input": 2, "output": 0, "forward": 0}},
+		{"fw,all", "net", map[string]int{"input": 0, "output": 1, "forward": 1}},
+	}
+	for _, tc := range cases {
+		t.Run(tc.src+"->"+tc.dst, func(t *testing.T) {
+			cfg := &config.Config{
+				Settings: config.Settings{TableName: "test", AddressFamily: config.FamilyINET},
+				Zones: map[string]config.Zone{
+					"fw":  {Type: config.ZoneFirewall},
+					"net": {Type: config.ZoneIP},
+					"loc": {Type: config.ZoneIP},
+				},
+				Interfaces: []config.Interface{{Zone: "net", Interface: "eth0"}, {Zone: "loc", Interface: "eth1"}},
+				Rules: []config.Rule{
+					{Action: config.RuleAccept, Source: tc.src, Dest: tc.dst, Proto: "icmp", DPort: config.PortSpec{"8"}},
+				},
+				PortGroups: map[string]config.PortGroup{},
+			}
+			state, err := NewCompiler(cfg).Compile()
+			if err != nil {
+				t.Fatalf("Compile() error: %v", err)
+			}
+			for chain, want := range tc.want {
+				got := 0
+				for _, r := range state.Rules[chain] {
+					if r.Tag == "rule:0" {
+						got++
+					}
+				}
+				if got != want {
+					t.Errorf("%s: got %d rule:0 entries, want %d", chain, got, want)
+				}
+			}
+		})
+	}
+}
+
+func TestSpecCount_CommaAllMatchesExpansion(t *testing.T) {
+	c := NewCompiler(&config.Config{
+		Zones: map[string]config.Zone{"fw": {Type: config.ZoneFirewall}, "net": {Type: config.ZoneIP}},
+	})
+	for _, tc := range []struct {
+		src, dst string
+		want     int
+	}{
+		{"net,all", "fw", 2},
+		{"fw,all", "net", 2},
+	} {
+		if got := c.specCount(tc.src, tc.dst, "", "fw", config.RuleAccept); got != tc.want {
+			t.Errorf("specCount(%s, %s) = %d, want %d", tc.src, tc.dst, got, tc.want)
+		}
+	}
+}
+
+func TestCompile_Dispositions(t *testing.T) {
+	cases := []struct {
+		invalid, untracked config.PolicyAction
+		want               map[string]expr.Any // tag prefix -> verdict expr, nil = absent
+	}{
+		{"", "", map[string]expr.Any{"ct:invalid:": &expr.Verdict{Kind: expr.VerdictDrop}, "ct:untracked:": nil}},
+		{config.PolicyContinue, config.PolicyContinue, map[string]expr.Any{"ct:invalid:": nil, "ct:untracked:": nil}},
+		{config.PolicyReject, config.PolicyAccept, map[string]expr.Any{
+			"ct:invalid:":   rejectExprs(0, config.FamilyINET)[0],
+			"ct:untracked:": &expr.Verdict{Kind: expr.VerdictAccept},
+		}},
+	}
+	for _, tc := range cases {
+		cfg := &config.Config{
+			Settings: config.Settings{
+				TableName:            "test",
+				AddressFamily:        config.FamilyINET,
+				InvalidDisposition:   tc.invalid,
+				UntrackedDisposition: tc.untracked,
+			},
+			Zones:      map[string]config.Zone{"fw": {Type: config.ZoneFirewall}, "net": {Type: config.ZoneIP}},
+			Interfaces: []config.Interface{{Zone: "net", Interface: "eth0"}},
+			Policy:     []config.Policy{{Source: "all", Dest: "all", Action: config.PolicyDrop}},
+			PortGroups: make(map[string]config.PortGroup),
+		}
+		state, err := NewCompiler(cfg).Compile()
+		if err != nil {
+			t.Fatalf("Compile: %v", err)
+		}
+		for _, chain := range []string{"input", "forward", "output"} {
+			for prefix, want := range tc.want {
+				var got *ManagedRule
+				for i, r := range state.Rules[chain] {
+					if r.Tag == prefix+chain {
+						got = &state.Rules[chain][i]
+					}
+				}
+				switch {
+				case want == nil && got != nil:
+					t.Errorf("%q/%q: unexpected %s%s rule", tc.invalid, tc.untracked, prefix, chain)
+				case want != nil && got == nil:
+					t.Errorf("%q/%q: missing %s%s rule", tc.invalid, tc.untracked, prefix, chain)
+				case want != nil && !reflect.DeepEqual(got.Exprs[len(got.Exprs)-1], want):
+					t.Errorf("%q/%q: %s%s verdict = %#v, want %#v", tc.invalid, tc.untracked, prefix, chain, got.Exprs[len(got.Exprs)-1], want)
+				}
+			}
+		}
+	}
+}
+
+func TestCompile_LogLimit(t *testing.T) {
+	cfg := &config.Config{
+		Settings: config.Settings{
+			TableName:     "test",
+			AddressFamily: config.FamilyINET,
+			LogLimit:      "1/sec:10",
+		},
+		Zones: map[string]config.Zone{
+			"fw":  {Type: config.ZoneFirewall},
+			"net": {Type: config.ZoneIP},
+		},
+		Interfaces: []config.Interface{{Zone: "net", Interface: "eth0"}},
+		Policy: []config.Policy{
+			{Source: "net", Dest: "all", Action: config.PolicyDrop, Log: "info"},
+		},
+		Rules: []config.Rule{
+			{Action: config.RuleAccept, Source: "net:192.0.2.1", Dest: "fw", Proto: "tcp", DPort: config.PortSpec{"22"}, Log: "info"},
+			{Action: config.RuleLog, Source: "net:198.51.100.0/24", Dest: "fw", Proto: "tcp", DPort: config.PortSpec{"23"}, Log: "info"},
+		},
+		PortGroups: make(map[string]config.PortGroup),
+	}
+	state, err := NewCompiler(cfg).Compile()
+	if err != nil {
+		t.Fatalf("Compile() error: %v", err)
+	}
+
+	want := &expr.Limit{Type: expr.LimitTypePkts, Rate: 1, Unit: expr.LimitTimeSecond, Burst: 10}
+	byTag := map[string][]ManagedRule{}
+	for _, r := range state.Rules["input"] {
+		byTag[r.Tag] = append(byTag[r.Tag], r)
+	}
+	for tag, n := range map[string]int{"policy:0": 2, "rule:0": 2, "rule:1": 1} {
+		rs := byTag[tag]
+		if len(rs) != n {
+			t.Fatalf("%s: %d rules, want %d", tag, len(rs), n)
+		}
+		logRule := rs[0].Exprs
+		l := len(logRule)
+		if l < 2 || !reflect.DeepEqual(logRule[l-2], want) {
+			t.Errorf("%s: want limit before log, got %#v", tag, logRule)
+		}
+		if _, ok := logRule[l-1].(*expr.Log); !ok {
+			t.Errorf("%s: log rule must end in log, got %T", tag, logRule[l-1])
+		}
+		for _, e := range rs[n-1].Exprs[:len(rs[n-1].Exprs)-1] {
+			if _, ok := e.(*expr.Log); n == 2 && ok {
+				t.Errorf("%s: verdict rule still logs", tag)
+			}
+		}
+	}
+	if logs := byTag["policy:0"]; len(logs) == 2 {
+		if _, ok := logs[1].Exprs[len(logs[1].Exprs)-1].(*expr.Verdict); !ok {
+			t.Errorf("policy verdict rule must end in a verdict")
+		}
+	}
+}
+
+func TestCompile_NoLogLimitKeepsInlineLog(t *testing.T) {
+	cfg := &config.Config{
+		Settings:   config.Settings{TableName: "test", AddressFamily: config.FamilyINET},
+		Zones:      map[string]config.Zone{"fw": {Type: config.ZoneFirewall}, "net": {Type: config.ZoneIP}},
+		Interfaces: []config.Interface{{Zone: "net", Interface: "eth0"}},
+		Policy:     []config.Policy{{Source: "net", Dest: "all", Action: config.PolicyDrop, Log: "info"}},
+		PortGroups: make(map[string]config.PortGroup),
+	}
+	state, err := NewCompiler(cfg).Compile()
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, r := range state.Rules["input"] {
+		for _, e := range r.Exprs {
+			if _, ok := e.(*expr.Limit); ok {
+				t.Errorf("%s: unexpected limit without log_limit", r.Tag)
+			}
+		}
+	}
+}
+
+func TestCompile_LogLimitSplitsAroundExtrasAndNAT(t *testing.T) {
+	cfg := listCfg(func(c *config.Config) {
+		c.Settings.LogLimit = "1/sec:10"
+		c.Zones["loc"] = config.Zone{Type: config.ZoneIP}
+		c.Interfaces = append(c.Interfaces, config.Interface{Zone: "loc", Interface: "eth1"})
+		c.Rules = []config.Rule{
+			{Action: config.RuleAccept, Source: "net:192.0.2.1", Dest: "fw", Proto: "tcp", DPort: config.PortSpec{"22"}, Log: "info",
+				Mark: "0x1", User: "0", Time: &config.TimeSpec{Start: "08:00", Stop: "17:00"}, RateLimit: "5/sec:20"},
+			{Action: config.RuleLog, Source: "net:192.0.2.2", Dest: "fw", Proto: "tcp", DPort: config.PortSpec{"23"}, Log: "info",
+				Mark: "0x2", RateLimit: "5/sec:20"},
+			{Action: config.RuleDNAT, Source: "net", Dest: "loc:198.51.100.10:80", Proto: "tcp", DPort: config.PortSpec{"8000"}, Log: "info"},
+			{Action: config.RuleAccept, Source: "net:192.0.2.3", Dest: "loc", Proto: "tcp", DPort: config.PortSpec{"443"}, Log: "info"},
+		}
+		c.SNAT = []config.SNATRule{{Action: config.SNATAddress, Source: "198.51.100.0/24", Dest: "eth0", Address: "203.0.113.7", Mark: "0x3", Log: "info"}}
+	})
+	state, err := NewCompiler(cfg).Compile()
+	if err != nil {
+		t.Fatalf("Compile() error: %v", err)
+	}
+	logLimit := &expr.Limit{Type: expr.LimitTypePkts, Rate: 1, Unit: expr.LimitTimeSecond, Burst: 10}
+	kinds := func(exprs []expr.Any) (logs, limits, marks, uids int) {
+		for _, e := range exprs {
+			switch v := e.(type) {
+			case *expr.Log:
+				logs++
+			case *expr.Limit:
+				limits++
+			case *expr.Meta:
+				if v.Key == expr.MetaKeyMARK && !v.SourceRegister {
+					marks++
+				}
+				if v.Key == expr.MetaKeySKUID {
+					uids++
+				}
+			}
+		}
+		return
+	}
+	check := func(name string, rs []ManagedRule, n, wantMarks, wantUIDs int) []expr.Any {
+		t.Helper()
+		if len(rs) != n {
+			t.Fatalf("%s: %d rules, want %d", name, len(rs), n)
+		}
+		l := rs[0].Exprs
+		if len(l) < 2 || !reflect.DeepEqual(l[len(l)-2], logLimit) {
+			t.Errorf("%s: log rule must end limit+log, got %#v", name, l)
+		}
+		if logs, limits, marks, uids := kinds(l); logs != 1 || limits != 1 || marks != wantMarks || uids != wantUIDs {
+			t.Errorf("%s: log rule logs=%d limits=%d marks=%d uids=%d, want 1/1/%d/%d", name, logs, limits, marks, uids, wantMarks, wantUIDs)
+		}
+		if n == 1 {
+			return nil
+		}
+		v := rs[1].Exprs
+		if logs, _, marks, uids := kinds(v); logs != 0 || marks != wantMarks || uids != wantUIDs {
+			t.Errorf("%s: action rule logs=%d marks=%d uids=%d, want 0/%d/%d", name, logs, marks, uids, wantMarks, wantUIDs)
+		}
+		return v
+	}
+
+	v := check("rule:0", taggedRules(state, "input", "rule:0"), 2, 1, 1)
+	if _, limits, _, _ := kinds(v); limits != 1 {
+		t.Errorf("rule:0: action rule must keep its ratelimit, got %d limits", limits)
+	}
+	if _, ok := v[len(v)-1].(*expr.Verdict); !ok {
+		t.Errorf("rule:0: action rule must end in a verdict, got %T", v[len(v)-1])
+	}
+	check("rule:1", taggedRules(state, "input", "rule:1"), 1, 1, 0)
+	if v := check("rule:2", taggedRules(state, "prerouting", "rule:2"), 2, 0, 0); v != nil {
+		if _, ok := v[len(v)-1].(*expr.NAT); !ok {
+			t.Errorf("rule:2: DNAT rule must end in nat, got %T", v[len(v)-1])
+		}
+	}
+	check("rule:3", taggedRules(state, "forward", "rule:3"), 2, 0, 0)
+	var snat []ManagedRule
+	for _, r := range state.Rules["postrouting"] {
+		if logs, _, _, _ := kinds(r.Exprs); logs > 0 || strings.HasPrefix(r.Tag, "snat:0") {
+			snat = append(snat, r)
+		}
+	}
+	if v := check("snat:0", snat, 2, 1, 0); v != nil {
+		if _, ok := v[len(v)-1].(*expr.NAT); !ok {
+			t.Errorf("snat:0: rule must end in nat, got %T", v[len(v)-1])
+		}
+	}
+}

internal/nftables/engine.go

@@ -5,6 +5,8 @@ import (
 
 	"github.com/google/nftables"
 	"github.com/google/nftables/expr"
+	"github.com/mdlayher/netlink"
+	"golang.org/x/sys/unix"
 
 	"git.unkin.net/unkin/tomswall/internal/config"
 )
@@ -15,16 +17,101 @@ type Engine struct {
 }
 
 func NewEngine(cfg *config.Config) (*Engine, error) {
-	conn, err := nftables.New()
+	conn, err := nftables.New(nftables.WithSockOptions(largeBuffers))
 	if err != nil {
 		return nil, fmt.Errorf("connecting to nftables: %w", err)
 	}
 	return &Engine{cfg: cfg, conn: conn}, nil
 }
 
+var tableFamilies = map[config.AddressFamily]nftables.TableFamily{
+	config.FamilyINET: nftables.TableFamilyINet,
+	config.FamilyIP:   nftables.TableFamilyIPv4,
+	config.FamilyIP6:  nftables.TableFamilyIPv6,
+}
+
+func (e *Engine) family() nftables.TableFamily {
+	if f, ok := tableFamilies[e.cfg.Settings.AddressFamily]; ok {
+		return f
+	}
+	return nftables.TableFamilyINet
+}
+
+func addressFamily(tf nftables.TableFamily) config.AddressFamily {
+	for f, t := range tableFamilies {
+		if t == tf {
+			return f
+		}
+	}
+	return config.FamilyINET
+}
+
+// withFamily is the engine for the same table name in another address family.
+func (e *Engine) withFamily(f config.AddressFamily) *Engine {
+	cfg := *e.cfg
+	cfg.Settings.AddressFamily = f
+	return &Engine{cfg: &cfg, conn: e.conn}
+}
+
+// overlaps reports whether tables of families a and b filter the same traffic:
+// inet covers both ip and ip6, which do not overlap each other.
+func overlaps(a, b nftables.TableFamily) bool {
+	if a == b {
+		return false
+	}
+	return (a == nftables.TableFamilyINet && (b == nftables.TableFamilyIPv4 || b == nftables.TableFamilyIPv6)) ||
+		(b == nftables.TableFamilyINet && (a == nftables.TableFamilyIPv4 || a == nftables.TableFamilyIPv6))
+}
+
+// staleTables are our-named tables left by a different address_family that
+// would still filter the traffic this family now owns.
+func (e *Engine) staleTables() ([]*nftables.Table, error) {
+	tables, err := e.conn.ListTables()
+	if err != nil {
+		return nil, fmt.Errorf("listing tables: %w", err)
+	}
+	var stale []*nftables.Table
+	for _, t := range tables {
+		if t.Name == e.cfg.Settings.TableName && overlaps(e.family(), t.Family) {
+			stale = append(stale, t)
+		}
+	}
+	return stale, nil
+}
+
+// batchBufSize bounds one batch: the kernel rejects a batch larger than the
+// send buffer (EMSGSIZE) and drops ACKs beyond the receive buffer (ENOBUFS)
+// after committing it.
+// ponytail: fixed cap of tens of thousands of rules; size per batch if exceeded.
+const batchBufSize = 64 << 20
+
+// largeBuffers raises both socket buffers, ignoring rmem_max/wmem_max when
+// CAP_NET_ADMIN allows it and falling back to the capped sizes otherwise.
+func largeBuffers(c *netlink.Conn) error {
+	rc, err := c.SyscallConn()
+	if err != nil {
+		return err
+	}
+	var serr error
+	err = rc.Control(func(fd uintptr) {
+		for _, o := range [][2]int{{unix.SO_SNDBUFFORCE, unix.SO_SNDBUF}, {unix.SO_RCVBUFFORCE, unix.SO_RCVBUF}} {
+			if unix.SetsockoptInt(int(fd), unix.SOL_SOCKET, o[0], batchBufSize) == nil {
+				continue
+			}
+			if serr = unix.SetsockoptInt(int(fd), unix.SOL_SOCKET, o[1], batchBufSize); serr != nil {
+				return
+			}
+		}
+	})
+	if err != nil {
+		return err
+	}
+	return serr
+}
+
 func (e *Engine) ensureTable() *nftables.Table {
 	return e.conn.AddTable(&nftables.Table{
-		Family: nftables.TableFamilyINet,
+		Family: e.family(),
 		Name:   e.cfg.Settings.TableName,
 	})
 }
@@ -132,6 +219,13 @@ func (e *Engine) Apply(changes *ChangeSet) error {
 }
 
 func (e *Engine) apply(changes *ChangeSet, policies map[string]nftables.ChainPolicy) error {
+	stale, err := e.staleTables()
+	if err != nil {
+		return err
+	}
+	for _, t := range stale {
+		e.conn.DelTable(t)
+	}
 	table := e.ensureTable()
 	chains := e.ensureChains(table, policies)
 
@@ -173,18 +267,24 @@ func (e *Engine) apply(changes *ChangeSet, policies map[string]nftables.ChainPol
 }
 
 func (e *Engine) Flush() error {
-	tables, err := e.conn.ListTables()
+	tables, err := e.staleTables()
 	if err != nil {
-		return fmt.Errorf("listing tables: %w", err)
+		return err
+	}
+	own, err := e.findTable()
+	if err != nil {
+		return err
+	}
+	if own != nil {
+		tables = append(tables, own)
+	}
+	if len(tables) == 0 {
+		return nil
 	}
-
 	for _, t := range tables {
-		if t.Name == e.cfg.Settings.TableName {
+		e.conn.DelTable(t)
-			e.conn.DelTable(t)
-			return e.conn.Flush()
-		}
 	}
-	return nil
+	return e.conn.Flush()
 }
 
 func (e *Engine) findTable() (*nftables.Table, error) {
@@ -193,7 +293,7 @@ func (e *Engine) findTable() (*nftables.Table, error) {
 		return nil, fmt.Errorf("listing tables: %w", err)
 	}
 	for _, t := range tables {
-		if t.Name == e.cfg.Settings.TableName && t.Family == nftables.TableFamilyINet {
+		if t.Name == e.cfg.Settings.TableName && t.Family == e.family() {
 			return t, nil
 		}
 	}
@@ -222,7 +322,7 @@ func (e *Engine) readCurrentState() (*FirewallState, error) {
 		}
 	}
 
-	chains, err := e.conn.ListChainsOfTableFamily(nftables.TableFamilyINet)
+	chains, err := e.conn.ListChainsOfTableFamily(e.family())
 	if err != nil {
 		return nil, fmt.Errorf("listing chains: %w", err)
 	}
@@ -252,6 +352,7 @@ func (e *Engine) readCurrentState() (*FirewallState, error) {
 // survives the process that took it.
 type Snapshot struct {
 	Table    string                          `json:"table"`
+	Family   config.AddressFamily            `json:"family,omitempty"`
 	Present  bool                            `json:"present"`
 	Policies map[string]nftables.ChainPolicy `json:"policies,omitempty"`
 	Rules    map[string][]SnapshotRule       `json:"rules,omitempty"`
@@ -264,16 +365,30 @@ type SnapshotRule struct {
 	Exprs [][]byte `json:"exprs"`
 }
 
-// Snapshot captures the live tomswall table so Restore can roll back to it.
+// Snapshot captures the live tomswall table so Restore can roll back to it,
+// falling back to the overlapping table of another family that apply replaces.
 func (e *Engine) Snapshot() (*Snapshot, error) {
-	snap := &Snapshot{Table: e.cfg.Settings.TableName}
 	t, err := e.findTable()
-	if err != nil || t == nil {
+	if err != nil {
-		return snap, err
+		return nil, err
+	}
+	if t == nil {
+		stale, err := e.staleTables()
+		if err != nil {
+			return nil, err
+		}
+		// ponytail: captures one stale table; an inet config replacing both ip and ip6 restores only the first.
+		if len(stale) > 0 {
+			return e.withFamily(addressFamily(stale[0].Family)).Snapshot()
+		}
+	}
+	snap := &Snapshot{Table: e.cfg.Settings.TableName, Family: addressFamily(e.family())}
+	if t == nil {
+		return snap, nil
 	}
 	snap.Present = true
 
-	chains, err := e.conn.ListChainsOfTableFamily(nftables.TableFamilyINet)
+	chains, err := e.conn.ListChainsOfTableFamily(e.family())
 	if err != nil {
 		return nil, fmt.Errorf("listing chains: %w", err)
 	}
@@ -288,7 +403,7 @@ func (e *Engine) Snapshot() (*Snapshot, error) {
 	if err != nil {
 		return nil, err
 	}
-	snap.Rules, err = encodeState(state)
+	snap.Rules, err = encodeState(state, byte(e.family()))
 	if err != nil {
 		return nil, err
 	}
@@ -302,10 +417,14 @@ func (e *Engine) Restore(s *Snapshot) error {
 	if s.Table != e.cfg.Settings.TableName {
 		return fmt.Errorf("snapshot is of table %q, engine manages %q", s.Table, e.cfg.Settings.TableName)
 	}
+	// Snapshots predating the family field are of the inet table.
+	if f := addressFamily(tableFamilies[s.Family]); f != addressFamily(e.family()) {
+		return e.withFamily(f).Restore(s)
+	}
 	if !s.Present {
 		return e.Flush()
 	}
-	want, err := decodeState(s.Rules)
+	want, err := decodeState(s.Rules, byte(e.family()))
 	if err != nil {
 		return err
 	}

internal/nftables/engine_test.go

@@ -0,0 +1,59 @@
+package nftables
+
+import (
+	"os"
+	"strconv"
+	"strings"
+	"testing"
+
+	"github.com/mdlayher/netlink"
+	"golang.org/x/sys/unix"
+)
+
+func TestLargeBuffersRaisesSocketBuffers(t *testing.T) {
+	c, err := netlink.Dial(unix.NETLINK_NETFILTER, nil)
+	if err != nil {
+		t.Skipf("netlink unavailable: %v", err)
+	}
+	defer c.Close()
+	if err := largeBuffers(c); err != nil {
+		t.Fatal(err)
+	}
+	// Without CAP_NET_ADMIN the kernel caps at the sysctl max; it doubles either way.
+	for opt, sysctl := range map[int]string{unix.SO_RCVBUF: "rmem_max", unix.SO_SNDBUF: "wmem_max"} {
+		want := 2 * min(batchBufSize, procInt(t, "/proc/sys/net/core/"+sysctl))
+		if got := sockBuf(t, c, opt); got < want {
+			t.Errorf("%s-bounded buffer = %d, want >= %d", sysctl, got, want)
+		}
+	}
+}
+
+func procInt(t *testing.T, path string) int {
+	t.Helper()
+	b, err := os.ReadFile(path)
+	if err != nil {
+		t.Skipf("reading %s: %v", path, err)
+	}
+	v, err := strconv.Atoi(strings.TrimSpace(string(b)))
+	if err != nil {
+		t.Fatal(err)
+	}
+	return v
+}
+
+func sockBuf(t *testing.T, c *netlink.Conn, opt int) int {
+	t.Helper()
+	rc, err := c.SyscallConn()
+	if err != nil {
+		t.Fatal(err)
+	}
+	var v int
+	var serr error
+	if err := rc.Control(func(fd uintptr) { v, serr = unix.GetsockoptInt(int(fd), unix.SOL_SOCKET, opt) }); err != nil {
+		t.Fatal(err)
+	}
+	if serr != nil {
+		t.Fatal(serr)
+	}
+	return v
+}

internal/nftables/family_test.go

@@ -0,0 +1,113 @@
+package nftables
+
+import (
+	"testing"
+
+	"github.com/google/nftables"
+	"github.com/google/nftables/expr"
+	"github.com/mdlayher/netlink"
+	"golang.org/x/sys/unix"
+
+	"git.unkin.net/unkin/tomswall/internal/config"
+)
+
+type sentTable struct {
+	msg    int
+	family nftables.TableFamily
+}
+
+// familyEngine fakes a kernel holding tomswall tables of the given families
+// and records table creations/deletions.
+func familyEngine(t *testing.T, af config.AddressFamily, live ...nftables.TableFamily) (*Engine, *[]sentTable) {
+	var sent []sentTable
+	e := testEngine(t, func(req []netlink.Message) ([]netlink.Message, error) {
+		var out []netlink.Message
+		for _, m := range req {
+			switch m.Header.Type {
+			case nftType(unix.NFT_MSG_GETTABLE):
+				for _, f := range live {
+					attrs, _ := netlink.MarshalAttributes([]netlink.Attribute{{Type: unix.NFTA_TABLE_NAME, Data: []byte("tomswall\x00")}})
+					out = append(out, netlink.Message{Header: netlink.Header{Type: nftType(unix.NFT_MSG_NEWTABLE), Sequence: m.Header.Sequence}, Data: append([]byte{byte(f), 0, 0, 0}, attrs...)})
+				}
+			case nftType(unix.NFT_MSG_NEWTABLE):
+				sent = append(sent, sentTable{unix.NFT_MSG_NEWTABLE, nftables.TableFamily(m.Data[0])})
+			case nftType(unix.NFT_MSG_DELTABLE):
+				sent = append(sent, sentTable{unix.NFT_MSG_DELTABLE, nftables.TableFamily(m.Data[0])})
+			}
+		}
+		return out, nil
+	})
+	e.cfg.Settings.AddressFamily = af
+	return e, &sent
+}
+
+func TestApplyIPFamilyReplacesInetTable(t *testing.T) {
+	e, sent := familyEngine(t, config.FamilyIP, nftables.TableFamilyINet, nftables.TableFamilyIPv6)
+	if err := e.Apply(&ChangeSet{}); err != nil {
+		t.Fatal(err)
+	}
+	want := []sentTable{{unix.NFT_MSG_DELTABLE, nftables.TableFamilyINet}, {unix.NFT_MSG_NEWTABLE, nftables.TableFamilyIPv4}}
+	if len(*sent) != len(want) || (*sent)[0] != want[0] || (*sent)[1] != want[1] {
+		t.Errorf("got %+v, want %+v (the ip6 table must survive)", *sent, want)
+	}
+}
+
+func TestApplyInetFamilyReplacesIPTables(t *testing.T) {
+	e, sent := familyEngine(t, config.FamilyINET, nftables.TableFamilyIPv4, nftables.TableFamilyIPv6)
+	if err := e.Apply(&ChangeSet{}); err != nil {
+		t.Fatal(err)
+	}
+	var dels int
+	for _, s := range *sent {
+		if s.msg == unix.NFT_MSG_DELTABLE {
+			dels++
+		}
+	}
+	if dels != 2 {
+		t.Errorf("want ip and ip6 tables deleted, got %+v", *sent)
+	}
+}
+
+func TestFlushIPFamilyKeepsIP6Table(t *testing.T) {
+	e, sent := familyEngine(t, config.FamilyIP, nftables.TableFamilyIPv6, nftables.TableFamilyIPv4)
+	if err := e.Flush(); err != nil {
+		t.Fatal(err)
+	}
+	if len(*sent) != 1 || (*sent)[0] != (sentTable{unix.NFT_MSG_DELTABLE, nftables.TableFamilyIPv4}) {
+		t.Errorf("got %+v, want only the ip table deleted", *sent)
+	}
+}
+
+func TestSnapshotFallsBackToReplacedTable(t *testing.T) {
+	e, _ := familyEngine(t, config.FamilyIP, nftables.TableFamilyINet)
+	snap, err := e.Snapshot()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !snap.Present || snap.Family != config.FamilyINET {
+		t.Fatalf("want present inet snapshot, got %+v", snap)
+	}
+
+	// Reverting to the inet snapshot drops the tried ip table.
+	e, sent := familyEngine(t, config.FamilyIP, nftables.TableFamilyIPv4)
+	if err := e.Restore(&Snapshot{Table: "tomswall", Family: config.FamilyINET, Present: true}); err != nil {
+		t.Fatal(err)
+	}
+	want := []sentTable{{unix.NFT_MSG_DELTABLE, nftables.TableFamilyIPv4}, {unix.NFT_MSG_NEWTABLE, nftables.TableFamilyINet}}
+	if len(*sent) != 2 || (*sent)[0] != want[0] || (*sent)[1] != want[1] {
+		t.Errorf("got %+v, want %+v", *sent, want)
+	}
+}
+
+func TestRejectExprsFamily(t *testing.T) {
+	for af, want := range map[config.AddressFamily]expr.Reject{
+		config.FamilyINET: {Type: unix.NFT_REJECT_ICMPX_UNREACH, Code: unix.NFT_REJECT_ICMPX_PORT_UNREACH},
+		config.FamilyIP:   {Type: unix.NFT_REJECT_ICMP_UNREACH, Code: 3},
+		config.FamilyIP6:  {Type: unix.NFT_REJECT_ICMP_UNREACH, Code: 4},
+	} {
+		got := rejectExprs(unix.IPPROTO_UDP, af)[0].(*expr.Reject)
+		if *got != want {
+			t.Errorf("%s: got %+v, want %+v", af, *got, want)
+		}
+	}
+}

internal/nftables/snapshot.go

@@ -4,14 +4,11 @@ import (
 	"encoding/binary"
 	"fmt"
 
-	"github.com/google/nftables"
 	"github.com/google/nftables/expr"
 	"github.com/mdlayher/netlink"
 	"golang.org/x/sys/unix"
 )
 
-const inet = byte(nftables.TableFamilyINet)
-
 // exprByName mirrors the expression types google/nftables can parse back from the kernel.
 var exprByName = map[string]func() expr.Any{
 	"ct":           func() expr.Any { return &expr.Ct{} },
@@ -40,13 +37,13 @@ var exprByName = map[string]func() expr.Any{
 	"notrack":      func() expr.Any { return &expr.Notrack{} },
 }
 
-func encodeState(state *FirewallState) (map[string][]SnapshotRule, error) {
+func encodeState(state *FirewallState, fam byte) (map[string][]SnapshotRule, error) {
 	out := make(map[string][]SnapshotRule, len(state.Rules))
 	for chain, rules := range state.Rules {
 		for _, r := range rules {
 			sr := SnapshotRule{Tag: r.Tag}
 			for _, e := range r.Exprs {
-				b, err := expr.Marshal(inet, e)
+				b, err := expr.Marshal(fam, e)
 				if err != nil {
 					return nil, fmt.Errorf("encoding %s rule %q: %w", chain, r.Tag, err)
 				}
@@ -58,13 +55,13 @@ func encodeState(state *FirewallState) (map[string][]SnapshotRule, error) {
 	return out, nil
 }
 
-func decodeState(rules map[string][]SnapshotRule) (*FirewallState, error) {
+func decodeState(rules map[string][]SnapshotRule, fam byte) (*FirewallState, error) {
 	state := &FirewallState{Rules: make(map[string][]ManagedRule, len(rules))}
 	for chain, rs := range rules {
 		for _, sr := range rs {
 			r := ManagedRule{Chain: chain, Tag: sr.Tag}
 			for _, b := range sr.Exprs {
-				e, err := decodeExpr(b)
+				e, err := decodeExpr(b, fam)
 				if err != nil {
 					return nil, fmt.Errorf("decoding %s rule %q: %w", chain, sr.Tag, err)
 				}
@@ -77,7 +74,7 @@ func decodeState(rules map[string][]SnapshotRule) (*FirewallState, error) {
 }
 
 // decodeExpr reverses expr.Marshal, as google/nftables does when reading rules.
-func decodeExpr(b []byte) (expr.Any, error) {
+func decodeExpr(b []byte, fam byte) (expr.Any, error) {
 	ad, err := netlink.NewAttributeDecoder(b)
 	if err != nil {
 		return nil, err
@@ -104,13 +101,13 @@ func decodeExpr(b []byte) (expr.Any, error) {
 	if name == "notrack" {
 		return e, nil
 	}
-	if err := expr.Unmarshal(inet, data, e); err != nil {
+	if err := expr.Unmarshal(fam, data, e); err != nil {
 		return nil, err
 	}
 	// A verdict is an immediate into the verdict register with no data.
 	if imm, ok := e.(*expr.Immediate); ok && imm.Register == unix.NFT_REG_VERDICT && len(imm.Data) == 0 {
 		v := &expr.Verdict{}
-		if err := expr.Unmarshal(inet, data, v); err != nil {
+		if err := expr.Unmarshal(fam, data, v); err != nil {
 			return nil, err
 		}
 		return v, nil

internal/nftables/snapshot_test.go

@@ -29,7 +29,7 @@ func TestSnapshotRulesRoundTrip(t *testing.T) {
 		"input": {{Chain: "input", Tag: "ssh", Exprs: exprs}, {Chain: "input", Tag: "drop", Exprs: []expr.Any{&expr.Verdict{Kind: expr.VerdictDrop}}}},
 	}}
 
-	rules, err := encodeState(state)
+	rules, err := encodeState(state, byte(nftables.TableFamilyINet))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -45,7 +45,7 @@ func TestSnapshotRulesRoundTrip(t *testing.T) {
 	if snap.Policies["input"] != nftables.ChainPolicyAccept {
 		t.Errorf("policy lost: %v", snap.Policies)
 	}
-	got, err := decodeState(snap.Rules)
+	got, err := decodeState(snap.Rules, byte(nftables.TableFamilyINet))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -79,7 +79,7 @@ func TestSnapshotAndRestoreAbsentTable(t *testing.T) {
 		for _, m := range req {
 			sent = append(sent, m.Header.Type)
 			if m.Header.Type == nftType(unix.NFT_MSG_GETTABLE) && tablePresent {
-				data := []byte{inet, 0, 0, 0}
+				data := []byte{byte(nftables.TableFamilyINet), 0, 0, 0}
 				attrs, _ := netlink.MarshalAttributes([]netlink.Attribute{{Type: unix.NFTA_TABLE_NAME, Data: []byte("tomswall\x00")}})
 				return []netlink.Message{{Header: netlink.Header{Type: nftType(unix.NFT_MSG_NEWTABLE), Sequence: m.Header.Sequence}, Data: append(data, attrs...)}}, nil
 			}
@@ -116,7 +116,7 @@ func TestRestorePresentTable(t *testing.T) {
 		{Tag: "ssh", Exprs: []expr.Any{&expr.Ct{Register: 1, Key: expr.CtKeySTATE}, &expr.Verdict{Kind: expr.VerdictAccept}}},
 		{Tag: "drop", Exprs: []expr.Any{&expr.Verdict{Kind: expr.VerdictDrop}}},
 	} {
-		enc, err := encodeState(&FirewallState{Rules: map[string][]ManagedRule{"input": {r}}})
+		enc, err := encodeState(&FirewallState{Rules: map[string][]ManagedRule{"input": {r}}}, byte(nftables.TableFamilyINet))
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -132,7 +132,7 @@ func TestRestorePresentTable(t *testing.T) {
 		if err != nil {
 			t.Fatal(err)
 		}
-		return append([]byte{inet, 0, 0, 0}, b...)
+		return append([]byte{byte(nftables.TableFamilyINet), 0, 0, 0}, b...)
 	}
 	handle := make([]byte, 8)
 	binary.BigEndian.PutUint64(handle, 7)

internal/shorewall/convert.go

@@ -2,6 +2,7 @@ package shorewall
 
 import (
 	"fmt"
+	"log/slog"
 	"strconv"
 	"strings"
 
@@ -99,6 +100,15 @@ func convertDir(dir string, ipv6 bool) (*config.Config, error) {
 	return cfg, nil
 }
 
+// disposition maps a shorewall *_DISPOSITION value; unset means CONTINUE and A_ (audit) variants map to their base action.
+func disposition(v string) config.PolicyAction {
+	v = strings.TrimPrefix(strings.ToLower(v), "a_")
+	if v == "" {
+		return config.PolicyContinue
+	}
+	return config.PolicyAction(v)
+}
+
 func subst(s string, params map[string]string) string {
 	if !strings.Contains(s, "$") {
 		return s
@@ -115,6 +125,8 @@ func convertConf(dir string, cfg *config.Config, params map[string]string, ipv6
 	if err != nil {
 		return err
 	}
+	cfg.Settings.InvalidDisposition = disposition(conf["INVALID_DISPOSITION"])
+	cfg.Settings.UntrackedDisposition = disposition(conf["UNTRACKED_DISPOSITION"])
 	if conf == nil {
 		return nil
 	}
@@ -130,13 +142,23 @@ func convertConf(dir string, cfg *config.Config, params map[string]string, ipv6
 	} else {
 		cfg.Settings.LogLevel = "info"
 	}
+	if v := conf["LOGLIMIT"]; v != "" {
+		if strings.HasPrefix(v, "s:") || strings.HasPrefix(v, "d:") {
+			slog.Warn("shorewall: per-address LOGLIMIT is not supported, limiting each log site globally", "loglimit", v)
+			v = v[2:]
+		}
+		if name, rest, ok := strings.Cut(v, ":"); ok && !strings.Contains(name, "/") {
+			slog.Warn("shorewall: named LOGLIMIT is not supported, dropping the name", "loglimit", conf["LOGLIMIT"])
+			v = rest
+		}
+		cfg.Settings.LogLimit = v
+	}
 	if v, ok := conf["IP_FORWARDING"]; ok {
 		cfg.Settings.IPForwarding = v == "Yes" || v == "On" || v == "on" || v == "Keep"
 	}
 	if v, ok := conf["IMPLICIT_CONTINUE"]; ok {
 		cfg.Settings.ImplicitContinue = v == "Yes"
 	}
-
 	return nil
 }
 

internal/shorewall/convert_test.go

@@ -725,3 +725,54 @@ func TestIsIPv6Dir(t *testing.T) {
 		}
 	})
 }
+
+func TestConvert_Dispositions(t *testing.T) {
+	cases := []struct {
+		conf               string
+		invalid, untracked config.PolicyAction
+	}{
+		{"", config.PolicyContinue, config.PolicyContinue},
+		{"IP_FORWARDING=Yes", config.PolicyContinue, config.PolicyContinue},
+		{"INVALID_DISPOSITION=CONTINUE\nUNTRACKED_DISPOSITION=ACCEPT", config.PolicyContinue, config.PolicyAccept},
+		{"INVALID_DISPOSITION=DROP\nUNTRACKED_DISPOSITION=A_DROP", config.PolicyDrop, config.PolicyDrop},
+		{"INVALID_DISPOSITION=A_REJECT", config.PolicyReject, config.PolicyContinue},
+	}
+	for _, tc := range cases {
+		dir := t.TempDir()
+		writeFile(t, dir, "shorewall.conf", tc.conf)
+		writeFile(t, dir, "zones", "fw firewall\nnet ipv4\n")
+		writeFile(t, dir, "interfaces", "net eth0 -\n")
+		writeFile(t, dir, "policy", "all all DROP\n")
+		cfg, err := Convert(dir)
+		if err != nil {
+			t.Fatalf("Convert(%q): %v", tc.conf, err)
+		}
+		if cfg.Settings.InvalidDisposition != tc.invalid || cfg.Settings.UntrackedDisposition != tc.untracked {
+			t.Errorf("%q: got invalid=%q untracked=%q, want %q/%q", tc.conf,
+				cfg.Settings.InvalidDisposition, cfg.Settings.UntrackedDisposition, tc.invalid, tc.untracked)
+		}
+		if err := cfg.Validate(); err != nil {
+			t.Errorf("%q: Validate: %v", tc.conf, err)
+		}
+	}
+}
+
+func TestConvert_LogLimit(t *testing.T) {
+	for in, want := range map[string]string{
+		`LOGLIMIT="s:1/sec:10"`:   "1/sec:10",
+		`LOGLIMIT=2/min`:          "2/min",
+		`LOGLIMIT=name:1/sec:5`:   "1/sec:5",
+		`LOGLIMIT=s:name:1/sec:5`: "1/sec:5",
+		`LOGLIMIT=`:               "",
+	} {
+		dir := minimalShorewallDir(t)
+		writeFile(t, dir, "shorewall.conf", "LOG_LEVEL=info\n"+in+"\n")
+		cfg, err := Convert(dir)
+		if err != nil {
+			t.Fatalf("%s: %v", in, err)
+		}
+		if cfg.Settings.LogLimit != want {
+			t.Errorf("%s: log_limit = %q, want %q", in, cfg.Settings.LogLimit, want)
+		}
+	}
+}

internal/tryapply/tryapply.go

@@ -25,15 +25,15 @@ const Unit = "tomswall-try-revert"
 var (
 	// Dir holds the lock and the pending snapshot.
 	Dir = "/var/lib/tomswall"
-	// run executes a systemd command; replaced in tests.
+	// Run executes a systemd command. Test hook; production code must not reassign.
-	run = func(name string, args ...string) error {
+	Run = func(name string, args ...string) error {
 		if out, err := exec.Command(name, args...).CombinedOutput(); err != nil {
 			return fmt.Errorf("%s %s: %w: %s", name, strings.Join(args, " "), err, strings.TrimSpace(string(out)))
 		}
 		return nil
 	}
-	// restore rolls the live table back to a snapshot; replaced in tests.
+	// Restore rolls the live table back to a snapshot. Test hook; production code must not reassign.
-	restore = func(s *nftables.Snapshot) error {
+	Restore = func(s *nftables.Snapshot) error {
 		engine, err := nftables.NewEngine(&config.Config{Settings: config.Settings{TableName: s.Table}})
 		if err != nil {
 			return err
@@ -94,23 +94,7 @@ func Arm(snap *nftables.Snapshot, pid int, delay time.Duration) (string, error)
 	if err != nil {
 		return "", err
 	}
-	f, err := os.CreateTemp(Dir, ".try-snapshot-*")
+	if err := WriteFile(snapshotPath(), b); err != nil {
-	if err != nil {
-		return "", err
-	}
-	defer os.Remove(f.Name())
-	if _, err := f.Write(b); err != nil {
-		f.Close()
-		return "", err
-	}
-	if err := f.Sync(); err != nil {
-		f.Close()
-		return "", err
-	}
-	if err := f.Close(); err != nil {
-		return "", err
-	}
-	if err := os.Rename(f.Name(), snapshotPath()); err != nil {
 		return "", err
 	}
 
@@ -119,13 +103,46 @@ func Arm(snap *nftables.Snapshot, pid int, delay time.Duration) (string, error)
 		return "", discardWith(err)
 	}
 	_ = disarm() // a leftover timer from an earlier try would block the unit name
-	if err := run("systemd-run", "--quiet", "--collect", "--unit", Unit,
+	if err := Run("systemd-run", "--quiet", "--collect", "--unit", Unit,
 		fmt.Sprintf("--on-active=%ds", int(delay.Round(time.Second).Seconds())), exe, "revert", "--id", id); err != nil {
 		return "", discardWith(fmt.Errorf("arming revert timer: %w", err))
 	}
 	return id, nil
 }
 
+// WriteFile durably replaces path with b: temp file, fsync, rename, fsync the directory.
+func WriteFile(path string, b []byte) error {
+	dir := filepath.Dir(path)
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		return err
+	}
+	f, err := os.CreateTemp(dir, "."+filepath.Base(path)+"-*")
+	if err != nil {
+		return err
+	}
+	defer os.Remove(f.Name())
+	if _, err := f.Write(b); err != nil {
+		f.Close()
+		return err
+	}
+	if err := f.Sync(); err != nil {
+		f.Close()
+		return err
+	}
+	if err := f.Close(); err != nil {
+		return err
+	}
+	if err := os.Rename(f.Name(), path); err != nil {
+		return err
+	}
+	d, err := os.Open(dir)
+	if err != nil {
+		return err
+	}
+	defer d.Close()
+	return d.Sync()
+}
+
 // Discard drops the pending snapshot and timer without restoring. The caller must hold the lock.
 func Discard() error {
 	_ = disarm()
@@ -143,7 +160,7 @@ func discardWith(err error) error {
 }
 
 func disarm() error {
-	return run("systemctl", "stop", Unit+".timer")
+	return Run("systemctl", "stop", Unit+".timer")
 }
 
 // Confirm keeps the tried ruleset. ok is false when no try was pending, i.e.
@@ -175,10 +192,28 @@ func Revert(id string) (reverted bool, err error) {
 	if err != nil || p == nil || (id != "" && p.ID != id) {
 		return false, err
 	}
-	if err := restore(p.Snapshot); err != nil {
+	return true, restorePending(p)
-		return false, fmt.Errorf("restoring snapshot: %w", err)
+}
+
+// Abort restores the pending snapshot after a failed apply, which may have
+// committed partially. A failed restore keeps the snapshot and timer so the
+// timer still reverts. The caller must hold the lock.
+func Abort() error {
+	p, err := load()
+	if err != nil {
+		return err
 	}
-	return true, Discard()
+	if p == nil {
+		return errors.New("no pending try to abort")
+	}
+	return restorePending(p)
+}
+
+func restorePending(p *pending) error {
+	if err := Restore(p.Snapshot); err != nil {
+		return fmt.Errorf("restoring snapshot: %w", err)
+	}
+	return Discard()
 }
 
 func load() (*pending, error) {

internal/tryapply/tryapply_test.go

@@ -15,12 +15,12 @@ func setup(t *testing.T) *[]string {
 	t.Helper()
 	Dir = t.TempDir()
 	var cmds []string
-	orig := run
+	orig := Run
-	run = func(name string, args ...string) error {
+	Run = func(name string, args ...string) error {
 		cmds = append(cmds, name+" "+strings.Join(args, " "))
 		return nil
 	}
-	t.Cleanup(func() { run = orig })
+	t.Cleanup(func() { Run = orig })
 	return &cmds
 }
 
@@ -91,7 +91,7 @@ func TestAcquireRefusesWhilePending(t *testing.T) {
 
 func TestArmFailureDiscardsSnapshot(t *testing.T) {
 	setup(t)
-	run = func(name string, args ...string) error {
+	Run = func(name string, args ...string) error {
 		if name == "systemd-run" {
 			return errors.New("no systemd")
 		}
@@ -145,12 +145,12 @@ func TestConfirmAfterRevertFails(t *testing.T) {
 func stubRestore(t *testing.T, err error) *[]*nftables.Snapshot {
 	t.Helper()
 	var got []*nftables.Snapshot
-	orig := restore
+	orig := Restore
-	restore = func(s *nftables.Snapshot) error {
+	Restore = func(s *nftables.Snapshot) error {
 		got = append(got, s)
 		return err
 	}
-	t.Cleanup(func() { restore = orig })
+	t.Cleanup(func() { Restore = orig })
 	return &got
 }
 
@@ -209,3 +209,41 @@ func TestRevertStaleIDIgnored(t *testing.T) {
 		t.Errorf("newer try's snapshot removed: %v", err)
 	}
 }
+
+func TestAbortRestoresAndDisarms(t *testing.T) {
+	cmds := setup(t)
+	restored := stubRestore(t, nil)
+	snap := &nftables.Snapshot{Table: "tomswall", Present: true}
+	arm(t, snap)
+
+	if err := Abort(); err != nil {
+		t.Fatal(err)
+	}
+	if len(*restored) != 1 || !reflect.DeepEqual((*restored)[0], snap) {
+		t.Errorf("restored %+v, want the armed snapshot", *restored)
+	}
+	if _, err := os.Stat(snapshotPath()); !os.IsNotExist(err) {
+		t.Error("snapshot not removed")
+	}
+	if last := (*cmds)[len(*cmds)-1]; last != "systemctl stop "+Unit+".timer" {
+		t.Errorf("timer not stopped, last command %q", last)
+	}
+}
+
+func TestAbortFailureKeepsSnapshotAndTimer(t *testing.T) {
+	cmds := setup(t)
+	boom := errors.New("netlink down")
+	stubRestore(t, boom)
+	arm(t, &nftables.Snapshot{Table: "tomswall"})
+	armed := len(*cmds)
+
+	if err := Abort(); !errors.Is(err, boom) {
+		t.Fatalf("Abort error = %v, want %v", err, boom)
+	}
+	if _, err := os.Stat(snapshotPath()); err != nil {
+		t.Fatalf("snapshot gone after failed abort: %v", err)
+	}
+	if len(*cmds) != armed {
+		t.Errorf("revert timer touched after failed abort: %v", (*cmds)[armed:])
+	}
+}

packaging/nfpm.yaml

@@ -47,6 +47,17 @@ contents:
     file_info:
       mode: 0640
 
+  # systemd unit + environment file for applying a local config at boot.
+  - src: packaging/tomswall.service
+    dst: /usr/lib/systemd/system/tomswall.service
+    file_info:
+      mode: 0644
+  - src: packaging/tomswall.env
+    dst: /etc/tomswall/tomswall.env
+    type: config|noreplace
+    file_info:
+      mode: 0644
+
   # Shell completions (generated by scripts/build-rpm.sh before packaging).
   - src: dist/completions/tomswall.bash
     dst: /usr/share/bash-completion/completions/tomswall

packaging/tomswall-agent.service

@@ -3,6 +3,7 @@ Description=tomswall control-plane agent (pull and apply firewall config)
 Documentation=https://git.unkin.net/unkin/tomswall
 After=network-online.target
 Wants=network-online.target
+Conflicts=tomswall.service
 
 [Service]
 Type=simple

packaging/tomswall.env

@@ -0,0 +1,3 @@
+# Config applied by tomswall.service: a tomswall YAML file or a shorewall directory.
+TOMSWALL_CONFIG=/etc/tomswall/tomswall.yaml
+#TOMSWALL_CONFIG=/etc/shorewall

packaging/tomswall.service

@@ -0,0 +1,25 @@
+[Unit]
+Description=tomswall firewall (apply local config at boot)
+Documentation=https://git.unkin.net/unkin/tomswall
+DefaultDependencies=no
+Wants=network-pre.target
+Before=network-pre.target shutdown.target
+After=local-fs.target systemd-sysctl.service
+Conflicts=shutdown.target tomswall-agent.service
+StartLimitIntervalSec=60
+StartLimitBurst=5
+
+[Service]
+Type=oneshot
+RemainAfterExit=yes
+Environment=TOMSWALL_CONFIG=/etc/tomswall/tomswall.yaml
+EnvironmentFile=-/etc/tomswall/tomswall.env
+ExecStart=/usr/sbin/tomswall apply -c ${TOMSWALL_CONFIG}
+ExecReload=/usr/sbin/tomswall apply -c ${TOMSWALL_CONFIG}
+# Fails open: after StartLimitBurst failures within StartLimitIntervalSec, boot continues without the ruleset.
+Restart=on-failure
+RestartSec=5
+# No ExecStop: stopping the unit leaves the ruleset in place (flush would open the firewall).
+
+[Install]
+WantedBy=sysinit.target

tomswall.example.yaml

@@ -6,8 +6,14 @@ settings:
   address_family: inet
   ip_forwarding: true
   log_level: info
+  # rate limit for every log site (shorewall LOGLIMIT, global form): rate/{sec|min|hour|day}[:burst]; unset logs every hit
+  log_limit: 1/sec:10
   table_name: tomswall
   implicit_continue: false
+  # ct state invalid/untracked verdict: accept, drop, reject, continue (pass to rules)
+  # defaults: invalid drop, untracked continue (migrate defaults both to continue, as shorewall)
+  invalid_disposition: drop
+  untracked_disposition: continue
 
 # Named port groups — reusable port+protocol combos referenced in rules
 portgroups: