Input validation / metadata.name derivation consistency for global variables

Proof of concept (actionable steps): Prerequisites: - Grafana instance containing the global variable API for dashboard variables (v2beta1) as patched by this commit. - An authenticated client token with permissions to create Variables. - A folder with UID "folder-z" existing in the cluster. 1) Attempt to create a global variable with a mismatched metadata.name: POST /apis/dashboard.k8s.io/v2beta1/Variable Content-Type: application/json Authorization: Bearer <token> { "metadata": { "name": "status", // intentionally not matching derived name "annotations": { "grafana.app/folder": "folder-z" } }, "spec": { "spec": { "name": "status" } } } Expected (pre-fix behavior on vulnerable versions): The server would accept this payload and create the variable with metadata.name "status" even though the derived value would be "status--folder-z". Expected (patched behavior): The server rejects with a BadRequest explaining that metadata.name does not match the derived value for the given spec and folder (or that omission is required to allow server-side derivation). 2) Create with metadata.name omitted to allow server-side derivation: POST /apis/dashboard.k8s.io/v2beta1/Variable Content-Type: application/json Authorization: Bearer <token> { "metadata": { "annotations": { "grafana.app/folder": "folder-z" } }, "spec": { "spec": { "name": "status" } } } Expected: The server derives metadata.name as "status--folder-z" and creates the resource if allowed by other validations. 3) Attempt to change the name to a mismatched value on update (to demonstrate protection against name spoofing after creation): PUT /apis/dashboard.k8s.io/v2beta1/Variable/<name> ... payload ... Expected: The update is rejected if the derived name would change or if the new metadata.name does not align with the current spec.folder-derived name (depending on the exact API semantics).

diff --git a/apps/dashboard/kinds/globalvariable.cue b/apps/dashboard/kinds/globalvariable.cue index 55634fb0c0f37..5c95698add675 100644 --- a/apps/dashboard/kinds/globalvariable.cue +++ b/apps/dashboard/kinds/globalvariable.cue @@ -7,6 +7,13 @@ import ( globalVariableV2beta1: { kind: "Variable" pluralName: "Variables" + // API contract: + // - spec.spec.name is the logical variable name input. + // - metadata.annotations["grafana.app/folder"] carries optional folder scope. + // - metadata.name may be omitted on CREATE; the server derives it as: + // - global: spec.spec.name + // - folder: spec.spec.name + "--" + folderUID + // - when metadata.name is provided, it must match the derived value. //TODO: // Adding selectableFields here causes the codegen to fail because it's a union parent path. // Until grafana-app-sdk supports selectableFields through union parent paths, diff --git a/pkg/registry/apis/dashboard/mutate.go b/pkg/registry/apis/dashboard/mutate.go index 5ef9a9b772a8c..8d895d0d56f0d 100644 --- a/pkg/registry/apis/dashboard/mutate.go +++ b/pkg/registry/apis/dashboard/mutate.go @@ -196,6 +196,10 @@ func mutateVariable(a admission.Attributes) error { } variable.SetLabels(labels) + if a.GetOperation() == admission.Create && variable.GetName() == "" { + variable.SetName(deriveVariableMetadataName(getVariableName(variable.Spec), meta.GetFolder())) + } + return nil } diff --git a/pkg/registry/apis/dashboard/register.go b/pkg/registry/apis/dashboard/register.go index 37d197df63d24..d6f6dfd877007 100644 --- a/pkg/registry/apis/dashboard/register.go +++ b/pkg/registry/apis/dashboard/register.go @@ -121,7 +121,6 @@ type DashboardsAPIBuilder struct { minRefreshInterval string dualWriter dualwrite.Service folderClientProvider client.K8sHandlerProvider - variableClientProvider client.K8sHandlerProvider libraryPanels libraryelements.Service // for legacy library panels libraryPanelsEnabled bool publicDashboardService publicdashboards.Service @@ -168,7 +167,6 @@ func RegisterAPIService( dbp := legacysql.NewDatabaseProvider(sql) namespacer := request.GetNamespaceMapper(cfg) folderClient := client.NewK8sHandler(request.GetNamespaceMapper(cfg), folders.FolderResourceInfo.GroupVersionResource(), restConfigProvider.GetRestConfig, userService, unified) - variableClient := client.NewK8sHandler(namespacer, dashv2beta1.VariableResourceInfo.GroupVersionResource(), restConfigProvider.GetRestConfig, userService, unified) dashboardClient := client.NewK8sHandler(namespacer, dashv1.DashboardResourceInfo.GroupVersionResource(), restConfigProvider.GetRestConfig, userService, unified) snapshotOptions := dashv0.SnapshotSharingOptions{ @@ -195,7 +193,6 @@ func RegisterAPIService( dualWriter: dual, dashboardK8sClient: dashboardClient, folderClientProvider: newSimpleClientProvider(folderClient), - variableClientProvider: newSimpleClientProvider(variableClient), libraryPanels: libraryPanels, libraryPanelsEnabled: features.IsEnabledGlobally(featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs), // nolint:staticcheck publicDashboardService: publicDashboardService, @@ -588,6 +585,10 @@ func (b *DashboardsAPIBuilder) validateVariableCreate(ctx context.Context, a adm return fmt.Errorf("error getting variable meta accessor: %w", err) } + if err := validateVariableMetadataName(variable.GetName(), getVariableName(variable.Spec), accessor.GetFolder()); err != nil { + return apierrors.NewBadRequest(err.Error()) + } + if !a.IsDryRun() && accessor.GetFolder() != "" { id, err := identity.GetRequester(ctx) if err != nil { @@ -603,17 +604,6 @@ func (b *DashboardsAPIBuilder) validateVariableCreate(ctx context.Context, a adm } } - if !a.IsDryRun() { - namespace := accessor.GetNamespace() - if namespace == "" { - namespace = a.GetNamespace() - } - - if err := b.validateVariableNameUniqueness(ctx, namespace, variable, variable.GetName()); err != nil { - return apierrors.NewBadRequest(err.Error()) - } - } - return nil } @@ -646,35 +636,12 @@ func (b *DashboardsAPIBuilder) validateVariableUpdate(ctx context.Context, a adm return fmt.Errorf("error getting new variable meta accessor: %w", err) } - if !a.IsDryRun() && newAccessor.GetFolder() != oldAccessor.GetFolder() && newAccessor.GetFolder() != "" { - id, err := identity.GetRequester(ctx) - if err != nil { - return fmt.Errorf("error getting requester: %w", err) - } - - if err := b.verifyFolderAccessPermissions(ctx, id, newAccessor.GetFolder()); err != nil { - return err - } - - nsInfo, err := authlib.ParseNamespace(newAccessor.GetNamespace()) - if err != nil { - return fmt.Errorf("failed to parse namespace: %w", err) - } - - if _, err := b.validateFolderExists(ctx, newAccessor.GetFolder(), nsInfo.OrgID); err != nil { - return apierrors.NewNotFound(folders.FolderResourceInfo.GroupResource(), newAccessor.GetFolder()) - } + if getVariableName(newVariable.Spec) != getVariableName(oldVariable.Spec) { + return apierrors.NewBadRequest("spec.spec.name cannot be changed; delete the variable and create a new one") } - if !a.IsDryRun() { - namespace := newAccessor.GetNamespace() - if namespace == "" { - namespace = a.GetNamespace() - } - - if err := b.validateVariableNameUniqueness(ctx, namespace, newVariable, newVariable.GetName()); err != nil { - return apierrors.NewBadRequest(err.Error()) - } + if newAccessor.GetFolder() != oldAccessor.GetFolder() { + return apierrors.NewBadRequest("folder scope cannot be changed; delete the variable and create a new one") } return nil @@ -948,7 +915,7 @@ func (b *DashboardsAPIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver } storage := apiGroupInfo.VersionedResourcesStorageMap[dashv2beta1.VERSION] - storage[dashv2beta1.VariableResourceInfo.StoragePath()] = newVariableStorage(gvStore, b.variableClientProvider) + storage[dashv2beta1.VariableResourceInfo.StoragePath()] = gvStore } return nil diff --git a/pkg/registry/apis/dashboard/variable.go b/pkg/registry/apis/dashboard/variable.go index f0d6f90dd26d0..34915843be5ac 100644 --- a/pkg/registry/apis/dashboard/variable.go +++ b/pkg/registry/apis/dashboard/variable.go @@ -10,6 +10,8 @@ import ( var variableNameFormat = regexp.MustCompile(`^\w+$`) +const variableMetadataNameMaxLength = 253 + // walkVariableKinds traverses the VariableSpec union in a single pass and // returns the spec name of whichever kind is populated (empty if none) along // with the total number of populated kinds. Centralising the walk here keeps @@ -61,6 +63,39 @@ func getVariableName(spec dashv2beta1.VariableSpec) string { return name } +func deriveVariableMetadataName(specName, folderUID string) string { + if folderUID == "" { + return specName + } + return specName + "--" + folderUID +} + +func validateVariableMetadataName(gotName, specName, folderUID string) error { + expectedName := deriveVariableMetadataName(specName, folderUID) + if len(expectedName) > variableMetadataNameMaxLength { + return fmt.Errorf("derived metadata.name exceeds maximum length (%d)", variableMetadataNameMaxLength) + } + if gotName == "" || gotName == expectedName { + return nil + } + + if folderUID == "" { + return fmt.Errorf( + "metadata.name %q does not match the name required for spec.spec.name %q: expected %q (omit metadata.name to let the server set it)", + gotName, + specName, + expectedName, + ) + } + return fmt.Errorf( + "metadata.name %q does not match the name required for spec.spec.name %q in folder %q: expected %q (omit metadata.name to let the server set it)", + gotName, + specName, + folderUID, + expectedName, + ) +} + func validateVariable(variable *dashv2beta1.Variable) error { if variable == nil { return fmt.Errorf("variable payload is required") diff --git a/pkg/registry/apis/dashboard/variable_mutation_test.go b/pkg/registry/apis/dashboard/variable_mutation_test.go index a5f71da45bc92..a8b7f54363ff7 100644 --- a/pkg/registry/apis/dashboard/variable_mutation_test.go +++ b/pkg/registry/apis/dashboard/variable_mutation_test.go @@ -54,4 +54,23 @@ func TestDashboardsAPIBuilderMutateVariable(t *testing.T) { ), nil) require.NoError(t, err) require.NotContains(t, v.GetLabels(), variableFolderLabelKey) + + // When metadata.name is omitted on create, mutation derives it from spec name + folder. + v = newCustomVariable("status", "") + v.SetAnnotations(map[string]string{utils.AnnoKeyFolder: "folder-z"}) + err = builder.Mutate(context.Background(), admission.NewAttributesRecord( + v, + nil, + dashv2beta1.VariableResourceInfo.GroupVersionKind(), + "stacks-1", + v.GetName(), + dashv2beta1.VariableResourceInfo.GroupVersionResource(), + "", + admission.Create, + &metav1.CreateOptions{}, + false, + nil, + ), nil) + require.NoError(t, err) + require.Equal(t, "status--folder-z", v.GetName()) } diff --git a/pkg/registry/apis/dashboard/variable_storage.go b/pkg/registry/apis/dashboard/variable_storage.go deleted file mode 100644 index 19663ea1a10f7..0000000000000 --- a/pkg/registry/apis/dashboard/variable_storage.go +++ /dev/null @@ -1,107 +0,0 @@ -package dashboard - -import ( - "context" - - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apiserver/pkg/registry/generic/registry" - "k8s.io/apiserver/pkg/registry/rest" - - "github.com/grafana/grafana-app-sdk/logging" - dashv2beta1 "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v2beta1" - "github.com/grafana/grafana/pkg/services/apiserver/client" -) - -// variableStorage wraps *registry.Store so that Create performs a post-write -// uniqueness verification. The admission-time uniqueness check in -// validateVariableNameUniqueness is TOCTOU-vulnerable: two concurrent Creates -// for the same (spec.spec.name, folder) can both see an empty list and both -// commit. This wrapper re-lists after the write and, if duplicates are -// observed, applies a deterministic tie-break (earliest creationTimestamp, -// then lowest UID). Both verifiers in a race reach the same conclusion, so -// exactly one object survives; the loser deletes itself and returns -// AlreadyExists to the client. The admission-level check remains the fast -// path for the common non-racing case. -type variableStorage struct { - *registry.Store - provider client.K8sHandlerProvider - - // innerCreate and innerDelete indirect through fields so tests can inject - // fakes without constructing a real *registry.Store. In production they - // point at the embedded store's methods. - innerCreate func(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) - innerDelete func(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) -} - -var _ rest.Creater = (*variableStorage)(nil) - -func newVariableStorage(store *registry.Store, provider client.K8sHandlerProvider) *variableStorage { - return &variableStorage{ - Store: store, - provider: provider, - innerCreate: store.Create, - innerDelete: store.Delete, - } -} - -func (s *variableStorage) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { - created, err := s.innerCreate(ctx, obj, createValidation, options) - if err != nil { - return created, err - } - - // Dry-run does not persist, so the post-create list cannot see it and the - // verification is meaningless. The admission-time check already ran. - if options != nil && len(options.DryRun) > 0 { - return created, nil - } - - if s.provider == nil { - return created, nil - } - - variable, ok := created.(*dashv2beta1.Variable) - if !ok { - return created, nil - } - - lost, verifyErr := resolveVariableNameConflictAfterCreate(ctx, s.provider, variable) - if verifyErr != nil { - // Degrade gracefully: the admission-level check is the primary - // guarantee. Do not fail the user's write because the post-write - // verification could not be completed. - logging.FromContext(ctx).Warn( - "variable post-create uniqueness verification failed; keeping object", - "name", variable.GetName(), - "namespace", variable.GetNamespace(), - "err", verifyErr, - ) - return created, nil - } - - if !lost { - return created, nil - } - - specName := getVariableName(variable.Spec) - uid := variable.GetUID() - deleteOpts := &metav1.DeleteOptions{ - Preconditions: &metav1.Preconditions{UID: &uid}, - } - if _, _, deleteErr := s.innerDelete(ctx, variable.GetName(), nil, deleteOpts); deleteErr != nil { - // The losing object may be briefly orphaned if this delete fails; the - // next admission-level check on any subsequent create will catch it. - // Surface AlreadyExists to the client either way - logging.FromContext(ctx).Error( - "failed to delete variable after losing uniqueness tie-break", - "name", variable.GetName(), - "namespace", variable.GetNamespace(), - "uid", string(uid), - "err", deleteErr, - ) - } - - return nil, apierrors.NewAlreadyExists(dashv2beta1.VariableResourceInfo.GroupResource(), specName) -} diff --git a/pkg/registry/apis/dashboard/variable_storage_test.go b/pkg/registry/apis/dashboard/variable_storage_test.go deleted file mode 100644 index beda16b820b9d..0000000000000 --- a/pkg/registry/apis/dashboard/variable_storage_test.go +++ /dev/null @@ -1,198 +0,0 @@ -package dashboard - -import ( - "context" - "errors" - "testing" - "time" - - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "k8s.io/apiserver/pkg/registry/rest" - - "github.com/grafana/grafana/pkg/services/apiserver/client" -) - -func TestVariableStorageCreate(t *testing.T) { - ctx := context.Background() - createdTime := time.Date(2026, 1, 1, 10, 0, 0, 0, time.UTC) - - buildCreatedVariable := func() runtime.Object { - return newCreatedVariable("region", "region-created", "uid-created", "42", "", createdTime) - } - - t.Run("happy path: no duplicate, returns created object and does not call Delete", func(t *testing.T) { - handler := &client.MockK8sHandler{} - handler.On("List", mock.Anything, int64(1), mock.Anything). - Return(&unstructured.UnstructuredList{ - Items: []unstructured.Unstructured{ - makeVariableListItem("region-created", "uid-created", "", createdTime), - }, - }, nil).Once() - - calls := &storeCallRecorder{} - s := &variableStorage{ - provider: &staticHandlerProvider{handler: handler}, - innerCreate: calls.create(buildCreated ... [truncated]