From 919184041fe5ff5e2292726d96c59934a8789e00 Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Sun, 26 Jul 2026 06:26:56 +0100 Subject: [PATCH 1/6] fix(model): count only providers holding a Public provide key A provider without a provide_mode=3 key cannot accept a contract from a client outside its own network, so counting it advertises supply nobody can reach. On beta this reported 39 US providers when 2 were reachable. Filters both UpdateClientLocations and UpdateClientScores: GetProviderLocations gates on loadLocationStables as well, so fixing only the first leaves the symptom in place. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- model/network_client_location_model.go | 31 +++++++- model/network_client_location_model_test.go | 84 +++++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/model/network_client_location_model.go b/model/network_client_location_model.go index 3693df74..f340d643 100644 --- a/model/network_client_location_model.go +++ b/model/network_client_location_model.go @@ -1519,8 +1519,20 @@ func UpdateClientLocations(ctx context.Context, ttl time.Duration) (returnErr er WHERE network_client_location_reliability.connected = true AND - network_client_location_reliability.valid = true + network_client_location_reliability.valid = true AND + -- a provider that does not hold a Public provide key cannot + -- accept a contract from a client outside its own network, so + -- counting it advertises supply nobody can reach. GetProvideRelationship + -- returns ProvideModePublic for cross-network pairs, and the + -- destination must hold a key for exactly that mode. + EXISTS ( + SELECT 1 FROM provide_key + WHERE + provide_key.client_id = network_client_location_reliability.client_id AND + provide_key.provide_mode = $1 + ) `, + ProvideModePublic, ) server.WithPgResult(result, err, func() { for result.Next() { @@ -2466,8 +2478,23 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r client_connection_reliability_score.client_id = network_client_location_reliability.client_id WHERE network_client_location_reliability.connected = true AND - network_client_location_reliability.valid = true + network_client_location_reliability.valid = true AND + -- a provider that does not hold a Public provide key cannot + -- accept a contract from a client outside its own network, so + -- counting it advertises supply nobody can reach. GetProvideRelationship + -- returns ProvideModePublic for cross-network pairs, and the + -- destination must hold a key for exactly that mode. + -- GetProviderLocations gates on loadLocationStables, populated + -- from here, so filtering UpdateClientLocations alone would + -- leave the symptom in place. + EXISTS ( + SELECT 1 FROM provide_key + WHERE + provide_key.client_id = network_client_location_reliability.client_id AND + provide_key.provide_mode = $1 + ) `, + ProvideModePublic, ) server.WithPgResult(result, err, func() { for result.Next() { diff --git a/model/network_client_location_model_test.go b/model/network_client_location_model_test.go index 3dcc5b26..a9058eb8 100644 --- a/model/network_client_location_model_test.go +++ b/model/network_client_location_model_test.go @@ -1233,6 +1233,14 @@ func TestUpdateClientLocationsCountsClientsWithoutReliabilityScores(t *testing.T err = SetConnectionLocation(ctx, connectionId, city.LocationId, &ConnectionLocationScores{}) connect.AssertEqual(t, err, nil) + // only clients holding a Public provide key are counted (see + // TestUpdateClientLocationsCountsOnlyPublicProviders); the + // reliability-score join is what is under test here, so satisfy that + // precondition explicitly + SetProvide(ctx, clientId, map[ProvideMode][]byte{ + ProvideModePublic: []byte("public-secret"), + }) + // populates network_client_location_reliability straight from the // live connection tables -- independent of, and deliberately without // ever touching, the reliability-scoring pipeline below @@ -1326,6 +1334,14 @@ func TestUpdateClientScoresCountsClientsWithoutReliabilityScores(t *testing.T) { err = SetConnectionLocation(ctx, connectionId, city.LocationId, &ConnectionLocationScores{}) connect.AssertEqual(t, err, nil) + // only clients holding a Public provide key are counted (see + // TestUpdateClientLocationsCountsOnlyPublicProviders); the + // reliability-score join is what is under test here, so satisfy that + // precondition explicitly + SetProvide(ctx, clientId, map[ProvideMode][]byte{ + ProvideModePublic: []byte("public-secret"), + }) + // good latency and speed tests so the quality score gate passes and // the reliability-score join is what's actually being tested here server.Tx(ctx, func(tx server.PgTx) { @@ -1425,3 +1441,71 @@ func TestSetConnectionLocationToleratesCountryOnlyLocation(t *testing.T) { connect.AssertEqual(t, *cty, country.CountryLocationId) }) } + +func TestUpdateClientLocationsCountsOnlyPublicProviders(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + city := &Location{ + LocationType: LocationTypeCity, + City: "Palo Alto", + Region: "California", + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, city) + + handlerId := CreateNetworkClientHandler(ctx) + + // connect a client and give it the provide modes supplied + connectOne := func(modes map[ProvideMode][]byte) server.Id { + networkId := server.NewId() + clientId := server.NewId() + Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + connectionId, _, _, _, err := ConnectNetworkClient(ctx, clientId, "0.0.0.1:0", handlerId) + connect.AssertEqual(t, err, nil) + err = SetConnectionLocation(ctx, connectionId, city.LocationId, &ConnectionLocationScores{}) + connect.AssertEqual(t, err, nil) + if modes != nil { + SetProvide(ctx, clientId, modes) + } + return clientId + } + + // serves strangers -- must be counted + connectOne(map[ProvideMode][]byte{ + ProvideModePublic: []byte("public-secret"), + ProvideModeNetwork: []byte("network-secret"), + }) + // own network only -- must NOT be counted, it cannot accept a + // contract from a user outside its network + connectOne(map[ProvideMode][]byte{ + ProvideModeNetwork: []byte("network-secret"), + }) + // no provide key at all -- must NOT be counted + connectOne(nil) + + UpdateClientLocationReliabilities(ctx, server.NowUtc().Add(-time.Hour), server.NowUtc()) + + err := UpdateClientLocations(ctx, time.Hour) + connect.AssertEqual(t, err, nil) + + initialClientLocations, err := loadInitialClientLocations(ctx) + connect.AssertEqual(t, err, nil) + if initialClientLocations == nil { + t.Fatal("expected a populated client locations cache, got nil") + } + + found := false + for _, clientLocation := range initialClientLocations.Locations { + if clientLocation.LocationId == city.CountryLocationId { + found = true + // exactly one of the three connected clients holds a Public + // key; counting the other two advertises supply no user can + // reach (39 advertised vs 2 reachable, observed on beta) + connect.AssertEqual(t, clientLocation.ClientCount, 1) + } + } + connect.AssertEqual(t, found, true) + }) +} From 41b6e90facef0372fa0552da21aec04600a022b7 Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Sun, 26 Jul 2026 07:07:09 +0100 Subject: [PATCH 2/6] fix(model): filter the location-group provider query too, and test the score filter Review follow-ups to "count only providers holding a Public provide key". 1. The location *group* source query in UpdateClientScores was still bare `connected = true AND valid = true`. It fills locationGroupClientScores -> the clientScoreLocationGroup* redis keys -> loadClientScores -> FindProviders2 whenever a spec carries a LocationGroupId, so a user selecting a promoted group (e.g. "Strong Privacy Laws") still received providers that cannot accept their contract, and CreateContract rejected them with NoPermission. Same EXISTS predicate as the other two queries. 2. The UpdateClientScores filter had no test of its own -- every existing test that reaches it gives all its clients a Public key, so deleting the clause broke nothing. Two new tests set up connected+valid providers that are identical except for the Public provide key: - TestUpdateClientScoresCountsOnlyPublicProviders covers the per-location query through loadLocationStables (the gate GetProviderLocations uses) and loadClientScores. - TestUpdateClientScoresGroupCountsOnlyPublicProviders covers the group query through the group cache. Both were verified to fail with their respective EXISTS clause removed. 3. The SQL comments claimed the destination "must hold a key for exactly that mode". That is not an invariant the code holds: resolveNonCompanionProvideMode lets a Stream-only destination settle a cross-network contract as a companion stream. Reworded to state the intent -- a companion-only destination is a return path, not general provider supply, so excluding it from provider counts is deliberate. No SQL change. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- model/network_client_location_model.go | 42 +++- model/network_client_location_model_test.go | 240 ++++++++++++++++++++ 2 files changed, 271 insertions(+), 11 deletions(-) diff --git a/model/network_client_location_model.go b/model/network_client_location_model.go index f340d643..d753a666 100644 --- a/model/network_client_location_model.go +++ b/model/network_client_location_model.go @@ -1520,11 +1520,18 @@ func UpdateClientLocations(ctx context.Context, ttl time.Duration) (returnErr er WHERE network_client_location_reliability.connected = true AND network_client_location_reliability.valid = true AND - -- a provider that does not hold a Public provide key cannot - -- accept a contract from a client outside its own network, so - -- counting it advertises supply nobody can reach. GetProvideRelationship - -- returns ProvideModePublic for cross-network pairs, and the - -- destination must hold a key for exactly that mode. + -- count only providers that can serve a stranger. + -- GetProvideRelationship returns ProvideModePublic for a + -- cross-network pair, so a Public provide key is what makes a + -- provider generally reachable; without one it advertises + -- supply nobody outside its own network can use. + -- This is deliberately narrower than what CreateContract will + -- ultimately accept: resolveNonCompanionProvideMode + -- (controller/connect_controller.go) also lets a Stream-only + -- destination settle a cross-network contract as a *companion* + -- stream, for backward compatibility with old clients. A + -- companion-only destination is a return path, not general + -- provider supply, so excluding it here is intended. EXISTS ( SELECT 1 FROM provide_key WHERE @@ -2479,11 +2486,10 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r WHERE network_client_location_reliability.connected = true AND network_client_location_reliability.valid = true AND - -- a provider that does not hold a Public provide key cannot - -- accept a contract from a client outside its own network, so - -- counting it advertises supply nobody can reach. GetProvideRelationship - -- returns ProvideModePublic for cross-network pairs, and the - -- destination must hold a key for exactly that mode. + -- count only providers that can serve a stranger; see the + -- matching comment in UpdateClientLocations above for why a + -- Public provide key is the rule and why the Stream companion + -- fallback is deliberately not honoured here. -- GetProviderLocations gates on loadLocationStables, populated -- from here, so filtering UpdateClientLocations alone would -- leave the symptom in place. @@ -2562,8 +2568,22 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r WHERE network_client_location_reliability.connected = true AND - network_client_location_reliability.valid = true + network_client_location_reliability.valid = true AND + -- same rule as the per-location query above. This one fills + -- locationGroupClientScores -> the clientScoreLocationGroup* + -- redis keys -> loadClientScores -> FindProviders2 whenever a + -- spec carries a LocationGroupId, so leaving it unfiltered + -- means a user who picks a promoted group (e.g. "Strong + -- Privacy Laws") still gets providers that cannot accept + -- their contract and CreateContract rejects with NoPermission. + EXISTS ( + SELECT 1 FROM provide_key + WHERE + provide_key.client_id = network_client_location_reliability.client_id AND + provide_key.provide_mode = $1 + ) `, + ProvideModePublic, ) server.WithPgResult(result, err, func() { for result.Next() { diff --git a/model/network_client_location_model_test.go b/model/network_client_location_model_test.go index a9058eb8..450554d3 100644 --- a/model/network_client_location_model_test.go +++ b/model/network_client_location_model_test.go @@ -1509,3 +1509,243 @@ func TestUpdateClientLocationsCountsOnlyPublicProviders(t *testing.T) { connect.AssertEqual(t, found, true) }) } + +// `UpdateClientScores` carries the same "can this provider serve a stranger" +// filter as `UpdateClientLocations`, and it matters on a different path: +// the per-location source query fills the per-location client scores, which +// become the `clientScoreLocation*` redis keys, which `loadLocationStables` +// reads and `GetProviderLocations` gates on. A location whose only providers +// are network-only must therefore not be stable at all. +// +// Two connected+valid providers with identical latency/speed data in two +// different countries, differing ONLY in whether they hold a Public provide +// key. Removing the `EXISTS (... provide_mode = ...)` clause from the +// per-location source query makes the network-only provider's country stable +// and puts its client score in the cache, and this test goes red. +func TestUpdateClientScoresCountsOnlyPublicProviders(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + publicCity := &Location{ + LocationType: LocationTypeCity, + City: "Palo Alto", + Region: "California", + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, publicCity) + + networkOnlyCity := &Location{ + LocationType: LocationTypeCity, + City: "Toronto", + Region: "Ontario", + Country: "Canada", + CountryCode: "ca", + } + CreateLocation(ctx, networkOnlyCity) + + publicClientId, networkOnlyClientId := connectPublicAndNetworkOnlyProviders( + ctx, + t, + publicCity, + networkOnlyCity, + ) + + UpdateClientLocationReliabilities(ctx, server.NowUtc().Add(-time.Hour), server.NowUtc()) + + err := UpdateClientScores(ctx, time.Hour, 1) + connect.AssertEqual(t, err, nil) + + locationStables, err := loadLocationStables( + ctx, + []server.Id{publicCity.CountryLocationId, networkOnlyCity.CountryLocationId}, + RankModeQuality, + server.Id{}, + ) + connect.AssertEqual(t, err, nil) + + // the Public provider's country has a provider a stranger can use + _, ok := locationStables[publicCity.CountryLocationId] + connect.AssertEqual(t, ok, true) + // the network-only provider's country has none. a missing entry is how + // loadLocationStables says "no providers", so it must be absent + _, ok = locationStables[networkOnlyCity.CountryLocationId] + connect.AssertEqual(t, ok, false) + + // and the same at the client-score level FindProviders2 consumes + clientScores, err := loadClientScores( + true, + RankModeQuality, + ctx, + map[server.Id]bool{ + publicCity.LocationId: true, + networkOnlyCity.LocationId: true, + }, + map[server.Id]bool{}, + server.Id{}, + 100, + ) + connect.AssertEqual(t, err, nil) + connect.AssertEqual(t, len(clientScores), 1) + _, ok = clientScores[publicClientId] + connect.AssertEqual(t, ok, true) + _, ok = clientScores[networkOnlyClientId] + connect.AssertEqual(t, ok, false) + }) +} + +// the location *group* source query in `UpdateClientScores` needs the same +// filter as the per-location one. It fills `locationGroupClientScores` -> +// the `clientScoreLocationGroup*` redis keys -> `loadClientScores` -> +// `FindProviders2` whenever the spec carries a `LocationGroupId`. Unfiltered, +// a user who selects a promoted group (e.g. "Strong Privacy Laws") is handed +// providers that cannot accept their contract, and `CreateContract` rejects +// them with `NoPermission`. +// +// Same two providers, differing only in the Public provide key, each in its +// own location group. Removing the `EXISTS` clause from the group query puts +// the network-only provider in its group's cache and this test goes red. +func TestUpdateClientScoresGroupCountsOnlyPublicProviders(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + publicCity := &Location{ + LocationType: LocationTypeCity, + City: "Palo Alto", + Region: "California", + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, publicCity) + + networkOnlyCity := &Location{ + LocationType: LocationTypeCity, + City: "Toronto", + Region: "Ontario", + Country: "Canada", + CountryCode: "ca", + } + CreateLocation(ctx, networkOnlyCity) + + publicGroup := &LocationGroup{ + Name: "Test Group Public", + Promoted: true, + MemberLocationIds: []server.Id{ + publicCity.CityLocationId, + publicCity.RegionLocationId, + publicCity.CountryLocationId, + }, + } + CreateLocationGroup(ctx, publicGroup) + + networkOnlyGroup := &LocationGroup{ + Name: "Test Group Network Only", + Promoted: true, + MemberLocationIds: []server.Id{ + networkOnlyCity.CityLocationId, + networkOnlyCity.RegionLocationId, + networkOnlyCity.CountryLocationId, + }, + } + CreateLocationGroup(ctx, networkOnlyGroup) + + publicClientId, networkOnlyClientId := connectPublicAndNetworkOnlyProviders( + ctx, + t, + publicCity, + networkOnlyCity, + ) + + UpdateClientLocationReliabilities(ctx, server.NowUtc().Add(-time.Hour), server.NowUtc()) + + err := UpdateClientScores(ctx, time.Hour, 1) + connect.AssertEqual(t, err, nil) + + // read the group cache only -- no location ids -- so this asserts on + // the group query alone + clientScores, err := loadClientScores( + true, + RankModeQuality, + ctx, + map[server.Id]bool{}, + map[server.Id]bool{ + publicGroup.LocationGroupId: true, + networkOnlyGroup.LocationGroupId: true, + }, + server.Id{}, + 100, + ) + connect.AssertEqual(t, err, nil) + connect.AssertEqual(t, len(clientScores), 1) + _, ok := clientScores[publicClientId] + connect.AssertEqual(t, ok, true) + _, ok = clientScores[networkOnlyClientId] + connect.AssertEqual(t, ok, false) + + // the network-only group on its own exports nothing + networkOnlyGroupClientScores, err := loadClientScores( + true, + RankModeQuality, + ctx, + map[server.Id]bool{}, + map[server.Id]bool{networkOnlyGroup.LocationGroupId: true}, + server.Id{}, + 100, + ) + connect.AssertEqual(t, err, nil) + connect.AssertEqual(t, len(networkOnlyGroupClientScores), 0) + }) +} + +// connects one provider per location that is identical in every respect the +// scoring pipeline looks at -- connected, valid, same latency and speed +// samples so both clear the strict minimums -- except that the first holds a +// Public provide key and the second holds only a Network one. Anything that +// separates the two downstream is the provide-mode filter and nothing else. +func connectPublicAndNetworkOnlyProviders( + ctx context.Context, + t testing.TB, + publicLocation *Location, + networkOnlyLocation *Location, +) (publicClientId server.Id, networkOnlyClientId server.Id) { + handlerId := CreateNetworkClientHandler(ctx) + + connectProvider := func(location *Location, modes map[ProvideMode][]byte) server.Id { + networkId := server.NewId() + clientId := server.NewId() + Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + connectionId, _, _, _, err := ConnectNetworkClient(ctx, clientId, "0.0.0.1:0", handlerId) + connect.AssertEqual(t, err, nil) + err = SetConnectionLocation(ctx, connectionId, location.LocationId, &ConnectionLocationScores{}) + connect.AssertEqual(t, err, nil) + SetProvide(ctx, clientId, modes) + + // good latency and speed samples so the client passes the strict + // minimums; loadLocationStables reads the force-minimum-false filter + // key, which is empty for a client that fails them + server.Tx(ctx, func(tx server.PgTx) { + server.RaisePgResult(tx.Exec( + ctx, + `INSERT INTO network_client_latency (connection_id, latency_ms, sample_count) VALUES ($1, $2, $3)`, + connectionId, 30, 1, + )) + server.RaisePgResult(tx.Exec( + ctx, + `INSERT INTO network_client_speed (connection_id, bytes_per_second, sample_count) VALUES ($1, $2, $3)`, + connectionId, 100*1024*1024, 1, + )) + }) + return clientId + } + + // serves strangers + publicClientId = connectProvider(publicLocation, map[ProvideMode][]byte{ + ProvideModePublic: []byte("public-secret"), + ProvideModeNetwork: []byte("network-secret"), + }) + // own network only -- cannot accept a contract from a user outside it + networkOnlyClientId = connectProvider(networkOnlyLocation, map[ProvideMode][]byte{ + ProvideModeNetwork: []byte("network-secret"), + }) + return +} From 307bbb0d8d2a5787530073bd58b02fd2d9fa3e54 Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Sun, 26 Jul 2026 08:11:26 +0100 Subject: [PATCH 3/6] docs: make prober confinement portable across docker and non-docker Beta runs Docker Compose; the mainstream deployment does not use Docker at all. A jail built on Docker networking, or on the process creating its own namespace (needing CAP_NET_ADMIN on a component that is unprivileged today), fits one and breaks the other. The prober now requires no privileges and assumes nothing about how it is confined. Each deployment supplies confinement natively -- a restricted network under compose, systemd IPAddressDeny/IPAddressAllow otherwise -- and the prober verifies it at startup by attempting a direct connection to a geolocation address and refusing to run if it succeeds. That turns correct operator configuration from an assumption the design rests on into a runtime precondition, identically in both environments. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- ...25-enforced-provider-geo-probing-design.md | 53 ++++++++++++++----- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md b/docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md index bd61f8e1..28bc1be4 100644 --- a/docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md +++ b/docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md @@ -31,7 +31,7 @@ code-level discipline alone (see *The jail*). | Question | Decision | | --- | --- | | Consequence of no fresh probe | **Advisory.** Provider stays listed, falls back to its mmdb location, and is flagged unverified. | -| Where the prober runs | **Taskworker, in a network-namespace jail** (a separate child process). | +| Where the prober runs | **A separate unprivileged process, confined by the deployment** (Docker network on beta, systemd `IPAddressAllow` upstream), self-verifying its confinement at startup. | | Tamper resistance | **Rotating identity + corroboration + multi-hop probing.** | | Which providers | **Public providers only** (`provide_mode = 3`). | @@ -99,16 +99,39 @@ Extended, not replaced. ## The jail -The prober process runs in a network namespace whose only route is the platform -websocket endpoint. Not firewall rules inside a shared namespace — a distinct -netns, so no route to a geolocation API exists at all. +The prober runs confined so that no route to a geolocation API exists at all, +rather than relying on application code to decline to use one. -The Go-level fail-closed behaviour stays as defence in depth, but correctness of -the hard constraint stops depending on it. A future refactor that introduces a -default `http.Client` then fails loudly instead of silently leaking. +**The prober itself requires no privileges and assumes nothing about how it is +confined.** This matters because the deployments differ fundamentally: beta runs +under Docker Compose, and the mainstream deployment does not use Docker at all. +A design that depended on Docker networking, or on the process creating its own +namespace (which would need `CAP_NET_ADMIN` on a component that today runs +completely unprivileged), would fit one environment and break the other. -The namespace is asserted by an integration test (see *Testing*), so the -constraint is verified as infrastructure rather than trusted as code. +Confinement is therefore supplied by the deployment, in whatever way is native +to it: + +| Deployment | Mechanism | +| --- | --- | +| beta (Docker Compose) | a service attached only to a network that reaches the platform | +| mainstream (no Docker) | systemd `IPAddressDeny=any` + `IPAddressAllow=`, kernel-enforced via eBPF | + +Neither requires the prober to hold a capability, and neither is code we have to +keep correct. + +**The prober verifies its own confinement at startup.** Before doing any work it +attempts a *direct* connection — not through a tunnel — to a known geolocation +API address. If that connection succeeds, the confinement is not in place, and +the prober exits non-zero without probing anything. + +This is the part that makes the arrangement trustworthy across environments. The +operator's configuration is no longer an assumption the design rests on; it is a +runtime precondition the prober refuses to run without, and it holds identically +under Docker, systemd, or a future deployment that resembles neither. + +The Go-level fail-closed behaviour stays as defence in depth, so a refactor that +introduced a default `http.Client` would fail loudly rather than leak silently. ## Identity rotation @@ -294,9 +317,11 @@ project's probe population would also give an honest count. - **`probeverdict`**: table-driven over every rule, including RTT-impossible cases with real distances, and an explicit case asserting mmdb divergence does **not** produce `Suspect`. -- **The jail**: an integration test asserting the namespace has *no route* to a - geolocation IP — the constraint verified as infrastructure, not trusted as - code. Complemented by a tcpdump check in staging. +- **Confinement**: a test asserting the startup self-check fails closed — that + the prober exits non-zero when a direct connection to a geolocation address + succeeds, and proceeds when it is refused. This is the constraint verified as + a runtime precondition rather than trusted as configuration. Complemented by a + tcpdump check in staging. - **Multi-hop**: assert the target observes the intermediary's `source_id`, not the prober's. - **Pool guard**: assert that below `min_intermediary_pool` the probe runs direct @@ -480,8 +505,8 @@ measured bandwidth survive a provider restart. The second is a user-visible bug today. The third gates everything else — without it P1's results and the lifecycle's probation both evaporate on restart. -**P1 — Automated probing in a jail.** The `egressprober` binary, the network -namespace, and the taskworker job. Direct probing only, single fixed identity, +**P1 — Automated probing, confined.** The `egressprober` worker, its startup +self-check, the deployment confinement for both environments, and the taskworker job. Direct probing only, single fixed identity, no verdict logic — results land exactly as they do today. Delivers the automation and, critically, makes the hard constraint structural. From 94eada01d100cb26371432fca55af516afcf5a1e Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Sun, 26 Jul 2026 08:11:51 +0100 Subject: [PATCH 4/6] docs: correct the stale namespace rationale in the architecture section It still justified the separate process by namespace confinement after the jail section moved to deployment-supplied restriction. The process boundary is still right, for two reasons that survive: it is the smallest unit either Docker or systemd can restrict, and it keeps geolocation code out of the component holding database credentials. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- .../2026-07-25-enforced-provider-geo-probing-design.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md b/docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md index 28bc1be4..8a912a96 100644 --- a/docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md +++ b/docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md @@ -64,9 +64,11 @@ Four units with clean boundaries: ### `egressprober` (new binary, operator-proxy repo) Wraps the existing `providertunnel`, `geolocate` and `ingest` libraries in a -long-lived worker that reads probe jobs and reports results. It is a separate -process specifically so it can be network-jailed; Go cannot confine a subset of -goroutines to a namespace. +worker that reads probe jobs and reports results. It is a separate process so +that the deployment can confine it as a whole — a process is the smallest unit +either Docker or systemd can apply an egress restriction to, and Go cannot +confine a subset of goroutines. Keeping it out of the taskworker also means the +component holding geolocation code is not the one holding database credentials. ### `provider_egress_probe_work.go` (new taskworker job, server repo) From e6d5a2002cdb1d16e92d74744e0972f4a0c358e7 Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Sun, 26 Jul 2026 08:14:55 +0100 Subject: [PATCH 5/6] docs: implementation plan for P1 automated confined probing Four tasks: honour forceMinimum on the location read path (the Important finding from the P0 final review), a server-side due-provider endpoint so selection is durable across restarts, a prober startup self-check, and deployment confinement for both environments. The confinement design is portable because it has to be -- beta runs Docker Compose and mainstream runs no Docker at all. The prober takes no capabilities and inspects no mechanism; it tests the property directly by attempting a direct connection to a geolocation address and refusing to start if one succeeds. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- ...-provider-probing-p1-automated-confined.md | 515 ++++++++++++++++++ 1 file changed, 515 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-26-provider-probing-p1-automated-confined.md diff --git a/docs/superpowers/plans/2026-07-26-provider-probing-p1-automated-confined.md b/docs/superpowers/plans/2026-07-26-provider-probing-p1-automated-confined.md new file mode 100644 index 00000000..87839117 --- /dev/null +++ b/docs/superpowers/plans/2026-07-26-provider-probing-p1-automated-confined.md @@ -0,0 +1,515 @@ +# Provider Probing P1 — Automated, Confined Probing + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Turn the operator's one-shot geolocation prober into an automated worker that runs confined, picks its work from durable server-side state, and refuses to start unless its confinement is actually in place. + +**Architecture:** The prober stays an unprivileged process that assumes nothing about how it is confined. Each deployment supplies confinement natively — a restricted Docker network on beta, systemd `IPAddressDeny`/`IPAddressAllow` upstream — and the prober verifies it at startup by attempting a direct connection to a geolocation address and exiting if it succeeds. Due-provider selection moves from the prober's in-memory TTL cache to a server endpoint backed by `provider_egress_location` freshness, so a restart no longer re-probes everything. + +**Tech Stack:** Go 1.26.5, PostgreSQL 16, Redis, Docker Compose (beta only), systemd (mainstream). + +**Source spec:** `docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md` +**Predecessor:** `docs/superpowers/plans/2026-07-26-provider-probing-p0-prerequisites.md` (complete) + +## Global Constraints + +- **The hard constraint:** the operator's server must never send a request directly to a geolocation API. Every lookup egresses through a provider. Nothing in this plan may add a code path that could contact `ip.pn`, `free.freeipapi.com` or `ipinfo.io` other than through a provider tunnel. +- `geolocate/` in the operator-proxy repo must remain **standard-library-only**. No `golang.org/x/...`. +- The prober must require **no Linux capabilities** and must not create namespaces or firewall rules itself. Beta runs Docker Compose; the mainstream deployment does not use Docker at all. Anything that only works under one is wrong. +- **Two branches per server-side change:** a feature branch off `beta/self-contained-env` PR'd to `Ryanmello07/server`, plus a cherry-picked branch PR'd to `urnetwork/server` `main`. +- Remotes differ per checkout — run `git remote -v` before pushing. `/root/urnetwork/server`: `origin` = `Ryanmello07/server`, `upstream` = `urnetwork/server`. `/tmp/sandbox/server` has these **inverted**. Never push a feature branch to an upstream repo. +- Do not alter `PassesMinimums`, scoring weights, or thresholds. +- P1 is **direct probing only** — no identity rotation, no multi-hop, no verdict logic. Those are P3 and P2. Results land exactly as they do today. +- Server tests need the local stack: `local/run-local.sh`, then `./test.sh -run TestName`. A whole-package `go test ./model` is not achievable in this environment (pre-existing missing `subsidy.yml`); say so explicitly rather than claiming it passed. +- A **live beta deployment** runs on the dev machine via `docker-compose.beta.yml`. Never stop, recreate or `down` it. + +--- + +## File Structure + +| File | Responsibility | Change | +| --- | --- | --- | +| `model/network_client_location_model.go` (server) | `loadLocationStables` hardcodes `forceMinimum=false` | Modify — thread the flag through | +| `model/provider_egress_location_model.go` (server) | Egress location storage | Modify — add due-provider query | +| `api/handlers/provider_egress_location_handlers.go` (server) | Operator-authenticated ingest | Modify — add the due-list handler | +| `api/api.go` (server) | Route table | Modify — register the route | +| `confinement/confinement.go` (operator-proxy) | **New.** Startup self-check | Create | +| `cmd/egress-prober/main.go` (operator-proxy) | Prober CLI | Modify — self-check + server-driven due list | +| `docker-compose.beta.yml` + `BETA.md` (server) | Beta deployment | Modify — confined prober service | +| `docs/operator/prober-systemd.md` (server) | **New.** Non-Docker deployment | Create | + +--- + +## Task 1: Location enumeration honours `force_minimum` + +**Repo:** `/root/urnetwork/server`. Two branches: `feat/probe-location-force-minimum` and `-upstream`. + +**Context:** P0 made the prober send `force_minimum: true` to `find-providers2`, which bypasses `PassesMinimums` for *provider* selection. But the prober gets its **locations** from `GET /network/provider-locations`, which only emits a location if `loadLocationStables` has an entry — and that reads `clientScoreLocationFilterKey(false, rankMode, ...)` at `model/network_client_location_model.go:1877` with `forceMinimum` **hardcoded false**. + +Consequence: a location where every provider fails minimums is invisible to the prober, so those providers are never probed and can never graduate probation. `force_minimum` on the second call cannot recover what the first call never listed. This was the Important finding from the P0 final review. + +**Files:** +- Modify: `model/network_client_location_model.go:1858-1890` (`loadLocationStables`) and its callers +- Test: `model/network_client_location_model_test.go` + +**Interfaces:** +- Produces: `loadLocationStables(ctx, rankMode, forceMinimum bool, locationIds, ...)` — exact parameter order to be matched to the existing signature; add `forceMinimum` adjacent to `rankMode`. + +- [ ] **Step 1: Create the branch** + +```bash +cd /root/urnetwork/server +git checkout beta/self-contained-env && git pull --ff-only origin beta/self-contained-env +git checkout -b feat/probe-location-force-minimum +``` + +- [ ] **Step 2: Read before writing** + +Read `loadLocationStables` in full (`model/network_client_location_model.go:1858` onward) and find every caller with `grep -n 'loadLocationStables' model/*.go controller/*.go`. `GetProviderLocations` is the caller that matters. Note whether the writer side (`UpdateClientScores`, around `:2848`) already writes **both** the `forceMinimum=false` and `forceMinimum=true` key families — it does, since `exportClientScores` is invoked in a `for _, forceMinimum := range []bool{false, true}` loop. This task only fixes the read side. + +- [ ] **Step 3: Write the failing test** + +Add to `model/network_client_location_model_test.go`. One connected+valid Public provider whose score falls below the minimums, asserting it is invisible with `forceMinimum=false` and visible with `true`: + +```go +func TestLoadLocationStablesHonoursForceMinimum(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + city := &Location{ + LocationType: LocationTypeCity, + City: "Palo Alto", + Region: "California", + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, city) + + networkId := server.NewId() + clientId := server.NewId() + Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + handlerId := CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := ConnectNetworkClient(ctx, clientId, "0.0.0.1:0", handlerId) + connect.AssertEqual(t, err, nil) + err = SetConnectionLocation(ctx, connectionId, city.LocationId, &ConnectionLocationScores{}) + connect.AssertEqual(t, err, nil) + SetProvide(ctx, clientId, map[ProvideMode][]byte{ + ProvideModePublic: []byte("public-secret"), + ProvideModeNetwork: []byte("network-secret"), + }) + + UpdateClientLocationReliabilities(ctx, server.NowUtc().Add(-time.Hour), server.NowUtc()) + err = UpdateClientScores(ctx) + connect.AssertEqual(t, err, nil) + + locationIds := map[server.Id]bool{city.CountryLocationId: true} + + // this provider has no latency or speed test, so scoring penalises it + // past the minimums gate -- exactly the population the prober needs to + // reach and currently cannot see + strict, err := loadLocationStables(ctx, RankModeQuality, false, locationIds, server.Id{}) + connect.AssertEqual(t, err, nil) + connect.AssertEqual(t, len(strict), 0) + + forced, err := loadLocationStables(ctx, RankModeQuality, true, locationIds, server.Id{}) + connect.AssertEqual(t, err, nil) + connect.AssertEqual(t, len(forced), 1) + }) +} +``` + +Adapt the argument list to `loadLocationStables`'s real signature — read it first; do not guess the parameter order. If the function returns a map keyed differently, assert on presence of `city.CountryLocationId` rather than length. + +- [ ] **Step 4: Run it and confirm it fails** + +```bash +cd /root/urnetwork/server && ./local/run-local.sh --keep-up +./test.sh -run TestLoadLocationStablesHonoursForceMinimum +``` + +Expected: a compile failure (`too many arguments`) because `loadLocationStables` does not yet take `forceMinimum`. That is the correct first failure. + +- [ ] **Step 5: Thread the flag through** + +Add a `forceMinimum bool` parameter to `loadLocationStables` and pass it to `clientScoreLocationFilterKey` in place of the literal `false` at `:1877`. Update every caller. `GetProviderLocations` must pass `false` — user-facing location listing keeps today's behaviour and must not change. + +Add this comment above the parameter: + +```go + // forceMinimum selects which pre-computed key family to read. The writer + // (UpdateClientScores) populates both, so this only chooses between them. + // User-facing listing passes false and keeps today's behaviour; an operator + // census passes true, because a location where every provider fails the + // minimums gate is otherwise invisible and its providers can never be + // probed or graduate probation. +``` + +- [ ] **Step 6: Run the test and the regressions** + +```bash +./test.sh -run TestLoadLocationStablesHonoursForceMinimum +./test.sh -run TestUpdateClientScores +./test.sh -run TestUpdateClientLocations +go build ./... && go vet ./... +``` + +Expected: the new test passes; no previously-passing test regresses. + +- [ ] **Step 7: Commit, push, open both PRs** + +```bash +git add model/network_client_location_model.go model/network_client_location_model_test.go +git commit -m "fix(model): let loadLocationStables honour forceMinimum + +The read side hardcoded forceMinimum=false, so a location where every +provider fails the minimums gate was invisible even to callers that +explicitly bypass minimums. Providers there could never be probed and so +could never graduate probation. The writer already populates both key +families; this only chooses between them. User-facing listing still +passes false." +git push -u origin feat/probe-location-force-minimum +gh pr create --repo Ryanmello07/server --base beta/self-contained-env \ + --head feat/probe-location-force-minimum \ + --title "fix(model): let loadLocationStables honour forceMinimum" \ + --body "loadLocationStables hardcoded forceMinimum=false on the read side, hiding locations where every provider fails minimums. The writer already populates both key families. User-facing listing is unchanged." + +git fetch upstream main +git checkout -b feat/probe-location-force-minimum-upstream upstream/main +git cherry-pick feat/probe-location-force-minimum +go build ./... && go vet ./... +git push -u origin feat/probe-location-force-minimum-upstream +gh pr create --repo urnetwork/server --base main \ + --head Ryanmello07:feat/probe-location-force-minimum-upstream \ + --title "fix(model): let loadLocationStables honour forceMinimum" \ + --body "The read side hardcoded forceMinimum=false, hiding locations where every provider fails the minimums gate from callers that explicitly bypass minimums." +``` + +If the cherry-pick conflicts, keep upstream's surrounding code and carry only the parameter and its use. + +--- + +## Task 2: Server-side due-provider selection + +**Repo:** `/root/urnetwork/server`. Two branches: `feat/probe-due-endpoint` and `-upstream`. + +**Context:** the prober currently decides what to probe from an in-memory TTL cache, so a restart re-probes everything and there is no durable record of what is due. Freshness already lives server-side in `provider_egress_location.observed_at`. Expose it, so selection is durable and the server owns the schedule. + +The endpoint is operator-to-server, authenticated by the same shared secret as the ingest endpoint — not a network JWT. + +**Files:** +- Modify: `model/provider_egress_location_model.go` — add the query +- Modify: `api/handlers/provider_egress_location_handlers.go` — add the handler +- Modify: `api/api.go:57` area — register the route +- Test: `model/provider_egress_location_model_test.go`, `api/handlers/provider_egress_location_handlers_test.go` + +**Interfaces:** +- Produces: `model.GetProviderEgressLocationDue(ctx context.Context, minObservedAt time.Time, limit int) []server.Id` — client ids of Public providers whose newest probe is older than `minObservedAt` or absent. +- Produces: `GET /network/provider-egress-due?limit=N` returning `{"client_ids":["..."]}`. + +- [ ] **Step 1: Create the branch** + +```bash +cd /root/urnetwork/server +git checkout beta/self-contained-env && git pull --ff-only origin beta/self-contained-env +git checkout -b feat/probe-due-endpoint +``` + +- [ ] **Step 2: Write the failing model test** + +Add to `model/provider_egress_location_model_test.go`: + +```go +func TestGetProviderEgressLocationDue(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + now := server.NowUtc() + + fresh := server.NewId() + stale := server.NewId() + never := server.NewId() + + location := &Location{ + LocationType: LocationTypeCountry, + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, location) + + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: fresh, LocationId: location.LocationId, + CountryCode: "us", ObservedAt: now.Add(-1 * time.Hour), + }) + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: stale, LocationId: location.LocationId, + CountryCode: "us", ObservedAt: now.Add(-72 * time.Hour), + }) + // `never` deliberately gets no row at all + + due := GetProviderEgressLocationDue(ctx, now.Add(-24*time.Hour), 100) + + // a provider probed an hour ago must not be re-probed; one probed three + // days ago must be; one never probed must be + connect.AssertEqual(t, slices.Contains(due, fresh), false) + connect.AssertEqual(t, slices.Contains(due, stale), true) + connect.AssertEqual(t, slices.Contains(due, never), true) + }) +} +``` + +Note: `never` only appears if the query sources candidates from connected Public providers rather than from `provider_egress_location` alone. That is the point — the set of things needing a probe is mostly things with no row yet. The test will need `never` set up as a connected client with a Public provide key, the same way Task 2 of the P0 plan did (`Testing_CreateDevice` + `ConnectNetworkClient` + `SetConnectionLocation` + `SetProvide`). Do that for all three ids so the only variable is probe freshness. + +- [ ] **Step 3: Run it and confirm it fails** + +Run: `./test.sh -run TestGetProviderEgressLocationDue` +Expected: compile failure, `undefined: GetProviderEgressLocationDue`. + +- [ ] **Step 4: Implement the query** + +Add to `model/provider_egress_location_model.go`. Source candidates from connected+valid Public providers, LEFT JOIN their egress row, and select those whose `observed_at` is missing or older than the cutoff. Mirror the P0 filter exactly — a provider without `provide_mode = 3` is not probeable and must not be returned. Order oldest-first so the longest-unprobed go first, and honour `limit`. + +Compute the cutoff in Go and pass it as an argument. **Do not** compare a naive `timestamp` column against SQL `now()` — this codebase has shipped that bug before; the comparison casts through the session timezone and silently skips a window. + +- [ ] **Step 5: Run the model test** + +Run: `./test.sh -run TestGetProviderEgressLocationDue` +Expected: PASS. + +- [ ] **Step 6: Write the failing handler test** + +Add to `api/handlers/provider_egress_location_handlers_test.go`, following the existing auth tests in that file. Cover: no secret → 401; wrong secret → 401; correct secret → 200 with a JSON body containing `client_ids`. The existing tests already show how `operatorIngestSecret` is stubbed — reuse that, and confirm your accept-case would fail against an always-401 handler (an earlier review caught exactly that gap in this file). + +- [ ] **Step 7: Implement the handler and route** + +Add `ProviderEgressLocationDue` to `api/handlers/provider_egress_location_handlers.go`, reusing `operatorSecretHeader`, the `hmac.Equal` comparison and the fail-closed `operatorIngestSecret()` already in that file. Parse `limit` from the query string, clamp it to a sane maximum (500) and default it (100). Register in `api/api.go` beside the existing route: + +```go + router.NewRoute("GET", "/network/provider-egress-due", handlers.ProviderEgressLocationDue), +``` + +- [ ] **Step 8: Verify and ship both branches** + +```bash +./test.sh -run TestGetProviderEgressLocationDue +./test.sh -run TestProviderEgressLocation +go build ./... && go vet ./... +``` + +Then commit, push, open the beta PR, cherry-pick onto `feat/probe-due-endpoint-upstream` based on `upstream/main`, and open the upstream PR — same shape as Task 1 Step 7. + +--- + +## Task 3: Prober confinement self-check and server-driven work + +**Repo:** `/tmp/sandbox/urnetwork-operator-proxy`, branch `main`. Single repo, no cherry-pick. + +**Context:** the prober must refuse to run unless its confinement is real. Because beta uses Docker and mainstream does not, the check cannot inspect namespaces or firewall rules — it must test the property directly: try to reach a geolocation API without a tunnel, and exit if that works. + +**Files:** +- Create: `confinement/confinement.go`, `confinement/confinement_test.go` +- Modify: `cmd/egress-prober/main.go` + +**Interfaces:** +- Consumes: `GET /network/provider-egress-due` from Task 2. +- Produces: `confinement.Verify(ctx context.Context, dial func(context.Context, string, string) (net.Conn, error), addrs []string, timeout time.Duration) error` + +- [ ] **Step 1: Write the failing test** + +Create `confinement/confinement_test.go`. The dialer is injected so the test never touches the real network: + +```go +func TestVerifyFailsWhenDirectConnectionSucceeds(t *testing.T) { + // a dialer that connects means nothing is stopping the prober reaching a + // geolocation api directly -- the confinement is absent and the whole + // guarantee is void, so Verify must refuse + dial := func(ctx context.Context, network, addr string) (net.Conn, error) { + c1, _ := net.Pipe() + return c1, nil + } + err := Verify(context.Background(), dial, []string{"34.117.59.81:443"}, time.Second) + if err == nil { + t.Fatal("Verify returned nil when a direct connection succeeded; it must refuse to run") + } +} + +func TestVerifyPassesWhenDirectConnectionRefused(t *testing.T) { + dial := func(ctx context.Context, network, addr string) (net.Conn, error) { + return nil, errors.New("connect: network is unreachable") + } + if err := Verify(context.Background(), dial, []string{"34.117.59.81:443"}, time.Second); err != nil { + t.Fatalf("Verify errored when the connection was refused: %s", err) + } +} + +func TestVerifyRequiresAtLeastOneAddress(t *testing.T) { + // an empty address list would vacuously "pass" and silently disable the + // entire check + dial := func(ctx context.Context, network, addr string) (net.Conn, error) { + return nil, errors.New("unreachable") + } + if err := Verify(context.Background(), dial, nil, time.Second); err == nil { + t.Fatal("Verify accepted an empty address list; that would disable the check") + } +} +``` + +- [ ] **Step 2: Run and confirm failure** + +Run: `cd /tmp/sandbox/urnetwork-operator-proxy && go test ./confinement/ -v` +Expected: build failure, `undefined: Verify`. + +- [ ] **Step 3: Implement** + +Create `confinement/confinement.go`: + +```go +// Package confinement verifies at startup that this process cannot reach a +// geolocation api directly. +// +// The prober's entire guarantee is that every geolocation lookup egresses +// through a provider, so the api reports the provider's address and never the +// operator's. That is enforced outside this process -- a restricted network +// under docker compose, systemd IPAddressDeny/IPAddressAllow otherwise -- and +// the mechanism differs per deployment. +// +// Rather than inspect a mechanism it cannot portably know, the prober tests the +// property: it attempts a direct connection and refuses to run if one succeeds. +// Operator configuration therefore stops being an assumption and becomes a +// precondition. +package confinement + +import ( + "context" + "errors" + "fmt" + "net" + "time" +) + +// ErrNotConfined reports that a direct connection succeeded. +var ErrNotConfined = errors.New("confinement: a direct connection to a geolocation address succeeded; this process is not confined") + +// ErrNoAddresses reports an empty address list, which would make the check +// vacuous. +var ErrNoAddresses = errors.New("confinement: at least one address is required") + +// DialFunc matches net.Dialer.DialContext. +type DialFunc func(ctx context.Context, network, addr string) (net.Conn, error) + +// Verify returns nil only when every address refuses a direct connection. +// +// A dial error is the expected, healthy outcome. A successful connection means +// the confinement is missing and returns ErrNotConfined. A timeout counts as +// refused: a dropped packet is what a deny rule looks like from inside. +func Verify(ctx context.Context, dial DialFunc, addrs []string, timeout time.Duration) error { + if len(addrs) == 0 { + return ErrNoAddresses + } + for _, addr := range addrs { + attemptCtx, cancel := context.WithTimeout(ctx, timeout) + conn, err := dial(attemptCtx, "tcp", addr) + cancel() + if err == nil { + if conn != nil { + conn.Close() + } + return fmt.Errorf("%w: %s", ErrNotConfined, addr) + } + } + return nil +} +``` + +- [ ] **Step 4: Run the tests** + +Run: `go test ./confinement/ -v` +Expected: all three PASS. + +- [ ] **Step 5: Wire the check into the prober** + +In `cmd/egress-prober/main.go`, immediately after flag parsing and before any tunnel or API work, call `confinement.Verify` with a real `net.Dialer{}.DialContext`, the geolocation IPs, and a short timeout (3s). On error, log and `os.Exit(1)`. + +Add a `--skip-confinement-check` flag defaulting to **false**, for the operator running the tool interactively as a one-shot diagnostic (which is how the manual probe is used). It must log loudly when set. Do not let it default true — a check that is off by default is not a check. + +Source the addresses from the same host list `geolocate` already knows, resolved to IPs at startup; do not hardcode a second copy of the endpoint list that can drift. + +- [ ] **Step 6: Replace the in-memory due cache with the server list** + +Add a `--due-url` flag (default: `/network/provider-egress-due`). When the operator secret is present, fetch the due client ids from Task 2's endpoint and probe exactly those, instead of enumerating everything and filtering by the in-memory TTL. Keep the existing enumeration path as the fallback when the endpoint returns 404, so the prober still works against a server that has not deployed Task 2. + +- [ ] **Step 7: Verify** + +```bash +go build ./... && go vet ./... && gofmt -l . && go test ./... -race -count=1 +``` + +Expected: all packages ok, `gofmt -l` silent. + +- [ ] **Step 8: Commit and push** + +```bash +git add confinement/ cmd/egress-prober/main.go +git commit -m "feat(confinement): refuse to run unless direct geolocation egress is blocked" +git push origin main +``` + +--- + +## Task 4: Deployment confinement for both environments + +**Repo:** `/root/urnetwork/server`. Two branches: `feat/probe-deployment-confinement` and `-upstream`. + +**Context:** the prober is only as confined as its deployment makes it. Beta gets a Compose service on a restricted network; mainstream gets a documented systemd unit. Task 3's self-check is what proves either one actually works. + +**Files:** +- Modify: `docker-compose.beta.yml` +- Modify: `BETA.md` +- Create: `docs/operator/prober-systemd.md` + +- [ ] **Step 1: Add the beta service** + +Add an `egress-prober` service to `docker-compose.beta.yml` on a **dedicated internal network** that reaches `api` and `connect` only. Give it the operator secret and prober JWT via the vault mount, `restart: unless-stopped`, and no added capabilities. + +The service must NOT be attached to the default network that reaches the public internet — that attachment is the confinement, and Task 3's self-check will fail the container at startup if it is wrong. That is the intended feedback loop: a misconfigured network stops the prober instead of silently leaking. + +- [ ] **Step 2: Verify the confinement actually holds** + +Bring up only the new service against the running beta stack, without recreating anything else: + +```bash +cd /root/urnetwork/server +docker compose -f docker-compose.beta.yml up -d --no-deps egress-prober +docker compose -f docker-compose.beta.yml logs --tail=30 egress-prober +``` + +Expected: the service starts and the self-check passes. Then prove the check has teeth by attaching it to the default network temporarily and confirming it **exits non-zero** with `ErrNotConfined`. Report both outcomes. + +- [ ] **Step 3: Document the non-Docker deployment** + +Create `docs/operator/prober-systemd.md` with a complete unit file using `IPAddressDeny=any` plus `IPAddressAllow=` entries for the platform api/connect addresses and localhost, `DynamicUser=yes`, `NoNewPrivileges=yes`, and no capabilities. State explicitly that `IPAddressAllow` takes addresses, not hostnames, so the operator must list the platform's addresses and update them if they change — and that Task 3's self-check is what catches it if they forget. + +- [ ] **Step 4: Update BETA.md** + +Document the new service, the secret it needs, and how to confirm the self-check passed. + +- [ ] **Step 5: Ship both branches** + +Commit, push, open the beta PR, cherry-pick onto `-upstream` based on `upstream/main`, open the upstream PR. Note that `docker-compose.beta.yml` is beta-only; if it does not exist upstream, carry only `docs/operator/prober-systemd.md` in the upstream cherry-pick and say so in the PR body. + +--- + +## Verification Summary + +| Task | Gate | +| --- | --- | +| 1 | New test proves a below-minimums location is hidden with `false` and visible with `true`; `GetProviderLocations` behaviour unchanged | +| 2 | Model test covers fresh / stale / never-probed; handler tests cover 401, 401, 200 and would fail against an always-401 handler | +| 3 | Three `confinement` tests including the empty-address case; full race suite green | +| 4 | Beta service starts confined; attaching it to the default network makes it exit non-zero | +| All | Both PRs open per server-side change | + +## Out of Scope + +- Identity rotation and multi-hop probing (P3). +- Verdict logic, RTT corroboration, the schema extension (P2). +- Bandwidth measurement and its byte budget (P2/P3). +- Letting measured bandwidth satisfy `PassesMinimums`, and whether providers should be penalised for tests their transport version cannot run. From 7717b76044771f356362b09abea365dac566515d Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Sun, 26 Jul 2026 15:58:44 +0100 Subject: [PATCH 6/6] fix(model): count country-only providers once, and keep Network providers discoverable to their own network Two production risks in the provider counting/scoring change, found by a backward-compatibility audit of this PR. 1. Country-only clients were counted three times. A client whose geo lookup resolved neither a city nor a region is stored with city_location_id = region_location_id = country_location_id -- SetConnectionLocation writes the coarsest available id into the NOT NULL city/region columns. Both fan-out loops then walked city, region and country unconditionally, so one such client added 3 to its own country's provider count and was inserted three times into its country's scoring map. The inflation is worst exactly where geo resolution is coarsest: datacenter, mobile and VPN egress. Both loops now go through the distinct set of location ids, so a client counts once per location it is actually in. A genuinely city-granular client still rolls up into its region and its country unchanged -- that half is asserted explicitly, because a dedupe keyed on the client rather than on (client, location) would silently stop city clients counting toward their country, a worse regression than the one being fixed. (The per-location and per-group scoring maps are keyed by client id, so they already absorbed the repeat; the count was the live defect. The loops are made explicit anyway so the two stay in step.) 2. Network-only providers vanished from their own network's discovery. The Public-only filter is right for the public provider count -- a stranger genuinely cannot use a ProvideModeNetwork provider -- but wrong for the candidate pool. Such a provider serves same-network sources today via the working CreateContractNoEscrow path and is discoverable to them now; filtering the pool to Public made it undiscoverable to exactly the users it exists for. That is a live regression for anyone running providers for their own organisation. The pool must also not hand a cross-network caller a provider whose contract will be refused, so this is not a revert: - UpdateClientLocations keeps the Public-only filter. That number is shown to everyone and should reflect what a stranger can reach. - The UpdateClientScores source queries now admit Public or Network -- exactly the two modes GetProvideRelationship can return, hence exactly what CreateContract can accept -- and record NetworkOnly on each ClientScore. - FindProviders2 filters at request time: a candidate is eligible if it is publicly usable, or if its NetworkId equals the requesting session's network. This cannot be baked into the cache: the client score redis entries are keyed by (forceMinimum, rankMode, locationId, callerLocationId) with no network component, so one cached set is shared by callers from every network. NetworkOnly is stored negated deliberately. The score cache is gob encoded with a 5h ttl, so entries written before the field existed decode with the zero value; the zero value therefore has to mean "publicly usable", or every provider would be treated as network-only until the cache turned over. The ClientFilter exported alongside the samples still counts only publicly usable providers. It is read solely by loadLocationStables, which decides the public Stable flag GetProviderLocations publishes -- a public surface, so it follows the same rule as the provider count. A location whose only supply is network-only is not stable and reports no providers. No Stream carve-out: resolveNonCompanionProvideMode does return companion=true for a Stream-only destination, but that dead-ends at CreateCompanionTransferEscrow, which requires a pre-existing reverse-direction origin contract, so a Stream-only destination can never bootstrap a cross-network session. The inline comments that implied Stream was the only excluded case are corrected. Tests: a country-only provider counted exactly once and a city provider still counted at all three granularities, for both UpdateClientLocations and UpdateClientScores; and all four visibility cases in FindProviders2 (Public and Network-only providers x same-network and other-network callers). The two existing pool assertions that encoded the Public-only pool are updated to the new two-tier contract; their location-stability assertions are unchanged and still pass. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- model/network_client_location_model.go | 271 ++++++++++---- model/network_client_location_model_test.go | 378 ++++++++++++++++++-- 2 files changed, 543 insertions(+), 106 deletions(-) diff --git a/model/network_client_location_model.go b/model/network_client_location_model.go index d753a666..db036e0a 100644 --- a/model/network_client_location_model.go +++ b/model/network_client_location_model.go @@ -1476,6 +1476,27 @@ func initialClientLocationsKey() string { return fmt.Sprintf("{cl}i") } +// returns the given ids with nils dropped and duplicates removed, preserving +// order. +// +// A client's city/region/country location ids are not guaranteed to differ: a +// country-only geo resolution stores the country id in all three columns (see +// SetConnectionLocation), and a region-only one stores it in two. Anything that +// fans a client out across the three columns has to treat them as a set, or the +// client is counted once per column instead of once per location it is in. +func distinctIds(ids ...*server.Id) []server.Id { + distinct := make([]server.Id, 0, len(ids)) + for _, id := range ids { + if id == nil { + continue + } + if !slices.Contains(distinct, *id) { + distinct = append(distinct, *id) + } + } + return distinct +} + func UpdateClientLocations(ctx context.Context, ttl time.Duration) (returnErr error) { topCitiesPerRegion := 20 topCitiesPerCountry := 10 @@ -1520,18 +1541,28 @@ func UpdateClientLocations(ctx context.Context, ttl time.Duration) (returnErr er WHERE network_client_location_reliability.connected = true AND network_client_location_reliability.valid = true AND - -- count only providers that can serve a stranger. - -- GetProvideRelationship returns ProvideModePublic for a - -- cross-network pair, so a Public provide key is what makes a - -- provider generally reachable; without one it advertises - -- supply nobody outside its own network can use. - -- This is deliberately narrower than what CreateContract will - -- ultimately accept: resolveNonCompanionProvideMode - -- (controller/connect_controller.go) also lets a Stream-only - -- destination settle a cross-network contract as a *companion* - -- stream, for backward compatibility with old clients. A - -- companion-only destination is a return path, not general - -- provider supply, so excluding it here is intended. + -- this is the number shown to everyone, so count only providers + -- a stranger can actually reach. GetProvideRelationship returns + -- ProvideModePublic for a cross-network pair, so a Public + -- provide key is what makes a provider generally reachable; + -- without one it advertises supply nobody outside its own + -- network can use. + -- + -- Note this is deliberately narrower than the candidate pool + -- UpdateClientScores builds. That pool also carries + -- ProvideModeNetwork providers, which are real usable supply + -- for sources inside their own network, and FindProviders2 + -- filters them per request against the caller's network. They + -- do not belong in a public count. + -- + -- Every other mode is excluded, not just Stream. In particular + -- resolveNonCompanionProvideMode + -- (controller/connect_controller.go) lets a Stream-only + -- destination be resolved as a *companion* stream, but that + -- dead-ends at CreateCompanionTransferEscrow, which requires a + -- pre-existing reverse-direction origin contract -- so a + -- Stream-only destination can never bootstrap a session and is + -- correctly absent from both the count and the pool. EXISTS ( SELECT 1 FROM provide_key WHERE @@ -1552,9 +1583,21 @@ func UpdateClientLocations(ctx context.Context, ttl time.Duration) (returnErr er &countryLocationId, )) - locationClientCounts[cityLocationId] += 1 - locationClientCounts[regionLocationId] += 1 - locationClientCounts[countryLocationId] += 1 + // count each client at most once per distinct location id. A + // client whose geo lookup resolved neither a city nor a region + // is stored with city = region = country (see + // SetConnectionLocation's country fallback), so incrementing + // all three unconditionally counted that one client three + // times in its own country. Distinct ids -- a real + // city-granular client -- still roll up into their region and + // country exactly as before. + for _, locationId := range distinctIds( + &cityLocationId, + ®ionLocationId, + &countryLocationId, + ) { + locationClientCounts[locationId] += 1 + } } }) @@ -2213,6 +2256,16 @@ type ClientScore struct { HasLatencyTest bool HasSpeedTest bool + // true when the provider holds a ProvideModeNetwork provide key but no + // ProvideModePublic one, i.e. it can only settle a contract with a source + // in its own network. FindProviders2 keeps such a provider only for callers + // in NetworkId. It is stored negated on purpose: the score cache is gob + // encoded with a 5h ttl, so entries written before this field existed decode + // with the zero value, and the zero value has to mean "publicly usable" -- + // the pre-existing behaviour -- or every provider would be treated as + // network-only until the cache turned over. + NetworkOnly bool + LookbackIndex int LookbackClientScores map[int]*ClientScore @@ -2303,6 +2356,7 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r clientScore = &ClientScore{ ClientId: lookbackClientScore.ClientId, NetworkId: lookbackClientScore.NetworkId, + NetworkOnly: lookbackClientScore.NetworkOnly, LookbackClientScores: map[int]*ClientScore{}, } m[lookbackClientScore.ClientId] = clientScore @@ -2404,6 +2458,7 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r var lookbackIndex int var reliabilityWeight float64 var independentReliabilityWeight float64 + var publiclyUsable bool server.Raise(result.Scan( &cityLocationXId, ®ionLocationXId, @@ -2419,11 +2474,13 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r &lookbackIndex, &reliabilityWeight, &independentReliabilityWeight, + &publiclyUsable, )) lookbackClientScore = &ClientScore{ ClientId: clientId, LookbackIndex: lookbackIndex, NetworkId: networkId, + NetworkOnly: !publiclyUsable, ReliabilityWeight: reliabilityWeight, IndependentReliabilityWeight: independentReliabilityWeight, MinRelativeLatencyMillis: minRelativeLatencyMillis, @@ -2470,7 +2527,17 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r -- fix(beta): see the LEFT JOIN comment below COALESCE(client_connection_reliability_score.lookback_index, 0), COALESCE(client_connection_reliability_score.reliability_weight, 1), - COALESCE(client_connection_reliability_score.independent_reliability_weight, 1) + COALESCE(client_connection_reliability_score.independent_reliability_weight, 1), + -- publicly usable: holds a Public provide key, so a caller from + -- any network can settle a contract with it. A provider that is + -- in the pool without this is Network-only, and FindProviders2 + -- hands it out to callers in its own network only. + EXISTS ( + SELECT 1 FROM provide_key + WHERE + provide_key.client_id = network_client_location_reliability.client_id AND + provide_key.provide_mode = $1 + ) FROM network_client_location_reliability @@ -2486,46 +2553,61 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r WHERE network_client_location_reliability.connected = true AND network_client_location_reliability.valid = true AND - -- count only providers that can serve a stranger; see the - -- matching comment in UpdateClientLocations above for why a - -- Public provide key is the rule and why the Stream companion - -- fallback is deliberately not honoured here. + -- the candidate pool, unlike the public count in + -- UpdateClientLocations, carries every provider that can settle + -- a contract with *someone*: Public for any caller, Network for + -- callers in the provider's own network. GetProvideRelationship + -- only ever returns one of those two, so this pair is exactly + -- the set CreateContract can accept. FindProviders2 then decides + -- eligibility per request -- it has to be done there and not + -- here, because the score cache is keyed by (forceMinimum, + -- rankMode, locationId, callerLocationId) and is not + -- network-scoped. + -- + -- Restricting this to Public would remove Network-only + -- providers from their own network's discovery, which works + -- today via CreateContractNoEscrow. Stream is still excluded: + -- resolveNonCompanionProvideMode can resolve a Stream-only + -- destination as a companion, but that dead-ends at + -- CreateCompanionTransferEscrow, which needs a pre-existing + -- reverse origin contract, so it can never bootstrap a session. + -- -- GetProviderLocations gates on loadLocationStables, populated - -- from here, so filtering UpdateClientLocations alone would - -- leave the symptom in place. + -- from here; see exportClientScores for why the ClientFilter it + -- reads stays Public-only. EXISTS ( SELECT 1 FROM provide_key WHERE provide_key.client_id = network_client_location_reliability.client_id AND - provide_key.provide_mode = $1 + provide_key.provide_mode IN ($1, $2) ) `, ProvideModePublic, + ProvideModeNetwork, ) server.WithPgResult(result, err, func() { for result.Next() { lookbackClientScore, cityLocationId, regionLocationId, countryLocationId := loadClientScore(result) - clientScores, ok := locationClientScores[*cityLocationId] - if !ok { - clientScores = map[server.Id]*ClientScore{} - locationClientScores[*cityLocationId] = clientScores - } - addClientScore(lookbackClientScore, clientScores) - - clientScores, ok = locationClientScores[*regionLocationId] - if !ok { - clientScores = map[server.Id]*ClientScore{} - locationClientScores[*regionLocationId] = clientScores - } - addClientScore(lookbackClientScore, clientScores) - - clientScores, ok = locationClientScores[*countryLocationId] - if !ok { - clientScores = map[server.Id]*ClientScore{} - locationClientScores[*countryLocationId] = clientScores + // once per distinct location id: a country-only client stores + // its country id in all three columns (see + // SetConnectionLocation), and a client belongs in a location's + // pool once. The per-location map is keyed by client id so a + // repeat is already absorbed, but going through the set makes + // the intent explicit and keeps this loop in step with the + // counting loop in UpdateClientLocations. + for _, locationId := range distinctIds( + cityLocationId, + regionLocationId, + countryLocationId, + ) { + clientScores, ok := locationClientScores[locationId] + if !ok { + clientScores = map[server.Id]*ClientScore{} + locationClientScores[locationId] = clientScores + } + addClientScore(lookbackClientScore, clientScores) } - addClientScore(lookbackClientScore, clientScores) } }) @@ -2546,7 +2628,14 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r network_client_location_reliability.has_speed_test, COALESCE(client_connection_reliability_score.lookback_index, 0), -- fix(beta): see LEFT JOIN comment below COALESCE(client_connection_reliability_score.reliability_weight, 1), - COALESCE(client_connection_reliability_score.independent_reliability_weight, 1) + COALESCE(client_connection_reliability_score.independent_reliability_weight, 1), + -- publicly usable; see the per-location query above + EXISTS ( + SELECT 1 FROM provide_key + WHERE + provide_key.client_id = network_client_location_reliability.client_id AND + provide_key.provide_mode = $1 + ) FROM network_client_location_reliability @@ -2569,49 +2658,39 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r WHERE network_client_location_reliability.connected = true AND network_client_location_reliability.valid = true AND - -- same rule as the per-location query above. This one fills - -- locationGroupClientScores -> the clientScoreLocationGroup* - -- redis keys -> loadClientScores -> FindProviders2 whenever a - -- spec carries a LocationGroupId, so leaving it unfiltered - -- means a user who picks a promoted group (e.g. "Strong - -- Privacy Laws") still gets providers that cannot accept - -- their contract and CreateContract rejects with NoPermission. + -- same rule as the per-location query above: Public or + -- Network. This one fills locationGroupClientScores -> the + -- clientScoreLocationGroup* redis keys -> loadClientScores + -- -> FindProviders2 whenever a spec carries a + -- LocationGroupId, so a user who picks a promoted group + -- (e.g. "Strong Privacy Laws") must be filtered by the same + -- request-time network check as a plain location. EXISTS ( SELECT 1 FROM provide_key WHERE provide_key.client_id = network_client_location_reliability.client_id AND - provide_key.provide_mode = $1 + provide_key.provide_mode IN ($1, $2) ) `, ProvideModePublic, + ProvideModeNetwork, ) server.WithPgResult(result, err, func() { for result.Next() { lookbackClientScore, cityLocationGroupId, regionLocationGroupId, countryLocationGroupId := loadClientScore(result) - if cityLocationGroupId != nil { - clientScores, ok := locationGroupClientScores[*cityLocationGroupId] - if !ok { - clientScores = map[server.Id]*ClientScore{} - locationGroupClientScores[*cityLocationGroupId] = clientScores - } - addClientScore(lookbackClientScore, clientScores) - } - - if regionLocationGroupId != nil { - clientScores, ok := locationGroupClientScores[*regionLocationGroupId] - if !ok { - clientScores = map[server.Id]*ClientScore{} - locationGroupClientScores[*regionLocationGroupId] = clientScores - } - addClientScore(lookbackClientScore, clientScores) - } - - if countryLocationGroupId != nil { - clientScores, ok := locationGroupClientScores[*countryLocationGroupId] + // once per distinct group id. The three location columns can + // be the same id (a country-only client), in which case all + // three group joins resolve to the same membership rows. + for _, locationGroupId := range distinctIds( + cityLocationGroupId, + regionLocationGroupId, + countryLocationGroupId, + ) { + clientScores, ok := locationGroupClientScores[locationGroupId] if !ok { clientScores = map[server.Id]*ClientScore{} - locationGroupClientScores[*countryLocationGroupId] = clientScores + locationGroupClientScores[locationGroupId] = clientScores } addClientScore(lookbackClientScore, clientScores) } @@ -2722,17 +2801,29 @@ func UpdateClientScores(ctx context.Context, ttl time.Duration, parallel int) (r filter *ClientFilter, ) { clientScores := []*ClientScore{} - netReliabilityWeight := float64(0) + publicCount := 0 + publicNetReliabilityWeight := float64(0) for _, clientScore := range s { if clientScore.PassesMinimums[rankMode] || forceMinimum { - netReliabilityWeight += clientScore.ReliabilityWeight clientScores = append(clientScores, clientScore) + if !clientScore.NetworkOnly { + publicCount += 1 + publicNetReliabilityWeight += clientScore.ReliabilityWeight + } } } + // the samples above carry Network-only providers too -- FindProviders2 + // filters them per request against the caller's network -- but the + // ClientFilter does not. It is read only by loadLocationStables, which + // decides the `Stable` flag GetProviderLocations publishes to every + // user. That is a public surface, so like the provider count in + // UpdateClientLocations it counts only providers a stranger can reach: + // a location whose only supply is network-only is not stable, and with + // zero public providers it reports no providers at all. filter = &ClientFilter{ - Count: len(clientScores), - NetReliabilityWeight: netReliabilityWeight, + Count: publicCount, + NetReliabilityWeight: publicNetReliabilityWeight, } mathrand.Shuffle(len(clientScores), func(i int, j int) { @@ -3123,6 +3214,34 @@ func FindProviders2( glog.Infof("[nclm]findproviders2 load %.2fms (%d)\n", loadMillis, len(clientScores)) } + // drop providers this caller cannot contract with. + // + // UpdateClientScores puts both Public and Network-only providers in the + // pool. A Network-only provider can only settle a contract with a + // source in its own network (GetProvideRelationship -> + // ProvideModeNetwork -> CreateContractNoEscrow); it is real, usable + // supply for those users and has always been discoverable to them, so + // it stays. For anyone else CreateContract would reject with + // NoPermission, so it goes. + // + // This must happen here, after loadClientScores, and must never be + // baked into the cached set: the client score redis entries are keyed + // by (forceMinimum, rankMode, locationId, callerLocationId) with no + // network component, so a single cached set is shared by callers from + // every network. + // + // A session with no jwt yields the zero network id, which matches no + // provider -- fail closed rather than leak. + var callerNetworkId server.Id + if session.ByJwt != nil { + callerNetworkId = session.ByJwt.NetworkId + } + for clientId, clientScore := range clientScores { + if clientScore.NetworkOnly && clientScore.NetworkId != callerNetworkId { + delete(clientScores, clientId) + } + } + for clientId, _ := range excludeFinalDestinations() { delete(clientScores, clientId) } diff --git a/model/network_client_location_model_test.go b/model/network_client_location_model_test.go index 450554d3..337b575f 100644 --- a/model/network_client_location_model_test.go +++ b/model/network_client_location_model_test.go @@ -1510,18 +1510,24 @@ func TestUpdateClientLocationsCountsOnlyPublicProviders(t *testing.T) { }) } -// `UpdateClientScores` carries the same "can this provider serve a stranger" -// filter as `UpdateClientLocations`, and it matters on a different path: -// the per-location source query fills the per-location client scores, which -// become the `clientScoreLocation*` redis keys, which `loadLocationStables` -// reads and `GetProviderLocations` gates on. A location whose only providers -// are network-only must therefore not be stable at all. +// `UpdateClientScores` writes two things, and the provide-mode rule differs +// between them. +// +// The `ClientFilter` -- read by `loadLocationStables`, which +// `GetProviderLocations` gates the public `Stable` flag on -- counts only +// providers a stranger can reach, the same rule as the provider count in +// `UpdateClientLocations`. A location whose only supply is network-only is not +// stable and reports no providers at all. +// +// The sampled candidate pool is wider: it also carries `ProvideModeNetwork` +// providers, which are real usable supply for sources in their own network. +// `FindProviders2` filters those per request (see +// TestFindProviders2NetworkOnlyProviderVisibleOnlyToItsOwnNetwork); the pool +// itself is shared by every caller and cannot make that decision. // // Two connected+valid providers with identical latency/speed data in two // different countries, differing ONLY in whether they hold a Public provide -// key. Removing the `EXISTS (... provide_mode = ...)` clause from the -// per-location source query makes the network-only provider's country stable -// and puts its client score in the cache, and this test goes red. +// key. func TestUpdateClientScoresCountsOnlyPublicProviders(t *testing.T) { server.DefaultTestEnv().Run(t, func(t testing.TB) { ctx := context.Background() @@ -1572,7 +1578,8 @@ func TestUpdateClientScoresCountsOnlyPublicProviders(t *testing.T) { _, ok = locationStables[networkOnlyCity.CountryLocationId] connect.AssertEqual(t, ok, false) - // and the same at the client-score level FindProviders2 consumes + // the pool FindProviders2 consumes carries both, each tagged with + // whether a caller outside its network may use it clientScores, err := loadClientScores( true, RankModeQuality, @@ -1586,26 +1593,33 @@ func TestUpdateClientScoresCountsOnlyPublicProviders(t *testing.T) { 100, ) connect.AssertEqual(t, err, nil) - connect.AssertEqual(t, len(clientScores), 1) - _, ok = clientScores[publicClientId] + connect.AssertEqual(t, len(clientScores), 2) + publicClientScore, ok := clientScores[publicClientId] connect.AssertEqual(t, ok, true) - _, ok = clientScores[networkOnlyClientId] - connect.AssertEqual(t, ok, false) + connect.AssertEqual(t, publicClientScore.NetworkOnly, false) + networkOnlyClientScore, ok := clientScores[networkOnlyClientId] + connect.AssertEqual(t, ok, true) + connect.AssertEqual(t, networkOnlyClientScore.NetworkOnly, true) }) } -// the location *group* source query in `UpdateClientScores` needs the same -// filter as the per-location one. It fills `locationGroupClientScores` -> -// the `clientScoreLocationGroup*` redis keys -> `loadClientScores` -> -// `FindProviders2` whenever the spec carries a `LocationGroupId`. Unfiltered, -// a user who selects a promoted group (e.g. "Strong Privacy Laws") is handed -// providers that cannot accept their contract, and `CreateContract` rejects -// them with `NoPermission`. +// the location *group* source query in `UpdateClientScores` follows the same +// Public-or-Network rule as the per-location one. It fills +// `locationGroupClientScores` -> the `clientScoreLocationGroup*` redis keys -> +// `loadClientScores` -> `FindProviders2` whenever the spec carries a +// `LocationGroupId`, so a user who selects a promoted group (e.g. "Strong +// Privacy Laws") has to be filtered by the same request-time network check as +// a user who selects a plain location -- and the group cache has to carry the +// flag that check reads. +// +// Dropping either `provide_mode` from the group query's `EXISTS` breaks this: +// without Network the group's own-network supply disappears, and without +// Public nothing is reachable at all. Dropping the `EXISTS` entirely would let +// in modes `CreateContract` can never accept. // // Same two providers, differing only in the Public provide key, each in its -// own location group. Removing the `EXISTS` clause from the group query puts -// the network-only provider in its group's cache and this test goes red. -func TestUpdateClientScoresGroupCountsOnlyPublicProviders(t *testing.T) { +// own location group. +func TestUpdateClientScoresGroupCarriesNetworkProvidersTagged(t *testing.T) { server.DefaultTestEnv().Run(t, func(t testing.TB) { ctx := context.Background() @@ -1676,13 +1690,17 @@ func TestUpdateClientScoresGroupCountsOnlyPublicProviders(t *testing.T) { 100, ) connect.AssertEqual(t, err, nil) - connect.AssertEqual(t, len(clientScores), 1) - _, ok := clientScores[publicClientId] + connect.AssertEqual(t, len(clientScores), 2) + publicClientScore, ok := clientScores[publicClientId] connect.AssertEqual(t, ok, true) - _, ok = clientScores[networkOnlyClientId] - connect.AssertEqual(t, ok, false) + connect.AssertEqual(t, publicClientScore.NetworkOnly, false) + networkOnlyClientScore, ok := clientScores[networkOnlyClientId] + connect.AssertEqual(t, ok, true) + connect.AssertEqual(t, networkOnlyClientScore.NetworkOnly, true) - // the network-only group on its own exports nothing + // the network-only group on its own exports exactly its one provider, + // tagged network-only so FindProviders2 hands it to that provider's + // own network and nobody else networkOnlyGroupClientScores, err := loadClientScores( true, RankModeQuality, @@ -1693,7 +1711,10 @@ func TestUpdateClientScoresGroupCountsOnlyPublicProviders(t *testing.T) { 100, ) connect.AssertEqual(t, err, nil) - connect.AssertEqual(t, len(networkOnlyGroupClientScores), 0) + connect.AssertEqual(t, len(networkOnlyGroupClientScores), 1) + networkOnlyClientScore, ok = networkOnlyGroupClientScores[networkOnlyClientId] + connect.AssertEqual(t, ok, true) + connect.AssertEqual(t, networkOnlyClientScore.NetworkOnly, true) }) } @@ -1749,3 +1770,300 @@ func connectPublicAndNetworkOnlyProviders( }) return } + +// A client whose geo lookup resolved no city and no region is stored with +// city_location_id = region_location_id = country_location_id -- the coarsest +// granularity available is written into all three columns. Both fan-out loops +// then walked city, region and country unconditionally, so one such client +// added 3 to its own country: `/network/provider-locations` advertised three +// times the supply that exists, and the inflation is worst exactly where geo +// resolution is coarsest (datacenter, mobile and VPN egress). +// +// The second half of this test is the guard on the fix: a genuinely +// city-granular client must still roll up into its region and its country. A +// dedupe keyed on the client alone rather than on (client, location) would +// silently stop city clients counting toward their country, which is a far +// worse regression than the one being fixed. +func TestUpdateClientLocationsCountsEachClientOncePerLocation(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + countryOnlyCity, city := createCountryOnlyAndCityProviders(ctx, t) + + err := UpdateClientLocations(ctx, time.Hour) + connect.AssertEqual(t, err, nil) + + clientLocations, err := loadClientLocations(ctx, map[server.Id]bool{ + countryOnlyCity.CountryLocationId: true, + city.CityLocationId: true, + city.RegionLocationId: true, + city.CountryLocationId: true, + }) + connect.AssertEqual(t, err, nil) + + countOf := func(locationId server.Id) int { + clientLocation, ok := clientLocations[locationId] + if !ok { + t.Fatalf("expected a cached client location for %s", locationId) + } + return clientLocation.ClientCount + } + + // one client, counted once -- not once per city/region/country column + connect.AssertEqual(t, countOf(countryOnlyCity.CountryLocationId), 1) + + // and the roll-up for a real city client is untouched + connect.AssertEqual(t, countOf(city.CityLocationId), 1) + connect.AssertEqual(t, countOf(city.RegionLocationId), 1) + connect.AssertEqual(t, countOf(city.CountryLocationId), 1) + }) +} + +// The same shape for `UpdateClientScores`: a country-only provider must appear +// once in its country's candidate pool, and a city-granular provider must still +// appear in its city's, its region's and its country's pools. +func TestUpdateClientScoresCountsEachClientOncePerLocation(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + countryOnlyCity, city := createCountryOnlyAndCityProviders(ctx, t) + + err := UpdateClientScores(ctx, time.Hour, 1) + connect.AssertEqual(t, err, nil) + + poolSizeAt := func(locationId server.Id) int { + clientScores, err := loadClientScores( + true, + RankModeQuality, + ctx, + map[server.Id]bool{locationId: true}, + map[server.Id]bool{}, + server.Id{}, + 100, + ) + connect.AssertEqual(t, err, nil) + return len(clientScores) + } + + connect.AssertEqual(t, poolSizeAt(countryOnlyCity.CountryLocationId), 1) + + // the city provider still rolls up to every granularity + connect.AssertEqual(t, poolSizeAt(city.CityLocationId), 1) + connect.AssertEqual(t, poolSizeAt(city.RegionLocationId), 1) + connect.AssertEqual(t, poolSizeAt(city.CountryLocationId), 1) + }) +} + +// connects two Public providers in two different countries and rolls the +// reliability table forward: +// +// - one whose `network_client_location` row has all three location columns +// collapsed to its country id, which is what a country-granularity geo +// lookup produces (`SetConnectionLocation` writes the coarsest available id +// into the NOT NULL city/region columns), and +// - one at genuine city granularity, with three distinct ids. +// +// The collapse is applied with a direct UPDATE rather than by handing +// `SetConnectionLocation` a country-only `location` row, so the fixture depends +// only on the shape of the stored row and not on which branch's fallback +// produced it. +func createCountryOnlyAndCityProviders(ctx context.Context, t testing.TB) ( + countryOnlyCity *Location, + city *Location, +) { + countryOnlyCity = &Location{ + LocationType: LocationTypeCity, + City: "Toronto", + Region: "Ontario", + Country: "Canada", + CountryCode: "ca", + } + CreateLocation(ctx, countryOnlyCity) + + city = &Location{ + LocationType: LocationTypeCity, + City: "Palo Alto", + Region: "California", + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, city) + + handlerId := CreateNetworkClientHandler(ctx) + connectPublicProvider := func(location *Location, ip string) server.Id { + networkId := server.NewId() + clientId := server.NewId() + Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + connectionId, _, _, _, err := ConnectNetworkClient(ctx, clientId, ip, handlerId) + connect.AssertEqual(t, err, nil) + err = SetConnectionLocation(ctx, connectionId, location.LocationId, &ConnectionLocationScores{}) + connect.AssertEqual(t, err, nil) + SetProvide(ctx, clientId, map[ProvideMode][]byte{ + ProvideModePublic: []byte("public-secret"), + }) + + // UpdateClientScores joins client_connection_reliability_score, so + // every provider needs a score row or the join, not the logic under + // test, is what excludes it. The address hash is shared across the + // fixture on purpose: validity is derived from + // network_client_connection, not from these stats. + AddClientReliabilityStats( + ctx, + networkId, + clientId, + [32]byte{}, + server.NowUtc(), + &ClientReliabilityStats{ + ConnectionEstablishedCount: 1, + ProvideEnabledCount: 1, + ReceiveMessageCount: 1, + ReceiveByteCount: 1024, + SendMessageCount: 1, + SendByteCount: 1024, + }, + ) + return clientId + } + + countryOnlyClientId := connectPublicProvider(countryOnlyCity, "0.0.0.1:0") + connectPublicProvider(city, "0.0.0.2:0") + + // collapse the first provider to country granularity + server.Tx(ctx, func(tx server.PgTx) { + server.RaisePgResult(tx.Exec( + ctx, + ` + UPDATE network_client_location + SET + city_location_id = $2, + region_location_id = $2 + WHERE client_id = $1 + `, + countryOnlyClientId, + countryOnlyCity.CountryLocationId, + )) + }) + + UpdateClientLocationReliabilities(ctx, server.NowUtc().Add(-time.Hour), server.NowUtc()) + UpdateClientReliabilityScores(ctx, server.NowUtc(), true) + return +} + +// `UpdateClientLocations` counts only providers holding a Public provide key, +// because that count is shown to everyone and a stranger genuinely cannot use a +// `Network`-only provider. The candidate pool is a different question: a +// `Network`-only provider serves sources inside its own network today (via +// `CreateContractNoEscrow`) and is discoverable to them. Restricting the pool +// to Public would make those providers invisible to the very users they exist +// for -- a live regression for anyone running providers for their own +// organisation. +// +// So the pool carries both, and eligibility is decided per request against the +// caller's network. All four cases: +// +// same-network caller other-network caller +// Public provider returned returned +// Network-only provider returned NOT returned +// +// This has to be a request-time filter, not a cache-time one: the client-score +// redis entries are keyed by (forceMinimum, rankMode, locationId, +// callerLocationId) and are not network-scoped, so one cached set is shared by +// callers from every network. +func TestFindProviders2NetworkOnlyProviderVisibleOnlyToItsOwnNetwork(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + city := &Location{ + LocationType: LocationTypeCity, + City: "Palo Alto", + Region: "California", + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, city) + + handlerId := CreateNetworkClientHandler(ctx) + + connectProvider := func(networkId server.Id, ip string, modes map[ProvideMode][]byte) server.Id { + clientId := server.NewId() + Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + connectionId, _, _, _, err := ConnectNetworkClient(ctx, clientId, ip, handlerId) + connect.AssertEqual(t, err, nil) + err = SetConnectionLocation(ctx, connectionId, city.LocationId, &ConnectionLocationScores{}) + connect.AssertEqual(t, err, nil) + SetProvide(ctx, clientId, modes) + + // upstream joins client_connection_reliability_score with an INNER + // JOIN, so a provider with no reliability row never reaches the + // pool at all. Give every provider one so this test asserts the + // provide-mode rule and nothing else. + AddClientReliabilityStats( + ctx, + networkId, + clientId, + [32]byte{}, + server.NowUtc(), + &ClientReliabilityStats{ + ConnectionEstablishedCount: 1, + ProvideEnabledCount: 1, + ReceiveMessageCount: 1, + ReceiveByteCount: 1024, + SendMessageCount: 1, + SendByteCount: 1024, + }, + ) + return clientId + } + + publicNetworkId := server.NewId() + publicClientId := connectProvider(publicNetworkId, "0.0.0.1:0", map[ProvideMode][]byte{ + ProvideModePublic: []byte("public-secret"), + ProvideModeNetwork: []byte("network-secret"), + }) + + networkOnlyNetworkId := server.NewId() + networkOnlyClientId := connectProvider(networkOnlyNetworkId, "0.0.0.2:0", map[ProvideMode][]byte{ + ProvideModeNetwork: []byte("network-secret"), + }) + + UpdateClientLocationReliabilities(ctx, server.NowUtc().Add(-time.Hour), server.NowUtc()) + UpdateClientReliabilityScores(ctx, server.NowUtc(), true) + + err := UpdateClientScores(ctx, time.Hour, 1) + connect.AssertEqual(t, err, nil) + + findFrom := func(networkId server.Id, name string) map[server.Id]bool { + clientSession := session.Testing_CreateClientSession( + ctx, + jwt.NewByJwt(networkId, server.NewId(), name, false, false), + ) + res, err := FindProviders2( + &FindProviders2Args{ + Specs: []*ProviderSpec{ + {LocationId: &city.LocationId}, + }, + Count: 100, + ForceMinimum: true, + }, + clientSession, + ) + connect.AssertEqual(t, err, nil) + found := map[server.Id]bool{} + for _, provider := range res.Providers { + found[provider.ClientId] = true + } + return found + } + + // the network-only provider's own network sees both + sameNetwork := findFrom(networkOnlyNetworkId, "same-network") + connect.AssertEqual(t, sameNetwork[publicClientId], true) + connect.AssertEqual(t, sameNetwork[networkOnlyClientId], true) + + // a stranger sees only the Public one. handing them the network-only + // provider would produce a `CreateContract` NoPermission rejection. + otherNetwork := findFrom(server.NewId(), "other-network") + connect.AssertEqual(t, otherNetwork[publicClientId], true) + connect.AssertEqual(t, otherNetwork[networkOnlyClientId], false) + }) +}