feat(beta,docs): confine the egress prober in both deployment styles

.gitignore

@@ -3,6 +3,8 @@ beta-vault/config/**/*.mmdb
 beta-vault/vault/tls/**/*.pem
 beta-vault/vault/tls/**/*.pub.pem
 beta-vault/beta-secrets.env
+# egress-prober client jwt and operator secret; the .example template is tracked
+beta-vault/prober.env
 beta-vault/vault/*.yml
 # Keep .example templates tracked
 !beta-vault/vault/*.yml.example

Caddyfile

@@ -1,7 +1,38 @@
+# connect derives every client's location from the address it sees on the
+# websocket. It only trusts a forwarded address when BOTH X-Forwarded-For and
+# X-Forwarded-Source-Port are present (connect/transport.go:220-227) -- that
+# pair is what the warp load balancer sends. Caddy sets X-Forwarded-For on its
+# own, but not the port, so connect fell through to r.RemoteAddr and saw
+# Caddy's container address (172.18.x.x) for every client. A private address
+# has no mmdb entry, so GuessLocationType returned "Unknown location type",
+# no network_client_location row was written, and no provider ever reached
+# network_client_location_reliability -- which silently emptied the provider
+# location list.
+#
+# Sending the source port restores the pair, and with it the real client
+# address. api needs it too: the same address feeds rate limiting and the
+# ARIN foreign-provider score, both of which would otherwise see one shared
+# proxy address for the entire deployment.
+(client_address) {
+	# connect trusts X-UR-Forwarded-For ahead of everything else
+	# (connect/transport.go:220) and the api derives the session client address
+	# from the same value. A proxy passes client headers through verbatim, so
+	# with Caddy as the sole ingress any client could send this header and pin
+	# itself to an address of its choosing -- forging its provider location and
+	# evading the per-address rate limits. Strip it on the way in; only the
+	# warp load balancer is entitled to set it, and it is not in this path.
+	header_up -X-UR-Forwarded-For
+	header_up X-Forwarded-Source-Port {http.request.remote.port}
+}
+
 api.beta-test.net {
-	reverse_proxy api:8080
+	reverse_proxy api:8080 {
+		import client_address
+	}
 }
 
 connect.beta-test.net {
-	reverse_proxy connect:80
+	reverse_proxy connect:80 {
+		import client_address
+	}
 }

beta-setup.sh

@@ -157,11 +157,12 @@ docker compose -f docker-compose.beta.yml up -d api connect taskworker
 
 echo
 echo "Beta network running:"
-echo "  API:     http://$PUBLIC_IP:8080"
-echo "  Connect: ws://$PUBLIC_IP:5080/"
+echo "  API:     https://api.beta-test.net"
+echo "  Connect: wss://connect.beta-test.net/"
 echo
 echo "All generated secrets live in $SECRETS_FILE and beta-vault/vault/*.yml"
 echo "These files are NOT tracked by git."
 echo "Postgres data is preserved across ./beta-setup.sh runs in the named volume."
 echo "To intentionally wipe the database, run ./beta-down.sh -v."
-echo "To use from another machine, open TCP ports 8080, 5080, and 15080."
+echo "Caddy is the only ingress; open TCP 80 and 443. The api and connect"
+echo "service ports are deliberately unpublished -- see BETA.md, Firewall."

beta-vault/prober.env.example

@@ -0,0 +1,21 @@
+# Secrets for the egress-prober service in docker-compose.beta.yml.
+#
+# Copy to beta-vault/prober.env and fill both values in. The real file is
+# gitignored; this template is not. Never commit the real one.
+#
+#   cp beta-vault/prober.env.example beta-vault/prober.env
+#
+# UR_PROBER_BY_JWT
+#   A network CLIENT jwt for the network the prober probes from. Create a
+#   network, then POST /network/auth-client with the dashboard jwt and use the
+#   by_client_jwt it returns. It must contain a client_id -- a plain user jwt
+#   has none and the prober exits with "parse by-jwt client id".
+#
+# UR_OPERATOR_SECRET
+#   Must equal ingest_secret in beta-vault/vault/provider_egress.yml, which
+#   ./beta-setup.sh generates. Read it with:
+#     grep ingest_secret beta-vault/vault/provider_egress.yml
+#   The api process memoizes it on first use, so restart api after rotating it.
+
+UR_PROBER_BY_JWT=
+UR_OPERATOR_SECRET=

controller/network_client_controller.go

@@ -52,6 +52,36 @@ func ConnectNetworkClient(
 // FIXME the client ip should be in memory only, never persisted
 // FIXME consider using dbip+a latency test for quality metrics
 
+// freshProviderEgressLocationForConnection is
+// model.GetFreshProviderEgressLocationForConnection made non-fatal: on any
+// failure it logs and returns nil, which the caller reads as "no probed
+// location" and falls through to the mmdb lookup.
+//
+// See the block comment in SetConnectionLocation for why nothing here may be
+// allowed to panic out.
+func freshProviderEgressLocationForConnection(
+	ctx context.Context,
+	connectionId server.Id,
+) *model.ProviderEgressLocation {
+	return server.HandleError1(
+		func() *model.ProviderEgressLocation {
+			return model.GetFreshProviderEgressLocationForConnection(
+				ctx,
+				connectionId,
+				model.ProviderEgressLocationMaxAge,
+			)
+		},
+		func(err error) *model.ProviderEgressLocation {
+			glog.Infof(
+				"[ncc][%s]probed egress location lookup failed, using mmdb. err = %s\n",
+				connectionId,
+				err,
+			)
+			return nil
+		},
+	)
+}
+
 func SetConnectionLocation(
 	ctx context.Context,
 	connectionId server.Id,
@@ -80,10 +110,31 @@ func SetConnectionLocation(
 	// not) on the connect-announce path and inside a retry loop, so it must
 	// not cost the two round trips (client lookup, then egress lookup) the
 	// naive version would.
-	if egress := model.GetFreshProviderEgressLocationForConnection(
+	//
+	// The lookup is non-fatal by construction. model.Db raises non-transient,
+	// non-connection postgres errors as a panic (see isTransientError /
+	// isConnectionError in db.go), and this call sits on
+	// ConnectNetworkClient's path *before* connect's disconnect-cleanup defer
+	// is registered (connect/transport_announce.go) -- so an escaping panic
+	// does not just fail the location lookup, it tears the connection down and
+	// leaves its network_client_connection row orphaned as connected = true.
+	//
+	// The concrete hazard is deploy ordering. provider_egress_location is a
+	// new table in this change; roll the binary before running
+	// `bringyourctl db migrate` and every announce hits undefined_table
+	// (42P01), which is neither transient nor a connection error, and every
+	// connection in that window orphans. That exact failure mode has already
+	// cost this project ~30k orphaned rows once, and deploy ordering is not
+	// something a test suite catches.
+	//
+	// So: swallow ANY failure here and fall through to the mmdb path.
+	// Deliberately not narrowed to transient errors -- the whole point is that
+	// an unmigrated or otherwise unhappy database must not be able to break
+	// connections. The probed location is an optimisation over mmdb, never a
+	// requirement, so mmdb is the correct answer whenever it is unavailable.
+	if egress := freshProviderEgressLocationForConnection(
 		ctx,
 		connectionId,
-		model.ProviderEgressLocationMaxAge,
 	); egress != nil && probedLocationPreferred(egress, location) {
 		scores := &model.ConnectionLocationScores{}
 		if egress.Hosting {
@@ -92,9 +143,18 @@ func SetConnectionLocation(
 		if egress.Proxy {
 			scores.NetTypePrivacy = 1
 		}
-		if egress.Mobile {
-			scores.NetTypeVirtual = 1
-		}
+		// egress.Mobile deliberately does NOT feed NetTypeVirtual: unlike
+		// Hosting/Proxy, Mobile has no mmdb-path equivalent (IpInfo has no
+		// Mobile concept; NetTypeVirtual is set from the ipinfo schema's
+		// is_satellite field only, see GetLocationForIp, and never from
+		// DB-IP). Deriving NetTypeVirtual from Mobile here would penalize a
+		// probed mobile provider's ranking with no equivalent penalty for an
+		// otherwise-identical unprobed one -- the opposite of the parity
+		// this feature is meant to preserve (see arinForeignScore's doc for
+		// the same parity reasoning applied to net_type_foreign). Mobile
+		// stays on the model/wire contract as metadata; it just does not
+		// feed the ranking score.
+
 		// keep the ARIN org-vs-country foreign check on the probed path too,
 		// so a probed provider is ranked on equal terms with an equivalent
 		// unprobed one (net_type_foreign feeds the ranking columns). It is

controller/network_client_controller_test.go

@@ -274,7 +274,14 @@ func TestSetConnectionLocationProbedNetTypeForeignMatchesMmdbParity(t *testing.T
 }
 
 // A fresh probed location's Hosting/Proxy flags must map onto the stored
-// connection's net_type_hosting/net_type_privacy scores.
+// connection's net_type_hosting/net_type_privacy scores. Mobile must NOT map
+// onto net_type_virtual: Hosting/Proxy have direct mmdb-path equivalents
+// (ipInfo.Hosting/ipInfo.Privacy, see GetLocationForIp), but Mobile has none
+// (IpInfo has no Mobile concept at all, and NetTypeVirtual is only ever set
+// from the ipinfo schema's is_satellite field, never from DB-IP or from
+// anything Mobile-shaped) -- deriving NetTypeVirtual from Mobile would give
+// a probed mobile provider a ranking penalty an identical unprobed mobile
+// provider never takes, breaking the parity this feature promises.
 func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) {
 	server.DefaultTestEnv().Run(t, func(t testing.TB) {
 		ctx := context.Background()
@@ -300,6 +307,7 @@ func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) {
 			CountryCode: "jp",
 			Hosting:     true,
 			Proxy:       true,
+			Mobile:      true,
 			ObservedAt:  server.NowUtc(),
 		})
 
@@ -308,20 +316,105 @@ func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) {
 
 		var netTypeHosting int
 		var netTypePrivacy int
+		var netTypeVirtual int
 		server.Db(ctx, func(conn server.PgConn) {
 			result, qerr := conn.Query(
 				ctx,
-				`SELECT net_type_hosting, net_type_privacy FROM network_client_location WHERE connection_id = $1`,
+				`SELECT net_type_hosting, net_type_privacy, net_type_virtual FROM network_client_location WHERE connection_id = $1`,
 				connectionId,
 			)
 			server.WithPgResult(result, qerr, func() {
 				if result.Next() {
-					server.Raise(result.Scan(&netTypeHosting, &netTypePrivacy))
+					server.Raise(result.Scan(&netTypeHosting, &netTypePrivacy, &netTypeVirtual))
 				}
 			})
 		})
 		assert.Equal(t, netTypeHosting, 1)
 		assert.Equal(t, netTypePrivacy, 1)
+		assert.Equal(t, netTypeVirtual, 0)
+	})
+}
+
+// A failing probed-egress lookup must not propagate out of
+// SetConnectionLocation, and the connection must still be located via mmdb.
+//
+// model.GetFreshProviderEgressLocationForConnection goes through server.Db,
+// which re-panics any postgres error that is neither transient nor a
+// connection error (see isTransientError / isConnectionError in db.go).
+// undefined_table (42P01) is exactly such an error, and it is not a
+// hypothetical one: provider_egress_location is a new table in this change,
+// so rolling the binary before running `bringyourctl db migrate` makes every
+// single connection announce hit it.
+//
+// That matters far more than a failed lookup, because SetConnectionLocation is
+// called from ConnectNetworkClient *before* connect's disconnect-cleanup defer
+// is registered (connect/transport_announce.go): an escaping panic tears the
+// connection down and orphans its network_client_connection row as
+// connected = true. This project has already lost ~30k rows to that exact
+// deploy-ordering mistake once.
+//
+// The failure is injected for real -- the table is dropped, so the query
+// genuinely raises 42P01 out of the pgx driver -- rather than asserted against
+// a mock, because the thing under test is what server.Db does with a real
+// postgres error, not what the call site does with a fabricated one.
+func TestSetConnectionLocationEgressLookupErrorFallsBackToMmdb(t *testing.T) {
+	server.DefaultTestEnv().Run(t, func(t testing.TB) {
+		ctx := context.Background()
+
+		clientIp := "8.8.8.8"
+
+		mmdbLocation, _, err := GetLocationForIp(ctx, clientIp)
+		assert.Equal(t, err, nil)
+		model.CreateLocation(ctx, mmdbLocation)
+
+		networkId := server.NewId()
+		clientId := server.NewId()
+		model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "")
+
+		handlerId := model.CreateNetworkClientHandler(ctx)
+		connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientIp+":0", handlerId)
+		assert.Equal(t, err, nil)
+
+		// a fresh probed entry exists, so the lookup is definitely reached --
+		// then the table it reads is dropped out from under it, which is the
+		// state a deploy-before-migrate leaves the binary in.
+		model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{
+			ClientId:    clientId,
+			LocationId:  mmdbLocation.LocationId,
+			CountryCode: "jp",
+			ObservedAt:  server.NowUtc(),
+		})
+		server.Tx(ctx, func(tx server.PgTx) {
+			server.RaisePgResult(tx.Exec(ctx, `DROP TABLE provider_egress_location`))
+		})
+
+		// the panic is caught here rather than left to kill the test binary,
+		// so a regression reports as this test failing with the reason
+		func() {
+			defer func() {
+				if r := recover(); r != nil {
+					t.Fatalf("the probed egress lookup must not panic out of SetConnectionLocation; it reached the caller as: %v", r)
+				}
+			}()
+			err = SetConnectionLocation(ctx, connectionId, clientIp)
+		}()
+		assert.Equal(t, err, nil)
+
+		// and the mmdb path still produced a location for the connection
+		var countryLocationId server.Id
+		server.Db(ctx, func(conn server.PgConn) {
+			result, qerr := conn.Query(
+				ctx,
+				`SELECT country_location_id FROM network_client_location WHERE connection_id = $1`,
+				connectionId,
+			)
+			server.WithPgResult(result, qerr, func() {
+				if result.Next() {
+					server.Raise(result.Scan(&countryLocationId))
+				}
+			})
+		})
+		assert.Equal(t, countryLocationId, mmdbLocation.CountryLocationId)
 	})
 }
 

controller/provider_egress_location_controller.go

@@ -14,6 +14,19 @@ import (
 // already older than this when it arrives. It bounds replay of an old probe.
 const MaxProviderEgressLocationSubmissionAge = 24 * time.Hour
 
+// MaxProviderEgressLocationSubmissionSkew rejects a submission whose
+// observed_at is further in the future than this. The prober and server
+// clocks should be roughly in sync, so a few minutes of allowance covers
+// ordinary clock drift without opening the door to a far-future timestamp.
+// Without this bound, a future observed_at would defeat every other
+// safeguard at once: it always wins the monotonic upsert in
+// model.SetProviderEgressLocation (so no later, legitimate probe can ever
+// overwrite it), it reads as "fresh" forever against
+// ProviderEgressLocationMaxAge, and it outlives the taskworker sweep in
+// RemoveExpiredProviderEgressLocations -- permanently pinning a provider to
+// whatever location was submitted, with no API-side recovery.
+const MaxProviderEgressLocationSubmissionSkew = 5 * time.Minute
+
 // maxLocationNameLen bounds country/city/region as submitted: these flow into
 // model.CreateLocation, whose location_name column is varchar(128). Rejecting
 // an over-long value here with a clear error is preferable to letting
@@ -66,6 +79,9 @@ func SubmitProviderEgressLocation(
 	if args.ObservedAt.Before(server.NowUtc().Add(-MaxProviderEgressLocationSubmissionAge)) {
 		return nil, fmt.Errorf("Submission is too old.")
 	}
+	if server.NowUtc().Add(MaxProviderEgressLocationSubmissionSkew).Before(args.ObservedAt) {
+		return nil, fmt.Errorf("Submission is too far in the future.")
+	}
 	if networkId := model.GetNetworkClientNetwork(ctx, args.ClientId); networkId == nil {
 		return nil, fmt.Errorf("Unknown client.")
 	}