WorkloadVerify (api.Service.WorkloadVerify) was permanently unwired: both
/v1/servers/{id}/register and /v1/servers/{id}/result always 503, because
the only design considered so far was verifying a Kubernetes-projected
service-account JWT (server/workload/jwt.go), which needs a live cluster's
TokenReview/JWKS endpoint to validate against safely -- something this
sandbox cannot do without guessing at a trust boundary.
The API layer doesn't actually require that specific mechanism. serverMutation
only compares WorkloadBinding.ServerID and .MatchID (server/api/service.go);
AdvanceServerRegistration only uses .MatchID/.ServerID/.AllocationID. Nothing
downstream needs Namespace/ServiceAcct/PodUID/GameServerUID populated.
This adds a self-contained alternative: a short-lived, HMAC-signed token the
control plane mints and verifies with a secret only it holds (server/workload/
signed_token.go), the same trust model domain.SessionStore already uses for
player sessions elsewhere in this codebase. It needs no cluster to verify --
signature + expiry is fully self-contained and unit-testable.
The design's soundness rests on the delivery channel, not the crypto: the
token is meant to reach the allocated GameServer via the same Agones
GameServerAllocation annotation channel allocation.go already uses for
match-id/allocation-id, readable only by that pod's own local SDK sidecar. A
caller presenting this token has already proven, via that channel, that it is
the pod Agones allocated. (Wiring the actual annotation delivery -- extending
agones.Client.Allocate and the supervisor's token source -- is a separate,
follow-up change; this commit lands the verification core it depends on.)
store.AllocationBindingStillValid adds defense-in-depth on top of signature
and expiry: it cross-checks the token's claims against the durable
allocations table (append-only, never leaves 'ALLOCATED'), so a validly-signed
token naming an allocation that was never recorded -- or a real allocation id
paired with a mismatched match/server -- is still rejected.
api.WorkloadVerifierFromSignedToken wires the two together and is now plugged
into cmd/control-plane (new --workload-secret / COSMIC_CLASH_WORKLOAD_SECRET
flag; a startup warning is logged if it's left unset, since the route then
stays 503 exactly as before) and cmd/testkit-api (fixed test secret, since
that binary is test-only already).
Verified: new unit tests in server/workload (signature tamper, wrong secret,
expiry boundary, malformed input) and a new Postgres integration suite in
server/api (real allocation row, real signed token, acceptance / unknown-
allocation rejection / mismatched-triple rejection / the previously-503
Service.WorkloadVerify field itself) -- both run clean with -race across
multiple passes against a live postgres:17-alpine container. Full
`go build ./... && go vet ./... && gofmt -l . && go test ./... -race` and
`go test -tags integration ./... -race` both clean.
Same discovery pattern as ResultSubmitter/SessionIssuer, one level
deeper: api.Service.rankedProfile and .profile both only ever read
from an in-memory RankedProfiles map with no durable-store equivalent
at all -- not "adapter exists but unwired" this time, there was no
adapter. Every real request to GET /v1/profile/ranked or /api/v1/profile
always 404'd regardless of a player's actual rating.
Add RankedProfileProvider (an interface, not a struct-literal adapter
this time) and store.PostgresRankedProfiles reading the ratings table;
Service.rankedProfileFor prefers it when set and falls back to the
map otherwise, so every existing test/direct Service literal keeps
compiling and passing unchanged. A missing ratings row maps to the
exact same (zero value, false, nil) the map lookup already produced,
preserving existing not-found semantics rather than reinterpreting
them. LastSeasonID/SeasonHistory are deliberately left unset -- the
ratings table has no season pointer, and reconstructing history needs
its own query and display semantics, not bundled in here speculatively.
Wired into both cmd/control-plane and cmd/testkit-api. Verified
against real PostgreSQL via curl: a fresh identity's ranked profile
correctly 404s through the real adapter (same behavior as before,
now for a real reason instead of an empty map).
Same pattern and same discovery method as the ResultSubmitter fix:
store.PostgresSessions already implements SessionIssuer.Issue (used by
steamSession() to mint sessions) as well as SessionBackend.Authenticate
(used to verify them), and was already wired for the latter -- but not
the former, so /v1/session/steam always 503'd with auth_unavailable
even before considering whether SteamLogin (the real, still-correctly-
unwired Steam blocker) was available. Wire it: same struct value,
second field.
Not independently visible via a black-box HTTP test yet -- SteamLogin
still nil means the handler's first guard clause still 503s before
ever reaching SessionIssuer, so the observable symptom is unchanged
until Steam access exists. Verified by reading the handler's actual
branch order, not by a test that would currently pass for the wrong
reason.
Investigating the fleet.yaml wiring task found something more
fundamental than a manifest problem: cmd/control-plane/main.go never
wires WorkloadVerify, and store.PostgresResults (a ready-made,
already-correct ResultSubmitter adapter matching the interface
exactly) was referenced from nowhere outside its own file -- not even
a test. Both server-authenticated routes this session built
(/v1/servers/{id}/register and the pre-existing /result) are
completely unreachable in the actual running control-plane binary
today: Service.serverMutation treats a nil WorkloadVerify as fatal
for both routes regardless of ServerRegistrar/ResultSubmitter being
present, so every real request 503s.
Wire the safe, obviously-correct half: ResultSubmitter now uses
store.PostgresResults{DB: db}, same pattern as ServerRegistrar.
Deliberately NOT attempting a WorkloadVerify implementation here.
server/workload/jwt.go's ParseAndValidate needs a pre-known "expected"
WorkloadBinding to construct its policy against (itself needing a
durable per-allocation lookup that doesn't exist yet) plus a real
cryptographic SignatureVerifier -- which for a Kubernetes projected
service account token means either fetching/caching the cluster's own
JWKS or delegating to the API server's TokenReview endpoint, a
different verification model that doesn't fit ParseAndValidate's
signature-callback shape at all and would need its own domain-level
adapter. This is authentication-critical code with no existing
wiring example anywhere in the codebase to follow, and the actual
trust boundary (a live cluster's key material) can't be validated
from this sandbox regardless of how carefully the client code is
written. Building it fast under this session's already-heavy pace
risked a subtle, dangerous mistake far more costly than leaving the
gap named precisely, which is what this commit does instead.
Added TestServerRoutesRequireWorkloadVerifyToBeWired: pins the
current 503-on-every-request behavior as an explicit, visible
regression trip-wire rather than a silent gap -- it's designed to
start failing (and be updated, not deleted) the day WorkloadVerify is
actually wired.
server/observability existed fully unit-tested but was imported by
nothing outside its own package -- no HTTP handler ever called it, so
its credential redaction protected zero real log output. Wire it into
Service via an optional Log field (nil-safe, so every existing Service
literal keeps compiling unchanged) and call it from the two
workload-authenticated server routes -- register and result -- at
every outcome: unauthorized, rejected, conflict and success. Wire
cmd/control-plane to actually emit those events as JSON lines on
stderr.
Add a secret canary test that drives both routes end to end with
realistic bearer-token and result-nonce values and asserts neither
literal secret appears anywhere in what Service.Log actually received
-- a stronger claim than the existing observability unit test, which
only proves redact() strips a synthetic value under a denylisted key
name. redact() is still key-name-based, not content-based: a future
call site that logs a secret under an unlisted key name would not be
caught by this test or by redact() itself, only by the same discipline
applied here of never putting raw request/token bytes into Fields.
Queue, proposal and assignment mutation routes are not wired yet.
Add POST /v1/servers/{id}/register (and its /api/v1 contract alias),
authenticated by the same workload binding as the result route. A
game server reports its protocol version and image digest and asks
to advance ALLOCATING -> PROCESS_READY -> ASSIGNMENT_READY; the store
boundary (AdvanceServerRegistration) does this as one idempotent
SERIALIZABLE transaction that also advances every participant's queue
ticket, and gates the final transition on every participant having a
live, unexpired assignment.
Adversarial review of the surrounding routing turned up a pre-existing
bug: contractServerMutation rejected any path containing '/', so the
already-documented /api/v1/servers/{id}/result route (and this new
/register route) 404'd for every real caller despite being declared
in the OpenAPI contract. Fix it to delegate shape validation to
serverMutation, matching how contractQueueMutation handles its own
two-segment paths, and add a regression test covering both contract
routes end to end.
Josh Creek