From d3e6e308a0efef772b0dc5d7f2da620df09ff41a Mon Sep 17 00:00:00 2001 From: Ryan McKinley Date: Tue, 8 Apr 2025 14:17:33 +0300 Subject: [PATCH] K8s/ManagedBy: Enforce who can CRUD provisioning resources (#103322) --- .../apis/provisioning/resources/dualwriter.go | 6 + .../apis/provisioning/resources/parser.go | 8 +- pkg/storage/unified/apistore/managed.go | 88 ++++++++++++ pkg/storage/unified/apistore/managed_test.go | 136 ++++++++++++++++++ pkg/storage/unified/apistore/prepare.go | 8 ++ pkg/storage/unified/apistore/prepare_test.go | 3 + pkg/storage/unified/apistore/store.go | 15 ++ .../apis/provisioning/provisioning_test.go | 34 ++++- 8 files changed, 290 insertions(+), 8 deletions(-) create mode 100644 pkg/storage/unified/apistore/managed.go create mode 100644 pkg/storage/unified/apistore/managed_test.go diff --git a/pkg/registry/apis/provisioning/resources/dualwriter.go b/pkg/registry/apis/provisioning/resources/dualwriter.go index 094e5c2fb2d..b15d66ae6cf 100644 --- a/pkg/registry/apis/provisioning/resources/dualwriter.go +++ b/pkg/registry/apis/provisioning/resources/dualwriter.go @@ -9,6 +9,7 @@ import ( "github.com/grafana/grafana-app-sdk/logging" "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/identity" provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository" "github.com/grafana/grafana/pkg/registry/apis/provisioning/safepath" @@ -80,6 +81,11 @@ func (r *DualReadWriter) Delete(ctx context.Context, path string, ref string, me // Delete the file in the grafana database if ref == "" { + ctx, _, err := identity.WithProvisioningIdentity(ctx, parsed.Obj.GetNamespace()) + if err != nil { + return parsed, err + } + // FIXME: empty folders with no repository files will remain in the system // until the next reconciliation. err = parsed.Client.Delete(ctx, parsed.Obj.GetName(), metav1.DeleteOptions{}) diff --git a/pkg/registry/apis/provisioning/resources/parser.go b/pkg/registry/apis/provisioning/resources/parser.go index 2fa91f48a52..e13ca775ee7 100644 --- a/pkg/registry/apis/provisioning/resources/parser.go +++ b/pkg/registry/apis/provisioning/resources/parser.go @@ -17,6 +17,7 @@ import ( "github.com/grafana/grafana-app-sdk/logging" dashboard "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository" @@ -240,7 +241,12 @@ func (f *ParsedResource) Run(ctx context.Context) error { return fmt.Errorf("unable to find client") } - var err error + // Always use the provisioning identity when writing + ctx, _, err := identity.WithProvisioningIdentity(ctx, f.Obj.GetNamespace()) + if err != nil { + return err + } + // FIXME: shouldn't we check for the specific error? // Run update or create f.Existing, _ = f.Client.Get(ctx, f.Obj.GetName(), metav1.GetOptions{}) diff --git a/pkg/storage/unified/apistore/managed.go b/pkg/storage/unified/apistore/managed.go new file mode 100644 index 00000000000..c8fb4a9389e --- /dev/null +++ b/pkg/storage/unified/apistore/managed.go @@ -0,0 +1,88 @@ +package apistore + +import ( + "net/http" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + authtypes "github.com/grafana/authlib/types" + "github.com/grafana/grafana/pkg/apimachinery/utils" +) + +func checkManagerPropertiesOnDelete(auth authtypes.AuthInfo, obj utils.GrafanaMetaAccessor) error { + return enforceManagerProperties(auth, obj) +} + +func checkManagerPropertiesOnCreate(auth authtypes.AuthInfo, obj utils.GrafanaMetaAccessor) error { + return enforceManagerProperties(auth, obj) +} + +func checkManagerPropertiesOnUpdateSpec(auth authtypes.AuthInfo, obj utils.GrafanaMetaAccessor, old utils.GrafanaMetaAccessor) error { + objKind := obj.GetAnnotation(utils.AnnoKeyManagerKind) + oldKind := old.GetAnnotation(utils.AnnoKeyManagerKind) + if objKind == "" && objKind == oldKind { + return nil // not managed + } + + // Check the current settings + err := checkManagerPropertiesOnCreate(auth, obj) + if err != nil { // new settings failed + return err + } + + managerNew, okNew := obj.GetManagerProperties() + managerOld, okOld := old.GetManagerProperties() + if managerNew == managerOld || (okNew && !okOld) { // added manager is OK + return nil + } + + if !okNew && okOld { + // This allows removing the managedBy annotations if you were allowed to write them originally + if err := checkManagerPropertiesOnCreate(auth, old); err != nil { + return &apierrors.StatusError{ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusForbidden, + Reason: metav1.StatusReasonForbidden, + Message: "Can not remove resource manager from resource", + }} + } + } + return nil +} + +func enforceManagerProperties(auth authtypes.AuthInfo, obj utils.GrafanaMetaAccessor) error { + kind := obj.GetAnnotation(utils.AnnoKeyManagerKind) + if kind == "" { + return nil + } + + switch utils.ParseManagerKindString(kind) { + case utils.ManagerKindUnknown: + return nil // not managed + + case utils.ManagerKindRepo: + if auth.GetUID() == "access-policy:provisioning" { + return nil // OK! + } + return &apierrors.StatusError{ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusForbidden, + Reason: metav1.StatusReasonForbidden, + Message: "Provisioned resources must be manaaged by the provisioning service account", + }} + + case utils.ManagerKindPlugin, utils.ManagerKindClassicFP: // nolint:staticcheck + // ?? what identity do we use for legacy internal requests? + return nil // no error + + case utils.ManagerKindTerraform, utils.ManagerKindKubectl: + manager, _ := obj.GetManagerProperties() + if manager.AllowsEdits { + return nil // no error anyone can do it + } + // TODO: check the kubectl+terraform resource + return nil + } + return nil +} diff --git a/pkg/storage/unified/apistore/managed_test.go b/pkg/storage/unified/apistore/managed_test.go new file mode 100644 index 00000000000..7623db8f473 --- /dev/null +++ b/pkg/storage/unified/apistore/managed_test.go @@ -0,0 +1,136 @@ +package apistore + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + + authtypes "github.com/grafana/authlib/types" + dashboard "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v1alpha1" + "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/apimachinery/utils" +) + +func TestManagedAuthorizer(t *testing.T) { + user := &identity.StaticRequester{Type: authtypes.TypeUser, UserUID: "uuu"} + _, provisioner, err := identity.WithProvisioningIdentity(context.Background(), "default") + require.NoError(t, err) + + tests := []struct { + name string + auth authtypes.AuthInfo + obj runtime.Object + old runtime.Object + err string + }{ + { + name: "user can create", + auth: user, + obj: &unstructured.Unstructured{}, + }, + { + name: "provisioning can create", + auth: provisioner, + obj: &dashboard.Dashboard{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + utils.AnnoKeyManagerKind: string(utils.ManagerKindRepo), + utils.AnnoKeyManagerIdentity: "abc", + }, + }, + }, + }, + { + name: "user can not create provisioned resource", + auth: user, + err: "Provisioned resources must be manaaged by the provisioning service account", + obj: &dashboard.Dashboard{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + utils.AnnoKeyManagerKind: string(utils.ManagerKindRepo), + utils.AnnoKeyManagerIdentity: "abc", + }, + }, + }, + }, + { + name: "user can not update provisioned resource", + auth: user, + err: "Provisioned resources must be manaaged by the provisioning service account", + obj: &dashboard.Dashboard{ + ObjectMeta: v1.ObjectMeta{ + Generation: 1, + }, + }, + old: &dashboard.Dashboard{ + ObjectMeta: v1.ObjectMeta{ + Generation: 2, + Annotations: map[string]string{ + utils.AnnoKeyManagerKind: string(utils.ManagerKindRepo), + utils.AnnoKeyManagerIdentity: "abc", + }, + }, + }, + }, + { + name: "provisioner can remove manager flags", + auth: provisioner, + obj: &dashboard.Dashboard{ + ObjectMeta: v1.ObjectMeta{ + Generation: 1, + }, + }, + old: &dashboard.Dashboard{ + ObjectMeta: v1.ObjectMeta{ + Generation: 2, + Annotations: map[string]string{ + utils.AnnoKeyManagerKind: string(utils.ManagerKindRepo), + utils.AnnoKeyManagerIdentity: "abc", + }, + }, + }, + }, + { + name: "provisioner can add manager flags", + auth: provisioner, + old: &dashboard.Dashboard{ + ObjectMeta: v1.ObjectMeta{ + Generation: 1, + }, + }, + obj: &dashboard.Dashboard{ + ObjectMeta: v1.ObjectMeta{ + Generation: 2, + Annotations: map[string]string{ + utils.AnnoKeyManagerKind: string(utils.ManagerKindRepo), + utils.AnnoKeyManagerIdentity: "abc", + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + obj, err := utils.MetaAccessor(tt.obj) + require.NoError(t, err) + + if tt.old == nil { + err = checkManagerPropertiesOnCreate(tt.auth, obj) + } else { + old, _ := utils.MetaAccessor(tt.old) + err = checkManagerPropertiesOnUpdateSpec(tt.auth, obj, old) + } + + if tt.err != "" { + require.Error(t, err, tt.err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/storage/unified/apistore/prepare.go b/pkg/storage/unified/apistore/prepare.go index 6aa6cbd5c7b..a090dfc225d 100644 --- a/pkg/storage/unified/apistore/prepare.go +++ b/pkg/storage/unified/apistore/prepare.go @@ -61,6 +61,9 @@ func (s *Storage) prepareObjectForStorage(ctx context.Context, newObject runtime if obj.GetFolder() != "" && !s.opts.EnableFolderSupport { return nil, apierrors.NewBadRequest(fmt.Sprintf("folders are not supported for: %s", s.gr.String())) } + if err := checkManagerPropertiesOnCreate(info, obj); err != nil { + return nil, err + } if s.opts.RequireDeprecatedInternalID { // nolint:staticcheck @@ -160,6 +163,11 @@ func (s *Storage) prepareObjectForUpdate(ctx context.Context, updateObject runti obj.SetGeneration(previous.GetGeneration() + 1) obj.SetUpdatedBy(info.GetUID()) obj.SetUpdatedTimestampMillis(time.Now().UnixMilli()) + + // Only validate when the generation has changed + if err := checkManagerPropertiesOnUpdateSpec(info, obj, previous); err != nil { + return nil, err + } } else { obj.SetGeneration(previous.GetGeneration()) obj.SetAnnotation(utils.AnnoKeyUpdatedBy, previous.GetAnnotation(utils.AnnoKeyUpdatedBy)) diff --git a/pkg/storage/unified/apistore/prepare_test.go b/pkg/storage/unified/apistore/prepare_test.go index 3172f95de80..aa5188a3a76 100644 --- a/pkg/storage/unified/apistore/prepare_test.go +++ b/pkg/storage/unified/apistore/prepare_test.go @@ -87,6 +87,9 @@ func TestPrepareObjectForStorage(t *testing.T) { }) t.Run("Should keep manager info", func(t *testing.T) { + ctx, _, err := identity.WithProvisioningIdentity(ctx, "default") + require.NoError(t, err) + dashboard := v1alpha1.Dashboard{} dashboard.Name = "test-name" obj := dashboard.DeepCopyObject() diff --git a/pkg/storage/unified/apistore/store.go b/pkg/storage/unified/apistore/store.go index 4557c0a5d39..d8aee80bd39 100644 --- a/pkg/storage/unified/apistore/store.go +++ b/pkg/storage/unified/apistore/store.go @@ -31,6 +31,7 @@ import ( "github.com/bwmarrin/snowflake" + authtypes "github.com/grafana/authlib/types" "github.com/grafana/grafana/pkg/apimachinery/utils" grafanaregistry "github.com/grafana/grafana/pkg/apiserver/registry/generic" "github.com/grafana/grafana/pkg/apiserver/rest" @@ -219,6 +220,11 @@ func (s *Storage) Delete( _ runtime.Object, opts storage.DeleteOptions, ) error { + info, ok := authtypes.AuthInfoFrom(ctx) + if !ok { + return errors.New("missing auth info") + } + if err := s.Get(ctx, key, storage.GetOptions{}, out); err != nil { return err } @@ -250,6 +256,15 @@ func (s *Storage) Delete( return err } } + + meta, err := utils.MetaAccessor(out) + if err != nil { + return fmt.Errorf("unable to read object %w", err) + } + if err = checkManagerPropertiesOnDelete(info, meta); err != nil { + return err + } + rsp, err := s.store.Delete(ctx, cmd) if err != nil { return resource.GetError(resource.AsErrorResult(err)) diff --git a/pkg/tests/apis/provisioning/provisioning_test.go b/pkg/tests/apis/provisioning/provisioning_test.go index 94939fc5fd0..c642c366cd4 100644 --- a/pkg/tests/apis/provisioning/provisioning_test.go +++ b/pkg/tests/apis/provisioning/provisioning_test.go @@ -14,6 +14,7 @@ import ( ghmock "github.com/migueleliasweb/go-github-mock/src/mock" "github.com/stretchr/testify/assert" "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" @@ -325,7 +326,7 @@ func TestIntegrationProvisioning_ImportAllPanelsFromLocalRepository(t *testing.T // Set up the repository and the file to import. helper.CopyToProvisioningPath(t, "testdata/all-panels.json", "all-panels.json") - localTmp := helper.RenderObject(t, "testdata/local-readonly.json.tmpl", map[string]any{ + localTmp := helper.RenderObject(t, "testdata/local-write.json.tmpl", map[string]any{ "Name": repo, "SyncEnabled": true, }) @@ -349,18 +350,37 @@ func TestIntegrationProvisioning_ImportAllPanelsFromLocalRepository(t *testing.T const allPanels = "n1jR8vnnz" _, err = helper.Dashboards.Resource.Get(ctx, allPanels, metav1.GetOptions{}) require.Error(t, err, "no all-panels dashboard should exist") + require.True(t, apierrors.IsNotFound(err)) // Now, we import it, such that it may exist helper.SyncAndWait(t, repo, nil) - found, err := helper.Dashboards.Resource.List(ctx, metav1.ListOptions{}) + _, err = helper.Dashboards.Resource.List(ctx, metav1.ListOptions{}) require.NoError(t, err, "can list values") - names := []string{} - for _, v := range found.Items { - names = append(names, v.GetName()) - } - require.Contains(t, names, allPanels, "all-panels dashboard should now exist") + obj, err = helper.Dashboards.Resource.Get(ctx, allPanels, metav1.GetOptions{}) + require.NoError(t, err, "all-panels dashboard should exist") + require.Equal(t, repo, obj.GetAnnotations()[utils.AnnoKeyManagerIdentity]) + + // Try writing the value directly + err = unstructured.SetNestedField(obj.Object, []any{"aaa", "bbb"}, "spec", "tags") + require.NoError(t, err, "set tags") + _, err = helper.Dashboards.Resource.Update(ctx, obj, metav1.UpdateOptions{}) + require.Error(t, err, "only the provisionding service should be able to update") + require.True(t, apierrors.IsForbidden(err)) + + // Should not be able to directly delete the managed resource + err = helper.Dashboards.Resource.Delete(ctx, allPanels, metav1.DeleteOptions{}) + require.Error(t, err, "only the provisioning service should be able to delete") + require.True(t, apierrors.IsForbidden(err)) + + // But we can delete the repository file, and this should also remove the resource + err = helper.Repositories.Resource.Delete(ctx, repo, metav1.DeleteOptions{}, "files", "all-panels.json") + require.NoError(t, err, "should delete the resource file") + + _, err = helper.Dashboards.Resource.Get(ctx, allPanels, metav1.GetOptions{}) + require.Error(t, err, "should delete the internal resource") + require.True(t, apierrors.IsNotFound(err)) } func TestProvisioning_ExportUnifiedToRepository(t *testing.T) {