Merge pull request 'fix: stop differential apply rewriting unchanged rules' (#18) from benvin/diff-expr-equality into main

Reviewed-on: #18

.woodpecker/build.yaml

@@ -3,9 +3,21 @@ when:
 
 steps:
   - name: build
-    image: golang:1.23
+    image: artifactapi.k8s.syd1.au.unkin.net/docker-internal/gobuilder:0.1.2-alma9
     commands:
       - go build ./...
+    environment:
+      GOCACHE_S3_BUCKET: gocache
+      GOCACHE_S3_REGION: us-east-1
+      GOCACHE_S3_ENDPOINT_URL: https://s3.ceph.unkin.net
+      GOCACHE_S3_PATH_STYLE: "true"
+      GOCACHE_KEY_PREFIX: ci-tomswall
+      GOCACHE_METRICS: "true"
+      GOCACHEPROG: go-cache-plugin --cache-dir=/tmp/gocache
+      AWS_ACCESS_KEY_ID:
+        from_secret: GOCACHE_AWS_ACCESS_KEY_ID
+      AWS_SECRET_ACCESS_KEY:
+        from_secret: GOCACHE_AWS_SECRET_ACCESS_KEY
     backend_options:
       kubernetes:
         serviceAccountName: default

.woodpecker/pre-commit.yaml

@@ -3,9 +3,21 @@ when:
 
 steps:
   - name: pre-commit
-    image: git.unkin.net/unkin/almalinux9-gobuilder:20260606
+    image: artifactapi.k8s.syd1.au.unkin.net/docker-internal/gobuilder:0.1.2-alma9
     commands:
       - uvx pre-commit run --all-files
+    environment:
+      GOCACHE_S3_BUCKET: gocache
+      GOCACHE_S3_REGION: us-east-1
+      GOCACHE_S3_ENDPOINT_URL: https://s3.ceph.unkin.net
+      GOCACHE_S3_PATH_STYLE: "true"
+      GOCACHE_KEY_PREFIX: ci-tomswall
+      GOCACHE_METRICS: "true"
+      GOCACHEPROG: go-cache-plugin --cache-dir=/tmp/gocache
+      AWS_ACCESS_KEY_ID:
+        from_secret: GOCACHE_AWS_ACCESS_KEY_ID
+      AWS_SECRET_ACCESS_KEY:
+        from_secret: GOCACHE_AWS_SECRET_ACCESS_KEY
     backend_options:
       kubernetes:
         serviceAccountName: default

.woodpecker/release.yaml

@@ -4,9 +4,21 @@ when:
 
 steps:
   - name: test
-    image: golang:1.23
+    image: artifactapi.k8s.syd1.au.unkin.net/docker-internal/gobuilder:0.1.2-alma9
     commands:
       - go test ./...
+    environment:
+      GOCACHE_S3_BUCKET: gocache
+      GOCACHE_S3_REGION: us-east-1
+      GOCACHE_S3_ENDPOINT_URL: https://s3.ceph.unkin.net
+      GOCACHE_S3_PATH_STYLE: "true"
+      GOCACHE_KEY_PREFIX: ci-tomswall
+      GOCACHE_METRICS: "true"
+      GOCACHEPROG: go-cache-plugin --cache-dir=/tmp/gocache
+      AWS_ACCESS_KEY_ID:
+        from_secret: GOCACHE_AWS_ACCESS_KEY_ID
+      AWS_SECRET_ACCESS_KEY:
+        from_secret: GOCACHE_AWS_SECRET_ACCESS_KEY
     backend_options:
       kubernetes:
         serviceAccountName: default
@@ -19,10 +31,22 @@ steps:
             cpu: 2
 
   - name: build
-    image: git.unkin.net/unkin/almalinux9-gobuilder:20260606
+    image: artifactapi.k8s.syd1.au.unkin.net/docker-internal/gobuilder:0.1.2-alma9
     commands:
       - make dist-build VERSION=${CI_COMMIT_TAG}
     depends_on: [test]
+    environment:
+      GOCACHE_S3_BUCKET: gocache
+      GOCACHE_S3_REGION: us-east-1
+      GOCACHE_S3_ENDPOINT_URL: https://s3.ceph.unkin.net
+      GOCACHE_S3_PATH_STYLE: "true"
+      GOCACHE_KEY_PREFIX: ci-tomswall
+      GOCACHE_METRICS: "true"
+      GOCACHEPROG: go-cache-plugin --cache-dir=/tmp/gocache
+      AWS_ACCESS_KEY_ID:
+        from_secret: GOCACHE_AWS_ACCESS_KEY_ID
+      AWS_SECRET_ACCESS_KEY:
+        from_secret: GOCACHE_AWS_SECRET_ACCESS_KEY
     backend_options:
       kubernetes:
         serviceAccountName: default

.woodpecker/test.yaml

@@ -3,10 +3,22 @@ when:
 
 steps:
   - name: test
-    image: golang:1.23
+    image: artifactapi.k8s.syd1.au.unkin.net/docker-internal/gobuilder:0.1.2-alma9
     commands:
       - go vet ./...
       - go test ./...
+    environment:
+      GOCACHE_S3_BUCKET: gocache
+      GOCACHE_S3_REGION: us-east-1
+      GOCACHE_S3_ENDPOINT_URL: https://s3.ceph.unkin.net
+      GOCACHE_S3_PATH_STYLE: "true"
+      GOCACHE_KEY_PREFIX: ci-tomswall
+      GOCACHE_METRICS: "true"
+      GOCACHEPROG: go-cache-plugin --cache-dir=/tmp/gocache
+      AWS_ACCESS_KEY_ID:
+        from_secret: GOCACHE_AWS_ACCESS_KEY_ID
+      AWS_SECRET_ACCESS_KEY:
+        from_secret: GOCACHE_AWS_SECRET_ACCESS_KEY
     backend_options:
       kubernetes:
         serviceAccountName: default

cmd/tomswall/guard_test.go

@@ -0,0 +1,30 @@
+package main
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/spf13/cobra"
+
+	"git.unkin.net/unkin/tomswall/internal/tryapply"
+)
+
+func TestMutatingCommandsRefuseWhileTryPending(t *testing.T) {
+	orig := tryapply.Dir
+	tryapply.Dir = t.TempDir()
+	t.Cleanup(func() { tryapply.Dir = orig })
+	if err := os.WriteFile(filepath.Join(tryapply.Dir, "try-snapshot.json"), []byte("{}"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	configPath = "../../tomswall.example.yaml"
+
+	for _, cmd := range []*cobra.Command{applyCmd(), flushCmd(), purgeCmd()} {
+		t.Run(cmd.Use, func(t *testing.T) {
+			if err := cmd.RunE(cmd, nil); !errors.Is(err, tryapply.ErrPending) {
+				t.Errorf("got %v, want ErrPending", err)
+			}
+		})
+	}
+}

cmd/tomswall/main.go

@@ -12,6 +12,7 @@ import (
 	"git.unkin.net/unkin/tomswall/internal/config"
 	"git.unkin.net/unkin/tomswall/internal/nftables"
 	"git.unkin.net/unkin/tomswall/internal/shorewall"
+	"git.unkin.net/unkin/tomswall/internal/tryapply"
 )
 
 var configPath string
@@ -33,6 +34,9 @@ Use 'tomswall migrate' to convert a shorewall config to YAML.`,
 
 	root.AddCommand(
 		applyCmd(),
+		tryCmd(),
+		confirmCmd(),
+		revertCmd(),
 		planCmd(),
 		validateCmd(),
 		statusCmd(),
@@ -86,6 +90,12 @@ The firewall is never torn down — existing connections are preserved.`,
 				return err
 			}
 
+			unlock, err := tryapply.Acquire()
+			if err != nil {
+				return err
+			}
+			defer unlock()
+
 			engine, err := nftables.NewEngine(cfg)
 			if err != nil {
 				return fmt.Errorf("initializing nftables: %w", err)
@@ -227,6 +237,14 @@ func purgeCmd() *cobra.Command {
 				return err
 			}
 
+			if !dryRun {
+				unlock, err := tryapply.Acquire()
+				if err != nil {
+					return err
+				}
+				defer unlock()
+			}
+
 			engine, err := nftables.NewEngine(cfg)
 			if err != nil {
 				return fmt.Errorf("initializing nftables: %w", err)
@@ -272,6 +290,12 @@ func flushCmd() *cobra.Command {
 				return err
 			}
 
+			unlock, err := tryapply.Acquire()
+			if err != nil {
+				return err
+			}
+			defer unlock()
+
 			engine, err := nftables.NewEngine(cfg)
 			if err != nil {
 				return fmt.Errorf("initializing nftables: %w", err)

cmd/tomswall/try.go

@@ -0,0 +1,168 @@
+package main
+
+import (
+	"fmt"
+	"os"
+	"os/signal"
+	"strings"
+	"syscall"
+	"time"
+
+	"github.com/spf13/cobra"
+
+	"git.unkin.net/unkin/tomswall/internal/config"
+	"git.unkin.net/unkin/tomswall/internal/nftables"
+	"git.unkin.net/unkin/tomswall/internal/tryapply"
+)
+
+// revertGrace lets the in-process revert win before the systemd fallback fires.
+const revertGrace = 30 * time.Second
+
+func tryCmd() *cobra.Command {
+	var timeout time.Duration
+	cmd := &cobra.Command{
+		Use:   "try",
+		Short: "Apply configuration and revert unless confirmed within a timeout",
+		Long: `Try snapshots the live tomswall table to disk, applies the configuration, then
+waits for 'tomswall confirm'. On timeout, interrupt or hangup the snapshot is
+restored atomically. A transient systemd timer restores it too if this process
+dies. Confirm from a new session to prove new connections still work.`,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			cfg, err := loadConfig()
+			if err != nil {
+				return err
+			}
+
+			// Registered before apply so a confirm or hangup cannot be missed.
+			signal.Ignore(syscall.SIGPIPE)
+			confirm := make(chan os.Signal, 1)
+			signal.Notify(confirm, syscall.SIGUSR1)
+			abort := make(chan os.Signal, 1)
+			signal.Notify(abort, syscall.SIGHUP, syscall.SIGINT, syscall.SIGTERM)
+
+			id, err := tryApply(cfg, timeout+revertGrace)
+			if err != nil || id == "" {
+				return err
+			}
+			fmt.Printf("Applied. Run 'tomswall confirm' within %s or the previous ruleset is restored.\n", timeout)
+			msg, err := confirmOrRevert(confirm, abort, timeout, func() (bool, error) { return tryapply.Revert(id) })
+			if err != nil {
+				return err
+			}
+			fmt.Println(msg)
+			return nil
+		},
+	}
+	cmd.Flags().DurationVar(&timeout, "timeout", 60*time.Second, "time to wait for confirmation before reverting")
+	return cmd
+}
+
+// tryApply applies cfg under a pending try and returns its ID, or "" when
+// there was nothing to change.
+func tryApply(cfg *config.Config, fallback time.Duration) (string, error) {
+	unlock, err := tryapply.Acquire()
+	if err != nil {
+		return "", err
+	}
+	defer unlock()
+
+	engine, err := nftables.NewEngine(cfg)
+	if err != nil {
+		return "", fmt.Errorf("initializing nftables: %w", err)
+	}
+	changes, err := engine.Plan()
+	if err != nil {
+		return "", fmt.Errorf("computing changes: %w", err)
+	}
+	if changes.Empty() {
+		fmt.Println("No changes needed — firewall is up to date.")
+		return "", nil
+	}
+	fmt.Println(changes.Summary())
+
+	snap, err := engine.Snapshot()
+	if err != nil {
+		return "", fmt.Errorf("snapshotting ruleset: %w", err)
+	}
+	id, err := tryapply.Arm(snap, os.Getpid(), fallback)
+	if err != nil {
+		return "", err
+	}
+	if err := engine.Apply(changes); err != nil {
+		if derr := tryapply.Discard(); derr != nil {
+			err = fmt.Errorf("%w (discarding snapshot: %v)", err, derr)
+		}
+		return "", fmt.Errorf("applying changes: %w", err)
+	}
+	return id, nil
+}
+
+// confirmOrRevert waits for confirm; on abort or timeout it runs revert.
+// A confirm already delivered wins over a simultaneous abort or timeout.
+func confirmOrRevert(confirm, abort <-chan os.Signal, timeout time.Duration, revert func() (bool, error)) (string, error) {
+	reason := "not confirmed within " + timeout.String()
+	select {
+	case <-confirm:
+		return "Confirmed.", nil
+	case s := <-abort:
+		reason = "interrupted by " + s.String()
+	case <-time.After(timeout):
+	}
+	select {
+	case <-confirm:
+		return "Confirmed.", nil
+	default:
+	}
+	reverted, err := revert()
+	if err != nil {
+		return "", fmt.Errorf("%s; revert failed: %w", reason, err)
+	}
+	if !reverted {
+		return "Already resolved by 'tomswall confirm' or 'tomswall revert'.", nil
+	}
+	return "", fmt.Errorf("%s: previous ruleset restored", reason)
+}
+
+func confirmCmd() *cobra.Command {
+	return &cobra.Command{
+		Use:   "confirm",
+		Short: "Keep the configuration applied by a pending 'tomswall try'",
+		RunE: func(cmd *cobra.Command, args []string) error {
+			pid, ok, err := tryapply.Confirm()
+			if err != nil {
+				return err
+			}
+			if !ok {
+				return fmt.Errorf("no pending 'tomswall try': it was already reverted or confirmed")
+			}
+			// A recycled PID must not be signalled.
+			if comm, err := os.ReadFile(fmt.Sprintf("/proc/%d/comm", pid)); err == nil && strings.TrimSpace(string(comm)) == "tomswall" {
+				_ = syscall.Kill(pid, syscall.SIGUSR1)
+			}
+			fmt.Println("Confirmed.")
+			return nil
+		},
+	}
+}
+
+func revertCmd() *cobra.Command {
+	var id string
+	cmd := &cobra.Command{
+		Use:   "revert",
+		Short: "Restore the ruleset saved by a pending 'tomswall try'",
+		RunE: func(cmd *cobra.Command, args []string) error {
+			reverted, err := tryapply.Revert(id)
+			if err != nil {
+				return err
+			}
+			if !reverted {
+				fmt.Println("No pending 'tomswall try'.")
+				return nil
+			}
+			fmt.Println("Previous ruleset restored.")
+			return nil
+		},
+	}
+	cmd.Flags().StringVar(&id, "id", "", "only revert the try with this ID (used by the revert timer)")
+	return cmd
+}

cmd/tomswall/try_test.go

@@ -0,0 +1,63 @@
+package main
+
+import (
+	"errors"
+	"os"
+	"syscall"
+	"testing"
+	"time"
+)
+
+func TestConfirmOrRevert(t *testing.T) {
+	tests := []struct {
+		name         string
+		confirm      bool
+		abort        bool
+		resolved     bool
+		revertErr    error
+		wantRevert   bool
+		wantErr      bool
+		wantResolved bool
+	}{
+		{name: "confirmed keeps ruleset", confirm: true},
+		{name: "confirm wins over simultaneous abort", confirm: true, abort: true},
+		{name: "timeout reverts", wantRevert: true, wantErr: true},
+		{name: "hangup reverts", abort: true, wantRevert: true, wantErr: true},
+		{name: "revert failure surfaces", revertErr: errors.New("boom"), wantRevert: true, wantErr: true},
+		{name: "resolved elsewhere", resolved: true, wantRevert: true, wantResolved: true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			for i := 0; i < 20; i++ { // select between ready channels is random
+				confirm := make(chan os.Signal, 1)
+				abort := make(chan os.Signal, 1)
+				if tt.confirm {
+					confirm <- syscall.SIGUSR1
+				}
+				if tt.abort {
+					abort <- syscall.SIGHUP
+				}
+				called := false
+				msg, err := confirmOrRevert(confirm, abort, 10*time.Millisecond, func() (bool, error) {
+					called = true
+					return !tt.resolved, tt.revertErr
+				})
+				if called != tt.wantRevert {
+					t.Fatalf("revert called = %v, want %v", called, tt.wantRevert)
+				}
+				if (err != nil) != tt.wantErr {
+					t.Fatalf("err = %v, wantErr %v", err, tt.wantErr)
+				}
+				if tt.revertErr != nil && !errors.Is(err, tt.revertErr) {
+					t.Fatalf("revert error not wrapped: %v", err)
+				}
+				if tt.wantResolved && msg == "" {
+					t.Fatal("expected already-resolved message")
+				}
+				if !(tt.confirm && tt.abort) {
+					break
+				}
+			}
+		})
+	}
+}

go.mod

@@ -4,6 +4,7 @@ go 1.23
 
 require (
 	github.com/google/nftables v0.2.0
+	github.com/mdlayher/netlink v1.7.2
 	github.com/spf13/cobra v1.8.1
 	golang.org/x/sys v0.18.0
 	gopkg.in/yaml.v3 v3.0.1
@@ -13,7 +14,6 @@ require (
 	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
 	github.com/josharian/native v1.1.0 // indirect
-	github.com/mdlayher/netlink v1.7.2 // indirect
 	github.com/mdlayher/socket v0.5.1 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
 	golang.org/x/net v0.23.0 // indirect

internal/agent/agent.go

@@ -8,6 +8,7 @@ import (
 
 	"git.unkin.net/unkin/tomswall/internal/config"
 	"git.unkin.net/unkin/tomswall/internal/nftables"
+	"git.unkin.net/unkin/tomswall/internal/tryapply"
 )
 
 // Applier applies a translated config to the firewall. Abstracted so the run
@@ -103,8 +104,14 @@ func (a *Agent) applyConfig(ctx context.Context, rc *RenderedConfig, report bool
 // EngineApplier applies via the real nftables differential engine.
 type EngineApplier struct{}
 
-// Apply computes and applies the differential change set for cfg.
+// Apply computes and applies the differential change set for cfg. It refuses
+// while a 'tomswall try' awaits confirmation.
 func (EngineApplier) Apply(_ context.Context, cfg *config.Config) error {
+	unlock, err := tryapply.Acquire()
+	if err != nil {
+		return err
+	}
+	defer unlock()
 	engine, err := nftables.NewEngine(cfg)
 	if err != nil {
 		return fmt.Errorf("initializing nftables: %w", err)

internal/config/blrules.go

@@ -57,17 +57,25 @@ func (c *Config) validateBlrules() error {
 
 		if r.Source != "all" && r.Source != "any" && r.Source != "none" &&
 			!hasPrefix(r.Source, "all!") && !hasPrefix(r.Source, "any!") {
-			srcZone := zoneFromSpec(r.Source)
-			if _, ok := c.Zones[srcZone]; !ok {
-				return fmt.Errorf("blrules[%d]: source zone %q not defined", i, srcZone)
+			for _, zs := range SplitZoneList(r.Source) {
+				if _, ok := c.Zones[zs.Zone]; !ok {
+					return fmt.Errorf("blrules[%d]: source zone %q not defined", i, zs.Zone)
+				}
+				if !validAddrList(zs.Addr) {
+					return fmt.Errorf("blrules[%d]: source %q: '!' may only prefix the whole address list", i, zs.Addr)
+				}
 			}
 		}
 
 		if r.Dest != "all" && r.Dest != "any" && r.Dest != "none" &&
 			!hasPrefix(r.Dest, "all!") && !hasPrefix(r.Dest, "any!") {
-			dstZone := zoneFromSpec(r.Dest)
-			if _, ok := c.Zones[dstZone]; !ok {
-				return fmt.Errorf("blrules[%d]: dest zone %q not defined", i, dstZone)
+			for _, zs := range SplitZoneList(r.Dest) {
+				if _, ok := c.Zones[zs.Zone]; !ok {
+					return fmt.Errorf("blrules[%d]: dest zone %q not defined", i, zs.Zone)
+				}
+				if !validAddrList(zs.Addr) {
+					return fmt.Errorf("blrules[%d]: dest %q: '!' may only prefix the whole address list", i, zs.Addr)
+				}
 			}
 		}
 	}

internal/config/config_test.go

@@ -3,6 +3,7 @@ package config
 import (
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
 	"testing"
 )
@@ -854,6 +855,32 @@ func TestValidateRules(t *testing.T) {
 			},
 			wantErr: "source zone \"missing\" not defined",
 		},
+		{
+			name: "comma zone lists are valid",
+			rules: []Rule{
+				{Action: RuleAccept, Source: "fw,loc", Dest: "loc,net:192.0.2.1,198.51.100.1"},
+			},
+		},
+		{
+			name: "undefined zone in dest list",
+			rules: []Rule{
+				{Action: RuleAccept, Source: "loc", Dest: "net,missing"},
+			},
+			wantErr: "dest zone \"missing\" not defined",
+		},
+		{
+			name: "negation prefixing the whole address list is valid",
+			rules: []Rule{
+				{Action: RuleAccept, Source: "net:!192.0.2.1,198.51.100.1", Dest: "fw"},
+			},
+		},
+		{
+			name: "negation inside an address list",
+			rules: []Rule{
+				{Action: RuleAccept, Source: "net", Dest: "loc:192.0.2.1,!198.51.100.1"},
+			},
+			wantErr: "'!' may only prefix the whole address list",
+		},
 		{
 			name: "all keyword is valid source",
 			rules: []Rule{
@@ -1006,3 +1033,22 @@ func TestValidateSNAT(t *testing.T) {
 		})
 	}
 }
+
+func TestSplitZoneList(t *testing.T) {
+	tests := []struct {
+		in   string
+		want []ZoneSpec
+	}{
+		{"net", []ZoneSpec{{Zone: "net"}}},
+		{"fw,lan,svr", []ZoneSpec{{Zone: "fw"}, {Zone: "lan"}, {Zone: "svr"}}},
+		{"svr:192.0.2.17", []ZoneSpec{{Zone: "svr", Addr: "192.0.2.17"}}},
+		{"net:192.0.2.1,198.51.100.1", []ZoneSpec{{Zone: "net", Addr: "192.0.2.1,198.51.100.1"}}},
+		{"lan,svr:192.0.2.17", []ZoneSpec{{Zone: "lan"}, {Zone: "svr", Addr: "192.0.2.17"}}},
+		{"net:2001:db8::1", []ZoneSpec{{Zone: "net", Addr: "2001:db8::1"}}},
+	}
+	for _, tt := range tests {
+		if got := SplitZoneList(tt.in); !reflect.DeepEqual(got, tt.want) {
+			t.Errorf("SplitZoneList(%q) = %+v, want %+v", tt.in, got, tt.want)
+		}
+	}
+}

internal/config/rules.go

@@ -1,6 +1,9 @@
 package config
 
-import "fmt"
+import (
+	"fmt"
+	"strings"
+)
 
 type RuleAction string
 
@@ -172,10 +175,12 @@ func (c *Config) validateRules() error {
 
 		if r.Source != "all" && r.Source != "any" && r.Source != "none" &&
 			!hasPrefix(r.Source, "all+") && !hasPrefix(r.Source, "all!") && !hasPrefix(r.Source, "any!") {
-			for _, srcPart := range splitZones(r.Source) {
-				srcZone := zoneFromSpec(srcPart)
-				if _, ok := c.Zones[srcZone]; !ok {
-					return fmt.Errorf("rule[%d]: source zone %q not defined", i, srcZone)
+			for _, zs := range SplitZoneList(r.Source) {
+				if _, ok := c.Zones[zs.Zone]; !ok {
+					return fmt.Errorf("rule[%d]: source zone %q not defined", i, zs.Zone)
+				}
+				if !validAddrList(zs.Addr) {
+					return fmt.Errorf("rule[%d]: source %q: '!' may only prefix the whole address list", i, zs.Addr)
 				}
 			}
 		}
@@ -183,10 +188,12 @@ func (c *Config) validateRules() error {
 		if r.Action != RuleDNAT && r.Action != RuleRedirect && r.Action != RuleNoNAT {
 			if r.Dest != "all" && r.Dest != "any" && r.Dest != "none" &&
 				!hasPrefix(r.Dest, "all+") && !hasPrefix(r.Dest, "all!") && !hasPrefix(r.Dest, "any!") {
-				for _, dstPart := range splitZones(r.Dest) {
-					dstZone := zoneFromSpec(dstPart)
-					if _, ok := c.Zones[dstZone]; !ok {
-						return fmt.Errorf("rule[%d]: dest zone %q not defined", i, dstZone)
+				for _, zs := range SplitZoneList(r.Dest) {
+					if _, ok := c.Zones[zs.Zone]; !ok {
+						return fmt.Errorf("rule[%d]: dest zone %q not defined", i, zs.Zone)
+					}
+					if !validAddrList(zs.Addr) {
+						return fmt.Errorf("rule[%d]: dest %q: '!' may only prefix the whole address list", i, zs.Addr)
 					}
 				}
 			}
@@ -223,6 +230,30 @@ func (c *Config) validateRules() error {
 	return nil
 }
 
+// ZoneSpec is one zone of a SOURCE/DEST list; Addr is its comma-separated address list, if any.
+type ZoneSpec struct{ Zone, Addr string }
+
+// SplitZoneList parses "lan,svr:a,b": commas before the first colon separate zones,
+// commas after it separate addresses of the last zone (shorewall semantics).
+func SplitZoneList(spec string) []ZoneSpec {
+	zones, addr, _ := strings.Cut(spec, ":")
+	var out []ZoneSpec
+	for _, z := range strings.Split(zones, ",") {
+		if z = strings.TrimSpace(z); z != "" {
+			out = append(out, ZoneSpec{Zone: z})
+		}
+	}
+	if len(out) > 0 {
+		out[len(out)-1].Addr = addr
+	}
+	return out
+}
+
+// validAddrList reports whether '!' appears only at the start, negating the whole list.
+func validAddrList(addr string) bool {
+	return !strings.Contains(strings.TrimPrefix(addr, "!"), "!")
+}
+
 // zoneFromSpec extracts the zone name from a zone spec like "net" or "net:192.168.1.0/24".
 func zoneFromSpec(spec string) string {
 	for i, c := range spec {

internal/nftables/compiler.go

@@ -3,7 +3,9 @@ package nftables
 import (
 	"encoding/binary"
 	"fmt"
+	"log/slog"
 	"net"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -14,7 +16,8 @@ import (
 )
 
 type Compiler struct {
-	cfg *config.Config
+	cfg    *config.Config
+	warned map[string]bool
 }
 
 func NewCompiler(cfg *config.Config) *Compiler {
@@ -25,6 +28,7 @@ func (c *Compiler) Compile() (*FirewallState, error) {
 	state := &FirewallState{
 		Rules: make(map[string][]ManagedRule),
 	}
+	c.warned = nil
 
 	c.compileLoopback(state)
 	if err := c.compileConntrackFastPath(state); err != nil {
@@ -220,34 +224,30 @@ func (c *Compiler) compileConntrack(state *FirewallState) error {
 			chains = []string{"prerouting", "output"}
 		}
 
+		matches, err := l4Matches(ct.Proto, ct.DPort, nil)
+		if err != nil {
+			return fmt.Errorf("conntrack[%d]: %w", i, err)
+		}
+
 		for _, chain := range chains {
-			var exprs []expr.Any
+			for _, m := range matches {
+				exprs := append([]expr.Any{}, m.exprs...)
 
-			if ct.Proto != "" {
-				exprs = append(exprs, matchProto(ct.Proto)...)
-			}
-			for _, p := range ct.DPort {
-				pe, err := parsePortOrRange(p)
-				if err != nil {
-					return fmt.Errorf("conntrack[%d]: %w", i, err)
+				switch ct.Action {
+				case config.ConntrackNoTrack:
+					exprs = append(exprs, &expr.Notrack{})
+				case config.ConntrackHelper:
+					continue
+				case config.ConntrackDrop:
+					exprs = append(exprs, &expr.Verdict{Kind: expr.VerdictDrop})
 				}
-				exprs = append(exprs, pe...)
-			}
 
-			switch ct.Action {
-			case config.ConntrackNoTrack:
-				exprs = append(exprs, &expr.Notrack{})
-			case config.ConntrackHelper:
-				continue
-			case config.ConntrackDrop:
-				exprs = append(exprs, &expr.Verdict{Kind: expr.VerdictDrop})
+				state.Rules[chain] = append(state.Rules[chain], ManagedRule{
+					Chain: chain,
+					Exprs: exprs,
+					Tag:   tag + ":" + chain,
+				})
 			}
-
-			state.Rules[chain] = append(state.Rules[chain], ManagedRule{
-				Chain: chain,
-				Exprs: exprs,
-				Tag:   tag + ":" + chain,
-			})
 		}
 	}
 	return nil
@@ -271,6 +271,16 @@ func (c *Compiler) compileRules(state *FirewallState) error {
 			sport = rule.SPort
 		}
 
+		if rule.RateLimit != "" || rule.ConnLimit != "" {
+			matches, err := l4Matches(proto, dports, sport)
+			if err != nil {
+				return fmt.Errorf("rule[%d]: %w", i, err)
+			}
+			if len(matches)*specCount(rule.Source, rule.Dest, 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)
+			}
+		}
+
 		if err := c.compileOneRule(state, tag, rule.Source, rule.Dest,
 			proto, dports, sport,
 			rule.Action, rule.Log, rule.Dest, fwZone, rule.Section); err != nil {
@@ -289,11 +299,14 @@ func (c *Compiler) compileRules(state *FirewallState) error {
 }
 
 func (c *Compiler) applyRuleExtras(state *FirewallState, tag string, rule config.Rule) {
-	fwZone := c.cfg.FirewallZone()
-	srcZone, _ := splitZoneSpec(rule.Source)
-	dstZone, _ := splitZoneSpec(rule.Dest)
-	chain := c.selectChain(srcZone, dstZone, fwZone)
+	for chain := range state.Rules {
+		if chain != "prerouting" {
+			c.applyChainExtras(state, chain, tag, rule)
+		}
+	}
+}
 
+func (c *Compiler) applyChainExtras(state *FirewallState, chain, tag string, rule config.Rule) {
 	rules := state.Rules[chain]
 	for idx := len(rules) - 1; idx >= 0; idx-- {
 		if rules[idx].Tag != tag {
@@ -322,7 +335,7 @@ func (c *Compiler) applyRuleExtras(state *FirewallState, tag string, rule config
 			extra = append(extra, setMarkExprs(rule.SetMark)...)
 		}
 		if rule.Action == config.RuleNFQueue {
-			replaceVerdict = []expr.Any{&expr.Queue{Num: uint16(rule.NFQueue)}}
+			replaceVerdict = []expr.Any{&expr.Queue{Num: uint16(rule.NFQueue), Total: 1}}
 		}
 
 		if len(extra) > 0 || len(replaceVerdict) > 0 {
@@ -349,51 +362,101 @@ func (c *Compiler) compileOneRule(state *FirewallState, tag, srcSpec, dstSpec, p
 	dports, sports config.PortSpec, action config.RuleAction, logLevel string,
 	dnatDest string, fwZone string, section config.RuleSection) error {
 
-	srcZone, srcAddr := splitZoneSpec(srcSpec)
-	dstZone, dstAddr := splitZoneSpec(dstSpec)
-
-	if action == config.RuleDNAT || action == config.RuleRedirect {
-		return c.compileDNATRule(state, tag, srcSpec, dstSpec, proto, dports, sports, action, logLevel, fwZone)
+	for _, src := range zoneSpecs(srcSpec) {
+		for _, srcAddr := range splitAddrs(src.Addr) {
+			if action == config.RuleDNAT || action == config.RuleRedirect {
+				if err := c.compileDNATRule(state, tag, src.Zone, srcAddr, dstSpec, proto, dports, action, logLevel); err != nil {
+					return err
+				}
+				continue
+			}
+			for _, dst := range zoneSpecs(dstSpec) {
+				for _, dstAddr := range splitAddrs(dst.Addr) {
+					if err := c.compileZonePair(state, tag, src.Zone, srcAddr, dst.Zone, dstAddr, proto,
+						dports, sports, action, logLevel, fwZone, section); err != nil {
+						return err
+					}
+				}
+			}
+		}
 	}
+	return nil
+}
 
+// specCount is how many zone/address combinations compileOneRule expands src and dst into.
+func specCount(srcSpec, dstSpec string, action config.RuleAction) int {
+	count := func(spec string) (n int) {
+		for _, z := range zoneSpecs(spec) {
+			n += len(splitAddrs(z.Addr))
+		}
+		return n
+	}
+	if action == config.RuleDNAT || action == config.RuleRedirect {
+		return count(srcSpec)
+	}
+	return count(srcSpec) * count(dstSpec)
+}
+
+// zoneSpecs expands a comma zone list; "all"/"any" forms keep their own comma (exclusion) syntax.
+func zoneSpecs(spec string) []config.ZoneSpec {
+	zone, addr := splitZoneSpec(spec)
+	if base, _, _ := strings.Cut(strings.TrimSuffix(zone, "+"), "!"); base == "all" || base == "any" {
+		return []config.ZoneSpec{{Zone: zone, Addr: addr}}
+	}
+	return config.SplitZoneList(spec)
+}
+
+// splitAddrs yields one alternative per listed address; a negated list stays one AND-ed match.
+func splitAddrs(addr string) []string {
+	if addr == "" || strings.HasPrefix(addr, "!") {
+		return []string{addr}
+	}
+	return strings.Split(addr, ",")
+}
+
+func (c *Compiler) compileZonePair(state *FirewallState, tag, srcZone, srcAddr, dstZone, dstAddr, proto string,
+	dports, sports config.PortSpec, action config.RuleAction, logLevel string,
+	fwZone string, section config.RuleSection) error {
 	srcIfaces := c.resolveZoneInterfaces(srcZone)
 	dstIfaces := c.resolveZoneInterfaces(dstZone)
 	chain := c.selectChain(srcZone, dstZone, fwZone)
 
 	for _, srcIface := range srcIfaces {
 		for _, dstIface := range dstIfaces {
-			exprs, err := c.buildMatchExprs(srcIface, dstIface, chain, proto, dports, sports, srcAddr, dstAddr)
+			matches, err := c.buildMatchExprs(srcIface, dstIface, chain, proto, dports, sports, srcAddr, dstAddr)
 			if err != nil {
 				return err
 			}
 
-			if section != "" && section != config.SectionAll {
-				exprs = append(exprs, matchSection(section)...)
-			}
+			for _, m := range matches {
+				exprs := m.exprs
 
-			if logLevel != "" {
-				exprs = append(exprs, buildLog(logLevel, tag)...)
-			}
+				if section != "" && section != config.SectionAll {
+					exprs = append(exprs, matchSection(section)...)
+				}
 
-			verdict := actionVerdict(action, proto, c.cfg.Settings.AddressFamily)
-			if verdict != nil {
-				exprs = append(exprs, verdict...)
-			}
+				if logLevel != "" {
+					exprs = append(exprs, buildLog(logLevel, tag)...)
+				}
 
-			state.Rules[chain] = append(state.Rules[chain], ManagedRule{
-				Chain: chain,
-				Exprs: exprs,
-				Tag:   tag,
-			})
+				verdict := actionVerdict(action, m.proto, c.cfg.Settings.AddressFamily)
+				if verdict != nil {
+					exprs = append(exprs, verdict...)
+				}
+
+				state.Rules[chain] = append(state.Rules[chain], ManagedRule{
+					Chain: chain,
+					Exprs: exprs,
+					Tag:   tag,
+				})
+			}
 		}
 	}
 	return nil
 }
 
-func (c *Compiler) compileDNATRule(state *FirewallState, tag, srcSpec, dstSpec, proto string,
-	dports, sports config.PortSpec, action config.RuleAction, logLevel, fwZone string) error {
-
-	srcZone, srcAddr := splitZoneSpec(srcSpec)
+func (c *Compiler) compileDNATRule(state *FirewallState, tag, srcZone, srcAddr, dstSpec, proto string,
+	dports config.PortSpec, action config.RuleAction, logLevel string) error {
 	chain := "prerouting"
 
 	parts := strings.SplitN(dstSpec, ":", 3)
@@ -413,102 +476,99 @@ func (c *Compiler) compileDNATRule(state *FirewallState, tag, srcSpec, dstSpec,
 
 	srcIfaces := c.resolveZoneInterfaces(srcZone)
 
+	matches, err := l4Matches(proto, dports, nil)
+	if err != nil {
+		return err
+	}
+
+	ip := net.ParseIP(dnatAddr)
+	if ip == nil {
+		return fmt.Errorf("invalid DNAT address %q", dnatAddr)
+	}
+
 	for _, srcIface := range srcIfaces {
-		var exprs []expr.Any
+		for _, m := range matches {
+			var exprs []expr.Any
 
-		if srcIface != "" {
-			exprs = append(exprs, matchIfaceName(true, srcIface)...)
-		}
-
-		if srcAddr != "" {
-			src, err := matchSourceCIDR(srcAddr)
-			if err != nil {
-				return err
+			if srcIface != "" {
+				exprs = append(exprs, matchIfaceName(true, srcIface)...)
 			}
-			exprs = append(exprs, src...)
-		}
 
-		if proto != "" {
-			exprs = append(exprs, matchProto(proto)...)
-		}
-
-		for _, portStr := range dports {
-			pe, err := parsePortOrRange(portStr)
-			if err != nil {
-				return err
-			}
-			exprs = append(exprs, pe...)
-		}
-
-		if logLevel != "" {
-			exprs = append(exprs, buildLog(logLevel, tag)...)
-		}
-
-		ip := net.ParseIP(dnatAddr)
-		if ip == nil {
-			return fmt.Errorf("invalid DNAT address %q", dnatAddr)
-		}
-
-		if action == config.RuleRedirect {
-			if dnatPort > 0 {
-				portBytes := make([]byte, 2)
-				binary.BigEndian.PutUint16(portBytes, dnatPort)
-				exprs = append(exprs,
-					&expr.Immediate{Register: 1, Data: portBytes},
-					&expr.Redir{RegisterProtoMin: 1},
-				)
-			} else {
-				exprs = append(exprs, &expr.Redir{})
-			}
-		} else {
-			if ip4 := ip.To4(); ip4 != nil {
-				exprs = append(exprs,
-					&expr.Immediate{Register: 1, Data: ip4},
-				)
-				natExpr := &expr.NAT{
-					Type:       expr.NATTypeDestNAT,
-					Family:     unix.NFPROTO_IPV4,
-					RegAddrMin: 1,
-					RegAddrMax: 1,
+			if srcAddr != "" {
+				src, err := matchSourceCIDR(srcAddr)
+				if err != nil {
+					return err
 				}
+				exprs = append(exprs, src...)
+			}
+
+			exprs = append(exprs, m.exprs...)
+
+			if logLevel != "" {
+				exprs = append(exprs, buildLog(logLevel, tag)...)
+			}
+
+			if action == config.RuleRedirect {
 				if dnatPort > 0 {
 					portBytes := make([]byte, 2)
 					binary.BigEndian.PutUint16(portBytes, dnatPort)
 					exprs = append(exprs,
-						&expr.Immediate{Register: 2, Data: portBytes},
+						&expr.Immediate{Register: 1, Data: portBytes},
+						&expr.Redir{RegisterProtoMin: 1, RegisterProtoMax: 1, Flags: unix.NF_NAT_RANGE_PROTO_SPECIFIED},
 					)
-					natExpr.RegProtoMin = 2
-					natExpr.RegProtoMax = 2
+				} else {
+					exprs = append(exprs, &expr.Redir{})
 				}
-				exprs = append(exprs, natExpr)
 			} else {
-				exprs = append(exprs,
-					&expr.Immediate{Register: 1, Data: ip.To16()},
-				)
-				natExpr := &expr.NAT{
-					Type:       expr.NATTypeDestNAT,
-					Family:     unix.NFPROTO_IPV6,
-					RegAddrMin: 1,
-					RegAddrMax: 1,
-				}
-				if dnatPort > 0 {
-					portBytes := make([]byte, 2)
-					binary.BigEndian.PutUint16(portBytes, dnatPort)
+				if ip4 := ip.To4(); ip4 != nil {
 					exprs = append(exprs,
-						&expr.Immediate{Register: 2, Data: portBytes},
+						&expr.Immediate{Register: 1, Data: ip4},
 					)
-					natExpr.RegProtoMin = 2
-					natExpr.RegProtoMax = 2
+					natExpr := &expr.NAT{
+						Type:       expr.NATTypeDestNAT,
+						Family:     unix.NFPROTO_IPV4,
+						RegAddrMin: 1,
+						RegAddrMax: 1,
+					}
+					if dnatPort > 0 {
+						portBytes := make([]byte, 2)
+						binary.BigEndian.PutUint16(portBytes, dnatPort)
+						exprs = append(exprs,
+							&expr.Immediate{Register: 2, Data: portBytes},
+						)
+						natExpr.RegProtoMin = 2
+						natExpr.RegProtoMax = 2
+					}
+					exprs = append(exprs, natExpr)
+				} else {
+					exprs = append(exprs,
+						&expr.Immediate{Register: 1, Data: ip.To16()},
+					)
+					natExpr := &expr.NAT{
+						Type:       expr.NATTypeDestNAT,
+						Family:     unix.NFPROTO_IPV6,
+						RegAddrMin: 1,
+						RegAddrMax: 1,
+					}
+					if dnatPort > 0 {
+						portBytes := make([]byte, 2)
+						binary.BigEndian.PutUint16(portBytes, dnatPort)
+						exprs = append(exprs,
+							&expr.Immediate{Register: 2, Data: portBytes},
+						)
+						natExpr.RegProtoMin = 2
+						natExpr.RegProtoMax = 2
+					}
+					exprs = append(exprs, natExpr)
 				}
-				exprs = append(exprs, natExpr)
 			}
-		}
 
-		state.Rules[chain] = append(state.Rules[chain], ManagedRule{
-			Chain: chain,
-			Exprs: exprs,
-			Tag:   tag,
-		})
+			state.Rules[chain] = append(state.Rules[chain], ManagedRule{
+				Chain: chain,
+				Exprs: exprs,
+				Tag:   tag,
+			})
+		}
 	}
 	return nil
 }
@@ -588,25 +648,12 @@ func (c *Compiler) compileSNAT(state *FirewallState) error {
 			exprs = append(exprs, srcExprs...)
 		}
 
-		if snat.Proto != "" {
-			exprs = append(exprs, matchProto(snat.Proto)...)
-		}
-
-		for _, portStr := range snat.DPort {
-			pe, err := parsePortOrRange(portStr)
-			if err != nil {
-				return fmt.Errorf("snat[%d] dport: %w", i, err)
-			}
-			exprs = append(exprs, pe...)
-		}
-
-		for _, portStr := range snat.SPort {
-			pe, err := parseSPortOrRange(portStr)
-			if err != nil {
-				return fmt.Errorf("snat[%d] sport: %w", i, err)
-			}
-			exprs = append(exprs, pe...)
+		matches, err := l4Matches(snat.Proto, snat.DPort, snat.SPort)
+		if err != nil {
+			return fmt.Errorf("snat[%d]: %w", i, err)
 		}
+		head := exprs
+		exprs = nil
 
 		if snat.Mark != "" {
 			exprs = append(exprs, matchMark(snat.Mark)...)
@@ -652,11 +699,13 @@ func (c *Compiler) compileSNAT(state *FirewallState) error {
 			}
 		}
 
-		state.Rules["postrouting"] = append(state.Rules["postrouting"], ManagedRule{
-			Chain: "postrouting",
-			Exprs: exprs,
-			Tag:   tag,
-		})
+		for _, m := range matches {
+			state.Rules["postrouting"] = append(state.Rules["postrouting"], ManagedRule{
+				Chain: "postrouting",
+				Exprs: append(append(append([]expr.Any{}, head...), m.exprs...), exprs...),
+				Tag:   tag,
+			})
+		}
 	}
 
 	return nil
@@ -790,7 +839,7 @@ func (c *Compiler) compileMSSClamp(state *FirewallState) {
 					Type:         2,
 					Offset:       2,
 					Len:          2,
-					Op:           0,
+					Op:           expr.ExthdrOpTcpopt,
 				},
 				&expr.Cmp{Op: expr.CmpOpGt, Register: 1, Data: mssBytes},
 				&expr.Immediate{Register: 1, Data: mssBytes},
@@ -799,7 +848,7 @@ func (c *Compiler) compileMSSClamp(state *FirewallState) {
 					Type:           2,
 					Offset:         2,
 					Len:            2,
-					Op:             1,
+					Op:             expr.ExthdrOpTcpopt,
 				},
 			)
 			state.Rules["forward"] = append(state.Rules["forward"], ManagedRule{
@@ -888,10 +937,29 @@ func (c *Compiler) resolveZoneInterfaces(zone string) []string {
 		return []string{""}
 	}
 	ifaces := c.cfg.ZoneInterfaces(zone)
-	if len(ifaces) == 0 {
-		return []string{""}
+	if len(ifaces) > 0 {
+		return ifaces
 	}
-	return ifaces
+	if z, ok := c.cfg.Zones[zone]; ok && z.Type == config.ZoneIP && !c.zoneHasHosts(zone) {
+		if !c.warned[zone] {
+			if c.warned == nil {
+				c.warned = map[string]bool{}
+			}
+			c.warned[zone] = true
+			slog.Warn("compiler: zone has no interfaces, skipping its rules", "zone", zone)
+		}
+		return nil
+	}
+	return []string{""}
+}
+
+func (c *Compiler) zoneHasHosts(zone string) bool {
+	for _, h := range c.cfg.Hosts {
+		if h.Zone == zone {
+			return true
+		}
+	}
+	return false
 }
 
 func (c *Compiler) expandZoneRef(ref string) []string {
@@ -917,12 +985,13 @@ func (c *Compiler) expandZoneRef(ref string) []string {
 			}
 			zones = append(zones, name)
 		}
+		sort.Strings(zones)
 		return zones
 	}
 	return []string{base}
 }
 
-func (c *Compiler) buildMatchExprs(srcIface, dstIface, chain, proto string, dports, sports config.PortSpec, srcAddr, dstAddr string) ([]expr.Any, error) {
+func (c *Compiler) buildMatchExprs(srcIface, dstIface, chain, proto string, dports, sports config.PortSpec, srcAddr, dstAddr string) ([]l4Match, error) {
 	var exprs []expr.Any
 
 	if srcIface != "" {
@@ -948,34 +1017,92 @@ func (c *Compiler) buildMatchExprs(srcIface, dstIface, chain, proto string, dpor
 		exprs = append(exprs, dst...)
 	}
 
+	matches, err := l4Matches(proto, dports, sports)
+	if err != nil {
+		return nil, err
+	}
+	for i := range matches {
+		matches[i].exprs = append(append([]expr.Any{}, exprs...), matches[i].exprs...)
+	}
+	return matches, nil
+}
+
+type l4Match struct {
+	proto byte
+	exprs []expr.Any
+}
+
+// l4Matches yields one alternative per (proto, dport, sport): nft ANDs a rule's exprs, so lists need one rule each.
+func l4Matches(proto string, dports, sports config.PortSpec) ([]l4Match, error) {
+	protos := []string{""}
 	if proto != "" {
-		exprs = append(exprs, matchProto(proto)...)
+		protos = strings.Split(proto, ",")
 	}
 
-	isICMP := strings.EqualFold(proto, "icmp") || strings.EqualFold(proto, "icmpv6") || strings.EqualFold(proto, "ipv6-icmp")
-
-	for _, portStr := range dports {
-		if isICMP {
-			pe := matchICMPType(portStr)
-			exprs = append(exprs, pe...)
-		} else {
-			pe, err := parsePortOrRange(portStr)
+	var out []l4Match
+	for _, p := range protos {
+		p = strings.TrimSpace(p)
+		if proto != "" && p == "" {
+			return nil, fmt.Errorf("empty element in proto list %q", proto)
+		}
+		var pm []expr.Any
+		var n byte
+		isICMP := false
+		if p != "" {
+			var err error
+			n, err = protoNumber(p)
 			if err != nil {
 				return nil, err
 			}
-			exprs = append(exprs, pe...)
+			isICMP = n == unix.IPPROTO_ICMP || n == unix.IPPROTO_ICMPV6
+			if (len(dports) > 0 && !isICMP || len(sports) > 0) && !hasPorts(n) {
+				return nil, fmt.Errorf("protocol %q does not support ports", p)
+			}
+			pm = matchProtoNum(n)
 		}
-	}
-
-	for _, portStr := range sports {
-		pe, err := parseSPortOrRange(portStr)
+		parseD := parsePortOrRange
+		if isICMP {
+			if len(protos) > 1 && len(dports) > 0 {
+				return nil, fmt.Errorf("dport %v is ambiguous with %s in proto list %q", dports, p, proto)
+			}
+			parseD = matchICMPType
+		}
+		dalts, err := portAlternatives(dports, parseD)
 		if err != nil {
 			return nil, err
 		}
-		exprs = append(exprs, pe...)
+		salts, err := portAlternatives(sports, parseSPortOrRange)
+		if err != nil {
+			return nil, err
+		}
+		for _, d := range dalts {
+			for _, sp := range salts {
+				e := append(append(append([]expr.Any{}, pm...), d...), sp...)
+				out = append(out, l4Match{proto: n, exprs: e})
+			}
+		}
 	}
+	return out, nil
+}
 
-	return exprs, nil
+func portAlternatives(ports config.PortSpec, parse func(string) ([]expr.Any, error)) ([][]expr.Any, error) {
+	var alts [][]expr.Any
+	for _, item := range ports {
+		for _, s := range strings.Split(item, ",") {
+			if s = strings.TrimSpace(s); s == "" {
+				return nil, fmt.Errorf("empty element in port list %q", item)
+			}
+			e, err := parse(s)
+			if err != nil {
+				return nil, err
+			}
+			alts = append(alts, e)
+		}
+	}
+	if len(alts) == 0 {
+		return [][]expr.Any{nil}, nil
+	}
+	return alts, nil
 }
 
 // matchIfaceName matches an interface name, supporting wildcard "+" suffix.
@@ -1003,33 +1130,27 @@ func matchIfaceName(input bool, name string) []expr.Any {
 	}
 }
 
-func matchProto(proto string) []expr.Any {
-	var protoNum byte
-	switch strings.ToLower(proto) {
-	case "tcp":
-		protoNum = unix.IPPROTO_TCP
-	case "udp":
-		protoNum = unix.IPPROTO_UDP
-	case "icmp":
-		protoNum = unix.IPPROTO_ICMP
-	case "icmpv6", "ipv6-icmp":
-		protoNum = unix.IPPROTO_ICMPV6
-	case "gre":
-		protoNum = 47
-	case "esp":
-		protoNum = 50
-	case "ah":
-		protoNum = 51
-	case "sctp":
-		protoNum = unix.IPPROTO_SCTP
-	default:
-		n, _ := strconv.Atoi(proto)
-		protoNum = byte(n)
+var protoNumbers = map[string]byte{
+	"icmp": 1, "igmp": 2, "ipip": 4, "ipencap": 4, "tcp": 6, "udp": 17,
+	"gre": 47, "esp": 50, "ah": 51, "icmpv6": 58, "ipv6-icmp": 58,
+	"ospf": 89, "ospfigp": 89, "pim": 103, "vrrp": 112, "l2tp": 115,
+	"sctp": 132, "udplite": 136,
+}
+
+// protoNumber resolves a common IANA protocol name or a 0-255 number.
+func protoNumber(proto string) (byte, error) {
+	if n, ok := protoNumbers[strings.ToLower(proto)]; ok {
+		return n, nil
 	}
-	return []expr.Any{
-		&expr.Meta{Key: expr.MetaKeyL4PROTO, Register: 1},
-		&expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{protoNum}},
+	n, err := strconv.ParseUint(proto, 10, 8)
+	if err != nil {
+		return 0, fmt.Errorf("unknown protocol %q", proto)
 	}
+	return byte(n), nil
+}
+
+func hasPorts(proto byte) bool {
+	return proto == unix.IPPROTO_TCP || proto == unix.IPPROTO_UDP || proto == unix.IPPROTO_SCTP || proto == unix.IPPROTO_UDPLITE
 }
 
 func matchDPort(port uint16) []expr.Any {
@@ -1141,33 +1262,33 @@ var icmpTypeNames = map[string]byte{
 	"address-mask-reply":      18,
 }
 
-func matchICMPType(spec string) []expr.Any {
+func matchICMPType(spec string) ([]expr.Any, error) {
 	if strings.Contains(spec, "/") {
 		parts := strings.SplitN(spec, "/", 2)
 		typeVal, ok := resolveICMPType(parts[0])
 		if !ok {
-			return nil
+			return nil, fmt.Errorf("invalid icmp type %q", parts[0])
 		}
 		code, err := strconv.ParseUint(parts[1], 10, 8)
 		if err != nil {
-			return nil
+			return nil, fmt.Errorf("invalid icmp code %q: %w", parts[1], err)
 		}
 		return []expr.Any{
 			&expr.Payload{DestRegister: 1, Base: expr.PayloadBaseTransportHeader, Offset: 0, Len: 1},
 			&expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{typeVal}},
 			&expr.Payload{DestRegister: 1, Base: expr.PayloadBaseTransportHeader, Offset: 1, Len: 1},
 			&expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{byte(code)}},
-		}
+		}, nil
 	}
 
 	typeVal, ok := resolveICMPType(spec)
 	if !ok {
-		return nil
+		return nil, fmt.Errorf("invalid icmp type %q", spec)
 	}
 	return []expr.Any{
 		&expr.Payload{DestRegister: 1, Base: expr.PayloadBaseTransportHeader, Offset: 0, Len: 1},
 		&expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{typeVal}},
-	}
+	}, nil
 }
 
 func resolveICMPType(s string) (byte, bool) {
@@ -1218,8 +1339,8 @@ func matchConnLimit(spec string) []expr.Any {
 }
 
 func parsePortOrRange(s string) ([]expr.Any, error) {
-	if strings.Contains(s, "-") {
-		parts := strings.SplitN(s, "-", 2)
+	if i := strings.IndexAny(s, "-:"); i >= 0 {
+		parts := []string{s[:i], s[i+1:]}
 		low, err := strconv.ParseUint(parts[0], 10, 16)
 		if err != nil {
 			return nil, fmt.Errorf("invalid port range low %q: %w", parts[0], err)
@@ -1238,8 +1359,8 @@ func parsePortOrRange(s string) ([]expr.Any, error) {
 }
 
 func parseSPortOrRange(s string) ([]expr.Any, error) {
-	if strings.Contains(s, "-") {
-		parts := strings.SplitN(s, "-", 2)
+	if i := strings.IndexAny(s, "-:"); i >= 0 {
+		parts := []string{s[:i], s[i+1:]}
 		low, err := strconv.ParseUint(parts[0], 10, 16)
 		if err != nil {
 			return nil, fmt.Errorf("invalid sport range low %q: %w", parts[0], err)
@@ -1270,6 +1391,17 @@ func matchAddrCIDR(cidr string, isSrc bool) ([]expr.Any, error) {
 	if strings.HasPrefix(cidr, "!") {
 		negated = true
 		cidr = cidr[1:]
+		if strings.Contains(cidr, ",") {
+			var all []expr.Any
+			for _, a := range strings.Split(cidr, ",") {
+				e, err := matchAddrCIDR("!"+a, isSrc)
+				if err != nil {
+					return nil, err
+				}
+				all = append(all, e...)
+			}
+			return all, nil
+		}
 	}
 
 	cmpOp := expr.CmpOpEq
@@ -1619,7 +1751,7 @@ func logLevelToNF(level string) expr.LogLevel {
 	}
 }
 
-func actionVerdict(action config.RuleAction, proto string, family config.AddressFamily) []expr.Any {
+func actionVerdict(action config.RuleAction, proto byte, family config.AddressFamily) []expr.Any {
 	switch action {
 	case config.RuleAccept:
 		return []expr.Any{&expr.Verdict{Kind: expr.VerdictAccept}}
@@ -1644,8 +1776,8 @@ func actionVerdict(action config.RuleAction, proto string, family config.Address
 	}
 }
 
-func rejectExprs(proto string, family config.AddressFamily) []expr.Any {
-	if strings.ToLower(proto) == "tcp" {
+func rejectExprs(proto byte, family config.AddressFamily) []expr.Any {
+	if proto == unix.IPPROTO_TCP {
 		return []expr.Any{&expr.Reject{
 			Type: unix.NFT_REJECT_TCP_RST,
 			Code: 0,
@@ -1664,7 +1796,7 @@ func policyVerdict(action config.PolicyAction, family config.AddressFamily) []ex
 	case config.PolicyDrop:
 		return []expr.Any{&expr.Verdict{Kind: expr.VerdictDrop}}
 	case config.PolicyReject:
-		return rejectExprs("", family)
+		return rejectExprs(0, family)
 	default:
 		return []expr.Any{&expr.Verdict{Kind: expr.VerdictDrop}}
 	}

internal/nftables/compiler_test.go

@@ -1,9 +1,14 @@
 package nftables
 
 import (
+	"encoding/binary"
+	"fmt"
+	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/google/nftables/expr"
+	"golang.org/x/sys/unix"
 
 	"git.unkin.net/unkin/tomswall/internal/config"
 )
@@ -973,7 +978,7 @@ func TestNegatedAddress(t *testing.T) {
 }
 
 func TestRejectTCPRST(t *testing.T) {
-	exprs := rejectExprs("tcp", config.FamilyINET)
+	exprs := rejectExprs(unix.IPPROTO_TCP, config.FamilyINET)
 	if len(exprs) != 1 {
 		t.Fatalf("expected 1 expr, got %d", len(exprs))
 	}
@@ -982,7 +987,7 @@ func TestRejectTCPRST(t *testing.T) {
 		t.Errorf("TCP reject should use NFT_REJECT_TCP_RST (1), got %d", rej.Type)
 	}
 
-	exprs = rejectExprs("udp", config.FamilyINET)
+	exprs = rejectExprs(unix.IPPROTO_UDP, config.FamilyINET)
 	rej = exprs[0].(*expr.Reject)
 	if rej.Type != 2 {
 		t.Errorf("non-TCP reject should use NFT_REJECT_ICMPX_UNREACH (2), got %d", rej.Type)
@@ -1197,14 +1202,26 @@ func TestCompile_MSSClamp(t *testing.T) {
 		t.Fatalf("Compile() error: %v", err)
 	}
 
-	found := false
-	for _, r := range state.Rules["forward"] {
+	var rule *ManagedRule
+	for i, r := range state.Rules["forward"] {
 		if r.Tag == "mss:eth1" {
-			found = true
+			rule = &state.Rules["forward"][i]
 		}
 	}
-	if !found {
-		t.Error("MSS clamp rule not found in forward chain")
+	if rule == nil {
+		t.Fatal("MSS clamp rule not found in forward chain")
+	}
+
+	mss := []byte{0x05, 0x78}
+	want := []expr.Any{
+		&expr.Exthdr{DestRegister: 1, Type: 2, Offset: 2, Len: 2, Op: expr.ExthdrOpTcpopt},
+		&expr.Cmp{Op: expr.CmpOpGt, Register: 1, Data: mss},
+		&expr.Immediate{Register: 1, Data: mss},
+		&expr.Exthdr{SourceRegister: 1, Type: 2, Offset: 2, Len: 2, Op: expr.ExthdrOpTcpopt},
+	}
+	got := rule.Exprs[len(rule.Exprs)-len(want):]
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("MSS clamp exprs = %#v, want %#v", got, want)
 	}
 }
 
@@ -1265,7 +1282,10 @@ func TestMatchICMPType(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		exprs := matchICMPType(tt.input)
+		exprs, err := matchICMPType(tt.input)
+		if err != nil {
+			t.Fatalf("matchICMPType(%q) error: %v", tt.input, err)
+		}
 		if len(exprs) != tt.wantLen {
 			t.Errorf("matchICMPType(%q) returned %d exprs, want %d", tt.input, len(exprs), tt.wantLen)
 		}
@@ -1531,3 +1551,707 @@ func TestCompile_PolicyRateLimit(t *testing.T) {
 	}
 	t.Error("policy:0 not found in input chain")
 }
+
+func diffTestConfig(port string) *config.Config {
+	return &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},
+			"dmz": {Type: config.ZoneIP},
+		},
+		Interfaces: []config.Interface{
+			{Zone: "net", Interface: "eth0"},
+			{Zone: "loc", Interface: "eth1"},
+			{Zone: "dmz", Interface: "eth2"},
+		},
+		Policy: []config.Policy{
+			{Source: "net", Dest: "all", Action: config.PolicyDrop, Log: "info"},
+			{Source: "all", Dest: "all", Action: config.PolicyReject},
+		},
+		Rules: []config.Rule{
+			{Action: config.RuleAccept, Source: "net:192.0.2.0/24", Dest: "fw", Proto: "tcp", DPort: config.PortSpec{port}},
+			{Action: config.RuleDNAT, Source: "net", Dest: "loc:198.51.100.10:80", Proto: "tcp", DPort: config.PortSpec{"8000"}},
+			{Action: config.RuleNFQueue, Source: "net", Dest: "fw", Proto: "tcp", DPort: config.PortSpec{"80"}, NFQueue: 3},
+			{Action: config.RuleRedirect, Source: "loc", Dest: "fw:192.0.2.1:3128", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			{Action: config.RuleAccept, Source: "loc,dmz", Dest: "fw,net", Proto: "tcp,udp", DPort: config.PortSpec{"53", "5353"}},
+		},
+		SNAT:       []config.SNATRule{{Action: config.SNATAddress, Source: "198.51.100.0/24", Dest: "eth0", Address: "203.0.113.7"}},
+		PortGroups: make(map[string]config.PortGroup),
+	}
+}
+
+func TestDiffEngine_IndependentCompilesMatch(t *testing.T) {
+	compile := func(port string) *FirewallState {
+		t.Helper()
+		s, err := NewCompiler(diffTestConfig(port)).Compile()
+		if err != nil {
+			t.Fatal(err)
+		}
+		return s
+	}
+
+	for i := 0; i < 20; i++ {
+		if cs := computeDiff(compile("22"), compile("22")); !cs.Empty() {
+			t.Fatalf("expected empty changeset, got:\n%s", cs.Summary())
+		}
+	}
+
+	cs := computeDiff(compile("22"), compile("2222"))
+	if len(cs.Add) != 1 || len(cs.Remove) != 1 || cs.Add[0].Tag != "rule:0" {
+		t.Errorf("expected rule:0 replaced, got:\n%s", cs.Summary())
+	}
+}
+
+// shapes observed by applying diffTestConfig in a netns and reading it back
+func TestCompile_QueueRedirMatchKernelReadback(t *testing.T) {
+	state, err := NewCompiler(diffTestConfig("22")).Compile()
+	if err != nil {
+		t.Fatal(err)
+	}
+	want := map[string]expr.Any{
+		"rule:2": &expr.Queue{Num: 3, Total: 1},
+		"rule:3": &expr.Redir{RegisterProtoMin: 1, RegisterProtoMax: 1, Flags: unix.NF_NAT_RANGE_PROTO_SPECIFIED},
+	}
+	for _, rules := range state.Rules {
+		for _, r := range rules {
+			w, ok := want[r.Tag]
+			if !ok {
+				continue
+			}
+			if got := r.Exprs[len(r.Exprs)-1]; !reflect.DeepEqual(got, w) {
+				t.Errorf("%s: got %#v, want %#v", r.Tag, got, w)
+			}
+			delete(want, r.Tag)
+		}
+	}
+	for tag := range want {
+		t.Errorf("%s not compiled", tag)
+	}
+}
+
+func withHandles(s *FirewallState) *FirewallState {
+	h := uint64(100)
+	for _, rules := range s.Rules {
+		for i := range rules {
+			rules[i].Handle = h
+			h++
+		}
+	}
+	return s
+}
+
+func tags(rules []ManagedRule) []string {
+	out := make([]string, len(rules))
+	for i, r := range rules {
+		out[i] = r.Chain + "/" + r.Tag
+	}
+	return out
+}
+
+func TestCompile_PortAndProtoLists(t *testing.T) {
+	type want struct {
+		proto byte
+		dport string // "80" for an exact compare, "8000-8100" for a range
+	}
+	tests := []struct {
+		name  string
+		rule  config.Rule
+		chain string
+		want  []want
+	}{
+		{
+			name:  "multi-port",
+			rule:  config.Rule{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "tcp", DPort: config.PortSpec{"80", "443"}},
+			chain: "input",
+			want:  []want{{6, "80"}, {6, "443"}},
+		},
+		{
+			name:  "range in list",
+			rule:  config.Rule{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "tcp", DPort: config.PortSpec{"22", "8000:8100"}},
+			chain: "input",
+			want:  []want{{6, "22"}, {6, "8000-8100"}},
+		},
+		{
+			name:  "comma string",
+			rule:  config.Rule{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "tcp", DPort: config.PortSpec{"80,443"}},
+			chain: "input",
+			want:  []want{{6, "80"}, {6, "443"}},
+		},
+		{
+			name:  "tcp,udp",
+			rule:  config.Rule{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "tcp,udp", DPort: config.PortSpec{"53"}},
+			chain: "input",
+			want:  []want{{6, "53"}, {17, "53"}},
+		},
+		{
+			name:  "dnat tcp,udp",
+			rule:  config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "fw:192.0.2.10", Proto: "tcp,udp", DPort: config.PortSpec{"53"}},
+			chain: "prerouting",
+			want:  []want{{6, "53"}, {17, "53"}},
+		},
+		{
+			name:  "protocol names",
+			rule:  config.Rule{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "ospf,OSPFIGP,igmp,gre,esp,ah,vrrp,pim,ipencap,ipv6-icmp"},
+			chain: "input",
+			want:  []want{{89, ""}, {89, ""}, {2, ""}, {47, ""}, {50, ""}, {51, ""}, {112, ""}, {103, ""}, {4, ""}, {58, ""}},
+		},
+		{
+			name:  "protocol numbers",
+			rule:  config.Rule{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "0,89,255"},
+			chain: "input",
+			want:  []want{{0, ""}, {89, ""}, {255, ""}},
+		},
+		{
+			name:  "numeric tcp with port",
+			rule:  config.Rule{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "6,udplite", DPort: config.PortSpec{"22"}},
+			chain: "input",
+			want:  []want{{6, "22"}, {136, "22"}},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.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"}},
+				Policy:     []config.Policy{{Source: "all", Dest: "all", Action: config.PolicyDrop}},
+				Rules:      []config.Rule{tt.rule},
+				PortGroups: make(map[string]config.PortGroup),
+			}
+			state, err := NewCompiler(cfg).Compile()
+			if err != nil {
+				t.Fatalf("Compile() error: %v", err)
+			}
+
+			var got []want
+			for _, r := range state.Rules[tt.chain] {
+				if r.Tag != "rule:0" {
+					continue
+				}
+				var w want
+				var portCmps []string
+				for i, e := range r.Exprs {
+					if m, ok := e.(*expr.Meta); ok && m.Key == expr.MetaKeyL4PROTO {
+						w.proto = r.Exprs[i+1].(*expr.Cmp).Data[0]
+					}
+					if p, ok := e.(*expr.Payload); ok && p.Base == expr.PayloadBaseTransportHeader && p.Offset == 2 {
+						for _, c := range r.Exprs[i+1:] {
+							cmp, ok := c.(*expr.Cmp)
+							if !ok {
+								break
+							}
+							portCmps = append(portCmps, fmt.Sprint(binary.BigEndian.Uint16(cmp.Data)))
+						}
+					}
+				}
+				w.dport = strings.Join(portCmps, "-")
+				got = append(got, w)
+			}
+
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("rules = %+v, want %+v", got, tt.want)
+			}
+		})
+	}
+}
+
+// describeRule renders a rule's iif/oif/saddr/daddr matches, e.g. "iif=eth1 oif=eth2 daddr=192.0.2.1".
+func describeRule(r ManagedRule) string {
+	var parts []string
+	for i, e := range r.Exprs {
+		cmp, ok := func() (*expr.Cmp, bool) {
+			if i+1 >= len(r.Exprs) {
+				return nil, false
+			}
+			c, ok := r.Exprs[i+1].(*expr.Cmp)
+			return c, ok
+		}()
+		if !ok {
+			continue
+		}
+		switch m := e.(type) {
+		case *expr.Meta:
+			switch m.Key {
+			case expr.MetaKeyIIFNAME:
+				parts = append(parts, "iif="+strings.TrimRight(string(cmp.Data), "\x00"))
+			case expr.MetaKeyOIFNAME:
+				parts = append(parts, "oif="+strings.TrimRight(string(cmp.Data), "\x00"))
+			}
+		case *expr.Payload:
+			if m.Base == expr.PayloadBaseNetworkHeader && m.Len == 4 {
+				name := map[uint32]string{12: "saddr", 16: "daddr"}[m.Offset]
+				if cmp.Op == expr.CmpOpNeq {
+					name = "!" + name
+				}
+				parts = append(parts, fmt.Sprintf("%s=%d.%d.%d.%d", name, cmp.Data[0], cmp.Data[1], cmp.Data[2], cmp.Data[3]))
+			}
+		}
+	}
+	return strings.Join(parts, " ")
+}
+
+func TestCompile_CommaZoneLists(t *testing.T) {
+	tests := []struct {
+		name   string
+		rule   config.Rule
+		blrule *config.BlruleRule
+		want   map[string][]string
+	}{
+		{
+			name: "fw in source list goes to output",
+			rule: config.Rule{Action: config.RuleAccept, Source: "fw,lan", Dest: "svr", Proto: "tcp", DPort: config.PortSpec{"22"}},
+			want: map[string][]string{"output": {""}, "forward": {"iif=eth1 oif=eth2"}},
+		},
+		{
+			name: "dest list with fw splits input and forward",
+			rule: config.Rule{Action: config.RuleAccept, Source: "lan", Dest: "fw,svr,net"},
+			want: map[string][]string{"input": {"iif=eth1"}, "forward": {"iif=eth1 oif=eth2", "iif=eth1 oif=eth0"}},
+		},
+		{
+			name: "zone without interfaces emits nothing",
+			rule: config.Rule{Action: config.RuleAccept, Source: "lan", Dest: "svr,dmz"},
+			want: map[string][]string{"forward": {"iif=eth1 oif=eth2"}},
+		},
+		{
+			name: "address list after colon belongs to one zone",
+			rule: config.Rule{Action: config.RuleAccept, Source: "lan", Dest: "net:192.0.2.1,198.51.100.1"},
+			want: map[string][]string{"forward": {"iif=eth1 oif=eth0 daddr=192.0.2.1", "iif=eth1 oif=eth0 daddr=198.51.100.1"}},
+		},
+		{
+			name: "zone:address inside a list",
+			rule: config.Rule{Action: config.RuleAccept, Source: "lan,svr:203.0.113.7", Dest: "fw"},
+			want: map[string][]string{"input": {"iif=eth1", "iif=eth2 saddr=203.0.113.7"}},
+		},
+		{
+			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"}},
+		},
+		{
+			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"}},
+		},
+		{
+			name: "negated address list stays one AND-ed rule",
+			rule: config.Rule{Action: config.RuleAccept, Source: "net:!192.0.2.5,198.51.100.5", Dest: "fw"},
+			want: map[string][]string{"input": {"iif=eth0 !saddr=192.0.2.5 !saddr=198.51.100.5"}},
+		},
+		{
+			name: "zone named like all/any keyword is a plain zone",
+			rule: config.Rule{Action: config.RuleAccept, Source: "lan", Dest: "anycast,net"},
+			want: map[string][]string{"forward": {"iif=eth1 oif=eth3", "iif=eth1 oif=eth0"}},
+		},
+		{
+			name: "interface-less ipsec zone keeps zone-agnostic rule, ip zone skipped",
+			rule: config.Rule{Action: config.RuleAccept, Source: "lan", Dest: "vpn,dmz"},
+			want: map[string][]string{"forward": {"iif=eth1"}},
+		},
+		{
+			name:   "blrule zone list",
+			blrule: &config.BlruleRule{Action: config.BlruleDrop, Source: "net,anycast", Dest: "fw"},
+			want:   map[string][]string{"input": {"iif=eth0", "iif=eth3"}},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.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},
+					"lan": {Type: config.ZoneIP}, "svr": {Type: config.ZoneIP}, "dmz": {Type: config.ZoneIP},
+					"anycast": {Type: config.ZoneIP}, "vpn": {Type: config.ZoneIPSec},
+				},
+				Interfaces: []config.Interface{
+					{Zone: "net", Interface: "eth0"}, {Zone: "lan", Interface: "eth1"}, {Zone: "svr", Interface: "eth2"},
+					{Zone: "anycast", Interface: "eth3"},
+				},
+				Rules:      []config.Rule{tt.rule},
+				PortGroups: make(map[string]config.PortGroup),
+			}
+			tag := "rule:0"
+			if tt.blrule != nil {
+				cfg.Rules, cfg.Blrules, tag = nil, []config.BlruleRule{*tt.blrule}, "blrule:0"
+			}
+			state, err := NewCompiler(cfg).Compile()
+			if err != nil {
+				t.Fatalf("Compile() error: %v", err)
+			}
+			got := map[string][]string{}
+			for chain, rules := range state.Rules {
+				for _, r := range rules {
+					if r.Tag == tag {
+						got[chain] = append(got[chain], describeRule(r))
+					}
+				}
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("rules = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func listCfg(mod func(*config.Config)) *config.Config {
+	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: "all", Dest: "all", Action: config.PolicyDrop}},
+		PortGroups: make(map[string]config.PortGroup),
+	}
+	mod(cfg)
+	return cfg
+}
+
+func taggedRules(state *FirewallState, chain, tag string) []ManagedRule {
+	var out []ManagedRule
+	for _, r := range state.Rules[chain] {
+		if r.Tag == tag {
+			out = append(out, r)
+		}
+	}
+	return out
+}
+
+func TestDiffEngine_FreshApplyKeepsDesiredOrder(t *testing.T) {
+	desired, err := NewCompiler(diffTestConfig("22")).Compile()
+	if err != nil {
+		t.Fatal(err)
+	}
+	var want []string
+	for _, chain := range []string{"forward", "input", "output", "postrouting", "prerouting"} {
+		want = append(want, tags(desired.Rules[chain])...)
+	}
+	for i := 0; i < 20; i++ {
+		cs := computeDiff(&FirewallState{Rules: map[string][]ManagedRule{}}, desired)
+		if got := tags(cs.Add); !reflect.DeepEqual(got, want) {
+			t.Fatalf("add order:\n got %v\nwant %v", got, want)
+		}
+		for _, r := range cs.Add {
+			if r.Before != 0 {
+				t.Fatalf("fresh apply should append, %s has Before=%d", r.Tag, r.Before)
+			}
+		}
+	}
+}
+
+func TestDiffEngine_MiddleChangeInsertsBeforeNextRule(t *testing.T) {
+	current := withHandles(mustCompile(t, diffTestConfig("22")))
+	desired := mustCompile(t, diffTestConfig("2222"))
+
+	cs := computeDiff(current, desired)
+	if len(cs.Add) != 1 || len(cs.Remove) != 1 {
+		t.Fatalf("expected one replace, got:\n%s", cs.Summary())
+	}
+	input := current.Rules["input"]
+	idx := -1
+	for i, r := range input {
+		if r.Tag == "rule:0" {
+			idx = i
+		}
+	}
+	if idx < 1 || idx == len(input)-1 {
+		t.Fatalf("rule:0 at %d is not mid-chain", idx)
+	}
+	if cs.Remove[0].Handle != input[idx].Handle {
+		t.Errorf("removed handle %d, want %d", cs.Remove[0].Handle, input[idx].Handle)
+	}
+	if cs.Add[0].Before != input[idx+1].Handle {
+		t.Errorf("insert before %d, want %d (%s)", cs.Add[0].Before, input[idx+1].Handle, input[idx+1].Tag)
+	}
+}
+
+func TestDiffEngine_ExpandedRuleReplacedInPlace(t *testing.T) {
+	current := withHandles(mustCompile(t, diffTestConfig("22")))
+	if cs := computeDiff(current, mustCompile(t, diffTestConfig("22"))); !cs.Empty() {
+		t.Fatalf("expected empty changeset, got:\n%s", cs.Summary())
+	}
+
+	cfg := diffTestConfig("22")
+	cfg.Rules[4].DPort = config.PortSpec{"53", "853"}
+	desired := mustCompile(t, cfg)
+	cs := computeDiff(current, desired)
+	for _, r := range append(append([]ManagedRule{}, cs.Add...), cs.Remove...) {
+		if r.Tag != "rule:4" {
+			t.Errorf("unexpected change to %s/%s", r.Chain, r.Tag)
+		}
+	}
+	for _, chain := range []string{"input", "forward"} {
+		if n := len(taggedRules(desired, chain, "rule:4")); n != 8 {
+			t.Fatalf("%s: expected 8 expanded rule:4 rules, got %d", chain, n)
+		}
+	}
+
+	applied := applyChangeSet(current, cs)
+	if cs := computeDiff(applied, desired); !cs.Empty() {
+		t.Fatalf("second plan not empty:\n%s", cs.Summary())
+	}
+}
+
+// applyChangeSet mimics the engine: removals by handle, adds inserted before r.Before or appended.
+func applyChangeSet(s *FirewallState, cs *ChangeSet) *FirewallState {
+	gone := map[uint64]bool{}
+	for _, r := range cs.Remove {
+		gone[r.Handle] = true
+	}
+	out := &FirewallState{Rules: map[string][]ManagedRule{}}
+	for chain, rules := range s.Rules {
+		for _, r := range rules {
+			if !gone[r.Handle] {
+				out.Rules[chain] = append(out.Rules[chain], r)
+			}
+		}
+	}
+	h := uint64(10000)
+	for _, r := range cs.Add {
+		r.Handle, h = h, h+1
+		rules := out.Rules[r.Chain]
+		i := len(rules)
+		for j, x := range rules {
+			if r.Before != 0 && x.Handle == r.Before {
+				i = j
+				break
+			}
+		}
+		r.Before = 0
+		out.Rules[r.Chain] = append(rules[:i], append([]ManagedRule{r}, rules[i:]...)...)
+	}
+	return out
+}
+
+func mustCompile(t *testing.T, cfg *config.Config) *FirewallState {
+	t.Helper()
+	s, err := NewCompiler(cfg).Compile()
+	if err != nil {
+		t.Fatal(err)
+	}
+	return s
+}
+
+func TestCompile_ListExpansionCounts(t *testing.T) {
+	tests := []struct {
+		name  string
+		mod   func(*config.Config)
+		chain string
+		tag   string
+		want  int
+	}{
+		{"proto x dport cross product", func(c *config.Config) {
+			c.Rules = []config.Rule{{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "tcp,udp", DPort: config.PortSpec{"80", "443"}}}
+		}, "input", "rule:0", 4},
+		{"sport list", func(c *config.Config) {
+			c.Rules = []config.Rule{{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "tcp", SPort: config.PortSpec{"1024,2048"}}}
+		}, "input", "rule:0", 2},
+		{"snat proto x dport", func(c *config.Config) {
+			c.SNAT = []config.SNATRule{{Action: config.SNATMasquerade, Dest: "eth0", Proto: "tcp,udp", DPort: config.PortSpec{"80,443"}}}
+		}, "postrouting", "snat:0", 4},
+		{"conntrack dport list", func(c *config.Config) {
+			c.Conntrack = []config.ConntrackRule{{Action: config.ConntrackNoTrack, Proto: "udp", DPort: config.PortSpec{"53", "123"}}}
+		}, "prerouting", "conntrack:0:prerouting", 2},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			state, err := NewCompiler(listCfg(tt.mod)).Compile()
+			if err != nil {
+				t.Fatalf("Compile() error: %v", err)
+			}
+			if got := len(taggedRules(state, tt.chain, tt.tag)); got != tt.want {
+				t.Errorf("%s rules in %s = %d, want %d", tt.tag, tt.chain, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestCompile_DNATGetsNoRuleExtras(t *testing.T) {
+	compile := func(mark string) []expr.Any {
+		state, err := NewCompiler(listCfg(func(c *config.Config) {
+			c.Rules = []config.Rule{{Action: config.RuleDNAT, Source: "net", Dest: "fw:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}, Mark: mark}}
+		})).Compile()
+		if err != nil {
+			t.Fatalf("Compile() error: %v", err)
+		}
+		return state.Rules["prerouting"][len(state.Rules["prerouting"])-1].Exprs
+	}
+	if plain, marked := compile(""), compile("0x1"); !reflect.DeepEqual(plain, marked) {
+		t.Errorf("DNAT prerouting rule changed by mark extra: %d exprs vs %d", len(plain), len(marked))
+	}
+}
+
+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"},
+	} {
+		t.Run(r.Source+">"+r.Dest, 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}, "lan": {Type: config.ZoneIP}},
+				Interfaces: []config.Interface{{Zone: "net", Interface: "eth0"}, {Zone: "lan", Interface: "eth1"}},
+				Rules:      []config.Rule{r},
+				PortGroups: make(map[string]config.PortGroup),
+			}
+			if _, err := NewCompiler(cfg).Compile(); err == nil {
+				t.Fatal("Compile() succeeded, want error")
+			}
+		})
+	}
+}
+
+func TestCompile_RejectPerProto(t *testing.T) {
+	tests := []struct {
+		proto string
+		want  []uint32
+	}{
+		{"tcp,udp", []uint32{unix.NFT_REJECT_TCP_RST, unix.NFT_REJECT_ICMPX_UNREACH}},
+		{"6", []uint32{unix.NFT_REJECT_TCP_RST}},
+		{"6,17", []uint32{unix.NFT_REJECT_TCP_RST, unix.NFT_REJECT_ICMPX_UNREACH}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.proto, func(t *testing.T) {
+			state, err := NewCompiler(listCfg(func(c *config.Config) {
+				c.Rules = []config.Rule{{Action: config.RuleReject, Source: "net", Dest: "fw", Proto: tt.proto, DPort: config.PortSpec{"53"}}}
+			})).Compile()
+			if err != nil {
+				t.Fatalf("Compile() error: %v", err)
+			}
+			rules := taggedRules(state, "input", "rule:0")
+			if len(rules) != len(tt.want) {
+				t.Fatalf("got %d rules, want %d", len(rules), len(tt.want))
+			}
+			for i, r := range rules {
+				rej, ok := r.Exprs[len(r.Exprs)-1].(*expr.Reject)
+				if !ok {
+					t.Fatalf("rule %d: last expr %T, want *expr.Reject", i, r.Exprs[len(r.Exprs)-1])
+				}
+				if rej.Type != tt.want[i] {
+					t.Errorf("rule %d: reject type %d, want %d", i, rej.Type, tt.want[i])
+				}
+			}
+		})
+	}
+}
+
+func TestNegatedAddressList(t *testing.T) {
+	exprs, err := matchDestCIDR("!192.0.2.1,198.51.100.1")
+	if err != nil {
+		t.Fatalf("matchDestCIDR error: %v", err)
+	}
+	if len(exprs) != 4 {
+		t.Fatalf("expected 4 exprs, got %d", len(exprs))
+	}
+	for _, i := range []int{1, 3} {
+		if exprs[i].(*expr.Cmp).Op != expr.CmpOpNeq {
+			t.Errorf("expr %d should be CmpOpNeq", i)
+		}
+	}
+}
+
+func TestCompile_ListErrors(t *testing.T) {
+	tests := []struct {
+		name string
+		rule config.Rule
+	}{
+		{"invalid port in list", config.Rule{Proto: "tcp", DPort: config.PortSpec{"80,abc"}}},
+		{"trailing empty proto", config.Rule{Proto: "tcp,", DPort: config.PortSpec{"80"}}},
+		{"leading empty proto", config.Rule{Proto: ",udp", DPort: config.PortSpec{"80"}}},
+		{"empty port element", config.Rule{Proto: "tcp", DPort: config.PortSpec{"80,"}}},
+		{"unknown icmp type", config.Rule{Proto: "icmp", DPort: config.PortSpec{"bogus"}}},
+		{"unknown icmp type with code", config.Rule{Proto: "icmp", DPort: config.PortSpec{"bogus/0"}}},
+		{"invalid icmp code", config.Rule{Proto: "icmp", DPort: config.PortSpec{"destination-unreachable/x"}}},
+		{"icmp code out of range", config.Rule{Proto: "icmp", DPort: config.PortSpec{"3/256"}}},
+		{"unknown proto in list", config.Rule{Proto: "tcp,udpp", DPort: config.PortSpec{"53"}}},
+		{"proto number out of range", config.Rule{Proto: "256"}},
+		{"unknown proto name", config.Rule{Proto: "bogus"}},
+		{"dport with ospf", config.Rule{Proto: "ospf", DPort: config.PortSpec{"80"}}},
+		{"sport with gre", config.Rule{Proto: "gre", SPort: config.PortSpec{"80"}}},
+		{"dport with icmp in proto list", config.Rule{Proto: "icmp,tcp", DPort: config.PortSpec{"80"}}},
+		{"ratelimit with port list", config.Rule{Proto: "tcp", DPort: config.PortSpec{"80,443"}, RateLimit: "10/sec"}},
+		{"connlimit with proto list", config.Rule{Proto: "tcp,udp", DPort: config.PortSpec{"53"}, ConnLimit: "10"}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			r := tt.rule
+			r.Action, r.Source, r.Dest = config.RuleAccept, "net", "fw"
+			_, err := NewCompiler(listCfg(func(c *config.Config) { c.Rules = []config.Rule{r} })).Compile()
+			if err == nil {
+				t.Fatal("Compile() succeeded, want error")
+			}
+		})
+	}
+}
+
+func TestCompile_ICMPList(t *testing.T) {
+	state, err := NewCompiler(listCfg(func(c *config.Config) {
+		c.Rules = []config.Rule{{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "icmp", DPort: config.PortSpec{"echo-request,echo-reply", "destination-unreachable/4"}}}
+	})).Compile()
+	if err != nil {
+		t.Fatalf("Compile() error: %v", err)
+	}
+	var got [][]byte
+	for _, r := range taggedRules(state, "input", "rule:0") {
+		var tc []byte
+		for i, e := range r.Exprs {
+			if p, ok := e.(*expr.Payload); ok && p.Base == expr.PayloadBaseTransportHeader && p.Len == 1 {
+				tc = append(tc, r.Exprs[i+1].(*expr.Cmp).Data[0])
+			}
+		}
+		got = append(got, tc)
+	}
+	want := [][]byte{{8}, {0}, {3, 4}}
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("icmp type/code per rule = %v, want %v", got, want)
+	}
+}
+
+func TestCompile_ColonRanges(t *testing.T) {
+	tests := []struct {
+		name   string
+		mod    func(*config.Config)
+		chain  string
+		tag    string
+		offset uint32
+	}{
+		{"rule sport", func(c *config.Config) {
+			c.Rules = []config.Rule{{Action: config.RuleAccept, Source: "net", Dest: "fw", Proto: "tcp", SPort: config.PortSpec{"1024:2048"}}}
+		}, "input", "rule:0", 0},
+		{"snat sport", func(c *config.Config) {
+			c.SNAT = []config.SNATRule{{Action: config.SNATMasquerade, Dest: "eth0", Proto: "udp", SPort: config.PortSpec{"1024:2048"}}}
+		}, "postrouting", "snat:0", 0},
+		{"snat dport", func(c *config.Config) {
+			c.SNAT = []config.SNATRule{{Action: config.SNATMasquerade, Dest: "eth0", Proto: "tcp", DPort: config.PortSpec{"1024:2048"}}}
+		}, "postrouting", "snat:0", 2},
+		{"conntrack dport", func(c *config.Config) {
+			c.Conntrack = []config.ConntrackRule{{Action: config.ConntrackNoTrack, Proto: "udp", DPort: config.PortSpec{"1024:2048"}}}
+		}, "prerouting", "conntrack:0:prerouting", 2},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			state, err := NewCompiler(listCfg(tt.mod)).Compile()
+			if err != nil {
+				t.Fatalf("Compile() error: %v", err)
+			}
+			rules := taggedRules(state, tt.chain, tt.tag)
+			if len(rules) != 1 {
+				t.Fatalf("got %d %s rules, want 1", len(rules), tt.tag)
+			}
+			var got []string
+			ex := rules[0].Exprs
+			for i, e := range ex {
+				if p, ok := e.(*expr.Payload); ok && p.Base == expr.PayloadBaseTransportHeader && p.Offset == tt.offset && p.Len == 2 && i+2 < len(ex) {
+					lo, hi := ex[i+1].(*expr.Cmp), ex[i+2].(*expr.Cmp)
+					got = append(got, fmt.Sprintf("%d>=%d,%d<=%d", lo.Op, binary.BigEndian.Uint16(lo.Data), hi.Op, binary.BigEndian.Uint16(hi.Data)))
+				}
+			}
+			want := []string{fmt.Sprintf("%d>=1024,%d<=2048", expr.CmpOpGte, expr.CmpOpLte)}
+			if !reflect.DeepEqual(got, want) {
+				t.Errorf("range match = %v, want %v", got, want)
+			}
+		})
+	}
+}

internal/nftables/diff.go

@@ -2,6 +2,8 @@ package nftables
 
 import (
 	"fmt"
+	"reflect"
+	"sort"
 	"strings"
 
 	"github.com/google/nftables/expr"
@@ -12,6 +14,8 @@ type ManagedRule struct {
 	Handle uint64
 	Exprs  []expr.Any
 	Tag    string
+	// Before is the handle of the existing rule an added rule is inserted ahead of; 0 appends.
+	Before uint64
 }
 
 type FirewallState struct {
@@ -32,7 +36,11 @@ func (cs *ChangeSet) Summary() string {
 	if len(cs.Add) > 0 {
 		fmt.Fprintf(&b, "  + %d rule(s) to add\n", len(cs.Add))
 		for _, r := range cs.Add {
-			fmt.Fprintf(&b, "    + [%s] %s\n", r.Chain, r.Tag)
+			if r.Before != 0 {
+				fmt.Fprintf(&b, "    + [%s] %s (before handle %d)\n", r.Chain, r.Tag, r.Before)
+			} else {
+				fmt.Fprintf(&b, "    + [%s] %s\n", r.Chain, r.Tag)
+			}
 		}
 	}
 	if len(cs.Remove) > 0 {
@@ -44,66 +52,82 @@ func (cs *ChangeSet) Summary() string {
 	return b.String()
 }
 
+// Rules are first-match, so order matters: keep the common prefix and suffix of
+// each chain, replace the middle, and insert the new rules before the first kept
+// suffix rule (or append when there is none).
 func computeDiff(current, desired *FirewallState) *ChangeSet {
 	cs := &ChangeSet{}
 
-	currentByTag := make(map[string][]ManagedRule)
-	for _, rules := range current.Rules {
-		for _, r := range rules {
+	chains := make([]string, 0, len(current.Rules)+len(desired.Rules))
+	for c := range current.Rules {
+		chains = append(chains, c)
+	}
+	for c := range desired.Rules {
+		if _, ok := current.Rules[c]; !ok {
+			chains = append(chains, c)
+		}
+	}
+	sort.Strings(chains)
+
+	for _, chain := range chains {
+		var cur []ManagedRule
+		for _, r := range current.Rules[chain] {
 			if r.Tag != "" {
-				currentByTag[r.Tag] = append(currentByTag[r.Tag], r)
+				cur = append(cur, r)
 			}
 		}
-	}
+		want := desired.Rules[chain]
 
-	desiredByTag := make(map[string][]ManagedRule)
-	for _, rules := range desired.Rules {
-		for _, r := range rules {
-			desiredByTag[r.Tag] = append(desiredByTag[r.Tag], r)
+		pre := 0
+		for pre < len(cur) && pre < len(want) && ruleEqual(cur[pre], want[pre]) {
+			pre++
+		}
+		suf := 0
+		for suf < len(cur)-pre && suf < len(want)-pre &&
+			ruleEqual(cur[len(cur)-1-suf], want[len(want)-1-suf]) {
+			suf++
 		}
-	}
 
-	for tag, desiredRules := range desiredByTag {
-		currentRules, exists := currentByTag[tag]
-		if !exists {
-			cs.Add = append(cs.Add, desiredRules...)
-			continue
+		var before uint64
+		if suf > 0 {
+			before = cur[len(cur)-suf].Handle
 		}
-		if !rulesMatch(currentRules, desiredRules) {
-			cs.Remove = append(cs.Remove, currentRules...)
-			cs.Add = append(cs.Add, desiredRules...)
-		}
-	}
-
-	for tag, currentRules := range currentByTag {
-		if _, exists := desiredByTag[tag]; !exists {
-			cs.Remove = append(cs.Remove, currentRules...)
+		cs.Remove = append(cs.Remove, cur[pre:len(cur)-suf]...)
+		for _, r := range want[pre : len(want)-suf] {
+			r.Before = before
+			cs.Add = append(cs.Add, r)
 		}
 	}
 
 	return cs
 }
 
-func rulesMatch(a, b []ManagedRule) bool {
-	if len(a) != len(b) {
-		return false
-	}
-	for i := range a {
-		if a[i].Chain != b[i].Chain {
-			return false
-		}
-		if !exprsEqual(a[i].Exprs, b[i].Exprs) {
-			return false
-		}
-	}
-	return true
+func ruleEqual(a, b ManagedRule) bool {
+	return a.Chain == b.Chain && a.Tag == b.Tag && reflect.DeepEqual(a.Exprs, b.Exprs)
 }
 
-func exprsEqual(a, b []expr.Any) bool {
-	if len(a) != len(b) {
-		return false
+// restoreChangeSet replaces every managed rule in current with the snapshot's,
+// in snapshot order, so a restore cannot reorder rules.
+func restoreChangeSet(current, snap *FirewallState) *ChangeSet {
+	cs := &ChangeSet{}
+	for _, rules := range current.Rules {
+		for _, r := range rules {
+			if r.Tag != "" {
+				cs.Remove = append(cs.Remove, r)
+			}
+		}
 	}
-	as := fmt.Sprintf("%v", a)
-	bs := fmt.Sprintf("%v", b)
-	return as == bs
+	chains := make([]string, 0, len(snap.Rules))
+	for c := range snap.Rules {
+		chains = append(chains, c)
+	}
+	sort.Strings(chains)
+	for _, c := range chains {
+		for _, r := range snap.Rules[c] {
+			if r.Tag != "" {
+				cs.Add = append(cs.Add, r)
+			}
+		}
+	}
+	return cs
 }

internal/nftables/diff_test.go

@@ -0,0 +1,66 @@
+package nftables
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/google/nftables/expr"
+)
+
+func TestRestoreChangeSet(t *testing.T) {
+	accept := []expr.Any{&expr.Verdict{Kind: expr.VerdictAccept}}
+	drop := []expr.Any{&expr.Verdict{Kind: expr.VerdictDrop}}
+
+	snap := &FirewallState{Rules: map[string][]ManagedRule{
+		"input": {
+			{Chain: "input", Tag: "ssh", Exprs: accept, Handle: 4},
+			{Chain: "input", Tag: "web", Exprs: accept, Handle: 5},
+			{Chain: "input", Tag: "", Exprs: drop, Handle: 6},
+		},
+		"forward": {{Chain: "forward", Tag: "fwd", Exprs: accept, Handle: 7}},
+	}}
+	current := &FirewallState{Rules: map[string][]ManagedRule{
+		"input": {
+			{Chain: "input", Tag: "web", Exprs: accept, Handle: 10},
+			{Chain: "input", Tag: "ssh", Exprs: drop, Handle: 11},
+			{Chain: "input", Tag: "", Exprs: drop, Handle: 12},
+		},
+	}}
+
+	cs := restoreChangeSet(current, snap)
+
+	var removed []uint64
+	for _, r := range cs.Remove {
+		removed = append(removed, r.Handle)
+	}
+	if len(removed) != 2 || removed[0] != 10 || removed[1] != 11 {
+		t.Errorf("expected managed handles [10 11] removed, untagged kept; got %v", removed)
+	}
+
+	var added []string
+	for _, r := range cs.Add {
+		added = append(added, r.Tag)
+	}
+	want := []string{"fwd", "ssh", "web"}
+	if len(added) != len(want) {
+		t.Fatalf("added %v, want %v", added, want)
+	}
+	for i := range want {
+		if added[i] != want[i] {
+			t.Fatalf("added %v, want %v (snapshot order per chain)", added, want)
+		}
+	}
+	if !reflect.DeepEqual(cs.Add[1].Exprs, accept) {
+		t.Error("ssh not restored to its snapshot exprs")
+	}
+}
+
+func TestRestoreChangeSetEmptySnapshotRemovesAll(t *testing.T) {
+	current := &FirewallState{Rules: map[string][]ManagedRule{
+		"input": {{Chain: "input", Tag: "x", Handle: 1}},
+	}}
+	cs := restoreChangeSet(current, &FirewallState{Rules: map[string][]ManagedRule{}})
+	if len(cs.Remove) != 1 || len(cs.Add) != 0 {
+		t.Errorf("expected 1 remove 0 add, got %d/%d", len(cs.Remove), len(cs.Add))
+	}
+}

internal/nftables/engine.go

@@ -28,7 +28,8 @@ func (e *Engine) ensureTable() *nftables.Table {
 	})
 }
 
-func (e *Engine) ensureChains(table *nftables.Table) map[string]*nftables.Chain {
+// ensureChains declares the base chains; policies overrides their default policy.
+func (e *Engine) ensureChains(table *nftables.Table, policies map[string]nftables.ChainPolicy) map[string]*nftables.Chain {
 	chains := map[string]*nftables.Chain{
 		"input": {
 			Name:     "input",
@@ -71,6 +72,9 @@ func (e *Engine) ensureChains(table *nftables.Table) map[string]*nftables.Chain
 	}
 
 	for name, chain := range chains {
+		if p, ok := policies[name]; ok {
+			chain.Policy = policyPtr(p)
+		}
 		chains[name] = e.conn.AddChain(chain)
 	}
 	return chains
@@ -93,8 +97,12 @@ func (e *Engine) Plan() (*ChangeSet, error) {
 }
 
 func (e *Engine) Apply(changes *ChangeSet) error {
+	return e.apply(changes, nil)
+}
+
+func (e *Engine) apply(changes *ChangeSet, policies map[string]nftables.ChainPolicy) error {
 	table := e.ensureTable()
-	chains := e.ensureChains(table)
+	chains := e.ensureChains(table, policies)
 
 	for _, r := range changes.Remove {
 		e.conn.DelRule(&nftables.Rule{
@@ -109,12 +117,18 @@ func (e *Engine) Apply(changes *ChangeSet) error {
 		if !ok {
 			return fmt.Errorf("unknown chain %q", r.Chain)
 		}
-		e.conn.AddRule(&nftables.Rule{
+		rule := &nftables.Rule{
 			Table:    table,
 			Chain:    chain,
 			Exprs:    r.Exprs,
 			UserData: []byte(r.Tag),
-		})
+		}
+		if r.Before != 0 {
+			rule.Position = r.Before
+			e.conn.InsertRule(rule)
+		} else {
+			e.conn.AddRule(rule)
+		}
 	}
 
 	return e.conn.Flush()
@@ -135,31 +149,32 @@ func (e *Engine) Flush() error {
 	return nil
 }
 
+func (e *Engine) findTable() (*nftables.Table, error) {
+	tables, err := e.conn.ListTables()
+	if err != nil {
+		return nil, fmt.Errorf("listing tables: %w", err)
+	}
+	for _, t := range tables {
+		if t.Name == e.cfg.Settings.TableName && t.Family == nftables.TableFamilyINet {
+			return t, nil
+		}
+	}
+	return nil, nil
+}
+
 func (e *Engine) readCurrentState() (*FirewallState, error) {
 	state := &FirewallState{
 		Rules: make(map[string][]ManagedRule),
 	}
 
-	tables, err := e.conn.ListTables()
-	if err != nil {
-		return state, nil
-	}
-
-	var ourTable *nftables.Table
-	for _, t := range tables {
-		if t.Name == e.cfg.Settings.TableName && t.Family == nftables.TableFamilyINet {
-			ourTable = t
-			break
-		}
-	}
-
-	if ourTable == nil {
-		return state, nil
+	ourTable, err := e.findTable()
+	if err != nil || ourTable == nil {
+		return state, err
 	}
 
 	chains, err := e.conn.ListChainsOfTableFamily(nftables.TableFamilyINet)
 	if err != nil {
-		return state, nil
+		return nil, fmt.Errorf("listing chains: %w", err)
 	}
 
 	for _, chain := range chains {
@@ -168,7 +183,7 @@ func (e *Engine) readCurrentState() (*FirewallState, error) {
 		}
 		rules, err := e.conn.GetRules(ourTable, chain)
 		if err != nil {
-			continue
+			return nil, fmt.Errorf("listing rules of %s: %w", chain.Name, err)
 		}
 		for _, rule := range rules {
 			state.Rules[chain.Name] = append(state.Rules[chain.Name], ManagedRule{
@@ -183,6 +198,72 @@ func (e *Engine) readCurrentState() (*FirewallState, error) {
 	return state, nil
 }
 
+// Snapshot is the tomswall table as captured live, serialisable so a revert
+// survives the process that took it.
+type Snapshot struct {
+	Table    string                          `json:"table"`
+	Present  bool                            `json:"present"`
+	Policies map[string]nftables.ChainPolicy `json:"policies,omitempty"`
+	Rules    map[string][]SnapshotRule       `json:"rules,omitempty"`
+}
+
+// SnapshotRule is a managed rule with its expressions in netlink wire format.
+type SnapshotRule struct {
+	Tag   string   `json:"tag"`
+	Exprs [][]byte `json:"exprs"`
+}
+
+// Snapshot captures the live tomswall table so Restore can roll back to it.
+func (e *Engine) Snapshot() (*Snapshot, error) {
+	snap := &Snapshot{Table: e.cfg.Settings.TableName}
+	t, err := e.findTable()
+	if err != nil || t == nil {
+		return snap, err
+	}
+	snap.Present = true
+
+	chains, err := e.conn.ListChainsOfTableFamily(nftables.TableFamilyINet)
+	if err != nil {
+		return nil, fmt.Errorf("listing chains: %w", err)
+	}
+	snap.Policies = make(map[string]nftables.ChainPolicy)
+	for _, c := range chains {
+		if c.Table.Name == snap.Table && c.Policy != nil {
+			snap.Policies[c.Name] = *c.Policy
+		}
+	}
+
+	state, err := e.readCurrentState()
+	if err != nil {
+		return nil, err
+	}
+	snap.Rules, err = encodeState(state)
+	if err != nil {
+		return nil, err
+	}
+	return snap, nil
+}
+
+// Restore atomically returns the tomswall table to the snapshot: rule order
+// and chain policies included, or removed if it was absent.
+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)
+	}
+	if !s.Present {
+		return e.Flush()
+	}
+	want, err := decodeState(s.Rules)
+	if err != nil {
+		return err
+	}
+	current, err := e.readCurrentState()
+	if err != nil {
+		return err
+	}
+	return e.apply(restoreChangeSet(current, want), s.Policies)
+}
+
 func policyPtr(p nftables.ChainPolicy) *nftables.ChainPolicy {
 	return &p
 }

internal/nftables/snapshot.go

@@ -0,0 +1,119 @@
+package nftables
+
+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{} },
+	"range":        func() expr.Any { return &expr.Range{} },
+	"meta":         func() expr.Any { return &expr.Meta{} },
+	"cmp":          func() expr.Any { return &expr.Cmp{} },
+	"counter":      func() expr.Any { return &expr.Counter{} },
+	"objref":       func() expr.Any { return &expr.Objref{} },
+	"payload":      func() expr.Any { return &expr.Payload{} },
+	"lookup":       func() expr.Any { return &expr.Lookup{} },
+	"immediate":    func() expr.Any { return &expr.Immediate{} },
+	"bitwise":      func() expr.Any { return &expr.Bitwise{} },
+	"redir":        func() expr.Any { return &expr.Redir{} },
+	"nat":          func() expr.Any { return &expr.NAT{} },
+	"limit":        func() expr.Any { return &expr.Limit{} },
+	"quota":        func() expr.Any { return &expr.Quota{} },
+	"dynset":       func() expr.Any { return &expr.Dynset{} },
+	"log":          func() expr.Any { return &expr.Log{} },
+	"exthdr":       func() expr.Any { return &expr.Exthdr{} },
+	"connlimit":    func() expr.Any { return &expr.Connlimit{} },
+	"queue":        func() expr.Any { return &expr.Queue{} },
+	"flow_offload": func() expr.Any { return &expr.FlowOffload{} },
+	"reject":       func() expr.Any { return &expr.Reject{} },
+	"masq":         func() expr.Any { return &expr.Masq{} },
+	"hash":         func() expr.Any { return &expr.Hash{} },
+	"notrack":      func() expr.Any { return &expr.Notrack{} },
+}
+
+func encodeState(state *FirewallState) (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)
+				if err != nil {
+					return nil, fmt.Errorf("encoding %s rule %q: %w", chain, r.Tag, err)
+				}
+				sr.Exprs = append(sr.Exprs, b)
+			}
+			out[chain] = append(out[chain], sr)
+		}
+	}
+	return out, nil
+}
+
+func decodeState(rules map[string][]SnapshotRule) (*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)
+				if err != nil {
+					return nil, fmt.Errorf("decoding %s rule %q: %w", chain, sr.Tag, err)
+				}
+				r.Exprs = append(r.Exprs, e)
+			}
+			state.Rules[chain] = append(state.Rules[chain], r)
+		}
+	}
+	return state, nil
+}
+
+// decodeExpr reverses expr.Marshal, as google/nftables does when reading rules.
+func decodeExpr(b []byte) (expr.Any, error) {
+	ad, err := netlink.NewAttributeDecoder(b)
+	if err != nil {
+		return nil, err
+	}
+	ad.ByteOrder = binary.BigEndian
+	var name string
+	var data []byte
+	for ad.Next() {
+		switch ad.Type() {
+		case unix.NFTA_EXPR_NAME:
+			name = ad.String()
+		case unix.NFTA_EXPR_DATA:
+			data = ad.Bytes()
+		}
+	}
+	if err := ad.Err(); err != nil {
+		return nil, err
+	}
+	newExpr, ok := exprByName[name]
+	if !ok {
+		return nil, fmt.Errorf("unsupported expression %q", name)
+	}
+	e := newExpr()
+	if name == "notrack" {
+		return e, nil
+	}
+	if err := expr.Unmarshal(inet, 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 {
+			return nil, err
+		}
+		return v, nil
+	}
+	return e, nil
+}

internal/nftables/snapshot_test.go

@@ -0,0 +1,245 @@
+package nftables
+
+import (
+	"bytes"
+	"encoding/binary"
+	"encoding/json"
+	"reflect"
+	"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"
+)
+
+func TestSnapshotRulesRoundTrip(t *testing.T) {
+	exprs := []expr.Any{
+		&expr.Meta{Key: expr.MetaKeyL4PROTO, Register: 1},
+		&expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{unix.IPPROTO_TCP}},
+		&expr.Payload{DestRegister: 1, Base: expr.PayloadBaseTransportHeader, Offset: 2, Len: 2},
+		&expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{0, 22}},
+		&expr.Ct{Register: 1, Key: expr.CtKeySTATE},
+		&expr.Notrack{},
+		&expr.Verdict{Kind: expr.VerdictAccept},
+	}
+	state := &FirewallState{Rules: map[string][]ManagedRule{
+		"input": {{Chain: "input", Tag: "ssh", Exprs: exprs}, {Chain: "input", Tag: "drop", Exprs: []expr.Any{&expr.Verdict{Kind: expr.VerdictDrop}}}},
+	}}
+
+	rules, err := encodeState(state)
+	if err != nil {
+		t.Fatal(err)
+	}
+	b, err := json.Marshal(&Snapshot{Table: "tomswall", Present: true, Rules: rules,
+		Policies: map[string]nftables.ChainPolicy{"input": nftables.ChainPolicyAccept}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	var snap Snapshot
+	if err := json.Unmarshal(b, &snap); err != nil {
+		t.Fatal(err)
+	}
+	if snap.Policies["input"] != nftables.ChainPolicyAccept {
+		t.Errorf("policy lost: %v", snap.Policies)
+	}
+	got, err := decodeState(snap.Rules)
+	if err != nil {
+		t.Fatal(err)
+	}
+	in := got.Rules["input"]
+	if len(in) != 2 || in[0].Tag != "ssh" || in[1].Tag != "drop" {
+		t.Fatalf("rules/order lost: %+v", in)
+	}
+	if !reflect.DeepEqual(in[0].Exprs, exprs) {
+		t.Errorf("exprs changed:\n got %#v\nwant %#v", in[0].Exprs, exprs)
+	}
+	if _, ok := in[1].Exprs[0].(*expr.Verdict); !ok {
+		t.Errorf("verdict decoded as %T", in[1].Exprs[0])
+	}
+}
+
+func TestEnsureChainsPolicyOverride(t *testing.T) {
+	e := testEngine(t, nil)
+	chains := e.ensureChains(e.ensureTable(), map[string]nftables.ChainPolicy{"input": nftables.ChainPolicyAccept})
+	if *chains["input"].Policy != nftables.ChainPolicyAccept {
+		t.Error("input policy not overridden")
+	}
+	if *chains["forward"].Policy != nftables.ChainPolicyDrop {
+		t.Error("forward policy should keep its default")
+	}
+}
+
+func TestSnapshotAndRestoreAbsentTable(t *testing.T) {
+	tablePresent := false
+	var sent []netlink.HeaderType
+	e := testEngine(t, func(req []netlink.Message) ([]netlink.Message, error) {
+		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}
+				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
+			}
+		}
+		return nil, nil
+	})
+
+	snap, err := e.Snapshot()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if snap.Present || snap.Table != "tomswall" {
+		t.Fatalf("want absent tomswall snapshot, got %+v", snap)
+	}
+
+	// The try created the table; restoring the absent snapshot deletes it.
+	tablePresent = true
+	sent = nil
+	if err := e.Restore(snap); err != nil {
+		t.Fatal(err)
+	}
+	deleted := false
+	for _, ht := range sent {
+		deleted = deleted || ht == nftType(unix.NFT_MSG_DELTABLE)
+	}
+	if !deleted {
+		t.Errorf("table not deleted; sent %v", sent)
+	}
+}
+
+func TestRestorePresentTable(t *testing.T) {
+	want := []SnapshotRule{}
+	for _, r := range []ManagedRule{
+		{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}}})
+		if err != nil {
+			t.Fatal(err)
+		}
+		want = append(want, enc["input"]...)
+	}
+	snap := &Snapshot{Table: "tomswall", Present: true,
+		Policies: map[string]nftables.ChainPolicy{"input": nftables.ChainPolicyAccept},
+		Rules:    map[string][]SnapshotRule{"input": want}}
+
+	// Live state: the tried ruleset left one managed rule (handle 7) in input.
+	attrs := func(a ...netlink.Attribute) []byte {
+		b, err := netlink.MarshalAttributes(a)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return append([]byte{inet, 0, 0, 0}, b...)
+	}
+	handle := make([]byte, 8)
+	binary.BigEndian.PutUint64(handle, 7)
+	var batch []netlink.Message
+	e := testEngine(t, func(req []netlink.Message) ([]netlink.Message, error) {
+		if len(req) == 0 {
+			return nil, nil
+		}
+		reply := func(msg int, data []byte) ([]netlink.Message, error) {
+			return []netlink.Message{{Header: netlink.Header{Type: nftType(msg), Sequence: req[0].Header.Sequence}, Data: data}}, nil
+		}
+		switch req[0].Header.Type {
+		case nftType(unix.NFT_MSG_GETTABLE):
+			return reply(unix.NFT_MSG_NEWTABLE, attrs(netlink.Attribute{Type: unix.NFTA_TABLE_NAME, Data: []byte("tomswall\x00")}))
+		case nftType(unix.NFT_MSG_GETCHAIN):
+			return reply(unix.NFT_MSG_NEWCHAIN, attrs(
+				netlink.Attribute{Type: unix.NFTA_CHAIN_TABLE, Data: []byte("tomswall\x00")},
+				netlink.Attribute{Type: unix.NFTA_CHAIN_NAME, Data: []byte("input\x00")}))
+		case nftType(unix.NFT_MSG_GETRULE):
+			return reply(unix.NFT_MSG_NEWRULE, attrs(
+				netlink.Attribute{Type: unix.NFTA_RULE_TABLE, Data: []byte("tomswall\x00")},
+				netlink.Attribute{Type: unix.NFTA_RULE_CHAIN, Data: []byte("input\x00")},
+				netlink.Attribute{Type: unix.NFTA_RULE_HANDLE, Data: handle},
+				netlink.Attribute{Type: unix.NFTA_RULE_USERDATA, Data: []byte("tried")}))
+		}
+		batch = append(batch, req...)
+		return nil, nil
+	})
+
+	if err := e.Restore(snap); err != nil {
+		t.Fatal(err)
+	}
+
+	var deleted []uint64
+	var added []SnapshotRule
+	policy := map[string]uint32{}
+	for _, m := range batch {
+		ad, err := netlink.NewAttributeDecoder(m.Data[4:])
+		if err != nil {
+			t.Fatal(err)
+		}
+		ad.ByteOrder = binary.BigEndian
+		var name string
+		var r SnapshotRule
+		var h uint64
+		var pol *uint32
+		for ad.Next() {
+			switch {
+			case m.Header.Type == nftType(unix.NFT_MSG_NEWCHAIN) && ad.Type() == unix.NFTA_CHAIN_NAME:
+				name = ad.String()
+			case m.Header.Type == nftType(unix.NFT_MSG_NEWCHAIN) && ad.Type() == unix.NFTA_CHAIN_POLICY:
+				v := ad.Uint32()
+				pol = &v
+			case m.Header.Type == nftType(unix.NFT_MSG_DELRULE) && ad.Type() == unix.NFTA_RULE_HANDLE:
+				h = ad.Uint64()
+			case m.Header.Type == nftType(unix.NFT_MSG_NEWRULE) && ad.Type() == unix.NFTA_RULE_USERDATA:
+				r.Tag = string(ad.Bytes())
+			case m.Header.Type == nftType(unix.NFT_MSG_NEWRULE) && ad.Type() == unix.NFTA_RULE_EXPRESSIONS:
+				ad.Nested(func(nad *netlink.AttributeDecoder) error {
+					for nad.Next() {
+						r.Exprs = append(r.Exprs, bytes.Clone(nad.Bytes()))
+					}
+					return nil
+				})
+			}
+		}
+		switch m.Header.Type {
+		case nftType(unix.NFT_MSG_NEWCHAIN):
+			if pol != nil {
+				policy[name] = *pol
+			}
+		case nftType(unix.NFT_MSG_DELRULE):
+			deleted = append(deleted, h)
+		case nftType(unix.NFT_MSG_NEWRULE):
+			added = append(added, r)
+		}
+	}
+
+	if !reflect.DeepEqual(deleted, []uint64{7}) {
+		t.Errorf("deleted handles %v, want [7]", deleted)
+	}
+	if !reflect.DeepEqual(added, want) {
+		t.Errorf("restored rules differ from snapshot:\n got %+v\nwant %+v", added, want)
+	}
+	if policy["input"] != uint32(nftables.ChainPolicyAccept) || policy["forward"] != uint32(nftables.ChainPolicyDrop) {
+		t.Errorf("chain policies %v: input must be restored to accept, forward keep drop", policy)
+	}
+}
+
+func TestRestoreRejectsOtherTable(t *testing.T) {
+	e := testEngine(t, nil)
+	if err := e.Restore(&Snapshot{Table: "other"}); err == nil {
+		t.Error("expected table mismatch error")
+	}
+}
+
+func nftType(msg int) netlink.HeaderType {
+	return netlink.HeaderType(unix.NFNL_SUBSYS_NFTABLES<<8 | msg)
+}
+
+func testEngine(t *testing.T, dial func([]netlink.Message) ([]netlink.Message, error)) *Engine {
+	if dial == nil {
+		dial = func([]netlink.Message) ([]netlink.Message, error) { return nil, nil }
+	}
+	conn, err := nftables.New(nftables.WithTestDial(dial))
+	if err != nil {
+		t.Fatal(err)
+	}
+	return &Engine{cfg: &config.Config{Settings: config.Settings{TableName: "tomswall"}}, conn: conn}
+}

internal/tryapply/tryapply.go

@@ -0,0 +1,200 @@
+// Package tryapply keeps the state of a pending 'tomswall try' on disk so the
+// revert survives the try process, backed by a transient systemd timer.
+package tryapply
+
+import (
+	"crypto/rand"
+	"encoding/hex"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"syscall"
+	"time"
+
+	"git.unkin.net/unkin/tomswall/internal/config"
+	"git.unkin.net/unkin/tomswall/internal/nftables"
+)
+
+// Unit is the transient systemd unit that reverts an unconfirmed try.
+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 = 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 = func(s *nftables.Snapshot) error {
+		engine, err := nftables.NewEngine(&config.Config{Settings: config.Settings{TableName: s.Table}})
+		if err != nil {
+			return err
+		}
+		return engine.Restore(s)
+	}
+)
+
+// ErrPending means a try awaits confirmation; nothing else may apply meanwhile.
+var ErrPending = errors.New("a 'tomswall try' is pending; run 'tomswall confirm' to keep it or 'tomswall revert' to restore the previous ruleset (also the recovery if an automatic revert failed)")
+
+type pending struct {
+	ID       string             `json:"id"`
+	PID      int                `json:"pid"`
+	Snapshot *nftables.Snapshot `json:"snapshot"`
+}
+
+func snapshotPath() string { return filepath.Join(Dir, "try-snapshot.json") }
+
+// Acquire takes the exclusive try lock, failing with ErrPending while a try is unconfirmed.
+func Acquire() (unlock func(), err error) {
+	unlock, err = lock()
+	if err != nil {
+		return nil, err
+	}
+	if _, err := os.Stat(snapshotPath()); err == nil {
+		unlock()
+		return nil, ErrPending
+	}
+	return unlock, nil
+}
+
+func lock() (func(), error) {
+	if err := os.MkdirAll(Dir, 0o755); err != nil {
+		return nil, err
+	}
+	f, err := os.OpenFile(filepath.Join(Dir, "try.lock"), os.O_CREATE|os.O_RDWR, 0o600)
+	if err != nil {
+		return nil, err
+	}
+	if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil {
+		f.Close()
+		return nil, fmt.Errorf("locking %s: %w", f.Name(), err)
+	}
+	return func() { f.Close() }, nil
+}
+
+// Arm persists snap and schedules an out-of-process revert after delay,
+// returning the try ID that scopes later reverts to this try.
+// The caller must hold the lock from Acquire.
+func Arm(snap *nftables.Snapshot, pid int, delay time.Duration) (string, error) {
+	raw := make([]byte, 8)
+	if _, err := rand.Read(raw); err != nil {
+		return "", err
+	}
+	id := hex.EncodeToString(raw)
+	b, err := json.Marshal(pending{ID: id, PID: pid, Snapshot: snap})
+	if err != nil {
+		return "", err
+	}
+	f, err := os.CreateTemp(Dir, ".try-snapshot-*")
+	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
+	}
+
+	exe, err := os.Executable()
+	if err != nil {
+		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,
+		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
+}
+
+// Discard drops the pending snapshot and timer without restoring. The caller must hold the lock.
+func Discard() error {
+	_ = disarm()
+	if err := os.Remove(snapshotPath()); err != nil && !os.IsNotExist(err) {
+		return err
+	}
+	return nil
+}
+
+func discardWith(err error) error {
+	if derr := Discard(); derr != nil {
+		return fmt.Errorf("%w (discarding snapshot: %v)", err, derr)
+	}
+	return err
+}
+
+func disarm() error {
+	return run("systemctl", "stop", Unit+".timer")
+}
+
+// Confirm keeps the tried ruleset. ok is false when no try was pending, i.e.
+// it was already reverted; pid is the waiting try process, if any.
+func Confirm() (pid int, ok bool, err error) {
+	unlock, err := lock()
+	if err != nil {
+		return 0, false, err
+	}
+	defer unlock()
+	p, err := load()
+	if err != nil || p == nil {
+		return 0, false, err
+	}
+	return p.PID, true, Discard()
+}
+
+// Revert restores the pending snapshot. A non-empty id only reverts that try,
+// so a stale timer cannot undo a newer one. reverted is false when nothing
+// matching was pending (already confirmed or reverted). A failed restore keeps
+// the snapshot so 'tomswall revert' can retry.
+func Revert(id string) (reverted bool, err error) {
+	unlock, err := lock()
+	if err != nil {
+		return false, err
+	}
+	defer unlock()
+	p, err := load()
+	if err != nil || p == nil || (id != "" && p.ID != id) {
+		return false, err
+	}
+	if err := restore(p.Snapshot); err != nil {
+		return false, fmt.Errorf("restoring snapshot: %w", err)
+	}
+	return true, Discard()
+}
+
+func load() (*pending, error) {
+	b, err := os.ReadFile(snapshotPath())
+	if os.IsNotExist(err) {
+		return nil, nil
+	}
+	if err != nil {
+		return nil, err
+	}
+	var p pending
+	if err := json.Unmarshal(b, &p); err != nil {
+		return nil, fmt.Errorf("parsing %s: %w", snapshotPath(), err)
+	}
+	if p.Snapshot == nil {
+		return nil, fmt.Errorf("%s has no snapshot", snapshotPath())
+	}
+	return &p, nil
+}

internal/tryapply/tryapply_test.go

@@ -0,0 +1,211 @@
+package tryapply
+
+import (
+	"errors"
+	"os"
+	"reflect"
+	"strings"
+	"testing"
+	"time"
+
+	"git.unkin.net/unkin/tomswall/internal/nftables"
+)
+
+func setup(t *testing.T) *[]string {
+	t.Helper()
+	Dir = t.TempDir()
+	var cmds []string
+	orig := run
+	run = func(name string, args ...string) error {
+		cmds = append(cmds, name+" "+strings.Join(args, " "))
+		return nil
+	}
+	t.Cleanup(func() { run = orig })
+	return &cmds
+}
+
+func arm(t *testing.T, snap *nftables.Snapshot) string {
+	t.Helper()
+	unlock, err := Acquire()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer unlock()
+	id, err := Arm(snap, 4242, 90*time.Second)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return id
+}
+
+func TestArmPersistsSnapshotAndTimer(t *testing.T) {
+	cmds := setup(t)
+	snap := &nftables.Snapshot{Table: "tomswall", Present: true,
+		Rules: map[string][]nftables.SnapshotRule{"input": {{Tag: "ssh", Exprs: [][]byte{{1, 2, 3}}}}}}
+	id := arm(t, snap)
+
+	info, err := os.Stat(snapshotPath())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if info.Mode().Perm() != 0o600 {
+		t.Errorf("snapshot mode %v, want 0600", info.Mode().Perm())
+	}
+	p, err := load()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p.ID != id || id == "" || p.PID != 4242 || !reflect.DeepEqual(p.Snapshot, snap) {
+		t.Errorf("round trip mismatch: %+v", p)
+	}
+	last := (*cmds)[len(*cmds)-1]
+	if !strings.HasPrefix(last, "systemd-run ") || !strings.Contains(last, "--unit "+Unit) ||
+		!strings.Contains(last, "--on-active=90s") || !strings.HasSuffix(last, " revert --id "+id) {
+		t.Errorf("unexpected arm command %q", last)
+	}
+}
+
+func TestAbsentTableSnapshotRoundTrip(t *testing.T) {
+	setup(t)
+	arm(t, &nftables.Snapshot{Table: "tomswall"})
+	p, err := load()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p.Snapshot.Present || p.Snapshot.Table != "tomswall" {
+		t.Errorf("absent table not preserved: %+v", p.Snapshot)
+	}
+}
+
+func TestAcquireRefusesWhilePending(t *testing.T) {
+	setup(t)
+	arm(t, &nftables.Snapshot{Table: "tomswall"})
+	_, err := Acquire()
+	if !errors.Is(err, ErrPending) {
+		t.Fatalf("second try: got %v, want ErrPending", err)
+	}
+	if !strings.Contains(err.Error(), "tomswall revert") {
+		t.Errorf("ErrPending does not name the recovery: %v", err)
+	}
+}
+
+func TestArmFailureDiscardsSnapshot(t *testing.T) {
+	setup(t)
+	run = func(name string, args ...string) error {
+		if name == "systemd-run" {
+			return errors.New("no systemd")
+		}
+		return nil
+	}
+	unlock, err := Acquire()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer unlock()
+	if _, err := Arm(&nftables.Snapshot{Table: "tomswall"}, 1, time.Minute); err == nil {
+		t.Fatal("expected arm error")
+	}
+	if _, err := os.Stat(snapshotPath()); !os.IsNotExist(err) {
+		t.Error("snapshot left behind without a revert timer")
+	}
+}
+
+func TestConfirmPendingDisarms(t *testing.T) {
+	cmds := setup(t)
+	arm(t, &nftables.Snapshot{Table: "tomswall"})
+	pid, ok, err := Confirm()
+	if err != nil || !ok || pid != 4242 {
+		t.Fatalf("Confirm = %d, %v, %v", pid, ok, err)
+	}
+	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)
+	}
+	unlock, err := Acquire()
+	if err != nil {
+		t.Fatalf("new try refused after confirm: %v", err)
+	}
+	unlock()
+}
+
+func TestConfirmAfterRevertFails(t *testing.T) {
+	setup(t)
+	_, ok, err := Confirm()
+	if err != nil || ok {
+		t.Fatalf("Confirm with nothing pending = %v, %v; want not ok", ok, err)
+	}
+	reverted, err := Revert("")
+	if err != nil || reverted {
+		t.Fatalf("Revert with nothing pending = %v, %v", reverted, err)
+	}
+}
+
+func stubRestore(t *testing.T, err error) *[]*nftables.Snapshot {
+	t.Helper()
+	var got []*nftables.Snapshot
+	orig := restore
+	restore = func(s *nftables.Snapshot) error {
+		got = append(got, s)
+		return err
+	}
+	t.Cleanup(func() { restore = orig })
+	return &got
+}
+
+func TestRevertPendingRestoresAndDisarms(t *testing.T) {
+	cmds := setup(t)
+	restored := stubRestore(t, nil)
+	snap := &nftables.Snapshot{Table: "tomswall", Present: true,
+		Rules: map[string][]nftables.SnapshotRule{"input": {{Tag: "ssh", Exprs: [][]byte{{1}}}}}}
+	id := arm(t, snap)
+
+	reverted, err := Revert(id)
+	if err != nil || !reverted {
+		t.Fatalf("Revert = %v, %v", reverted, 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 TestRevertFailureKeepsSnapshot(t *testing.T) {
+	setup(t)
+	boom := errors.New("netlink down")
+	stubRestore(t, boom)
+	id := arm(t, &nftables.Snapshot{Table: "tomswall"})
+
+	if _, err := Revert(id); !errors.Is(err, boom) {
+		t.Fatalf("Revert error = %v, want %v", err, boom)
+	}
+	if _, err := os.Stat(snapshotPath()); err != nil {
+		t.Fatalf("snapshot gone after failed revert: %v", err)
+	}
+	if _, err := Acquire(); !errors.Is(err, ErrPending) {
+		t.Errorf("failed revert must keep the try pending, got %v", err)
+	}
+}
+
+func TestRevertStaleIDIgnored(t *testing.T) {
+	setup(t)
+	restored := stubRestore(t, nil)
+	arm(t, &nftables.Snapshot{Table: "tomswall"})
+
+	reverted, err := Revert("stale-try")
+	if err != nil || reverted {
+		t.Fatalf("stale Revert = %v, %v; want no-op", reverted, err)
+	}
+	if len(*restored) != 0 {
+		t.Error("stale timer restored a newer try's snapshot")
+	}
+	if _, err := os.Stat(snapshotPath()); err != nil {
+		t.Errorf("newer try's snapshot removed: %v", err)
+	}
+}