diff --git a/peers/allowedips_test.go b/peers/allowedips_test.go new file mode 100644 index 0000000..0d71f67 --- /dev/null +++ b/peers/allowedips_test.go @@ -0,0 +1,210 @@ +package peers + +import ( + "strings" + "testing" + "time" + + "github.com/fosrl/newt/network" + "golang.zx2c4.com/wireguard/conn" + "golang.zx2c4.com/wireguard/device" + "golang.zx2c4.com/wireguard/tun/tuntest" + "golang.zx2c4.com/wireguard/wgctrl/wgtypes" +) + +// newTestDevice returns a real *device.Device backed by an in-memory channel +// TUN (golang.zx2c4.com/wireguard/tun/tuntest) and a standard UDP bind - no +// OS TUN interface or elevated privileges required, so this is safe to run +// as a normal unit test. Used to exercise the real AddAllowedIP/ +// RemoveAllowedIP/ConfigurePeer IPC calls that suppressResourceRoutesLocked/ +// restoreResourceRoutesLocked make, which a hand-rolled PeerManager with +// device left nil (see newGatewayTestManager/newExitNodeTestManager) can't +// safely call. +func newTestDevice(t *testing.T) (*device.Device, wgtypes.Key) { + t.Helper() + privateKey, err := wgtypes.GeneratePrivateKey() + if err != nil { + t.Fatalf("GeneratePrivateKey: %v", err) + } + + dev := device.NewDevice(tuntest.NewChannelTUN().TUN(), conn.NewDefaultBind(), device.NewLogger(device.LogLevelError, "test: ")) + t.Cleanup(dev.Close) + + if err := dev.IpcSet("private_key=" + hexKey(privateKey) + "\n"); err != nil { + t.Fatalf("failed to set device private key: %v", err) + } + + return dev, privateKey +} + +func hexKey(k wgtypes.Key) string { + b := [32]byte(k) + const hextable = "0123456789abcdef" + out := make([]byte, 64) + for i, c := range b { + out[i*2] = hextable[c>>4] + out[i*2+1] = hextable[c&0x0f] + } + return string(out) +} + +// TestSuppressRestoreResourceRoutesStripsWireGuardAllowedIPs is the +// counterpart to TestSetGatewaySuppressesResourceRoutesInNetworkSettings, +// but for WireGuard's own AllowedIPs rather than the OS routing table/ +// NetworkSettings: it verifies suppressResourceRoutesLocked/ +// restoreResourceRoutesLocked actually add/remove the site peer's resource +// CIDRs (remote subnet, alias) from WireGuard itself - not just the system +// route - so that nothing reaching the tunnel interface directly (e.g. a +// mobile netstack/FD consumer bypassing the OS route table) can still reach +// a suppressed resource via WireGuard's own crypto-key routing. The server +// IP allowed-ip entry must survive throughout, since it's what keeps the +// site's own control/monitoring traffic (pings, handshakes) alive while +// suppressed. +func TestSuppressRestoreResourceRoutesStripsWireGuardAllowedIPs(t *testing.T) { + network.ClearNetworkSettings() + defer network.ClearNetworkSettings() + + dev, privKey := newTestDevice(t) + peerKey, err := wgtypes.GeneratePrivateKey() + if err != nil { + t.Fatalf("GeneratePrivateKey (peer): %v", err) + } + peerPubKey := peerKey.PublicKey() + + pm := &PeerManager{ + device: dev, + peers: make(map[int]SiteConfig), + allowedIPOwners: make(map[string]int), + allowedIPClaims: make(map[string]map[int]bool), + lastOwnerChange: make(map[string]time.Time), + gatewaySiteIds: make(map[int]bool), + gatewayExcludedIPs: make(map[string]int), + gatewayExtraEndpoints: make(map[string]bool), + privateKey: privKey, + interfaceName: "fake0", + localIP: "100.90.128.8", + disableRoutesAndAliasesOnExitNode: true, + } + + site := SiteConfig{ + SiteId: 5, + PublicKey: peerPubKey.String(), + Endpoint: "127.0.0.1:1", // never dialed - IpcSet doesn't connect + ServerIP: "100.90.128.1/20", + RemoteSubnets: []string{"172.18.21.32/24"}, + } + + // Directly claim ownership and push the peer's full AllowedIPs, mirroring + // what AddPeer does, without needing the rest of AddPeer's machinery + // (DNS proxy, peer monitor, holepunch test) that isn't relevant here. + pm.peers[5] = site + pm.claimAllowedIP(5, "172.18.21.32/24") + wgConfig := site + wgConfig.AllowedIps = []string{"172.18.21.32/24"} + if err := ConfigurePeer(dev, wgConfig, privKey, false, 0, nil); err != nil { + t.Fatalf("seed ConfigurePeer: %v", err) + } + + before, err := dev.IpcGet() + if err != nil { + t.Fatalf("IpcGet before suppress: %v", err) + } + if !strings.Contains(before, "172.18.21.0/24") { + t.Fatalf("test setup broken: seeded allowed_ip missing before suppress:\n%s", before) + } + if !strings.Contains(before, "100.90.128.1/32") { + t.Fatalf("test setup broken: server IP allowed_ip missing before suppress:\n%s", before) + } + + pm.mu.Lock() + pm.suppressResourceRoutesLocked() + pm.mu.Unlock() + + after, err := dev.IpcGet() + if err != nil { + t.Fatalf("IpcGet after suppress: %v", err) + } + if strings.Contains(after, "172.18.21.0/24") { + t.Fatalf("resource allowed_ip still present in WireGuard after suppression (exit node active):\n%s", after) + } + if !strings.Contains(after, "100.90.128.1/32") { + t.Fatalf("server IP allowed_ip must survive suppression (needed for site monitoring/liveness):\n%s", after) + } + + pm.mu.Lock() + pm.restoreResourceRoutesLocked() + pm.mu.Unlock() + + restored, err := dev.IpcGet() + if err != nil { + t.Fatalf("IpcGet after restore: %v", err) + } + if !strings.Contains(restored, "172.18.21.0/24") { + t.Fatalf("resource allowed_ip not restored in WireGuard after restore:\n%s", restored) + } + if !strings.Contains(restored, "100.90.128.1/32") { + t.Fatalf("server IP allowed_ip missing after restore:\n%s", restored) + } +} + +// TestShouldPushAllowedIPLockedAndOwnershipFilteringWhileSuppressed covers +// the "tunnel starts with an exit node already active" case at the unit +// level: getOwnedAllowedIPs/getWireGuardAllowedIPs (what AddPeer's inline +// ownership computation, addAllowedIp, the route optimizer, etc. all defer +// to - see shouldPushAllowedIPLocked) must exclude a resource CIDR while +// suppressed even though the underlying claim/ownership is registered +// normally, and must always keep the gateway CIDR. +func TestShouldPushAllowedIPLockedAndOwnershipFilteringWhileSuppressed(t *testing.T) { + pm := &PeerManager{ + peers: make(map[int]SiteConfig), + allowedIPOwners: make(map[string]int), + allowedIPClaims: make(map[string]map[int]bool), + disableRoutesAndAliasesOnExitNode: true, + } + pm.peers[5] = SiteConfig{SiteId: 5, ServerIP: "100.90.128.1/20"} + + pm.claimAllowedIP(5, "172.18.21.0/24") + pm.claimAllowedIP(5, gatewayCIDR) + + if !pm.shouldPushAllowedIPLocked(gatewayCIDR) { + t.Fatalf("gateway CIDR must always be pushable") + } + if !pm.shouldPushAllowedIPLocked("172.18.21.0/24") { + t.Fatalf("resource CIDR must be pushable while not suppressed") + } + owned := pm.getOwnedAllowedIPs(5) + if len(owned) != 2 { + t.Fatalf("expected both claimed CIDRs owned before suppression, got %v", owned) + } + + pm.resourceRoutesSuppressed = true + + if pm.shouldPushAllowedIPLocked("172.18.21.0/24") { + t.Fatalf("resource CIDR must not be pushable while suppressed") + } + if !pm.shouldPushAllowedIPLocked(gatewayCIDR) { + t.Fatalf("gateway CIDR must still be pushable while suppressed") + } + + owned = pm.getOwnedAllowedIPs(5) + if len(owned) != 1 || owned[0] != gatewayCIDR { + t.Fatalf("expected only the gateway CIDR owned while suppressed, got %v", owned) + } + + wgIPs := pm.getWireGuardAllowedIPs(5) + want := map[string]bool{"100.90.128.1/32": true, gatewayCIDR: true} + if len(wgIPs) != len(want) { + t.Fatalf("expected server IP + gateway CIDR only while suppressed, got %v", wgIPs) + } + for _, ip := range wgIPs { + if !want[ip] { + t.Fatalf("unexpected allowed IP %q while suppressed: %v", ip, wgIPs) + } + } + + pm.resourceRoutesSuppressed = false + owned = pm.getOwnedAllowedIPs(5) + if len(owned) != 2 { + t.Fatalf("expected both claimed CIDRs owned again after restore, got %v", owned) + } +} diff --git a/peers/manager.go b/peers/manager.go index c31dac3..fc0f866 100644 --- a/peers/manager.go +++ b/peers/manager.go @@ -767,14 +767,35 @@ func (pm *PeerManager) exitNodeOrGatewayActiveLocked() bool { return pm.exitNodeActive || pm.gatewayActive } +// shouldPushAllowedIPLocked reports whether cidr should actually be pushed +// into WireGuard right now. The gateway CIDR (see gatewayCIDR) always should +// be - gateway/full-tunnel routing must keep working regardless of resource +// suppression. Every other (resource) CIDR should be unless resource routes/ +// aliases are currently suppressed (see resourceRoutesSuppressed), in which +// case WireGuard's own crypto-key routing must not be able to reach it +// either - not just the OS routing table - since anything that reaches the +// tunnel interface directly (e.g. a mobile netstack/FD consumer bypassing +// the OS route table) would otherwise still get forwarded there. Ownership +// bookkeeping (allowedIPOwners/allowedIPClaims/the route optimizer) keeps +// running normally regardless, via getOwnedAllowedIPs/getWireGuardAllowedIPs +// deferring to this check only for what actually gets pushed to WireGuard - +// so the correct set is immediately ready the moment suppression lifts. Must +// be called with pm.mu held. +func (pm *PeerManager) shouldPushAllowedIPLocked(cidr string) bool { + return cidr == gatewayCIDR || !pm.resourceRoutesSuppressed +} + // suppressResourceRoutesLocked removes routes and alias DNS records for every -// tracked site peer's resources (server IP, remote subnets, aliases) so that -// only the exit node's own routes remain in effect. WireGuard peer -// configuration (AllowedIps) is left untouched - the tunnel remains usable by -// anything that reaches it without relying on the OS routing table. Called -// when either "exit node" signal becomes active while -// DisableRoutesAndAliasesOnExitNode is enabled (see SetExitNode/SetGateway). -// No-op if already suppressed. Must be called with pm.mu held. +// tracked site peer's resources (server IP, remote subnets, aliases), and +// strips those same resource CIDRs from each peer's WireGuard AllowedIPs, so +// that only the exit node's own routes remain reachable at all - both via +// the OS routing table and via WireGuard's own crypto-key routing. The +// server IP and any owned gateway-CIDR claim are always kept in WireGuard +// (see shouldPushAllowedIPLocked), so the tunnel's control/monitoring +// traffic and gateway/full-tunnel routing are unaffected. Called when either +// "exit node" signal becomes active while DisableRoutesAndAliasesOnExitNode +// is enabled (see SetExitNode/SetGateway). No-op if already suppressed. Must +// be called with pm.mu held. // // Deliberately calls network.* and pm.dnsProxy directly rather than through // the addRoutes/removeRoutes/addServerRoute/removeServerRoute/addDNSRecord/ @@ -785,10 +806,14 @@ func (pm *PeerManager) suppressResourceRoutesLocked() { if pm.resourceRoutesSuppressed { return } + // Flipped before the loop below (rather than after, like the OS-route/DNS + // work above it) because getWireGuardAllowedIPs must already reflect the + // suppressed state for the RemoveAllowedIP replace-call below to compute + // the correct reduced set to keep. pm.resourceRoutesSuppressed = true removedSubnets := make(map[string]bool, len(pm.peers)) - for _, peer := range pm.peers { + for siteId, peer := range pm.peers { if err := network.RemoveRouteForServerIPWithSource(normalizeServerRouteDestination(peer.ServerIP), pm.interfaceName, pm.localIP); err != nil { logger.Warn("Exit node active: failed to remove route for server IP %s: %v", peer.ServerIP, err) } @@ -810,24 +835,34 @@ func (pm *PeerManager) suppressResourceRoutesLocked() { pm.dnsProxy.RemoveDNSRecordForSite(alias.Alias, address, peer.SiteId) } } + + if peer.PublicKey != "" { + remaining := pm.getWireGuardAllowedIPs(siteId) + if err := RemoveAllowedIP(pm.device, peer.PublicKey, remaining); err != nil { + logger.Warn("Exit node active: failed to strip resource allowed IPs for site %d: %v", siteId, err) + } + } } logger.Info("Exit node active: removed resource routes/aliases for %d site(s)", len(pm.peers)) } // restoreResourceRoutesLocked is suppressResourceRoutesLocked's inverse, -// re-adding routes and alias DNS records for every tracked site peer's -// resources. Called when neither "exit node" signal remains active (see -// ClearExitNode/clearGatewayLocked). No-op if not currently suppressed. Must -// be called with pm.mu held. +// re-adding routes, alias DNS records, and WireGuard AllowedIPs for every +// tracked site peer's resources. Called when neither "exit node" signal +// remains active (see ClearExitNode/clearGatewayLocked). No-op if not +// currently suppressed. Must be called with pm.mu held. func (pm *PeerManager) restoreResourceRoutesLocked() { if !pm.resourceRoutesSuppressed { return } + // Flipped before the loop below (rather than after) because + // getOwnedAllowedIPs must already reflect the restored state for the + // AddAllowedIP calls below to know which resource CIDRs to add back. pm.resourceRoutesSuppressed = false addedSubnets := make(map[string]bool, len(pm.peers)) - for _, peer := range pm.peers { + for siteId, peer := range pm.peers { if err := network.AddRouteForServerIPWithSource(normalizeServerRouteDestination(peer.ServerIP), pm.interfaceName, pm.localIP); err != nil { logger.Warn("Exit node inactive: failed to add route for server IP %s: %v", peer.ServerIP, err) } @@ -851,6 +886,20 @@ func (pm *PeerManager) restoreResourceRoutesLocked() { } } } + + if peer.PublicKey != "" { + // getOwnedAllowedIPs already reflects the just-restored state, so + // this is exactly the resource CIDRs (plus the gateway CIDR, + // already present and unaffected by suppression) this peer + // currently owns. AddAllowedIP is additive/idempotent, so + // re-adding an already-present entry (e.g. the gateway CIDR) is + // harmless. + for _, cidr := range pm.getOwnedAllowedIPs(siteId) { + if err := AddAllowedIP(pm.device, peer.PublicKey, cidr); err != nil { + logger.Warn("Exit node inactive: failed to restore allowed IP %s for site %d: %v", cidr, siteId, err) + } + } + } } logger.Info("Exit node inactive: restored resource routes/aliases for %d site(s)", len(pm.peers)) @@ -881,12 +930,16 @@ func (pm *PeerManager) AddPeer(siteConfig SiteConfig) error { } siteConfig.AllowedIps = allowedIPs - // Register claims for all allowed IPs and determine which ones this peer will own + // Register claims for all allowed IPs and determine which ones this peer + // will own in WireGuard. Claims are registered regardless of suppression + // (ownership bookkeeping always continues - see + // shouldPushAllowedIPLocked), but an owned resource IP is only actually + // pushed to WireGuard if resource routes/aliases aren't currently + // suppressed. ownedIPs := make([]string, 0, len(allowedIPs)) for _, ip := range allowedIPs { pm.claimAllowedIP(siteConfig.SiteId, ip) - // Check if this peer became the owner - if pm.allowedIPOwners[ip] == siteConfig.SiteId { + if pm.allowedIPOwners[ip] == siteConfig.SiteId && pm.shouldPushAllowedIPLocked(ip) { ownedIPs = append(ownedIPs, ip) } } @@ -1329,14 +1382,20 @@ func (pm *PeerManager) releaseAllowedIP(siteId int, cidr string) (newOwner int, return -1, false } -// getOwnedAllowedIPs returns the list of allowed IPs that a peer currently owns in WireGuard. -// Must be called with lock held. +// getOwnedAllowedIPs returns the list of allowed IPs that a peer currently +// owns and that should be reflected in WireGuard right now - see +// shouldPushAllowedIPLocked for what's excluded while resource routes are +// suppressed. Must be called with lock held. func (pm *PeerManager) getOwnedAllowedIPs(siteId int) []string { var owned []string for cidr, owner := range pm.allowedIPOwners { - if owner == siteId { - owned = append(owned, cidr) + if owner != siteId { + continue } + if !pm.shouldPushAllowedIPLocked(cidr) { + continue + } + owned = append(owned, cidr) } return owned } @@ -1363,8 +1422,11 @@ func (pm *PeerManager) addAllowedIp(siteId int, ip string) error { peer.AllowedIps = append(peer.AllowedIps, ip) pm.peers[siteId] = peer - // Only update WireGuard if we own this IP - if pm.allowedIPOwners[ip] == siteId { + // Only update WireGuard if we own this IP and it should currently be + // pushed (see shouldPushAllowedIPLocked - resource CIDRs are held back + // while suppressed, even though the claim above still registers + // ownership). + if pm.allowedIPOwners[ip] == siteId && pm.shouldPushAllowedIPLocked(ip) { if err := AddAllowedIP(pm.device, peer.PublicKey, ip); err != nil { return err } @@ -1424,8 +1486,11 @@ func (pm *PeerManager) removeAllowedIp(siteId int, cidr string) error { return err } - // If another peer was promoted to owner, add the IP to their WireGuard config - if promoted && newOwner >= 0 { + // If another peer was promoted to owner, add the IP to their WireGuard + // config - unless it's a resource CIDR held back by suppression (see + // shouldPushAllowedIPLocked); the promotion itself (ownership bookkeeping) + // still happened above via releaseAllowedIP regardless. + if promoted && newOwner >= 0 && pm.shouldPushAllowedIPLocked(cidr) { if newOwnerPeer, exists := pm.peers[newOwner]; exists { if err := AddAllowedIP(pm.device, newOwnerPeer.PublicKey, cidr); err != nil { logger.Error("Failed to promote peer %d for IP %s: %v", newOwner, cidr, err) @@ -1993,22 +2058,18 @@ func (pm *PeerManager) shouldSwitchOwner(cidr string, currentSiteId, candidateSi return true } -// getWireGuardAllowedIPs returns the full set of IPs that should be in WireGuard -// for a peer: server IP /32 plus all shared IPs it currently owns. -// Must be called with pm.mu held. +// getWireGuardAllowedIPs returns the full set of IPs that should be in +// WireGuard for a peer right now: server IP /32 (always) plus every shared +// IP it currently owns that getOwnedAllowedIPs/shouldPushAllowedIPLocked +// says should actually be pushed (i.e. excluding resource CIDRs while +// suppressed). Must be called with pm.mu held. func (pm *PeerManager) getWireGuardAllowedIPs(siteId int) []string { peer, exists := pm.peers[siteId] if !exists { return nil } serverIP := strings.Split(peer.ServerIP, "/")[0] + "/32" - ips := []string{serverIP} - for cidr, owner := range pm.allowedIPOwners { - if owner == siteId { - ips = append(ips, cidr) - } - } - return ips + return append([]string{serverIP}, pm.getOwnedAllowedIPs(siteId)...) } // transferOwnership moves WireGuard ownership of cidr from fromSiteId to toSiteId. @@ -2027,8 +2088,11 @@ func (pm *PeerManager) transferOwnership(cidr string, fromSiteId int, toSiteId i } } - // Add cidr to new owner's WireGuard allowed IPs - if toPeer, exists := pm.peers[toSiteId]; exists { + // Add cidr to new owner's WireGuard allowed IPs - unless it's a resource + // CIDR held back by suppression (see shouldPushAllowedIPLocked); the + // ownership change above still stands regardless, so it's ready to push + // the moment suppression lifts (see restoreResourceRoutesLocked). + if toPeer, exists := pm.peers[toSiteId]; exists && pm.shouldPushAllowedIPLocked(cidr) { if err := AddAllowedIP(pm.device, toPeer.PublicKey, cidr); err != nil { return fmt.Errorf("add IP %s to site %d: %v", cidr, toSiteId, err) } @@ -2058,10 +2122,13 @@ func (pm *PeerManager) optimizeRoutes() { } if !hasOwner { - // No current owner, just assign + // No current owner, just assign. Ownership is recorded + // regardless of suppression; the WireGuard push is skipped for a + // resource CIDR held back by suppression (see + // shouldPushAllowedIPLocked), same rationale as transferOwnership. pm.allowedIPOwners[cidr] = bestOwner pm.lastOwnerChange[cidr] = time.Now() - if toPeer, exists := pm.peers[bestOwner]; exists { + if toPeer, exists := pm.peers[bestOwner]; exists && pm.shouldPushAllowedIPLocked(cidr) { if err := AddAllowedIP(pm.device, toPeer.PublicKey, cidr); err != nil { logger.Error("Failed to assign IP %s to site %d: %v", cidr, bestOwner, err) }