diff --git a/pkg/apimachinery/utils/meta.go b/pkg/apimachinery/utils/meta.go index bdf9a100540..453099e8add 100644 --- a/pkg/apimachinery/utils/meta.go +++ b/pkg/apimachinery/utils/meta.go @@ -869,16 +869,18 @@ func (m *grafanaMetaAccessor) GetSecureValues() (vals common.InlineSecureValues, if ok { vals = make(common.InlineSecureValues, len(u)) for k, v := range u { - sv, ok := v.(map[string]any) + inline, ok := v.(common.InlineSecureValue) if !ok { - return nil, fmt.Errorf("unsupported nested secure value: %t", v) - } - inline := common.InlineSecureValue{} - inline.Name, _, _ = unstructured.NestedString(sv, "name") - inline.Remove, _, _ = unstructured.NestedBool(sv, "remove") - create, _, _ := unstructured.NestedString(sv, "create") - if create != "" { - inline.Create = common.NewSecretValue(create) + sv, ok := v.(map[string]any) + if !ok { + return nil, fmt.Errorf("unsupported nested secure value: %t", v) + } + inline.Name, _, _ = unstructured.NestedString(sv, "name") + inline.Remove, _, _ = unstructured.NestedBool(sv, "remove") + create, _, _ := unstructured.NestedString(sv, "create") + if create != "" { + inline.Create = common.NewSecretValue(create) + } } vals[k] = inline } diff --git a/pkg/storage/unified/apistore/prepare.go b/pkg/storage/unified/apistore/prepare.go index 19ca5aeca9c..a7f94a69706 100644 --- a/pkg/storage/unified/apistore/prepare.go +++ b/pkg/storage/unified/apistore/prepare.go @@ -5,9 +5,9 @@ import ( "context" "errors" "fmt" - "math" "time" + "github.com/dustin/go-humanize" "github.com/google/uuid" apiequality "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -16,59 +16,97 @@ import ( "k8s.io/apiserver/pkg/storage" "k8s.io/klog/v2" - authtypes "github.com/grafana/authlib/types" - + authlib "github.com/grafana/authlib/types" + "github.com/grafana/grafana-app-sdk/logging" + common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/utils" + secrets "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" ) -func logN(n, b float64) float64 { - return math.Log(n) / math.Log(b) +type objectForStorage struct { + // The value to save in unistore + raw bytes.Buffer + + // Reference to the owner object + ref common.ObjectReference + + // apply permissions after create (defined in the resource body) + grantPermissions string + + // Synchronous AfterCreate permissions -- allows users to become "admin" of the thing they made + permissionCreator permissionCreatorFunc + + // These secrets where created, should be cleaned up if storage fails + createdSecureValues []string + + // These should be deleted if storage succeeds + deleteSecureValues []string + + // We know something changed + // This will ensure that the generation increments + hasChanged bool } -// Slightly modified function from https://github.com/dustin/go-humanize (MIT). -func formatBytes(numBytes int) string { - base := 1024.0 - sizes := []string{"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"} - if numBytes < 10 { - return fmt.Sprintf("%d B", numBytes) +func (v *objectForStorage) finish(ctx context.Context, err error, secrets secrets.InlineSecureValueSupport) error { + if err != nil { + // Remove the secure values that were created + for _, s := range v.createdSecureValues { + if e := secrets.DeleteWhenOwnedByResource(ctx, v.ref, s); e != nil { + logging.FromContext(ctx).Warn("unable to clean up new secure value", "name", s, "err", e) + } + } + return err } - e := math.Floor(logN(float64(numBytes), base)) - suffix := sizes[int(e)] - val := math.Floor(float64(numBytes)/math.Pow(base, e)*10+0.5) / 10 - return fmt.Sprintf("%.1f %s", val, suffix) + + // Delete secure values after successfully saving the object + if len(v.deleteSecureValues) > 0 { + for _, s := range v.deleteSecureValues { + if e := secrets.DeleteWhenOwnedByResource(ctx, v.ref, s); e != nil { + logging.FromContext(ctx).Warn("unable to clean up new secure value", "name", s, "err", e) + } + } + } + + // Create permissions + if v.permissionCreator != nil { + return v.permissionCreator(ctx) + } + + return nil } // Called on create -func (s *Storage) prepareObjectForStorage(ctx context.Context, newObject runtime.Object) ([]byte, string, error) { - info, ok := authtypes.AuthInfoFrom(ctx) +func (s *Storage) prepareObjectForStorage(ctx context.Context, newObject runtime.Object) (objectForStorage, error) { + v := objectForStorage{} + info, ok := authlib.AuthInfoFrom(ctx) if !ok { - return nil, "", errors.New("missing auth info") + return v, errors.New("missing auth info") } obj, err := utils.MetaAccessor(newObject) if err != nil { - return nil, "", err + return v, err } if obj.GetName() == "" { - return nil, "", storage.NewInvalidObjError("", "missing name") + return v, storage.NewInvalidObjError("", "missing name") } if obj.GetResourceVersion() != "" { - return nil, "", storage.ErrResourceVersionSetOnCreate + return v, storage.ErrResourceVersionSetOnCreate } if obj.GetUID() == "" { obj.SetUID(types.UID(uuid.NewString())) } if obj.GetFolder() != "" && !s.opts.EnableFolderSupport { - return nil, "", apierrors.NewBadRequest(fmt.Sprintf("folders are not supported for: %s", s.gr.String())) + return v, apierrors.NewBadRequest(fmt.Sprintf("folders are not supported for: %s", s.gr.String())) } - grantPermisions := obj.GetAnnotation(utils.AnnoKeyGrantPermissions) - if grantPermisions != "" { + v.grantPermissions = obj.GetAnnotation(utils.AnnoKeyGrantPermissions) + if v.grantPermissions != "" { obj.SetAnnotation(utils.AnnoKeyGrantPermissions, "") // remove the annotation } if err := checkManagerPropertiesOnCreate(info, obj); err != nil { - return nil, "", err + return v, err } if s.opts.RequireDeprecatedInternalID { @@ -92,33 +130,37 @@ func (s *Storage) prepareObjectForStorage(ctx context.Context, newObject runtime obj.SetCreatedBy(info.GetUID()) obj.SetGeneration(1) // the first time we write - var buf bytes.Buffer - if err = s.codec.Encode(newObject, &buf); err != nil { - return nil, "", err + err = prepareSecureValues(ctx, s.opts.SecureValues, obj, nil, &v) + if err != nil { + return v, err } - val, err := s.handleLargeResources(ctx, obj, buf) - return val, grantPermisions, err + err = s.codec.Encode(newObject, &v.raw) + if err == nil { + err = s.handleLargeResources(ctx, obj, &v.raw) + } + return v, err } // Called on update -func (s *Storage) prepareObjectForUpdate(ctx context.Context, updateObject runtime.Object, previousObject runtime.Object) ([]byte, error) { - info, ok := authtypes.AuthInfoFrom(ctx) +func (s *Storage) prepareObjectForUpdate(ctx context.Context, updateObject runtime.Object, previousObject runtime.Object) (objectForStorage, error) { + v := objectForStorage{} + info, ok := authlib.AuthInfoFrom(ctx) if !ok { - return nil, errors.New("missing auth info") + return v, errors.New("missing auth info") } obj, err := utils.MetaAccessor(updateObject) if err != nil { - return nil, err + return v, err } if obj.GetName() == "" { - return nil, fmt.Errorf("updated object must have a name") + return v, fmt.Errorf("updated object must have a name") } previous, err := utils.MetaAccessor(previousObject) if err != nil { - return nil, err + return v, err } if previous.GetUID() == "" { @@ -133,7 +175,7 @@ func (s *Storage) prepareObjectForUpdate(ctx context.Context, updateObject runti } if obj.GetName() != previous.GetName() { - return nil, fmt.Errorf("name mismatch between existing and updated object") + return v, fmt.Errorf("name mismatch between existing and updated object") } obj.SetCreatedBy(previous.GetCreatedBy()) @@ -148,34 +190,39 @@ func (s *Storage) prepareObjectForUpdate(ctx context.Context, updateObject runti obj.SetDeprecatedInternalID(previousInternalID) // nolint:staticcheck } + err = prepareSecureValues(ctx, s.opts.SecureValues, obj, previous, &v) + if err != nil { + return v, err + } + // Check if we should bump the generation - changed := obj.GetFolder() != previous.GetFolder() - if changed { + if obj.GetFolder() != previous.GetFolder() { if !s.opts.EnableFolderSupport { - return nil, apierrors.NewBadRequest(fmt.Sprintf("folders are not supported for: %s", s.gr.String())) + return v, apierrors.NewBadRequest(fmt.Sprintf("folders are not supported for: %s", s.gr.String())) } // TODO: check that we can move the folder? + v.hasChanged = true } else if obj.GetDeletionTimestamp() != nil && previous.GetDeletionTimestamp() == nil { - changed = true // bump generation when deleted - } else { + v.hasChanged = true // bump generation when deleted + } else if !v.hasChanged { spec, e1 := obj.GetSpec() oldSpec, e2 := previous.GetSpec() if e1 == nil && e2 == nil { if !apiequality.Semantic.DeepEqual(spec, oldSpec) { - changed = true + v.hasChanged = true } } } // Mark the resource as changed - if changed { + if v.hasChanged { 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 + return v, err } } else { obj.SetGeneration(previous.GetGeneration()) @@ -183,19 +230,20 @@ func (s *Storage) prepareObjectForUpdate(ctx context.Context, updateObject runti obj.SetAnnotation(utils.AnnoKeyUpdatedTimestamp, previous.GetAnnotation(utils.AnnoKeyUpdatedTimestamp)) } - var buf bytes.Buffer - if err = s.codec.Encode(updateObject, &buf); err != nil { - return nil, err + err = s.codec.Encode(updateObject, &v.raw) + if err == nil { + err = s.handleLargeResources(ctx, obj, &v.raw) } - return s.handleLargeResources(ctx, obj, buf) + return v, err } -func (s *Storage) handleLargeResources(ctx context.Context, obj utils.GrafanaMetaAccessor, buf bytes.Buffer) ([]byte, error) { +// The bytes buffer will be reset with the proper value +func (s *Storage) handleLargeResources(ctx context.Context, obj utils.GrafanaMetaAccessor, buf *bytes.Buffer) error { support := s.opts.LargeObjectSupport size := buf.Len() if support != nil && size > support.Threshold() { if support.MaxSize() > 0 && size > support.MaxSize() { - return nil, fmt.Errorf("request object is too big (%s > %s)", formatBytes(size), formatBytes(support.MaxSize())) + return fmt.Errorf("request object is too big (%s > %s)", humanize.Bytes(uint64(size)), humanize.Bytes(uint64(support.MaxSize()))) } key := &resourcepb.ResourceKey{ @@ -207,19 +255,17 @@ func (s *Storage) handleLargeResources(ctx context.Context, obj utils.GrafanaMet err := support.Deconstruct(ctx, key, s.store, obj, buf.Bytes()) if err != nil { - return nil, err + return err } buf.Reset() orig, ok := obj.GetRuntimeObject() if !ok { - return nil, fmt.Errorf("error using object as runtime object") + return fmt.Errorf("error using object as runtime object") } // Now encode the smaller version - if err = s.codec.Encode(orig, &buf); err != nil { - return nil, err - } + return s.codec.Encode(orig, buf) } - return buf.Bytes(), nil + return nil } diff --git a/pkg/storage/unified/apistore/prepare_test.go b/pkg/storage/unified/apistore/prepare_test.go index b11081dea9b..1501b63da40 100644 --- a/pkg/storage/unified/apistore/prepare_test.go +++ b/pkg/storage/unified/apistore/prepare_test.go @@ -15,7 +15,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apiserver/pkg/storage" - authtypes "github.com/grafana/authlib/types" + authlib "github.com/grafana/authlib/types" dashv1 "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v1beta1" "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" @@ -37,19 +37,19 @@ func TestPrepareObjectForStorage(t *testing.T) { }, } - ctx := authtypes.WithAuthInfo(context.Background(), - &identity.StaticRequester{UserID: 1, UserUID: "user-uid", Type: authtypes.TypeUser}, + ctx := authlib.WithAuthInfo(context.Background(), + &identity.StaticRequester{UserID: 1, UserUID: "user-uid", Type: authlib.TypeUser}, ) t.Run("Error getting auth info from context", func(t *testing.T) { - _, _, err := s.prepareObjectForStorage(context.Background(), nil) + _, err := s.prepareObjectForStorage(context.Background(), nil) require.Error(t, err) require.Contains(t, err.Error(), "missing auth info") }) t.Run("Error on missing name", func(t *testing.T) { dashboard := dashv1.Dashboard{} - _, _, err := s.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) + _, err := s.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) require.Error(t, err) require.Contains(t, err.Error(), "missing name") }) @@ -58,7 +58,7 @@ func TestPrepareObjectForStorage(t *testing.T) { dashboard := dashv1.Dashboard{} dashboard.Name = "test-name" dashboard.ResourceVersion = "123" - _, _, err := s.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) + _, err := s.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) require.Error(t, err) require.Equal(t, storage.ErrResourceVersionSetOnCreate, err) }) @@ -67,10 +67,10 @@ func TestPrepareObjectForStorage(t *testing.T) { dashboard := dashv1.Dashboard{} dashboard.Name = "test-name" - encodedData, _, err := s.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) + v, err := s.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) require.NoError(t, err) - newObject, _, err := s.codec.Decode(encodedData, nil, &dashv1.Dashboard{}) + newObject, _, err := s.codec.Decode(v.raw.Bytes(), nil, &dashv1.Dashboard{}) require.NoError(t, err) obj, err := utils.MetaAccessor(newObject) require.NoError(t, err) @@ -106,10 +106,10 @@ func TestPrepareObjectForStorage(t *testing.T) { TimestampMillis: now.UnixMilli(), }) - encodedData, _, err := s.prepareObjectForStorage(ctx, obj) + v, err := s.prepareObjectForStorage(ctx, obj) require.NoError(t, err) - newObject, _, err := s.codec.Decode(encodedData, nil, &dashv1.Dashboard{}) + newObject, _, err := s.codec.Decode(v.raw.Bytes(), nil, &dashv1.Dashboard{}) require.NoError(t, err) meta, err = utils.MetaAccessor(newObject) require.NoError(t, err) @@ -133,10 +133,10 @@ func TestPrepareObjectForStorage(t *testing.T) { meta.SetFolder("aaa") require.NoError(t, err) - encodedData, _, err := s.prepareObjectForStorage(ctx, obj) + v, err := s.prepareObjectForStorage(ctx, obj) require.NoError(t, err) - insertedObject, _, err := s.codec.Decode(encodedData, nil, &dashv1.Dashboard{}) + insertedObject, _, err := s.codec.Decode(v.raw.Bytes(), nil, &dashv1.Dashboard{}) require.NoError(t, err) meta, err = utils.MetaAccessor(insertedObject) require.NoError(t, err) @@ -148,8 +148,8 @@ func TestPrepareObjectForStorage(t *testing.T) { require.Nil(t, ts) // Change the user... and only update metadata - ctx = authtypes.WithAuthInfo(context.Background(), - &identity.StaticRequester{UserID: 1, UserUID: "user2", Type: authtypes.TypeUser}, + ctx = authlib.WithAuthInfo(context.Background(), + &identity.StaticRequester{UserID: 1, UserUID: "user2", Type: authlib.TypeUser}, ) // Change the status... but generation is the same @@ -189,9 +189,9 @@ func TestPrepareObjectForStorage(t *testing.T) { dashboard := dashv1.Dashboard{} dashboard.Name = "test-name" - encodedData, _, err := s.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) + v, err := s.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) require.NoError(t, err) - newObject, _, err := s.codec.Decode(encodedData, nil, &dashv1.Dashboard{}) + newObject, _, err := s.codec.Decode(v.raw.Bytes(), nil, &dashv1.Dashboard{}) require.NoError(t, err) obj, err := utils.MetaAccessor(newObject) require.NoError(t, err) @@ -208,9 +208,9 @@ func TestPrepareObjectForStorage(t *testing.T) { require.NoError(t, err) meta.SetDeprecatedInternalID(1) // nolint:staticcheck - encodedData, _, err := s.prepareObjectForStorage(ctx, obj) + v, err := s.prepareObjectForStorage(ctx, obj) require.NoError(t, err) - newObject, _, err := s.codec.Decode(encodedData, nil, &dashv1.Dashboard{}) + newObject, _, err := s.codec.Decode(v.raw.Bytes(), nil, &dashv1.Dashboard{}) require.NoError(t, err) meta, err = utils.MetaAccessor(newObject) require.NoError(t, err) @@ -225,14 +225,14 @@ func TestPrepareObjectForStorage(t *testing.T) { require.NoError(t, err) meta.SetAnnotation(utils.AnnoKeyGrantPermissions, "default") - encodedData, p, err := s.prepareObjectForStorage(ctx, obj) + v, err := s.prepareObjectForStorage(ctx, obj) require.NoError(t, err) - newObject, _, err := s.codec.Decode(encodedData, nil, &dashv1.Dashboard{}) + newObject, _, err := s.codec.Decode(v.raw.Bytes(), nil, &dashv1.Dashboard{}) require.NoError(t, err) meta, err = utils.MetaAccessor(newObject) require.NoError(t, err) require.Empty(t, meta.GetAnnotation(utils.AnnoKeyGrantPermissions)) - require.Equal(t, p, "default") + require.Equal(t, v.grantPermissions, "default") }) t.Run("calculate generation", func(t *testing.T) { @@ -295,23 +295,56 @@ func TestPrepareObjectForStorage(t *testing.T) { require.Equal(t, int64(1), out.GetGeneration()) // still 1 }) }) + + t.Run("should fail invalid input", func(t *testing.T) { + _, err := s.prepareObjectForStorage(context.Background(), &dashv1.Dashboard{}) + require.Error(t, err) + require.Contains(t, err.Error(), "missing auth info") + + _, err = s.prepareObjectForUpdate(context.Background(), &dashv1.Dashboard{}, &dashv1.Dashboard{}) + require.Error(t, err) + require.Contains(t, err.Error(), "missing auth info") + + _, err = s.prepareObjectForStorage(ctx, &dashv1.Dashboard{}) + require.Error(t, err) + require.Contains(t, err.Error(), "missing name") + + _, err = s.prepareObjectForUpdate(ctx, &dashv1.Dashboard{}, &dashv1.Dashboard{}) + require.Error(t, err) + require.Contains(t, err.Error(), "updated object must have a name") + + _, err = s.prepareObjectForUpdate(ctx, &dashv1.Dashboard{ObjectMeta: v1.ObjectMeta{ + Name: "test-name", + }}, &dashv1.Dashboard{ObjectMeta: v1.ObjectMeta{ + Name: "not-the-same-name", + }}) + require.Error(t, err) + require.Contains(t, err.Error(), "name mismatch between") + + _, err = s.prepareObjectForStorage(ctx, &dashv1.Dashboard{ObjectMeta: v1.ObjectMeta{ + Name: "test-name", + ResourceVersion: "123", // RV must not be set + }}) + require.Error(t, err) + require.Equal(t, storage.ErrResourceVersionSetOnCreate, err) + }) } func getPreparedObject(t *testing.T, ctx context.Context, s *Storage, obj runtime.Object, old runtime.Object) utils.GrafanaMetaAccessor { t.Helper() - var raw []byte + var v objectForStorage var err error if old == nil { - raw, _, err = s.prepareObjectForStorage(ctx, obj) + v, err = s.prepareObjectForStorage(ctx, obj) } else { - raw, err = s.prepareObjectForUpdate(ctx, obj, old) + v, err = s.prepareObjectForUpdate(ctx, obj, old) } require.NoError(t, err) out := &unstructured.Unstructured{} - err = out.UnmarshalJSON(raw) + err = out.UnmarshalJSON(v.raw.Bytes()) require.NoError(t, err) meta, err := utils.MetaAccessor(out) @@ -324,7 +357,7 @@ func TestPrepareLargeObjectForStorage(t *testing.T) { node, err := snowflake.NewNode(rand.Int64N(1024)) require.NoError(t, err) - ctx := authtypes.WithAuthInfo(context.Background(), &identity.StaticRequester{UserID: 1, UserUID: "user-uid", Type: authtypes.TypeUser}) + ctx := authlib.WithAuthInfo(context.Background(), &identity.StaticRequester{UserID: 1, UserUID: "user-uid", Type: authlib.TypeUser}) dashboard := dashv1.Dashboard{} dashboard.Name = "test-name" @@ -341,7 +374,7 @@ func TestPrepareLargeObjectForStorage(t *testing.T) { }, } - _, _, err := f.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) + _, err := f.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) require.Nil(t, err) require.True(t, los.deconstructed) }) @@ -359,7 +392,7 @@ func TestPrepareLargeObjectForStorage(t *testing.T) { }, } - _, _, err := f.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) + _, err := f.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) require.Nil(t, err) require.False(t, los.deconstructed) }) diff --git a/pkg/storage/unified/apistore/secure.go b/pkg/storage/unified/apistore/secure.go new file mode 100644 index 00000000000..3f88ba19a63 --- /dev/null +++ b/pkg/storage/unified/apistore/secure.go @@ -0,0 +1,149 @@ +package apistore + +import ( + "context" + "fmt" + + common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/utils" + secret "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" +) + +// prepareSecureValues will create any new secure values and register changes inside the provided objectForStorage +// any call to this function MUST be followed by a call to info.finish(ctx, nil, store) to ensure that the secure values are cleaned up +func prepareSecureValues(ctx context.Context, store secret.InlineSecureValueSupport, obj utils.GrafanaMetaAccessor, previousObject utils.GrafanaMetaAccessor, v *objectForStorage) (err error) { + secure, err := obj.GetSecureValues() + if err != nil { + return err + } + + // Owner reference for inline values + v.ref = utils.ToObjectReference(obj) + + var previous common.InlineSecureValues + if previousObject == nil { + if len(secure) == 0 { + return nil // nothing needs to change + } + if store == nil { + return fmt.Errorf("secure value support is not configured (create)") + } + previous = make(common.InlineSecureValues, 0) + } else { + // Merge in any values from the previous object and handle remove + previous, err = previousObject.GetSecureValues() + if err != nil { + return err + } + for _, p := range previous { + if p.Name == "" || p.Remove || !p.Create.IsZero() { + return fmt.Errorf("invalid state, saved values must only have a name") + } + } + + // Keep exactly what we had before + if len(secure) == 0 { + if len(previous) > 0 { + return obj.SetSecureValues(previous) + } + return nil + } + + if store == nil { + return fmt.Errorf("secure value support is not configured (update)") + } + } + + for k, val := range secure { + before := previous[k] + if val.Name == "" { + if before.Name != "" { // implicitly delete previous secure value if the same field no longer references it + v.deleteSecureValues = append(v.deleteSecureValues, before.Name) + delete(previous, k) + } + if val.Remove { + if before.Name == "" { + return fmt.Errorf("cannot remove secure value '%s', it did not exist in the previous value", k) + } + delete(secure, k) + v.hasChanged = true + continue + } + if !val.Create.IsZero() { + n, err := store.CreateInline(ctx, v.ref, val.Create) + if err != nil { + return err + } + v.createdSecureValues = append(v.createdSecureValues, n) + v.hasChanged = true + secure[k] = common.InlineSecureValue{Name: n} + continue + } + return fmt.Errorf("invalid secure value state: %s", k) + } + + // The name changed from the previously stored value + if before.Name != "" && before.Name != val.Name { + // This can happen when explicitly shifting from an inline value to a shared secret + v.deleteSecureValues = append(v.deleteSecureValues, before.Name) + v.hasChanged = true + } + + delete(previous, k) + } + + // Keep all previous values that were not referenced in the update + for k, v := range previous { + _, found := secure[k] + if !found { + secure[k] = v // the previous value + } + } + + return cleanupSecureValues(v, obj, secure) +} + +// make sure the registered changes are unique and valid +func cleanupSecureValues(v *objectForStorage, obj utils.GrafanaMetaAccessor, secure common.InlineSecureValues) error { + // Make sure the deleted list is unique and does not contain any referenced values + if len(v.deleteSecureValues) > 0 && len(secure) > 0 { + confirm := v.deleteSecureValues + v.deleteSecureValues = make([]string, 0, len(v.deleteSecureValues)) + used := make(map[string]bool, len(secure)) + for _, v := range secure { + used[v.Name] = true + } + for _, name := range confirm { + if _, ok := used[name]; ok { + continue + } + used[name] = true + v.deleteSecureValues = append(v.deleteSecureValues, name) + } + } + + if len(v.deleteSecureValues) > 0 || len(v.createdSecureValues) > 0 { + v.hasChanged = true + } + return obj.SetSecureValues(secure) +} + +// Mutation hook that will delete secure values +func handleSecureValuesDelete(ctx context.Context, store secret.InlineSecureValueSupport, obj utils.GrafanaMetaAccessor) error { + secure, err := obj.GetSecureValues() + if err != nil || len(secure) == 0 { + return err + } + + if store == nil { + return fmt.Errorf("secure value support is not configured (delete)") + } + + owner := utils.ToObjectReference(obj) + for _, v := range secure { + if err = store.DeleteWhenOwnedByResource(ctx, owner, v.Name); err != nil { + return err + } + } + return obj.SetSecureValues(nil) // remove them from the object +} diff --git a/pkg/storage/unified/apistore/secure_test.go b/pkg/storage/unified/apistore/secure_test.go new file mode 100644 index 00000000000..89408121a67 --- /dev/null +++ b/pkg/storage/unified/apistore/secure_test.go @@ -0,0 +1,281 @@ +package apistore + +import ( + "context" + "encoding/json" + "fmt" + "testing" + + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/utils" + "github.com/grafana/grafana/pkg/registry/apis/secret" +) + +func TestSecureLifecycle(t *testing.T) { + resourceWithSecureValues := func(sv common.InlineSecureValues) utils.GrafanaMetaAccessor { + obj, err := utils.MetaAccessor(&unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "something.grafana.app/v1beta1", + "kind": "CustomKind", + "metadata": map[string]any{ + "namespace": "default", + "name": "test", + }, + "secure": sv, + }, + }) + require.NoError(t, err) + return obj + } + + t.Run("create secure values", func(t *testing.T) { + secureStore := secret.NewMockInlineSecureValueSupport(t) + secureStore.On("CreateInline", mock.Anything, mock.Anything, common.RawSecureValue("SecretAAA")). + Return("NameForA", nil).Once() + secureStore.On("CreateInline", mock.Anything, mock.Anything, common.RawSecureValue("SecretBBB")). + Return("NameForB", nil).Once() + + info := &objectForStorage{} + obj := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Create: "SecretAAA"}, + "b": common.InlineSecureValue{Create: "SecretBBB"}, + }) + + err := prepareSecureValues(context.Background(), secureStore, obj, nil, info) + require.NoError(t, err) + require.True(t, info.hasChanged) + require.Equal(t, []string{"NameForA", "NameForB"}, info.createdSecureValues) + secure, err := obj.GetSecureValues() + require.NoError(t, err) + require.JSONEq(t, `{ + "a": {"name": "NameForA"}, + "b": {"name": "NameForB"} + }`, asJSON(secure, true)) + secureStore.AssertExpectations(t) + }) + + t.Run("create secure values with errors", func(t *testing.T) { + obj := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Create: "SecretAAA"}, + "b": common.InlineSecureValue{Create: "SecretBBB"}, + }) + + info := &objectForStorage{} + expectError := fmt.Errorf("expected error") + secureStore := secret.NewMockInlineSecureValueSupport(t) + secureStore.On("CreateInline", mock.Anything, mock.Anything, common.RawSecureValue("SecretAAA")). + Return("", expectError).Once() + err := prepareSecureValues(context.Background(), secureStore, obj, nil, info) + require.Error(t, err, "should error when secure value creation fails") + require.Equal(t, expectError, err, "error should be propagated") + secureStore.AssertExpectations(t) + }) + + t.Run("change name manually", func(t *testing.T) { + secureStore := secret.NewMockInlineSecureValueSupport(t) + + info := &objectForStorage{} + previous := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Name: "111"}, + "b": common.InlineSecureValue{Name: "222"}, + "c": common.InlineSecureValue{Name: "333"}, + }) + obj := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Name: "222"}, + "b": common.InlineSecureValue{Name: "333"}, // no change + // "c" will be loaded from the previous object without changes + }) + + err := prepareSecureValues(context.Background(), secureStore, obj, previous, info) + require.NoError(t, err) + require.True(t, info.hasChanged) + require.Empty(t, info.createdSecureValues) + require.Equal(t, info.deleteSecureValues, []string{"111"}) // will be removed if storage succeeds + secure, err := obj.GetSecureValues() + require.NoError(t, err) + require.JSONEq(t, `{ + "a": {"name": "222"}, + "b": {"name": "333"}, + "c": {"name": "333"} + }`, asJSON(secure, true)) + }) + + t.Run("update without secrets", func(t *testing.T) { + secureStore := secret.NewMockInlineSecureValueSupport(t) + + info := &objectForStorage{} + previousObject := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Name: "NameForA"}, + "b": common.InlineSecureValue{Name: "NameForB"}, + }) + objWithoutSecrets := resourceWithSecureValues(nil) + + // Note that the secure values from the previous object are copied over + err := prepareSecureValues(context.Background(), secureStore, objWithoutSecrets, previousObject, info) + require.NoError(t, err) + require.False(t, info.hasChanged) + secure, err := objWithoutSecrets.GetSecureValues() + require.NoError(t, err) + require.JSONEq(t, `{ + "a": {"name": "NameForA"}, + "b": {"name": "NameForB"} + }`, asJSON(secure, true)) + }) + + t.Run("remove secure values", func(t *testing.T) { + secureStore := secret.NewMockInlineSecureValueSupport(t) + previous := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Name: "NameForA"}, + "b": common.InlineSecureValue{Name: "NameForB"}, + "c": common.InlineSecureValue{Name: "NameForC"}, + }) + + // Remove "b" with an explicit command + obj := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Name: "NameForA"}, // no change + "b": common.InlineSecureValue{Remove: true}, + // "c" will be loaded from the previous object + }) + + // Prepare secure values does not change anything when removing + info := &objectForStorage{} + err := prepareSecureValues(context.Background(), secureStore, obj, previous, info) + require.NoError(t, err) + require.True(t, info.hasChanged) // value was removed + secureStore.AssertExpectations(t) // nothing called + + secure, err := obj.GetSecureValues() + require.NoError(t, err) + require.JSONEq(t, `{ + "a": {"name": "NameForA"}, + "c": {"name": "NameForC"} + }`, asJSON(secure, true)) + + // When there is not an error, the finish command will do a real delete + owner := utils.ToObjectReference(obj) + secureStore.On("DeleteWhenOwnedByResource", mock.Anything, owner, "NameForB"). + Return(nil).Once() + err = info.finish(context.Background(), nil, secureStore) + require.NoError(t, err) + require.True(t, info.hasChanged) // value was removed + secureStore.AssertExpectations(t) // nothing called + + // When an error exists, no values will be deleted + err = fmt.Errorf("expected error") + outErr := info.finish(context.Background(), err, secureStore) + require.Equal(t, err, outErr, "error should be passed through") + }) + + t.Run("remove invalid secure values", func(t *testing.T) { + secureStore := secret.NewMockInlineSecureValueSupport(t) + obj := resourceWithSecureValues(common.InlineSecureValues{ + "b": common.InlineSecureValue{Remove: true}, + }) + + // Previous values must exist for remove to execute + info := &objectForStorage{} + err := prepareSecureValues(context.Background(), secureStore, obj, resourceWithSecureValues(nil), info) + require.Error(t, err, "should error when previous secure values does not exist") + require.Equal(t, "cannot remove secure value 'b', it did not exist in the previous value", err.Error()) + secureStore.AssertExpectations(t) + }) + + t.Run("delete resource", func(t *testing.T) { + secureStore := secret.NewMockInlineSecureValueSupport(t) + obj := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Name: "NameForA"}, + }) + sv, err := obj.GetSecureValues() + require.NoError(t, err) + require.Len(t, sv, 1) + + owner := utils.ToObjectReference(obj) + secureStore.On("DeleteWhenOwnedByResource", mock.Anything, owner, "NameForA"). + Return(nil).Once() + + err = handleSecureValuesDelete(context.Background(), secureStore, obj) + require.NoError(t, err) + secureStore.AssertExpectations(t) + sv, err = obj.GetSecureValues() + require.NoError(t, err) + require.Empty(t, sv, "secure values should be empty after delete") + + // Delete should propagate deletion errors + obj = resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Name: "NameForA"}, + }) + expectError := fmt.Errorf("expected error") + secureStore = secret.NewMockInlineSecureValueSupport(t) + secureStore.On("DeleteWhenOwnedByResource", mock.Anything, owner, "NameForA"). + Return(expectError).Once() + err = handleSecureValuesDelete(context.Background(), secureStore, obj) + require.Equal(t, expectError, err, "error should be passed through") + secureStore.AssertExpectations(t) + }) + + t.Run("invalid states", func(t *testing.T) { + secureStore := secret.NewMockInlineSecureValueSupport(t) + + info := &objectForStorage{} + err := prepareSecureValues(context.Background(), secureStore, resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{}, // MUST have Create, Remove or Name + }), nil, info) + require.Error(t, err) + }) + + t.Run("setup errors", func(t *testing.T) { + objWithoutSecrets := resourceWithSecureValues(nil) + objWithCreateSecret := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Create: "SecretAAA"}, + }) + invalid, _ := utils.MetaAccessor(&unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "something.grafana.app/v1beta1", + "kind": "CustomKind", + "metadata": map[string]any{ + "namespace": "default", + "name": "test", + }, + "secure": t, // something NOT a secure value + }, + }) + info := &objectForStorage{} + err := prepareSecureValues(context.Background(), nil, invalid, nil, info) + require.Error(t, err, "should error when secure values are not a map") + + err = prepareSecureValues(context.Background(), nil, objWithCreateSecret, invalid, info) + require.Error(t, err, "should error when previous secure values are not a map") + + err = prepareSecureValues(context.Background(), nil, objWithCreateSecret, nil, info) + require.Error(t, err, "should error when secure value storage is not configured") + + err = prepareSecureValues(context.Background(), nil, objWithCreateSecret, objWithoutSecrets, info) + require.Error(t, err, "should error when secure value storage is not configured") + + err = prepareSecureValues(context.Background(), nil, objWithoutSecrets, objWithCreateSecret, info) + require.Error(t, err, "should error when previous value does not have a name") + + // DELETE Setup errors + err = handleSecureValuesDelete(context.Background(), nil, invalid) + require.Error(t, err, "should error when secure values are not a map") + + err = handleSecureValuesDelete(context.Background(), nil, objWithCreateSecret) + require.Error(t, err, "should error when secure value storage is not configured") + }) +} + +func asJSON(v any, pretty bool) string { + if v == nil { + return "" + } + if pretty { + bytes, _ := json.MarshalIndent(v, "", " ") + return string(bytes) + } + bytes, _ := json.Marshal(v) + return string(bytes) +} diff --git a/pkg/storage/unified/apistore/store.go b/pkg/storage/unified/apistore/store.go index ea2761eb86e..52f36fecdcc 100644 --- a/pkg/storage/unified/apistore/store.go +++ b/pkg/storage/unified/apistore/store.go @@ -32,6 +32,7 @@ import ( "k8s.io/client-go/tools/cache" authtypes "github.com/grafana/authlib/types" + "github.com/grafana/grafana-app-sdk/logging" "github.com/grafana/grafana/pkg/apimachinery/utils" grafanaregistry "github.com/grafana/grafana/pkg/apiserver/registry/generic" secrets "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" @@ -186,33 +187,33 @@ func (s *Storage) convertToObject(data []byte, obj runtime.Object) (runtime.Obje // in seconds (0 means forever). If no error is returned and out is not nil, out will be // set to the read value from database. func (s *Storage) Create(ctx context.Context, key string, obj runtime.Object, out runtime.Object, ttl uint64) error { - var err error - var permissions string - req := &resourcepb.CreateRequest{} - req.Value, permissions, err = s.prepareObjectForStorage(ctx, obj) + v, err := s.prepareObjectForStorage(ctx, obj) if err != nil { return s.handleManagedResourceRouting(ctx, err, resourcepb.WatchEvent_ADDED, key, obj, out) } - + req := &resourcepb.CreateRequest{ + Value: v.raw.Bytes(), + } req.Key, err = s.getKey(key) if err != nil { return err } - grantPermissions, err := afterCreatePermissionCreator(ctx, req.Key, permissions, obj, s.opts.Permissions) + v.permissionCreator, err = afterCreatePermissionCreator(ctx, req.Key, v.grantPermissions, obj, s.opts.Permissions) if err != nil { return err } rsp, err := s.store.Create(ctx, req) if err != nil { - return resource.GetError(resource.AsErrorResult(err)) + return v.finish(ctx, resource.GetError(resource.AsErrorResult(err)), s.opts.SecureValues) } if rsp.Error != nil { + err = resource.GetError(rsp.Error) if rsp.Error.Code == http.StatusConflict { - return storage.NewKeyExistsError(key, 0) + err = storage.NewKeyExistsError(key, 0) } - return resource.GetError(rsp.Error) + return v.finish(ctx, err, s.opts.SecureValues) } if _, err := s.convertToObject(req.Value, out); err != nil { @@ -234,12 +235,7 @@ func (s *Storage) Create(ctx context.Context, key string, obj runtime.Object, ou }) } - // Synchronous AfterCreate permissions -- allows users to become "admin" of the thing they made - if grantPermissions != nil { - return grantPermissions(ctx) - } - - return nil + return v.finish(ctx, nil, s.opts.SecureValues) } // Delete removes the specified key and returns the value that existed at that spot. @@ -308,6 +304,11 @@ func (s *Storage) Delete( if rsp.Error != nil { return resource.GetError(rsp.Error) } + + if err = handleSecureValuesDelete(ctx, s.opts.SecureValues, meta); err != nil { + logging.FromContext(ctx).Warn("failed to delete inline secure values", "err", err) + } + if err := s.versioner.UpdateObject(out, uint64(rsp.ResourceVersion)); err != nil { return err } @@ -591,21 +592,27 @@ func (s *Storage) GuaranteedUpdate( break } - req.Value, err = s.prepareObjectForUpdate(ctx, updatedObj, existingObj) + v, err := s.prepareObjectForUpdate(ctx, updatedObj, existingObj) if err != nil { return s.handleManagedResourceRouting(ctx, err, resourcepb.WatchEvent_MODIFIED, key, updatedObj, destination) } - var rv uint64 // Only update (for real) if the bytes have changed + var rv uint64 + req.Value = v.raw.Bytes() if !bytes.Equal(req.Value, existingBytes) { updateResponse, err := s.store.Update(ctx, req) if err != nil { - return resource.GetError(resource.AsErrorResult(err)) + err = resource.GetError(resource.AsErrorResult(err)) + } else if updateResponse.Error != nil { + err = resource.GetError(updateResponse.Error) } - if updateResponse.Error != nil { - return resource.GetError(updateResponse.Error) + + // Cleanup secure values + if err = v.finish(ctx, err, s.opts.SecureValues); err != nil { + return err } + rv = uint64(updateResponse.ResourceVersion) } diff --git a/pkg/storage/unified/resource/errors.go b/pkg/storage/unified/resource/errors.go index 903f6e4cff9..79d3911e470 100644 --- a/pkg/storage/unified/resource/errors.go +++ b/pkg/storage/unified/resource/errors.go @@ -4,13 +4,14 @@ import ( "errors" "net/http" + "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" + grpcstatus "google.golang.org/grpc/status" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation/field" - "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" - grpcstatus "google.golang.org/grpc/status" - + "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" "github.com/grafana/grafana/pkg/util/scheduler" ) @@ -59,6 +60,58 @@ func NewTooManyRequestsError(msg string) *resourcepb.ErrorResult { } } +func newInvalidFieldError( + obj utils.GrafanaMetaAccessor, + detail string, + path string, + morePath ...string, +) *resourcepb.ErrorResult { + gvk := obj.GetGroupVersionKind() + return &resourcepb.ErrorResult{ + Message: detail, + Code: http.StatusUnprocessableEntity, + Reason: string(metav1.StatusReasonInvalid), + Details: &resourcepb.ErrorDetails{ + Name: obj.GetName(), + Group: gvk.Group, + Kind: gvk.Kind, + Uid: string(obj.GetUID()), + Causes: []*resourcepb.ErrorCause{ + { + Reason: string(field.ErrorTypeForbidden), + Field: field.NewPath(path, morePath...).String(), + }, + }, + }, + } +} + +func newRequiredFieldError( + obj utils.GrafanaMetaAccessor, + detail string, + path string, + morePath ...string, +) *resourcepb.ErrorResult { + gvk := obj.GetGroupVersionKind() + return &resourcepb.ErrorResult{ + Message: detail, + Code: http.StatusUnprocessableEntity, + Reason: string(metav1.StatusReasonInvalid), + Details: &resourcepb.ErrorDetails{ + Name: obj.GetName(), + Group: gvk.Group, + Kind: gvk.Kind, + Uid: string(obj.GetUID()), + Causes: []*resourcepb.ErrorCause{ + { + Reason: string(field.ErrorTypeRequired), + Field: field.NewPath(path, morePath...).String(), + }, + }, + }, + } +} + // Convert golang errors to status result errors that can be returned to a client func AsErrorResult(err error) *resourcepb.ErrorResult { if err == nil { diff --git a/pkg/storage/unified/resource/secure.go b/pkg/storage/unified/resource/secure.go new file mode 100644 index 00000000000..18ff83af7e7 --- /dev/null +++ b/pkg/storage/unified/resource/secure.go @@ -0,0 +1,81 @@ +package resource + +import ( + "context" + "net/http" + "slices" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/grafana/grafana/pkg/apimachinery/utils" + secrets "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" + "github.com/grafana/grafana/pkg/storage/unified/resourcepb" +) + +// The "CanReference" check exists to avoid writing references to secrets +// the user should not allow granting access. We only check it when the value changes +func canReferenceSecureValues(ctx context.Context, + obj utils.GrafanaMetaAccessor, + old utils.GrafanaMetaAccessor, + secrets secrets.InlineSecureValueSupport, +) *resourcepb.ErrorResult { + secure, err := obj.GetSecureValues() + if err != nil || len(secure) == 0 { + return AsErrorResult(err) + } + + if secrets == nil { + return &resourcepb.ErrorResult{ + Message: "secure storage not configured", + Code: http.StatusServiceUnavailable, + Reason: string(metav1.StatusReasonServiceUnavailable), + } + } + + // All references should only set a name + names := make([]string, 0, len(secure)) + for k, v := range secure { + if !v.Create.IsZero() { + return newInvalidFieldError(obj, + "unable to create values in unified storage", + "secure", k, "create") + } + if v.Remove { + return newInvalidFieldError(obj, + "unable to save the remove command", + "secure", k, "remove") + } + if v.Name == "" { + return newRequiredFieldError(obj, + "missing name", + "secure", k, "name") + } + names = append(names, v.Name) + } + + // This will call the real service to check access, converting any errors to protobuf + canReference := func() *resourcepb.ErrorResult { + slices.Sort(names) + names = slices.Compact(names) // + if err := secrets.CanReference(ctx, utils.ToObjectReference(obj), names...); err != nil { + return AsErrorResult(err) + } + return nil + } + + // Always check for create + if old == nil { + return canReference() + } + + oldSecureValues, err := old.GetSecureValues() + if err != nil || len(secure) != len(oldSecureValues) { + return canReference() + } + for k, v := range secure { + if oldSecureValues[k].Name != v.Name { + return canReference() + } + } + return nil // no need to check if the values are the same +} diff --git a/pkg/storage/unified/resource/secure_test.go b/pkg/storage/unified/resource/secure_test.go new file mode 100644 index 00000000000..67388c5ae4c --- /dev/null +++ b/pkg/storage/unified/resource/secure_test.go @@ -0,0 +1,188 @@ +package resource + +import ( + "context" + "fmt" + "net/http" + "testing" + + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/utils" + "github.com/grafana/grafana/pkg/registry/apis/secret" +) + +func TestSecureValues(t *testing.T) { + raw := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "playlist.grafana.app/v0alpha1", + "kind": "Playlist", + "metadata": map[string]any{ + "name": "nn", + "namespace": "ns", + }, + "spec": map[string]any{ + "title": "hello", + }, + "secure": map[string]any{}, // empty + }, + } + obj, err := utils.MetaAccessor(raw) + require.NoError(t, err) + owner := utils.ToObjectReference(obj) + + t.Run("Invalid input", func(t *testing.T) { + raw.Object["secure"] = map[string]any{ + "A": common.InlineSecureValue{ + Create: common.NewSecretValue("XXX"), + }, + } + secureMock := secret.NewMockInlineSecureValueSupport(t) + pberr := canReferenceSecureValues(context.Background(), obj, nil, nil) + require.Equal(t, http.StatusServiceUnavailable, int(pberr.Code), "missing store") + + t.Run("create", func(t *testing.T) { + raw.Object["secure"] = map[string]any{ + "A": common.InlineSecureValue{ + Create: common.NewSecretValue("XXX"), + }, + } + pberr = canReferenceSecureValues(context.Background(), obj, nil, secureMock) + require.Equal(t, http.StatusUnprocessableEntity, int(pberr.Code)) + require.Equal(t, "secure.A.create", pberr.Details.Causes[0].Field) + }) + + t.Run("remove", func(t *testing.T) { + raw.Object["secure"] = map[string]any{ + "A": common.InlineSecureValue{ + Remove: true, + }, + } + pberr = canReferenceSecureValues(context.Background(), obj, nil, secureMock) + require.Equal(t, http.StatusUnprocessableEntity, int(pberr.Code)) + require.Equal(t, "secure.A.remove", pberr.Details.Causes[0].Field) + }) + + t.Run("missing name", func(t *testing.T) { + raw.Object["secure"] = map[string]any{ + "A": common.InlineSecureValue{ + Name: "", // EMPTY + }, + } + pberr = canReferenceSecureValues(context.Background(), obj, nil, secureMock) + require.Equal(t, http.StatusUnprocessableEntity, int(pberr.Code)) + require.Equal(t, "secure.A.name", pberr.Details.Causes[0].Field) + }) + + secureMock.AssertExpectations(t) + }) + + t.Run("OnCreate", func(t *testing.T) { + raw.Object["secure"] = map[string]any{ + "A": common.InlineSecureValue{ + Name: "111", + }, + "B": common.InlineSecureValue{ + Name: "111", // duplicate reference, but only checked once + }, + } + secureMock := secret.NewMockInlineSecureValueSupport(t) + secureMock.On("CanReference", mock.Anything, owner, "111"). + Return(nil).Once() + + pberr := canReferenceSecureValues(context.Background(), obj, nil, secureMock) + require.Nil(t, pberr) + secureMock.AssertExpectations(t) + }) + + t.Run("OnUpdate", func(t *testing.T) { + raw.Object["secure"] = map[string]any{ + "A": common.InlineSecureValue{ + Name: "111", + }, + "B": common.InlineSecureValue{ + Name: "222", + }, + } + + old, _ := utils.MetaAccessor(&unstructured.Unstructured{}) + secureMock := secret.NewMockInlineSecureValueSupport(t) + secureMock.On("CanReference", mock.Anything, owner, "111", "222"). + Return(nil).Once() + + pberr := canReferenceSecureValues(context.Background(), obj, old, secureMock) + require.Nil(t, pberr) + secureMock.AssertExpectations(t) + }) + + t.Run("OnUpdate with same keys", func(t *testing.T) { + raw.Object["secure"] = map[string]any{ + "A": common.InlineSecureValue{ + Name: "111", + }, + "B": common.InlineSecureValue{ + Name: "222", + }, + } + + old, _ := utils.MetaAccessor(&unstructured.Unstructured{ + Object: map[string]any{ + "secure": map[string]any{ + "A": common.InlineSecureValue{ + Name: "111", // same + }, + "B": common.InlineSecureValue{ + Name: "Not222", + }, + }}}) + secureMock := secret.NewMockInlineSecureValueSupport(t) + secureMock.On("CanReference", mock.Anything, owner, "111", "222"). + Return(nil).Once() + + pberr := canReferenceSecureValues(context.Background(), obj, old, secureMock) + require.Nil(t, pberr) + secureMock.AssertExpectations(t) + }) + + t.Run("Update without changes should skip CanReference", func(t *testing.T) { + raw.Object["secure"] = map[string]any{ + "A": common.InlineSecureValue{ + Name: "111", + }, + } + secureMock := secret.NewMockInlineSecureValueSupport(t) + + pberr := canReferenceSecureValues(context.Background(), obj, obj, secureMock) + require.Nil(t, pberr) + secureMock.AssertExpectations(t) // CanReference should not be called + }) + + t.Run("upstream errors", func(t *testing.T) { + raw.Object["secure"] = map[string]any{ + "A": common.InlineSecureValue{ + Name: "111", + }, + } + secureMock := secret.NewMockInlineSecureValueSupport(t) + secureMock.On("CanReference", mock.Anything, owner, "111"). + Return(fmt.Errorf("nope")).Once() // <<< error in CanReference + + pberr := canReferenceSecureValues(context.Background(), obj, nil, secureMock) + require.NotNil(t, pberr) + secureMock.AssertExpectations(t) + + // Check CanReference when the old value is invalid + old, _ := utils.MetaAccessor(&unstructured.Unstructured{ + Object: map[string]any{"secure": t}}) + + secureMock = secret.NewMockInlineSecureValueSupport(t) + secureMock.On("CanReference", mock.Anything, owner, "111"). + Return(nil).Once() + pberr = canReferenceSecureValues(context.Background(), obj, old, secureMock) + require.Nil(t, pberr) + secureMock.AssertExpectations(t) + }) +} diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index 39e38062fb5..c41b187af16 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -234,7 +234,7 @@ type ResourceServerOptions struct { RingLifecycler *ring.BasicLifecycler } -func NewResourceServer(opts ResourceServerOptions) (ResourceServer, error) { +func NewResourceServer(opts ResourceServerOptions) (*server, error) { if opts.Tracer == nil { opts.Tracer = noop.NewTracerProvider().Tracer("resource-server") } @@ -473,17 +473,8 @@ func (s *server) newEvent(ctx context.Context, user claims.AuthInfo, key *resour } // Verify that this resource can reference secure values - secure, err := obj.GetSecureValues() - if err != nil { - return nil, AsErrorResult(err) - } - if len(secure) > 0 { - if s.secure == nil { - return nil, NewBadRequestError("secure storage not configured") - } - - // See: https://github.com/grafana/grafana/pull/107803 - return nil, NewBadRequestError("Saving secure values is not yet supported") + if err := canReferenceSecureValues(ctx, obj, event.ObjectOld, s.secure); err != nil { + return nil, err } if key.Namespace != obj.GetNamespace() { diff --git a/pkg/storage/unified/resource/server_test.go b/pkg/storage/unified/resource/server_test.go index da92e67d7c0..b0dff754a18 100644 --- a/pkg/storage/unified/resource/server_test.go +++ b/pkg/storage/unified/resource/server_test.go @@ -14,8 +14,7 @@ import ( "gocloud.dev/blob/memblob" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - claims "github.com/grafana/authlib/types" - + authlib "github.com/grafana/authlib/types" "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" @@ -23,14 +22,14 @@ import ( func TestSimpleServer(t *testing.T) { testUserA := &identity.StaticRequester{ - Type: claims.TypeUser, + Type: authlib.TypeUser, Login: "testuser", UserID: 123, UserUID: "u123", OrgRole: identity.RoleAdmin, IsGrafanaAdmin: true, // can do anything } - ctx := claims.WithAuthInfo(context.Background(), testUserA) + ctx := authlib.WithAuthInfo(context.Background(), testUserA) bucket := memblob.OpenBucket(nil) if false {