Skip to content

Commit e257128

Browse files
cansofgreaseclaude
andcommitted
Fix proxied speedtests, container lockouts, and first-run consent gaps
Speedtests behind a proxy work again: - The new connection guard mistook an operator's own HTTP/SOCKS proxy for something hostile and refused it, killing every speedtest on machines that reach the internet through one. The proxy you configured is now trusted, and the guard checks the real destination instead - so the protection still holds (a malicious server still can't steer traffic at your machine or private network), and it now also covers the automatic city race and the packet-loss probe, which had escaped it entirely. - The guard also refuses a few reserved address ranges it previously let through. A locked-out container can let itself back in: - Upgrading an older bridged-container install could leave the dashboard refusing its own published port with no way back in. Passing -access network (or -e PINGULARITY_ACCESS=network) now takes effect at startup even when a saved setting disagrees - an explicit flag is the recovery path, and it works in either direction. First-run consent holds: - A fresh install that hit a database hiccup on its very first start could begin monitoring without anyone answering the first-run dialog. That window is closed from both ends: the offer is seeded before settings go live, and the hold refuses to release while it is unseeded. - Once a login is set, dismissing the first-run dialog requires it, so a stranger on the network can't wave it away for you. Settings save what you saved: - Saving a value that happened to match a command-line flag used to write nothing, so the setting silently changed when the flag was later removed. Save now persists what you saved; untouched fields keep following the flag or the shipped default, as before. Leaner, steadier speedtest runs: - Server health probes no longer hold sockets open after each check. - Auto-select contacts its backup servers only when it actually needs them, roughly halving its traffic when the nearby pool is healthy. - Two probes checking the same server at once no longer lose count of its failures, so a dead server is retired as quickly as intended. - Retried transfer attempts are now fully isolated from each other. - The server picker remembers a server proven broken even when a later listing has nothing new to say about it. Release pipeline: promoting a release candidate to stable on the same commit no longer trips the safety check that verifies the published tag. Every fix carries a test that fails without it, and the security model docs now describe how speedtest destinations are vetted, proxies included. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 2eac907 commit e257128

26 files changed

Lines changed: 2497 additions & 184 deletions

.github/workflows/release.yml

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -139,18 +139,17 @@ jobs:
139139

140140
# GoReleaser does NOT resolve the current tag with `git describe`: it reads
141141
# GORELEASER_CURRENT_TAG first (pinned on the Run GoReleaser step below to
142-
# the guarded ref), else the HIGHEST version tag pointing at HEAD. On a
143-
# commit carrying more than one tag those two disagree, so the env pin is
144-
# what actually guarantees the publish uses the exact tag the guards
145-
# approved. This assert stays as a cheap belt-and-suspenders check that HEAD
146-
# is on the guarded tag; `git describe` only approximates GoReleaser's
147-
# resolution, so treat the env pin above as the real guarantee, not this.
148-
- name: Assert HEAD tag matches the ref
142+
# the guarded ref), else the HIGHEST version tag pointing at HEAD. The env
143+
# pin is what guarantees the publish uses the exact tag the guards
144+
# approved; this assert is the belt-and-suspenders check that the
145+
# checked-out HEAD actually carries that tag. Membership, not equality: a
146+
# commit may legitimately carry several tags (the rc-to-stable promote
147+
# cuts the stable tag on the same SHA as the final rc), so require
148+
# GITHUB_REF_NAME to be AMONG the tags pointing at HEAD.
149+
- name: Assert the pushed tag points at HEAD
149150
run: |
150-
head_tag="$(git describe --exact-match --tags HEAD)" || {
151-
echo "::error::HEAD is not on an exact tag; refusing to release"; exit 1; }
152-
if [ "$head_tag" != "${GITHUB_REF_NAME}" ]; then
153-
echo "::error::HEAD tag '$head_tag' != ref '${GITHUB_REF_NAME}'; the release would publish a different version than was guarded"
151+
if ! git tag --points-at HEAD | grep -Fxq -- "${GITHUB_REF_NAME}"; then
152+
echo "::error::tag '${GITHUB_REF_NAME}' does not point at HEAD (tags at HEAD: $(git tag --points-at HEAD | paste -sd ' ' - || true)); the release would publish a different commit than was guarded"
154153
exit 1
155154
fi
156155

README.md

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,14 @@ The image is multi-arch (amd64 + arm64). Two flags matter:
198198
> `localhost:9000`, but other devices on your LAN get `403` until you opt in with
199199
> `-access network` (or `-e PINGULARITY_ACCESS=network`) - set a login at the same
200200
> time. A bridged container that publishes a port with `-p` needs the same flag,
201-
> or the published port returns `403`.
201+
> or the published port returns `403`. An explicitly passed `-access` /
202+
> `PINGULARITY_ACCESS` is authoritative at every start: it updates a disagreeing
203+
> saved setting (in either direction) and logs the change, so
204+
> `-e PINGULARITY_ACCESS=network` also recovers an install whose saved
205+
> local-only would otherwise lock its published port out. The flip side: while
206+
> the flag or env stays pinned in your unit/compose file, changing **Network
207+
> access** in the UI is overridden again at the next restart - drop the flag to
208+
> let the UI choice stick.
202209
203210
The **`-v pingularity-data:/var/lib/pingularity`** volume is what makes updates
204211
safe: the SQLite database *and* `pingularity.key` (which encrypts saved iperf3
@@ -738,7 +745,10 @@ pingularity version print version
738745
```
739746
740747
Flags only **seed** the initial values - almost everything is adjustable live in
741-
the settings drawer afterward and persists across restarts.
748+
the settings drawer afterward and persists across restarts. A value you **save**
749+
in the UI is persisted even when it equals what a flag currently supplies, and
750+
wins from then on - removing the flag later keeps what you saved. Fields you
751+
never save keep following the flag (or the shipped default).
742752
743753
| Flag | Default | Purpose |
744754
| --- | --- | --- |
@@ -1148,7 +1158,8 @@ constant memory.
11481158
- `POST /api/quick-setup` - apply the first-run **Quick Setup** answer in ONE
11491159
transaction (speedtest cadence, network access, update check, and an optional
11501160
login) and mark it answered so the dialog never returns; `{dismiss:true}` marks
1151-
it answered without changing anything else. `auth_enabled` must agree with
1161+
it answered without changing anything else (once a login is active, dismissing
1162+
requires that login too). `auth_enabled` must agree with
11521163
whether a `password` is sent, and it refuses (`403`) once a login is already
11531164
configured - change access under Settings then. Fresh installs only; the offer
11541165
is `quick_setup_pending` in `/api/status`

RELEASING.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,10 @@ of failing with `422 already_exists`. That overwrite is confined to prerelease
6363
tags: a **stable** release is immutable, so the workflow refuses to re-cut a
6464
stable tag whose release already exists (its artifacts are provenance-attested,
6565
and overwriting them would break that). To re-issue a stable build, bump the
66-
version.
66+
version. Cutting the stable tag on the same commit as its final rc (the
67+
promote) is fully supported: the workflow asserts the pushed tag is *among*
68+
the tags pointing at HEAD - a second tag on the commit doesn't confuse it -
69+
and pins GoReleaser to exactly that tag via `GORELEASER_CURRENT_TAG`.
6770

6871
None of this touches `latest.json` or `internal/update` - the running fleet is
6972
not notified yet (see [Update notifications](#update-notifications)).

defaults_test.go

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
package main
22

33
import (
4+
"context"
45
"log/slog"
56
"testing"
67
"time"
78

89
"github.com/pingular/pingularity/internal/config"
10+
"github.com/pingular/pingularity/internal/settings"
11+
"github.com/pingular/pingularity/internal/store"
912
)
1013

1114
// TestDefaultSettings executes the fresh-install configuration literal - which no
@@ -106,6 +109,79 @@ func TestDefaultSettings(t *testing.T) {
106109
}
107110
}
108111

112+
// reconcileAccess: an EXPLICIT -access / PINGULARITY_ACCESS must override a
113+
// disagreeing STORED access_local_only at boot - in either direction - and
114+
// persist the result; without explicit input the stored value always wins.
115+
// This is the recovery path for a bridged container that persisted
116+
// local_only=true under the old warn-only regime and is 403'd off its own
117+
// published port on upgrade, with no shell in the distroless image to repair
118+
// the database from.
119+
func TestReconcileAccess(t *testing.T) {
120+
ctx := context.Background()
121+
newSet := func(t *testing.T, storedLocalOnly bool) *settings.Controller {
122+
t.Helper()
123+
st, err := store.Open(":memory:")
124+
if err != nil {
125+
t.Fatalf("open store: %v", err)
126+
}
127+
t.Cleanup(func() { st.Close() })
128+
set, err := settings.New(ctx, st, settings.Values{
129+
Latency: 5 * time.Second, Speed: time.Hour, Timeout: 2 * time.Second,
130+
DownAfter: 3, UpAfter: 2,
131+
})
132+
if err != nil {
133+
t.Fatalf("new settings: %v", err)
134+
}
135+
// Simulate the upgraded install: the value sits in the STORE, where the
136+
// overlay makes it win over the flag-seeded default.
137+
if err := set.SetAccessLocalOnly(ctx, storedLocalOnly); err != nil {
138+
t.Fatalf("seed stored access_local_only: %v", err)
139+
}
140+
return set
141+
}
142+
143+
cases := []struct {
144+
name string
145+
stored bool
146+
cfg config.Config
147+
wantChanged bool
148+
wantLocal bool
149+
}{
150+
{"explicit network overrides stored local-only (the upgrade lockout)",
151+
true, config.Config{Access: "network", AccessExplicit: true}, true, false},
152+
{"explicit local overrides stored network",
153+
false, config.Config{Access: "local", AccessExplicit: true}, true, true},
154+
{"explicit and agreeing: nothing to do",
155+
false, config.Config{Access: "network", AccessExplicit: true}, false, false},
156+
{"silent default never overrides stored local-only",
157+
true, config.Config{Access: "local", AccessExplicit: false}, false, true},
158+
{"silent default leaves stored network alone too",
159+
false, config.Config{Access: "local", AccessExplicit: false}, false, false},
160+
}
161+
for _, c := range cases {
162+
t.Run(c.name, func(t *testing.T) {
163+
set := newSet(t, c.stored)
164+
changed, err := reconcileAccess(ctx, c.cfg, set)
165+
if err != nil {
166+
t.Fatalf("reconcileAccess: %v", err)
167+
}
168+
if changed != c.wantChanged {
169+
t.Errorf("changed = %v, want %v", changed, c.wantChanged)
170+
}
171+
if got := set.AccessLocalOnly(); got != c.wantLocal {
172+
t.Errorf("AccessLocalOnly = %v, want %v", got, c.wantLocal)
173+
}
174+
// The override must PERSIST (survive a reload), not just flip memory.
175+
if err := set.Reload(ctx); err != nil {
176+
t.Fatalf("reload: %v", err)
177+
}
178+
if got := set.AccessLocalOnly(); got != c.wantLocal {
179+
t.Errorf("after reload: AccessLocalOnly = %v, want %v (override not persisted)", got, c.wantLocal)
180+
}
181+
})
182+
}
183+
}
184+
109185
// TestLogLevelOffKeepsErrors pins B2: the default log level "off" must still
110186
// surface WARN and ERROR. It silences routine INFO/DEBUG, but a failed sample
111187
// write or a wedged webhook must not vanish just because logging is at its

docs/security-model.md

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,12 @@ Two things worth knowing about the filter:
140140
container sees real peer addresses, so local-only there behaves exactly as it
141141
does natively. Pingularity still detects a bridged container, but only to warn
142142
that container DNS may distort the reported upstream resolver; that detection
143-
has no say over access.
143+
has no say over access. An explicitly passed `-access` or `PINGULARITY_ACCESS`
144+
is authoritative at every start: if it disagrees with the stored setting, the
145+
daemon updates the stored value to match (in either direction) and logs the
146+
change - which also makes `-e PINGULARITY_ACCESS=network` the recovery path
147+
for an install whose saved local-only would otherwise 403 its own published
148+
port.
144149
- **A same-host reverse proxy passes it.** Traffic arriving through a proxy on
145150
the same machine looks local, because it is. The filter cannot block a remote
146151
visitor arriving that way, and the daemon warns once when it detects the
@@ -195,6 +200,33 @@ such as `plex:9000`, and the `.local`, `.lan`, `.home`, `.internal` and
195200
`.home.arpa` suffixes. Serving it on a real domain is exactly the case that
196201
needs `-allow-host`.
197202

203+
## Speedtest servers are untrusted input
204+
205+
Speedtest destinations come from third-party catalogue data, so the daemon
206+
treats them as hostile until proven otherwise. Every measurement connection -
207+
ranking pings, health probes, uploads, downloads, the packet-loss probe - is
208+
checked at dial time and refused if it points somewhere internal: loopback,
209+
RFC1918, link-local (including cloud metadata), CGNAT (RFC 6598), the
210+
documentation and benchmark ranges (RFC 5737 TEST-NET-1/2/3, RFC 2544), IETF
211+
protocol assignments (192.0.0.0/24), and class E space (240.0.0.0/4). A server
212+
that redirects a probe is re-checked against the same rules before the new
213+
address is adopted.
214+
215+
Configuring an HTTP(S)/SOCKS proxy through the standard
216+
`HTTP_PROXY`/`HTTPS_PROXY`/`ALL_PROXY` variables changes the shape of the
217+
problem: the dial the guard sees is to your proxy, and the real destination is
218+
only named inside the request. So the proxy's exact address is trusted as your
219+
own configuration and exempted from the dial guard, and the daemon separately
220+
validates every speedtest destination before use - internal IP addresses are
221+
refused outright, hostnames are resolved and checked first, and hostnames that
222+
do not resolve are refused rather than trusted. Every discovery path passes
223+
the same filter, including the automatic city race, which vets both a
224+
catalogue entry's host and its URL. One caveat remains: the daemon's DNS
225+
lookup and your proxy's are two separate resolutions, so a hostile hostname
226+
that answers differently to each (DNS rebinding) could still direct proxied
227+
traffic at an address the daemon would have refused; internal IP-literal
228+
entries - the realistic hostile shape - have no such window.
229+
198230
## What needs a login and what does not
199231

200232
When a password is set:
@@ -300,5 +332,6 @@ Shortest version:
300332
- **On your LAN**: set a password first, then enable Network access.
301333
- **Reachable from the internet**: set a password, and put it behind a reverse
302334
proxy that terminates TLS. Do not expose `:9000` directly.
303-
- **In a container**: the access filter cannot help you. Publish the port
304-
carefully and set a password.
335+
- **In a container**: local-only is enforced there too - a published port
336+
returns 403 until you opt in with `-access network`. Do that deliberately,
337+
and set a password at the same time.

firstrun_hold_test.go

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
package main
2+
3+
import (
4+
"context"
5+
"io"
6+
"log/slog"
7+
"os"
8+
"strings"
9+
"testing"
10+
"time"
11+
12+
"github.com/pingular/pingularity/internal/settings"
13+
"github.com/pingular/pingularity/internal/store"
14+
)
15+
16+
// A LOADED controller with Quick Setup unanswered and NO offer clock is the
17+
// late-load window: a retry/SIGHUP Reload flips Loaded()==true and broadcasts
18+
// the wake BEFORE materializeQuickSetup seeds the offer clock. QuickSetupHold
19+
// reads the bare zero as "never offered -> release", and monitoringLive used
20+
// to LATCH that release permanently - a fresh install started probing without
21+
// first-run consent. The predicate must fail CLOSED there: hold a fresh
22+
// install, latch nothing, and let the real 48h hold take over once the clock
23+
// is seeded.
24+
func TestMonitoringLiveHoldsFreshInstallWithUnseededOfferClock(t *testing.T) {
25+
ctx := context.Background()
26+
st, err := store.Open(":memory:")
27+
if err != nil {
28+
t.Fatalf("open store: %v", err)
29+
}
30+
defer st.Close()
31+
set, err := settings.New(ctx, st, settings.Values{Monitoring: true})
32+
if err != nil {
33+
t.Fatalf("settings: %v", err)
34+
}
35+
// The window's exact state: loaded, offer open, clock unseeded.
36+
if !set.Loaded() {
37+
t.Fatal("precondition: controller must be loaded")
38+
}
39+
if set.QuickSetupDone() {
40+
t.Fatal("precondition: quick setup must be unanswered")
41+
}
42+
if since, err := set.QuickSetupOfferSinceErr(ctx); err != nil || since != 0 {
43+
t.Fatalf("precondition: offer clock must be unseeded, got %d, %v", since, err)
44+
}
45+
46+
live := newMonitoringLiveFn(ctx, set, nil)
47+
if live() {
48+
t.Fatal("fresh install with a loaded controller and an unseeded offer clock must HOLD, not probe (consent bypass)")
49+
}
50+
// Nothing may have latched in that window: once the clock IS seeded (the
51+
// materialize step catching up), the normal 48h hold must be in force, not
52+
// a permanently released latch.
53+
if err := set.EnsureQuickSetupOffer(ctx, time.Now().Unix()); err != nil {
54+
t.Fatalf("EnsureQuickSetupOffer: %v", err)
55+
}
56+
if live() {
57+
t.Fatal("hold released during the unseeded window and LATCHED - the seeded offer clock no longer holds")
58+
}
59+
// Answering releases for real.
60+
if err := set.SetQuickSetupDone(ctx, true); err != nil {
61+
t.Fatalf("SetQuickSetupDone: %v", err)
62+
}
63+
if !live() {
64+
t.Fatal("monitoring must run once quick setup is answered")
65+
}
66+
}
67+
68+
// The fail-closed branch must not over-hold: the same window state on an
69+
// install whose store already carries operator configuration (it consented
70+
// long ago; only the answered marker hasn't been materialized yet) keeps
71+
// monitoring running.
72+
func TestMonitoringLiveEstablishedInstallRunsWithUnseededOfferClock(t *testing.T) {
73+
ctx := context.Background()
74+
st, err := store.Open(":memory:")
75+
if err != nil {
76+
t.Fatalf("open store: %v", err)
77+
}
78+
defer st.Close()
79+
// Prior operator configuration = established (see hasPriorConfiguration).
80+
if err := st.SetSetting(ctx, "monitoring", "true"); err != nil {
81+
t.Fatalf("seed config: %v", err)
82+
}
83+
set, err := settings.New(ctx, st, settings.Values{Monitoring: true})
84+
if err != nil {
85+
t.Fatalf("settings: %v", err)
86+
}
87+
if set.QuickSetupDone() {
88+
t.Fatal("precondition: quick setup must be unanswered")
89+
}
90+
if !newMonitoringLiveFn(ctx, set, nil)() {
91+
t.Fatal("established install must keep monitoring through the unseeded-clock window")
92+
}
93+
}
94+
95+
// The recovery paths (settings-retry loop, SIGHUP) must seed the offer clock
96+
// BEFORE calling Reload: Reload itself flips Loaded()==true and broadcasts
97+
// the wake, so a post-Reload seed leaves the latch window open. This drives
98+
// the pre-seed against a REAL unloaded controller on a REAL working store (a
99+
// canceled context makes New's initial read fail exactly like a transient
100+
// store fault, and a live context recovers it).
101+
func TestQuickSetupOfferPreSeedRunsWhileUnloaded(t *testing.T) {
102+
st, err := store.Open(":memory:")
103+
if err != nil {
104+
t.Fatalf("open store: %v", err)
105+
}
106+
defer st.Close()
107+
dead, cancel := context.WithCancel(context.Background())
108+
cancel()
109+
set, err := settings.New(dead, st, settings.Values{Monitoring: true})
110+
if err == nil {
111+
t.Fatal("settings.New must fail on the canceled context (simulated failed first load)")
112+
}
113+
if set.Loaded() {
114+
t.Fatal("precondition: controller must be unloaded")
115+
}
116+
117+
ctx := context.Background()
118+
p := &program{log: slog.New(slog.NewTextHandler(io.Discard, nil))}
119+
p.seedQuickSetupOfferEarly(ctx, set)
120+
if set.Loaded() {
121+
t.Fatal("pre-seed must not flip Loaded - only Reload may")
122+
}
123+
if since, err := set.QuickSetupOfferSinceErr(ctx); err != nil {
124+
t.Fatalf("offer clock read: %v", err)
125+
} else if since == 0 {
126+
t.Fatal("pre-seed left the offer clock unseeded on a fresh install - the latch window before materializeQuickSetup stays open")
127+
}
128+
// The recovery Reload now finds the clock already there: the hold is in
129+
// force from the first woken round.
130+
if err := set.Reload(ctx); err != nil {
131+
t.Fatalf("Reload: %v", err)
132+
}
133+
if newMonitoringLiveFn(ctx, set, nil)() {
134+
t.Fatal("hold must be in force after a recovered first load")
135+
}
136+
}
137+
138+
// The pre-seed's placement lives in run()'s SIGHUP goroutine and in
139+
// retrySettingsLoad's loop, neither unit-testable end-to-end - the same gap
140+
// TestMainWiresFirstRunAndOptsHooks covers, with the same deliberately literal
141+
// remedy: in both recovery paths the pre-seed must appear, and appear BEFORE
142+
// the set.Reload call it protects.
143+
func TestRecoveryPathsPreSeedBeforeReload(t *testing.T) {
144+
src, err := os.ReadFile("main.go")
145+
if err != nil {
146+
t.Fatal(err)
147+
}
148+
s := string(src)
149+
for _, tc := range []struct{ name, anchor string }{
150+
{"settings-retry loop", "func (p *program) retrySettingsLoad"},
151+
{"SIGHUP handler", "case <-hup:"},
152+
} {
153+
i := strings.Index(s, tc.anchor)
154+
if i < 0 {
155+
t.Fatalf("%s: anchor %q not found in main.go", tc.name, tc.anchor)
156+
}
157+
rest := s[i:]
158+
j := strings.Index(rest, "set.Reload(")
159+
if j < 0 {
160+
t.Fatalf("%s: no set.Reload call after %q", tc.name, tc.anchor)
161+
}
162+
if !strings.Contains(rest[:j], "seedQuickSetupOfferEarly") {
163+
t.Errorf("%s: seedQuickSetupOfferEarly must run BEFORE set.Reload - Reload flips Loaded and broadcasts, and a post-Reload seed reopens the consent-hold latch window", tc.name)
164+
}
165+
}
166+
}

0 commit comments

Comments
 (0)