Input Validation

Proof-of-concept exploit (pre-fix behavior): An attacker could submit an annotation with a time far in the future (e.g., now + 10 days) or a timeEnd that is before time. Before this patch, the API accepted such timestamps, potentially causing retention misalignment or data integrity issues. The following illustrative examples assume access to Grafana's annotation API (authenticated and namespace-scoped): 1) Create annotation with a far-future time (vulnerability example): - Endpoint: POST https://grafana.example/api/annotations (or the appropriate Kubernetes-backed API path used by Grafana for annotations) - Headers: Authorization: Bearer <token>, Content-Type: application/json - Payload (illustrative): { "metadata": {"name": "poc-annotation", "namespace": "org-1"}, "spec": {"time": 1700000000000 + 365*24*3600*1000, "timeEnd": null, "text": "poc"} } Expected (pre-fix): The server would accept the request and store the annotation with a timestamp far in the future, which could lead to misaligned retention plans, partitioning issues, or future-dated events appearing in analytics. 2) Create annotation with timeEnd before time (vulnerability example): Payload: {"metadata": {"name": "poc-annotation2", "namespace": "org-1"}, "spec": {"time": now_ms, "timeEnd": now_ms - 1000, "text": "poc"}} Expected (pre-fix): If not validated, this could store an invalid timeEnd relationship which may confuse clients or cause data integrity issues. Mitigation demonstrated by the fix: The patch introduces API-layer validation that rejects such inputs with 400 Bad Request and explanatory messages like time cannot be more than 1 week in the future or timeEnd must be after time. It also enforces a retention TTL boundary to prevent accepting timestamps older than the configured retention window. Note: After applying the fix, the above requests would be rejected by validation (e.g., 400 Bad Request) with messages indicating the specific timestamp constraint violation.

diff --git a/pkg/registry/apps/annotation/authz_test.go b/pkg/registry/apps/annotation/authz_test.go index 917fd371012b9..3d3e2a53eb70a 100644 --- a/pkg/registry/apps/annotation/authz_test.go +++ b/pkg/registry/apps/annotation/authz_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "testing" + "time" authtypes "github.com/grafana/authlib/types" "github.com/stretchr/testify/assert" @@ -36,6 +37,8 @@ func newTestAdapter(store Store, ac authtypes.AccessClient, fr ...DashboardFolde tracer: tracing.InitializeTracerForTest(), logger: log.NewNopLogger(), metrics: ProvideMetrics(nil), + // use a large retention window in case of long-running tests. + retentionTTL: 200 * 365 * 24 * time.Hour, } } diff --git a/pkg/registry/apps/annotation/k8s_adapter.go b/pkg/registry/apps/annotation/k8s_adapter.go index 409da8c4fb396..57765346aaa89 100644 --- a/pkg/registry/apps/annotation/k8s_adapter.go +++ b/pkg/registry/apps/annotation/k8s_adapter.go @@ -34,6 +34,10 @@ var annotationGR = annotationV0.AnnotationKind().GroupVersionResource().GroupRes // This follows the convention in pkg/storage/unified/apistore/prepare.go. const maxSafeJSInt = (1 << 52) - 1 +// maxFutureWindow bounds how far ahead of now an annotation's time (or timeEnd) may be set. +// TODO: determine appropriate future bound and maybe make configurable. +const maxFutureWindow = 7 * 24 * time.Hour + // toAPIError maps store-layer sentinels to the right k8s apierror so HTTP // status + telemetry classification agree. Already-typed apierrors and unknown // errors pass through unchanged (the apiserver will wrap the latter as 500). @@ -83,6 +87,11 @@ type k8sRESTAdapter struct { // rejected by the settings loader. maxScopeCount int + // retentionTTL bounds how far in the past an annotation's time may be, + // matching the cleanup window so we don't accept data that would be + // immediately purged. + retentionTTL time.Duration + tracer trace.Tracer metrics *Metrics logger log.Logger @@ -216,24 +225,22 @@ func (s *k8sRESTAdapter) Create(ctx context.Context, return nil, apierrors.NewInternalError(fmt.Errorf("expected *Annotation, got %T", obj)) } - allowed, err := canAccessAnnotation(ctx, s.accessClient, s.folderResolver, namespace, annotation, utils.VerbCreate) + err = s.validateAnnotation(annotation) if err != nil { return nil, err } - if !allowed { - return nil, apierrors.NewForbidden(annotationGR, annotation.Name, fmt.Errorf("insufficient permissions")) - } - if annotation.Name == "" && annotation.GenerateName == "" { - return nil, apierrors.NewBadRequest("metadata.name or metadata.generateName is required") - } if annotation.Name == "" && annotation.GenerateName != "" { annotation.Name = annotation.GenerateName + util.GenerateShortUID() } - if err := s.validateScopeCount(annotation); err != nil { + allowed, err := canAccessAnnotation(ctx, s.accessClient, s.folderResolver, namespace, annotation, utils.VerbCreate) + if err != nil { return nil, err } + if !allowed { + return nil, apierrors.NewForbidden(annotationGR, annotation.Name, fmt.Errorf("insufficient permissions")) + } user, err := identity.GetRequester(ctx) if err != nil { @@ -411,6 +418,18 @@ func parseFieldSelector(fs fields.Selector, opts *ListOptions) error { return nil } +func (s *k8sRESTAdapter) validateAnnotation(anno *annotationV0.Annotation) error { + if err := s.validateScopeCount(anno); err != nil { + return err + } + + if err := s.validateTimes(anno); err != nil { + return err + } + + return validateNames(anno) +} + func (s *k8sRESTAdapter) validateScopeCount(a *annotationV0.Annotation) error { if len(a.Spec.Scopes) > s.maxScopeCount { return apierrors.NewBadRequest(fmt.Sprintf( @@ -418,3 +437,39 @@ func (s *k8sRESTAdapter) validateScopeCount(a *annotationV0.Annotation) error { } return nil } + +func (s *k8sRESTAdapter) validateTimes(anno *annotationV0.Annotation) error { + now := time.Now().UTC() + maxFuture := now.Add(maxFutureWindow).UnixMilli() + maxPast := now.Add(-s.retentionTTL).UnixMilli() + + if anno.Spec.Time > maxFuture { + return apierrors.NewBadRequest( + fmt.Sprintf("%v: time cannot be more than 1 week in the future", ErrInvalidInput)) + } + if anno.Spec.Time < maxPast { + return apierrors.NewBadRequest( + fmt.Sprintf("%v: time cannot be older than retention TTL (%v)", ErrInvalidInput, s.retentionTTL)) + } + + // If timeEnd is set, validate it's after time and within future bounds + if anno.Spec.TimeEnd != nil { + if *anno.Spec.TimeEnd < anno.Spec.Time { + return apierrors.NewBadRequest(fmt.Sprintf("%v: timeEnd must be after time", ErrInvalidInput)) + } + if *anno.Spec.TimeEnd > maxFuture { + return apierrors.NewBadRequest( + fmt.Sprintf("%v: timeEnd cannot be more than 1 week in the future", ErrInvalidInput)) + } + } + + return nil +} + +func validateNames(anno *annotationV0.Annotation) error { + if anno.Name == "" && anno.GenerateName == "" { + return apierrors.NewBadRequest("metadata.name or metadata.generateName is required") + } + + return nil +} diff --git a/pkg/registry/apps/annotation/k8s_adapter_test.go b/pkg/registry/apps/annotation/k8s_adapter_test.go index eab85999e8bfc..095d4faf7a67f 100644 --- a/pkg/registry/apps/annotation/k8s_adapter_test.go +++ b/pkg/registry/apps/annotation/k8s_adapter_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "testing" + "time" "github.com/bwmarrin/snowflake" authtypes "github.com/grafana/authlib/types" @@ -538,6 +539,65 @@ func TestK8sAdapter_MaxScopeCount(t *testing.T) { }) } +// TestK8sAdapter_ValidateAnnotation pins the time-bounds validation applied on annotation.time for the Create function. +func TestK8sAdapter_ValidateAnnotation(t *testing.T) { + ns := "org-1" + allowAll := &fakeAccessClient{fn: func(_ authtypes.BatchCheckItem) bool { return true }} + + const retentionTTL = 90 * 24 * time.Hour + now := time.Now().UTC().UnixMilli() + second := time.Second.Milliseconds() + futureWindowMs := maxFutureWindow.Milliseconds() + retentionMs := retentionTTL.Milliseconds() + + timeEnd := func(ms int64) *int64 { return &ms } + + cases := []struct { + name string + time int64 + timeEnd *int64 + expectErr bool + errContains string + }{ + {"time is current", now, nil, false, ""}, + {"recent past within retention", now - retentionMs/2, nil, false, ""}, + {"inside future bound", now + futureWindowMs - second, nil, false, ""}, + {"too far in the future", now + futureWindowMs + second, nil, true, "time cannot be more than 1 week in the future"}, + {"older than retention TTL", now - retentionMs - second, nil, true, "time cannot be older than retention TTL"}, + {"valid timeEnd after time", now, timeEnd(now + second), false, ""}, + {"timeEnd before time", now, timeEnd(now - second), true, "timeEnd must be after time"}, + {"timeEnd too far in the future", now, timeEnd(now + futureWindowMs + second), true, "timeEnd cannot be more than 1 week in the future"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + store := NewMemoryStore() + adapter := newTestAdapter(store, allowAll) + adapter.retentionTTL = retentionTTL + ctx := k8srequest.WithNamespace(identity.WithServiceIdentityContext(t.Context(), 1), ns) + + name := "anno" + obj := &annotationV0.Annotation{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: annotationV0.AnnotationSpec{Text: "test", Time: tc.time, TimeEnd: tc.timeEnd}, + } + _, err := adapter.Create(ctx, obj, nil, &metav1.CreateOptions{}) + + if !tc.expectErr { + require.NoError(t, err) + return + } + + require.Error(t, err) + assert.True(t, apierrors.IsBadRequest(err), "expected 400 BadRequest, got %v", err) + assert.Contains(t, err.Error(), tc.errContains) + + _, getErr := store.Get(ctx, ns, name) + assert.ErrorIs(t, getErr, ErrNotFound, "invalid annotation should not have been persisted") + }) + } +} + // compile-time assertion that errStore implements Store var _ Store = (*errStore)(nil) diff --git a/pkg/registry/apps/annotation/memory_store.go b/pkg/registry/apps/annotation/memory_store.go index a4d7f0a8aa04f..9a0d175f63bb5 100644 --- a/pkg/registry/apps/annotation/memory_store.go +++ b/pkg/registry/apps/annotation/memory_store.go @@ -6,7 +6,6 @@ import ( "strings" "sync" - "github.com/google/uuid" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -157,10 +156,6 @@ func (m *memoryStore) Create(ctx context.Context, anno *annotationV0.Annotation) m.mu.Lock() defer m.mu.Unlock() - if anno.Name == "" { - anno.Name = uuid.New().String() - } - key := anno.Namespace + "/" + anno.Name if _, exists := m.data[key]; exists { diff --git a/pkg/registry/apps/annotation/postgres_partitioned.go b/pkg/registry/apps/annotation/postgres_partitioned.go index 62fb49bfe88ca..9486a8581d8db 100644 --- a/pkg/registry/apps/annotation/postgres_partitioned.go +++ b/pkg/registry/apps/annotation/postgres_partitioned.go @@ -30,7 +30,6 @@ type PostgreSQLStoreConfig struct { MaxConnections int MaxIdleConns int ConnMaxLifetime time.Duration - RetentionTTL time.Duration TagCacheTTL time.Duration TagCacheSize int } @@ -161,10 +160,6 @@ func (s *PostgreSQLStore) Get(ctx context.Context, namespace, name string) (*ann // Create creates a new annotation func (s *PostgreSQLStore) Create(ctx context.Context, anno *annotationV0.Annotation) (*annotationV0.Annotation, error) { - if err := s.validateAnnotation(anno); err != nil { - return nil, err - } - // Ensure partition exists for this timestamp if err := ensurePartition(ctx, s.pool, s.logger, anno.Spec.Time); err != nil { return nil, fmt.Errorf("failed to ensure partition: %w", err) @@ -466,29 +461,3 @@ func rowToAnnotation(namespace, name string, timeMs int64, timeEnd *int64, return anno } - -func (s *PostgreSQLStore) validateAnnotation(anno *annotationV0.Annotation) error { - now := time.Now().UTC() - // TODO: determine appropriate future bound and maybe make configurable - maxFuture := now.Add(7 * 24 * time.Hour).UnixMilli() - maxPast := now.Add(-s.config.RetentionTTL).UnixMilli() - - if anno.Spec.Time > maxFuture { - return fmt.Errorf("%w: time cannot be more than 1 week in the future", ErrInvalidInput) - } - if anno.Spec.Time < maxPast { - return fmt.Errorf("%w: time cannot be older than retention TTL (%v)", ErrInvalidInput, s.config.RetentionTTL) - } - - // If timeEnd is set, validate it's after time and within future bounds - if anno.Spec.TimeEnd != nil { - if *anno.Spec.TimeEnd < anno.Spec.Time { - return fmt.Errorf("%w: timeEnd must be after time", ErrInvalidInput) - } - if *anno.Spec.TimeEnd > maxFuture { - return fmt.Errorf("%w: timeEnd cannot be more than 1 week in the future", ErrInvalidInput) - } - } - - return nil -} diff --git a/pkg/registry/apps/annotation/register.go b/pkg/registry/apps/annotation/register.go index 3d5d973e16d3e..208475030cca7 100644 --- a/pkg/registry/apps/annotation/register.go +++ b/pkg/registry/apps/annotation/register.go @@ -125,6 +125,7 @@ func NewAppInstaller( installer: installer, snowflakeNode: sfNode, maxScopeCount: cfg.MaxScopeCount, + retentionTTL: cfg.RetentionTTL, tracer: installer.tracer, metrics: installer.metrics, logger: logger, @@ -232,7 +233,6 @@ func newPostgresStore(ctx context.Context, cfg Config, m *Metrics) (Store, error MaxConnections: cfg.PostgresMaxConnections, MaxIdleConns: cfg.PostgresMaxIdleConns, ConnMaxLifetime: cfg.PostgresConnMaxLifetime, - RetentionTTL: cfg.RetentionTTL, TagCacheTTL: cfg.PostgresTagCacheTTL, TagCacheSize: cfg.PostgresTagCacheSize, } diff --git a/pkg/tests/apis/annotations/annotations_test.go b/pkg/tests/apis/annotations/annotations_test.go index be8a0f5f13132..194af2899002c 100644 --- a/pkg/tests/apis/annotations/annotations_test.go +++ b/pkg/tests/apis/annotations/annotations_test.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "testing" + "time" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -61,7 +62,7 @@ func newAnnotation(text string, tags ...string) *unstructured.Unstructured { }, "spec": map[string]any{ "text": text, - "time": int64(1700000000000), + "time": time.Now().UnixMilli(), "tags": tagSlice, }, },