Information Disclosure

PoC (conceptual, before fix behavior): - Scenario: A client triggers a batch authorization check failure while attempting to access/modify annotations in a restricted scope. - Expected pre-fix behavior: The HTTP 500 response (or equivalent) includes the raw internal error message, e.g. "batch authz check failed: permission denied on group annotation.grafana.app". - After fix: The server returns an internal error wrapper, concealing the specific inner error details from the client. Example (Go-like pseudocode demonstrating the difference): // Before fix (vulnerable): innerErr := fmt.Errorf("permission denied on group %s", group) err := fmt.Errorf("batch authz check failed: %w", innerErr) // API responds with: batch authz check failed: permission denied on group annotation.grafana.app // After fix (patched): innerErr := fmt.Errorf("permission denied on group %s", group) err := apierrors.NewInternalError(fmt.Errorf("batch authz check failed: %w", innerErr)) // API responds with a generic internal error envelope, masking the inner details Attack vector: An attacker who can trigger batch authz failures (e.g., by manipulating requested resources or scopes) could previously receive specific internal error details in the API response, aiding fingerprinting or debugging. With the fix, such details are hidden behind a generalized internal error, reducing information exposed to the client. Prerequisites: Access to an endpoint performing batch authorization checks (as part of annotation handling) and a user/request that causes a batch check to fail. Notes: The exact HTTP response payload depends on Grafana's API error wiring, but the core issue is leakage of internal error strings from batch authz failures prior to this patch.

diff --git a/pkg/registry/apps/annotation/authz.go b/pkg/registry/apps/annotation/authz.go index aecf12d36e899..0399829b1a5d4 100644 --- a/pkg/registry/apps/annotation/authz.go +++ b/pkg/registry/apps/annotation/authz.go @@ -91,7 +91,7 @@ func canAccessAnnotations(ctx context.Context, accessClient authtypes.AccessClie Checks: checks[start:end], }) if err != nil { - return nil, fmt.Errorf("batch authz check failed: %w", err) + return nil, apierrors.NewInternalError(fmt.Errorf("batch authz check failed: %w", err)) } for id, result := range res.Results { if idx, err := strconv.Atoi(id); err == nil { diff --git a/pkg/registry/apps/annotation/authz_test.go b/pkg/registry/apps/annotation/authz_test.go index 99c5d8b14b6cf..1db9b03aa4523 100644 --- a/pkg/registry/apps/annotation/authz_test.go +++ b/pkg/registry/apps/annotation/authz_test.go @@ -17,8 +17,28 @@ import ( annotationV0 "github.com/grafana/grafana/apps/annotation/pkg/apis/annotation/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/tracing" ) +// newTestAdapter builds a k8sRESTAdapter with the observability deps wired to +// no-op test implementations. An optional folderResolver may be passed; otherwise +// a no-op resolver is used (sufficient for tests that don't exercise dashboard authz). +func newTestAdapter(store Store, ac authtypes.AccessClient, fr ...DashboardFolderResolver) *k8sRESTAdapter { + var resolver DashboardFolderResolver = NoopDashboardFolderResolver{} + if len(fr) > 0 && fr[0] != nil { + resolver = fr[0] + } + return &k8sRESTAdapter{ + store: store, + accessClient: ac, + folderResolver: resolver, + tracer: tracing.InitializeTracerForTest(), + logger: log.NewNopLogger(), + metrics: ProvideMetrics(nil), + } +} + // fakeAccessClient delegates allow/deny to fn. Single Check is synthesized into a BatchCheckItem // so tests can assert on item.Folder. type fakeAccessClient struct { @@ -264,13 +284,9 @@ func TestK8sRESTAdapter_UpdateScopeEscalation(t *testing.T) { // Allow writes on org annotations (annotation.grafana.app) but deny on dashboard scope. // The update attempts to move an org annotation onto a dashboard the caller cannot write. - adapter := &k8sRESTAdapter{ - store: store, - accessClient: &fakeAccessClient{fn: func(req authtypes.BatchCheckItem) bool { - return req.Group == "annotation.grafana.app" - }}, - folderResolver: newFakeFolderResolver(map[string]string{dashUID: ""}), - } + adapter := newTestAdapter(store, &fakeAccessClient{fn: func(req authtypes.BatchCheckItem) bool { + return req.Group == "annotation.grafana.app" + }}, newFakeFolderResolver(map[string]string{dashUID: ""})) orgAnno.Spec.DashboardUID = &dashUID _, _, err = adapter.Update(ctx, orgAnno.Name, registryrest.DefaultUpdatedObjectInfo(orgAnno), nil, nil, false, nil) @@ -298,13 +314,9 @@ func TestK8sRESTAdapter_ListFiltersUnauthorized(t *testing.T) { require.NoError(t, err) t.Run("filters out denied annotations", func(t *testing.T) { - adapter := &k8sRESTAdapter{ - store: store, - accessClient: &fakeAccessClient{fn: func(req authtypes.BatchCheckItem) bool { - return req.Group == "annotation.grafana.app" - }}, - folderResolver: newFakeFolderResolver(map[string]string{dashUID: ""}), - } + adapter := newTestAdapter(store, &fakeAccessClient{fn: func(req authtypes.BatchCheckItem) bool { + return req.Group == "annotation.grafana.app" + }}, newFakeFolderResolver(map[string]string{dashUID: ""})) obj, err := adapter.List(ctx, &internalversion.ListOptions{}) require.NoError(t, err) @@ -315,11 +327,7 @@ func TestK8sRESTAdapter_ListFiltersUnauthorized(t *testing.T) { }) t.Run("returns all when all allowed", func(t *testing.T) { - adapter := &k8sRESTAdapter{ - store: store, - accessClient: &fakeAccessClient{fn: func(_ authtypes.BatchCheckItem) bool { return true }}, - folderResolver: newFakeFolderResolver(map[string]string{dashUID: ""}), - } + adapter := newTestAdapter(store, &fakeAccessClient{fn: func(_ authtypes.BatchCheckItem) bool { return true }}, newFakeFolderResolver(map[string]string{dashUID: ""})) obj, err := adapter.List(ctx, &internalversion.ListOptions{}) require.NoError(t, err) @@ -329,11 +337,7 @@ func TestK8sRESTAdapter_ListFiltersUnauthorized(t *testing.T) { }) t.Run("returns empty when all denied", func(t *testing.T) { - adapter := &k8sRESTAdapter{ - store: store, - accessClient: &fakeAccessClient{fn: func(_ authtypes.BatchCheckItem) bool { return false }}, - folderResolver: newFakeFolderResolver(map[string]string{dashUID: ""}), - } + adapter := newTestAdapter(store, &fakeAccessClient{fn: func(_ authtypes.BatchCheckItem) bool { return false }}, newFakeFolderResolver(map[string]string{dashUID: ""})) obj, err := adapter.List(ctx, &internalversion.ListOptions{}) require.NoError(t, err) diff --git a/pkg/registry/apps/annotation/grpc_store.go b/pkg/registry/apps/annotation/grpc_store.go index 1c0a8000d29f4..4e14ff1dbda48 100644 --- a/pkg/registry/apps/annotation/grpc_store.go +++ b/pkg/registry/apps/annotation/grpc_store.go @@ -19,6 +19,7 @@ import ( // LifecycleManager, and TagProvider interfaces type storeGRPC struct { client storev1.AnnotationStoreClient + conn *grpc.ClientConn } var _ Store = (*storeGRPC)(nil) @@ -26,12 +27,21 @@ var _ LifecycleManager = (*storeGRPC)(nil) var _ TagProvider = (*storeGRPC)(nil) // NewStoreGRPC creates a new gRPC-based annotation store client -func NewStoreGRPC(conn grpc.ClientConnInterface) *storeGRPC { +func NewStoreGRPC(conn *grpc.ClientConn) *storeGRPC { return &storeGRPC{ client: storev1.NewAnnotationStoreClient(conn), + conn: conn, } } +// Close shuts down the underlying gRPC connection +func (s *storeGRPC) Close() error { + if s.conn == nil { + return nil + } + return s.conn.Close() +} + func (s *storeGRPC) Get(ctx context.Context, namespace, name string) (*annotationV0.Annotation, error) { req := &storev1.GetRequest{ Namespace: namespace, @@ -212,11 +222,11 @@ func mapGRPCError(err error) error { switch st.Code() { case codes.NotFound: - return ErrNotFound + return fmt.Errorf("%w: %s", ErrNotFound, st.Message()) case codes.AlreadyExists: - return fmt.Errorf("annotation already exists") + return fmt.Errorf("%w: %s", ErrAlreadyExists, st.Message()) case codes.InvalidArgument: - return fmt.Errorf("invalid argument: %s", st.Message()) + return fmt.Errorf("%w: %s", ErrInvalidInput, st.Message()) default: return fmt.Errorf("grpc error: %s", st.Message()) } @@ -228,8 +238,13 @@ func mapToGRPCStatus(err error) error { return nil } - if errors.Is(err, ErrNotFound) { + switch { + case errors.Is(err, ErrNotFound): return status.Error(codes.NotFound, err.Error()) + case errors.Is(err, ErrAlreadyExists): + return status.Error(codes.AlreadyExists, err.Error()) + case errors.Is(err, ErrInvalidInput): + return status.Error(codes.InvalidArgument, err.Error()) } return status.Error(codes.Internal, err.Error()) diff --git a/pkg/registry/apps/annotation/instrumented_store.go b/pkg/registry/apps/annotation/instrumented_store.go new file mode 100644 index 0000000000000..1d708466cfd9a --- /dev/null +++ b/pkg/registry/apps/annotation/instrumented_store.go @@ -0,0 +1,107 @@ +package annotation + +import ( + "context" + "time" + + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" + + annotationV0 "github.com/grafana/grafana/apps/annotation/pkg/apis/annotation/v0alpha1" + "github.com/grafana/grafana/pkg/infra/log" +) + +// instrumentedStore decorates any Store with tracing, logging, and metrics. +type instrumentedStore struct { + innerStore Store + tracer trace.Tracer + metrics *Metrics + logger log.Logger +} + +var _ Store = (*instrumentedStore)(nil) + +func newInstrumentedStore(innerStore Store, tracer trace.Tracer, metrics *Metrics, logger log.Logger) *instrumentedStore { + return &instrumentedStore{ + innerStore: innerStore, + tracer: tracer, + metrics: metrics, + logger: logger, + } +} + +func (s *instrumentedStore) Get(ctx context.Context, namespace, name string) (out *annotationV0.Annotation, err error) { + ctx, span := s.tracer.Start(ctx, "annotation.store.get", trace.WithAttributes( + attribute.String("namespace", namespace), + attribute.String("name", name), + )) + defer span.End() + start := time.Now() + defer func() { observe(ctx, s.logger, s.metrics.StoreOperationDuration, "get", start, err) }() + + return s.innerStore.Get(ctx, namespace, name) +} + +func (s *instrumentedStore) List(ctx context.Context, namespace string, opts ListOptions) (out *AnnotationList, err error) { + attrs := []attribute.KeyValue{ + attribute.String("namespace", namespace), + attribute.Int64("limit", opts.Limit), + attribute.Bool("has_continue", opts.Continue != ""), + } + if opts.DashboardUID != "" { + attrs = append(attrs, attribute.String("dashboard_uid", opts.DashboardUID)) + } + if opts.PanelID != 0 { + attrs = append(attrs, attribute.Int64("panel_id", opts.PanelID)) + } + ctx, span := s.tracer.Start(ctx, "annotation.store.list", trace.WithAttributes(attrs...)) + defer span.End() + start := time.Now() + defer func() { + if out != nil { + span.SetAttributes(attribute.Int("item_count", len(out.Items))) + } + observe(ctx, s.logger, s.metrics.StoreOperationDuration, "list", start, err) + }() + + return s.innerStore.List(ctx, namespace, opts) +} + +func (s *instrumentedStore) Create(ctx context.Context, anno *annotationV0.Annotation) (out *annotationV0.Annotation, err error) { + ctx, span := s.tracer.Start(ctx, "annotation.store.create", trace.WithAttributes( + attribute.String("namespace", anno.Namespace), + )) + defer span.End() + start := time.Now() + defer func() { observe(ctx, s.logger, s.metrics.StoreOperationDuration, "create", start, err) }() + + return s.innerStore.Create(ctx, anno) +} + +func (s *instrumentedStore) Update(ctx context.Context, anno *annotationV0.Annotation) (out *annotationV0.Annotation, err error) { + ctx, span := s.tracer.Start(ctx, "annotation.store.update", trace.WithAttributes( + attribute.String("namespace", anno.Namespace), + attribute.String("name", anno.Name), + )) + defer span.End() + start := time.Now() + defer func() { observe(ctx, s.logger, s.metrics.StoreOperationDuration, "update", start, err) }() + + return s.innerStore.Update(ctx, anno) +} + +func (s *instrumentedStore) Delete(ctx context.Context, namespace, name string) (err error) { + ctx, span := s.tracer.Start(ctx, "annotation.store.delete", trace.WithAttributes( + attribute.String("namespace", namespace), + attribute.String("name", name), + )) + defer span.End() + start := time.Now() + defer func() { observe(ctx, s.logger, s.metrics.StoreOperationDuration, "delete", start, err) }() + + return s.innerStore.Delete(ctx, namespace, name) +} + +func (s *instrumentedStore) Close() error { + return s.innerStore.Close() +} diff --git a/pkg/registry/apps/annotation/instrumented_store_test.go b/pkg/registry/apps/annotation/instrumented_store_test.go new file mode 100644 index 0000000000000..92ee835fbd40f --- /dev/null +++ b/pkg/registry/apps/annotation/instrumented_store_test.go @@ -0,0 +1,171 @@ +package annotation + +import ( + "context" + "errors" + "testing" + + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + otelcodes "go.opentelemetry.io/otel/codes" + tracesdk "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + + annotationV0 "github.com/grafana/grafana/apps/annotation/pkg/apis/annotation/v0alpha1" + "github.com/grafana/grafana/pkg/infra/log" +) + +// fakeStore lets tests inject specific Get/List/Create/Update/Delete results so +// observeOp can be exercised across the full status-label space. +type fakeStore struct { + getErr error + getRes *annotationV0.Annotation +} + +func (f *fakeStore) Get(_ context.Context, _, _ string) (*annotationV0.Annotation, error) { + return f.getRes, f.getErr +} + +func (f *fakeStore) List(_ context.Context, _ string, _ ListOptions) (*AnnotationList, error) { + return &AnnotationList{}, nil +} + +func (f *fakeStore) Create(_ context.Context, a *annotationV0.Annotation) (*annotationV0.Annotation, error) { + return a, nil +} + +func (f *fakeStore) Update(_ context.Context, a *annotationV0.Annotation) (*annotationV0.Annotation, error) { + return a, nil +} + +func (f *fakeStore) Delete(_ context.Context, _, _ string) error { + return nil +} + +func (f *fakeStore) Close() error { return nil } + +// newTestInstrumentedStore wires a real (in-memory) tracer + prometheus +// registry so tests can read back what the instrumentation actually emitted. +func newTestInstrumentedStore(t *testing.T, inner Store) (*instrumentedStore, *tracetest.SpanRecorder, prometheus.Gatherer) { + t.Helper() + recorder := tracetest.NewSpanRecorder() + tp := tracesdk.NewTracerProvider(tracesdk.WithSpanProcessor(recorder)) + t.Cleanup(func() { _ = tp.Shutdown(context.Background()) }) + + reg := prometheus.NewRegistry() + m := ProvideMetrics(reg) + store := newInstrumentedStore(inner, tp.Tracer("annotation-test"), m, log.NewNopLogger()) + return store, recorder, reg +} + +func TestInstrumentedStore_RecordsSuccessfulCall(t *testing.T) { + store, recorder, reg := newTestInstrumentedStore(t, &fakeStore{getRes: &annotationV0.Annotation{}}) + + _, err := store.Get(context.Background(), "ns", "n") + require.NoError(t, err) + + spans := recorder.Ended() + require.Len(t, spans, 1) + assert.Equal(t, "annotation.store.get", spans[0].Name()) + assert.Equal(t, otelcodes.Unset, spans[0].Status().Code, "successful op should not set Error status") + assert.Empty(t, spans[0].Events(), "successful op should not record any error events") + + count := histogramSampleCount(t, reg, "grafana_annotations_store_operation_duration_seconds", map[string]string{ + "operation": "get", + "status": "ok", + }) + assert.Equal(t, uint64(1), count) +} + +func TestInstrumentedStore_ExpectedClientErrorAttachesButDoesNotFail(t *testing.T) { + store, recorder, reg := newTestInstrumentedStore(t, &fakeStore{getErr: ErrNotFound}) + + _, err := store.Get(context.Background(), "ns", "missing") + require.ErrorIs(t, err, ErrNotFound) + + spans := recorder.Ended() + require.Len(t, spans, 1) + assert.Equal(t, otelcodes.Unset, spans[0].Status().Code, + "expected client errors must NOT mark span as Error — keeps trace error rates clean") + require.NotEmpty(t, spans[0].Events(), + "expected client errors must still appear on the span via AddEvent so traces show the outcome") + assert.Equa ... [truncated]