Information Disclosure / Improper Error Handling (NotFound leakage)

PoC (proof of concept) - information-disclosure through NotFound behavior in annotation endpoints Prerequisites: - Grafana 12.x deployment containing the vulnerable error handling (prior to this patch). - Two user contexts: - userA: has read access to some resources but not the specific target resource - userB: limited or no access to the resource namespace - Access to the Grafana HTTP API (REST) and/or gRPC endpoints for annotations Steps to test (REST): 1) Obtain tokens for userA and userB. 2) Attempt to GET a non-existent annotation in a namespace where at least some resources exist, for both users. - curl -s -D - -H "Authorization: Bearer $TOKEN_A" https://grafana.example/api/annotations/default/does-not-exist - curl -s -D - -H "Authorization: Bearer $TOKEN_B" https://grafana.example/api/annotations/default/does-not-exist 3) Observe HTTP status codes and messages. - Expected post-fix behavior (this patch): both requests return 404 Not Found with uniform messaging that does not reveal resource existence beyond the not-found condition. - Before the fix, userA might have seen a not-found vs a forbidden or a more descriptive message depending on access rights, potentially leaking whether the resource existed. Steps to test (gRPC): 1) Use a gRPC client against the annotation service with both users. 2) Call the Get method for a non-existent annotation. 3) Verify that the returned error is a NotFound rather than leaking additional details or a mix of error strings, and that the error contains only generic not-found semantics about the resource. Expected outcome after the patch: - Non-existent resources yield a NotFound response consistently for both authenticated users, regardless of their per-resource permissions, reducing information disclosure about resource existence. Some flows also convert internal NotFound into Kubernetes API NotFound for Kubernetes-backed adapters to avoid leaking existence information.

diff --git a/pkg/registry/apps/annotation/grpc_store.go b/pkg/registry/apps/annotation/grpc_store.go index 241612c0a2556..267391d8716ab 100644 --- a/pkg/registry/apps/annotation/grpc_store.go +++ b/pkg/registry/apps/annotation/grpc_store.go @@ -2,8 +2,8 @@ package annotation import ( "context" + "errors" "fmt" - "strings" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -211,7 +211,7 @@ func mapGRPCError(err error) error { switch st.Code() { case codes.NotFound: - return fmt.Errorf("annotation not found") + return ErrNotFound case codes.AlreadyExists: return fmt.Errorf("annotation already exists") case codes.InvalidArgument: @@ -227,19 +227,11 @@ func mapToGRPCStatus(err error) error { return nil } - msg := err.Error() - - if strings.Contains(msg, "not found") { - return status.Error(codes.NotFound, msg) - } - if strings.Contains(msg, "already exists") { - return status.Error(codes.AlreadyExists, msg) - } - if strings.Contains(msg, "invalid") { - return status.Error(codes.InvalidArgument, msg) + if errors.Is(err, ErrNotFound) { + return status.Error(codes.NotFound, err.Error()) } - return status.Error(codes.Internal, msg) + return status.Error(codes.Internal, err.Error()) } // toProtoAnnotation converts a v0alpha1.Annotation to proto Annotation diff --git a/pkg/registry/apps/annotation/k8s_adapter.go b/pkg/registry/apps/annotation/k8s_adapter.go index 26b04c28c41e5..0249a3f45259b 100644 --- a/pkg/registry/apps/annotation/k8s_adapter.go +++ b/pkg/registry/apps/annotation/k8s_adapter.go @@ -2,6 +2,7 @@ package annotation import ( "context" + "errors" "fmt" "strconv" @@ -19,6 +20,8 @@ import ( "github.com/grafana/grafana/pkg/util" ) +var annotationGR = annotationV0.AnnotationKind().GroupVersionResource().GroupResource() + var ( _ rest.Scoper = (*k8sRESTAdapter)(nil) _ rest.SingularNameProvider = (*k8sRESTAdapter)(nil) @@ -162,6 +165,9 @@ func (s *k8sRESTAdapter) Get(ctx context.Context, name string, options *metav1.G annotation, err := s.store.Get(ctx, namespace, name) if err != nil { + if errors.Is(err, ErrNotFound) { + return nil, apierrors.NewNotFound(annotationGR, name) + } return nil, err } @@ -171,9 +177,7 @@ func (s *k8sRESTAdapter) Get(ctx context.Context, name string, options *metav1.G } if !allowed { // Return NotFound to avoid leaking existence. - return nil, apierrors.NewNotFound( - annotationV0.AnnotationKind().GroupVersionResource().GroupResource(), name, - ) + return nil, apierrors.NewNotFound(annotationGR, name) } return annotation, nil @@ -196,10 +200,7 @@ func (s *k8sRESTAdapter) Create(ctx context.Context, return nil, err } if !allowed { - return nil, apierrors.NewForbidden( - annotationV0.AnnotationKind().GroupVersionResource().GroupResource(), - annotation.Name, fmt.Errorf("insufficient permissions"), - ) + return nil, apierrors.NewForbidden(annotationGR, annotation.Name, fmt.Errorf("insufficient permissions")) } if annotation.Name == "" && annotation.GenerateName == "" { @@ -226,6 +227,9 @@ func (s *k8sRESTAdapter) Update(ctx context.Context, // Fetch the existing annotation for patch merging and to verify authz on the pre-update resource. existing, err := s.store.Get(ctx, namespace, name) if err != nil { + if errors.Is(err, ErrNotFound) { + return nil, false, apierrors.NewNotFound(annotationGR, name) + } return nil, false, err } @@ -253,20 +257,14 @@ func (s *k8sRESTAdapter) Update(ctx context.Context, return nil, false, err } if !allowed { - return nil, false, apierrors.NewForbidden( - annotationV0.AnnotationKind().GroupVersionResource().GroupResource(), - existing.Name, fmt.Errorf("insufficient permissions"), - ) + return nil, false, apierrors.NewForbidden(annotationGR, existing.Name, fmt.Errorf("insufficient permissions")) } allowed, err = canAccessAnnotation(ctx, s.accessClient, namespace, resource, utils.VerbUpdate) if err != nil { return nil, false, err } if !allowed { - return nil, false, apierrors.NewForbidden( - annotationV0.AnnotationKind().GroupVersionResource().GroupResource(), - resource.Name, fmt.Errorf("insufficient permissions"), - ) + return nil, false, apierrors.NewForbidden(annotationGR, resource.Name, fmt.Errorf("insufficient permissions")) } updated, err := s.store.Update(ctx, resource) @@ -282,6 +280,9 @@ func (s *k8sRESTAdapter) Delete(ctx context.Context, name string, deleteValidati annotation, err := s.store.Get(ctx, namespace, name) if err != nil { + if errors.Is(err, ErrNotFound) { + return nil, false, apierrors.NewNotFound(annotationGR, name) + } return nil, false, err } @@ -296,17 +297,15 @@ func (s *k8sRESTAdapter) Delete(ctx context.Context, name string, deleteValidati return nil, false, err } if !allowedRead { - return nil, false, apierrors.NewNotFound( - annotationV0.AnnotationKind().GroupVersionResource().GroupResource(), name, - ) + return nil, false, apierrors.NewNotFound(annotationGR, name) } - return nil, false, apierrors.NewForbidden( - annotationV0.AnnotationKind().GroupVersionResource().GroupResource(), - name, fmt.Errorf("insufficient permissions"), - ) + return nil, false, apierrors.NewForbidden(annotationGR, name, fmt.Errorf("insufficient permissions")) } err = s.store.Delete(ctx, namespace, name) + if errors.Is(err, ErrNotFound) { + return nil, false, apierrors.NewNotFound(annotationGR, name) + } return nil, false, err } diff --git a/pkg/registry/apps/annotation/memory_store.go b/pkg/registry/apps/annotation/memory_store.go index 99ce5d0cb532a..b3326e00dc7e6 100644 --- a/pkg/registry/apps/annotation/memory_store.go +++ b/pkg/registry/apps/annotation/memory_store.go @@ -30,7 +30,7 @@ func (m *memoryStore) Get(ctx context.Context, namespace, name string) (*annotat key := namespace + "/" + name anno, ok := m.data[key] if !ok { - return nil, fmt.Errorf("annotation not found") + return nil, ErrNotFound } return anno.DeepCopy(), nil @@ -175,7 +175,7 @@ func (m *memoryStore) Update(ctx context.Context, anno *annotationV0.Annotation) key := anno.Namespace + "/" + anno.Name if _, exists := m.data[key]; !exists { - return nil, fmt.Errorf("annotation not found") + return nil, ErrNotFound } updated := anno.DeepCopy() @@ -191,7 +191,7 @@ func (m *memoryStore) Delete(ctx context.Context, namespace, name string) error key := namespace + "/" + name if _, exists := m.data[key]; !exists { - return fmt.Errorf("annotation not found") + return ErrNotFound } delete(m.data, key) diff --git a/pkg/registry/apps/annotation/postgres_partitioned.go b/pkg/registry/apps/annotation/postgres_partitioned.go index c9ea731db24a6..f3cba9c743a5e 100644 --- a/pkg/registry/apps/annotation/postgres_partitioned.go +++ b/pkg/registry/apps/annotation/postgres_partitioned.go @@ -16,10 +16,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -var ( - ErrNotFound = errors.New("annotation not found") -) - const ( defaultMaxConnections = 10 defaultMaxIdleConns = 5 diff --git a/pkg/registry/apps/annotation/register_test.go b/pkg/registry/apps/annotation/register_test.go index 69d853b10e0f8..d9e7161df775d 100644 --- a/pkg/registry/apps/annotation/register_test.go +++ b/pkg/registry/apps/annotation/register_test.go @@ -7,6 +7,7 @@ import ( authtypes "github.com/grafana/authlib/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8srequest "k8s.io/apiserver/pkg/endpoints/request" @@ -265,3 +266,41 @@ func TestK8sRESTAdapter_TenantIsolation(t *testing.T) { require.Error(t, err) }) } + +func TestK8sRESTAdapter_NotFound(t *testing.T) { + store := NewMemoryStore() + adapter := &k8sRESTAdapter{ + store: store, + accessClient: authtypes.FixedAccessClient(true), + } + + ctx := k8srequest.WithNamespace(identity.WithServiceIdentityContext(t.Context(), 1), "default") + + t.Run("get returns k8s NotFound for nonexistent annotation", func(t *testing.T) { + _, err := adapter.Get(ctx, "does-not-exist", nil) + require.Error(t, err) + assert.True(t, apierrors.IsNotFound(err), "expected IsNotFound, got: %v", err) + }) + + t.Run("update returns k8s NotFound for nonexistent annotation", func(t *testing.T) { + updated := &annotationV0.Annotation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-exist", + Namespace: "default", + }, + Spec: annotationV0.AnnotationSpec{ + Text: "updated text", + Time: 12345, + }, + } + _, _, err := adapter.Update(ctx, "does-not-exist", rest.DefaultUpdatedObjectInfo(updated), nil, nil, false, nil) + require.Error(t, err) + assert.True(t, apierrors.IsNotFound(err), "expected IsNotFound, got: %v", err) + }) + + t.Run("delete returns k8s NotFound for nonexistent annotation", func(t *testing.T) { + _, _, err := adapter.Delete(ctx, "does-not-exist", nil, nil) + require.Error(t, err) + assert.True(t, apierrors.IsNotFound(err), "expected IsNotFound, got: %v", err) + }) +} diff --git a/pkg/registry/apps/annotation/sql_adapter.go b/pkg/registry/apps/annotation/sql_adapter.go index 94cae993fd72b..40c01a7b253bd 100644 --- a/pkg/registry/apps/annotation/sql_adapter.go +++ b/pkg/registry/apps/annotation/sql_adapter.go @@ -63,7 +63,7 @@ func (a *sqlAdapter) Get(ctx context.Context, namespace, name string) (*annotati } } - return nil, apierrors.NewNotFound(annotationV0.AnnotationKind().GroupVersionResource().GroupResource(), name) + return nil, apierrors.NewNotFound(annotationGR, name) } func (a *sqlAdapter) List(ctx context.Context, namespace string, opts ListOptions) (*AnnotationList, error) { diff --git a/pkg/registry/apps/annotation/storage.go b/pkg/registry/apps/annotation/storage.go index e982f0d594048..137d1feca762f 100644 --- a/pkg/registry/apps/annotation/storage.go +++ b/pkg/registry/apps/annotation/storage.go @@ -2,10 +2,14 @@ package annotation import ( "context" + "errors" annotationV0 "github.com/grafana/grafana/apps/annotation/pkg/apis/annotation/v0alpha1" ) +// ErrNotFound is returned by Store implementations when the requested annotation does not exist. +var ErrNotFound = errors.New("annotation not found") + type Store interface { Get(ctx context.Context, namespace, name string) (*annotationV0.Annotation, error) List(ctx context.Context, namespace string, opts ListOptions) (*AnnotationList, error)