Authorization / Access control

PoC: Demonstrate the pre-fix vulnerability by issuing a BatchCheck containing both a valid item and an invalid per-object relation, which previously could error the batch and drop unrelated results. Example scenario: 1) A valid dashboard GET check is included (e.g., dashboard:dash-1, verb GET). 2) An invalid per-object create on a user resource is included (e.g., user:create for user:1). Before the fix, the invalid relation could cause the entire batch to error and poison the other checks. After the fix, the response should show the first item allowed (dashboard GET) and the second item denied (user:create), with no batch-wide error. POC request (pseudo-structure): send a BatchCheck with items [ {name:dash-get, group:dashboard, resource:dashboard, verb:GET, target:dash-1}, {name:user-create, group:iam, resource:user, verb:CREATE, target:user:1} ]; expect results: dash-get: allowed=true, user-create: allowed=false, and no overall batch error. This demonstrates the bug before the fix (batch poisoning) and the corrected behavior after the fix.

diff --git a/pkg/services/authz/zanzana/common/info.go b/pkg/services/authz/zanzana/common/info.go index a71004b2613c5..2d8a749fa6c3c 100644 --- a/pkg/services/authz/zanzana/common/info.go +++ b/pkg/services/authz/zanzana/common/info.go @@ -22,22 +22,22 @@ var typedResources = map[string]typeInfo{ folders.FolderResourceInfo.GroupResource().Group, folders.FolderResourceInfo.GroupResource().Resource, "", - ): {Type: "folder", Relations: RelationsTyped}, + ): {Type: "folder", Relations: RelationsFolder}, FormatGroupResource( iamv0alpha1.TeamResourceInfo.GroupResource().Group, iamv0alpha1.TeamResourceInfo.GroupResource().Resource, "", - ): {Type: "team", Relations: RelationsTyped}, + ): {Type: "team", Relations: RelationsTeam}, FormatGroupResource( iamv0alpha1.UserResourceInfo.GroupResource().Group, iamv0alpha1.UserResourceInfo.GroupResource().Resource, "", - ): {Type: "user", Relations: RelationsTyped}, + ): {Type: "user", Relations: RelationsUser}, FormatGroupResource( iamv0alpha1.ServiceAccountResourceInfo.GroupResource().Group, iamv0alpha1.ServiceAccountResourceInfo.GroupResource().Resource, "", - ): {Type: "service-account", Relations: RelationsTyped}, + ): {Type: "service-account", Relations: RelationsServiceAccount}, } func getTypeInfo(group, resource string) (typeInfo, bool) { diff --git a/pkg/services/authz/zanzana/common/info_test.go b/pkg/services/authz/zanzana/common/info_test.go index c78e12b107abb..a794ff2191fc6 100644 --- a/pkg/services/authz/zanzana/common/info_test.go +++ b/pkg/services/authz/zanzana/common/info_test.go @@ -4,9 +4,11 @@ import ( "testing" authzv1 "github.com/grafana/authlib/authz/proto/v1" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" folders "github.com/grafana/grafana/apps/folder/pkg/apis/folder/v1" + iamv0alpha1 "github.com/grafana/grafana/apps/iam/pkg/apis/iam/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/services/accesscontrol" ) @@ -27,6 +29,99 @@ func TestNewResourceInfoFromCheck_FolderCreateUnderParentUsesParentForPermission require.Equal(t, NewTypedIdent(TypeFolder, parentUID), info.ResourceIdent()) } +// TestResourceInfoIsValidRelation_TypedResources locks the per-type relation sets to the +// OpenFGA model. The List/Check/BatchCheck paths gate their per-object ListObjects/Check on +// IsValidRelation, so an inaccurate set makes the server issue a relation OpenFGA rejects +// (e.g. user#create). The `false` rows are the ones that matter. +func TestResourceInfoIsValidRelation_TypedResources(t *testing.T) { + listReq := func(group, resource string) *authzv1.ListRequest { + return &authzv1.ListRequest{Group: group, Resource: resource} + } + + type relCase struct { + relation string + valid bool + } + + tests := []struct { + name string + group string + resource string + cases []relCase + }{ + { + name: "team", + group: iamv0alpha1.TeamResourceInfo.GroupResource().Group, + resource: iamv0alpha1.TeamResourceInfo.GroupResource().Resource, + cases: []relCase{ + {RelationGet, true}, + {RelationCreate, true}, + {RelationUpdate, true}, + {RelationDelete, true}, + {RelationGetPermissions, true}, + {RelationSetPermissions, true}, + {RelationSubresourceGet, true}, + {RelationSubresourceCreate, true}, + {RelationSubresourceGetPermissions, false}, + {RelationSubresourceSetPermissions, false}, + }, + }, + { + name: "user", + group: iamv0alpha1.UserResourceInfo.GroupResource().Group, + resource: iamv0alpha1.UserResourceInfo.GroupResource().Resource, + cases: []relCase{ + {RelationGet, true}, + {RelationCreate, false}, + {RelationUpdate, true}, + {RelationDelete, true}, + {RelationGetPermissions, true}, + {RelationSetPermissions, true}, + {RelationSubresourceGet, true}, + {RelationSubresourceGetPermissions, false}, + }, + }, + { + name: "service-account", + group: iamv0alpha1.ServiceAccountResourceInfo.GroupResource().Group, + resource: iamv0alpha1.ServiceAccountResourceInfo.GroupResource().Resource, + cases: []relCase{ + {RelationGet, true}, + {RelationCreate, false}, + {RelationUpdate, true}, + {RelationDelete, true}, + {RelationGetPermissions, false}, + {RelationSetPermissions, false}, + {RelationSubresourceGet, true}, + {RelationSubresourceGetPermissions, false}, + }, + }, + { + name: "folder", + group: folders.FolderResourceInfo.GroupResource().Group, + resource: folders.FolderResourceInfo.GroupResource().Resource, + cases: []relCase{ + {RelationGet, true}, + {RelationCreate, true}, + {RelationGetPermissions, true}, + {RelationSubresourceGetPermissions, true}, + {RelationSubresourceSetPermissions, true}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + info := NewResourceInfoFromList(listReq(tt.group, tt.resource)) + require.Equal(t, tt.name, info.Type()) + for _, c := range tt.cases { + assert.Equalf(t, c.valid, info.IsValidRelation(c.relation), + "%s: relation %q expected valid=%v", tt.name, c.relation, c.valid) + } + }) + } +} + func TestNewResourceInfoFromCheck_FolderCreateAtRootUsesGeneral(t *testing.T) { r := &authzv1.CheckRequest{ Verb: utils.VerbCreate, diff --git a/pkg/services/authz/zanzana/common/tuple.go b/pkg/services/authz/zanzana/common/tuple.go index bd41582c15684..ed2e738e12af6 100644 --- a/pkg/services/authz/zanzana/common/tuple.go +++ b/pkg/services/authz/zanzana/common/tuple.go @@ -120,8 +120,9 @@ var RelationsSubresource = []string{ RelationSubresourceSetPermissions, } -// RelationsTyped are relations that can be added to typed resources (folders, teams, users, etc). -var RelationsTyped = append( +// RelationsFolder are the relations valid on type "folder" (schema_folder.fga). Folders are the +// one typed object with a full per-object relation set; the flat IAM types use the vars below. +var RelationsFolder = append( RelationsSubresource, RelationGet, RelationUpdate, @@ -131,6 +132,44 @@ var RelationsTyped = append( RelationSetPermissions, ) +// RelationsSubresourceTyped are the subresource relations valid on the flat IAM types. +// Unlike folders, these types have no resource_get_permissions / resource_set_permissions. +var RelationsSubresourceTyped = []string{ + RelationSubresourceGet, + RelationSubresourceUpdate, + RelationSubresourceCreate, + RelationSubresourceDelete, +} + +// RelationsTeam are the relations valid on type "team". Teams keep a per-object `create` +// (granted to admins), unlike user / service-account. +var RelationsTeam = append(append([]string{}, RelationsSubresourceTyped...), + RelationGet, + RelationCreate, + RelationUpdate, + RelationDelete, + RelationGetPermissions, + RelationSetPermissions, +) + +// RelationsUser are the relations valid on type "user": no per-object `create` +// (governed by the group_resource), but get_permissions / set_permissions exist. +var RelationsUser = append(append([]string{}, RelationsSubresourceTyped...), + RelationGet, + RelationUpdate, + RelationDelete, + RelationGetPermissions, + RelationSetPermissions, +) + +// RelationsServiceAccount are the relations valid on type "service-account": +// no per-object `create`, and no get_permissions / set_permissions. +var RelationsServiceAccount = append(append([]string{}, RelationsSubresourceTyped...), + RelationGet, + RelationUpdate, + RelationDelete, +) + // VerbMapping is mapping a k8s verb to a zanzana relation. var VerbMapping = map[string]string{ utils.VerbGet: RelationGet, diff --git a/pkg/services/authz/zanzana/server/server_batch_check_test.go b/pkg/services/authz/zanzana/server/server_batch_check_test.go index 989773e07f8d7..b15fd6181e102 100644 --- a/pkg/services/authz/zanzana/server/server_batch_check_test.go +++ b/pkg/services/authz/zanzana/server/server_batch_check_test.go @@ -319,6 +319,81 @@ func TestIntegrationServerBatchCheck(t *testing.T) { assert.False(t, res.GetResults()["sa"].GetAllowed()) }) + // Typed `create` batch checks. user / service-account have no per-object `create`, so an + // unresolved create item must resolve to denied rather than error the whole batch. + t.Run("create on user/service-account is denied, not errored", func(t *testing.T) { + items := []*authzv1.BatchCheckItem{ + newItem("user-create", utils.VerbCreate, userGroup, userResource, "", "", "someuser"), + newItem("sa-create", utils.VerbCreate, serviceAccountGroup, serviceAccountResource, "", "", "some-sa"), + } + res, err := server.BatchCheck(newContextWithNamespace(), newBatchReq("user:1", items)) + require.NoError(t, err) + require.Len(t, res.GetResults(), 2) + assert.False(t, res.GetResults()["user-create"].GetAllowed()) + assert.False(t, res.GetResults()["sa-create"].GetAllowed()) + }) + + t.Run("invalid create item does not poison other checks in the batch", func(t *testing.T) { + // The original symptom: one such create item used to error the entire batch, dropping + // unrelated valid results like the dashboard check below. + items := []*authzv1.BatchCheckItem{ + newItem("dash", utils.VerbGet, dashboardGroup, dashboardResource, "", "1", "1"), // user:1 has access + newItem("user-create", utils.VerbCreate, userGroup, userResource, "", "", "someuser"), // invalid per-object relation + } + res, err := server.BatchCheck(newContextWithNamespace(), newBatchReq("user:1", items)) + require.NoError(t, err) + require.Len(t, res.GetResults(), 2) + assert.True(t, res.GetResults()["dash"].GetAllowed()) + assert.False(t, res.GetResults()["user-create"].GetAllowed()) + }) + + t.Run("user:20 create on users is allowed via group_resource", func(t *testing.T) { + items := []*authzv1.BatchCheckItem{ + newItem("user-create", utils.VerbCreate, userGroup, userResource, "", "", ""), + } + res, err := server.BatchCheck(newContextWithNamespace(), newBatchReq("user:20", items)) + require.NoError(t, err) + require.Len(t, res.GetResults(), 1) + assert.True(t, res.GetResults()["user-create"].GetAllowed()) + }) + + t.Run("user:21 (team admin) create on their team is allowed via per-object team create", func(t *testing.T) { + items := []*authzv1.BatchCheckItem{ + newItem("team-create", utils.VerbCreate, teamGroup, teamResource, "", "", "admin-team"), + } + res, err := server.BatchCheck(newContextWithNamespace(), newBatchReq("user:21", items)) + require.NoError(t, err) + require.Len(t, res.GetResults(), 1) + assert.True(t, res.GetResults()["team-create"].GetAllowed()) + }) + + t.Run("subresource create on a user is allowed via resource_create", func(t *testing.T) { + // user has no base `create`, but the subresource branch is gated independently on + // resource_create, so a subresource create grant still resolves allowed. + items := []*authzv1.BatchCheckItem{ + newItem("user-sub-create", utils.VerbCreate, userGroup, userResource, statusSubresource, "", "1"), + } + res, err := server.BatchCheck(newContextWithNamespace(), newBatchReq("user:22", items)) + require.NoError(t, err) + require.Len(t, res.GetResults(), 1) + assert.True(t, res.GetResults()["user-sub-create"].GetAllowed()) + }) + + t.Run("subresource get/set_permissions on user/team is denied, not errored", func(t *testing.T) { + // user/team have no subresource get/set_permissions in the model (folder-only). + // resolveTypedItems gates the subresource branch on the subresource relation, so these + // resolve to denied rather than erroring the batch on an undefined relation. + items := []*authzv1.BatchCheckItem{ + newItem("user-sub-getperm", utils.VerbGetPermissions, userGroup, userResource, statusSubresource, "", "1"), + newItem("team-sub-setperm", utils.VerbSetPermissions, teamGroup, teamResource, statusSubresource, "", "1"), + } + res, err := server.BatchCheck(newContextWithNamespace(), newBatchReq("user:1", items)) + require.NoError(t, err) + require.Len(t, res.GetResults(), 2) + assert.False(t, res.GetResults()["user-sub-getperm"].GetAllowed()) + assert.False(t, res.GetResults()["team-sub-setperm"].GetAllowed()) + }) + t.Run("folder subresource access via set_edit", func(t *testing.T) { // user:5 has set_edit on dashboards in folder 1, which grants get/update/create/delete items := []*authzv1.BatchCheckItem{ diff --git a/pkg/services/authz/zanzana/server/server_check.go b/pkg/services/authz/zanzana/server/server_check.go index 3bbb52f17166b..61fe8bb8c061e 100644 --- a/pkg/services/authz/zanzana/server/server_check.go +++ b/pkg/services/authz/zanzana/server/server_check.go @@ -120,17 +120,16 @@ func (s *Server) checkTyped(ctx context.Context, subject, relation string, resou ctx, span := s.tracer.Start(ctx, "server.checkTyped") defer span.End() - if !resource.IsValidRelation(relation) { - return &authzv1.CheckResponse{Allowed: false}, nil - } - var ( resourceIdent = resource.ResourceIdent() resourceCtx = resource.Context() subresourceRelation = common.SubresourceRelation(relation) ) - if resource.HasSubresource() { + // The subresource branch is gated on the subresource relation, independently of the base + // relation: a type may support e.g. resource_create without a per-object base create + // (user / service-account), so the base-relation guard below must not block it. + if resource.HasSubresource() && resource.IsValidRelation(subresourceRelation) { // Check if subject has access as a subresource res, err := s.openfgaCheck(ctx, store, subject, common.SubresourcePermissionRelation(subresourceRelation), resourceIdent, contextuals, resourceCtx) if err != nil { @@ -142,7 +141,7 @@ func (s *Server) checkTyped(ctx context.Context, subject, relation string, resou } } - if resourceIdent == "" { + if !resource.IsValidRelation(relation) || resourceIdent == "" { return &authzv1.CheckResponse{Allowed: false}, nil } diff --git a/pkg/services/authz/zanzana/server/server_check_test.go b/pkg/services/authz/zanzana/server/server_check_test.go index 6828fefdb6e2e..fe9f697f2d17f 100644 --- a/pkg/services/authz/zanzana/server/server_check_test.go +++ b/pkg/services/authz/zanzana/server/server_check_test.go @@ -265,4 +265,67 @@ func TestIntegrationServerCheck(t *testing.T) { require.NoError(t, err) assert.True(t, res.GetAllowed()) }) + + // Typed `create` checks. user / service-account have no per-object `create`, so a named + // create check must resolve to denied rather than issue an invalid Check that errors. + t.Run("named create check on user without access is denied, not errored", func(t *testing.T) { + res, err := server.Check(newContextWithNamespace(), newReq("user:1", utils.VerbCreate, userGroup, userResource, "", "", "someuser")) + require.NoError(t, err) + assert.False(t, res.GetAllowed()) + }) + + t.Run("named create check on service-account without access is d ... [truncated]