Access Control / Authorization

PoC (conceptual): 1) Pre-fix scenario (Grafana <= 12.3.x with legacy access control in place): an authenticated user with limited permissions attempts to fetch annotations for a dashboard they should not access. Because the patch removes the legacy permission filter from the annotation retrieval path, there is a risk that the response may include annotations (and associated dashboards) the user should not see if the new authorization path is not yet consistently enforced across all code paths. 2) Attack surface: access to annotations tied to restricted dashboards via the API that returns aggregated or per-dashboard annotation data. 3) Steps (illustrative, non-operational without a running target): - Ensure existence of at least one private dashboard (UID: uid_private) and one public dashboard (UID: uid_public) within org 1. - Create annotations linked to both dashboards. - Authenticate as a user with limited permissions (e.g., viewer only on public dashboards). - Call GET /api/annotations/search?dashboardUid=uid_public (and then omit dashboardUid to test aggregation) to observe whether annotations for uid_private leak into the response. - Compare responses between pre-fix (where legacy filters might have allowed leakage) and post-fix (where legacy filters are removed and the new access control path governs visibility). Notes: - The exact endpoints and payloads depend on Grafana's API at the time; adapt to /api/annotations or analogous endpoints that surface annotation data by dashboard context. The PoC focuses on illustrating potential leakage due to legacy RBAC filter removal prior to full adoption of the new authorization flow.

diff --git a/apps/advisor/go.mod b/apps/advisor/go.mod index 2493ee5a81a59..747a6add526e6 100644 --- a/apps/advisor/go.mod +++ b/apps/advisor/go.mod @@ -64,7 +64,6 @@ replace ( require ( cel.dev/expr v0.25.1 // indirect - cloud.google.com/go/compute/metadata v0.9.0 // indirect dario.cat/mergo v1.0.2 // indirect filippo.io/edwards25519 v1.2.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/azcore v1.20.0 // indirect diff --git a/pkg/infra/db/sqlbuilder.go b/pkg/infra/db/sqlbuilder.go index 8ab187ed596ba..0bd114bf58d6f 100644 --- a/pkg/infra/db/sqlbuilder.go +++ b/pkg/infra/db/sqlbuilder.go @@ -3,11 +3,8 @@ package db import ( "bytes" - "github.com/grafana/grafana/pkg/apimachinery/identity" - "github.com/grafana/grafana/pkg/services/dashboards/dashboardaccess" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/sqlstore/migrator" - "github.com/grafana/grafana/pkg/services/sqlstore/permissions" "github.com/grafana/grafana/pkg/setting" ) @@ -62,24 +59,3 @@ func (sb *SQLBuilder) GetParams() []any { func (sb *SQLBuilder) AddParams(params ...any) { sb.params = append(sb.params, params...) } - -func (sb *SQLBuilder) WriteDashboardPermissionFilter(user identity.Requester, permission dashboardaccess.PermissionType, queryType string) { - var ( - sql string - params []any - recQry string - recQryParams []any - leftJoin string - ) - - filterRBAC := permissions.NewAccessControlDashboardPermissionFilter(user, permission, queryType, sb.features, sb.recursiveQueriesAreSupported, sb.dialect, sb.cfg.MaxNestedFolderDepth) - leftJoin = filterRBAC.LeftJoin() - sql, params = filterRBAC.Where() - recQry, recQryParams = filterRBAC.With() - - sb.sql.WriteString(" AND " + sql) - sb.params = append(sb.params, params...) - sb.recQry = recQry - sb.recQryParams = recQryParams - sb.leftJoin = leftJoin -} diff --git a/pkg/services/annotations/accesscontrol/accesscontrol.go b/pkg/services/annotations/accesscontrol/accesscontrol.go index 2bd50e619ede8..824968a2ba846 100644 --- a/pkg/services/annotations/accesscontrol/accesscontrol.go +++ b/pkg/services/annotations/accesscontrol/accesscontrol.go @@ -8,10 +8,8 @@ import ( ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/annotations" "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/dashboards/dashboardaccess" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/search/model" - "github.com/grafana/grafana/pkg/services/sqlstore/permissions" "github.com/grafana/grafana/pkg/setting" ) @@ -108,28 +106,14 @@ func (authz *AuthService) getAnnotationDashboard(ctx context.Context, query anno } func (authz *AuthService) dashboardsWithVisibleAnnotations(ctx context.Context, query annotations.ItemQuery) (map[string]int64, error) { - recursiveQueriesSupported, err := authz.db.RecursiveQueriesAreSupported() - if err != nil { - return nil, err - } - - filters := []any{ - permissions.NewAccessControlDashboardPermissionFilter(query.SignedInUser, dashboardaccess.PERMISSION_VIEW, model.TypeAnnotation, authz.features, recursiveQueriesSupported, authz.db.GetDialect(), authz.maxDepth), - model.OrgFilter{OrgId: query.OrgID}, - } - var dashboardUIDs []string if query.DashboardUID != "" { - dashboardUIDs = append(dashboardUIDs, query.DashboardUID) - filters = append(filters, model.DashboardFilter{ - UIDs: []string{query.DashboardUID}, - }) + dashboardUIDs = []string{query.DashboardUID} } dashs, err := authz.dashSvc.SearchDashboards(ctx, &dashboards.FindPersistedDashboardsQuery{ DashboardUIDs: dashboardUIDs, OrgId: query.SignedInUser.GetOrgID(), - Filters: filters, SignedInUser: query.SignedInUser, Page: query.Page, Type: model.TypeAnnotation, diff --git a/pkg/services/annotations/accesscontrol/accesscontrol_test.go b/pkg/services/annotations/accesscontrol/accesscontrol_test.go index 28feb6f479018..e03bb0268b57e 100644 --- a/pkg/services/annotations/accesscontrol/accesscontrol_test.go +++ b/pkg/services/annotations/accesscontrol/accesscontrol_test.go @@ -4,13 +4,10 @@ import ( "context" "testing" - "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/services/annotations" "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/dashboards/dashboardaccess" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/search/model" - "github.com/grafana/grafana/pkg/services/sqlstore/permissions" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/tests/testsuite" @@ -21,91 +18,73 @@ import ( func TestMain(m *testing.M) { testsuite.Run(m) } + func TestDashboardsWithVisibleAnnotations(t *testing.T) { - store := db.InitTestDB(t) cfg := setting.NewCfg() - user := &user.SignedInUser{ - OrgID: 1, - } - - // Create permission filters - p1 := permissions.NewAccessControlDashboardPermissionFilter(user, dashboardaccess.PERMISSION_VIEW, model.TypeAnnotation, featuremgmt.WithFeatures(), true, store.GetDialect(), cfg.MaxNestedFolderDepth) - p2 := model.OrgFilter{OrgId: 1} - - // If DashboardUID is provided, it should be added as a filter - p3 := model.DashboardFilter{UIDs: []string{"uid1"}} + signedInUser := &user.SignedInUser{OrgID: 1} dashSvc := &dashboards.FakeDashboardService{} - // First call, without DashboardUID - queryNoDashboardUID := &dashboards.FindPersistedDashboardsQuery{ + // without DashboardUID: DashboardUIDs should be nil so the K8s search returns all visible dashboards + queryAllDashboards := &dashboards.FindPersistedDashboardsQuery{ OrgId: 1, - SignedInUser: user, - Type: "dash-annotation", + SignedInUser: signedInUser, + Type: model.TypeAnnotation, Limit: int64(100), Page: int64(1), - Filters: []any{ - p1, - p2, - }, } - dashSvc.On("SearchDashboards", mock.Anything, queryNoDashboardUID).Return(model.HitList{ - &model.Hit{UID: "uid1", ID: 101}, - &model.Hit{UID: "uid2", ID: 102}, + dashSvc.On("SearchDashboards", mock.Anything, queryAllDashboards).Return(model.HitList{ + {UID: "uid1", ID: 101}, + {UID: "uid2", ID: 102}, }, nil) - // Second call, with DashboardUID filter - queryWithDashboardUID := &dashboards.FindPersistedDashboardsQuery{ - OrgId: 1, - SignedInUser: user, - Type: "dash-annotation", - Limit: int64(100), - Page: int64(1), - Filters: []any{ - p1, - p2, - // This filter should be added on second call - p3, - }, + // with DashboardUID: DashboardUIDs should be set so the K8s search is scoped to that dashboard + queryOneDashboard := &dashboards.FindPersistedDashboardsQuery{ DashboardUIDs: []string{"uid1"}, + OrgId: 1, + SignedInUser: signedInUser, + Type: model.TypeAnnotation, + Limit: int64(100), + Page: int64(1), } - - dashSvc.On("SearchDashboards", mock.Anything, queryWithDashboardUID).Return(model.HitList{ - &model.Hit{UID: "uid1", ID: 101}, + dashSvc.On("SearchDashboards", mock.Anything, queryOneDashboard).Return(model.HitList{ + {UID: "uid1", ID: 101}, }, nil) - // Create auth service authz := &AuthService{ - db: store, features: featuremgmt.WithFeatures(), dashSvc: dashSvc, searchDashboardsPageLimit: 100, maxDepth: cfg.MaxNestedFolderDepth, } - // First call without DashboardUID - result, err := authz.dashboardsWithVisibleAnnotations(context.Background(), annotations.ItemQuery{ - SignedInUser: user, - OrgID: 1, - Page: 1, + t.Run("without DashboardUID returns all visible dashboards", func(t *testing.T) { + result, err := authz.dashboardsWithVisibleAnnotations(context.Background(), annotations.ItemQuery{ + SignedInUser: signedInUser, + OrgID: 1, + Page: 1, + }) + assert.NoError(t, err) + assert.Equal(t, map[string]int64{"uid1": 101, "uid2": 102}, result) + dashSvc.AssertCalled(t, "SearchDashboards", mock.Anything, queryAllDashboards) + }) + + t.Run("with DashboardUID filters to that dashboard", func(t *testing.T) { + result, err := authz.dashboardsWithVisibleAnnotations(context.Background(), annotations.ItemQuery{ + SignedInUser: signedInUser, + OrgID: 1, + Page: 1, + DashboardUID: "uid1", + }) + assert.NoError(t, err) + assert.Equal(t, map[string]int64{"uid1": 101}, result) + dashSvc.AssertCalled(t, "SearchDashboards", mock.Anything, queryOneDashboard) }) - assert.NoError(t, err) - // Should return two dashboards - assert.Equal(t, map[string]int64{"uid1": 101, "uid2": 102}, result) - // Ensure SearchDashboards was called with correct query - dashSvc.AssertCalled(t, "SearchDashboards", mock.Anything, queryNoDashboardUID) - // Second call with DashboardUID - result, err = authz.dashboardsWithVisibleAnnotations(context.Background(), annotations.ItemQuery{ - SignedInUser: user, - OrgID: 1, - Page: 1, - DashboardUID: "uid1", + t.Run("passes signed-in user to SearchDashboards for K8s access control enforcement", func(t *testing.T) { + dashSvc.AssertCalled(t, "SearchDashboards", mock.Anything, mock.MatchedBy(func(q *dashboards.FindPersistedDashboardsQuery) bool { + return q.SignedInUser == signedInUser + })) }) - assert.NoError(t, err) - // Should only return one dashboard - assert.Equal(t, map[string]int64{"uid1": 101}, result) - // Ensure SearchDashboards was called with correct query (including DashboardUID filter) - dashSvc.AssertCalled(t, "SearchDashboards", mock.Anything, queryWithDashboardUID) } diff --git a/pkg/services/dashboards/models.go b/pkg/services/dashboards/models.go index 2069f15af9005..bd905d2f5277b 100644 --- a/pkg/services/dashboards/models.go +++ b/pkg/services/dashboards/models.go @@ -534,8 +534,6 @@ type FindPersistedDashboardsQuery struct { SourcePath string ManagerIdentityNotIn []string - Filters []any - // Skip access control checks. This field is used by OpenFGA search implementation. // Should not be used anywhere else. SkipAccessControlFilter bool