Privilege retention / Privilege escalation due to orphaned access records (legacy storage)

Proof-of-concept steps (SQL-based validation of cascade cleanup): Assumptions: - Grafana deployment with legacy storage enabled (unified storage path in use). - Database schema includes team(org_id, id, uid, ...) and team_member(org_id, team_id, user_id, ...). - Access check uses legacy team_member data to determine membership privileges. Step 1: Setup - Create an organization (org_id = 1). - Create a team with UID 'team-1' and numeric id 1 for org 1. - Add two members to the team in the legacy table: INSERT INTO team_member (org_id, team_id, user_id, ...) VALUES (1, 1, 101, ...), (1, 1, 102, ...); Step 2: Verify current membership rows exist - Count legacy membership rows for this team: SELECT COUNT(*) FROM team_member WHERE org_id = 1 AND team_id = 1; - Expected result: 2 Step 3: Delete the team via the API (or internal DeleteTeam flow) - Issue a delete for the team (e.g., REST API or internal call): DELETE /api/teams/team-1?orgId=1 - This simulates the normal deletion path before the patch that also cascades the legacy rows. Step 4: Post-deletion verification (pre-patch behavior) - Check legacy membership rows after deletion: SELECT COUNT(*) FROM team_member WHERE org_id = 1 AND team_id = 1; - Expected result (without the patch): 2 (or at least > 0) indicating orphaned records still exist and may grant privileges. Step 5: Post-patch expectation - With the patch applied, the cascade delete should remove legacy team_member rows as part of team deletion. - Run the same check again (after applying the patch and performing the delete): SELECT COUNT(*) FROM team_member WHERE org_id = 1 AND team_id = 1; - Expected result: 0 Impact demonstration (conceptual): - Before fix: Deleting a team leaves team_member rows intact, potentially allowing membership-based access checks to succeed against the deleted team, effectively retaining privileges. - After fix: Deleting a team also deletes its legacy team_member rows, removing any prior privileges tied to that team. Notes: - The exact IDs, table/column names may vary by environment; adapt the queries to match your schema. - The key proof of vulnerability is the existence of non-cleared team_member rows after a team delete in legacy storage, which the patch remedies by cascading delete.

diff --git a/pkg/registry/apis/iam/legacy/delete_team_members_by_team.sql b/pkg/registry/apis/iam/legacy/delete_team_members_by_team.sql new file mode 100644 index 0000000000000..55cff410b8cd2 --- /dev/null +++ b/pkg/registry/apis/iam/legacy/delete_team_members_by_team.sql @@ -0,0 +1,3 @@ +DELETE FROM {{ .Ident .TeamMemberTable }} +WHERE org_id = {{ .Arg .Command.OrgID }} + AND team_id = {{ .Arg .Command.TeamID }} diff --git a/pkg/registry/apis/iam/legacy/sql_test.go b/pkg/registry/apis/iam/legacy/sql_test.go index 85714e4358915..43f0b1fbd54c5 100644 --- a/pkg/registry/apis/iam/legacy/sql_test.go +++ b/pkg/registry/apis/iam/legacy/sql_test.go @@ -109,6 +109,12 @@ func TestIdentityQueries(t *testing.T) { return &v } + deleteTeamMembersByTeam := func(q *DeleteTeamMembersByTeamCommand) sqltemplate.SQLTemplate { + v := newDeleteTeamMembersByTeam(nodb, q) + v.SQLTemplate = mocks.NewTestingSQLTemplate() + return &v + } + createTeamMembersBulk := func(q *CreateTeamMembersBulkCommand) sqltemplate.SQLTemplate { v := newCreateTeamMembersBulk(nodb, q) v.SQLTemplate = mocks.NewTestingSQLTemplate() @@ -419,6 +425,15 @@ func TestIdentityQueries(t *testing.T) { }), }, }, + sqlDeleteTeamMembersByTeamQuery: { + { + Name: "delete_team_members_by_team", + Data: deleteTeamMembersByTeam(&DeleteTeamMembersByTeamCommand{ + OrgID: 1, + TeamID: 1, + }), + }, + }, sqlCreateTeamMembersBulkQuery: { { Name: "create_team_members_bulk_single", diff --git a/pkg/registry/apis/iam/legacy/team.go b/pkg/registry/apis/iam/legacy/team.go index 94d3dd88b3784..708efe4188362 100644 --- a/pkg/registry/apis/iam/legacy/team.go +++ b/pkg/registry/apis/iam/legacy/team.go @@ -625,6 +625,11 @@ func (s *legacySQLStore) DeleteTeam(ctx context.Context, ns claims.NamespaceInfo } teamID := existing.ID + memberDeleteReq := newDeleteTeamMembersByTeam(sql, &DeleteTeamMembersByTeamCommand{ + OrgID: ns.OrgID, + TeamID: teamID, + }) + return sql.DB.GetSqlxSession().WithTransaction(ctx, func(st *session.SessionTx) error { if cmd.ExternalGroupReconciler != nil { if err := cmd.ExternalGroupReconciler.DeleteAll(ctx, st, ns.OrgID, teamID); err != nil { @@ -632,6 +637,14 @@ func (s *legacySQLStore) DeleteTeam(ctx context.Context, ns claims.NamespaceInfo } } + memberDeleteQuery, err := sqltemplate.Execute(sqlDeleteTeamMembersByTeamQuery, memberDeleteReq) + if err != nil { + return fmt.Errorf("error executing team members delete template: %w", err) + } + if _, err := st.Exec(ctx, memberDeleteQuery, memberDeleteReq.GetArgs()...); err != nil { + return fmt.Errorf("failed to delete team members: %w", err) + } + teamDeleteQuery, err := sqltemplate.Execute(sqlDeleteTeamTemplate, teamDeleteReq) if err != nil { return fmt.Errorf("error executing team delete template: %w", err) diff --git a/pkg/registry/apis/iam/legacy/team_binding.go b/pkg/registry/apis/iam/legacy/team_binding.go index 861747680e75a..9f98bf2b81966 100644 --- a/pkg/registry/apis/iam/legacy/team_binding.go +++ b/pkg/registry/apis/iam/legacy/team_binding.go @@ -402,6 +402,15 @@ type DeleteTeamMembersBulkCommand struct { UIDs []string } +// DeleteTeamMembersByTeamCommand removes all team_member rows for a team in a +// single SQL DELETE. Used by DeleteTeam to cascade member cleanup the way the +// legacy team store does. OrgID scopes the DELETE so a TeamID can't reach +// across orgs. +type DeleteTeamMembersByTeamCommand struct { + OrgID int64 + TeamID int64 +} + // CreateTeamMembersBulkCommand inserts multiple team_member rows in a single // multi-row INSERT. Each member must be fully populated (TeamID/TeamUID/UserID // already resolved, OrgID set, timestamps filled). @@ -411,6 +420,7 @@ type CreateTeamMembersBulkCommand struct { var sqlDeleteTeamMemberQuery = mustTemplate("delete_team_member_query.sql") var sqlDeleteTeamMembersBulkQuery = mustTemplate("delete_team_members_bulk.sql") +var sqlDeleteTeamMembersByTeamQuery = mustTemplate("delete_team_members_by_team.sql") var sqlCreateTeamMembersBulkQuery = mustTemplate("create_team_members_bulk.sql") type deleteTeamMembersBulkQuery struct { @@ -431,6 +441,24 @@ func newDeleteTeamMembersBulk(sql *legacysql.LegacyDatabaseHelper, cmd *DeleteTe } } +type deleteTeamMembersByTeamQuery struct { + sqltemplate.SQLTemplate + TeamMemberTable string + Command *DeleteTeamMembersByTeamCommand +} + +func (r deleteTeamMembersByTeamQuery) Validate() error { + return nil +} + +func newDeleteTeamMembersByTeam(sql *legacysql.LegacyDatabaseHelper, cmd *DeleteTeamMembersByTeamCommand) deleteTeamMembersByTeamQuery { + return deleteTeamMembersByTeamQuery{ + SQLTemplate: sqltemplate.New(sql.DialectForDriver()), + TeamMemberTable: sql.Table("team_member"), + Command: cmd, + } +} + type createTeamMembersBulkQuery struct { sqltemplate.SQLTemplate TeamMemberTable string diff --git a/pkg/registry/apis/iam/legacy/testdata/mysql--delete_team_members_by_team-delete_team_members_by_team.sql b/pkg/registry/apis/iam/legacy/testdata/mysql--delete_team_members_by_team-delete_team_members_by_team.sql new file mode 100755 index 0000000000000..25c95c75c0fe9 --- /dev/null +++ b/pkg/registry/apis/iam/legacy/testdata/mysql--delete_team_members_by_team-delete_team_members_by_team.sql @@ -0,0 +1,3 @@ +DELETE FROM `grafana`.`team_member` +WHERE org_id = 1 + AND team_id = 1 diff --git a/pkg/registry/apis/iam/legacy/testdata/postgres--delete_team_members_by_team-delete_team_members_by_team.sql b/pkg/registry/apis/iam/legacy/testdata/postgres--delete_team_members_by_team-delete_team_members_by_team.sql new file mode 100755 index 0000000000000..93ee87d8faa15 --- /dev/null +++ b/pkg/registry/apis/iam/legacy/testdata/postgres--delete_team_members_by_team-delete_team_members_by_team.sql @@ -0,0 +1,3 @@ +DELETE FROM "grafana"."team_member" +WHERE org_id = 1 + AND team_id = 1 diff --git a/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_team_members_by_team-delete_team_members_by_team.sql b/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_team_members_by_team-delete_team_members_by_team.sql new file mode 100755 index 0000000000000..93ee87d8faa15 --- /dev/null +++ b/pkg/registry/apis/iam/legacy/testdata/sqlite--delete_team_members_by_team-delete_team_members_by_team.sql @@ -0,0 +1,3 @@ +DELETE FROM "grafana"."team_member" +WHERE org_id = 1 + AND team_id = 1 diff --git a/pkg/tests/apis/iam/team/team_integration_test.go b/pkg/tests/apis/iam/team/team_integration_test.go index b1fe977d70ab5..2698166cc72dd 100644 --- a/pkg/tests/apis/iam/team/team_integration_test.go +++ b/pkg/tests/apis/iam/team/team_integration_test.go @@ -25,7 +25,7 @@ import ( func TestIntegrationTeams(t *testing.T) { testutil.SkipIntegrationTestInShortMode(t) - modes := []rest.DualWriterMode{rest.Mode0, rest.Mode1, rest.Mode5} + modes := []rest.DualWriterMode{rest.Mode0, rest.Mode1, rest.Mode3, rest.Mode5} for _, mode := range modes { t.Run(fmt.Sprintf("Team CRUD operations with dual writer mode %d", mode), func(t *testing.T) { helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ @@ -47,9 +47,12 @@ func TestIntegrationTeams(t *testing.T) { doTeamSpecMembersTests(t, helper) doTeamSpecExternalGroupsOSSTests(t, helper) - if mode < 3 { + if mode < rest.Mode3 { doTeamCRUDTestsUsingTheLegacyAPIs(t, helper, mode) } + if mode < rest.Mode4 { + doTeamDeleteCascadesLegacyMembersTest(t, helper) + } }) } } @@ -285,6 +288,70 @@ func doTeamCRUDTestsUsingTheNewAPIs(t *testing.T, helper *apis.K8sTestHelper) { }) } +// doTeamDeleteCascadesLegacyMembersTest verifies that deleting a team cleans up +// its legacy team_member rows. Only meaningful when legacy storage is written +// (dual-write mode < 4), so the caller gates on mode. +func doTeamDeleteCascadesLegacyMembersTest(t *testing.T, helper *apis.K8sTestHelper) { + t.Run("delete team cascades legacy team_member rows", func(t *testing.T) { + ctx := context.Background() + env := helper.GetEnv() + + teamClient := helper.GetResourceClient(apis.ResourceClientArgs{ + User: helper.Org1.Admin, + Namespace: helper.Namespacer(helper.Org1.Admin.Identity.GetOrgID()), + GVR: gvrTeams, + }) + + editorUID := helper.Org1.Editor.Identity.GetIdentifier() + viewerUID := helper.Org1.Viewer.Identity.GetIdentifier() + + created, err := teamClient.Resource.Create(ctx, &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "iam.grafana.app/v0alpha1", + "kind": "Team", + "metadata": map[string]interface{}{"generateName": "team-del-cascade-"}, + "spec": map[string]interface{}{ + "title": "Team del cascade", + "email": "del-cascade@example.com", + "provisioned": false, + "externalUID": "", + "members": []map[string]interface{}{ + {"kind": "User", "name": editorUID, "permission": "member", "external": false}, + {"kind": "User", "name": viewerUID, "permission": "admin", "external": false}, + }, + }, + }}, metav1.CreateOptions{}) + require.NoError(t, err) + teamUID := created.GetName() + + // Resolve the team's legacy int64 id before deletion. Orphaned + // team_member rows keep this team_id even though the team row is gone, + // so we must capture it now and count by id rather than joining back + // to the team table. + var teamID int64 + rows, err := env.SQLStore.GetSqlxSession().Query(ctx, "SELECT id FROM team WHERE uid = ?", teamUID) + require.NoError(t, err) + require.True(t, rows.Next()) + require.NoError(t, rows.Scan(&teamID)) + require.NoError(t, rows.Close()) + + countMembers := func() int { + var n int + r, err := env.SQLStore.GetSqlxSession().Query(ctx, "SELECT COUNT(*) FROM team_member WHERE team_id = ?", teamID) + require.NoError(t, err) + require.True(t, r.Next()) + require.NoError(t, r.Scan(&n)) + require.NoError(t, r.Close()) + return n + } + + require.Equal(t, 2, countMembers(), "expected legacy team_member rows to exist before delete") + + require.NoError(t, teamClient.Resource.Delete(ctx, teamUID, metav1.DeleteOptions{})) + + require.Equal(t, 0, countMembers(), "team_member rows must be cleaned up after team delete") + }) +} + func doTeamCRUDTestsUsingTheLegacyAPIs(t *testing.T, helper *apis.K8sTestHelper, mode rest.DualWriterMode) { t.Run("should create team using legacy APIs and get/update/delete it using the new APIs", func(t *testing.T) { ctx := context.Background()