Authorization bypass / RBAC misconfiguration

PoC (proof-of-concept) for potential pre-fix exploitation): - Context: An editor (non-admin) with permission to create jobs could abuse the refs subresource gating to trigger admin provisioning paths, effectively bypassing the intended admin-only controls if the authorization flow borrows the editor’s jobs:create permission as a surrogate for admin access. - Scenario: A Grafana deployment (12.x line) with a user having Editor rights but not Repository write/Admin rights attempts to perform a repository refs operation that could spawn provisioning actions. - Prereqs: An existing repository (e.g., default/my-repo) and a user token that has Editor role with jobs:create permission but without repositories:write admin rights. - Exploit steps (before fix, illustrative): 1) Use an Editor token to call the provisioning API for the repository refs subresource (e.g., refs on repositories/default/my-repo). 2) The request is accepted due to the prior gating logic that allowed Editor via the Jobs resource as a proxy, potentially triggering provisioning actions that should be admin-only. 3) Observe that provisioning resources could be modified or created without admin-level repository access. - Example (curl, placeholder URL): curl -i -X POST 'https://grafana.example.com/api/provisioning/v0alpha1/repositories/default/my-repo/refs' \ -H 'Authorization: Bearer <editor-token>' \ -H 'Content-Type: application/json' \ -d '{"ref":"refs/heads/feature-branch","action":"update"}' - Expected outcome (pre-fix): 200/201 meaning the operation is allowed despite not having admin-level repository permissions. - After the fix: This path enforces explicit admin permission on the repository resource (repositories:write) or the editor path must be satisfied via a valid jobs:create on the appropriate resource, reducing the risk of privilege escalation. Notes: The PoC is highly dependent on exact API surface paths and payload schemas in Grafana’s provisioning API. The intent is to illustrate how an improper OR-gating between admin (repositories:write) and editor (jobs:create) checks could allow inappropriate access via the refs subresource when used with an editor account. The actual exploitation would require a live environment exposing these endpoints and tokens with the described roles.

diff --git a/pkg/apimachinery/identity/context.go b/pkg/apimachinery/identity/context.go index 2298c58de29e..8f4a34819fe2 100644 --- a/pkg/apimachinery/identity/context.go +++ b/pkg/apimachinery/identity/context.go @@ -195,10 +195,7 @@ var serviceIdentityTokenPermissions = []string{ "query.grafana.app:*", "datasource.grafana.app:*", "iam.grafana.app:*", - "provisioning.grafana.app/repositories:read", - "provisioning.grafana.app/repositories:watch", - "provisioning.grafana.app/settings:read", - "provisioning.grafana.app/settings:watch", + "provisioning.grafana.app:*", "preferences.grafana.app:*", // user, team, and org preferences "collections.grafana.app:*", // user stars "plugins.grafana.app:*", diff --git a/pkg/registry/apis/provisioning/jobs.go b/pkg/registry/apis/provisioning/jobs.go index 57035a3d9e15..ac2d149770ab 100644 --- a/pkg/registry/apis/provisioning/jobs.go +++ b/pkg/registry/apis/provisioning/jobs.go @@ -382,11 +382,16 @@ func (c *jobsConnector) authorizeResourceRefs(ctx context.Context, authorizer re // authorizeAdminJob checks that the requesting user has admin privileges. // Used for job types that are restricted to administrators. +// +// We check repositories:write (an admin-only RBAC action) rather than +// jobs:create, because jobs:create is granted to Editor and would let editors +// trigger admin-restricted jobs (pull, releaseResources, deleteResources). +// The fallback role still allows admins whose RBAC isn't explicitly set up. func (c *jobsConnector) authorizeAdminJob(ctx context.Context, cfg *provisioning.Repository) error { return c.access.WithFallbackRole(identity.RoleAdmin).Check(ctx, authlib.CheckRequest{ - Verb: "create", + Verb: utils.VerbUpdate, Group: provisioning.GROUP, - Resource: provisioning.JobResourceInfo.GetName(), + Resource: provisioning.RepositoryResourceInfo.GetName(), Namespace: cfg.Namespace, }, "") } diff --git a/pkg/registry/apis/provisioning/jobs_test.go b/pkg/registry/apis/provisioning/jobs_test.go index c5bfc18864f3..9d72d0d2460c 100644 --- a/pkg/registry/apis/provisioning/jobs_test.go +++ b/pkg/registry/apis/provisioning/jobs_test.go @@ -978,9 +978,9 @@ func TestAuthorizeAdminJob(t *testing.T) { t.Run("admin is authorized", func(t *testing.T) { adminChecker := auth.NewMockAccessChecker(t) adminChecker.EXPECT().Check(mock.Anything, mock.MatchedBy(func(req authlib.CheckRequest) bool { - return req.Verb == "create" && + return req.Verb == utils.VerbUpdate && req.Group == provisioning.GROUP && - req.Resource == provisioning.JobResourceInfo.GetName() && + req.Resource == provisioning.RepositoryResourceInfo.GetName() && req.Namespace == cfg.Namespace }), "").Return(nil) @@ -995,9 +995,9 @@ func TestAuthorizeAdminJob(t *testing.T) { t.Run("non-admin is forbidden", func(t *testing.T) { adminChecker := auth.NewMockAccessChecker(t) adminChecker.EXPECT().Check(mock.Anything, mock.MatchedBy(func(req authlib.CheckRequest) bool { - return req.Verb == "create" && + return req.Verb == utils.VerbUpdate && req.Group == provisioning.GROUP && - req.Resource == provisioning.JobResourceInfo.GetName() + req.Resource == provisioning.RepositoryResourceInfo.GetName() }), "").Return(apierrors.NewForbidden(schema.GroupResource{}, "", fmt.Errorf("admin role is required"))) accessMock := auth.NewMockAccessChecker(t) diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 4db3863e5e44..26194acd6438 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -544,20 +544,48 @@ func (b *APIBuilder) authorizeRepositorySubresource(ctx context.Context, a autho case "files": return authorizer.DecisionAllow, "", nil - // refs subresource - editors need to see branches to push changes + // refs subresource - lists the branches/commits of the repository. + // + // Two distinct flows legitimately need refs, and the repositories resource has no + // Editor tier to express both (repositories:read is granted to Viewer, write/create/ + // delete to Admin), so we authorize with an OR of two semantically-correct checks: + // 1. Admins / repository owners setting up or editing a repository pick a target + // branch -> repositories:write (VerbUpdate), scoped to this repository. + // 2. Editors saving changes or opening a PR via the files/push-job flow need the + // branch list -> jobs:create (VerbCreate), namespace-scoped (as authorizeAdminJob). + // Viewers satisfy neither and are denied, which is the intended tightening. Each check + // uses its own correct resource, so this stays coherent under future per-repository + // scoping (unlike borrowing the jobs resource with the repository name). case "refs": - return toAuthorizerDecision(b.accessWithEditor.Check(ctx, authlib.CheckRequest{ - Verb: apiutils.VerbGet, + if err := b.accessWithAdmin.Check(ctx, authlib.CheckRequest{ + Verb: apiutils.VerbUpdate, Group: provisioning.GROUP, Resource: provisioning.RepositoryResourceInfo.GetName(), Name: a.GetName(), Namespace: a.GetNamespace(), + }, ""); err == nil { + return authorizer.DecisionAllow, "", nil + } + return toAuthorizerDecision(b.accessWithEditor.Check(ctx, authlib.CheckRequest{ + Verb: apiutils.VerbCreate, + Group: provisioning.GROUP, + Resource: provisioning.JobResourceInfo.GetName(), + Namespace: a.GetNamespace(), }, "")) - // Read-only subresources: resources, history, status (admin only) + // Read-only subresources: resources, history, status (admin only). + // + // These expose repository management/inspection views and must be admin-only. We gate + // on repositories:write (VerbUpdate) - an admin-only action - rather than + // repositories:read, which is granted to Viewer and would expose these views to + // viewers/editors. There is no admin-only *read* action on the repositories resource, + // so write is the honest gate ("you can inspect a repository's management views if you + // can manage it"). We keep the repositories resource with the repository Name rather + // than proxying through an unrelated resource (e.g. stats), so this remains correct if + // access is later scoped to individual repositories. case "resources", "history", "status": return toAuthorizerDecision(b.accessWithAdmin.Check(ctx, authlib.CheckRequest{ - Verb: apiutils.VerbGet, + Verb: apiutils.VerbUpdate, Group: provisioning.GROUP, Resource: provisioning.RepositoryResourceInfo.GetName(), Name: a.GetName(), diff --git a/pkg/registry/apis/provisioning/register_authz_test.go b/pkg/registry/apis/provisioning/register_authz_test.go new file mode 100644 index 000000000000..2060c7e5826d --- /dev/null +++ b/pkg/registry/apis/provisioning/register_authz_test.go @@ -0,0 +1,115 @@ +package provisioning + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/authorization/authorizer" + + authlib "github.com/grafana/authlib/types" + "github.com/grafana/grafana/apps/provisioning/pkg/apis/auth" + provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/utils" +) + +func mockForbidden() error { + return apierrors.NewForbidden(schema.GroupResource{}, "", fmt.Errorf("forbidden")) +} + +func subresourceAttrs(sub, name, ns string) authorizer.Attributes { + return authorizer.AttributesRecord{ + Subresource: sub, + Name: name, + Namespace: ns, + Verb: "get", + } +} + +// TestAuthorizeRepositorySubresource verifies the refs and admin-view subresource gates. +// +// refs must be available to editors (push/PR flow) and to repository managers/admins +// (repo setup), but NOT to viewers. Since the repositories resource has no Editor tier, +// it is authorized as repositories:write (admin) OR jobs:create (editor). +// +// resources/history/status are admin-only and gated on repositories:write so that the +// repository-scoped Name is preserved (rather than proxying through an unrelated resource). +func TestAuthorizeRepositorySubresource(t *testing.T) { + ctx := context.Background() + const ns, name = "default", "my-repo" + + repoWriteReq := func(req authlib.CheckRequest) bool { + return req.Verb == utils.VerbUpdate && + req.Group == provisioning.GROUP && + req.Resource == provisioning.RepositoryResourceInfo.GetName() && + req.Name == name && + req.Namespace == ns + } + jobsCreateReq := func(req authlib.CheckRequest) bool { + return req.Verb == utils.VerbCreate && + req.Group == provisioning.GROUP && + req.Resource == provisioning.JobResourceInfo.GetName() && + req.Namespace == ns + } + + t.Run("refs: repository writer (admin) is allowed without consulting jobs", func(t *testing.T) { + adminChecker := auth.NewMockAccessChecker(t) + adminChecker.EXPECT().Check(mock.Anything, mock.MatchedBy(repoWriteReq), "").Return(nil) + + b := &APIBuilder{accessWithAdmin: adminChecker, accessWithEditor: auth.NewMockAccessChecker(t)} + decision, _, err := b.authorizeRepositorySubresource(ctx, subresourceAttrs("refs", name, ns)) + require.NoError(t, err) + assert.Equal(t, authorizer.DecisionAllow, decision) + }) + + t.Run("refs: editor who can create jobs is allowed", func(t *testing.T) { + adminChecker := auth.NewMockAccessChecker(t) + adminChecker.EXPECT().Check(mock.Anything, mock.MatchedBy(repoWriteReq), "").Return(mockForbidden()) + editorChecker := auth.NewMockAccessChecker(t) + editorChecker.EXPECT().Check(mock.Anything, mock.MatchedBy(jobsCreateReq), "").Return(nil) + + b := &APIBuilder{accessWithAdmin: adminChecker, accessWithEditor: editorChecker} + decision, _, err := b.authorizeRepositorySubresource(ctx, subresourceAttrs("refs", name, ns)) + require.NoError(t, err) + assert.Equal(t, authorizer.DecisionAllow, decision) + }) + + t.Run("refs: viewer (neither repo write nor jobs create) is denied", func(t *testing.T) { + adminChecker := auth.NewMockAccessChecker(t) + adminChecker.EXPECT().Check(mock.Anything, mock.MatchedBy(repoWriteReq), "").Return(mockForbidden()) + editorChecker := auth.NewMockAccessChecker(t) + editorChecker.EXPECT().Check(mock.Anything, mock.MatchedBy(jobsCreateReq), "").Return(mockForbidden()) + + b := &APIBuilder{accessWithAdmin: adminChecker, accessWithEditor: editorChecker} + decision, _, err := b.authorizeRepositorySubresource(ctx, subresourceAttrs("refs", name, ns)) + require.NoError(t, err) + assert.Equal(t, authorizer.DecisionDeny, decision) + }) + + for _, sub := range []string{"resources", "history", "status"} { + t.Run(sub+": admin (repositories:write) is allowed", func(t *testing.T) { + adminChecker := auth.NewMockAccessChecker(t) + adminChecker.EXPECT().Check(mock.Anything, mock.MatchedBy(repoWriteReq), "").Return(nil) + + b := &APIBuilder{accessWithAdmin: adminChecker} + decision, _, err := b.authorizeRepositorySubresource(ctx, subresourceAttrs(sub, name, ns)) + require.NoError(t, err) + assert.Equal(t, authorizer.DecisionAllow, decision) + }) + + t.Run(sub+": non-admin is denied", func(t *testing.T) { + adminChecker := auth.NewMockAccessChecker(t) + adminChecker.EXPECT().Check(mock.Anything, mock.MatchedBy(repoWriteReq), "").Return(mockForbidden()) + + b := &APIBuilder{accessWithAdmin: adminChecker} + decision, _, err := b.authorizeRepositorySubresource(ctx, subresourceAttrs(sub, name, ns)) + require.NoError(t, err) + assert.Equal(t, authorizer.DecisionDeny, decision) + }) + } +} diff --git a/pkg/services/authz/rbac/service_identity_permissions_test.go b/pkg/services/authz/rbac/service_identity_permissions_test.go new file mode 100644 index 000000000000..92977beb18f5 --- /dev/null +++ b/pkg/services/authz/rbac/service_identity_permissions_test.go @@ -0,0 +1,99 @@ +package rbac + +import ( + "fmt" + "strings" + "testing" + + authnlib "github.com/grafana/authlib/authn" + authzlib "github.com/grafana/authlib/authz" + "github.com/grafana/authlib/types" + "github.com/stretchr/testify/assert" + + "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/apimachinery/utils" +) + +// TestServiceIdentityTokenPermissionsCoverMapper ensures that every (group, resource, verb) +// combination defined in the rbac mapper is also covered by the service identity's +// delegated permissions in pkg/apimachinery/identity/context.go. +// +// Why this matters: authlib.CheckServicePermissions runs *before* the user permission +// check on every authz call. For any caller that isn't a TypeAccessPolicy (i.e. real +// users and service accounts going through Grafana's normal auth flow), it evaluates the +// caller's DelegatedPermissions — which AccessClaimsHook hard-codes to +// identity.ServiceIdentityClaims.Rest.DelegatedPermissions whenever there's no upstream +// access token. If the service identity token doesn't cover an action the validator (or +// any other authzlib client) asks for, the request is denied before the user's RBAC +// permissions are even consulted. +func TestServiceIdentityTokenPermissionsCoverMapper(t *testing.T) { + // Simulate the post-AccessClaimsHook state for a normal user/SA: identity is not + // TypeAccessPolicy, so CheckServicePermissions reads DelegatedPermissions. + caller := &identity.StaticRequester{ + Type: types.TypeUser, + AccessTokenClaims: &authnlib.Claims[authnlib.AccessTokenClaims]{ + Rest: authnlib.AccessTokenClaims{ + DelegatedPermissions: identity.ServiceIdentityClaims.Rest.DelegatedPermissions, + }, + }, + } + + // Sanity-check the test setup: a non-TypeAccessPolicy caller should be evaluated + // against DelegatedPermissions, and the token must be non-empty. + probe := authzlib.CheckServicePermissions(caller, "dashboard.grafana.app", "dashboards", utils.VerbGet) + assert.False(t, probe.ServiceCall, "test caller must not be a service call so DelegatedPermissions are evaluated") + assert.NotEmpty(t, probe.Permissions, "service identity delegated permissions must be populated") + + registry, ok := NewMapperRegistry().(mapper) + if !ok { + t.Fatal("NewMapperRegistry did not return the expected mapper type") + } + + // Dedupe (group, apiResource, verb) triples: aliased mappings (e.g. iam.grafana.app + // has both "globalroles" and "roles" pointing at apiResource="roles") would + // otherwise produce duplicate subtests with the same assertion. + type triple struct{ group, resource, verb string } + checks := map[triple]struct{}{} + + for group, resources := range registry { + // Wildcard groups (e.g. "*.datasource.grafana.app") are matched against + // concrete groups at request time. The RolePermissionValidator explicitly + // falls back to ... [truncated]