Skip to content

Commit b1cde69

Browse files
cansofgreaseclaude
cansofgrease
and
claude
committed
The first speedtest picks its server right, and containers stop eating flags
The first speedtest after Start monitoring used to choose its server before the connection had been looked at. Auto-select centres on a city race whose candidates come from the connection info - the exit router's city and the ISP's - but on a fresh install those lookups were still gated when the ten-second countdown ended, so the race was skipped and the Ookla API's guess at your location picked the server instead. Every later run raced real cities and could land somewhere else entirely, which made the first sample - the one that seeds the believed capacity of your line - a measurement against a server nothing afterwards uses. Now the connection lookups start the moment you press Start monitoring, and the first test waits for them - up to twenty extra seconds, only if it has to, and not at all when a server is pinned, a city is searched, a working iperf3 engine is configured, or connection info is off. In practice the lookup lands inside the existing ten-second countdown and the first test starts exactly when it used to, now racing the same cities every later test races. The edges hold, each pinned by a deterministic test. Settings changes can no longer slip between a check and the sleep that was supposed to notice them - both loops subscribe before they read. Consent arriving inside the daemon's three-second settle window takes the full first-run path instead of running instantly. An install that starts on its own after the 48-hour quiet period gets the same treatment (that path flips no settings, so it gets its own poke). A test that finishes anywhere in the first-run window - even in the instructions between the final check and the start of the test - counts as the first test and schedules the next one an interval after itself, never doubling up. And reboots of an already-configured install still test within seconds, never waiting on lookups. Also in this release: setting PINGULARITY_OPTS in a container now tells you it does nothing. The variable is a native Linux convention - the systemd units expand it into command-line flags - and the container images ignore it, which our first filed issue (#16) found the hard way. A container that boots with it set now says so plainly, as the first line of docker logs and again in the About tab - naming the variable, never its value, which can carry secrets. Container flags go after the image name (compose: under command:), and the README, the /etc/default/pingularity file itself, and the security model doc now say so. The Docker test workflow proves the whole chain on both images, warning included. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent dd9b09b commit b1cde69

12 files changed

Lines changed: 1364 additions & 63 deletions

File tree

‎.github/workflows/deep-test.yml‎

Lines changed: 92 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ name: Deep test
44
# covers: the OS service lifecycle (install -> auto-start -> probes -> stop /
55
# restart -> uninstall), data-dir permissions, netinfo/traceroute paths, and the
66
# Docker image + the canonical compose flow. Dispatch after platform-sensitive
7-
# changes; artifacts carry full logs for every leg.
7+
# changes; artifacts carry assertion output and diagnostic log tails per leg.
88
on: workflow_dispatch
99

1010
permissions:
@@ -294,9 +294,14 @@ jobs:
294294
note() { echo "ASSERT FAIL: $*"; echo 1 >> "$FAILS"; }
295295
{
296296
# This block mirrors the canonical hosted compose (served by the dl
297-
# Worker at dl.pingularity.dev/compose.yaml; template lives in the
297+
# Worker at install.pingularity.dev/compose.yaml; template lives in the
298298
# pingularity.dev repo, dl/worker.js). Keep shapes in sync.
299-
mkdir -p compose-test && cd compose-test
299+
mkdir -p compose-test
300+
cd compose-test
301+
# Leave nothing running if any assertion path dies mid-flow (local
302+
# reruns would otherwise find port 9000 taken); explicit downs still
303+
# run between phases.
304+
trap 'docker compose down --remove-orphans >/dev/null 2>&1 || true' EXIT
300305
cat > compose.yaml <<'EOF'
301306
services:
302307
pingularity:
@@ -329,6 +334,56 @@ jobs:
329334
echo "compose bridge-mode metrics -> $bm"
330335
[ "$bm" = "200" ] || note "compose bridge-mode /metrics returned $bm, want 200"
331336
docker compose down
337+
echo "--- reverse-proxy shape: command: flags reach the Host guard (issue #16) ---"
338+
# PINGULARITY_OPTS is the native unit's convention; a container must get
339+
# flags via command:. Assert the chain on the DEFAULT image - compose
340+
# command: syntax, its exec-form entrypoint, the flag parser, and the
341+
# DNS-rebinding guard - by admitting one public Host and rejecting
342+
# another. The iperf variant gets the same pair in its own step below.
343+
sed -i 's/^ ports: \["9000:9000"\]$/ ports: ["9000:9000"]\n command: ["-allow-host=ping.example.com"]/' compose.yaml
344+
docker compose up -d
345+
for i in $(seq 1 15); do curl -fsS --max-time 3 http://127.0.0.1:9000/metrics >/dev/null 2>&1 && break; sleep 2; done
346+
ah=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 -H 'Host: ping.example.com' http://127.0.0.1:9000/ || echo FAIL)
347+
oh=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 -H 'Host: other.example.com' http://127.0.0.1:9000/ || echo FAIL)
348+
echo "allow-host admitted -> $ah, other public Host -> $oh"
349+
[ "$ah" = "200" ] || note "command:-supplied -allow-host did not admit its Host (got $ah, want 200)"
350+
[ "$oh" = "403" ] || note "unlisted public Host was not rejected (got $oh, want 403)"
351+
args=$(docker inspect pingularity --format '{{json .Args}}')
352+
echo "argv: $args"
353+
case "$args" in *"-allow-host=ping.example.com"*) ;; *) note "compose command: flag missing from container argv";; esac
354+
# The variable itself must stay inert and WARNED about in a container.
355+
docker compose down
356+
# The sentinel is a value shape /etc/default/pingularity really carries
357+
# (-metrics-token) and must never surface in any log; the allow-host
358+
# part is what the inertness 403 is asserted against. Do NOT sentinel
359+
# on the domain - the Host guard legitimately logs rejected Hosts.
360+
sed -i 's/^ command: \["-allow-host=ping.example.com"\]$/ environment:\n - PINGULARITY_OPTS=-allow-host=ping.example.com -metrics-token=DEEP_SECRET_SENTINEL/' compose.yaml
361+
docker compose up -d
362+
for i in $(seq 1 15); do curl -fsS --max-time 3 http://127.0.0.1:9000/metrics >/dev/null 2>&1 && break; sleep 2; done
363+
eh=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 -H 'Host: ping.example.com' http://127.0.0.1:9000/ || echo FAIL)
364+
echo "env-var variant Host -> $eh (must stay 403: nothing expands PINGULARITY_OPTS in a container)"
365+
[ "$eh" = "403" ] || note "PINGULARITY_OPTS env var unexpectedly took effect (got $eh, want 403)"
366+
# Capture logs first: a bare `docker logs | grep -q` under pipefail can
367+
# turn a successful early match into SIGPIPE pipeline failure.
368+
clogs=$(docker logs pingularity 2>&1)
369+
grep -Fq 'WARNING: PINGULARITY_OPTS' <<<"$clogs" || note "ignored-PINGULARITY_OPTS stderr warning missing from container logs"
370+
# The stderr grep above cannot see the structured replay (different
371+
# text, different sink) - assert the About-ring copy via /api/logs,
372+
# or the whole replay could be deleted with everything still green.
373+
# Fail CLOSED: a partial body could carry the replay phrase yet
374+
# truncate before leaked data, so a failed fetch is its own failure.
375+
if api_logs=$(curl -fsS --max-time 5 http://127.0.0.1:9000/api/logs); then
376+
grep -Fq 'official container images do not expand it' <<<"$api_logs" \
377+
|| note "structured ignored-PINGULARITY_OPTS warning missing from /api/logs (About ring)"
378+
grep -Fq 'DEEP_SECRET_SENTINEL' <<<"$api_logs" && note "the PINGULARITY_OPTS value leaked into /api/logs"
379+
else
380+
note "fetching /api/logs failed"
381+
fi
382+
grep -Fq 'DEEP_SECRET_SENTINEL' <<<"$clogs" && note "the PINGULARITY_OPTS value leaked into container logs"
383+
# Keep diagnostics in docker.log without ever printing a leaked value.
384+
echo "--- env-leg container logs (sentinel-filtered tail) ---"
385+
{ grep -Fv 'DEEP_SECRET_SENTINEL' <<<"$clogs" || true; } | tail -15
386+
docker compose down
332387
} 2>&1 | tee -a docker.log
333388
n=$(wc -l < "$FAILS" | tr -d ' ')
334389
[ "$n" -eq 0 ] || { echo "::error::$n assertion(s) failed in the compose flow - see docker.log"; exit 1; }
@@ -344,6 +399,7 @@ jobs:
344399
# no distroless `nonroot` for, the /data COPY that gives the volume its
345400
# mode - would first surface in a user's release. The amd64 binary is
346401
# already staged at linux/amd64/pingularity by the build step above.
402+
trap 'docker rm -fv ping-iperf ping-iperf-cmd ping-iperf-env >/dev/null 2>&1 || true' EXIT
347403
docker build -f Dockerfile.iperf -t ping-iperf .
348404
uid=$(docker run --rm --entrypoint id ping-iperf -u)
349405
perm=$(docker run --rm --entrypoint stat ping-iperf -c '%a' /var/lib/pingularity)
@@ -357,10 +413,40 @@ jobs:
357413
im=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 http://127.0.0.1:9112/metrics || echo FAIL)
358414
echo "iperf image metrics -> $im"
359415
[ "$im" = "200" ] || note "iperf image /metrics returned $im, want 200"
360-
docker logs ping-iperf 2>&1 | grep -q "group/world-accessible" \
416+
ilogs=$(docker logs ping-iperf 2>&1)
417+
grep -Fq "group/world-accessible" <<<"$ilogs" \
361418
&& note "the iperf image warned about its own data directory on startup"
362-
echo "--- iperf container logs ---"; docker logs ping-iperf 2>&1 | tail -15
363-
docker rm -f ping-iperf
419+
echo "--- iperf container logs ---"; tail -15 <<<"$ilogs"
420+
docker rm -fv ping-iperf
421+
echo "--- issue #16 parity: same Host-guard pair on the variant the report used ---"
422+
docker run -d --name ping-iperf-cmd -p 9113:9000 ping-iperf -allow-host=ping.example.com
423+
for i in $(seq 1 15); do curl -fsS --max-time 3 http://127.0.0.1:9113/metrics >/dev/null 2>&1 && break; sleep 2; done
424+
iah=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 -H 'Host: ping.example.com' http://127.0.0.1:9113/ || echo FAIL)
425+
ioh=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 -H 'Host: other.example.com' http://127.0.0.1:9113/ || echo FAIL)
426+
iargs=$(docker inspect ping-iperf-cmd --format '{{json .Args}}')
427+
echo "iperf command-arg: allowed -> $iah, other -> $ioh, argv: $iargs"
428+
[ "$iah" = "200" ] || note "iperf image: command-arg -allow-host did not admit its Host (got $iah, want 200)"
429+
[ "$ioh" = "403" ] || note "iperf image: unlisted public Host not rejected (got $ioh, want 403)"
430+
case "$iargs" in *"-allow-host=ping.example.com"*) ;; *) note "iperf image: run-arg flag missing from container argv";; esac
431+
docker rm -fv ping-iperf-cmd
432+
docker run -d --name ping-iperf-env -p 9113:9000 -e "PINGULARITY_OPTS=-allow-host=ping.example.com -metrics-token=DEEP_SECRET_SENTINEL" ping-iperf
433+
for i in $(seq 1 15); do curl -fsS --max-time 3 http://127.0.0.1:9113/metrics >/dev/null 2>&1 && break; sleep 2; done
434+
ieh=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 -H 'Host: ping.example.com' http://127.0.0.1:9113/ || echo FAIL)
435+
echo "iperf env-var: Host -> $ieh (must stay 403)"
436+
[ "$ieh" = "403" ] || note "iperf image: PINGULARITY_OPTS env var unexpectedly took effect (got $ieh, want 403)"
437+
elogs=$(docker logs ping-iperf-env 2>&1)
438+
grep -Fq 'WARNING: PINGULARITY_OPTS' <<<"$elogs" || note "iperf image: ignored-PINGULARITY_OPTS stderr warning missing from container logs"
439+
if iapi_logs=$(curl -fsS --max-time 5 http://127.0.0.1:9113/api/logs); then
440+
grep -Fq 'official container images do not expand it' <<<"$iapi_logs" \
441+
|| note "iperf image: structured warning missing from /api/logs (About ring)"
442+
grep -Fq 'DEEP_SECRET_SENTINEL' <<<"$iapi_logs" && note "iperf image: the PINGULARITY_OPTS value leaked into /api/logs"
443+
else
444+
note "iperf image: fetching /api/logs failed"
445+
fi
446+
grep -Fq 'DEEP_SECRET_SENTINEL' <<<"$elogs" && note "iperf image: the PINGULARITY_OPTS value leaked into container logs"
447+
echo "--- iperf env-leg container logs (sentinel-filtered tail) ---"
448+
{ grep -Fv 'DEEP_SECRET_SENTINEL' <<<"$elogs" || true; } | tail -15
449+
docker rm -fv ping-iperf-env
364450
} 2>&1 | tee -a docker.log
365451
n=$(wc -l < "$FAILS" | tr -d ' ')
366452
[ "$n" -eq 0 ] || { echo "::error::$n assertion(s) failed in the iperf3 image - see docker.log"; exit 1; }

‎README.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,11 @@ The **`-v pingularity-data:/var/lib/pingularity`** volume is what makes updates
195195
safe: the SQLite database *and* `pingularity.key` (which encrypts saved iperf3
196196
passwords) live there. Skip the volume and a `docker pull` + recreate throws
197197
away your history and key. Pass flags as arguments after the image name, e.g.
198-
`ghcr.io/pingular/pingularity -speedtest-interval 30m`.
198+
`ghcr.io/pingular/pingularity -speedtest-interval 30m` - in compose, that means
199+
under `command:`, e.g. `command: ["-allow-host=your.domain"]`. Pingularity does
200+
not interpret `PINGULARITY_OPTS` as flags: that variable is expanded by the
201+
native Linux systemd units from `/etc/default/pingularity`, and the container
202+
images ignore it.
199203
200204
Two image variants ship to the same repo. The default
201205
`ghcr.io/pingular/pingularity` is a lean distroless image and deliberately ships

‎docs/security-model.md‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,8 @@ The rows in detail:
7171
read-only for a root daemon writing under `/var`), `ProtectHome=yes`,
7272
`PrivateTmp=yes`, `ProtectKernelTunables=yes`, `RestrictSUIDSGID=yes` and
7373
`LockPersonality=yes` (svcopts_other.go). It matches the packaged unit in every other respect
74-
(`$PINGULARITY_OPTS` from `/etc/default/pingularity`, a 5s restart, a bounded
74+
(`$PINGULARITY_OPTS` from `/etc/default/pingularity` - a native-systemd
75+
mechanism; the container images ignore that variable - a 5s restart, a bounded
7576
restart loop, `ExecReload` for the SIGHUP settings reload) but not the
7677
de-rooting — aligning it with the packaged unit is planned but not yet
7778
shipped, so today treat a self-install as root, though not unprotected. If you want
@@ -175,7 +176,9 @@ TLS is terminated upstream. Instead:
175176

176177
So the practical rule is: **if you front it with a TLS proxy, set
177178
`-allow-host` to the public domain and have the proxy preserve the `Host`
178-
header.** That one flag is what turns on the `Secure` cookie, and it is also
179+
header.** (In a container that flag goes after the image name - compose:
180+
under `command:`; `PINGULARITY_OPTS` is expanded only by the native systemd
181+
units and the images ignore it.) That one flag is what turns on the `Secure` cookie, and it is also
179182
what tells the rebinding guard below to admit your public domain. Add
180183
`-trusted-proxy` with the proxy's address so the login rate limiter keys on the
181184
real client rather than on the proxy.

‎firstrun_wiring_test.go‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
package main
2+
3+
import (
4+
"context"
5+
"io"
6+
"log/slog"
7+
"testing"
8+
"time"
9+
10+
"github.com/pingular/pingularity/internal/netinfo"
11+
"github.com/pingular/pingularity/internal/settings"
12+
"github.com/pingular/pingularity/internal/speedtest"
13+
"github.com/pingular/pingularity/internal/store"
14+
)
15+
16+
// The production assembly IS the feature: package tests cover injected hooks,
17+
// but deleting `ni.WakeFn = set.Changed` (or miswiring it) would leave every
18+
// package test green while the first speedtest quietly went back to racing an
19+
// empty netinfo. This drives the REAL settings controller's close-and-replace
20+
// broadcast into the REAL netinfo loop, wired exactly as main.go wires them.
21+
func TestFirstRunWiringSettingsBroadcastWakesNetinfo(t *testing.T) {
22+
ctx, cancel := context.WithCancel(context.Background())
23+
defer cancel()
24+
st, err := store.Open(":memory:")
25+
if err != nil {
26+
t.Fatalf("open store: %v", err)
27+
}
28+
defer st.Close()
29+
set, err := settings.New(ctx, st, settings.Values{Monitoring: false, NetinfoEnabled: true})
30+
if err != nil {
31+
t.Fatalf("settings: %v", err)
32+
}
33+
34+
ni := netinfo.NewManager(slog.New(slog.NewTextHandler(io.Discard, nil)))
35+
ni.EnabledFn = func() bool { return set.Monitoring() && set.NetinfoEnabled() }
36+
ni.WakeFn = set.Changed // the production wiring under test (main.go)
37+
38+
loopCtx, loopCancel := context.WithCancel(ctx)
39+
done := make(chan struct{})
40+
defer func() { loopCancel(); <-done }()
41+
go func() { defer close(done); ni.Loop(loopCtx, time.Hour) }()
42+
43+
time.Sleep(150 * time.Millisecond)
44+
if ni.Get().UpdatedAt != 0 {
45+
t.Fatal("lookup attempted while monitoring was off")
46+
}
47+
48+
// The power button. A real mutate -> a real broadcast -> the loop must
49+
// attempt a refresh well inside its one-minute disabled poll.
50+
if err := set.SetMonitoring(ctx, true); err != nil {
51+
t.Fatalf("SetMonitoring: %v", err)
52+
}
53+
deadline := time.Now().Add(15 * time.Second)
54+
for ni.Get().UpdatedAt == 0 {
55+
if time.Now().After(deadline) {
56+
t.Fatal("power-on broadcast never reached the netinfo loop; the ni.WakeFn = set.Changed wiring is dead")
57+
}
58+
time.Sleep(20 * time.Millisecond)
59+
}
60+
}
61+
62+
// newFirstRunReadyFn's truth table, against the real controller and manager -
63+
// this is the composition main.go hands to the scheduler, so a reordered or
64+
// dropped short-circuit here is a 20-second stall (or a defeated wait) on
65+
// every fresh install's first test.
66+
func TestFirstRunReadyFnComposition(t *testing.T) {
67+
ctx := context.Background()
68+
mk := func(v settings.Values) (*settings.Controller, *netinfo.Manager) {
69+
st, err := store.Open(":memory:")
70+
if err != nil {
71+
t.Fatalf("open store: %v", err)
72+
}
73+
t.Cleanup(func() { st.Close() })
74+
set, err := settings.New(ctx, st, v)
75+
if err != nil {
76+
t.Fatalf("settings: %v", err)
77+
}
78+
return set, netinfo.NewManager(slog.New(slog.NewTextHandler(io.Discard, nil)))
79+
}
80+
81+
// Fresh install, netinfo on, nothing published: not ready (the wait is the
82+
// feature - without it the first run races an empty netinfo).
83+
set, ni := mk(settings.Values{NetinfoEnabled: true})
84+
if newFirstRunReadyFn(set, ni)() {
85+
t.Error("fresh install with netinfo pending must NOT be ready")
86+
}
87+
88+
// Pinned server: the race is irrelevant.
89+
set, ni = mk(settings.Values{NetinfoEnabled: true, SpeedServerID: "12345"})
90+
if !newFirstRunReadyFn(set, ni)() {
91+
t.Error("pinned server must be ready immediately")
92+
}
93+
94+
// Searched city: overrides the race.
95+
set, ni = mk(settings.Values{NetinfoEnabled: true, SpeedAutoLoc: "43.65,-79.38"})
96+
if !newFirstRunReadyFn(set, ni)() {
97+
t.Error("searched city must be ready immediately")
98+
}
99+
100+
// Connection info off: nothing is ever coming.
101+
set, ni = mk(settings.Values{NetinfoEnabled: false})
102+
if !newFirstRunReadyFn(set, ni)() {
103+
t.Error("netinfo disabled must be ready immediately")
104+
}
105+
106+
// iperf3 engine: ready only when the binary is actually usable - an
107+
// unavailable iperf3 falls back to Ookla, which DOES need the race.
108+
set, ni = mk(settings.Values{NetinfoEnabled: true, SpeedEngine: "iperf3"})
109+
if got, want := newFirstRunReadyFn(set, ni)(), speedtest.IperfAvailable(); got != want {
110+
t.Errorf("iperf3 engine readiness = %v, want %v (IperfAvailable on this host)", got, want)
111+
}
112+
}

0 commit comments

Comments
 (0)