From b5c3e84b812f08b2ebcd797129c6cba020ab6cb6 Mon Sep 17 00:00:00 2001 From: pbardea Date: Thu, 30 Jul 2026 21:40:29 +0000 Subject: [PATCH 1/3] lib: make restarts hitless by adopting the kernel WireGuard dataplane Co-authored-by: Codesmith Staging --- lib/iputils.go | 21 +++++ lib/iputils_test.go | 26 +++++++ lib/server.go | 177 +++++++++++++++++++++++++++++++++++++++--- lib/server_manager.go | 14 +++- lib/server_test.go | 75 ++++++++++++++++++ 5 files changed, 302 insertions(+), 11 deletions(-) create mode 100644 lib/server_test.go diff --git a/lib/iputils.go b/lib/iputils.go index 28d3525..9bb614f 100644 --- a/lib/iputils.go +++ b/lib/iputils.go @@ -185,6 +185,27 @@ func (ipa *IpAllocator) Allocate() netip.Addr { } } +// Claim marks the given specific IP address as allocated, so that Allocate +// will never hand it out. It is used to rebuild allocator state from an +// external source of truth (e.g. the kernel WireGuard peer list after a +// restart). +// +// It returns false, without modifying state, if the address is outside the +// allocator's prefix or is already allocated. +func (ipa *IpAllocator) Claim(addr netip.Addr) bool { + ipa.mu.Lock() + defer ipa.mu.Unlock() + + if !ipa.prefix.Contains(addr) { + return false + } + if _, ok := ipa.allocated[addr]; ok { + return false + } + ipa.allocated[addr] = struct{}{} + return true +} + // Free marks the given IP address as available for allocation. func (ipa *IpAllocator) Free(addr netip.Addr) bool { ipa.mu.Lock() diff --git a/lib/iputils_test.go b/lib/iputils_test.go index ae5fec3..5fb38b8 100644 --- a/lib/iputils_test.go +++ b/lib/iputils_test.go @@ -7,6 +7,32 @@ import ( "github.com/stretchr/testify/assert" ) +func TestIpAllocatorClaim(t *testing.T) { + ipa := NewIpAllocator(netip.MustParsePrefix("192.168.0.0/24")) + + // Claiming a free address succeeds; claiming it again fails. + addr := netip.MustParseAddr("192.168.0.5") + assert.True(t, ipa.Claim(addr)) + assert.False(t, ipa.Claim(addr)) + + // Claiming an address outside the prefix fails. + assert.False(t, ipa.Claim(netip.MustParseAddr("192.168.1.5"))) + + // Claiming an address returned by Allocate fails. + first := ipa.Allocate() + assert.Equal(t, netip.MustParseAddr("192.168.0.1"), first) + assert.False(t, ipa.Claim(first)) + + // Allocate skips claimed addresses. + assert.True(t, ipa.Claim(netip.MustParseAddr("192.168.0.2"))) + assert.True(t, ipa.Claim(netip.MustParseAddr("192.168.0.3"))) + assert.Equal(t, netip.MustParseAddr("192.168.0.4"), ipa.Allocate()) + + // A freed address can be claimed again. + assert.True(t, ipa.Free(addr)) + assert.True(t, ipa.Claim(addr)) +} + func TestAfterOneIpBlock(t *testing.T) { ip1 := netip.AddrFrom4([4]byte{192, 168, 1, 0}) ip2 := netip.AddrFrom4([4]byte{192, 168, 2, 0}) diff --git a/lib/server.go b/lib/server.go index b7cd375..f5fccec 100644 --- a/lib/server.go +++ b/lib/server.go @@ -254,25 +254,62 @@ func (srv *Server) Ifname() string { return fmt.Sprintf("vprox%d", srv.Index) } +// StartWireguard brings up the server's WireGuard interface. +// +// If an interface with the expected name already exists (e.g. left behind by +// a previous vprox process across a restart), it is adopted rather than +// recreated, so that its kernel peer list, and therefore the tunnels of every +// registered peer, survive the restart. The interface is only deleted and +// recreated if its address doesn't match the expected CIDR, as a safety net +// for configuration changes. func (srv *Server) StartWireguard() error { ifname := srv.Ifname() - link := &linkWireguard{LinkAttrs: netlink.LinkAttrs{Name: ifname}} - _ = netlink.LinkDel(link) // remove if it already exists - err := netlink.LinkAdd(link) - if err != nil { - return fmt.Errorf("failed to create WireGuard device: %v", err) + + link, err := netlink.LinkByName(ifname) + if err == nil { + if srv.canAdoptLink(link) { + log.Printf("[%v] adopting existing WireGuard device %s", srv.BindAddr, ifname) + } else { + log.Printf("[%v] existing device %s doesn't match config; recreating it", + srv.BindAddr, ifname) + _ = netlink.LinkDel(link) + link = nil + } + } else { + link = nil + } + + created := false + if link == nil { + wgLink := &linkWireguard{LinkAttrs: netlink.LinkAttrs{Name: ifname}} + if err := netlink.LinkAdd(wgLink); err != nil { + return fmt.Errorf("failed to create WireGuard device: %v", err) + } + link = wgLink + created = true + } + + // The remaining steps are non-destructive to existing peers: AddrReplace + // is idempotent, and ConfigureDevice without ReplacePeers leaves the + // kernel peer list untouched. On failure, only delete the device if we + // created it ourselves; deleting an adopted device would kill live + // tunnels. + cleanupOnError := func() { + if created { + _ = netlink.LinkDel(link) + } } ipnet := prefixToIPNet(srv.WgCidr) - err = netlink.AddrAdd(link, &netlink.Addr{IPNet: &ipnet}) + err = netlink.AddrReplace(link, &netlink.Addr{IPNet: &ipnet}) if err != nil { - netlink.LinkDel(link) + cleanupOnError() return fmt.Errorf("failed to add address to WireGuard device: %v", err) } err = netlink.LinkSetUp(link) if err != nil { - netlink.LinkDel(link) + cleanupOnError() return fmt.Errorf("failed to bring up WireGuard device: %v", err) } @@ -282,13 +319,135 @@ func (srv *Server) StartWireguard() error { ListenPort: &listenPort, }) if err != nil { - netlink.LinkDel(link) + cleanupOnError() return err } return nil } +// canAdoptLink reports whether an existing link can be adopted as this +// server's WireGuard interface: it must be a WireGuard device whose address +// matches the server's peer CIDR. +func (srv *Server) canAdoptLink(link netlink.Link) bool { + if link.Type() != "wireguard" { + return false + } + addrs, err := netlink.AddrList(link, netlink.FAMILY_V4) + if err != nil { + return false + } + want := prefixToIPNet(srv.WgCidr) + wantOnes, _ := want.Mask.Size() + for _, addr := range addrs { + if addr.IPNet == nil { + continue + } + ones, _ := addr.Mask.Size() + if addr.IP.Equal(want.IP) && ones == wantOnes { + return true + } + } + return false +} + +// restoredPeers holds the result of rebuilding in-memory peer state from a +// kernel WireGuard device dump. +type restoredPeers struct { + // peerIPs maps each valid peer to its assigned IP. + peerIPs map[wgtypes.Key]netip.Addr + // invalid lists peers whose AllowedIPs don't encode a valid assigned IP; + // they should be removed from the device. + invalid []wgtypes.Key +} + +// restorePeerState rebuilds peer-to-IP assignments from a kernel WireGuard +// peer list, claiming each assigned IP in the allocator. A peer is valid if +// its first AllowedIP is an IPv4 /32 whose address can be claimed (inside the +// allocator's prefix and not already taken). Invalid peers are returned for +// removal so in-memory state and the kernel device stay consistent. +func restorePeerState(peers []wgtypes.Peer, alloc *IpAllocator) restoredPeers { + result := restoredPeers{peerIPs: make(map[wgtypes.Key]netip.Addr)} + for _, peer := range peers { + addr, ok := peerAssignedIp(peer) + if !ok || !alloc.Claim(addr) { + result.invalid = append(result.invalid, peer.PublicKey) + continue + } + result.peerIPs[peer.PublicKey] = addr + } + return result +} + +// peerAssignedIp extracts the IP assigned to a peer from its AllowedIPs, +// which vprox always writes as a single IPv4 /32. +func peerAssignedIp(peer wgtypes.Peer) (netip.Addr, bool) { + if len(peer.AllowedIPs) == 0 { + return netip.Addr{}, false + } + ipnet := peer.AllowedIPs[0] + ipv4 := ipnet.IP.To4() + if ipv4 == nil { + return netip.Addr{}, false + } + if ones, bits := ipnet.Mask.Size(); ones != 32 || bits != 32 { + return netip.Addr{}, false + } + return netip.AddrFrom4([4]byte(ipv4)), true +} + +// RestorePeersFromKernel rebuilds the in-memory peer index (peerIPs, +// ipAllocator, newPeers) from the kernel WireGuard device's peer list. It is +// called once on startup, after StartWireguard adopts an interface that +// survived a restart, so that: +// +// - existing peers keep their IPs and the allocator never hands an +// already-assigned IP to a new peer (which would silently steal the +// existing peer's AllowedIPs routing and blackhole it), and +// - the idle reaper grants restored peers the usual grace period instead of +// instantly reaping ones whose last handshake predates the restart. +func (srv *Server) RestorePeersFromKernel() error { + device, err := srv.WgClient.Device(srv.Ifname()) + if err != nil { + return fmt.Errorf("failed to get WireGuard device: %v", err) + } + if len(device.Peers) == 0 { + return nil + } + + restored := restorePeerState(device.Peers, srv.ipAllocator) + + now := time.Now() + srv.mu.Lock() + for key, addr := range restored.peerIPs { + srv.peerIPs[key] = addr + srv.newPeers[key] = now + } + srv.mu.Unlock() + + if len(restored.peerIPs) > 0 { + log.Printf("[%v] restored %d peer(s) from existing WireGuard device", + srv.BindAddr, len(restored.peerIPs)) + } + + // Remove peers with missing or malformed AllowedIPs from the device. + // These shouldn't exist, but dropping them keeps in-memory state + // consistent with the kernel. + if len(restored.invalid) > 0 { + removals := make([]wgtypes.PeerConfig, 0, len(restored.invalid)) + for _, key := range restored.invalid { + log.Printf("[%v] removing peer with invalid allowed IPs during restore: %v", + srv.BindAddr, key) + removals = append(removals, wgtypes.PeerConfig{PublicKey: key, Remove: true}) + } + err := srv.WgClient.ConfigureDevice(srv.Ifname(), wgtypes.Config{Peers: removals}) + if err != nil { + return fmt.Errorf("failed to remove invalid peers: %v", err) + } + } + return nil +} + func (srv *Server) CleanupWireguard() { ifname := srv.Ifname() _ = netlink.LinkDel(&linkWireguard{LinkAttrs: netlink.LinkAttrs{Name: ifname}}) diff --git a/lib/server_manager.go b/lib/server_manager.go index 876c86b..9218518 100644 --- a/lib/server_manager.go +++ b/lib/server_manager.go @@ -130,17 +130,27 @@ func (sm *ServerManager) Start(ip netip.Addr) error { defer sm.waitGroup.Done() defer sm.freeIndex(i) + // Note: we intentionally do NOT clean up the WireGuard interface or + // iptables rules on shutdown. The kernel dataplane keeps forwarding + // for existing peers while the process is down, which makes restarts + // (i.e. deploys) hitless. On startup, StartWireguard adopts the + // surviving interface and RestorePeersFromKernel rebuilds the + // in-memory peer state from it. CleanupWireguard/CleanupIptables + // remain available for manual decommissioning. if err := srv.StartWireguard(); err != nil { log.Printf("[%v] failed to start WireGuard: %v", ip, err) return } - defer srv.CleanupWireguard() + + if err := srv.RestorePeersFromKernel(); err != nil { + log.Printf("[%v] failed to restore peers from kernel: %v", ip, err) + return + } if err := srv.StartIptables(); err != nil { log.Printf("[%v] failed to start iptables: %v", ip, err) return } - defer srv.CleanupIptables() if err := srv.ListenForHttps(); err != nil { log.Printf("[%v] https server failed: %v", ip, err) diff --git a/lib/server_test.go b/lib/server_test.go new file mode 100644 index 0000000..b5cea9c --- /dev/null +++ b/lib/server_test.go @@ -0,0 +1,75 @@ +package lib + +import ( + "net" + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + "golang.zx2c4.com/wireguard/wgctrl/wgtypes" +) + +func mustKey(t *testing.T) wgtypes.Key { + t.Helper() + key, err := wgtypes.GenerateKey() + assert.NoError(t, err) + return key +} + +func peerWithPrefix(key wgtypes.Key, prefix string) wgtypes.Peer { + ipnet := prefixToIPNet(netip.MustParsePrefix(prefix)) + return wgtypes.Peer{PublicKey: key, AllowedIPs: []net.IPNet{ipnet}} +} + +func TestRestorePeerState(t *testing.T) { + alloc := NewIpAllocator(netip.MustParsePrefix("10.1.0.0/24")) + // Reserve the server's own address, as InitState does. + assert.Equal(t, netip.MustParseAddr("10.1.0.1"), alloc.Allocate()) + + k1 := mustKey(t) + k2 := mustKey(t) + k3 := mustKey(t) + k4 := mustKey(t) + k5 := mustKey(t) + peers := []wgtypes.Peer{ + peerWithPrefix(k1, "10.1.0.2/32"), + peerWithPrefix(k2, "10.1.0.7/32"), + {PublicKey: k3}, // no AllowedIPs + peerWithPrefix(k4, "10.1.0.8/31"), // not a /32 + peerWithPrefix(k5, "10.2.0.2/32"), // outside the allocator prefix + } + + restored := restorePeerState(peers, alloc) + + assert.Equal(t, map[wgtypes.Key]netip.Addr{ + k1: netip.MustParseAddr("10.1.0.2"), + k2: netip.MustParseAddr("10.1.0.7"), + }, restored.peerIPs) + assert.ElementsMatch(t, []wgtypes.Key{k3, k4, k5}, restored.invalid) + + // Restored IPs are claimed in the allocator, so new allocations skip them. + assert.Equal(t, netip.MustParseAddr("10.1.0.3"), alloc.Allocate()) + + // A peer whose IP is already claimed (duplicate) is flagged as invalid. + k6 := mustKey(t) + dup := restorePeerState([]wgtypes.Peer{peerWithPrefix(k6, "10.1.0.2/32")}, alloc) + assert.Empty(t, dup.peerIPs) + assert.Equal(t, []wgtypes.Key{k6}, dup.invalid) +} + +func TestPeerAssignedIp(t *testing.T) { + key := mustKey(t) + + addr, ok := peerAssignedIp(peerWithPrefix(key, "10.1.0.9/32")) + assert.True(t, ok) + assert.Equal(t, netip.MustParseAddr("10.1.0.9"), addr) + + _, ok = peerAssignedIp(wgtypes.Peer{PublicKey: key}) + assert.False(t, ok) + + _, ok = peerAssignedIp(peerWithPrefix(key, "10.1.0.0/24")) + assert.False(t, ok) + + _, ok = peerAssignedIp(peerWithPrefix(key, "fd00::1/128")) + assert.False(t, ok) +} From b9b5d0dfa21d2770185bd836a8fd3aa81acd6254 Mon Sep 17 00:00:00 2001 From: pbardea Date: Thu, 30 Jul 2026 21:52:44 +0000 Subject: [PATCH 2/3] lib: remove stale internal SNAT rules on startup Co-authored-by: Codesmith Staging --- lib/server.go | 55 +++++++++++++++++++++++++++++++++++++++++++++- lib/server_test.go | 30 +++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/lib/server.go b/lib/server.go index f5fccec..cf6ffc4 100644 --- a/lib/server.go +++ b/lib/server.go @@ -483,6 +483,50 @@ func (srv *Server) iptablesSnatRule(enabled bool) error { } } +// internalSnatRuleComment tags the nat POSTROUTING rule that SNATs traffic +// from the WireGuard subnet to the internal network. +const internalSnatRuleComment = "SNAT for WireGuard to internal network" + +// staleInternalSnatRuleIds returns the rule numbers (descending, so they stay +// valid while deleting one by one) of internal-network SNAT rules for wgCidr +// whose SNAT target is not bindAddr. rules is an `iptables -S` listing of nat +// POSTROUTING, where index 0 is the chain policy and the rule at index i has +// rule number i. Such rules survive a restart when a server index is reused +// with a different bind address; since AppendUnique adds the new rule after +// them, they would keep matching internal traffic and SNAT it to the stale +// address. +func staleInternalSnatRuleIds(rules []string, wgCidr netip.Prefix, bindAddr netip.Addr) []int { + // iptables normalizes the source to the masked network address. + source := fmt.Sprintf("-s %s ", wgCidr.Masked().String()) + target := fmt.Sprintf("--to-source %s ", bindAddr.String()) + var ids []int + for i := len(rules) - 1; i >= 1; i-- { + rule := rules[i] + " " + if strings.Contains(rule, internalSnatRuleComment) && + strings.Contains(rule, source) && + !strings.Contains(rule, target) { + ids = append(ids, i) + } + } + return ids +} + +// cleanupStaleInternalSnatRules removes internal-network SNAT rules for this +// server's WireGuard subnet that target a different bind address. +func (srv *Server) cleanupStaleInternalSnatRules() error { + rules, err := srv.Ipt.List("nat", "POSTROUTING") + if err != nil { + return fmt.Errorf("failed to list nat POSTROUTING rules: %v", err) + } + for _, id := range staleInternalSnatRuleIds(rules, srv.WgCidr, srv.BindAddr) { + log.Printf("[%v] removing stale internal SNAT rule: %v", srv.BindAddr, rules[id]) + if err := srv.Ipt.DeleteById("nat", "POSTROUTING", id); err != nil { + return fmt.Errorf("failed to delete stale SNAT rule: %v", err) + } + } + return nil +} + func (srv *Server) StartIptables() error { // Add masquerade rule for the outbound interface. rule := []string{ @@ -532,12 +576,21 @@ func (srv *Server) StartIptables() error { // SNAT rule for internal network traffic. This is currently only applicable for boxes in // the US. if srv.Region == "us-west" { + // Shutdown intentionally leaves iptables rules in place (see + // ServerManager.Start), so if this server's index was previously + // bound to a different address, a stale SNAT rule targeting the old + // address would precede the one added below and keep matching + // internal traffic. Remove any such rules first. + if err := srv.cleanupStaleInternalSnatRules(); err != nil { + return err + } + rule = []string{ "-s", srv.WgCidr.String(), "-d", srv.InternalNetworkCidr, "-o", srv.InternalBindIface.Attrs().Name, "-j", "SNAT", "--to-source", srv.BindAddr.String(), - "-m", "comment", "--comment", "SNAT for WireGuard to internal network", + "-m", "comment", "--comment", internalSnatRuleComment, } if err := srv.Ipt.AppendUnique("nat", "POSTROUTING", rule...); err != nil { return fmt.Errorf("failed to add SNAT rule: %v", err) diff --git a/lib/server_test.go b/lib/server_test.go index b5cea9c..0af2726 100644 --- a/lib/server_test.go +++ b/lib/server_test.go @@ -57,6 +57,36 @@ func TestRestorePeerState(t *testing.T) { assert.Equal(t, []wgtypes.Key{k6}, dup.invalid) } +func TestStaleInternalSnatRuleIds(t *testing.T) { + wgCidr := netip.MustParsePrefix("10.1.0.1/24") + bindAddr := netip.MustParseAddr("192.168.1.10") + + snatRule := func(source, toSource string) string { + return "-A POSTROUTING -s " + source + " -d 10.0.0.0/8 -o eth1" + + ` -m comment --comment "` + internalSnatRuleComment + `"` + + " -j SNAT --to-source " + toSource + } + + rules := []string{ + "-P POSTROUTING ACCEPT", + // Stale: our subnet, old bind address. + snatRule("10.1.0.0/24", "192.168.1.9"), + // Stale: old bind address that is a prefix of the current one. + snatRule("10.1.0.0/24", "192.168.1.1"), + // Current rule; must be kept. + snatRule("10.1.0.0/24", "192.168.1.10"), + // Another server's subnet; must be kept. + snatRule("10.2.0.0/24", "192.168.1.9"), + // Unrelated rule without the vprox comment; must be kept. + "-A POSTROUTING -s 10.1.0.0/24 -j SNAT --to-source 192.168.1.9", + } + + // Ids are rule numbers in descending order so deletes don't shift them. + assert.Equal(t, []int{2, 1}, staleInternalSnatRuleIds(rules, wgCidr, bindAddr)) + + assert.Empty(t, staleInternalSnatRuleIds([]string{"-P POSTROUTING ACCEPT"}, wgCidr, bindAddr)) +} + func TestPeerAssignedIp(t *testing.T) { key := mustKey(t) From 6eaff99c642436e1cf3bd38cb9300c82d808556c Mon Sep 17 00:00:00 2001 From: pbardea Date: Thu, 30 Jul 2026 22:02:12 +0000 Subject: [PATCH 3/3] lib: delete stale SNAT rules by rule match instead of rule number Co-authored-by: Codesmith Staging --- lib/server.go | 83 ++++++++++++++++++++++++++++++++++++---------- lib/server_test.go | 34 ++++++++++++++++--- 2 files changed, 95 insertions(+), 22 deletions(-) diff --git a/lib/server.go b/lib/server.go index cf6ffc4..e775fc3 100644 --- a/lib/server.go +++ b/lib/server.go @@ -487,40 +487,87 @@ func (srv *Server) iptablesSnatRule(enabled bool) error { // from the WireGuard subnet to the internal network. const internalSnatRuleComment = "SNAT for WireGuard to internal network" -// staleInternalSnatRuleIds returns the rule numbers (descending, so they stay -// valid while deleting one by one) of internal-network SNAT rules for wgCidr -// whose SNAT target is not bindAddr. rules is an `iptables -S` listing of nat -// POSTROUTING, where index 0 is the chain policy and the rule at index i has -// rule number i. Such rules survive a restart when a server index is reused +// staleInternalSnatRules returns the rule specs (the arguments after +// "-A POSTROUTING") of internal-network SNAT rules for wgCidr whose SNAT +// target is not bindAddr. rules is an `iptables -S` listing of nat +// POSTROUTING. Such rules survive a restart when a server index is reused // with a different bind address; since AppendUnique adds the new rule after // them, they would keep matching internal traffic and SNAT it to the stale // address. -func staleInternalSnatRuleIds(rules []string, wgCidr netip.Prefix, bindAddr netip.Addr) []int { +func staleInternalSnatRules(rules []string, wgCidr netip.Prefix, bindAddr netip.Addr) [][]string { // iptables normalizes the source to the masked network address. source := fmt.Sprintf("-s %s ", wgCidr.Masked().String()) target := fmt.Sprintf("--to-source %s ", bindAddr.String()) - var ids []int - for i := len(rules) - 1; i >= 1; i-- { - rule := rules[i] + " " - if strings.Contains(rule, internalSnatRuleComment) && - strings.Contains(rule, source) && - !strings.Contains(rule, target) { - ids = append(ids, i) + var specs [][]string + for _, rule := range rules { + padded := rule + " " + if strings.Contains(padded, internalSnatRuleComment) && + strings.Contains(padded, source) && + !strings.Contains(padded, target) { + if spec := iptablesRuleSpec(rule); spec != nil { + specs = append(specs, spec) + } } } - return ids + return specs +} + +// iptablesRuleSpec parses an `iptables -S` "-A ..." line into the +// rule's argument list (without the leading "-A "), suitable for a +// match-based delete. Double-quoted tokens (e.g. comments containing spaces) +// are unquoted and backslash escapes are resolved. Returns nil for lines that +// are not append rules, such as the "-P " line. +func iptablesRuleSpec(rule string) []string { + var args []string + var cur strings.Builder + inQuotes := false + escaped := false + inToken := false + for _, r := range rule { + switch { + case escaped: + cur.WriteRune(r) + escaped = false + case r == '\\': + escaped = true + case r == '"': + inQuotes = !inQuotes + inToken = true + case r == ' ' && !inQuotes: + if inToken { + args = append(args, cur.String()) + cur.Reset() + inToken = false + } + default: + cur.WriteRune(r) + inToken = true + } + } + if inToken { + args = append(args, cur.String()) + } + if len(args) < 2 || args[0] != "-A" { + return nil + } + return args[2:] } // cleanupStaleInternalSnatRules removes internal-network SNAT rules for this -// server's WireGuard subnet that target a different bind address. +// server's WireGuard subnet that target a different bind address. Deletion is +// by exact rule match rather than by rule number: each server runs +// StartIptables in its own goroutine against the shared iptables handle, so +// deleting by number races with concurrent inserts/deletes shifting the +// numbering. Match-based deletes are unaffected, and rules for other servers' +// subnets never match this server's specs. func (srv *Server) cleanupStaleInternalSnatRules() error { rules, err := srv.Ipt.List("nat", "POSTROUTING") if err != nil { return fmt.Errorf("failed to list nat POSTROUTING rules: %v", err) } - for _, id := range staleInternalSnatRuleIds(rules, srv.WgCidr, srv.BindAddr) { - log.Printf("[%v] removing stale internal SNAT rule: %v", srv.BindAddr, rules[id]) - if err := srv.Ipt.DeleteById("nat", "POSTROUTING", id); err != nil { + for _, spec := range staleInternalSnatRules(rules, srv.WgCidr, srv.BindAddr) { + log.Printf("[%v] removing stale internal SNAT rule: %v", srv.BindAddr, strings.Join(spec, " ")) + if err := srv.Ipt.DeleteIfExists("nat", "POSTROUTING", spec...); err != nil { return fmt.Errorf("failed to delete stale SNAT rule: %v", err) } } diff --git a/lib/server_test.go b/lib/server_test.go index 0af2726..7544db2 100644 --- a/lib/server_test.go +++ b/lib/server_test.go @@ -57,7 +57,7 @@ func TestRestorePeerState(t *testing.T) { assert.Equal(t, []wgtypes.Key{k6}, dup.invalid) } -func TestStaleInternalSnatRuleIds(t *testing.T) { +func TestStaleInternalSnatRules(t *testing.T) { wgCidr := netip.MustParsePrefix("10.1.0.1/24") bindAddr := netip.MustParseAddr("192.168.1.10") @@ -66,6 +66,13 @@ func TestStaleInternalSnatRuleIds(t *testing.T) { ` -m comment --comment "` + internalSnatRuleComment + `"` + " -j SNAT --to-source " + toSource } + snatSpec := func(source, toSource string) []string { + return []string{ + "-s", source, "-d", "10.0.0.0/8", "-o", "eth1", + "-m", "comment", "--comment", internalSnatRuleComment, + "-j", "SNAT", "--to-source", toSource, + } + } rules := []string{ "-P POSTROUTING ACCEPT", @@ -81,10 +88,29 @@ func TestStaleInternalSnatRuleIds(t *testing.T) { "-A POSTROUTING -s 10.1.0.0/24 -j SNAT --to-source 192.168.1.9", } - // Ids are rule numbers in descending order so deletes don't shift them. - assert.Equal(t, []int{2, 1}, staleInternalSnatRuleIds(rules, wgCidr, bindAddr)) + // Stale rules are returned as full rule specs for match-based deletion, + // with the quoted comment unwrapped into a single argument. + assert.Equal(t, [][]string{ + snatSpec("10.1.0.0/24", "192.168.1.9"), + snatSpec("10.1.0.0/24", "192.168.1.1"), + }, staleInternalSnatRules(rules, wgCidr, bindAddr)) + + assert.Empty(t, staleInternalSnatRules([]string{"-P POSTROUTING ACCEPT"}, wgCidr, bindAddr)) +} + +func TestIptablesRuleSpec(t *testing.T) { + assert.Equal(t, + []string{"-s", "10.1.0.0/24", "-m", "comment", "--comment", "two words", "-j", "ACCEPT"}, + iptablesRuleSpec(`-A POSTROUTING -s 10.1.0.0/24 -m comment --comment "two words" -j ACCEPT`)) + + // Backslash escapes inside quotes are resolved. + assert.Equal(t, + []string{"-m", "comment", "--comment", `say "hi"`, "-j", "ACCEPT"}, + iptablesRuleSpec(`-A FORWARD -m comment --comment "say \"hi\"" -j ACCEPT`)) - assert.Empty(t, staleInternalSnatRuleIds([]string{"-P POSTROUTING ACCEPT"}, wgCidr, bindAddr)) + // Non-append lines (e.g. chain policies) are not rule specs. + assert.Nil(t, iptablesRuleSpec("-P POSTROUTING ACCEPT")) + assert.Nil(t, iptablesRuleSpec("")) } func TestPeerAssignedIp(t *testing.T) {