Stop a failed up from tearing down a working tunnel
The previous commit moved `down` to the top of `up` to fix a resolv.conf idempotency bug, and in doing so put it *before* every network fetch that can fail. Re-review caught it and I reproduced it: with a tunnel up, an `up` that fails on bad credentials left `pia0` gone and traffic silently back on the real address, while the error talked only about credentials. A privacy regression introduced by a correctness fix. `down` now runs after the token, server list and key registration have all succeeded — nothing above that line touches the network stack — and still clears the stale backup it was added for. Everything after it is covered by a rollback. Note this is an EXIT trap with a flag, not `trap ... ERR`: my first attempt used ERR and did not fire at all, because ERR is not inherited by shell functions without `set -E`, so a failure inside add_route missed it, and `die` exits explicitly, which is not an error. Verified by forcing a route collision — the tunnel is torn down and DNS is intact, where before the fix it was left half-configured with DNS dead. Also from the review: - **The private key lived on disk for the whole life of the tunnel.** `commit_container_snapshot` blanks env vars, never files, and nothing tears the tunnel down before a recreate or migrate — so the `down`-time cleanup never covered the path that put a key in a snapshot in the first place. It is now deleted the moment `wg set` has read it; the kernel keeps its own copy, verified by checking the interface still works afterwards. - **`up` claimed success without a handshake.** An unreachable peer still routes — into a black hole — so `up --full` could exit 0 having pointed all traffic and resolv.conf at a peer that never answered, with `status` printing "mode: full tunnel". Now polls for a handshake and rolls back if none arrives. - **`status` needed root and did not check.** `wg show` fails unprivileged and was swallowed, so an unprivileged run printed "no tunnel up" and then "mode: full tunnel" in the same breath. An agent reading the first line would re-run `up` — which, before the fix above, destroyed the tunnel it failed to see. - **The killswitch bullet was false.** It said `iptables` is not in the image; it is, so a killswitch is buildable. It stays unbuilt because it would cut Claude Code's own API traffic — an honest reason, unlike the previous one. - `install_feature_skill` rejects path-traversal names, not just blank ones — verified `../skills` would have deleted the whole skills directory including Mission Control's — and reports `mkdir`/`cp` failures instead of printing a success line regardless. - The usage text ended by printing `set -euo pipefail`, off by one line. - HOW-TO-USE claimed "the container says so on start". entrypoint prints to PID 1's stdout, which no terminal or UI surfaces — `docker logs` appears nowhere in the repo. Now points at the migration pre-flight, which does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+15
-5
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user