diff --git a/container/entrypoint.sh b/container/entrypoint.sh index f4e049d..2bb9d95 100644 --- a/container/entrypoint.sh +++ b/container/entrypoint.sh @@ -359,21 +359,31 @@ install_feature_skill() { 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; } + # Reject anything that is not a plain directory name. The disabled branch + # `rm -rf`s $_dest under a *persisted volume*, so a blank name would take the + # whole skills directory (Mission Control's included) and `../x` would escape + # it entirely. Only the literal `pia-vpn` is passed today; this is so that + # stays true. + case "$_name" in + ''|*/*|.*) echo "entrypoint: install_feature_skill: bad skill name '$_name'"; return 1 ;; + esac if [ "$_enabled" = "1" ]; then 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 + # Checked, not assumed: with no `set -e` in this script every step here + # can fail (full volume, read-only mount, a file where the directory + # should be) and the success line would still print. + mkdir -p /home/claude/.claude/skills || { + echo "entrypoint: $_name skill install FAILED (cannot create ~/.claude/skills)"; return 1; } # 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 "$_src" "$_dest" + cp -r "$_src" "$_dest" || { + echo "entrypoint: $_name skill install FAILED (copy from $_src)"; return 1; } chown -R claude:claude "$_dest" echo "entrypoint: $_name skill installed to ~/.claude/skills/" elif [ -e "$_dest" ] || [ -L "$_dest" ]; then diff --git a/container/skills/pia-vpn/SKILL.md b/container/skills/pia-vpn/SKILL.md index 52cffc0..7dea400 100644 --- a/container/skills/pia-vpn/SKILL.md +++ b/container/skills/pia-vpn/SKILL.md @@ -183,22 +183,34 @@ true in test mode too, and means much less than it sounds like. `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. +and 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. -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. +`up` calls it too, but only *after* every network fetch has succeeded, so a +failed `up` leaves an existing tunnel alone rather than tearing it down to +report a bad password. From that point on a rollback is armed: if any step of +the setup fails, the tunnel is torn down rather than left half-configured. + +The private key is deleted earlier still — the moment `wg set` has read it, +while the tunnel is being built. That is not housekeeping: `/run` is in the +container's writable layer, and recreating or migrating the project runs +`docker commit` over it *without* tearing the tunnel down first. A key that +lived for the tunnel's lifetime would be baked into the snapshot image and +copied forward from then on. The kernel keeps its own copy, so nothing is lost. ## What this deliberately does not do -- **No killswitch.** Blocking non-tunnel egress needs `iptables`, which is not - in the image, and would cut Claude Code's API traffic whenever the tunnel is - down. If the user needs guaranteed egress rather than convenient egress, say - so plainly rather than improvising one — it is a real design decision. +- **No killswitch.** `iptables` *is* in the image, so one is buildable — this + is a deliberate omission, not a missing dependency. Blocking non-tunnel egress + cuts Claude Code's own API traffic the moment the tunnel drops, which ends the + session that would otherwise fix it. If the user needs guaranteed egress + rather than convenient egress, say so plainly and let them decide, rather than + improvising one. - **No autostart.** There is no service manager in the container and Triple-C - has no start hook, so nothing can re-establish the tunnel automatically. + has no start hook, so nothing re-establishes the tunnel on its own. `cron` is + in the image and `triple-c-scheduler` runs on it, so a scheduled reconnect is + possible if the user wants one — it is just not set up, and a tunnel that + reconnects unattended deserves an explicit decision. - **Not PIA's desktop client.** `pia-daemon` and `piactl` are installable but cannot work headless: the daemon never accepts a client connection without the GUI, and `piactl --help` states that connecting requires it. If you find diff --git a/container/skills/pia-vpn/pia-wg.sh b/container/skills/pia-vpn/pia-wg.sh index 3b1ce4f..50bea37 100644 --- a/container/skills/pia-vpn/pia-wg.sh +++ b/container/skills/pia-vpn/pia-wg.sh @@ -27,6 +27,9 @@ set -euo pipefail # Not ~/pia-creds: under sudo, HOME is /root. +# Read by up()'s EXIT trap, which runs after the function's locals are gone. +SETUP_OK=0 + CREDS=${PIA_CREDS:-/home/claude/pia-creds} REGION=${PIA_REGION:-us_chicago} IFACE=pia0 @@ -88,7 +91,7 @@ preflight() { # 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." + ip route add "$@" || die "could not add route '$*'" printf '%s\n' "$*" >> "$STATE/routes" } @@ -100,11 +103,6 @@ up() { 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" # `curl -o` creates the file before it knows the request failed, so a plain @@ -135,6 +133,27 @@ up() { sip=$(echo "$srv" | jq -r .ip); scn=$(echo "$srv" | jq -r .cn) [ -n "$sip" ] && [ "$sip" != null ] || die "no WireGuard server for region '$REGION'" + # Only now tear down any previous tunnel. Doing it up front (as an earlier + # version did) meant a failed token fetch or an unreachable server list took + # a *working* tunnel down with it and silently reverted the container to its + # real address, while the error talked about credentials. Everything above + # this line can fail; nothing above it has touched the network stack. + # + # It also still does the job it was added for: clearing a stale resolv.conf + # backup so a second `up` cannot save PIA's own resolvers over the real ones. + down >/dev/null 2>&1 || true + + # From here on the network stack is being modified, so any failure has to put + # it back rather than exit half-configured. `down` is idempotent and restores + # routes and resolv.conf exactly. + # + # EXIT rather than ERR, and a flag rather than the trap's own exit status: an + # ERR trap is not inherited by shell functions without `set -E`, so a failure + # inside add_route would not fire it, and `die` exits explicitly, which is not + # an error and would not fire it either. EXIT catches both. + SETUP_OK=0 + trap '[ "$SETUP_OK" = 1 ] || { echo "pia-wg: setup failed - rolling back" >&2; down >/dev/null 2>&1; }' EXIT + # 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 ) @@ -159,6 +178,12 @@ up() { peer "$(echo "$resp" | jq -r .server_key)" \ endpoint "$(echo "$resp" | jq -r .server_ip):$(echo "$resp" | jq -r .server_port)" \ allowed-ips 0.0.0.0/0 persistent-keepalive 25 + # The kernel holds the key from here, so the file has no reason to outlive + # this line -- and every reason not to: /run is in the writable layer, and a + # recreate or migrate runs `docker commit` over it without tearing the tunnel + # down first, baking the key into the project's snapshot image. `down` also + # removes it, for the case where `up` never got this far. + rm -f wg.priv ip addr add "$(echo "$resp" | jq -r .peer_ip)/32" dev "$IFACE" ip link set "$IFACE" up @@ -201,7 +226,18 @@ up() { echo "test route only: 1.1.1.1 goes via PIA, everything else unchanged" fi - sleep 2 + # A tunnel with no handshake still routes -- into a black hole. Without this + # `up --full` would exit 0 having pointed all traffic *and* resolv.conf at a + # peer that never answered, and `status` would print "mode: full tunnel". + local waited=0 + until [ "$(wg show "$IFACE" latest-handshakes | awk '{print $2; exit}')" != 0 ]; do + waited=$((waited + 1)) + [ "$waited" -lt 20 ] || die "no handshake from $REGION after 10s - rolled back" + sleep 0.5 + done + + SETUP_OK=1 + trap - EXIT status } @@ -242,6 +278,10 @@ TRACE_DIRECT=https://1.0.0.1/cdn-cgi/trace exit_ip() { curl -s -m 20 "$1" | sed -n 's/^ip=//p'; } status() { + # `wg show` needs root; `ip route`/`ip link` do not. Without this guard an + # unprivileged run prints "no tunnel up" and then "mode: full tunnel" in the + # same breath, and an agent reading the first line re-runs `up`. + [ "$(id -u)" = 0 ] || die "run with sudo" wg show "$IFACE" 2>/dev/null | grep -E "latest handshake|transfer" || echo "no tunnel up" # Resolve a name, not an IP literal. A curl to 1.1.1.1 succeeds while DNS is @@ -274,5 +314,5 @@ case "${1:-}" in up) shift; up "${1:-}" ;; down) down ;; status) status ;; - *) sed -n '2,27p' "$0" | sed 's/^# \{0,1\}//'; exit 1 ;; + *) sed -n '2,26p' "$0" | sed 's/^# \{0,1\}//'; exit 1 ;; esac