diff --git a/internal/http_handlers/token_exchange.go b/internal/http_handlers/token_exchange.go index 6bf546f0..c5c7242f 100644 --- a/internal/http_handlers/token_exchange.go +++ b/internal/http_handlers/token_exchange.go @@ -256,13 +256,19 @@ func (h *httpProvider) handleTokenExchangeGrant(gc *gin.Context, agent *schemas. } delegated, err := h.TokenProvider.CreateDelegatedAccessToken(&token.DelegationTokenConfig{ - Subject: subject, - Actor: act, - Audience: resource, - Scope: effective, - ClientID: agent.ClientID, - HostName: hostname, - SessionID: sessionID, + Subject: subject, + Actor: act, + Audience: resource, + Scope: effective, + ClientID: agent.ClientID, + HostName: hostname, + // Carry a machine subject's identity onto the minted token. Without + // this the token drops login_method entirely and service.fga + // re-classifies the service account as a human user + // (GHSA-vq29-8q3c-3hrm). onBehalfOfType was derived above from the + // subject_token's OWN login_method, not from anything the caller sent. + ServiceAccountSubject: onBehalfOfType == "agent", + SessionID: sessionID, }) if err != nil { log.Debug().Err(err).Msg("failed to mint delegated token") diff --git a/internal/integration_tests/delegation_machine_identity_test.go b/internal/integration_tests/delegation_machine_identity_test.go new file mode 100644 index 00000000..065d15cd --- /dev/null +++ b/internal/integration_tests/delegation_machine_identity_test.go @@ -0,0 +1,441 @@ +package integration_tests + +import ( + "context" + "encoding/json" + "net/http" + "net/url" + "testing" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" + + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/graph/model" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// newDelegationAgentFull is newDelegationAgent, but returns the whole client so +// a test can assert on the SURROGATE id (schemas.Client.ID) that a machine +// token carries as `sub` — distinct from the public client_id that appears in +// the `act` chain. +func newDelegationAgentFull(t *testing.T, ts *testSetup, ceiling string) (*schemas.Client, string) { + t.Helper() + secret := "agent-secret-" + uuid.New().String() + hash, err := bcrypt.GenerateFromPassword([]byte(secret), bcrypt.DefaultCost) + require.NoError(t, err) + agent, err := ts.StorageProvider.AddClient(context.Background(), &schemas.Client{ + Name: "agent-" + uuid.New().String(), + Kind: constants.ClientKindServiceAccount, + ClientSecret: string(hash), + AllowedScopes: ceiling, + IsActive: true, + }) + require.NoError(t, err) + return agent, secret +} + +// exchangeTokens performs an RFC 8693 exchange and returns the response recorder. +func exchangeTokens(t *testing.T, ts *testSetup, router http.Handler, subjectToken, actorToken, agentClientID, agentSecret, resource string) (int, string) { + t.Helper() + form := url.Values{} + form.Set("grant_type", tokenExchangeGrant) + form.Set("subject_token", subjectToken) + form.Set("subject_token_type", accessTokenType) + form.Set("actor_token", actorToken) + form.Set("actor_token_type", accessTokenType) + form.Set("requested_token_type", accessTokenType) + form.Set("resource", resource) + w := postTokenExchange(ts, router, form, agentClientID, agentSecret) + if w.Code != http.StatusOK { + return w.Code, "" + } + var resp map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + tok, _ := resp["access_token"].(string) + return w.Code, tok +} + +// TestDelegatedTokenKeepsMachineIdentity is the regression test for +// GHSA-vq29-8q3c-3hrm. +// +// A delegated token minted from a SERVICE-ACCOUNT subject dropped the +// `login_method` claim. service/fga.go classifies a caller with no +// login_method as "user:", so an autonomous machine identity was +// re-classified as an interactive user — flipping OpenFGA decisions from deny +// to allow and slipping past every login_method-keyed guard. +// +// The report framed this as SELF-delegation (subject == actor) and proposed +// rejecting that shape. That is a symptom patch: the laundering is a property +// of CreateDelegatedAccessToken omitting the claim, so it applies identically +// to the multi-hop agent-to-agent chain the design explicitly supports and +// which the proposed check does NOT cover. Both shapes are asserted here. +func TestDelegatedTokenKeepsMachineIdentity(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + router := gin.New() + router.POST("/oauth/token", ts.HttpProvider.TokenHandler()) + + resource := "https://api.example.com/v1" + + t.Run("self delegation: subject and actor are the same service account", func(t *testing.T) { + agent, secret := newDelegationAgentFull(t, ts, "openid,email") + machine := agentAccessToken(t, ts, router, agent.ClientID, secret) + + // Sanity: the machine token identifies itself as a service account. + mc := decodeJWTPayload(t, machine) + require.Equal(t, constants.AuthRecipeMethodServiceAccount, mc["login_method"]) + require.Equal(t, agent.ID, mc["sub"], "machine token sub is the surrogate id") + + code, delegated := exchangeTokens(t, ts, router, machine, machine, agent.ClientID, secret, resource) + require.Equal(t, http.StatusOK, code, "self-exchange currently succeeds") + + dc := decodeJWTPayload(t, delegated) + assert.Equal(t, agent.ID, dc["sub"], "sub is still the service account") + // THE BUG: without login_method, fga.go resolves this to user:. + assert.Equal(t, constants.AuthRecipeMethodServiceAccount, dc["login_method"], + "a delegated token whose SUBJECT is a service account must keep the machine identity") + }) + + t.Run("multi-hop: agent A delegates agent B's machine identity", func(t *testing.T) { + // The shape the reporter's subject!=actor check would NOT catch. + agentA, secretA := newDelegationAgentFull(t, ts, "openid,email") + agentB, secretB := newDelegationAgentFull(t, ts, "openid,email") + + subjectMachine := agentAccessToken(t, ts, router, agentB.ClientID, secretB) + actorMachine := agentAccessToken(t, ts, router, agentA.ClientID, secretA) + + code, delegated := exchangeTokens(t, ts, router, subjectMachine, actorMachine, agentA.ClientID, secretA, resource) + require.Equal(t, http.StatusOK, code, "multi-hop agent chain is a supported shape") + + dc := decodeJWTPayload(t, delegated) + assert.Equal(t, agentB.ID, dc["sub"], "subject is agent B") + assert.Equal(t, constants.AuthRecipeMethodServiceAccount, dc["login_method"], + "agent B's machine identity must survive delegation by agent A") + + act, ok := dc["act"].(map[string]interface{}) + require.True(t, ok, "delegation must record the actor") + assert.Equal(t, agentA.ClientID, act["sub"], "immediate actor is agent A") + }) + + t.Run("user subject is unchanged: no login_method stamped", func(t *testing.T) { + // Backward-compatibility guard. A USER-subject delegation must keep + // resolving to user:, i.e. carry no login_method, exactly as + // before. Stamping it here would break every existing delegation. + agent, secret := newDelegationAgentFull(t, ts, "openid,email,profile") + actor := agentAccessToken(t, ts, router, agent.ClientID, secret) + userToken := testAccessToken(t, ts) + + code, delegated := exchangeTokens(t, ts, router, userToken, actor, agent.ClientID, secret, resource) + require.Equal(t, http.StatusOK, code) + + dc := decodeJWTPayload(t, delegated) + _, hasLoginMethod := dc["login_method"] + assert.False(t, hasLoginMethod, + "a USER-subject delegation must carry no login_method, so it still resolves to user:") + }) +} + +// TestDelegatedMachineTokenRevocationWorks disproves the report's third claim, +// that the machine-derived `sid` ("service_account:|") addresses "a +// coordinate machine tokens never occupy", so revocation silently misfires. +// +// It does occupy it: the client_credentials handler registers the machine token +// at session key "service_account:" under "access_token_" +// (internal/http_handlers/token.go), which is exactly what +// delegationSessionIsLive looks up. Deleting that session must kill the +// delegated token at Authorizer's own API. +func TestDelegatedMachineTokenRevocationWorks(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + router := gin.New() + router.POST("/oauth/token", ts.HttpProvider.TokenHandler()) + + agent, secret := newDelegationAgentFull(t, ts, "openid,email") + machine := agentAccessToken(t, ts, router, agent.ClientID, secret) + mc := decodeJWTPayload(t, machine) + nonce, _ := mc["nonce"].(string) + require.NotEmpty(t, nonce, "machine token must carry a nonce") + + sessionKey := constants.AuthRecipeMethodServiceAccount + ":" + agent.ID + // The session the delegation will bind to must exist right now. + _, err := ts.MemoryStoreProvider.GetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+nonce) + require.NoError(t, err, "machine token IS registered at the coordinate the report calls unoccupied") + + code, delegated := exchangeTokens(t, ts, router, machine, machine, + agent.ClientID, secret, testAuthorizerHost(ts)) + require.Equal(t, http.StatusOK, code) + + dc := decodeJWTPayload(t, delegated) + assert.Equal(t, sessionKey+"|"+nonce, dc["sid"], "sid addresses the machine token's own session") + + // Revoke by deleting that session, then the delegated token must stop + // authenticating at Authorizer's own API. + gc := &gin.Context{} + gc.Request, _ = http.NewRequest(http.MethodGet, testAuthorizerHost(ts), nil) + gc.Request.Header.Set("X-Authorizer-URL", testAuthorizerHost(ts)) + + _, beforeErr := ts.TokenProvider.ValidateDelegatedAccessToken(gc, delegated) + require.NoError(t, beforeErr, "delegated token should validate while the session is live") + + require.NoError(t, ts.MemoryStoreProvider.DeleteUserSession(sessionKey, nonce)) + + _, afterErr := ts.TokenProvider.ValidateDelegatedAccessToken(gc, delegated) + assert.Error(t, afterErr, "revocation must take the delegated token down with the session") +} + +// fgaMachineDelegationModel adds `service_account` alongside `user`/`agent` so +// a machine subject can actually be expressed. fgaAgentModel omits it. +const fgaMachineDelegationModel = `model + schema 1.1 +type user +type agent +type service_account +type document + relations + define viewer: [user, agent, service_account] + define can_view: viewer +` + +// TestDelegatedMachineIdentityDoesNotFlipFgaDecision is the end-to-end +// acceptance test for GHSA-vq29-8q3c-3hrm: the deny -> allow flip itself, +// driven through the public GraphQL permission API with a real delegated token +// minted by the real /oauth/token endpoint. +func TestDelegatedMachineIdentityDoesNotFlipFgaDecision(t *testing.T) { + cfg := getTestConfig() + ts, _ := initFGATestSetup(t, cfg) + _, ctx := createContext(ts) + + router := gin.New() + router.POST("/oauth/token", ts.HttpProvider.TokenHandler()) + + setAdminCookie(t, ts) + _, err := ts.GraphQLProvider.FgaWriteModel(ctx, &model.FgaWriteModelInput{Dsl: fgaMachineDelegationModel}) + require.NoError(t, err) + + agent, secret := newDelegationAgentFull(t, ts, "openid,email") + machine := agentAccessToken(t, ts, router, agent.ClientID, secret) + code, delegated := exchangeTokens(t, ts, router, machine, machine, + agent.ClientID, secret, testAuthorizerHost(ts)) + require.Equal(t, http.StatusOK, code) + + checkAs := func(t *testing.T, tok string) bool { + t.Helper() + presentDelegatedToken(ts, tok) + res, cErr := ts.GraphQLProvider.CheckPermissions(ctx, &model.CheckPermissionsInput{ + Checks: []*model.PermissionCheckInput{{Relation: "can_view", Object: "document:readme"}}, + }) + require.NoError(t, cErr) + require.NotNil(t, res) + require.Len(t, res.Results, 1) + return res.Results[0].Allowed + } + + t.Run("a user: grant must NOT reach the machine's delegated token", func(t *testing.T) { + // This is the exploit, reproduced exactly as reported: the delegated + // token was classified as "user:", so a grant written for a + // user-shaped principal answered for an autonomous machine. + // + // BOTH tuples are required to reproduce it. The delegated caller is + // still subject to perms(agent) ∩ perms(subject), so granting only the + // user half is denied by the AGENT half for an unrelated reason — a + // test that writes one tuple passes against the vulnerable code and + // proves nothing. The reporter's PoC wrote both; so does this. + setAdminCookie(t, ts) + _, wErr := ts.GraphQLProvider.FgaWriteTuples(ctx, &model.FgaWriteTuplesInput{ + Tuples: []*model.FgaTupleInput{ + {User: "user:" + agent.ID, Relation: "viewer", Object: "document:readme"}, + {User: "agent:" + agent.ClientID, Relation: "viewer", Object: "document:readme"}, + }, + }) + require.NoError(t, wErr) + + assert.False(t, checkAs(t, delegated), + "delegated token must resolve to service_account:, not user:") + assert.False(t, checkAs(t, machine), + "the machine token was always denied; the delegated one must agree with it") + }) + + t.Run("the correct service_account grant plus an agent grant is allowed", func(t *testing.T) { + // Proves the token still WORKS under its real identity — the fix denies + // the laundered subject, it does not break machine delegation. Both + // halves of perms(agent) ∩ perms(subject) must be granted. + setAdminCookie(t, ts) + _, wErr := ts.GraphQLProvider.FgaWriteTuples(ctx, &model.FgaWriteTuplesInput{ + // The agent half was already granted above; only the correct + // service_account subject is missing. + Tuples: []*model.FgaTupleInput{ + {User: "service_account:" + agent.ClientID, Relation: "viewer", Object: "document:readme"}, + }, + }) + require.NoError(t, wErr) + + assert.True(t, checkAs(t, delegated), + "with both halves granted the delegated machine token must be allowed") + }) + + t.Run("dropping the agent half denies again: the intersection is still enforced", func(t *testing.T) { + // The widening regression guard. Stamping login_method routes this + // caller down resolveFgaCaller's MACHINE branch, which used to discard + // actorID. If it still did, authority would collapse to + // perms(subject) alone and this would wrongly stay allowed. + setAdminCookie(t, ts) + _, dErr := ts.GraphQLProvider.FgaDeleteTuples(ctx, &model.FgaWriteTuplesInput{ + Tuples: []*model.FgaTupleInput{ + {User: "agent:" + agent.ClientID, Relation: "viewer", Object: "document:readme"}, + }, + }) + require.NoError(t, dErr) + + assert.False(t, checkAs(t, delegated), + "perms(agent) ∩ perms(subject) must still hold for a machine subject") + }) +} + +// TestChainedMachineSubjectReExchange pins a BEHAVIOUR CHANGE introduced by +// stamping login_method, so it is a deliberate decision rather than a surprise. +// +// Before the fix, a machine-subject delegated token carried no login_method, so +// re-exchanging it sent token_exchange.go down the USER branch, which did +// GetUserByID(), found nothing, and rejected the hop +// with "subject could not be verified". The multi-hop agent chain therefore +// only ever worked for its FIRST hop. +// +// After the fix the token names its real identity, so the hop takes the agent +// branch and succeeds — which is what the design intends. It stays bounded by +// every existing control: the subject client must still be active, scope is +// still intersected downward, and maxActChainDepth still caps the chain. +func TestChainedMachineSubjectReExchange(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + router := gin.New() + router.POST("/oauth/token", ts.HttpProvider.TokenHandler()) + + resource := "https://api.example.com/v1" + subjectAgent, subjectSecret := newDelegationAgentFull(t, ts, "openid,email,profile") + hop1Agent, hop1Secret := newDelegationAgentFull(t, ts, "openid,email") + hop2Agent, hop2Secret := newDelegationAgentFull(t, ts, "openid") + + subjectMachine := agentAccessToken(t, ts, router, subjectAgent.ClientID, subjectSecret) + hop1Actor := agentAccessToken(t, ts, router, hop1Agent.ClientID, hop1Secret) + + code, hop1Tok := exchangeTokens(t, ts, router, subjectMachine, hop1Actor, + hop1Agent.ClientID, hop1Secret, resource) + require.Equal(t, http.StatusOK, code) + + hop2Actor := agentAccessToken(t, ts, router, hop2Agent.ClientID, hop2Secret) + code, hop2Tok := exchangeTokens(t, ts, router, hop1Tok, hop2Actor, + hop2Agent.ClientID, hop2Secret, resource) + require.Equal(t, http.StatusOK, code, "a machine-subject chain must now survive past hop 1") + + c2 := decodeJWTPayload(t, hop2Tok) + assert.Equal(t, subjectAgent.ID, c2["sub"], "subject stays agent B across hops") + assert.Equal(t, constants.AuthRecipeMethodServiceAccount, c2["login_method"], + "the machine identity must survive every hop, not just the first") + assert.ElementsMatch(t, []string{"openid"}, claimScope(t, c2), + "attenuation still narrows monotonically down the chain") + + act2, ok := c2["act"].(map[string]interface{}) + require.True(t, ok) + assert.Equal(t, hop2Agent.ClientID, act2["sub"], "immediate actor is hop 2") + prior, ok := act2["act"].(map[string]interface{}) + require.True(t, ok, "hop 1 must remain nested beneath hop 2") + assert.Equal(t, hop1Agent.ClientID, prior["sub"]) + + t.Run("a deactivated subject stops the chain", func(t *testing.T) { + // The control that bounds the newly-reachable path: liveness is still + // re-checked at every hop, not just the first. + subjectAgent.IsActive = false + _, uErr := ts.StorageProvider.UpdateClient(context.Background(), subjectAgent) + require.NoError(t, uErr) + + hop3Actor := agentAccessToken(t, ts, router, hop2Agent.ClientID, hop2Secret) + code, _ := exchangeTokens(t, ts, router, hop2Tok, hop3Actor, + hop2Agent.ClientID, hop2Secret, resource) + assert.Equal(t, http.StatusBadRequest, code, + "a deactivated service-account subject must not seed a further hop") + }) +} + +// TestRequiredRelationsClassifiesMachineSubject covers the THIRD FGA decision +// surface. check_permissions and list_permissions both base their decision on +// resolveFgaCaller's classified subject; enforceRequiredRelations hardcoded +// "user:" and threw that classification away, so a machine identity was +// answered as a human user there. +// +// It needs no delegation at all: a plain client_credentials token presented to +// validate_jwt_token with required_relations was satisfied by a tuple written +// for "user:", while check_permissions denied the very +// same token. That is exactly the "two answers to one authority question" the +// surface's own doc comment warns about, and a gateway gating on +// required_relations would admit requests the permission API refuses. +func TestRequiredRelationsClassifiesMachineSubject(t *testing.T) { + cfg := getTestConfig() + ts, _ := initFGATestSetup(t, cfg) + _, ctx := createContext(ts) + router := gin.New() + router.POST("/oauth/token", ts.HttpProvider.TokenHandler()) + + setAdminCookie(t, ts) + _, err := ts.GraphQLProvider.FgaWriteModel(ctx, &model.FgaWriteModelInput{Dsl: fgaMachineDelegationModel}) + require.NoError(t, err) + + agent, secret := newDelegationAgentFull(t, ts, "openid,email") + machine := agentAccessToken(t, ts, router, agent.ClientID, secret) + + gate := func(t *testing.T, object string) error { + t.Helper() + _, gErr := ts.GraphQLProvider.ValidateJWTToken(ctx, &model.ValidateJWTTokenRequest{ + Token: machine, + TokenType: constants.TokenTypeAccessToken, + RequiredRelations: []*model.FgaRelationInput{ + {Relation: "can_view", Object: object}, + }, + }) + return gErr + } + + t.Run("a user: tuple must NOT satisfy the gate", func(t *testing.T) { + setAdminCookie(t, ts) + _, wErr := ts.GraphQLProvider.FgaWriteTuples(ctx, &model.FgaWriteTuplesInput{ + Tuples: []*model.FgaTupleInput{ + {User: "user:" + agent.ID, Relation: "viewer", Object: "document:laundered"}, + }, + }) + require.NoError(t, wErr) + + assert.Error(t, gate(t, "document:laundered"), + "required_relations must classify the machine token as service_account:") + }) + + t.Run("the correct service_account tuple DOES satisfy the gate", func(t *testing.T) { + // Proves the surface still works under the real identity — the fix + // denies the laundered subject, it does not break machine callers. + setAdminCookie(t, ts) + _, wErr := ts.GraphQLProvider.FgaWriteTuples(ctx, &model.FgaWriteTuplesInput{ + Tuples: []*model.FgaTupleInput{ + {User: "service_account:" + agent.ClientID, Relation: "viewer", Object: "document:proper"}, + }, + }) + require.NoError(t, wErr) + + assert.NoError(t, gate(t, "document:proper"), + "a machine token granted under its real subject must pass the gate") + }) + + t.Run("required_relations agrees with check_permissions", func(t *testing.T) { + // The invariant the surface's doc comment promises. Both must give the + // same answer for the same token, relation and object. + presentDelegatedToken(ts, machine) + res, cErr := ts.GraphQLProvider.CheckPermissions(ctx, &model.CheckPermissionsInput{ + Checks: []*model.PermissionCheckInput{{Relation: "can_view", Object: "document:laundered"}}, + }) + require.NoError(t, cErr) + require.Len(t, res.Results, 1) + assert.False(t, res.Results[0].Allowed, "check_permissions denies the laundered subject") + assert.Error(t, gate(t, "document:laundered"), "required_relations must agree") + }) +} diff --git a/internal/integration_tests/fga_service_account_test.go b/internal/integration_tests/fga_service_account_test.go index 1b60d45f..69882c1c 100644 --- a/internal/integration_tests/fga_service_account_test.go +++ b/internal/integration_tests/fga_service_account_test.go @@ -220,9 +220,11 @@ func TestFGAServiceAccountSubject(t *testing.T) { // (c)+(d) A user/session token still resolves to user: and NEVER inherits // the service account's grants — even though a service_account with tuples - // exists. A real RFC 8693 delegated token carries a user sub + an act chain - // and no service_account login_method, so it is classified here exactly like - // this user token: the guard is structural (see callerOwnSubject). + // exists. A USER-SUBJECT RFC 8693 delegated token carries a user sub + an act + // chain and no login_method, so it is classified here exactly like this user + // token. (A delegated token whose SUBJECT is a service account is a different + // case: it carries login_method=service_account and resolves to + // service_account: — see TestDelegatedTokenKeepsMachineIdentity.) t.Run("user token stays user: and does not inherit service_account grants", func(t *testing.T) { clearCookies(ts) ts.GinContext.Request.Header.Del("Authorization") diff --git a/internal/integration_tests/token_exchange_session_liveness_test.go b/internal/integration_tests/token_exchange_session_liveness_test.go index 4cbf853a..4a57c4a8 100644 --- a/internal/integration_tests/token_exchange_session_liveness_test.go +++ b/internal/integration_tests/token_exchange_session_liveness_test.go @@ -101,8 +101,9 @@ func TestTokenExchangeServiceAccountSubjectIsExemptFromSessionCheck(t *testing.T // TestTokenExchangeChainedHopFollowsTheSubjectSession pins that the check reads // the `sid` a chained exchange carries, not just a first-hop `nonce`. // -// A delegated token deliberately carries no login_method or nonce claim, so hop 2 -// resolves its session through the `sid` hop 1 stamped. If the check only ever +// A delegated token carries no nonce claim (and, for the user subject used here, +// no login_method either), so hop 2 resolves its session through the `sid` hop 1 +// stamped. If the check only ever // looked at `nonce`, hop 2 would silently skip it and a logout would stop the // first hop while leaving every subsequent one working. func TestTokenExchangeChainedHopFollowsTheSubjectSession(t *testing.T) { diff --git a/internal/service/fga.go b/internal/service/fga.go index bcadd85f..0bb4fb67 100644 --- a/internal/service/fga.go +++ b/internal/service/fga.go @@ -135,21 +135,29 @@ func (p *provider) resolveFgaSubject(ctx context.Context, meta RequestMetadata, // // MACHINE vs USER vs DELEGATED — the classification keys ONLY on the token's // login_method claim: -// - login_method == constants.AuthRecipeMethodServiceAccount is stamped -// EXCLUSIVELY on client_credentials machine tokens -// (token.createMachineAccessToken). Those tokens have no resource-owner user -// (sub is the service account's surrogate id) and never carry an RFC 8693 -// `act` delegation claim. Such a caller resolves to -// "service_account:". -// - every other login_method (human recipes, sso) resolves to "user:". +// - login_method == constants.AuthRecipeMethodServiceAccount marks a MACHINE +// subject. It is stamped on client_credentials tokens +// (token.createMachineAccessToken) and on a delegated token whose SUBJECT is +// a service account (token.DelegationTokenConfig.ServiceAccountSubject). +// Either way `sub` is the service account's surrogate id and the caller +// resolves to "service_account:". +// - every other login_method — INCLUDING its absence — resolves to +// "user:". // -// This makes the delegation guard structural, not a runtime check: an RFC 8693 -// delegated token (token.CreateDelegatedAccessToken) is stateless, carries a -// user `sub` plus an `act` chain, and carries NO login_method claim — so it can -// never be classified as a machine subject and always resolves to "user:". -// The security-critical rule (delegated and user tokens stay user subjects; only -// autonomous machine tokens become service_account subjects) holds by -// construction. +// The absence rule is why the claim must be stamped. An earlier version relied +// on a delegated token carrying NO login_method and concluded that a delegated +// token "always resolves to user: by construction". That was correct for a +// user subject and wrong for a machine one: a service account exchanging a +// token got a credential with no login_method, so this function classified an +// autonomous machine as a human user, flipping OpenFGA decisions from deny to +// allow (GHSA-vq29-8q3c-3hrm). Classification follows the SUBJECT's real +// identity, which the mint now records explicitly rather than leaving to be +// inferred from a missing claim. +// +// Delegation is orthogonal to this split and is carried by actorID, not by +// login_method: BOTH a user-subject and a machine-subject delegated token +// resolve with a non-empty actorID, so delegationSubjects applies the +// perms(agent) ∩ perms(subject) intersection to each identically. // // The actor is read from the same source as the subject, never from a second // lookup: authctx.Principal is populated ONLY by the gRPC interceptor, so a @@ -173,17 +181,50 @@ func (p *provider) resolveFgaCaller(ctx context.Context, meta RequestMetadata) ( if callerID == "" { return fgaCaller{}, nil } + // actorID is carried on BOTH branches, machine included. + // + // It used to be dropped for a machine subject, on the reasoning that a + // machine token never carries an `act` chain so a service_account subject + // is never delegated. That is true of a client_credentials token — which + // has no `act`, so actorID is "" there anyway — but NOT of a delegated + // token whose SUBJECT is a service account (the multi-hop agent chain). + // Since such a token now correctly carries login_method=service_account it + // classifies as a machine subject WITH an actor, and dropping the actor + // would collapse authority from perms(agent) ∩ perms(subject) to + // perms(subject) alone — trading identity laundering for privilege + // widening. + subject, err := p.fgaSubjectFor(ctx, callerID, loginMethod) + if err != nil { + return fgaCaller{}, err + } + return fgaCaller{subject: subject, actorID: actorID}, nil +} + +// fgaSubjectFor maps a (subject id, login_method) pair to its canonical OpenFGA +// subject. It is the SINGLE source of truth for that classification, shared by +// resolveFgaCaller (which classifies the CALLER) and enforceRequiredRelations +// (which classifies the identity a presented token represents). +// +// It exists because those two used to classify differently. +// enforceRequiredRelations hardcoded "user:", so a machine identity was +// evaluated as a human user on that surface — the same laundering primitive as +// GHSA-vq29-8q3c-3hrm, reachable without any delegation at all: a plain +// client_credentials token presented to validate_jwt_token with +// required_relations was answered against "user:", +// so a tuple written for a user-shaped principal satisfied a gate for an +// autonomous machine. check_permissions denied the very same token, which is +// precisely the "two answers to one authority question" this surface's own doc +// comment warns about. +// +// Fail-closed: machineFgaSubject denies on any lookup failure, a client whose +// kind is not service_account, an inactive client, or separator smuggling in +// the client_id. A machine subject is therefore never silently downgraded to a +// user subject. +func (p *provider) fgaSubjectFor(ctx context.Context, subjectID, loginMethod string) (string, error) { if loginMethod == constants.AuthRecipeMethodServiceAccount { - subject, err := p.machineFgaSubject(ctx, callerID) - if err != nil { - return fgaCaller{}, err - } - // A machine token never carries an `act` chain (see above), so a - // service_account subject is never delegated. Dropping any actorID here - // keeps that invariant enforced rather than merely documented. - return fgaCaller{subject: subject}, nil + return p.machineFgaSubject(ctx, subjectID) } - return fgaCaller{subject: "user:" + callerID, actorID: actorID}, nil + return "user:" + subjectID, nil } // machineFgaSubject maps an authenticated client_credentials caller — whose @@ -299,7 +340,7 @@ func toContextualTuples(in []*model.FgaTupleInput) ([]engine.ContextualTuple, er // relation, same object. Two answers to one authority question is worse than // either answer: a gateway gating on required_relations would admit a request // the permission API refuses. -func (p *provider) enforceRequiredRelations(ctx context.Context, meta RequestMetadata, log zerolog.Logger, userID string, required []*model.FgaRelationInput) error { +func (p *provider) enforceRequiredRelations(ctx context.Context, meta RequestMetadata, log zerolog.Logger, userID, subjectLoginMethod string, required []*model.FgaRelationInput) error { if len(required) == 0 { return nil } @@ -316,7 +357,17 @@ func (p *provider) enforceRequiredRelations(ctx context.Context, meta RequestMet if err != nil { return PermissionDenied("unauthorized") } - subjects, err := p.delegationSubjects(ctx, caller, "user:"+userID, metrics.FgaOpRequiredRelations) + // Classify the presented token's OWN identity, exactly as the permission + // APIs classify theirs. Hardcoding "user:"+userID here is what let a + // machine token be answered as a human user on this surface — see + // fgaSubjectFor. + subject, err := p.fgaSubjectFor(ctx, userID, subjectLoginMethod) + if err != nil { + metrics.RecordFgaCheck(metrics.FgaOpRequiredRelations, metrics.FgaResultError) + log.Debug().Err(err).Msg("required relations: failed to classify the token subject; denying") + return PermissionDenied("unauthorized") + } + subjects, err := p.delegationSubjects(ctx, caller, subject, metrics.FgaOpRequiredRelations) if err != nil { metrics.RecordFgaCheck(metrics.FgaOpRequiredRelations, metrics.FgaResultError) log.Debug().Err(err).Msg("required relations: failed to resolve delegation subjects; denying") diff --git a/internal/service/session.go b/internal/service/session.go index 11841fb0..63936d13 100644 --- a/internal/service/session.go +++ b/internal/service/session.go @@ -61,7 +61,7 @@ func (p *provider) Session(ctx context.Context, meta RequestMetadata, params *mo // Fine-grained authorization gate (AND semantics, fail-closed). if params != nil && len(params.RequiredRelations) > 0 { - if err := p.enforceRequiredRelations(ctx, meta, log, userID, params.RequiredRelations); err != nil { + if err := p.enforceRequiredRelations(ctx, meta, log, userID, "", params.RequiredRelations); err != nil { log.Debug().Err(err).Msg("Required relations not satisfied") return nil, nil, err } diff --git a/internal/service/validate_jwt_token.go b/internal/service/validate_jwt_token.go index f0577cbd..ca274758 100644 --- a/internal/service/validate_jwt_token.go +++ b/internal/service/validate_jwt_token.go @@ -40,6 +40,10 @@ func (p *provider) ValidateJwtToken(ctx context.Context, meta RequestMetadata, p return nil, nil, Unauthenticated("invalid token") } userID = sub + // The PRESENTED token's own login_method. It classifies the subject for the + // required_relations gate below, so a machine token is answered as + // service_account: rather than user:. + subjectLoginMethod, _ := claims["login_method"].(string) if tokenType == constants.TokenTypeAccessToken || tokenType == constants.TokenTypeRefreshToken { nonceVal, ok := claims["nonce"].(string) @@ -116,7 +120,7 @@ func (p *provider) ValidateJwtToken(ctx context.Context, meta RequestMetadata, p } // Fine-grained authorization gate (AND semantics, fail-closed). if len(params.RequiredRelations) > 0 { - if err := p.enforceRequiredRelations(ctx, meta, log, userID, params.RequiredRelations); err != nil { + if err := p.enforceRequiredRelations(ctx, meta, log, userID, subjectLoginMethod, params.RequiredRelations); err != nil { log.Debug().Err(err).Msg("Required relations not satisfied") return nil, nil, err } diff --git a/internal/service/validate_session.go b/internal/service/validate_session.go index b9171031..60246f8f 100644 --- a/internal/service/validate_session.go +++ b/internal/service/validate_session.go @@ -64,7 +64,7 @@ func (p *provider) ValidateSession(ctx context.Context, meta RequestMetadata, pa } // Fine-grained authorization gate (AND semantics, fail-closed). if params != nil && len(params.RequiredRelations) > 0 { - if err := p.enforceRequiredRelations(ctx, meta, log, userID, params.RequiredRelations); err != nil { + if err := p.enforceRequiredRelations(ctx, meta, log, userID, "", params.RequiredRelations); err != nil { log.Debug().Err(err).Msg("Required relations not satisfied") return nil, nil, err } diff --git a/internal/token/delegation_token.go b/internal/token/delegation_token.go index 37648790..a94bfb7e 100644 --- a/internal/token/delegation_token.go +++ b/internal/token/delegation_token.go @@ -74,6 +74,21 @@ type DelegationTokenConfig struct { // produce one) but a token without it can never authenticate HERE; it // remains usable at the downstream resource server it was bound to. SessionID string + // ServiceAccountSubject stamps login_method=service_account on tokens whose + // SUBJECT is a service account (the multi-hop agent chain, where agent A + // exchanges agent B's machine token — and the degenerate case where they + // are the same account). + // + // Without it such a token carries no login_method at all, and + // service.resolveFgaCaller classifies "no login_method" as a human user. + // A machine identity therefore laundered itself into "user:", + // flipping OpenFGA decisions from deny to allow and slipping past every + // login_method-keyed guard (GHSA-vq29-8q3c-3hrm). + // + // It is set ONLY for a service-account subject. A user-subject delegation + // still carries no login_method and still resolves to "user:", which + // is what keeps every existing delegation working unchanged. + ServiceAccountSubject bool } // DelegationSessionID encodes the memory-store coordinates of the session a @@ -96,10 +111,18 @@ type DelegationTokenConfig struct { // The format mirrors ValidateAccessToken's session-key derivation // (":|", the login_method half omitted when the // token carries none) so both paths address the same entry. It is deliberately -// NOT stamped as separate `nonce` and `login_method` claims: a `login_method` -// claim on a delegated token would make service/fga.go classify the caller as a -// service_account subject, silently breaking the invariant that a delegated -// token always resolves to "user:". +// NOT stamped as a separate `nonce` claim, which would make the stateless token +// look addressable by the stateful validator. +// +// An earlier version of this comment also claimed a `login_method` claim must +// never appear on a delegated token, because service/fga.go would then classify +// the caller as a service_account subject and break "a delegated token always +// resolves to user:". That invariant was itself the bug +// (GHSA-vq29-8q3c-3hrm): it is correct for a USER subject and wrong for a +// SERVICE-ACCOUNT one, where resolving to "user:" launders a machine +// identity into a human one. login_method is now stamped for exactly the +// service-account case — see DelegationTokenConfig.ServiceAccountSubject — and +// still omitted for a user subject. // // NOTE the OIDC Back-Channel Logout token (backchannel_logout.go) also carries // a `sid`, and sends the BARE NONCE. The two are deliberately not identical — @@ -162,6 +185,13 @@ func (p *provider) CreateDelegatedAccessToken(cfg *DelegationTokenConfig) (*JWTT if cfg.SessionID != "" { claims["sid"] = cfg.SessionID } + // Carry a machine subject's identity forward. See ServiceAccountSubject: + // omitting this let a service account's delegated token be classified as a + // human user. Deliberately NOT set for a user subject, whose absence of the + // claim is what keeps it resolving to "user:". + if cfg.ServiceAccountSubject { + claims["login_method"] = constants.AuthRecipeMethodServiceAccount + } signed, err := p.signJWTToken(claims, accessTokenJWTType) if err != nil { return nil, err