Access Control / Authorization Bypass (RBAC) via Dashboard Annotations subresource

PoC (conceptual and environment-dependent; demonstrates how an attacker could exploit a pre-fix path): Prereqs: - Grafana RBAC present with resource groups for dashboards (dashboard.grafana.app) - A user token with minimal permissions, e.g., dashboards:view (read access to the dashboard) but without explicit annotations permissions. - Target dashboard with UID: some-dashboard-uid Attack steps (pre-fix behavior): 1) Attacker sends a request to create an annotation on a dashboard via the annotations subresource endpoint (path example may vary by Grafana version): POST https://grafana.example/api/dashboards/some-dashboard-uid/annotations Content-Type: application/json Authorization: Bearer <token> {"annotation": {"text": "test", "time": 1234567890}} 2) Because the old implementation could map to a virtual resource or consolidate checks in a way that did not scope per-action to the subresource, the authorization middleware might incorrectly permit the operation if the attacker possessed general dashboard permissions (e.g., read) or if the batch-check path did not properly enforce per-action billing for annotations. 3) If the request succeeds (HTTP 200/201) and the annotation is persisted, the attacker has successfully created an annotation without explicit per-annotation permission granted, illustrating an authorization bypass. Expected behavior after the fix (what the patch enforces): - Annotations must be accessed via the dashboards/annotations subresource with explicit per-action permissions (e.g., annotations:create for creation, annotations:write for updates, annotations:read for reads, etc.). - The BatchCheck now validates multiple verb checks (dash_read, dash_write, dash_delete, dash_admin, annot_create, annot_update, annot_delete) in a single call and denies if any required permission is missing. - A request to create/update/delete an annotation will be denied unless the user has the corresponding per-annotation permission (annotations:create/update/delete) bound to the specific dashboard UID. This PoC is environment-dependent and assumes the presence of the older, non-subresource permission flow. In Grafana versions containing the fix (12.4.x+), the same request should be rejected with a 403/denied response when the user lacks the appropriate annotations:create permission for the specific dashboard UID.

diff --git a/pkg/registry/apis/dashboard/sub_dto.go b/pkg/registry/apis/dashboard/sub_dto.go index e69ca486d6e5f..feb27af6e7422 100644 --- a/pkg/registry/apis/dashboard/sub_dto.go +++ b/pkg/registry/apis/dashboard/sub_dto.go @@ -139,62 +139,91 @@ func (r *DTOConnector) Connect(ctx context.Context, name string, opts runtime.Ob gvr := dashv1.DashboardResourceInfo.GroupVersionResource() - // Check read permission using authlib.AccessClient - readRes, err := r.accessClient.Check(ctx, authInfo, authlib.CheckRequest{ - Verb: utils.VerbGet, - Group: gvr.Group, - Resource: gvr.Resource, + checkRes, err := r.accessClient.BatchCheck(ctx, authInfo, authlib.BatchCheckRequest{ Namespace: ns, - Name: name, - }, folder) + Checks: []authlib.BatchCheckItem{ + { + CorrelationID: "dash_read", + Verb: utils.VerbGet, + Group: gvr.Group, + Resource: gvr.Resource, + Name: name, + Folder: folder, + }, + { + CorrelationID: "dash_write", + Verb: utils.VerbUpdate, + Group: gvr.Group, + Resource: gvr.Resource, + Name: name, + Folder: folder, + }, + { + CorrelationID: "dash_delete", + Verb: utils.VerbDelete, + Group: gvr.Group, + Resource: gvr.Resource, + Name: name, + Folder: folder, + }, + { + CorrelationID: "dash_admin", + Verb: utils.VerbSetPermissions, + Group: gvr.Group, + Resource: gvr.Resource, + Name: name, + Folder: folder, + }, + { + CorrelationID: "annot_create", + Verb: utils.VerbCreate, + Group: gvr.Group, + Resource: gvr.Resource, + Subresource: "annotations", + Name: name, + Folder: folder, + }, + { + CorrelationID: "annot_update", + Verb: utils.VerbUpdate, + Group: gvr.Group, + Resource: gvr.Resource, + Subresource: "annotations", + Name: name, + Folder: folder, + }, + { + CorrelationID: "annot_delete", + Verb: utils.VerbDelete, + Group: gvr.Group, + Resource: gvr.Resource, + Subresource: "annotations", + Name: name, + Folder: folder, + }, + }, + }) if err != nil { - logger.Warn("Failed to check read permission", "err", err) - responder.Error(fmt.Errorf("not allowed to view")) + logger.Warn("Failed to batch check permissions", "err", err) + responder.Error(fmt.Errorf("failed to check permissions")) return } - if !readRes.Allowed { + + if !checkRes.Results["dash_read"].Allowed { responder.Error(fmt.Errorf("not allowed to view")) return } - // Check write permission - writeRes, err := r.accessClient.Check(ctx, authInfo, authlib.CheckRequest{ - Verb: utils.VerbUpdate, - Group: gvr.Group, - Resource: gvr.Resource, - Namespace: ns, - Name: name, - }, folder) - // Keeping the same logic as with accessControl.Evaluate. - // On errors we default on deny. - if err != nil { - logger.Warn("Failed to check write permission", "err", err) - } - access.CanSave = writeRes.Allowed - access.CanEdit = writeRes.Allowed - - // Check delete permission - deleteRes, err := r.accessClient.Check(ctx, authInfo, authlib.CheckRequest{ - Verb: utils.VerbDelete, - Group: gvr.Group, - Resource: gvr.Resource, - Namespace: ns, - Name: name, - }, folder) - if err != nil { - logger.Warn("Failed to check delete permission", "err", err) - } - access.CanDelete = deleteRes.Allowed - - // For admin permission, use write as a proxy for now - access.CanAdmin = writeRes.Allowed - access.CanStar = user.IsIdentityType(authlib.TypeUser) - - // Annotation permissions - use write permission as proxy - access.AnnotationsPermissions = &dashboard.AnnotationPermission{ - Dashboard: dashboard.AnnotationActions{CanAdd: writeRes.Allowed, CanEdit: writeRes.Allowed, CanDelete: writeRes.Allowed}, - } + access.CanSave = checkRes.Results["dash_write"].Allowed + access.CanEdit = checkRes.Results["dash_write"].Allowed + access.CanDelete = checkRes.Results["dash_delete"].Allowed + access.CanAdmin = checkRes.Results["dash_admin"].Allowed + access.AnnotationsPermissions = &dashboard.AnnotationPermission{Dashboard: dashboard.AnnotationActions{ + CanAdd: checkRes.Results["annot_create"].Allowed, + CanEdit: checkRes.Results["annot_update"].Allowed, + CanDelete: checkRes.Results["annot_delete"].Allowed, + }} title := obj.FindTitle("") access.Slug = slugify.Slugify(title) diff --git a/pkg/registry/apps/annotation/authz.go b/pkg/registry/apps/annotation/authz.go index 8b843846204aa..3ad70fac505bc 100644 --- a/pkg/registry/apps/annotation/authz.go +++ b/pkg/registry/apps/annotation/authz.go @@ -50,14 +50,15 @@ func canAccessAnnotation(ctx context.Context, accessClient authtypes.AccessClien Name: "organization", } } else { - // Dashboard annotation: use dashboard.grafana.app/annotations virtual resource, + // Dashboard annotation: use dashboards/annotations subresource, // which maps to annotation actions scoped to dashboards:uid:<dashboardUID>. checkReq = authtypes.CheckRequest{ - Verb: verb, - Group: "dashboard.grafana.app", - Resource: "annotations", - Namespace: namespace, - Name: *anno.Spec.DashboardUID, + Verb: verb, + Group: "dashboard.grafana.app", + Resource: "dashboards", + Subresource: "annotations", + Namespace: namespace, + Name: *anno.Spec.DashboardUID, } } diff --git a/pkg/registry/apps/annotation/authz_test.go b/pkg/registry/apps/annotation/authz_test.go index 4454372d9c88a..80b208f8576ea 100644 --- a/pkg/registry/apps/annotation/authz_test.go +++ b/pkg/registry/apps/annotation/authz_test.go @@ -53,52 +53,44 @@ func TestCanAccessAnnotation(t *testing.T) { ctx := k8srequest.WithNamespace(identity.WithServiceIdentityContext(t.Context(), 1), ns) - tests := []struct { - desc string - anno *annotationV0.Annotation - expectedGroup string - expectedName string - expectedNamespace string - }{ - { - desc: "org annotation uses annotation.grafana.app/organization scope", - anno: &annotationV0.Annotation{ - ObjectMeta: metav1.ObjectMeta{Name: "org-anno", Namespace: ns}, - }, - expectedGroup: "annotation.grafana.app", - expectedName: "organization", - expectedNamespace: ns, - }, - { - desc: "dashboard annotation uses dashboard.grafana.app/<dashUID> scope", - anno: &annotationV0.Annotation{ - ObjectMeta: metav1.ObjectMeta{Name: "dash-anno", Namespace: ns}, - Spec: annotationV0.AnnotationSpec{DashboardUID: &dashUID}, - }, - expectedGroup: "dashboard.grafana.app", - expectedName: dashUID, - expectedNamespace: ns, - }, - } + var captured authtypes.CheckRequest + accessClient := &fakeAccessClient{fn: func(req authtypes.CheckRequest) bool { + captured = req + return true + }} + + t.Run("org annotation", func(t *testing.T) { + anno := &annotationV0.Annotation{ + ObjectMeta: metav1.ObjectMeta{Name: "org-anno", Namespace: ns}, + } + allowed, err := canAccessAnnotation(ctx, accessClient, ns, anno, utils.VerbGet) + require.NoError(t, err) + require.True(t, allowed) + + assert.Equal(t, "annotation.grafana.app", captured.Group) + assert.Equal(t, "annotations", captured.Resource) + assert.Equal(t, "organization", captured.Name) + assert.Equal(t, ns, captured.Namespace) + assert.Equal(t, utils.VerbGet, captured.Verb) + assert.Equal(t, "", captured.Subresource) + }) - for _, tc := range tests { - t.Run(tc.desc, func(t *testing.T) { - var captured authtypes.CheckRequest - accessClient := &fakeAccessClient{fn: func(req authtypes.CheckRequest) bool { - captured = req - return true - }} - - allowed, err := canAccessAnnotation(ctx, accessClient, ns, tc.anno, utils.VerbGet) - require.NoError(t, err) - require.True(t, allowed) - - assert.Equal(t, tc.expectedGroup, captured.Group) - assert.Equal(t, "annotations", captured.Resource) - assert.Equal(t, tc.expectedName, captured.Name) - assert.Equal(t, tc.expectedNamespace, captured.Namespace) - }) - } + t.Run("dashboard annotation", func(t *testing.T) { + anno := &annotationV0.Annotation{ + ObjectMeta: metav1.ObjectMeta{Name: "dash-anno", Namespace: ns}, + Spec: annotationV0.AnnotationSpec{DashboardUID: &dashUID}, + } + allowed, err := canAccessAnnotation(ctx, accessClient, ns, anno, utils.VerbGet) + require.NoError(t, err) + require.True(t, allowed) + + assert.Equal(t, "dashboard.grafana.app", captured.Group) + assert.Equal(t, "dashboards", captured.Resource) + assert.Equal(t, "annotations", captured.Subresource) + assert.Equal(t, dashUID, captured.Name) + assert.Equal(t, ns, captured.Namespace) + assert.Equal(t, utils.VerbGet, captured.Verb) + }) } func TestCanAccessAnnotations(t *testing.T) { diff --git a/pkg/services/authz/rbac/mapper.go b/pkg/services/authz/rbac/mapper.go index 9b45f4c7bdfc1..024ddb7044007 100644 --- a/pkg/services/authz/rbac/mapper.go +++ b/pkg/services/authz/rbac/mapper.go @@ -213,13 +213,15 @@ func NewMapperRegistry() MapperRegistry { "dashboard.grafana.app": { "dashboards": newDashboardTranslation(), "librarypanels": newResourceTranslation("library.panels", "uid", true, nil), - // Virtual resource: maps annotation verbs to annotation actions scoped to dashboards:uid:<uid>. - "annotations": translation{ + // Annotations subresource for dashboards + // Uses dashboard scope (dashboards:uid:...) but annotation actions + "dashboards/annotations": translation{ resource: "dashboards", attribute: "uid", verbMapping: map[string]string{ utils.VerbGet: "annotations:read", utils.VerbList: "annotations:read", + utils.VerbWatch: "annotations:read", utils.VerbCreate: "annotations:create", utils.VerbUpdate: "annotations:write", utils.VerbPatch: "annotations:write", @@ -230,14 +232,14 @@ func NewMapperRegistry() MapperRegistry { actionSetMapping: map[string][]string{ utils.VerbGet: {"dashboards:view", "folders:view", "dashboards:edit", "folders:edit", "dashboards:admin", "folders:admin"}, utils.VerbList: {"dashboards:view", "folders:view", "dashboards:edit", "folders:edit", "dashboards:admin", "folders:admin"}, + utils.VerbWatch: {"dashboards:view", "folders:view", "dashboards:edit", "folders:edit", "dashboards:admin", "folders:admin"}, utils.VerbCreate: {"dashboards:edit", "folders:edit", "dashboards:admin", "folders:admin"}, utils.VerbUpdate: {"dashboards:edit", "folders:edit", "dashboards:admin", "folders:admin"}, utils.VerbPatch: {"dashboards:edit", "folders:edit", "dashboards:admin", "folders:admin"}, utils.VerbDelete: {"dashboards:edit", "folders:edit", "dashboards:admin", "folders:admin"}, utils.VerbDeleteCollection: {"dashboards:edit", "folders:edit", "dashboards:admin", "folders:admin"}, }, - folderSupport: false, - skipScopeOnVerb: nil, + folderSupport: true, }, }, "folder.grafana.app": { diff --git a/pkg/services/authz/rbac/mapper_test.go b/pkg/services/authz/rbac/mapper_test.go index e32511f543ddf..dab22298f9175 100644 --- a/pkg/services/authz/rbac/mapper_test.go +++ b/pkg/services/authz/rbac/mapper_test.go @@ -4,6 +4,7 @@ import ( "strings" "testing" + "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -184,3 +185,33 @@ func TestMapperRegistry_SubresourceLookup(t *testing.T) { assert.False(t, ok) }) } + +// TestMapper_AnnotationSubresource_ActionSets verifies that managed roles (dashboards:view etc.) +// flow through to annotation verbs via the subresource action set mapping. +func TestMapper_AnnotationSubresource_ActionSets(t *testing.T) { + mapper := NewMapperRegistry() + mapping, ok := mapper.Get("dashboard.grafana.app", "dashboards", "annotations") + require.True(t, ok) + + readActionSets := []string{"dashboards:view", "folders:view", "dashboards:edit", "folders:edit", "dashboards:admin", "folders:admin"} + writeActionSets := []string{"dashboards:edit", "folders:edit", "dashboards:admin", "folders:admin"} + + tests := []struct { + verb string + expected []string + }{ + {utils.VerbGet, readActionSets}, + {utils.VerbList, readActionSets}, + {utils.VerbWatch, readActionSets}, + {utils.VerbCreate, writeActionSets}, + {utils.VerbUpdate, writeActionSets}, + {utils.VerbPatch, writeActionSets}, + {utils.VerbDelete, writeActionSets}, + } + + for _, tt := range tests { + t.Run(tt.verb, func(t *testing.T) { + assert.ElementsMatch(t, tt.expected, mapping.ActionSets(tt.verb)) + }) + } +} diff --git a/pkg/services/authz/rbac/service_test.go b/pkg/services/authz/rbac/service_test.go index f9c80c65beac5..fee2714c38211 100644 --- a/pkg/services/authz/rbac/service_test.go +++ b/pkg/services/authz/rbac/service_test.go @@ -264,6 +264,25 @@ func TestService_checkPermission(t *testing.T) { }, expected: true, }, + { + name: "should return true if user has annotation create permission on dashboard (subresource)", + permissions: []accesscontrol.Permission{ + { + Action: "annotations:create", + Scope: "dashboards:uid:some_dashboard", + Kind: "dashboards", + Attribute: "uid", + Identifier: "some_dashboard", + }, + }, + check: checkRequest{ + Action: "annotations:create", + Group: "dashboard.grafana.app", + Resource: "dashboards", + Name: "some_dashboard", + }, + expected: true, + }, { name: "should allow querying a datasource", permissions: []accesscontrol.Permission{ @@ -378,6 +397,29 @@ func TestService_mapping(t *testing.T) { }, }, }, + { + name: "should map annotations subresource to annotation actions", + input: &authzv1.CheckRequest{ + Group: "dashboard.grafana.app", + Resource: "dashboards", + Subresource: "annotations", + Name: "dash1", + Verb: utils.VerbCreate, + }, + output: &checkRequest{ + Action: "annotations:create", + ActionSets: []string{"folders:edit", "folders:admin", "dashboards:edit", "dashboards:admin"}, + Group: "dashboard.grafana.app", + Resource: "dashboards", + Subresource: "annotations", + Name: "dash1", + Verb: "create", + Namespace: types.NamespaceInfo{ + Value: ns, + OrgID: 1, + }, + }, + }, } for _, tc := range testCases { @@ -398,6 +440,11 @@ func TestService_mapping(t *testing.T) { ... [truncated]