Authorization / Information disclosure

Poc: Demonstrates how an unauthorized user could previously leak annotations via a batch listing, and how the fix prevents it. Preconditions: - Grafana/k8s-custom-resource for annotations exists in namespace = ns (e.g., "org-1"). - Two annotations exist: - org-anno: organization-level annotation (no DashboardUID) - dash-anno: dashboard-level annotation (DashboardUID = "dash-abc") - A user identity is provided that has permission to view organization-level annotations but NOT dashboard-level annotations. Reproduction (pre-fix behavior): 1) Authenticate as a user with limited scope. 2) Call the listing API for annotations in namespace ns. 3) Observe that both items org-anno and dash-anno are returned, leaking the dashboard-scoped item to an unauthorized user. Example curl (conceptual): curl -H "Authorization: Bearer $TOKEN" \ https://grafana.example/apis/annotation.grafana.app/v0/namespaces/org-1/annotations Expected pre-fix response (leak): { "items": [ {"name": "org-anno", "namespace": "org-1", "spec": {}}, {"name": "dash-anno", "namespace": "org-1", "spec": {"dashboardUID": "dash-abc"}} ] } Reproduction (post-fix behavior, after this commit): 1) Use the same authenticated request as above. 2) The server now applies batch per-item authorization and filters results. 3) Observe that only allowed items are returned (org-anno is visible; dash-anno is omitted). Example curl (conceptual): curl -H "Authorization: Bearer $TOKEN" \ https://grafana.example/apis/annotation.grafana.app/v0/namespaces/org-1/annotations Post-fix response (no leak): { "items": [ {"name": "org-anno", "namespace": "org-1", "spec": {}} ] } Notes: - The patch also adds tests to validate canAccessAnnotations behavior and List-time filtering in both the REST adapter and search handler. - If you have tooling to simulate different tokens with restricted scopes, you can verify that a dashboard-scoped annotation is not included in list results for restricted users after the fix.

diff --git a/pkg/registry/apps/annotation/authz.go b/pkg/registry/apps/annotation/authz.go index 289b6885db35e..8b843846204aa 100644 --- a/pkg/registry/apps/annotation/authz.go +++ b/pkg/registry/apps/annotation/authz.go @@ -3,6 +3,7 @@ package annotation import ( "context" "fmt" + "strconv" authtypes "github.com/grafana/authlib/types" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -67,3 +68,54 @@ func canAccessAnnotation(ctx context.Context, accessClient authtypes.AccessClien return resp.Allowed, nil } + +// canAccessAnnotations checks permissions for a batch of annotations, +// returning a boolean slice aligned with the input items slice +func canAccessAnnotations(ctx context.Context, accessClient authtypes.AccessClient, namespace string, items []annotationV0.Annotation, verb string) ([]bool, error) { + if len(items) == 0 { + return nil, nil + } + + authInfo, ok := authtypes.AuthInfoFrom(ctx) + if !ok { + return nil, apierrors.NewUnauthorized("no identity found for request") + } + + checks := make([]authtypes.BatchCheckItem, 0, len(items)) + for i, anno := range items { + var item authtypes.BatchCheckItem + item.CorrelationID = strconv.Itoa(i) + item.Verb = verb + + if anno.Spec.DashboardUID == nil || *anno.Spec.DashboardUID == "" { + item.Group = "annotation.grafana.app" + item.Resource = "annotations" + item.Name = "organization" + } else { + item.Group = "dashboard.grafana.app" + item.Resource = "annotations" + item.Name = *anno.Spec.DashboardUID + } + + checks = append(checks, item) + } + + allowed := make([]bool, len(items)) + for start := 0; start < len(checks); start += authtypes.MaxBatchCheckItems { + end := min(start+authtypes.MaxBatchCheckItems, len(checks)) + res, err := accessClient.BatchCheck(ctx, authInfo, authtypes.BatchCheckRequest{ + Namespace: namespace, + Checks: checks[start:end], + }) + if err != nil { + return nil, fmt.Errorf("batch authz check failed: %w", err) + } + for id, result := range res.Results { + if idx, err := strconv.Atoi(id); err == nil { + allowed[idx] = result.Allowed + } + } + } + + return allowed, nil +} diff --git a/pkg/registry/apps/annotation/authz_test.go b/pkg/registry/apps/annotation/authz_test.go index 109c66329940d..4454372d9c88a 100644 --- a/pkg/registry/apps/annotation/authz_test.go +++ b/pkg/registry/apps/annotation/authz_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8srequest "k8s.io/apiserver/pkg/endpoints/request" registryrest "k8s.io/apiserver/pkg/registry/rest" @@ -30,8 +31,20 @@ func (c *fakeAccessClient) Compile(_ context.Context, _ authtypes.AuthInfo, _ au return nil, nil, nil } -func (c *fakeAccessClient) BatchCheck(_ context.Context, _ authtypes.AuthInfo, _ authtypes.BatchCheckRequest) (authtypes.BatchCheckResponse, error) { - return authtypes.BatchCheckResponse{}, nil +func (c *fakeAccessClient) BatchCheck(_ context.Context, _ authtypes.AuthInfo, req authtypes.BatchCheckRequest) (authtypes.BatchCheckResponse, error) { + results := make(map[string]authtypes.BatchCheckResult, len(req.Checks)) + for _, item := range req.Checks { + results[item.CorrelationID] = authtypes.BatchCheckResult{ + Allowed: c.fn(authtypes.CheckRequest{ + Verb: item.Verb, + Group: item.Group, + Resource: item.Resource, + Namespace: req.Namespace, + Name: item.Name, + }), + } + } + return authtypes.BatchCheckResponse{Results: results}, nil } func TestCanAccessAnnotation(t *testing.T) { @@ -88,6 +101,75 @@ func TestCanAccessAnnotation(t *testing.T) { } } +func TestCanAccessAnnotations(t *testing.T) { + ns := "org-1" + dashUID := "dash-abc" + + ctx := k8srequest.WithNamespace(identity.WithServiceIdentityContext(t.Context(), 1), ns) + + orgAnno := annotationV0.Annotation{ + ObjectMeta: metav1.ObjectMeta{Name: "org-anno", Namespace: ns}, + } + dashAnno := annotationV0.Annotation{ + ObjectMeta: metav1.ObjectMeta{Name: "dash-anno", Namespace: ns}, + Spec: annotationV0.AnnotationSpec{DashboardUID: &dashUID}, + } + + t.Run("empty items returns nil", func(t *testing.T) { + client := &fakeAccessClient{fn: func(_ authtypes.CheckRequest) bool { return true }} + allowed, err := canAccessAnnotations(ctx, client, ns, nil, utils.VerbList) + require.NoError(t, err) + assert.Nil(t, allowed) + }) + + t.Run("all allowed", func(t *testing.T) { + client := &fakeAccessClient{fn: func(_ authtypes.CheckRequest) bool { return true }} + allowed, err := canAccessAnnotations(ctx, client, ns, []annotationV0.Annotation{orgAnno, dashAnno}, utils.VerbList) + require.NoError(t, err) + assert.Equal(t, []bool{true, true}, allowed) + }) + + t.Run("all denied", func(t *testing.T) { + client := &fakeAccessClient{fn: func(_ authtypes.CheckRequest) bool { return false }} + allowed, err := canAccessAnnotations(ctx, client, ns, []annotationV0.Annotation{orgAnno, dashAnno}, utils.VerbList) + require.NoError(t, err) + assert.Equal(t, []bool{false, false}, allowed) + }) + + t.Run("mixed - allow org deny dashboard", func(t *testing.T) { + client := &fakeAccessClient{fn: func(req authtypes.CheckRequest) bool { + return req.Group == "annotation.grafana.app" + }} + allowed, err := canAccessAnnotations(ctx, client, ns, []annotationV0.Annotation{orgAnno, dashAnno}, utils.VerbList) + require.NoError(t, err) + assert.Equal(t, []bool{true, false}, allowed) + }) + + t.Run("correct check fields for dashboard annotation", func(t *testing.T) { + var captured []authtypes.CheckRequest + client := &fakeAccessClient{fn: func(req authtypes.CheckRequest) bool { + captured = append(captured, req) + return true + }} + _, err := canAccessAnnotations(ctx, client, ns, []annotationV0.Annotation{dashAnno}, utils.VerbList) + require.NoError(t, err) + require.Len(t, captured, 1) + assert.Equal(t, "dashboard.grafana.app", captured[0].Group) + assert.Equal(t, "annotations", captured[0].Resource) + assert.Equal(t, dashUID, captured[0].Name) + assert.Equal(t, ns, captured[0].Namespace) + assert.Equal(t, utils.VerbList, captured[0].Verb) + }) + + t.Run("no auth info returns error", func(t *testing.T) { + ctxNoAuth := k8srequest.WithNamespace(context.Background(), ns) + client := &fakeAccessClient{fn: func(_ authtypes.CheckRequest) bool { return true }} + _, err := canAccessAnnotations(ctxNoAuth, client, ns, []annotationV0.Annotation{orgAnno}, utils.VerbList) + require.Error(t, err) + assert.True(t, apierrors.IsUnauthorized(err)) + }) +} + func TestK8sRESTAdapter_UpdateScopeEscalation(t *testing.T) { const ns = "org-1" dashUID := "dash-abc" @@ -116,3 +198,65 @@ func TestK8sRESTAdapter_UpdateScopeEscalation(t *testing.T) { require.Error(t, err) assert.True(t, apierrors.IsForbidden(err)) } + +func TestK8sRESTAdapter_ListFiltersUnauthorized(t *testing.T) { + const ns = "org-1" + dashUID := "dash-abc" + + ctx := k8srequest.WithNamespace(identity.WithServiceIdentityContext(t.Context(), 1), ns) + + store := NewMemoryStore() + orgAnno := &annotationV0.Annotation{ + ObjectMeta: metav1.ObjectMeta{Name: "org-anno", Namespace: ns}, + } + dashAnno := &annotationV0.Annotation{ + ObjectMeta: metav1.ObjectMeta{Name: "dash-anno", Namespace: ns}, + Spec: annotationV0.AnnotationSpec{DashboardUID: &dashUID}, + } + _, err := store.Create(ctx, orgAnno) + require.NoError(t, err) + _, err = store.Create(ctx, dashAnno) + require.NoError(t, err) + + t.Run("filters out denied annotations", func(t *testing.T) { + adapter := &k8sRESTAdapter{ + store: store, + accessClient: &fakeAccessClient{fn: func(req authtypes.CheckRequest) bool { + return req.Group == "annotation.grafana.app" + }}, + } + + obj, err := adapter.List(ctx, &internalversion.ListOptions{}) + require.NoError(t, err) + + list := obj.(*annotationV0.AnnotationList) + require.Len(t, list.Items, 1) + assert.Equal(t, "org-anno", list.Items[0].Name) + }) + + t.Run("returns all when all allowed", func(t *testing.T) { + adapter := &k8sRESTAdapter{ + store: store, + accessClient: &fakeAccessClient{fn: func(_ authtypes.CheckRequest) bool { return true }}, + } + + obj, err := adapter.List(ctx, &internalversion.ListOptions{}) + require.NoError(t, err) + + list := obj.(*annotationV0.AnnotationList) + assert.Len(t, list.Items, 2) + }) + + t.Run("returns empty when all denied", func(t *testing.T) { + adapter := &k8sRESTAdapter{ + store: store, + accessClient: &fakeAccessClient{fn: func(_ authtypes.CheckRequest) bool { return false }}, + } + + obj, err := adapter.List(ctx, &internalversion.ListOptions{}) + require.NoError(t, err) + + list := obj.(*annotationV0.AnnotationList) + assert.Empty(t, list.Items) + }) +} diff --git a/pkg/registry/apps/annotation/k8s_adapter.go b/pkg/registry/apps/annotation/k8s_adapter.go index 5cfe68478d2a3..26b04c28c41e5 100644 --- a/pkg/registry/apps/annotation/k8s_adapter.go +++ b/pkg/registry/apps/annotation/k8s_adapter.go @@ -140,13 +140,13 @@ func (s *k8sRESTAdapter) List(ctx context.Context, options *internalversion.List } // TODO: post-fetch filtering breaks pagination - cursor advances by opts.Limit regardless of authz results. + allowed, err := canAccessAnnotations(ctx, s.accessClient, namespace, result.Items, utils.VerbList) + if err != nil { + return nil, err + } filtered := make([]annotationV0.Annotation, 0, len(result.Items)) - for _, anno := range result.Items { - allowed, err := canAccessAnnotation(ctx, s.accessClient, namespace, &anno, utils.VerbList) - if err != nil { - return nil, err - } - if allowed { + for i, anno := range result.Items { + if allowed[i] { filtered = append(filtered, anno) } } diff --git a/pkg/registry/apps/annotation/search_handler.go b/pkg/registry/apps/annotation/search_handler.go index 40253a5678034..5905f7d500a69 100644 --- a/pkg/registry/apps/annotation/search_handler.go +++ b/pkg/registry/apps/annotation/search_handler.go @@ -25,13 +25,14 @@ func newSearchHandler(store Store, accessClient authtypes.AccessClient) func(ctx return err } + allowed, err := canAccessAnnotations(ctx, accessClient, namespace, result.Items, utils.VerbList) + if err != nil { + return err + } + filtered := make([]annotationV0.Annotation, 0, len(result.Items)) - for _, anno := range result.Items { - allowed, err := canAccessAnnotation(ctx, accessClient, namespace, &anno, utils.VerbList) - if err != nil { - return err - } - if allowed { + for i, anno := range result.Items { + if allowed[i] { filtered = append(filtered, anno) } }