lib: add /connect-ipip endpoint for IPIP tunnel peers#12
Open
eltonkl wants to merge 12 commits into
Open
Conversation
Adds a companion to /connect that returns an inner IP from the same WgCidr pool but provisions a Linux IPIP tunnel keyed on the HTTPS source address instead of a WireGuard peer. Each request creates vp<srv.Index>-<host> with local=srv.BindAddr, remote=<client_ip>, and installs a /32 host route so decapsulated return traffic exits the right tunnel. Wildcard FORWARD/TCP-MSS rules cover every IPIP interface this server creates. Idle peers (no observed rx for PeerIdleTimeout) are pruned in a background loop and their IPs returned to the shared allocator. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
netlink.LinkAdd does not populate Index on the passed Iptun struct, so the host route was being installed with LinkIndex 0 and never bound to the tunnel. Look up the link by name after creation and use the resolved attrs for both LinkSetUp and RouteReplace. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Defense in depth against inner-source spoofing on the ipip data plane. Once a tunnel is registered, anyone who can deliver an ipip packet to us with the registered client's outer source IP can inject inner traffic. Two cheap server-side mitigations: - Set rp_filter=1 on each ipip iface as it's created. With the per-peer /32 route we install, the kernel drops decapsulated packets whose inner source IP wouldn't route back through the same iface they arrived on. - Replace the broad 'FORWARD ACCEPT for vp<idx>-+' wildcard with per-peer iptables rules: ACCEPT -i <ifname> -s <peerIP>/32, then DROP -i <ifname>. Each tunnel only forwards traffic with the inner source we assigned to it. Wildcard TCP MSS clamping stays as-is; it doesn't depend on the inner source. tearDownIpipLink now takes peerIP so it can clean up the per-peer rules before deleting the interface. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
When iptablesIpipMssRules ran with enabled=false, a failure to delete the outbound rule returned immediately and skipped the inbound delete, leaking that rule. Split the add and cleanup paths: add still fails fast, cleanup attempts both deletes independently and logs errors, matching the WG MSS cleanup pattern in CleanupIptables. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
- connectIpipHandler: refresh lastSeen on the winner in the race-loser path, matching the idempotent path - createIpipLink: correct the rp_filter comment — max(all, iface) means setting the per-iface value to 1 degrades to loose mode when conf.all.rp_filter is 2; the per-peer iptables filter is authoritative - removeIdleIpipPeers: count tx_bytes as well as rx_bytes so a peer in a one-way transfer isn't pruned mid-stream - connectIpipHandler: note that a stale idempotent assignment is healed by removeIdleIpipPeers within one poll interval - add ipip_test.go covering ifname/wildcard/iptables-rule helpers, including the IFNAMSIZ bound Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
If an ipip interface is destroyed out-of-band, the peer entry becomes unusable but two paths kept it alive forever: - The idempotent path in connectIpipHandler refreshed lastSeen on every retry without checking whether the interface still existed, handing the client back a broken assignment indefinitely. - removeIdleIpipPeers added vanished entries to toRemove but still applied the lastSeen <= PeerIdleTimeout guard in the final loop, so a freshly-refreshed (broken) peer never aged out. connectIpipHandler now verifies the interface with LinkByName before reusing an existing entry; if it's gone, the stale entry is dropped (iptables filter removed, IP returned to the allocator) and the request falls through to fresh allocation. removeIdleIpipPeers tracks a 'vanished' flag per snapshot and skips the freshness guard for vanished entries so they reap unconditionally. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
- drop the per-interface rp_filter layer. On these hosts conf.all.rp_filter is forced to 2 (loose) fleet-wide for the WireGuard path, so max(all, iface) can never reach strict mode and the per-interface setting was a no-op masquerading as a control. - add iptablesIpipHostGuardRule: a wildcard raw/PREROUTING DROP for IPIP traffic destined to a host-local address. The per-peer FORWARD filter only covers transit traffic, leaving packets that terminate on the host (notably the vprox HTTPS control plane) reachable with a forged inner source. raw/PREROUTING runs before conntrack and ufw's filter chains, so ufw's port accepts can't shadow it. - drop the always-nil error return from removeIdleIpipPeers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The old encoding used only the low 16 bits of peerIP for the suffix. For WgCidr widths wider than /16 (allowed: wgBlockPerIp can equal wgBlock.Bits(), which has no explicit lower bound), two distinct peers could produce the same ifname (e.g. 10.0.0.1 and 10.1.0.1 in a /8 both become vp0-1). createIpipLink does a best-effort LinkDel of any existing same-named interface before LinkAdd, so a collision would silently destroy a live peer's tunnel. Use the offset from srv.WgCidr.Addr() as the suffix instead, which is unique within any CIDR by construction, and length-check the result against IFNAMSIZ so a wildly oversized CIDR fails the /connect-ipip request loudly rather than producing a colliding or truncated name. Tests updated to set WgCidr on the test Server and cover both the wide-CIDR regression and the IFNAMSIZ rejection. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
connectIpipHandler's idempotent path held ipipMu across a netlink.LinkByName syscall and treated any lookup error as "interface vanished". Read the entry under ipipMu, release it, probe the interface lock-free, then re-acquire and re-check the entry's identity before mutating. Only netlink.LinkNotFoundError now counts as vanished; other errors log and reuse the tunnel. Also validate in InitState that WgCidr isn't wide enough for a peer's IPIP interface name to exceed IFNAMSIZ, so a misconfigured server fails fast instead of 500-ing every /connect-ipip request. The test helper now builds WgCidr the way server_manager.go does (network base + 1), so test ifnames match a real server's. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
removeIdleIpipPeers treated any LinkByName error as vanished and set vanished=true on the removal entry, which then bypasses the lastSeen freshness guard. A transient netlink failure (kernel resource pressure, brief glitch) would unconditionally tear down an actively-used tunnel. Mirror the discrimination already used in connectIpipHandler: only a genuine netlink.LinkNotFoundError counts as vanished. Any other error is logged and the peer is left in place for the next poll to retry. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
The "Protects the fields below" comment on Server.mu predated ipipMu/ipipPeers being added below it. mu only guards newPeers; ipipPeers has its own ipipMu. Scope the comment to newPeers so it no longer over-claims. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The raw/PREROUTING host guard was framed as anti-spoofing for host-local traffic from IPIP peers, but on closer inspection the threat it was meant to stop (peer-map poisoning via forged RemoteAddr in connectIpipHandler) is not practically exploitable: the SYN-ACK for a TCP handshake with a forged inner source routes to the forged address, not back to the attacker, so the TLS+HTTP exchange never completes. Meanwhile the guard breaks legitimate inner-to-host ICMP -- including the mac agent's `ping 10.100.0.1` health check. Drop the guard and instead encode the actual design invariant at the right layer: a requireExternalRemote middleware rejects control-plane requests whose r.RemoteAddr is inside srv.WgCidr, i.e. requests that arrived from inside a VPN tunnel. Both /connect and /connect-ipip key on RemoteAddr (for peer-map and log respectively), so the assumption "RemoteAddr is the client's real outer address" is now enforced once at the seam rather than each handler having to recheck. New control-plane endpoints inherit the protection by wrapping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fdd882f. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the server-side companion to
/connectthat lets vprox terminate IPIP tunnels in addition to WireGuard. Clients that can't run WireGuard (e.g. the macOS static-IP path going through pf) can now POST/connect-ipipwith the sameBearer <password>header, get back an inner IP from the sameWgCidrpool, and immediately have a kernel IPIP tunnel pointing at their HTTPS source address.How it works:
connectIpipHandlerauthenticates with the existing password, parses the HTTPS source address, and allocates an inner IP via the sharedsrv.ipAllocatorso WG and IPIP peers can't collide. Repeated calls from the same client IP are idempotent (the existing tunnel is reused andlastSeenis refreshed); a race between two parallel requests cleans up the loser instead of leaking interfaces or IPs. If the kernel interface vanished out-of-band, the stale entry is dropped and the request falls through to fresh allocation; only a genuinenetlink.LinkNotFoundErrorcounts as vanished so transient lookup failures don't tear down working tunnels.createIpipLinkcreates avp<srv.Index>-<offset>netlink.Iptunwithlocal=srv.BindAddr,remote=<client_ip>, brings it up, and installs a<peerIP>/32 dev <ifname>host route. The suffix is the peer's offset fromsrv.WgCidr.Addr()(length-checked against IFNAMSIZ) so it stays unique across any allowed CIDR width. I intentionally don't putsrv.WgCidr.Addr()/16on the interface (as in the sketch) because that address is already on the WireGuard interface; the /32 route does the equivalent job for return traffic without colliding.StartIptablesadds wildcardvp<srv.Index>-+TCP MSS clamping (inbound and outbound). The FORWARD ACCEPT is installed per peer byaddIpipPeerFilterat/connect-ipiptime, paired with aDROPso each tunnel only forwards transit traffic whose inner source IP matches the peer it was assigned to. The existingMASQUERADE -o BindIfacerule already handles outbound NAT.requireExternalRemotemiddleware that wraps/connectand/connect-ipip: it rejects requests whoser.RemoteAddris insidesrv.WgCidr, i.e. requests that arrived from inside a VPN tunnel. Both handlers treatRemoteAddras identity-relevant input (peer-map key for IPIP, log for WG), so this enforces "RemoteAddr is the client's real outer address" once at the seam instead of each handler having to recheck. An earlierraw/PREROUTINGhost guard was tried and dropped: the threat it was meant to stop (peer-map poisoning via a forged inner source on/connect-ipip) is not practically exploitable because the SYN-ACK for the TLS handshake routes to the forged address, not back to the attacker, and the guard broke legitimate inner-to-host ICMP including the mac agent'sping 10.100.0.1health check.removeIdleIpipPeersLooppolls each tunnel'srx_bytes/tx_bytesvia netlink, refresheslastSeenon activity, and prunes peers idle longer thanPeerIdleTimeout, freeing both the interface and the allocator slot.CleanupIpiptears down every remaining tunnel on shutdown.Response shape matches the request:
{\"AssignedAddr\": \"10.100.0.X/<bits>\"}. Until this lands, the macOS branch's agent will get a clean 404 on/connect-ipiprather than waste a WireGuard peer slot.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled. (Staging)