From f6e5cf3f0575c996f0c9973d3abf817fac68c0c0 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Mon, 17 Aug 2026 14:04:39 -0700 Subject: [PATCH] Fix what review found in the skill: five real defects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of #29 found bugs I confirmed by reproducing each one. **Every hand-written error message was unreachable.** `tok=$(curl ...)` is a plain assignment, so `set -e` acts on the command substitution before the following `|| die` can run. A wrong password produced exit 22 and no output at all — the most likely way this gets used wrongly, and the least explained. All four captures now go through a `run` helper that takes a *description* rather than echoing the command, because one of them carries the account password in `-u`. **`up` was not idempotent, and the second run destroyed DNS.** The resolv.conf backup was copied unconditionally, so `up --full` twice overwrote the good backup with PIA's own resolvers; the later `down` then "restored" those and left the container with no working DNS and no way back. `up` now runs `down` first. Verified: two `up --full` runs, then `down`, and the backup still holds the original 192.168.65.7. **An empty gateway produced total connectivity loss, reported as healthy.** `$gw` was never validated and `add_route` swallowed every failure to /dev/null. The two half-routes need no gateway and would succeed, so the tunnel captured everything while the exclusions keeping DNS and the Docker host reachable silently did not exist — and `status` still printed "full tunnel". Routes are now fatal on failure, and a via-less default (`$3` is the literal "eth0") is rejected. **The PIA session token was in the process arguments** — confirmed in `ps` and /proc/*/cmdline, a ~24h bearer credential for the account readable by anything in the container. It now goes to curl on stdin as a config. Verified: 60 polls across a full `up`, zero sightings. **The preflight diagnosed the wrong kernel module.** It checked /dev/net/tun and blamed the tun module, but kernel WireGuard is a netlink interface and does not use it — verified by creating one with NET_ADMIN and no tun device. The check is dropped (the container could not have started without the device anyway) and `ip link add` now reports the real dependency. Also: a full tunnel with no DNS servers from PIA used to warn and carry on, which is a tunnel leaking every lookup while reporting itself healthy — now fatal. `down` validates the backup before restoring it, so a truncated one cannot leave the container with no resolver at all. `wg.priv` is shredded on teardown and created under umask 077, because /run rides `docker commit` into the snapshot image. A mistyped `up --ful` is rejected instead of silently giving a test route. entrypoint: `install_feature_skill` gets `local`, a blank-name guard (the disabled branch would otherwise `rm -rf` the whole skills directory under a persisted volume), `-e`/`-L` so a leftover *file* at the destination is cleaned up, and a chown of the parent so `claude` can still add skills of their own when Mission Control is off. When the base image predates the skill it now says so instead of returning silently — and `/opt/triple-c-skills` joins FEATURE_PROBES so the migration pre-flight reports it. Docs corrected to match: neither half reaches an existing project without a migration. `vpn_env_var` extracted and tested, pinning the property the whole removal path rests on — that the variable is emitted as 0 rather than omitted. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 20 ++- HOW-TO-USE.md | 4 + app/src-tauri/src/docker/container.rs | 37 ++++-- app/src-tauri/src/docker/migration.rs | 1 + container/Dockerfile | 4 +- container/entrypoint.sh | 31 ++++- container/skills/pia-vpn/SKILL.md | 52 ++++++-- container/skills/pia-vpn/pia-wg.sh | 169 +++++++++++++++++++------- 8 files changed, 244 insertions(+), 74 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e9a32a9..4e26325 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -342,12 +342,20 @@ container is created once by a very long function where a dropped capability is directly avoids both, which is what the skill does. - **The `pia-vpn` skill is installed *and removed* from `VPN_SUPPORT_ENABLED`.** `container/skills/` is baked to `/opt/triple-c-skills` and `install_feature_skill()` in `entrypoint.sh` copies it into - `~/.claude/skills/` on every start — refreshed each time, so a fix reaches existing projects, and - `rm -rf`'d first, so files dropped from a later version do not linger. The removal branch matters - as much as the install: `~/.claude` is a persisted volume, so a skill left behind after the toggle - goes off would keep instructing an agent to use a capability the container no longer has. Which is - also why the variable is sent as `0` rather than omitted, and why it is in `RESERVED_ENV_EXACT` — - a custom env var of that name could otherwise claim the skill without the capability behind it. + `~/.claude/skills/` on every start — refreshed each time, so a fix reaches any project whose base + image has the source, and `rm -rf`'d first, so files dropped from a later version do not linger. + The removal branch matters as much as the install: `~/.claude` is a persisted volume, so a skill + left behind after the toggle goes off would keep instructing an agent to use a capability the + container no longer has. Which is also why the variable is sent as `0` rather than omitted (see + `vpn_env_var`, tested), and why it is in `RESERVED_ENV_EXACT` — a custom env var of that name + could otherwise claim the skill without the capability behind it. +- **Both halves of that live in the base image, so neither reaches an existing project.** A + recreation builds from the project's *own snapshot*, which has no `/opt/triple-c-skills` and no + updated `entrypoint.sh`; only a migration or a Reset delivers them. The install path says so out + loud rather than returning silently, and `/opt/triple-c-skills` is in `FEATURE_PROBES` so the + migration pre-flight lists it as missing. Worth knowing before adding anything else behind an + existing toggle: the label fingerprints *the setting*, not the set of things the setting drives, + so a project already at `true` gets no recreation at all on upgrade. ### Container Lifecycle diff --git a/HOW-TO-USE.md b/HOW-TO-USE.md index e674406..d36b859 100644 --- a/HOW-TO-USE.md +++ b/HOW-TO-USE.md @@ -495,6 +495,10 @@ easy to get wrong (see the DNS note below), and it is removed again when you tur It needs your PIA credentials in `~/pia-creds`, two lines, username then password. If you use a different provider, ignore it and set up your own client; nothing else depends on it. +Like the VPN tooling above, the skill ships in the container image, so a project whose container +predates it will not get one by toggling the setting — **migrate the project** and it appears; the +migration pre-flight lists it among what you would gain. + With the setting **off**, a client such as PIA or OpenVPN installs and its daemon starts normally, but the connection attempt **hangs until it times out** — a default container has no tun device to open and no permission to add an interface or a route, and most clients report that as a generic timeout diff --git a/app/src-tauri/src/docker/container.rs b/app/src-tauri/src/docker/container.rs index 9a5e864..f4ca0c0 100644 --- a/app/src-tauri/src/docker/container.rs +++ b/app/src-tauri/src/docker/container.rs @@ -841,6 +841,23 @@ type VpnHostConfigParts = ( /// host-kernel module auto-loading. It is also enough to flush netfilter rules /// inside the container, so pair it with `sandbox_mode_enabled` advisedly. /// Hence opt-in, per project, rather than on for everyone. +/// The env var `entrypoint.sh` installs and removes the `pia-vpn` skill from. +/// +/// **Emitted either way, never omitted.** `~/.claude` is a persisted volume, so +/// turning the toggle off has to actively tell entrypoint to remove a skill an +/// earlier run left there, and an absent variable cannot say that. It is also +/// what stops a `=1` baked into a snapshot by `docker commit` from outliving +/// the setting — the explicit `=0` overwrites it. +/// +/// Extracted for the same reason as [`vpn_host_config`]: the emitting code sits +/// in a very long function where a dropped or inverted value is invisible, and +/// `MISSION_CONTROL_ENABLED` twenty lines above shows the failure this avoids — +/// it is pushed only when true, so a snapshot's baked `=1` survives the toggle +/// going off. +fn vpn_env_var(enabled: bool) -> String { + format!("VPN_SUPPORT_ENABLED={}", u8::from(enabled)) +} + fn vpn_host_config(enabled: bool) -> VpnHostConfigParts { if !enabled { return (None, None, None); @@ -1276,14 +1293,7 @@ pub async fn create_container( env_vars.push("MISSION_CONTROL_ENABLED=1".to_string()); } - // Drives the pia-vpn skill install in entrypoint.sh. Sent as 0 rather than - // omitted when off, because ~/.claude is a persisted volume: entrypoint has - // to be told to *remove* a skill left there by an earlier run with the - // toggle on, and an absent variable cannot say that. - env_vars.push(format!( - "VPN_SUPPORT_ENABLED={}", - u8::from(project.vpn_support_enabled) - )); + env_vars.push(vpn_env_var(project.vpn_support_enabled)); // Permission mode — read by triple-c-task-runner for scheduled (headless) // Claude Code runs. Interactive terminals get the flags directly instead. @@ -2799,6 +2809,17 @@ mod tests { assert_eq!(cap_add.unwrap(), vec!["NET_ADMIN"]); } + #[test] + fn the_vpn_skill_flag_is_emitted_either_way_never_omitted() { + // The whole removal path depends on this. If `false` ever became "emit + // nothing", a project that had the toggle on would keep the skill + // forever: the container recreates from a snapshot whose baked + // VPN_SUPPORT_ENABLED=1 would then go unchallenged, and entrypoint + // would reinstall a skill for a capability the container no longer has. + assert_eq!(vpn_env_var(true), "VPN_SUPPORT_ENABLED=1"); + assert_eq!(vpn_env_var(false), "VPN_SUPPORT_ENABLED=0"); + } + #[test] fn the_vpn_skill_flag_is_reserved_from_custom_env() { // entrypoint.sh installs and removes the pia-vpn skill from this diff --git a/app/src-tauri/src/docker/migration.rs b/app/src-tauri/src/docker/migration.rs index 1b0b580..d9b1f61 100644 --- a/app/src-tauri/src/docker/migration.rs +++ b/app/src-tauri/src/docker/migration.rs @@ -136,6 +136,7 @@ pub const FEATURE_PROBES: &[(&str, &str)] = &[ ("/usr/local/bin/triple-c-sso-refresh", "AWS SSO auto-refresh"), ("/opt/mission-control", "Mission Control (Flight Control)"), ("/usr/bin/wg", "VPN tooling (WireGuard, for the VPN Support toggle)"), + ("/opt/triple-c-skills", "Bundled skills (PIA VPN, for the VPN Support toggle)"), ]; /// Headroom demanded on Docker's storage backend on top of the measured diff --git a/container/Dockerfile b/container/Dockerfile index d59de29..3f0523d 100644 --- a/container/Dockerfile +++ b/container/Dockerfile @@ -394,7 +394,9 @@ COPY mission-control /opt/mission-control # no skill at all. Staged in /opt because ~/.claude is a volume mount: an image # copy underneath it would be masked from the project's first start onward. COPY skills /opt/triple-c-skills -RUN chmod +x /opt/triple-c-skills/*/*.sh +# `find`, not a `*/*.sh` glob: the glob fails the build the day a skill ships +# without a script, which is a legitimate thing for a skill to do. +RUN find /opt/triple-c-skills -name '*.sh' -exec chmod +x {} + COPY entrypoint.sh /usr/local/bin/entrypoint.sh RUN chmod +x /usr/local/bin/entrypoint.sh diff --git a/container/entrypoint.sh b/container/entrypoint.sh index 92435d7..f4e049d 100644 --- a/container/entrypoint.sh +++ b/container/entrypoint.sh @@ -347,18 +347,37 @@ fi # Copied on every start rather than only when absent, so a fix to a skill # reaches projects that already have the old copy. Local edits under these # directories do not survive — treat /opt/triple-c-skills as the source. +# +# The source lives in the *base image*, so a project whose container predates it +# recreates from its own snapshot and has no /opt/triple-c-skills to copy from. +# That case says so rather than returning silently: the toggle is on, the +# capability is there, and the skill simply never appears — which is impossible +# to work out from the outside. install_feature_skill() { - _name=$1 - _enabled=$2 - _dest="/home/claude/.claude/skills/$_name" + local _name="$1" + local _enabled="$2" + local _src="/opt/triple-c-skills/$1" + local _dest="/home/claude/.claude/skills/$1" + + # A blank name would make the disabled branch `rm -rf` the whole skills + # directory, Mission Control's included, under a persisted volume. + [ -n "$_name" ] || { echo "entrypoint: install_feature_skill called with no name"; return 1; } + if [ "$_enabled" = "1" ]; then - [ -d "/opt/triple-c-skills/$_name" ] || return 0 + if [ ! -d "$_src" ]; then + echo "entrypoint: $_name skill unavailable — this container's base image predates it; migrate the project to get it" + return 0 + fi mkdir -p /home/claude/.claude/skills + # Not just $_dest: when Mission Control is off nothing else creates the + # parent, so root would own it and `claude` could not add a skill there. + chown claude:claude /home/claude/.claude/skills rm -rf "$_dest" - cp -r "/opt/triple-c-skills/$_name" "$_dest" + cp -r "$_src" "$_dest" chown -R claude:claude "$_dest" echo "entrypoint: $_name skill installed to ~/.claude/skills/" - elif [ -d "$_dest" ]; then + elif [ -e "$_dest" ] || [ -L "$_dest" ]; then + # -e/-L rather than -d: a leftover *file* at that path must go too. rm -rf "$_dest" echo "entrypoint: $_name skill removed (feature disabled)" fi diff --git a/container/skills/pia-vpn/SKILL.md b/container/skills/pia-vpn/SKILL.md index d7554ad..52cffc0 100644 --- a/container/skills/pia-vpn/SKILL.md +++ b/container/skills/pia-vpn/SKILL.md @@ -65,6 +65,19 @@ PIA's own resolvers through the tunnel with `/32` routes that outrank the `10/8` exclusion. If you ever route traffic by hand, you owe both halves — the exclusions *and* a resolver reachable from wherever you pointed the default. +The failure has a quiet twin. Do only the first half — exclude the private +ranges, leave the resolver alone — and everything *works*, while every DNS +query travels outside the tunnel to your ISP. A VPN that leaks the full list of +what you looked up is worse than one that is visibly broken, so `up --full` +refuses to proceed if PIA does not hand back resolvers rather than carrying on +without them. + +The mechanism above is Docker Desktop's. On a user-defined Docker network the +resolver is `127.0.0.11`, which is loopback and never captured by a default +route — the trap still exists there (that resolver forwards upstream from +inside the container's namespace) but arrives by a different path. Check +`/etc/resolv.conf` rather than assuming which case you are in. + ## Trap 2: an IP-literal health check cannot see a dead resolver `curl https://1.1.1.1/cdn-cgi/trace` needs no DNS, so it returns a cheerful @@ -116,9 +129,20 @@ p1234567 your-password ``` -Set `PIA_CREDS` to use a different path. Treat the contents as secret: never -print the file, never echo the values, and never include them in a commit, a -log or a message. The script reads it directly and does not echo it. +Treat the contents as secret: never print the file, never echo the values, and +never include them in a commit, a log or a message. The script reads it directly +and does not echo it, and passes PIA's session token to `curl` on stdin rather +than in the argv, where `ps` would expose it to everything in the container. + +`PIA_CREDS` points somewhere else — but `sudo` resets the environment, so it +only takes effect **after** the word `sudo`: + +```bash +sudo PIA_CREDS=/path/to/creds ~/.claude/skills/pia-vpn/pia-wg.sh up # works +PIA_CREDS=/path/to/creds sudo ~/.claude/skills/pia-vpn/pia-wg.sh up # ignored +``` + +The second form fails silently back to the default path. Same for `PIA_REGION`. ## Regions @@ -156,10 +180,16 @@ true in test mode too, and means much less than it sounds like. ## Tearing down -`down` restores `resolv.conf` from its backup and removes exactly the routes -that were added, in reverse order, then deletes the interface. It is safe to -run when nothing is up. Confirm afterwards that the public address is back to -the container's own. +`down` restores `resolv.conf` from its backup (only if that backup still looks +like a resolver file — restoring a truncated one would leave the container with +no DNS at all), removes exactly the routes that were added, in reverse order, +deletes the interface, and shreds the WireGuard private key. It is safe to run +when nothing is up, and `up` runs it first so a repeat `up` cannot stack state. +Confirm afterwards that the public address is back to the container's own. + +The key deletion is not housekeeping: `/run` is in the container's writable +layer, so `docker commit` bakes whatever is there into the project's snapshot +image. A key left behind rides that image into every future container. ## What this deliberately does not do @@ -173,3 +203,11 @@ the container's own. cannot work headless: the daemon never accepts a client connection without the GUI, and `piactl --help` states that connecting requires it. If you find one installed, it is not a working alternative to this script. +- **Not `wg-quick`.** Its `Table=auto` full-tunnel mode routes by firewall mark + and needs `xt_CONNMARK` from the host kernel, which Docker Desktop for + Windows (WSL2) does not have and a container cannot load. This script adds + the routes with `ip route` directly, which works on every host. +- **IPv4 only.** The `0.0.0.0/1` + `128.0.0.0/1` pair covers v4. A container + with a global IPv6 address and a v6 default route would leak all v6 traffic + outside the tunnel; Triple-C's containers do not have one by default, but + check `ip -6 route show default` before relying on this where it matters. diff --git a/container/skills/pia-vpn/pia-wg.sh b/container/skills/pia-vpn/pia-wg.sh index 8e5c8c6..3b1ce4f 100644 --- a/container/skills/pia-vpn/pia-wg.sh +++ b/container/skills/pia-vpn/pia-wg.sh @@ -13,13 +13,20 @@ # # Requires the project's "VPN support" setting (Config -> Runtime) to be on. # +# Settings are read from the environment, but note that sudo resets it: they +# have to be passed *through* sudo, after the word `sudo`, not before it. +# +# sudo PIA_REGION=uk_london pia-wg.sh up --full # works +# PIA_REGION=uk_london sudo pia-wg.sh up --full # silently ignored +# # PIA_CREDS credentials file, two lines: username, then password -# (default ~/pia-creds; never echoed by this script) +# (default /home/claude/pia-creds; never echoed by this script) # PIA_REGION region id (default us_chicago). List them with: # curl -s https://serverlist.piaservers.net/vpninfo/servers/v6 \ # | head -1 | jq -r '.regions[].id' set -euo pipefail +# Not ~/pia-creds: under sudo, HOME is /root. CREDS=${PIA_CREDS:-/home/claude/pia-creds} REGION=${PIA_REGION:-us_chicago} IFACE=pia0 @@ -36,11 +43,28 @@ PRIVATE_NETS="10.0.0.0/8 172.16.0.0/12 192.168.0.0/16 169.254.0.0/16" # source lines without the indentation ending up in the output. die() { echo "pia-wg: $*" >&2; exit 1; } +# `x=$(cmd)` is a plain assignment, so `set -e` kills the script on a non-zero +# cmd *before* any `[ -z "$x" ] || die` line can run. Every capture below +# therefore goes through `run`; without it a wrong password exits 22 with no +# output at all, which is the most likely way this is used wrongly and was the +# least explained. +# +# It takes a description rather than reporting the command it ran: one of these +# invocations carries the account password in `-u`, and an error message is +# exactly the wrong place for that to surface. +run() { local what=$1; shift; "$@" || die "$what (exit $?)"; } + preflight() { [ "$(id -u)" = 0 ] || die "run with sudo" # CAP_NET_ADMIN is bit 12. Checking it by name gives a usable error; without # it the first `ip` call fails with a bare "Operation not permitted" that # points nowhere near the setting that actually needs changing. + # + # Deliberately NOT checking /dev/net/tun: kernel WireGuard is a netlink + # interface and does not use it (verified -- `ip link add type wireguard` + # succeeds with NET_ADMIN and no tun device). It is OpenVPN and userspace + # wireguard-go that need it. The real kernel dependency here is the + # `wireguard` module, which `ip link add` below reports on directly. local caps caps=$(awk '/^CapEff:/{print $2}' /proc/self/status) if [ $(( 0x$caps & 0x1000 )) -eq 0 ]; then @@ -49,51 +73,88 @@ preflight() { "again. That recreates the container; the home and .claude volumes" \ "are preserved, so nothing in them is lost." fi - [ -e /dev/net/tun ] || \ - die "/dev/net/tun is missing." \ - "Same fix: turn on \"VPN support\" in Config -> Runtime. If it is" \ - "already on, the Docker host's kernel is missing the tun module." - command -v wg >/dev/null || die "wireguard-tools is not installed." + command -v wg >/dev/null || \ + die "wireguard-tools is not installed." \ + "If this project's container was built from an older base image," \ + "migrate it onto the current one -- that is what ships \`wg\`." [ -r "$CREDS" ] || \ die "no credentials at $CREDS." \ "Two lines are expected: username, then password." \ - "Set PIA_CREDS to read them from somewhere else." + "Set PIA_CREDS (after the word \`sudo\`) to read them elsewhere." } -# Record every route we add so teardown removes exactly those and nothing else. -add_route() { ip route add $1 2>/dev/null && echo "$1" >> "$STATE/routes" || true; } +# Routes that must work. A silent failure here is the worst state this script +# can reach: the two half-routes need no gateway and would succeed, so the +# tunnel captures everything while the exclusions that keep DNS and the Docker +# host reachable are quietly missing -- and `status` still says "full tunnel". +add_route() { + ip route add "$@" || die "could not add route '$*'. Run 'down' to undo the partial setup." + printf '%s\n' "$*" >> "$STATE/routes" +} up() { + case "${1:-}" in + ""|--full) ;; + *) die "unknown option '$1' (expected --full or nothing)." \ + "Refusing rather than silently giving you a test route." ;; + esac preflight + + # Always start from a known state. Without this a second `up` overwrites the + # saved resolv.conf with PIA's own resolvers, so the later `down` "restores" + # those and leaves the container with no working DNS and no way back. + down >/dev/null 2>&1 || true + mkdir -p "$STATE"; cd "$STATE" - [ -f ca.rsa.4096.crt ] || curl -sf -m 20 -o ca.rsa.4096.crt \ - https://raw.githubusercontent.com/pia-foss/manual-connections/master/ca.rsa.4096.crt \ - || die "could not fetch PIA's CA certificate" + + # `curl -o` creates the file before it knows the request failed, so a plain + # `[ -f ]` cache check can pin a truncated cert forever -- and /run rides the + # snapshot, so "forever" outlives the container. Fetch to a temp name and + # rename only on success. + if [ ! -s ca.rsa.4096.crt ]; then + run "could not download PIA's CA certificate" \ + curl -sf -m 20 -o ca.crt.part \ + https://raw.githubusercontent.com/pia-foss/manual-connections/master/ca.rsa.4096.crt + [ -s ca.crt.part ] || die "PIA's CA certificate downloaded empty" + mv ca.crt.part ca.rsa.4096.crt + fi local u p tok srv sip scn priv pub resp ep gw dns u=$(sed -n 1p "$CREDS"); p=$(sed -n 2p "$CREDS") - tok=$(curl -sf -m 25 -u "$u:$p" \ - https://www.privateinternetaccess.com/gtoken/generateToken | jq -r .token) - [ -n "$tok" ] && [ "$tok" != null ] || die "PIA authentication failed - check $CREDS" + [ -n "$u" ] && [ -n "$p" ] || die "$CREDS needs two lines: username, then password" - curl -sf -m 30 https://serverlist.piaservers.net/vpninfo/servers/v6 | head -1 > servers.json + tok=$(run "PIA rejected the credentials in $CREDS, or could not be reached" \ + curl -sf -m 25 -u "$u:$p" \ + https://www.privateinternetaccess.com/gtoken/generateToken | jq -r .token) + [ -n "$tok" ] && [ "$tok" != null ] || die "PIA returned no token - check the credentials in $CREDS" + + run "could not fetch PIA's server list" \ + curl -sf -m 30 https://serverlist.piaservers.net/vpninfo/servers/v6 \ + | head -1 > servers.json srv=$(jq -r --arg r "$REGION" '.regions[] | select(.id==$r) | .servers.wg[0]' servers.json) sip=$(echo "$srv" | jq -r .ip); scn=$(echo "$srv" | jq -r .cn) - [ -n "$sip" ] && [ "$sip" != null ] || die "no WireGuard server for region $REGION" + [ -n "$sip" ] && [ "$sip" != null ] || die "no WireGuard server for region '$REGION'" - priv=$(wg genkey); pub=$(echo "$priv" | wg pubkey) - printf '%s' "$priv" > wg.priv; chmod 600 wg.priv + # umask, not a later chmod: the file is created under the inherited 0022 + # otherwise, so the key is world-readable for the moment in between. + ( umask 077; priv=$(wg genkey); printf '%s' "$priv" > wg.priv ) + priv=$(cat wg.priv); pub=$(printf '%s' "$priv" | wg pubkey) + # The token goes in on stdin as a curl config rather than in the argv, where + # `ps` and /proc/*/cmdline expose it to every process in the container -- + # verified. It is a ~24h bearer credential for the whole PIA account. # PIA pins its certificate to the server's common name, which is why this # connects by CN and lets --connect-to point that name at the real address. - resp=$(curl -sf -m 25 -G --connect-to "$scn::$sip:" --cacert ca.rsa.4096.crt \ - --data-urlencode "pt=$tok" --data-urlencode "pubkey=$pub" \ - "https://$scn:1337/addKey") + resp=$(printf -- '--data-urlencode "pt=%s"\n--data-urlencode "pubkey=%s"\n' "$tok" "$pub" \ + | run "could not register the key with $scn" \ + curl -sf -m 25 -G -K - --connect-to "$scn::$sip:" \ + --cacert ca.rsa.4096.crt "https://$scn:1337/addKey") [ "$(echo "$resp" | jq -r .status)" = OK ] || die "key registration failed: $resp" : > "$STATE/routes" - ip link del "$IFACE" 2>/dev/null || true - ip link add "$IFACE" type wireguard + ip link add "$IFACE" type wireguard 2>/dev/null || \ + die "could not create a WireGuard interface." \ + "The Docker host's kernel has no 'wireguard' module." wg set "$IFACE" private-key wg.priv \ peer "$(echo "$resp" | jq -r .server_key)" \ endpoint "$(echo "$resp" | jq -r .server_ip):$(echo "$resp" | jq -r .server_port)" \ @@ -102,36 +163,41 @@ up() { ip link set "$IFACE" up if [ "${1:-}" = "--full" ]; then + ep=$(echo "$resp" | jq -r .server_ip) + gw=$(ip route show default | awk '{print $3; exit}') + # `default dev eth0` with no `via` yields the literal "eth0" here, which + # would make every exclusion below a malformed no-op. + [[ $gw =~ ^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$ ]] || \ + die "no usable default gateway to pin the tunnel against (got '${gw:-none}')" + + # PIA's resolvers are required in --full. Without them the 10/8 exclusion + # below is already in place, so every lookup would go to the container's + # own resolver *outside* the tunnel -- a full tunnel leaking all its DNS, + # reported by `status` as perfectly healthy. + dns=$(echo "$resp" | jq -r '.dns_servers[]? // empty' | head -2) + [ -n "$dns" ] || die "PIA returned no DNS servers; refusing a full tunnel that would leak every lookup" + # Pin the endpoint to the pre-existing gateway first, so the tunnel's own # packets do not try to route through the tunnel. Then beat the default # route with two half-routes rather than replacing it -- nothing to restore # on teardown, and the container keeps working if this script dies midway. - ep=$(echo "$resp" | jq -r .server_ip) - gw=$(ip route show default | awk '{print $3; exit}') - add_route "$ep/32 via $gw" - add_route "0.0.0.0/1 dev $IFACE" - add_route "128.0.0.0/1 dev $IFACE" + add_route "$ep/32" via "$gw" + add_route 0.0.0.0/1 dev "$IFACE" + add_route 128.0.0.0/1 dev "$IFACE" # Keep container, host and LAN traffic off the tunnel. Longer prefixes than # the two halves above, so these win. - for n in $PRIVATE_NETS; do add_route "$n via $gw"; done + for n in $PRIVATE_NETS; do add_route "$n" via "$gw"; done - # PIA's resolver lives inside 10/8, so pin it back through the tunnel with a - # /32 -- longer still, so it beats the 10.0.0.0/8 exclusion just added. - # Using PIA's resolver rather than the container's keeps DNS from leaking, - # and the container's own resolver is unreachable from inside the tunnel. - dns=$(echo "$resp" | jq -r '.dns_servers[]? // empty' | head -2) - if [ -n "$dns" ]; then - cp /etc/resolv.conf "$STATE/resolv.conf.bak" - for d in $dns; do add_route "$d/32 dev $IFACE"; done - # resolv.conf is a bind mount: write through it, never replace it. - for d in $dns; do echo "nameserver $d"; done > /etc/resolv.conf - else - echo "pia-wg: warning - PIA returned no DNS servers; leaving resolv.conf alone" >&2 - fi + # PIA's resolvers live inside 10/8, so pin them back through the tunnel with + # /32s -- longer still, so they beat the exclusion just added. + cp /etc/resolv.conf "$STATE/resolv.conf.bak" + for d in $dns; do add_route "$d/32" dev "$IFACE"; done + # resolv.conf is a bind mount: write through it, never replace it. + for d in $dns; do echo "nameserver $d"; done > /etc/resolv.conf echo "full tunnel: public traffic exits via PIA; private ranges stay local" else - add_route "1.1.1.1/32 dev $IFACE" + add_route 1.1.1.1/32 dev "$IFACE" echo "test route only: 1.1.1.1 goes via PIA, everything else unchanged" fi @@ -141,8 +207,15 @@ up() { down() { [ "$(id -u)" = 0 ] || die "run with sudo" + # Only restore something that actually looks like a resolver file. Restoring + # an empty or truncated backup leaves the container with no DNS at all, which + # is worse than leaving the current one alone. if [ -f "$STATE/resolv.conf.bak" ]; then - cat "$STATE/resolv.conf.bak" > /etc/resolv.conf + if grep -q '^nameserver' "$STATE/resolv.conf.bak" 2>/dev/null; then + cat "$STATE/resolv.conf.bak" > /etc/resolv.conf + else + echo "pia-wg: warning - saved resolv.conf looks empty; leaving the current one alone" >&2 + fi rm -f "$STATE/resolv.conf.bak" fi if [ -f "$STATE/routes" ]; then @@ -153,6 +226,10 @@ down() { rm -f "$STATE/routes" fi ip link del "$IFACE" 2>/dev/null || true + # /run is in the writable layer and `docker commit` bakes it into the + # project's snapshot image, so a key left here rides that image into every + # future container. Verified: a snapshot already carried one. + rm -f "$STATE/wg.priv" echo "tunnel down" } @@ -197,5 +274,5 @@ case "${1:-}" in up) shift; up "${1:-}" ;; down) down ;; status) status ;; - *) sed -n '2,20p' "$0" | sed 's/^# \{0,1\}//'; exit 1 ;; + *) sed -n '2,27p' "$0" | sed 's/^# \{0,1\}//'; exit 1 ;; esac