Authorization / Access control correctness

PoC (illustrative; shows the pre-fix exploitation path vs post-fix behavior): Pre-fix scenario (vulnerable behavior): - An authenticated user with permissions to list users and read user roles triggers a user search API that internally runs a batch of access-control checks for various verbs, including ones not defined on the authz model's user type (e.g., org.users:add, users.permissions:read). - The authz engine encounters a check mapped to a relation not defined for the user type and returns an error (e.g., "relation 'user#create' not found"), which blanks out all AccessControl metadata for the search results. - As a result, the client receives either an error or a response with missing/blank AccessControl entries, potentially leaking information about what was attempted or denying valid results. Proof-of-concept steps (pseudo-code / curl): 1) Prepare a batch-check request that includes an unsupported verb for the user type: curl -X POST https://grafana.example/api/iam/batch_check \ -H "Authorization: Bearer <token>" \ -H "Content-Type: application/json" \ -d '{ "checks": [ {"action":"org.users:add","group":"grafana","resource":"users","verb":"CREATE"}, {"action":"org.users:read","group":"grafana","resource":"users","verb":"LIST"}, {"action":"users.permissions:read","group":"grafana","resource":"users","verb":"GET_PERMISSIONS"} ] }' 2) Observe the response: - Likely an error or a response where AccessControl metadata is blank/missing for all hits due to the unsupported verb being evaluated in the batch. 3) Post-fix behavior (after this commit in 12.4.0): - The batch only includes verbs defined on the authz model's user type (get/update/delete). The unsupported checks (org.users:add, users.permissions:read) are omitted, so the batch completes and AccessControl metadata is populated correctly for results the user is allowed to view. - No blanking of metadata occurs, and legitimate access decisions are enforced as expected. Note: The test TestUserAccessControlChecksUseSupportedVerbs in the patch ensures that only supported verbs are included for the user resource, preventing this class of issue from reoccurring.

diff --git a/pkg/registry/apis/iam/user/search.go b/pkg/registry/apis/iam/user/search.go index 4f8b5a499bc65..213de2e2b996d 100644 --- a/pkg/registry/apis/iam/user/search.go +++ b/pkg/registry/apis/iam/user/search.go @@ -49,12 +49,15 @@ type accessControlCheck struct { name string // user UID of the resource being checked } +// Only verbs whose relation is defined on the authz model's "user" type may be +// checked here. A check for a relation the type lacks fails the whole batch +// (e.g. "relation 'user#create' not found"), blanking out all access control +// metadata. The "user" type defines only get/update/delete, so VerbCreate +// (org.users:add) and VerbGetPermissions (users.permissions:read) are omitted. var userAccessControlChecks = []accessControlCheck{ {action: "org.users:read", group: iamv0.GROUP, resource: "users", verb: utils.VerbList}, - {action: "org.users:add", group: iamv0.GROUP, resource: "users", verb: utils.VerbCreate}, {action: "org.users:remove", group: iamv0.GROUP, resource: "users", verb: utils.VerbDelete}, {action: "org.users:write", group: iamv0.GROUP, resource: "users", verb: utils.VerbUpdate}, - {action: "users.permissions:read", group: iamv0.GROUP, resource: "users", verb: utils.VerbGetPermissions}, {action: "users.roles:read", group: iamv0.GROUP, resource: "rolebindings", verb: utils.VerbList}, } diff --git a/pkg/registry/apis/iam/user/search_test.go b/pkg/registry/apis/iam/user/search_test.go index f208b239cadc9..4120792db28e7 100644 --- a/pkg/registry/apis/iam/user/search_test.go +++ b/pkg/registry/apis/iam/user/search_test.go @@ -16,6 +16,7 @@ import ( iamv0 "github.com/grafana/grafana/apps/iam/pkg/apis/iam/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/apiserver/rest" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/featuremgmt" @@ -227,13 +228,34 @@ func TestSearchSort(t *testing.T) { } } +// The authz model's "user" type defines only get/update/delete relations +// (schema_core.fga). A check whose verb maps to any other relation fails the +// whole batch check at the authz server, blanking out all metadata, so guard +// against re-adding one for the "users" resource. +func TestUserAccessControlChecksUseSupportedVerbs(t *testing.T) { + supportedUserVerbs := map[string]bool{ + utils.VerbGet: true, + utils.VerbList: true, + utils.VerbWatch: true, + utils.VerbUpdate: true, + utils.VerbPatch: true, + utils.VerbDelete: true, + } + for _, c := range userAccessControlChecks { + if c.resource != "users" { + continue + } + assert.True(t, supportedUserVerbs[c.verb], + "check %q uses verb %q whose relation is not defined on the authz \"user\" type", c.action, c.verb) + } +} + func TestAccessControl(t *testing.T) { partialClient := &mockAccessClient{ batchCheckFunc: func(_ context.Context, _ authlib.AuthInfo, req authlib.BatchCheckRequest) (authlib.BatchCheckResponse, error) { allowed := map[string]bool{ - "org.users:read": true, - "users.permissions:read": true, - "users.roles:read": true, + "org.users:read": true, + "users.roles:read": true, } results := make(map[string]authlib.BatchCheckResult, len(req.Checks)) for _, check := range req.Checks { @@ -335,9 +357,7 @@ func TestAccessControl(t *testing.T) { for _, hit := range hits { require.NotNil(t, hit.AccessControl) assert.True(t, hit.AccessControl["org.users:read"]) - assert.True(t, hit.AccessControl["users.permissions:read"]) assert.True(t, hit.AccessControl["users.roles:read"]) - assert.False(t, hit.AccessControl["org.users:add"]) assert.False(t, hit.AccessControl["org.users:remove"]) assert.False(t, hit.AccessControl["org.users:write"]) }