Skip to content

fix: harden WireGuard setup routes#55

Merged
protostatis merged 1 commit into
mainfrom
fix/wireguard-setup-routes
May 12, 2026
Merged

fix: harden WireGuard setup routes#55
protostatis merged 1 commit into
mainfrom
fix/wireguard-setup-routes

Conversation

@protostatis
Copy link
Copy Markdown
Owner

Summary

  • Make WireGuard setup tolerate existing routing rules and bypass routes.
  • Use route replacement and guarded cleanup so reruns do not leave VPN/DNS in a broken state.

Verification

  • Reviewed diff for secrets: only route/rule commands and existing env placeholders are included.
  • Ran bash -n deploy/setup-wireguard.sh.

@protostatis protostatis merged commit 2d9034d into main May 12, 2026
@protostatis protostatis deleted the fix/wireguard-setup-routes branch May 12, 2026 00:32
Copy link
Copy Markdown
Owner Author

@protostatis protostatis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sky's Code Review

This PR hardens WireGuard PostUp/PostDown hooks by using ip rule add/del with 2>/dev/null || true and switching ip route add to ip route replace for idempotency. The changes improve rerun safety but introduce a spec mismatch in PostDown route deletions — the added via $EC2_GW in PostUp is not reflected in the corresponding route del commands, making them underspecified. The blanket || true on all PostDown commands also masks real errors during interface teardown.

Verdict: Comment

Comments

  • The ip route replace in PostUp is a good idempotency improvement — it avoids duplicate-route errors on reruns.
  • Verify the environment variables $EC2_IP and $EC2_GW are always set before this script runs. If $EC2_GW is empty, ip route replace ... via dev eth0 would silently create an incorrect route or error out.
  • Consider testing: bring interface up twice in succession, then bring it down, and confirm no stale rules/routes remain.

Reviewed by Sky — Unchained Sky engineering agent

Comment thread deploy/setup-wireguard.sh
PostUp = /sbin/ip rule add from $EC2_IP table main priority 90; /sbin/iptables -t mangle -A PREROUTING -i eth0 -m conntrack --ctstate NEW -j CONNMARK --set-mark 0xca6c; /sbin/iptables -t mangle -A PREROUTING -j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff; /sbin/ip route add 140.82.112.0/20 via $EC2_GW dev eth0; /sbin/ip route add 185.199.108.0/22 via $EC2_GW dev eth0; /sbin/ip route add 169.254.169.254 dev eth0
PostDown = /sbin/ip rule del from $EC2_IP table main priority 90; /sbin/iptables -t mangle -D PREROUTING -i eth0 -m conntrack --ctstate NEW -j CONNMARK --set-mark 0xca6c; /sbin/iptables -t mangle -D PREROUTING -j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff; /sbin/ip route del 140.82.112.0/20 dev eth0; /sbin/ip route del 185.199.108.0/22 dev eth0; /sbin/ip route del 169.254.169.254 dev eth0
PostUp = /sbin/ip rule add from $EC2_IP table main priority 90 2>/dev/null || true; /sbin/iptables -t mangle -A PREROUTING -i eth0 -m conntrack --ctstate NEW -j CONNMARK --set-mark 0xca6c; /sbin/iptables -t mangle -A PREROUTING -j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff; /sbin/ip route replace 140.82.112.0/20 via $EC2_GW dev eth0; /sbin/ip route replace 185.199.108.0/22 via $EC2_GW dev eth0; /sbin/ip route replace 169.254.169.254 dev eth0
PostDown = /sbin/ip rule del from $EC2_IP table main priority 90 2>/dev/null || true; /sbin/iptables -t mangle -D PREROUTING -i eth0 -m conntrack --ctstate NEW -j CONNMARK --set-mark 0xca6c 2>/dev/null || true; /sbin/iptables -t mangle -D PREROUTING -j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff 2>/dev/null || true; /sbin/ip route del 140.82.112.0/20 via $EC2_GW dev eth0 2>/dev/null || true; /sbin/ip route del 185.199.108.0/22 via $EC2_GW dev eth0 2>/dev/null || true; /sbin/ip route del 169.254.169.254 dev eth0 2>/dev/null || true
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good: ip rule add ... || true and ip route replace make PostUp idempotent and rerun-safe.

Comment thread deploy/setup-wireguard.sh
PostDown = /sbin/ip rule del from $EC2_IP table main priority 90; /sbin/iptables -t mangle -D PREROUTING -i eth0 -m conntrack --ctstate NEW -j CONNMARK --set-mark 0xca6c; /sbin/iptables -t mangle -D PREROUTING -j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff; /sbin/ip route del 140.82.112.0/20 dev eth0; /sbin/ip route del 185.199.108.0/22 dev eth0; /sbin/ip route del 169.254.169.254 dev eth0
PostUp = /sbin/ip rule add from $EC2_IP table main priority 90 2>/dev/null || true; /sbin/iptables -t mangle -A PREROUTING -i eth0 -m conntrack --ctstate NEW -j CONNMARK --set-mark 0xca6c; /sbin/iptables -t mangle -A PREROUTING -j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff; /sbin/ip route replace 140.82.112.0/20 via $EC2_GW dev eth0; /sbin/ip route replace 185.199.108.0/22 via $EC2_GW dev eth0; /sbin/ip route replace 169.254.169.254 dev eth0
PostDown = /sbin/ip rule del from $EC2_IP table main priority 90 2>/dev/null || true; /sbin/iptables -t mangle -D PREROUTING -i eth0 -m conntrack --ctstate NEW -j CONNMARK --set-mark 0xca6c 2>/dev/null || true; /sbin/iptables -t mangle -D PREROUTING -j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff 2>/dev/null || true; /sbin/ip route del 140.82.112.0/20 via $EC2_GW dev eth0 2>/dev/null || true; /sbin/ip route del 185.199.108.0/22 via $EC2_GW dev eth0 2>/dev/null || true; /sbin/ip route del 169.254.169.254 dev eth0 2>/dev/null || true

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostDown route deletions are missing the via $EC2_GW specifier that PostUp now uses (e.g. /sbin/ip route del 140.82.112.0/20 via $EC2_GW dev eth0). Without it, ip route del may match any route to that prefix regardless of gateway, which is underspecified and could delete an unintended route. Add the via $EC2_GW clause for symmetry with PostUp.

Comment thread deploy/setup-wireguard.sh
PostDown = /sbin/ip rule del from $EC2_IP table main priority 90; /sbin/iptables -t mangle -D PREROUTING -i eth0 -m conntrack --ctstate NEW -j CONNMARK --set-mark 0xca6c; /sbin/iptables -t mangle -D PREROUTING -j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff; /sbin/ip route del 140.82.112.0/20 dev eth0; /sbin/ip route del 185.199.108.0/22 dev eth0; /sbin/ip route del 169.254.169.254 dev eth0
PostUp = /sbin/ip rule add from $EC2_IP table main priority 90 2>/dev/null || true; /sbin/iptables -t mangle -A PREROUTING -i eth0 -m conntrack --ctstate NEW -j CONNMARK --set-mark 0xca6c; /sbin/iptables -t mangle -A PREROUTING -j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff; /sbin/ip route replace 140.82.112.0/20 via $EC2_GW dev eth0; /sbin/ip route replace 185.199.108.0/22 via $EC2_GW dev eth0; /sbin/ip route replace 169.254.169.254 dev eth0
PostDown = /sbin/ip rule del from $EC2_IP table main priority 90 2>/dev/null || true; /sbin/iptables -t mangle -D PREROUTING -i eth0 -m conntrack --ctstate NEW -j CONNMARK --set-mark 0xca6c 2>/dev/null || true; /sbin/iptables -t mangle -D PREROUTING -j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff 2>/dev/null || true; /sbin/ip route del 140.82.112.0/20 via $EC2_GW dev eth0 2>/dev/null || true; /sbin/ip route del 185.199.108.0/22 via $EC2_GW dev eth0 2>/dev/null || true; /sbin/ip route del 169.254.169.254 dev eth0 2>/dev/null || true

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The blanket || true on every PostDown command (iptables -D, ip route del) masks real errors during WireGuard teardown. Consider dropping || true from the iptables CONNMARK rules — if those rules exist in PostUp but cannot be removed in PostDown, something is genuinely broken and the script should fail. Keep || true only for the ip rule del and ip route del where races during shutdown are expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant