Authorization Bypass / Identity Immutability (classic provisioning shim kinds)

PoC steps (conceptual): 1) In a Grafana cluster with 12.x, create a custom resource representing a classic shim Dashboard (classic-api-provisioning or classic-converted-prometheus) that has no identity annotation (Identity empty) and set Kind to a classic shim kind (e.g., ManagerKindClassicAPI). 2) Run a provisioning/migration/cleanup workflow (e.g., NamespaceCleaner) that previously relied on identity to determine ownership; the resource would be treated as unmanaged and could be deleted as an orphan or skipped in export/update logic. 3) After applying the fix (this commit), classic shim kinds are reported as managed even without an identity, so the resource is skipped and preserved instead of being deleted/exported as unmanaged. Example resource (illustrative): apiVersion: dashboard.grafana.app/v1alpha1 kind: Dashboard metadata: name: classic-managed-dashboard annotations: grafana.app/managedBy: "" # empty identity by design for classic shim kinds # spec: … (omitted for brevity) Attack vectors (before fix): a provisioning/cleanup step could delete or modify classic-managed resources because the absence of a stable identity caused them to be treated as unmanaged. Attack vectors (after fix): classic shim resources are considered managed and protected from identity-based immutability checks, preventing unintended modification/deletion.

diff --git a/pkg/apimachinery/utils/manager.go b/pkg/apimachinery/utils/manager.go index 4b9a76dc65f21..f5fdf53563f3b 100644 --- a/pkg/apimachinery/utils/manager.go +++ b/pkg/apimachinery/utils/manager.go @@ -33,9 +33,31 @@ const ( ManagerKindPlugin ManagerKind = "plugin" ManagerKindGrafana ManagerKind = "grafana" - // Deprecated: this is used as a shim/migration path for legacy file provisioning - // Previously this was a "file:" prefix + // ManagerKindClassicFP marks resources that originate from the + // legacy on-disk file provisioning system (dashboards, folders, correlations, + // alerting with the "file" provenance). The manager identity, when present, is + // the provisioner/reader name from the provisioning config. Previously this was + // a "file:" prefix. + // + // Deprecated: shim/migration path only. New resources should use a real manager + // kind (repo, terraform, kubectl, ...). ManagerKindClassicFP ManagerKind = "classic-file-provisioning" + + // ManagerKindClassicAPI marks resources created through the legacy provisioning + // HTTP API (alerting "api" provenance) where the concrete tool that made the + // call was not recorded, so there is no meaningful manager identity. + // + // Deprecated: shim/migration path only. Prefer ManagerKindTerraform, + // ManagerKindKubectl, etc. for new resources. + ManagerKindClassicAPI ManagerKind = "classic-api-provisioning" + + // ManagerKindClassicConvertedPrometheus marks resources imported by the Grafana + // Alerting "Convert Prometheus" API, which converts Prometheus/Mimir/Cortex rule + // groups into Grafana-managed rules (alerting "converted_prometheus" provenance). + // The import has no owning manager instance, so there is no manager identity. + // + // Deprecated: shim/migration path only. + ManagerKindClassicConvertedPrometheus ManagerKind = "classic-converted-prometheus" ) // ParseManagerKindString parses a string into a ManagerKind. @@ -55,11 +77,29 @@ func ParseManagerKindString(v string) ManagerKind { return ManagerKindGrafana case string(ManagerKindClassicFP): // nolint:staticcheck return ManagerKindClassicFP // nolint:staticcheck + case string(ManagerKindClassicAPI): // nolint:staticcheck + return ManagerKindClassicAPI // nolint:staticcheck + case string(ManagerKindClassicConvertedPrometheus): // nolint:staticcheck + return ManagerKindClassicConvertedPrometheus // nolint:staticcheck default: return ManagerKindUnknown } } +// IsClassic returns true for shim kinds that represent legacy provisioning +// mechanisms (file/API provisioning, converted Prometheus). These origins have no +// stable per-instance manager, so unlike other kinds they are considered managed +// even without a manager Identity. Because their identity is absent or unstable, it +// must not be treated as immutable the way user-defined identities are. +func (k ManagerKind) IsClassic() bool { + switch k { //nolint:staticcheck + case ManagerKindClassicFP, ManagerKindClassicAPI, ManagerKindClassicConvertedPrometheus: + return true + default: + return false + } +} + // SourceProperties is used to identify the source of a provisioned resource. // It is used by managers for reconciling data from a source to Grafana. // Not all managers use these properties, some (like Terraform) don't have a concept of a source. diff --git a/pkg/apimachinery/utils/meta.go b/pkg/apimachinery/utils/meta.go index b395733b02eee..790060fcdcb6c 100644 --- a/pkg/apimachinery/utils/meta.go +++ b/pkg/apimachinery/utils/meta.go @@ -679,11 +679,16 @@ func (m *grafanaMetaAccessor) GetManagerProperties() (ManagerProperties, bool) { }, true } - // If the identity is not set, we should ignore the other annotations and return the default values. - // - // This is to prevent inadvertently marking resources as managed, - // since that can potentially block updates from other sources. - return res, false + // Classic shim kinds (legacy file/API provisioning) have no meaningful identity, + // so allow them through without one. + kind := ParseManagerKindString(annot[AnnoKeyManagerKind]) + if !kind.IsClassic() { + // If the identity is not set, we should ignore the other annotations and return the default values. + // + // This is to prevent inadvertently marking resources as managed, + // since that can potentially block updates from other sources. + return res, false + } } res.Identity = id diff --git a/pkg/apimachinery/utils/meta_test.go b/pkg/apimachinery/utils/meta_test.go index 7bc89143a0c69..03ca85d35471e 100644 --- a/pkg/apimachinery/utils/meta_test.go +++ b/pkg/apimachinery/utils/meta_test.go @@ -783,6 +783,41 @@ func TestMetaAccessor(t *testing.T) { }, wantOK: true, }, + { + // Classic shim kinds have no meaningful identity, so unlike other kinds + // they must still be reported as managed when the identity is empty. + name: "classic file-provisioning kind without identity is managed", + setProperties: &utils.ManagerProperties{ + Kind: utils.ManagerKindClassicFP, //nolint:staticcheck + }, + wantProperties: utils.ManagerProperties{ + Identity: "", + Kind: utils.ManagerKindClassicFP, //nolint:staticcheck + }, + wantOK: true, + }, + { + name: "classic api-provisioning kind without identity is managed", + setProperties: &utils.ManagerProperties{ + Kind: utils.ManagerKindClassicAPI, //nolint:staticcheck + }, + wantProperties: utils.ManagerProperties{ + Identity: "", + Kind: utils.ManagerKindClassicAPI, //nolint:staticcheck + }, + wantOK: true, + }, + { + name: "classic converted-prometheus kind without identity is managed", + setProperties: &utils.ManagerProperties{ + Kind: utils.ManagerKindClassicConvertedPrometheus, //nolint:staticcheck + }, + wantProperties: utils.ManagerProperties{ + Identity: "", + Kind: utils.ManagerKindClassicConvertedPrometheus, //nolint:staticcheck + }, + wantOK: true, + }, } for _, tt := range tests { diff --git a/pkg/registry/apis/provisioning/jobs/export/folders.go b/pkg/registry/apis/provisioning/jobs/export/folders.go index f6225312c33d7..dc4b8142229e1 100644 --- a/pkg/registry/apis/provisioning/jobs/export/folders.go +++ b/pkg/registry/apis/provisioning/jobs/export/folders.go @@ -32,9 +32,10 @@ func ExportFolders(ctx context.Context, repoName string, options provisioning.Ex return fmt.Errorf("extract meta accessor: %w", err) } - manager, _ := meta.GetManagerProperties() - // Skip if already managed by any manager (repository, file provisioning, etc.) - if manager.Identity != "" { + _, managed := meta.GetManagerProperties() + // Skip if already managed by any manager (repository, file provisioning, etc.). + // Classic shim kinds are managed without an identity, so rely on the managed flag. + if managed { return nil } @@ -135,7 +136,7 @@ func collectFolderAncestry(ctx context.Context, folderUID string, folderClient d } seen[current] = struct{}{} - if manager, _ := meta.GetManagerProperties(); manager.Identity != "" { + if _, managed := meta.GetManagerProperties(); managed { return nil } diff --git a/pkg/registry/apis/provisioning/jobs/export/folders_test.go b/pkg/registry/apis/provisioning/jobs/export/folders_test.go index 5ab9478042835..14d5532b64403 100644 --- a/pkg/registry/apis/provisioning/jobs/export/folders_test.go +++ b/pkg/registry/apis/provisioning/jobs/export/folders_test.go @@ -517,6 +517,46 @@ func TestFolderMetaAccessor(t *testing.T) { Branch: "feature/branch", }, fakeFolderClient, mockRepoResources, progress) + require.NoError(t, err) + mockRepoResources.AssertExpectations(t) + progress.AssertExpectations(t) + }) + t.Run("should skip classic-managed folder that has no identity", func(t *testing.T) { + // Classic shim kinds are reported as managed without an identity. + // Such a folder must still be skipped, since it is owned elsewhere. + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-folder", + "annotations": map[string]interface{}{ + "folder.grafana.app/uid": "test-folder-uid", + }, + }, + }, + } + meta, err := utils.MetaAccessor(obj) + require.NoError(t, err) + meta.SetManagerProperties(utils.ManagerProperties{ + Kind: utils.ManagerKindClassicAPI, //nolint:staticcheck + }) + fakeFolderClient := &mockDynamicInterface{ + items: []unstructured.Unstructured{*obj}, + } + + mockRepoResources := resources.NewMockRepositoryResources(t) + progress := jobs.NewMockJobProgressRecorder(t) + progress.On("SetMessage", mock.Anything, mock.Anything).Return().Twice() + mockRepoResources.On("EnsureFolderTreeExists", mock.Anything, mock.MatchedBy(func(tree resources.FolderTree) bool { + return tree.Count() == 0 // Should be empty since folder was skipped + }), mock.MatchedBy(func(opts resources.EnsureFolderTreeExistsOptions) bool { + return opts.Ref == "feature/branch" && opts.Path == "grafana" + })).Return(nil) + + err = ExportFolders(context.Background(), "test-repo", v0alpha1.ExportJobOptions{ + Path: "grafana", + Branch: "feature/branch", + }, fakeFolderClient, mockRepoResources, progress) + require.NoError(t, err) mockRepoResources.AssertExpectations(t) progress.AssertExpectations(t) diff --git a/pkg/registry/apis/provisioning/jobs/export/resources.go b/pkg/registry/apis/provisioning/jobs/export/resources.go index 84f7113565175..bc61860141ddd 100644 --- a/pkg/registry/apis/provisioning/jobs/export/resources.go +++ b/pkg/registry/apis/provisioning/jobs/export/resources.go @@ -229,12 +229,12 @@ func exportItem(ctx context.Context, return nil } - manager, _ := meta.GetManagerProperties() - if manager.Identity != "" { + manager, managed := meta.GetManagerProperties() + if managed { if explicitlyRequested { // Leave the default action in place: the recorder discards errors // on FileActionIgnored results, and we want this failure to count. - resultBuilder.WithError(fmt.Errorf("%s %q is managed by %q and cannot be exported", kindName, name, manager.Identity)) + resultBuilder.WithError(fmt.Errorf("%s %q is managed by %q and cannot be exported", kindName, name, manager.Kind)) progress.Record(ctx, resultBuilder.Build()) return progress.TooManyErrors() } diff --git a/pkg/registry/apis/provisioning/jobs/export/resources_test.go b/pkg/registry/apis/provisioning/jobs/export/resources_test.go index d14928e914bc8..93d42145b0321 100644 --- a/pkg/registry/apis/provisioning/jobs/export/resources_test.go +++ b/pkg/registry/apis/provisioning/jobs/export/resources_test.go @@ -555,6 +555,37 @@ func TestExportResources_Dashboards_SkipsManagedResources(t *testing.T) { require.NoError(t, err) } +func TestExportResources_Dashboards_SkipsClassicManagedResourceWithoutIdentity(t *testing.T) { + // Classic shim kinds are reported as managed without an identity. + // Such a resource must still be skipped, not exported. + dashboard := createDashboardObject("classic-managed-dashboard") + + meta, err := utils.MetaAccessor(&dashboard) + require.NoError(t, err) + meta.SetManagerProperties(utils.ManagerProperties{ + Kind: utils.ManagerKindClassicAPI, //nolint:staticcheck + }) + + mockItems := []unstructured.Unstructured{dashboard} + + setupProgress := func(progress *jobs.MockJobProgressRecorder) { + progress.On("SetMessage", mock.Anything, "start resource export").Return() + progress.On("SetMessage", mock.Anything, "export dashboards").Return() + progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { + return result.Name() == "classic-managed-dashboard" && result.Action() == repository.FileActionIgnored + })).Return() + progress.On("TooManyErrors").Return(nil).Maybe() + } + + setupResources := func(repoResources *resources.MockRepositoryResources, resourceClients *resources.MockResourceClients, mockClient *mockDynamicInterface, gvk schema.GroupVersionKind) { + resourceClients.On("ForKind", mock.Anything, mock.Anything).Return(mockClient, resources.DashboardResource, nil) + // No WriteResourceFileFromObject call expected since resource should be skipped + } + + err = runExportTest(t, mockItems, setupProgress, setupResources) + require.NoError(t, err) +} + func TestExportResources_GenerateNewUIDs(t *testing.T) { mockItems := []unstructured.Unstructured{ createDashboardObject("original-name-1"), diff --git a/pkg/registry/apis/provisioning/jobs/migrate/clean.go b/pkg/registry/apis/provisioning/jobs/migrate/clean.go index 48a10fe161d8b..60f01d83276c9 100644 --- a/pkg/registry/apis/provisioning/jobs/migrate/clean.go +++ b/pkg/registry/apis/provisioning/jobs/migrate/clean.go @@ -52,9 +52,11 @@ func (c *namespaceCleaner) Clean(ctx context.Context, namespace string, progress return nil // Continue with next resource } - manager, _ := meta.GetManagerProperties() - // Skip if resource is managed by any provisioning system - if manager.Identity != "" { + _, managed := meta.GetManagerProperties() + // Skip if resource is managed by any provisioning system. Use the managed + // flag rather than a non-empty identity: classic shim kinds are reported as + // managed without an identity and would otherwise be deleted as orphans. + if managed { resultBuilder.WithAction(repository.FileActionIgnored) progress.Record(ctx, resultBuilder.Build()) return nil // Skip this resource diff --git a/pkg/registry/apis/provisioning/jobs/migrate/clean_test.go b/pkg/registry/apis/provisioning/jobs/migrate/clean_test.go index 6485c1616f6f5..4b01493130162 100644 --- a/pkg/registry/apis/provisioning/jobs/migrate/clean_test.go +++ b/pkg/registry/apis/provisioning/jobs/migrate/clean_test.go @@ -292,6 +292,59 @@ func TestNamespaceCleaner_Clean(t *testing.T) { clients.AssertExpectations(t) progress.AssertExpectations(t) }) + + t.Run("should skip classic-managed resources without an identity", func(t *testing.T) { + // A classic shim kind is reported as managed even though it has no manager + // identity. It must be ignored, not deleted as an orphan. Under the previous + // "identity != empty" check this resource would have been deleted. + mockDynamicClient := &mockDynamicInterface{ + items: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "apiVersion": "dashboard.grafana.app/v1alpha1", + "kind": "Dashboard", + "metadata": map[string]interface{}{ + "name": "classic-managed-dashboard", + "annotations": map[string]interface{}{ + // Manager kind is set, but there is no manager identity. + "grafana.app/managedBy": "classic-converted-prometheus", + }, + }, + }, + }, + }, + } + + clients := &mockClients{} + clients.On("ForKind", mock.Anything, schema.GroupVersionKind{Group: resources.DashboardResource.Group, ... [truncated]