Fix what review found in the skill: five real defects
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user