diff --git a/server/api/store_adapters.go b/server/api/store_adapters.go index 48ea51c3..ba2e6292 100644 --- a/server/api/store_adapters.go +++ b/server/api/store_adapters.go @@ -90,12 +90,16 @@ func WorkloadVerifierFromSignedToken(secret []byte, db *sql.DB) WorkloadVerifier return domain.WorkloadBinding{}, err } // WorkloadVerifier has no context parameter (see its type in - // service.go) so the durable cross-check below cannot inherit the + // service.go) so the durable lookup below cannot inherit the // caller's request context; bound it locally instead of running // unbounded against context.Background(). ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - ok, err := store.AllocationBindingStillValid(ctx, db, claims.AllocationID, claims.MatchID, claims.ServerID) + // The token only names allocation_id (see signed_token.go for why); + // match_id/server_id come from the durable allocator record, never + // from the caller, so a token can never claim a pairing that wasn't + // actually, durably allocated. + matchID, serverID, ok, err := store.AllocationBindingByAllocationID(ctx, db, claims.AllocationID) if err != nil { return domain.WorkloadBinding{}, err } @@ -104,8 +108,8 @@ func WorkloadVerifierFromSignedToken(secret []byte, db *sql.DB) WorkloadVerifier } return domain.WorkloadBinding{ AllocationID: claims.AllocationID, - MatchID: claims.MatchID, - ServerID: claims.ServerID, + MatchID: matchID, + ServerID: serverID, }, nil } } diff --git a/server/api/workload_verifier_integration_test.go b/server/api/workload_verifier_integration_test.go index b2d52648..fdf3b347 100644 --- a/server/api/workload_verifier_integration_test.go +++ b/server/api/workload_verifier_integration_test.go @@ -48,8 +48,8 @@ func openIntegrationPostgres(t *testing.T) *sql.DB { } // seedRealAllocation claims a real ready server and allocation row, exactly -// the durable state a signed workload token must later be cross-checked -// against (see store.AllocationBindingStillValid). +// the durable state a signed workload token's allocation_id must resolve +// against (see store.AllocationBindingByAllocationID). func seedRealAllocation(t *testing.T, db *sql.DB, allocationID, matchID string, now time.Time) domain.Allocation { t.Helper() ctx := context.Background() @@ -65,11 +65,13 @@ func seedRealAllocation(t *testing.T, db *sql.DB, allocationID, matchID string, } // TestWorkloadVerifierFromSignedTokenAcceptsARealAllocation proves the full -// wired path: a token issued by workload.IssueSignedWorkloadToken for a real -// allocation row verifies successfully through -// WorkloadVerifierFromSignedToken and returns a binding matching what -// serverMutation actually checks (ServerID, MatchID). This is the "wired, -// working" counterpart to cmd/control-plane's +// wired path: a token issued by workload.IssueSignedWorkloadToken naming only +// a real allocation_id verifies successfully through +// WorkloadVerifierFromSignedToken and returns a binding whose match_id/ +// server_id came from the durable allocation record (the token itself never +// carries them -- see signed_token.go), matching what serverMutation +// actually checks (ServerID, MatchID). This is the "wired, working" +// counterpart to cmd/control-plane's // TestServerRoutesRequireWorkloadVerifyToBeWired, which pins the // unconfigured-503 case. func TestWorkloadVerifierFromSignedTokenAcceptsARealAllocation(t *testing.T) { @@ -78,7 +80,7 @@ func TestWorkloadVerifierFromSignedTokenAcceptsARealAllocation(t *testing.T) { allocation := seedRealAllocation(t, db, "alloc-verify-1", "match-verify-1", now) secret := []byte("integration-test-secret") - token, err := workload.IssueSignedWorkloadToken(secret, allocation.AllocationID, allocation.MatchID, allocation.ServerID, now, time.Minute) + token, err := workload.IssueSignedWorkloadToken(secret, allocation.AllocationID, now, time.Minute) if err != nil { t.Fatalf("issue token: %v", err) } @@ -97,14 +99,14 @@ func TestWorkloadVerifierFromSignedTokenAcceptsARealAllocation(t *testing.T) { } // TestWorkloadVerifierFromSignedTokenRejectsAnUnknownAllocation proves the -// durable cross-check actually runs: a validly-signed, unexpired token whose -// allocation was never recorded (e.g. superseded, or simply fabricated) must -// still be rejected. Signature and expiry checks alone are not enough. +// durable lookup actually runs: a validly-signed, unexpired token whose +// allocation was never recorded (e.g. simply fabricated) must still be +// rejected. Signature and expiry checks alone are not enough. func TestWorkloadVerifierFromSignedTokenRejectsAnUnknownAllocation(t *testing.T) { db := openIntegrationPostgres(t) now := time.Now().UTC() secret := []byte("integration-test-secret") - token, err := workload.IssueSignedWorkloadToken(secret, "alloc-never-recorded", "match-never-recorded", "server-never-recorded", now, time.Minute) + token, err := workload.IssueSignedWorkloadToken(secret, "alloc-never-recorded", now, time.Minute) if err != nil { t.Fatalf("issue token: %v", err) } @@ -114,22 +116,44 @@ func TestWorkloadVerifierFromSignedTokenRejectsAnUnknownAllocation(t *testing.T) } } -// TestWorkloadVerifierFromSignedTokenRejectsAMismatchedTriple proves the -// cross-check binds all three identifiers together, not each independently: -// a real allocation's own allocation_id combined with someone else's -// match/server must still fail. -func TestWorkloadVerifierFromSignedTokenRejectsAMismatchedTriple(t *testing.T) { +// TestWorkloadVerifierFromSignedTokenNeverTrustsCallerSuppliedBinding proves +// the binding returned is entirely derived from the durable allocation row, +// never from anything embedded in or inferable from the token: two distinct +// allocations produce tokens that resolve to their own, and only their own, +// match/server pairing. +func TestWorkloadVerifierFromSignedTokenNeverTrustsCallerSuppliedBinding(t *testing.T) { db := openIntegrationPostgres(t) now := time.Now().UTC() - allocation := seedRealAllocation(t, db, "alloc-verify-2", "match-verify-2", now) + first := seedRealAllocation(t, db, "alloc-verify-2a", "match-verify-2a", now) + second := seedRealAllocation(t, db, "alloc-verify-2b", "match-verify-2b", now) secret := []byte("integration-test-secret") - token, err := workload.IssueSignedWorkloadToken(secret, allocation.AllocationID, "a-different-match", allocation.ServerID, now, time.Minute) - if err != nil { - t.Fatalf("issue token: %v", err) - } verify := WorkloadVerifierFromSignedToken(secret, db) - if _, err := verify(token, now.Add(time.Second)); err == nil { - t.Fatal("expected rejection for a real allocation id paired with the wrong match id") + + firstToken, err := workload.IssueSignedWorkloadToken(secret, first.AllocationID, now, time.Minute) + if err != nil { + t.Fatalf("issue first token: %v", err) + } + firstBinding, err := verify(firstToken, now.Add(time.Second)) + if err != nil { + t.Fatalf("verify first: %v", err) + } + if firstBinding.MatchID != first.MatchID || firstBinding.ServerID != first.ServerID { + t.Fatalf("first binding %+v resolved to the wrong allocation", firstBinding) + } + + secondToken, err := workload.IssueSignedWorkloadToken(secret, second.AllocationID, now, time.Minute) + if err != nil { + t.Fatalf("issue second token: %v", err) + } + secondBinding, err := verify(secondToken, now.Add(time.Second)) + if err != nil { + t.Fatalf("verify second: %v", err) + } + if secondBinding.MatchID != second.MatchID || secondBinding.ServerID != second.ServerID { + t.Fatalf("second binding %+v resolved to the wrong allocation", secondBinding) + } + if secondBinding.MatchID == firstBinding.MatchID || secondBinding.ServerID == firstBinding.ServerID { + t.Fatalf("distinct allocations resolved to the same binding: %+v vs %+v", firstBinding, secondBinding) } } @@ -148,7 +172,7 @@ func TestWorkloadVerifierFromSignedTokenClosesTheDefaultUnwiredGap(t *testing.T) now := time.Now().UTC() allocation := seedRealAllocation(t, db, "alloc-verify-3", "match-verify-3", now) secret := []byte("integration-test-secret") - token, err := workload.IssueSignedWorkloadToken(secret, allocation.AllocationID, allocation.MatchID, allocation.ServerID, now, time.Minute) + token, err := workload.IssueSignedWorkloadToken(secret, allocation.AllocationID, now, time.Minute) if err != nil { t.Fatalf("issue token: %v", err) } diff --git a/server/store/allocation_binding_sql.go b/server/store/allocation_binding_sql.go index 7d6ab001..173ea1fd 100644 --- a/server/store/allocation_binding_sql.go +++ b/server/store/allocation_binding_sql.go @@ -5,32 +5,33 @@ import ( "database/sql" ) -// AllocationBindingStillValidSQL cross-checks a signed workload token's -// claims against the durable allocation record before trusting it. A -// validly-signed, unexpired token alone is not proof the allocation it names -// is still the live binding for that match/server pair -- this closes that -// gap defense-in-depth. allocations rows are append-only and never leave -// 'ALLOCATED' (see allocator_sql.go), so this is a simple existence check, -// not a state-machine walk. -const AllocationBindingStillValidSQL = `SELECT 1 FROM allocations -WHERE allocation_id = $1 AND match_id = $2 AND server_id = $3 AND state = 'ALLOCATED'` +// AllocationBindingByAllocationIDSQL resolves the durable match_id/server_id +// pairing for an allocation_id. A signed workload token only ever names +// allocation_id (see workload/signed_token.go for why match_id/server_id +// aren't embedded in the token itself); this is what lets WorkloadVerify +// return a binding whose match_id/server_id came from the durable allocator +// record, not from anything the caller supplied. allocations rows are +// append-only and never leave 'ALLOCATED' (see allocator_sql.go), so this is +// a simple existence lookup, not a state-machine walk. +const AllocationBindingByAllocationIDSQL = `SELECT match_id, server_id FROM allocations +WHERE allocation_id = $1 AND state = 'ALLOCATED'` -// AllocationBindingStillValid reports whether the given (allocationID, -// matchID, serverID) triple names a real, still-allocated row. db, and every -// identifier, must be non-empty -- callers pass this an already-parsed and -// signature-verified token's claims, so empty fields here indicate a caller -// bug rather than a legitimate "not found". -func AllocationBindingStillValid(ctx context.Context, db *sql.DB, allocationID, matchID, serverID string) (bool, error) { - if db == nil || allocationID == "" || matchID == "" || serverID == "" { - return false, sql.ErrNoRows +// AllocationBindingByAllocationID returns the (matchID, serverID) durably +// recorded for allocationID, and false if no such allocated row exists. db +// and allocationID must be non-empty -- callers pass this an +// already-parsed and signature-verified token's claims, so an empty +// allocationID here indicates a caller bug rather than a legitimate +// "not found". +func AllocationBindingByAllocationID(ctx context.Context, db *sql.DB, allocationID string) (matchID, serverID string, ok bool, err error) { + if db == nil || allocationID == "" { + return "", "", false, sql.ErrNoRows } - var one int - err := db.QueryRowContext(ctx, AllocationBindingStillValidSQL, allocationID, matchID, serverID).Scan(&one) + err = db.QueryRowContext(ctx, AllocationBindingByAllocationIDSQL, allocationID).Scan(&matchID, &serverID) if err == sql.ErrNoRows { - return false, nil + return "", "", false, nil } if err != nil { - return false, err + return "", "", false, err } - return true, nil + return matchID, serverID, true, nil } diff --git a/server/workload/signed_token.go b/server/workload/signed_token.go index 3d687a86..b4ebe00a 100644 --- a/server/workload/signed_token.go +++ b/server/workload/signed_token.go @@ -18,22 +18,31 @@ import ( // cluster to validate against and so cannot be built or tested here. // // This sidesteps that requirement entirely: the control plane signs its own -// short-lived token over (allocation_id, match_id, server_id, expiry) with a -// secret only it holds, exactly the way domain.SessionStore already mints -// player session tokens elsewhere in this codebase. It needs no Kubernetes -// trust boundary to verify -- HMAC signature plus expiry is self-contained. +// short-lived token over (allocation_id, expiry) with a secret only it +// holds, exactly the way domain.SessionStore already mints player session +// tokens elsewhere in this codebase. It needs no Kubernetes trust boundary +// to verify -- HMAC signature plus expiry is self-contained. +// +// The token deliberately binds ONLY allocation_id, not match_id/server_id +// too: it is meant to be requested as a GameServerAllocation annotation +// (see agones/allocation.go) in the SAME request that asks Agones to pick a +// server for this allocation -- so at mint time, the allocator knows +// allocation_id (it generates it) but not yet which server_id Agones will +// return. match_id and server_id are instead resolved durably at verify +// time from the allocations table, which the allocator records immediately +// after Agones responds (see store.AllocationBindingByAllocationID) -- so a +// token can never claim a match/server pairing that isn't what was actually, +// durably allocated. // // The delivery channel is what makes this safe despite not proving pod -// identity the way a Kubernetes-issued token would: the token is meant to be -// handed to the allocated GameServer via the same Agones GameServerAllocation -// annotation channel allocation.go already uses for match-id/allocation-id -// (see agones/allocation.go), which only the actually-allocated pod's local -// SDK sidecar can read. A caller who can present this token has already -// proven, via that channel, that it is the pod Agones allocated. +// identity the way a Kubernetes-issued token would: the token reaches the +// allocated GameServer via the same annotation channel allocation.go +// already uses for match-id/allocation-id, which only the actually- +// allocated pod's local SDK sidecar can read. A caller who can present this +// token has already proven, via that channel, that it is the pod Agones +// allocated. type SignedWorkloadToken struct { AllocationID string `json:"a"` - MatchID string `json:"m"` - ServerID string `json:"s"` ExpiresAt time.Time `json:"e"` } @@ -46,15 +55,15 @@ var ( ) // IssueSignedWorkloadToken produces a compact "payload.signature" token -// binding the three identifiers the API layer actually checks (see -// api.Service's WorkloadVerify call site: it only compares ServerID and -// MatchID on the returned domain.WorkloadBinding). now must be non-zero and -// ttl must be positive so a token is never silently issued already-expired. -func IssueSignedWorkloadToken(secret []byte, allocationID, matchID, serverID string, now time.Time, ttl time.Duration) (string, error) { +// binding allocation_id, the one identifier known at mint time (see the +// type doc above for why match_id/server_id aren't embedded). now must be +// non-zero and ttl must be positive so a token is never silently issued +// already-expired. +func IssueSignedWorkloadToken(secret []byte, allocationID string, now time.Time, ttl time.Duration) (string, error) { if len(secret) == 0 { return "", ErrEmptyWorkloadSecret } - if allocationID == "" || matchID == "" || serverID == "" { + if allocationID == "" { return "", ErrTokenClaims } if now.IsZero() || ttl <= 0 { @@ -62,8 +71,6 @@ func IssueSignedWorkloadToken(secret []byte, allocationID, matchID, serverID str } claims := SignedWorkloadToken{ AllocationID: allocationID, - MatchID: matchID, - ServerID: serverID, ExpiresAt: now.Add(ttl).UTC(), } payload, err := json.Marshal(claims) @@ -113,7 +120,7 @@ func ParseSignedWorkloadToken(secret []byte, token string, now time.Time) (Signe if err := json.Unmarshal(payload, &claims); err != nil { return SignedWorkloadToken{}, ErrMalformedToken } - if claims.AllocationID == "" || claims.MatchID == "" || claims.ServerID == "" || claims.ExpiresAt.IsZero() { + if claims.AllocationID == "" || claims.ExpiresAt.IsZero() { return SignedWorkloadToken{}, ErrTokenClaims } if now.IsZero() { diff --git a/server/workload/signed_token_test.go b/server/workload/signed_token_test.go index 3ad99228..41352995 100644 --- a/server/workload/signed_token_test.go +++ b/server/workload/signed_token_test.go @@ -9,7 +9,7 @@ import ( func TestSignedWorkloadTokenRoundTrips(t *testing.T) { secret := []byte("test-secret") now := time.Unix(1_700_000_000, 0).UTC() - token, err := IssueSignedWorkloadToken(secret, "alloc-1", "match-1", "server-1", now, time.Minute) + token, err := IssueSignedWorkloadToken(secret, "alloc-1", now, time.Minute) if err != nil { t.Fatalf("issue: %v", err) } @@ -17,7 +17,7 @@ func TestSignedWorkloadTokenRoundTrips(t *testing.T) { if err != nil { t.Fatalf("parse: %v", err) } - if claims.AllocationID != "alloc-1" || claims.MatchID != "match-1" || claims.ServerID != "server-1" { + if claims.AllocationID != "alloc-1" { t.Fatalf("unexpected claims: %+v", claims) } } @@ -25,7 +25,7 @@ func TestSignedWorkloadTokenRoundTrips(t *testing.T) { func TestSignedWorkloadTokenRejectsExpiry(t *testing.T) { secret := []byte("test-secret") now := time.Unix(1_700_000_000, 0).UTC() - token, err := IssueSignedWorkloadToken(secret, "alloc-1", "match-1", "server-1", now, time.Minute) + token, err := IssueSignedWorkloadToken(secret, "alloc-1", now, time.Minute) if err != nil { t.Fatalf("issue: %v", err) } @@ -43,7 +43,7 @@ func TestSignedWorkloadTokenRejectsExpiry(t *testing.T) { func TestSignedWorkloadTokenRejectsTamperedPayload(t *testing.T) { secret := []byte("test-secret") now := time.Unix(1_700_000_000, 0).UTC() - token, err := IssueSignedWorkloadToken(secret, "alloc-1", "match-1", "server-1", now, time.Minute) + token, err := IssueSignedWorkloadToken(secret, "alloc-1", now, time.Minute) if err != nil { t.Fatalf("issue: %v", err) } @@ -55,7 +55,7 @@ func TestSignedWorkloadTokenRejectsTamperedPayload(t *testing.T) { func TestSignedWorkloadTokenRejectsWrongSecret(t *testing.T) { now := time.Unix(1_700_000_000, 0).UTC() - token, err := IssueSignedWorkloadToken([]byte("secret-a"), "alloc-1", "match-1", "server-1", now, time.Minute) + token, err := IssueSignedWorkloadToken([]byte("secret-a"), "alloc-1", now, time.Minute) if err != nil { t.Fatalf("issue: %v", err) } @@ -80,20 +80,16 @@ func TestIssueSignedWorkloadTokenRejectsInvalidInput(t *testing.T) { name string secret []byte allocationID string - matchID string - serverID string now time.Time ttl time.Duration }{ - {"empty secret", nil, "a", "m", "s", now, time.Minute}, - {"empty allocation id", []byte("k"), "", "m", "s", now, time.Minute}, - {"empty match id", []byte("k"), "a", "", "s", now, time.Minute}, - {"empty server id", []byte("k"), "a", "m", "", now, time.Minute}, - {"zero now", []byte("k"), "a", "m", "s", time.Time{}, time.Minute}, - {"non-positive ttl", []byte("k"), "a", "m", "s", now, 0}, + {"empty secret", nil, "a", now, time.Minute}, + {"empty allocation id", []byte("k"), "", now, time.Minute}, + {"zero now", []byte("k"), "a", time.Time{}, time.Minute}, + {"non-positive ttl", []byte("k"), "a", now, 0}, } for _, c := range cases { - if _, err := IssueSignedWorkloadToken(c.secret, c.allocationID, c.matchID, c.serverID, c.now, c.ttl); err == nil { + if _, err := IssueSignedWorkloadToken(c.secret, c.allocationID, c.now, c.ttl); err == nil { t.Fatalf("%s: expected an error, got nil", c.name) } }