From 146dd9a9066ee90ec01057e390ca671a1780ec40 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Tue, 6 Oct 2026 02:28:37 -0500 Subject: [PATCH] fix(vpn-ingress): new-peer.sh counted its own commented-out example as an allocated address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first real enrolment failed: $ ./infra/vpn-ingress/new-peer.sh keith-phone 2 new-peer.sh: 10.90.0.2/32 is already allocated ... Pick another octet. Nothing was allocated -- peers.yaml had zero peers, as the pod's init container reported (`0 peer(s)`). The duplicate-address check grepped the whole file, and peers.yaml line 66 is a COMMENTED-OUT example stanza I wrote containing literally `# AllowedIPs = 10.90.0.2/32`. So .2 -- the first address anyone would ask for, and the one my own runbook uses as its example -- was permanently un-allocatable, with a message asserting the opposite of the truth. Fixed on both sides, because either alone leaves the trap half-set: * the check strips comment lines before matching, which is the correct predicate rather than a patch: a real stanza's AllowedIPs line is never commented, while the template and every peer's `# — ` label always are. * the example now uses placeholders that cannot match anything real (`10.90.0./32`, ``), so a future grep cannot be fooled by it either. The bug class is worth naming: a documentation example that is byte-identical to real data will eventually be parsed as real data. This is its third instance in this component -- flux-postbuild-substitution flagged a `${...}` written inside a comment explaining that hazard, and no-plaintext-secrets flagged the .sops.yaml.example because a kustomization named it in a comment. Make examples structurally unmatchable, not merely commented out. Verified by running it: .2 now renders a profile (via the real `sops -d | wg pubkey` path, which this also proves against the operator's actual key for the first time), and an uncommented duplicate stanza is still correctly refused. No sensitive paths; merges on a green gate. Co-Authored-By: Claude Opus 5 (1M context) --- ...0-06-new-peer-ignores-template-comments.md | 73 +++++++++++++++++++ infra/vpn-ingress/new-peer.sh | 8 +- platform/vpn-ingress/peers.yaml | 10 ++- 3 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 decisions/2026-10-06-new-peer-ignores-template-comments.md diff --git a/decisions/2026-10-06-new-peer-ignores-template-comments.md b/decisions/2026-10-06-new-peer-ignores-template-comments.md new file mode 100644 index 0000000..2b6089f --- /dev/null +++ b/decisions/2026-10-06-new-peer-ignores-template-comments.md @@ -0,0 +1,73 @@ +--- +date: 2026-10-06 +branch: fix/new-peer-ignores-template-comments +title: new-peer.sh refused the first VPN address because its duplicate check matched its own commented-out example +--- + +## What changed + +The operator's first real enrolment failed: + +``` +$ ./infra/vpn-ingress/new-peer.sh keith-phone 2 +new-peer.sh: 10.90.0.2/32 is already allocated in platform/vpn-ingress/peers.yaml. Pick another octet. +``` + +**Nothing was allocated.** `peers.yaml` had zero peers — the pod's own init container reported +`wg0.conf assembled: 0 peer(s)`. The duplicate-address check grepped the WHOLE file, and +`peers.yaml` line 66 is a **commented-out example stanza** I wrote, containing literally +`# AllowedIPs = 10.90.0.2/32`. So `.2` — the first address anybody would ask for, and the one my +own runbook uses in its example — was permanently un-allocatable, with a message asserting the +opposite of the truth. + +Both halves fixed, because either alone leaves the trap half-set: + +- **`infra/vpn-ingress/new-peer.sh`**: the check strips comment lines before matching + (`grep -vE '^[[:space:]]*#' | grep -q …`). That is the correct predicate rather than a patch: a + real stanza's `AllowedIPs` line is never commented, while both the template and every peer's + `# — ` label always are. +- **`platform/vpn-ingress/peers.yaml`**: the example now uses placeholders that cannot match + anything real — `# — , enrolled ` and + `AllowedIPs = 10.90.0./32`. A future grep cannot be fooled by it either. + +The bug class is worth naming: **a documentation example that is byte-identical to real data will +eventually be parsed as real data.** The same shape caused two earlier defects in this component — +`flux-postbuild-substitution` flagged a `${...}` written inside a comment *explaining* that hazard, +and `no-plaintext-secrets` flagged the `.sops.yaml.example` because the kustomization named it in a +comment. Three instances now; the rule is to make examples structurally unmatchable, not merely +commented out. + +No sensitive paths (`platform/vpn-ingress/**`, `infra/vpn-ingress/**`, `decisions/**`), so this +merges on a green gate with no override. + +## What I verified + +Run in the pinned toolbox against the live tree, 2026-10-06. + +- **`.2` now works**, and it went through the REAL key path: `new-peer.sh keith-phone 2` rendered a + profile with `Address 10.90.0.2/32`, `AllowedIPs = 10.90.0.2/32` in the printed stanza, full + tunnel, at `.cache/wg-peers/keith-phone.conf` (0600). The throwaway keypair was deleted after. +- **This incidentally closes a gap left open in #60**: the profile could only have been rendered by + deriving the server's public key via `sops -d --extract … | wg pubkey` from + `wg-server-key.sops.yaml` — so that pipeline is now proven **against the operator's real key**, + not just against a placeholder it was expected to reject. +- **A real duplicate is still refused.** Appending an uncommented + `AllowedIPs = 10.90.0.5/32` stanza and asking for octet 5 produced the correct refusal; the tree + was restored afterwards and `git diff --stat` showed only the intended template edit. +- **Zero active allocations remain**: `grep -vE '^[[:space:]]*#' peers.yaml | grep -c AllowedIPs` + → `0`. +- **Toolbox gate**: rc 3, with `spec-immutability`, `decisions-log`, `lint`, `l0`, `render-golden`, + `l1` and `approval-policy` all pass; rc 3 is the two environmental skips (`gitea-settings`, + conformance L2/L3). **Not green. Reported as rc 3.** + +## What I did NOT verify + +- **No peer is enrolled by this branch.** The keypair I generated was a throwaway and was deleted; + the operator must run the script themselves, since the device's private key must exist only on + the device. So the end of the enrolment path — a stanza merged into `peers.yaml`, Flux rolling + the DaemonSet, and `wg show` listing a peer — is still unexercised. +- **The duplicate-NAME warning** (`grep -qi "^ *# *${NAME} "`) deliberately still inspects + comments, because per-peer labels are comments. It is no longer tripped by the template, but I + did not test it against a real enrolled peer. +- **Nothing about client traffic.** The WAN port-forward is still not configured and there is still + no off-LAN vantage point, so the tunnel has carried no packet. diff --git a/infra/vpn-ingress/new-peer.sh b/infra/vpn-ingress/new-peer.sh index 4a1209f..00e3cdb 100755 --- a/infra/vpn-ingress/new-peer.sh +++ b/infra/vpn-ingress/new-peer.sh @@ -133,7 +133,13 @@ CLIENT_ADDR="${CLIENT_BASE}.${OCTET}" # --- refuse a collision, because the failure is otherwise invisible ----------------------------- # Two devices sharing an address does not error anywhere: the server routes that address to # whichever peer handshook most recently, so BOTH devices appear to work intermittently. -if [ -r "$PEERS_FILE" ] && grep -q "AllowedIPs *= *${CLIENT_ADDR}/32" "$PEERS_FILE"; then +# COMMENT LINES ARE NOT ALLOCATIONS. This used to grep the whole file and so matched the +# COMMENTED-OUT EXAMPLE STANZA in peers.yaml, which made `.2` — the first address anyone would +# ask for — permanently un-allocatable with a message claiming it was already taken. Strip +# comments first: a real stanza's `AllowedIPs` line is never commented, while both the template +# and every peer's `# — ` label are. +if [ -r "$PEERS_FILE" ] \ + && grep -vE '^[[:space:]]*#' "$PEERS_FILE" | grep -q "AllowedIPs *= *${CLIENT_ADDR}/32"; then die "${CLIENT_ADDR}/32 is already allocated in platform/vpn-ingress/peers.yaml. Pick another octet." fi if [ -r "$PEERS_FILE" ] && grep -qi "^ *# *${NAME} " "$PEERS_FILE"; then diff --git a/platform/vpn-ingress/peers.yaml b/platform/vpn-ingress/peers.yaml index a6d7137..8424037 100644 --- a/platform/vpn-ingress/peers.yaml +++ b/platform/vpn-ingress/peers.yaml @@ -60,10 +60,16 @@ data: # # COPY THIS TEMPLATE, uncomment, one block per device: # - # # pixel-8 — Keith's phone, enrolled 2026-10-05 + # # — , enrolled # [Peer] # PublicKey = - # AllowedIPs = 10.90.0.2/32 + # AllowedIPs = 10.90.0./32 + # + # The placeholders above are deliberately NOT valid values. An earlier version of this template + # used a real-looking `10.90.0.2/32`, and new-peer.sh's duplicate-address check — which grepped + # the whole file — matched it, so `.2` could never be allocated and the script insisted it was + # already taken. The check now ignores comments; keeping the example unmatchable too means a + # future grep cannot be fooled either. # # Keep the `# — , enrolled ` comment line. It is the only place a key is # tied to a human-recognisable device; `wg show` can only ever print the key itself, so without -- 2.54.0