From baa89f3eac21cb8b2eae8a1a5fd5c2cd08d06e59 Mon Sep 17 00:00:00 2001 From: Bruno Date: Mon, 14 Jul 2025 09:28:07 -0300 Subject: [PATCH] Secrets: encryption encryption storage uses versioning (#108036) * Secrets: delete unused FakeKeeper * Secrets: encrypted value storage stores versions * add version to span * trigger build * remove ineffectual assignment * lint * drop secret_encrypted_value.uid / add name and version columns --- .../apis/secret/contracts/encryption.go | 11 +- pkg/registry/apis/secret/contracts/keeper.go | 8 +- .../secret/secretkeeper/fakes/fake_keeper.go | 68 ----- .../secret/secretkeeper/sqlkeeper/keeper.go | 38 +-- .../secretkeeper/sqlkeeper/keeper_test.go | 154 +++++------ .../apis/secret/service/secure_value.go | 18 +- .../apis/secret/testutils/testutils.go | 10 +- .../data/encrypted_value_create.sql | 6 +- .../data/encrypted_value_delete.sql | 6 +- .../encryption/data/encrypted_value_read.sql | 9 +- .../data/encrypted_value_update.sql | 6 +- .../encryption/encrypted_value_model.go | 3 +- .../encryption/encrypted_value_store.go | 55 ++-- .../encryption/encrypted_value_store_test.go | 241 +++++++++++++++--- pkg/storage/secret/encryption/query.go | 9 +- pkg/storage/secret/encryption/query_test.go | 12 +- .../mysql--encrypted_value_create-create.sql | 6 +- .../mysql--encrypted_value_delete-delete.sql | 6 +- .../mysql--encrypted_value_read-read.sql | 9 +- .../mysql--encrypted_value_update-update.sql | 6 +- ...ostgres--encrypted_value_create-create.sql | 6 +- ...ostgres--encrypted_value_delete-delete.sql | 6 +- .../postgres--encrypted_value_read-read.sql | 9 +- ...ostgres--encrypted_value_update-update.sql | 6 +- .../sqlite--encrypted_value_create-create.sql | 6 +- .../sqlite--encrypted_value_delete-delete.sql | 6 +- .../sqlite--encrypted_value_read-read.sql | 9 +- .../sqlite--encrypted_value_update-update.sql | 6 +- pkg/storage/secret/metadata/decrypt_store.go | 8 +- .../secret/metadata/secure_value_store.go | 1 + pkg/storage/secret/migrator/migrator.go | 12 +- 31 files changed, 454 insertions(+), 302 deletions(-) delete mode 100644 pkg/registry/apis/secret/secretkeeper/fakes/fake_keeper.go diff --git a/pkg/registry/apis/secret/contracts/encryption.go b/pkg/registry/apis/secret/contracts/encryption.go index 176fb8ace24..eb5292ee794 100644 --- a/pkg/registry/apis/secret/contracts/encryption.go +++ b/pkg/registry/apis/secret/contracts/encryption.go @@ -13,16 +13,17 @@ type EncryptionManager interface { } type EncryptedValue struct { - UID string Namespace string + Name string + Version int64 EncryptedData []byte Created int64 Updated int64 } type EncryptedValueStorage interface { - Create(ctx context.Context, namespace string, encryptedData []byte) (*EncryptedValue, error) - Update(ctx context.Context, namespace string, uid string, encryptedData []byte) error - Get(ctx context.Context, namespace string, uid string) (*EncryptedValue, error) - Delete(ctx context.Context, namespace string, uid string) error + Create(ctx context.Context, namespace, name string, version int64, encryptedData []byte) (*EncryptedValue, error) + Update(ctx context.Context, namespace, name string, version int64, encryptedData []byte) error + Get(ctx context.Context, namespace, name string, version int64) (*EncryptedValue, error) + Delete(ctx context.Context, namespace, name string, version int64) error } diff --git a/pkg/registry/apis/secret/contracts/keeper.go b/pkg/registry/apis/secret/contracts/keeper.go index b8d25cf651a..48db9a13dc0 100644 --- a/pkg/registry/apis/secret/contracts/keeper.go +++ b/pkg/registry/apis/secret/contracts/keeper.go @@ -95,10 +95,10 @@ func (s ExternalID) String() string { // Keeper is the interface for secret keepers. type Keeper interface { - Store(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, exposedValueOrRef string) (ExternalID, error) - Update(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, externalID ExternalID, exposedValueOrRef string) error - Expose(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, externalID ExternalID) (secretv1beta1.ExposedSecureValue, error) - Delete(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, externalID ExternalID) error + Store(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace, name string, version int64, exposedValueOrRef string) (ExternalID, error) + Update(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace, name string, version int64, exposedValueOrRef string) error + Expose(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace, name string, version int64) (secretv1beta1.ExposedSecureValue, error) + Delete(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace, name string, version int64) error } // Service is the interface for secret keeper services. diff --git a/pkg/registry/apis/secret/secretkeeper/fakes/fake_keeper.go b/pkg/registry/apis/secret/secretkeeper/fakes/fake_keeper.go deleted file mode 100644 index d24f9c0221c..00000000000 --- a/pkg/registry/apis/secret/secretkeeper/fakes/fake_keeper.go +++ /dev/null @@ -1,68 +0,0 @@ -package fakes - -import ( - "context" - "errors" - - "github.com/google/uuid" - - secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1" - "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" -) - -var ErrSecretNotFound = errors.New("secret not found") - -type FakeKeeper struct { - values map[string]map[string]string -} - -var _ contracts.Keeper = (*FakeKeeper)(nil) - -func NewFakeKeeper() *FakeKeeper { - return &FakeKeeper{ - values: make(map[string]map[string]string), - } -} - -func (s *FakeKeeper) Store(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, exposedValueOrRef string) (contracts.ExternalID, error) { - ns, ok := s.values[namespace] - if !ok { - ns = make(map[string]string) - } - uid := uuid.New().String() - ns[uid] = exposedValueOrRef - s.values[namespace] = ns - - return contracts.ExternalID(uid), nil -} - -func (s *FakeKeeper) Expose(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, externalID contracts.ExternalID) (secretv1beta1.ExposedSecureValue, error) { - ns, ok := s.values[namespace] - if !ok { - return "", ErrSecretNotFound - } - exposedVal, ok := ns[externalID.String()] - if !ok { - return "", ErrSecretNotFound - } - - return secretv1beta1.NewExposedSecureValue(exposedVal), nil -} - -func (s *FakeKeeper) Delete(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, externalID contracts.ExternalID) error { - return nil -} - -func (s *FakeKeeper) Update(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, externalID contracts.ExternalID, exposedValueOrRef string) error { - ns, ok := s.values[namespace] - if !ok { - return ErrSecretNotFound - } - _, ok = ns[externalID.String()] - if !ok { - return ErrSecretNotFound - } - - ns[externalID.String()] = exposedValueOrRef - return nil -} diff --git a/pkg/registry/apis/secret/secretkeeper/sqlkeeper/keeper.go b/pkg/registry/apis/secret/secretkeeper/sqlkeeper/keeper.go index a65052c40f0..bfe3972f72b 100644 --- a/pkg/registry/apis/secret/secretkeeper/sqlkeeper/keeper.go +++ b/pkg/registry/apis/secret/secretkeeper/sqlkeeper/keeper.go @@ -36,8 +36,13 @@ func NewSQLKeeper( } } -func (s *SQLKeeper) Store(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, exposedValueOrRef string) (contracts.ExternalID, error) { - ctx, span := s.tracer.Start(ctx, "SQLKeeper.Store", trace.WithAttributes(attribute.String("namespace", namespace))) +func (s *SQLKeeper) Store(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace, name string, version int64, exposedValueOrRef string) (contracts.ExternalID, error) { + ctx, span := s.tracer.Start(ctx, "SQLKeeper.Store", + trace.WithAttributes( + attribute.String("namespace", namespace), + attribute.String("name", name), + attribute.Int64("version", version)), + ) defer span.End() start := time.Now() @@ -46,27 +51,28 @@ func (s *SQLKeeper) Store(ctx context.Context, cfg secretv1beta1.KeeperConfig, n return "", fmt.Errorf("unable to encrypt value: %w", err) } - encryptedVal, err := s.store.Create(ctx, namespace, encryptedData) + _, err = s.store.Create(ctx, namespace, name, version, encryptedData) if err != nil { return "", fmt.Errorf("unable to store encrypted value: %w", err) } s.metrics.StoreDuration.WithLabelValues(string(cfg.Type())).Observe(time.Since(start).Seconds()) - externalID := contracts.ExternalID(encryptedVal.UID) - span.SetAttributes(attribute.String("externalID", externalID.String())) - return externalID, nil + // An external id is not required to interact with the sql keeper. + // An empty string is returned just to comply with the Keeper interface. + return contracts.ExternalID(""), nil } -func (s *SQLKeeper) Expose(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, externalID contracts.ExternalID) (secretv1beta1.ExposedSecureValue, error) { +func (s *SQLKeeper) Expose(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace, name string, version int64) (secretv1beta1.ExposedSecureValue, error) { ctx, span := s.tracer.Start(ctx, "SQLKeeper.Expose", trace.WithAttributes( attribute.String("namespace", namespace), - attribute.String("externalID", externalID.String()), + attribute.String("name", name), + attribute.Int64("version", version), )) defer span.End() start := time.Now() - encryptedValue, err := s.store.Get(ctx, namespace, externalID.String()) + encryptedValue, err := s.store.Get(ctx, namespace, name, version) if err != nil { return "", fmt.Errorf("unable to get encrypted value: %w", err) } @@ -82,15 +88,16 @@ func (s *SQLKeeper) Expose(ctx context.Context, cfg secretv1beta1.KeeperConfig, return exposedValue, nil } -func (s *SQLKeeper) Delete(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, externalID contracts.ExternalID) error { +func (s *SQLKeeper) Delete(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace, name string, version int64) error { ctx, span := s.tracer.Start(ctx, "SQLKeeper.Delete", trace.WithAttributes( attribute.String("namespace", namespace), - attribute.String("externalID", externalID.String()), + attribute.String("name", name), + attribute.Int64("version", version), )) defer span.End() start := time.Now() - err := s.store.Delete(ctx, namespace, externalID.String()) + err := s.store.Delete(ctx, namespace, name, version) if err != nil { return fmt.Errorf("failed to delete encrypted value: %w", err) } @@ -100,10 +107,11 @@ func (s *SQLKeeper) Delete(ctx context.Context, cfg secretv1beta1.KeeperConfig, return nil } -func (s *SQLKeeper) Update(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace string, externalID contracts.ExternalID, exposedValueOrRef string) error { +func (s *SQLKeeper) Update(ctx context.Context, cfg secretv1beta1.KeeperConfig, namespace, name string, version int64, exposedValueOrRef string) error { ctx, span := s.tracer.Start(ctx, "SQLKeeper.Update", trace.WithAttributes( attribute.String("namespace", namespace), - attribute.String("externalID", externalID.String()), + attribute.String("name", name), + attribute.Int64("version", version), )) defer span.End() @@ -113,7 +121,7 @@ func (s *SQLKeeper) Update(ctx context.Context, cfg secretv1beta1.KeeperConfig, return fmt.Errorf("unable to encrypt value: %w", err) } - err = s.store.Update(ctx, namespace, externalID.String(), encryptedData) + err = s.store.Update(ctx, namespace, name, version, encryptedData) if err != nil { return fmt.Errorf("failed to update encrypted value: %w", err) } diff --git a/pkg/registry/apis/secret/secretkeeper/sqlkeeper/keeper_test.go b/pkg/registry/apis/secret/secretkeeper/sqlkeeper/keeper_test.go index 902c1f64b63..1333cf894c7 100644 --- a/pkg/registry/apis/secret/secretkeeper/sqlkeeper/keeper_test.go +++ b/pkg/registry/apis/secret/secretkeeper/sqlkeeper/keeper_test.go @@ -1,23 +1,13 @@ -package sqlkeeper +package sqlkeeper_test import ( - "context" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.opentelemetry.io/otel/trace/noop" secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1" - "github.com/grafana/grafana/pkg/infra/usagestats" - "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" - encryptionmanager "github.com/grafana/grafana/pkg/registry/apis/secret/encryption/manager" - "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/grafana/grafana/pkg/setting" - "github.com/grafana/grafana/pkg/storage/secret/database" - encryptionstorage "github.com/grafana/grafana/pkg/storage/secret/encryption" - "github.com/grafana/grafana/pkg/storage/secret/migrator" + "github.com/grafana/grafana/pkg/registry/apis/secret/testutils" "github.com/grafana/grafana/pkg/tests/testsuite" ) @@ -26,159 +16,133 @@ func TestMain(m *testing.M) { } func Test_SQLKeeperSetup(t *testing.T) { - ctx := context.Background() namespace1 := "namespace1" + name1 := "name1" + version1 := int64(1) namespace2 := "namespace2" + name2 := "name2" plaintext1 := "very secret string in namespace 1" plaintext2 := "very secret string in namespace 2" - nonExistentID := contracts.ExternalID("non existent") - - cfg := &setting.Cfg{ - SecretsManagement: setting.SecretsManagerSettings{ - SecretKey: "sdDkslslld", - EncryptionProvider: "secretKey.v1", - }, - } - - sqlKeeper, err := setupTestService(t, cfg) - require.NoError(t, err) - require.NotNil(t, sqlKeeper) keeperCfg := &secretv1beta1.SystemKeeperConfig{} t.Run("storing an encrypted value returns no error", func(t *testing.T) { - externalId1, err := sqlKeeper.Store(ctx, keeperCfg, namespace1, plaintext1) - require.NoError(t, err) - require.NotEmpty(t, externalId1) + sut := testutils.Setup(t) - externalId2, err := sqlKeeper.Store(ctx, keeperCfg, namespace2, plaintext2) + _, err := sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace1, name1, version1, plaintext1) + require.NoError(t, err) + + _, err = sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace2, name2, version1, plaintext2) require.NoError(t, err) - require.NotEmpty(t, externalId2) t.Run("expose the encrypted value from existing namespace", func(t *testing.T) { - exposedVal1, err := sqlKeeper.Expose(ctx, keeperCfg, namespace1, externalId1) + sut := testutils.Setup(t) + + _, err := sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace1, name1, version1, plaintext1) + require.NoError(t, err) + + exposedVal1, err := sut.SQLKeeper.Expose(t.Context(), keeperCfg, namespace1, name1, version1) require.NoError(t, err) require.NotNil(t, exposedVal1) assert.Equal(t, plaintext1, exposedVal1.DangerouslyExposeAndConsumeValue()) - exposedVal2, err := sqlKeeper.Expose(ctx, keeperCfg, namespace2, externalId2) + _, err = sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace2, name2, version1, plaintext2) + require.NoError(t, err) + + exposedVal2, err := sut.SQLKeeper.Expose(t.Context(), keeperCfg, namespace2, name2, version1) require.NoError(t, err) require.NotNil(t, exposedVal2) assert.Equal(t, plaintext2, exposedVal2.DangerouslyExposeAndConsumeValue()) }) t.Run("expose encrypted value from different namespace returns error", func(t *testing.T) { - exposedVal, err := sqlKeeper.Expose(ctx, keeperCfg, namespace2, externalId1) + sut := testutils.Setup(t) + + _, err = sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace1, name1, version1, plaintext1) + require.NoError(t, err) + + exposedVal, err := sut.SQLKeeper.Expose(t.Context(), keeperCfg, namespace2, name1, version1) require.Error(t, err) assert.Empty(t, exposedVal) - exposedVal, err = sqlKeeper.Expose(ctx, keeperCfg, namespace1, externalId2) + exposedVal, err = sut.SQLKeeper.Expose(t.Context(), keeperCfg, namespace1, name2, version1) require.Error(t, err) assert.Empty(t, exposedVal) }) }) - t.Run("storing same value in same namespace returns no error", func(t *testing.T) { - externalId1, err := sqlKeeper.Store(ctx, keeperCfg, namespace1, plaintext1) - require.NoError(t, err) - require.NotEmpty(t, externalId1) + t.Run("storing same value in same namespace returns error", func(t *testing.T) { + sut := testutils.Setup(t) - externalId2, err := sqlKeeper.Store(ctx, keeperCfg, namespace1, plaintext1) + _, err := sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace1, name1, version1, plaintext1) require.NoError(t, err) - require.NotEmpty(t, externalId2) - assert.NotEqual(t, externalId1, externalId2) + _, err = sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace1, name1, version1, plaintext1) + require.NotNil(t, err) }) t.Run("storing same value in different namespace returns no error", func(t *testing.T) { - externalId1, err := sqlKeeper.Store(ctx, keeperCfg, namespace1, plaintext1) - require.NoError(t, err) - require.NotEmpty(t, externalId1) + sut := testutils.Setup(t) - externalId2, err := sqlKeeper.Store(ctx, keeperCfg, namespace2, plaintext1) + _, err := sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace1, name1, version1, plaintext1) require.NoError(t, err) - require.NotEmpty(t, externalId2) - assert.NotEqual(t, externalId1, externalId2) + _, err = sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace2, name1, version1, plaintext1) + require.NoError(t, err) }) t.Run("exposing non existing values returns error", func(t *testing.T) { - exposedVal, err := sqlKeeper.Expose(ctx, keeperCfg, namespace1, nonExistentID) + sut := testutils.Setup(t) + + exposedVal, err := sut.SQLKeeper.Expose(t.Context(), keeperCfg, namespace1, "non_existing_name", version1) require.Error(t, err) assert.Empty(t, exposedVal) }) t.Run("deleting an existing encrypted value does not return error", func(t *testing.T) { - externalID, err := sqlKeeper.Store(ctx, keeperCfg, namespace1, plaintext1) - require.NoError(t, err) - require.NotEmpty(t, externalID) + sut := testutils.Setup(t) - exposedVal, err := sqlKeeper.Expose(ctx, keeperCfg, namespace1, externalID) + _, err := sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace1, name1, version1, plaintext1) + require.NoError(t, err) + + exposedVal, err := sut.SQLKeeper.Expose(t.Context(), keeperCfg, namespace1, name1, version1) require.NoError(t, err) assert.NotNil(t, exposedVal) assert.Equal(t, plaintext1, exposedVal.DangerouslyExposeAndConsumeValue()) - err = sqlKeeper.Delete(ctx, keeperCfg, namespace1, externalID) + err = sut.SQLKeeper.Delete(t.Context(), keeperCfg, namespace1, name1, version1) require.NoError(t, err) }) t.Run("deleting an non existing encrypted value does not return error", func(t *testing.T) { - err = sqlKeeper.Delete(ctx, keeperCfg, namespace1, nonExistentID) + sut := testutils.Setup(t) + + err := sut.SQLKeeper.Delete(t.Context(), keeperCfg, namespace1, "non_existing_name", version1) require.NoError(t, err) }) t.Run("updating an existent encrypted value returns no error", func(t *testing.T) { - externalId1, err := sqlKeeper.Store(ctx, keeperCfg, namespace1, plaintext1) - require.NoError(t, err) - require.NotEmpty(t, externalId1) + sut := testutils.Setup(t) - err = sqlKeeper.Update(ctx, keeperCfg, namespace1, externalId1, plaintext2) + _, err := sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace1, name1, version1, plaintext1) require.NoError(t, err) - exposedVal, err := sqlKeeper.Expose(ctx, keeperCfg, namespace1, externalId1) + err = sut.SQLKeeper.Update(t.Context(), keeperCfg, namespace1, name1, version1, plaintext2) + require.NoError(t, err) + + exposedVal, err := sut.SQLKeeper.Expose(t.Context(), keeperCfg, namespace1, name1, version1) require.NoError(t, err) assert.NotNil(t, exposedVal) assert.Equal(t, plaintext2, exposedVal.DangerouslyExposeAndConsumeValue()) }) t.Run("updating a non existent encrypted value returns error", func(t *testing.T) { - externalId1, err := sqlKeeper.Store(ctx, keeperCfg, namespace1, plaintext1) - require.NoError(t, err) - require.NotEmpty(t, externalId1) + sut := testutils.Setup(t) - err = sqlKeeper.Update(ctx, nil, namespace1, nonExistentID, plaintext2) + _, err := sut.SQLKeeper.Store(t.Context(), keeperCfg, namespace1, name1, version1, plaintext1) + require.NoError(t, err) + + err = sut.SQLKeeper.Update(t.Context(), nil, namespace1, "non_existing_name", version1, plaintext2) require.Error(t, err) }) } - -func setupTestService(t *testing.T, cfg *setting.Cfg) (*SQLKeeper, error) { - testDB := sqlstore.NewTestStore(t, sqlstore.WithMigrator(migrator.New())) - tracer := noop.NewTracerProvider().Tracer("test") - database := database.ProvideDatabase(testDB, tracer) - - features := featuremgmt.WithFeatures(featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, featuremgmt.FlagSecretsManagementAppPlatform) - - // Initialize the encryption manager - dataKeyStore, err := encryptionstorage.ProvideDataKeyStorage(database, tracer, features, nil) - require.NoError(t, err) - - usageStats := &usagestats.UsageStatsMock{T: t} - - encMgr, err := encryptionmanager.ProvideEncryptionManager( - tracer, - dataKeyStore, - cfg, - usageStats, - nil, - ) - require.NoError(t, err) - - // Initialize encrypted value storage with a fake db - encValueStore, err := encryptionstorage.ProvideEncryptedValueStorage(database, tracer, features) - require.NoError(t, err) - - // Initialize the SQLKeeper - sqlKeeper := NewSQLKeeper(tracer, encMgr, encValueStore, nil) - - return sqlKeeper, nil -} diff --git a/pkg/registry/apis/secret/service/secure_value.go b/pkg/registry/apis/secret/service/secure_value.go index 6170bca37b6..3b9de019458 100644 --- a/pkg/registry/apis/secret/service/secure_value.go +++ b/pkg/registry/apis/secret/service/secure_value.go @@ -60,7 +60,7 @@ func (s *SecureValueService) Update(ctx context.Context, newSecureValue *secretv defer span.End() if newSecureValue.Spec.Value == nil { - decrypted, err := s.secureValueMetadataStorage.ReadForDecrypt(ctx, xkube.Namespace(newSecureValue.Namespace), newSecureValue.Name) + currentVersion, err := s.secureValueMetadataStorage.Read(ctx, xkube.Namespace(newSecureValue.Namespace), newSecureValue.Name, contracts.ReadOpts{}) if err != nil { return nil, false, fmt.Errorf("reading secure value secret: %+w", err) } @@ -77,7 +77,7 @@ func (s *SecureValueService) Update(ctx context.Context, newSecureValue *secretv } logging.FromContext(ctx).Debug("retrieved keeper", "namespace", newSecureValue.Namespace, "keeperName", newSecureValue.Spec.Keeper, "type", keeperCfg.Type()) - secret, err := keeper.Expose(ctx, keeperCfg, newSecureValue.Namespace, contracts.ExternalID(decrypted.ExternalID)) + secret, err := keeper.Expose(ctx, keeperCfg, newSecureValue.Namespace, newSecureValue.Name, currentVersion.Status.Version) if err != nil { return nil, false, fmt.Errorf("reading secret value from keeper: %w", err) } @@ -100,31 +100,31 @@ func (s *SecureValueService) createNewVersion(ctx context.Context, sv *secretv1b } // TODO: does this need to be for update? - keeperCfg, err := s.keeperMetadataStorage.GetKeeperConfig(ctx, sv.Namespace, sv.Spec.Keeper, contracts.ReadOpts{ForUpdate: true}) + keeperCfg, err := s.keeperMetadataStorage.GetKeeperConfig(ctx, createdSv.Namespace, createdSv.Spec.Keeper, contracts.ReadOpts{ForUpdate: true}) if err != nil { - return nil, fmt.Errorf("fetching keeper config: namespace=%+v keeperName=%+v %w", sv.Namespace, sv.Spec.Keeper, err) + return nil, fmt.Errorf("fetching keeper config: namespace=%+v keeperName=%+v %w", createdSv.Namespace, createdSv.Spec.Keeper, err) } keeper, err := s.keeperService.KeeperForConfig(keeperCfg) if err != nil { - return nil, fmt.Errorf("getting keeper for config: namespace=%+v keeperName=%+v %w", sv.Namespace, sv.Spec.Keeper, err) + return nil, fmt.Errorf("getting keeper for config: namespace=%+v keeperName=%+v %w", createdSv.Namespace, createdSv.Spec.Keeper, err) } - logging.FromContext(ctx).Debug("retrieved keeper", "namespace", sv.Namespace, "keeperName", sv.Spec.Keeper, "type", keeperCfg.Type()) + logging.FromContext(ctx).Debug("retrieved keeper", "namespace", createdSv.Namespace, "keeperName", createdSv.Spec.Keeper, "type", keeperCfg.Type()) // TODO: can we stop using external id? // TODO: store uses only the namespace and returns and id. It could be a kv instead. // TODO: check that the encrypted store works with multiple versions - externalID, err := keeper.Store(ctx, keeperCfg, sv.Namespace, sv.Spec.Value.DangerouslyExposeAndConsumeValue()) + externalID, err := keeper.Store(ctx, keeperCfg, createdSv.Namespace, createdSv.Name, createdSv.Status.Version, sv.Spec.Value.DangerouslyExposeAndConsumeValue()) if err != nil { return nil, fmt.Errorf("storing secure value in keeper: %w", err) } createdSv.Status.ExternalID = string(externalID) - if err := s.secureValueMetadataStorage.SetExternalID(ctx, xkube.Namespace(sv.Namespace), sv.Name, createdSv.Status.Version, externalID); err != nil { + if err := s.secureValueMetadataStorage.SetExternalID(ctx, xkube.Namespace(createdSv.Namespace), createdSv.Name, createdSv.Status.Version, externalID); err != nil { return nil, fmt.Errorf("setting secure value external id: %w", err) } - if err := s.secureValueMetadataStorage.SetVersionToActive(ctx, xkube.Namespace(sv.Namespace), sv.Name, createdSv.Status.Version); err != nil { + if err := s.secureValueMetadataStorage.SetVersionToActive(ctx, xkube.Namespace(createdSv.Namespace), createdSv.Name, createdSv.Status.Version); err != nil { return nil, fmt.Errorf("marking secure value version as active: %w", err) } diff --git a/pkg/registry/apis/secret/testutils/testutils.go b/pkg/registry/apis/secret/testutils/testutils.go index 0485b44bfba..3c189390c7a 100644 --- a/pkg/registry/apis/secret/testutils/testutils.go +++ b/pkg/registry/apis/secret/testutils/testutils.go @@ -101,10 +101,10 @@ func Setup(t *testing.T, opts ...func(*SetupConfig)) Sut { require.NoError(t, err) // Initialize encrypted value storage with a fake db - encValueStore, err := encryptionstorage.ProvideEncryptedValueStorage(database, tracer, features) + encryptedValueStorage, err := encryptionstorage.ProvideEncryptedValueStorage(database, tracer, features) require.NoError(t, err) - sqlKeeper := sqlkeeper.NewSQLKeeper(tracer, encryptionManager, encValueStore, nil) + sqlKeeper := sqlkeeper.NewSQLKeeper(tracer, encryptionManager, encryptedValueStorage, nil) var keeperService contracts.KeeperService = newKeeperServiceWrapper(sqlKeeper) @@ -124,9 +124,11 @@ func Setup(t *testing.T, opts ...func(*SetupConfig)) Sut { return Sut{ SecureValueService: secureValueService, SecureValueMetadataStorage: secureValueMetadataStorage, - Database: database, DecryptStorage: decryptStorage, DecryptService: decryptService, + EncryptedValueStorage: encryptedValueStorage, + SQLKeeper: sqlKeeper, + Database: database, } } @@ -135,6 +137,8 @@ type Sut struct { SecureValueMetadataStorage contracts.SecureValueMetadataStorage DecryptStorage contracts.DecryptStorage DecryptService service.DecryptService + EncryptedValueStorage contracts.EncryptedValueStorage + SQLKeeper *sqlkeeper.SQLKeeper Database *database.Database } diff --git a/pkg/storage/secret/encryption/data/encrypted_value_create.sql b/pkg/storage/secret/encryption/data/encrypted_value_create.sql index a3c4378959e..7bce6a19bd4 100644 --- a/pkg/storage/secret/encryption/data/encrypted_value_create.sql +++ b/pkg/storage/secret/encryption/data/encrypted_value_create.sql @@ -1,12 +1,14 @@ INSERT INTO {{ .Ident "secret_encrypted_value" }} ( - {{ .Ident "uid" }}, {{ .Ident "namespace" }}, + {{ .Ident "name" }}, + {{ .Ident "version" }}, {{ .Ident "encrypted_data" }}, {{ .Ident "created" }}, {{ .Ident "updated" }} ) VALUES ( - {{ .Arg .Row.UID }}, {{ .Arg .Row.Namespace }}, + {{ .Arg .Row.Name }}, + {{ .Arg .Row.Version }}, {{ .Arg .Row.EncryptedData }}, {{ .Arg .Row.Created }}, {{ .Arg .Row.Updated }} diff --git a/pkg/storage/secret/encryption/data/encrypted_value_delete.sql b/pkg/storage/secret/encryption/data/encrypted_value_delete.sql index 8c505c23af1..e1c78f97d39 100644 --- a/pkg/storage/secret/encryption/data/encrypted_value_delete.sql +++ b/pkg/storage/secret/encryption/data/encrypted_value_delete.sql @@ -1,4 +1,6 @@ DELETE FROM {{ .Ident "secret_encrypted_value" }} -WHERE {{ .Ident "namespace" }} = {{ .Arg .Namespace }} AND - {{ .Ident "uid" }} = {{ .Arg .UID }} +WHERE + {{ .Ident "namespace" }} = {{ .Arg .Namespace }} AND + {{ .Ident "name" }} = {{ .Arg .Name }} AND + {{ .Ident "version" }} = {{ .Arg .Version }} ; diff --git a/pkg/storage/secret/encryption/data/encrypted_value_read.sql b/pkg/storage/secret/encryption/data/encrypted_value_read.sql index f1e5e771b40..6a672554efb 100644 --- a/pkg/storage/secret/encryption/data/encrypted_value_read.sql +++ b/pkg/storage/secret/encryption/data/encrypted_value_read.sql @@ -1,11 +1,14 @@ SELECT - {{ .Ident "uid" }}, {{ .Ident "namespace" }}, + {{ .Ident "name" }}, + {{ .Ident "version" }}, {{ .Ident "encrypted_data" }}, {{ .Ident "created" }}, {{ .Ident "updated" }} FROM {{ .Ident "secret_encrypted_value" }} -WHERE {{ .Ident "namespace" }} = {{ .Arg .Namespace }} AND - {{ .Ident "uid" }} = {{ .Arg .UID }} +WHERE + {{ .Ident "namespace" }} = {{ .Arg .Namespace }} AND + {{ .Ident "name" }} = {{ .Arg .Name }} AND + {{ .Ident "version" }} = {{ .Arg .Version }} ; diff --git a/pkg/storage/secret/encryption/data/encrypted_value_update.sql b/pkg/storage/secret/encryption/data/encrypted_value_update.sql index a068d53455e..08e985297a1 100644 --- a/pkg/storage/secret/encryption/data/encrypted_value_update.sql +++ b/pkg/storage/secret/encryption/data/encrypted_value_update.sql @@ -3,6 +3,8 @@ UPDATE SET {{ .Ident "encrypted_data" }} = {{ .Arg .EncryptedData }}, {{ .Ident "updated" }} = {{ .Arg .Updated }} -WHERE {{ .Ident "namespace" }} = {{ .Arg .Namespace }} AND - {{ .Ident "uid" }} = {{ .Arg .UID }} +WHERE + {{ .Ident "namespace" }} = {{ .Arg .Namespace }} AND + {{ .Ident "name" }} = {{ .Arg .Name }} AND + {{ .Ident "version" }} = {{ .Arg .Version }} ; diff --git a/pkg/storage/secret/encryption/encrypted_value_model.go b/pkg/storage/secret/encryption/encrypted_value_model.go index 42e3fb324b6..eb66247c021 100644 --- a/pkg/storage/secret/encryption/encrypted_value_model.go +++ b/pkg/storage/secret/encryption/encrypted_value_model.go @@ -3,8 +3,9 @@ package encryption import "github.com/grafana/grafana/pkg/storage/secret/migrator" type EncryptedValue struct { - UID string Namespace string + Name string + Version int64 EncryptedData []byte Created int64 Updated int64 diff --git a/pkg/storage/secret/encryption/encrypted_value_store.go b/pkg/storage/secret/encryption/encrypted_value_store.go index a369a7a74f5..f681a93a887 100644 --- a/pkg/storage/secret/encryption/encrypted_value_store.go +++ b/pkg/storage/secret/encryption/encrypted_value_store.go @@ -6,17 +6,19 @@ import ( "fmt" "time" - "github.com/google/uuid" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/storage/unified/sql" "github.com/grafana/grafana/pkg/storage/unified/sql/sqltemplate" ) var ( - ErrEncryptedValueNotFound = errors.New("encrypted value not found") + ErrEncryptedValueNotFound = errors.New("encrypted value not found") + ErrEncryptedValueAlreadyExists = errors.New("encrypted value alredy exists") + ErrUnexpectedNumberOfRowsAffected = errors.New("unexpected number of rows modified by query") ) func ProvideEncryptedValueStorage( @@ -42,7 +44,7 @@ type encryptedValStorage struct { tracer trace.Tracer } -func (s *encryptedValStorage) Create(ctx context.Context, namespace string, encryptedData []byte) (ev *contracts.EncryptedValue, err error) { +func (s *encryptedValStorage) Create(ctx context.Context, namespace, name string, version int64, encryptedData []byte) (ev *contracts.EncryptedValue, err error) { ctx, span := s.tracer.Start(ctx, "EncryptedValueStorage.Create", trace.WithAttributes( attribute.String("namespace", namespace), )) @@ -50,14 +52,20 @@ func (s *encryptedValStorage) Create(ctx context.Context, namespace string, encr defer func() { if ev != nil { - span.SetAttributes(attribute.String("uid", ev.UID)) + span.SetAttributes( + attribute.String("namespace", ev.Namespace), + attribute.String("name", ev.Name), + attribute.Int64("version", ev.Version), + ) } }() createdTime := time.Now().Unix() + encryptedValue := &EncryptedValue{ - UID: uuid.New().String(), Namespace: namespace, + Name: name, + Version: version, EncryptedData: encryptedData, Created: createdTime, Updated: createdTime, @@ -74,6 +82,9 @@ func (s *encryptedValStorage) Create(ctx context.Context, namespace string, encr res, err := s.db.ExecContext(ctx, query, req.GetArgs()...) if err != nil { + if sql.IsRowAlreadyExistsError(err) { + return nil, ErrEncryptedValueAlreadyExists + } return nil, fmt.Errorf("inserting row: %w", err) } @@ -84,25 +95,28 @@ func (s *encryptedValStorage) Create(ctx context.Context, namespace string, encr } return &contracts.EncryptedValue{ - UID: encryptedValue.UID, Namespace: encryptedValue.Namespace, + Name: encryptedValue.Name, + Version: encryptedValue.Version, EncryptedData: encryptedValue.EncryptedData, Created: encryptedValue.Created, Updated: encryptedValue.Updated, }, nil } -func (s *encryptedValStorage) Update(ctx context.Context, namespace string, uid string, encryptedData []byte) error { +func (s *encryptedValStorage) Update(ctx context.Context, namespace, name string, version int64, encryptedData []byte) error { ctx, span := s.tracer.Start(ctx, "EncryptedValueStorage.Update", trace.WithAttributes( - attribute.String("uid", uid), attribute.String("namespace", namespace), + attribute.String("name", name), + attribute.Int64("version", version), )) defer span.End() req := updateEncryptedValue{ SQLTemplate: sqltemplate.New(s.dialect), Namespace: namespace, - UID: uid, + Name: name, + Version: version, EncryptedData: encryptedData, Updated: time.Now().Unix(), } @@ -120,23 +134,25 @@ func (s *encryptedValStorage) Update(ctx context.Context, namespace string, uid if rowsAffected, err := res.RowsAffected(); err != nil { return fmt.Errorf("getting rows affected: %w", err) } else if rowsAffected != 1 { - return fmt.Errorf("expected 1 row affected, got %d on %s", rowsAffected, namespace) + return fmt.Errorf("expected 1 row affected, got %d on %s: %w", rowsAffected, namespace, ErrUnexpectedNumberOfRowsAffected) } return nil } -func (s *encryptedValStorage) Get(ctx context.Context, namespace string, uid string) (*contracts.EncryptedValue, error) { +func (s *encryptedValStorage) Get(ctx context.Context, namespace, name string, version int64) (*contracts.EncryptedValue, error) { ctx, span := s.tracer.Start(ctx, "EncryptedValueStorage.Get", trace.WithAttributes( - attribute.String("uid", uid), attribute.String("namespace", namespace), + attribute.String("name", name), + attribute.Int64("version", version), )) defer span.End() req := &readEncryptedValue{ SQLTemplate: sqltemplate.New(s.dialect), Namespace: namespace, - UID: uid, + Name: name, + Version: version, } query, err := sqltemplate.Execute(sqlEncryptedValueRead, req) if err != nil { @@ -154,7 +170,7 @@ func (s *encryptedValStorage) Get(ctx context.Context, namespace string, uid str } var encryptedValue EncryptedValue - err = rows.Scan(&encryptedValue.UID, &encryptedValue.Namespace, &encryptedValue.EncryptedData, &encryptedValue.Created, &encryptedValue.Updated) + err = rows.Scan(&encryptedValue.Namespace, &encryptedValue.Name, &encryptedValue.Version, &encryptedValue.EncryptedData, &encryptedValue.Created, &encryptedValue.Updated) if err != nil { return nil, fmt.Errorf("failed to scan encrypted value row: %w", err) } @@ -163,25 +179,28 @@ func (s *encryptedValStorage) Get(ctx context.Context, namespace string, uid str } return &contracts.EncryptedValue{ - UID: encryptedValue.UID, Namespace: encryptedValue.Namespace, + Name: encryptedValue.Name, + Version: encryptedValue.Version, EncryptedData: encryptedValue.EncryptedData, Created: encryptedValue.Created, Updated: encryptedValue.Updated, }, nil } -func (s *encryptedValStorage) Delete(ctx context.Context, namespace string, uid string) error { +func (s *encryptedValStorage) Delete(ctx context.Context, namespace, name string, version int64) error { ctx, span := s.tracer.Start(ctx, "EncryptedValueStorage.Delete", trace.WithAttributes( - attribute.String("uid", uid), attribute.String("namespace", namespace), + attribute.String("name", name), + attribute.Int64("version", version), )) defer span.End() req := deleteEncryptedValue{ SQLTemplate: sqltemplate.New(s.dialect), Namespace: namespace, - UID: uid, + Name: name, + Version: version, } query, err := sqltemplate.Execute(sqlEncryptedValueDelete, req) if err != nil { diff --git a/pkg/storage/secret/encryption/encrypted_value_store_test.go b/pkg/storage/secret/encryption/encrypted_value_store_test.go index 92afe58e7e2..18b67a651fb 100644 --- a/pkg/storage/secret/encryption/encrypted_value_store_test.go +++ b/pkg/storage/secret/encryption/encrypted_value_store_test.go @@ -1,46 +1,47 @@ -package encryption +package encryption_test import ( - "context" + "errors" + "slices" "testing" - "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/grafana/grafana/pkg/storage/secret/database" - "github.com/grafana/grafana/pkg/storage/secret/migrator" + "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" + "github.com/grafana/grafana/pkg/registry/apis/secret/testutils" + "github.com/grafana/grafana/pkg/storage/secret/encryption" "github.com/stretchr/testify/require" - "go.opentelemetry.io/otel/trace/noop" + "pgregory.net/rapid" ) func TestEncryptedValueStoreImpl(t *testing.T) { - // Initialize data key storage with a fake db - testDB := sqlstore.NewTestStore(t, sqlstore.WithMigrator(migrator.New())) - tracer := noop.NewTracerProvider().Tracer("test") - database := database.ProvideDatabase(testDB, tracer) - features := featuremgmt.WithFeatures(featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, featuremgmt.FlagSecretsManagementAppPlatform) - ctx := context.Background() - - store, err := ProvideEncryptedValueStorage(database, tracer, features) - require.NoError(t, err) + t.Parallel() t.Run("creating an encrypted value returns it", func(t *testing.T) { - createdEV, err := store.Create(ctx, "test-namespace", []byte("test-data")) + t.Parallel() + + sut := testutils.Setup(t) + createdEV, err := sut.EncryptedValueStorage.Create(t.Context(), "test-namespace", "test-name", 1, []byte("test-data")) require.NoError(t, err) - require.NotEmpty(t, createdEV.UID) + require.NotEmpty(t, createdEV.Namespace) + require.NotEmpty(t, createdEV.Name) require.NotEmpty(t, createdEV.Created) require.NotEmpty(t, createdEV.Updated) require.NotEmpty(t, createdEV.EncryptedData) require.Equal(t, "test-namespace", createdEV.Namespace) + require.Equal(t, "test-name", createdEV.Name) }) t.Run("get an existent encrypted value returns it", func(t *testing.T) { - createdEV, err := store.Create(ctx, "test-namespace", []byte("test-data")) + t.Parallel() + + sut := testutils.Setup(t) + createdEV, err := sut.EncryptedValueStorage.Create(t.Context(), "test-namespace", "test-name", 1, []byte("test-data")) require.NoError(t, err) - obtainedEV, err := store.Get(ctx, "test-namespace", createdEV.UID) + obtainedEV, err := sut.EncryptedValueStorage.Get(t.Context(), createdEV.Namespace, createdEV.Name, createdEV.Version) require.NoError(t, err) - require.Equal(t, createdEV.UID, obtainedEV.UID) + require.Equal(t, createdEV.Namespace, obtainedEV.Namespace) + require.Equal(t, createdEV.Name, obtainedEV.Name) require.Equal(t, createdEV.Created, obtainedEV.Created) require.Equal(t, createdEV.Updated, obtainedEV.Updated) require.Equal(t, createdEV.EncryptedData, obtainedEV.EncryptedData) @@ -48,10 +49,13 @@ func TestEncryptedValueStoreImpl(t *testing.T) { }) t.Run("get an existent encrypted value with a different namespace returns error", func(t *testing.T) { - createdEV, err := store.Create(ctx, "test-namespace", []byte("test-data")) + t.Parallel() + + sut := testutils.Setup(t) + createdEV, err := sut.EncryptedValueStorage.Create(t.Context(), "ns1", "test-name", 1, []byte("test-data")) require.NoError(t, err) - obtainedEV, err := store.Get(ctx, "other-test-namespace", createdEV.UID) + obtainedEV, err := sut.EncryptedValueStorage.Get(t.Context(), "ns2", createdEV.Name, createdEV.Version) require.Error(t, err) require.Equal(t, "encrypted value not found", err.Error()) @@ -59,20 +63,26 @@ func TestEncryptedValueStoreImpl(t *testing.T) { }) t.Run("get a non existent encrypted value returns error", func(t *testing.T) { - obtainedEV, err := store.Get(ctx, "test-namespace", "test-uid") + t.Parallel() + + sut := testutils.Setup(t) + obtainedEV, err := sut.EncryptedValueStorage.Get(t.Context(), "test-namespace", "test-name", 1) require.Error(t, err) require.Equal(t, "encrypted value not found", err.Error()) require.Nil(t, obtainedEV) }) t.Run("updating an existing encrypted value returns no error", func(t *testing.T) { - createdEV, err := store.Create(ctx, "test-namespace", []byte("test-data")) + t.Parallel() + + sut := testutils.Setup(t) + createdEV, err := sut.EncryptedValueStorage.Create(t.Context(), "test-namespace", "test-name", 1, []byte("test-data")) require.NoError(t, err) - err = store.Update(ctx, "test-namespace", createdEV.UID, []byte("test-data-updated")) + err = sut.EncryptedValueStorage.Update(t.Context(), createdEV.Namespace, createdEV.Name, createdEV.Version, []byte("test-data-updated")) require.NoError(t, err) - updatedEV, err := store.Get(ctx, "test-namespace", createdEV.UID) + updatedEV, err := sut.EncryptedValueStorage.Get(t.Context(), createdEV.Namespace, createdEV.Name, createdEV.Version) require.NoError(t, err) require.Equal(t, []byte("test-data-updated"), updatedEV.EncryptedData) @@ -81,27 +91,192 @@ func TestEncryptedValueStoreImpl(t *testing.T) { }) t.Run("updating a non existing encrypted value returns error", func(t *testing.T) { - err := store.Update(ctx, "test-namespace", "test-uid", []byte("test-data")) + t.Parallel() + + sut := testutils.Setup(t) + err := sut.EncryptedValueStorage.Update(t.Context(), "test-namespace", "test-uid", 1, []byte("test-data")) require.Error(t, err) }) t.Run("delete an existing encrypted value returns error", func(t *testing.T) { - createdEV, err := store.Create(ctx, "test-namespace", []byte("ttttest-data")) + t.Parallel() + + sut := testutils.Setup(t) + createdEV, err := sut.EncryptedValueStorage.Create(t.Context(), "test-namespace", "test-name", 1, []byte("ttttest-data")) require.NoError(t, err) - obtainedEV, err := store.Get(ctx, "test-namespace", createdEV.UID) + _, err = sut.EncryptedValueStorage.Get(t.Context(), createdEV.Namespace, createdEV.Name, createdEV.Version) require.NoError(t, err) - err = store.Delete(ctx, "test-namespace", obtainedEV.UID) + err = sut.EncryptedValueStorage.Delete(t.Context(), createdEV.Namespace, createdEV.Name, createdEV.Version) require.NoError(t, err) - obtainedEV, err = store.Get(ctx, "test-namespace", createdEV.UID) + obtainedEV, err := sut.EncryptedValueStorage.Get(t.Context(), createdEV.Namespace, createdEV.Name, createdEV.Version) require.Error(t, err) require.Nil(t, obtainedEV) }) t.Run("delete a non existing encrypted value does not return error", func(t *testing.T) { - err := store.Delete(ctx, "test-namespace", "test-uid") + t.Parallel() + + sut := testutils.Setup(t) + err := sut.EncryptedValueStorage.Delete(t.Context(), "test-namespace", "test-name", 1) require.NoError(t, err) }) } + +func TestStateMachine(t *testing.T) { + t.Parallel() + + tt := t + + rapid.Check(t, func(t *rapid.T) { + sut := testutils.Setup(tt) + m := newModel() + + t.Repeat(map[string]func(*rapid.T){ + "create": func(t *rapid.T) { + ns := namespaceGen.Draw(t, "ns") + name := nameGen.Draw(t, "name") + version := versionGen.Draw(t, "version") + plaintext := rapid.String().Draw(t, "plaintext") + + _, modelErr := m.create(ns, name, version, []byte(plaintext)) + _, err := sut.EncryptedValueStorage.Create(t.Context(), ns, name, version, []byte(plaintext)) + if modelErr != nil || err != nil { + require.ErrorIs(t, err, modelErr) + return + } + }, + "update": func(t *rapid.T) { + ns := namespaceGen.Draw(t, "ns") + name := nameGen.Draw(t, "name") + version := versionGen.Draw(t, "version") + plaintext := rapid.String().Draw(t, "plaintext") + + modelErr := m.update(ns, name, version, []byte(plaintext)) + err := sut.EncryptedValueStorage.Update(t.Context(), ns, name, version, []byte(plaintext)) + if modelErr != nil || err != nil { + require.ErrorIs(t, err, modelErr) + return + } + }, + "get": func(t *rapid.T) { + ns := namespaceGen.Draw(t, "ns") + name := nameGen.Draw(t, "name") + version := versionGen.Draw(t, "version") + + modelValue, modelErr := m.get(ns, name, version) + value, err := sut.EncryptedValueStorage.Get(t.Context(), ns, name, version) + if modelErr != nil || err != nil { + require.ErrorIs(t, err, modelErr) + return + } + // Do not compare timestamps because the model doesn't model them. + require.Equal(t, modelValue.Namespace, value.Namespace) + require.Equal(t, modelValue.Name, value.Name) + require.Equal(t, modelValue.EncryptedData, value.EncryptedData) + require.Equal(t, modelValue.Version, value.Version) + }, + "delete": func(t *rapid.T) { + ns := namespaceGen.Draw(t, "ns") + name := nameGen.Draw(t, "name") + version := versionGen.Draw(t, "version") + + modelErr := m.delete(ns, name, version) + err := sut.EncryptedValueStorage.Delete(t.Context(), ns, name, version) + if modelErr != nil || err != nil { + require.ErrorIs(t, err, modelErr) + return + } + }, + }) + }) +} + +var ( + namespaceGen = rapid.Custom(func(t *rapid.T) string { + return rapid.SampledFrom([]string{"ns1", "ns2", "ns3", "ns4", "ns5"}).Draw(t, "namespace") + }) + nameGen = rapid.Custom(func(t *rapid.T) string { + return rapid.SampledFrom([]string{"name1", "name2", "name3", "name4", "name5"}).Draw(t, "name") + }) + versionGen = rapid.Custom(func(t *rapid.T) int64 { + return rapid.Int64Range(1, 5).Draw(t, "version") + }) +) + +// A simplified model of the encrypted value storage +type model struct { + entries []*entry +} + +type entry struct { + namespace string + name string + version int64 + encryptedData []byte +} + +func newModel() *model { + return &model{} +} + +func (m *model) create(namespace, name string, version int64, encryptedData []byte) (*contracts.EncryptedValue, error) { + v, err := m.get(namespace, name, version) + if err != nil && !errors.Is(err, encryption.ErrEncryptedValueNotFound) { + return nil, err + } + // The entry being creted already exists + if v != nil { + return nil, encryption.ErrEncryptedValueAlreadyExists + } + + m.entries = append(m.entries, &entry{ + namespace: namespace, + name: name, + version: version, + encryptedData: encryptedData, + }) + return &contracts.EncryptedValue{ + Namespace: namespace, + Name: name, + Version: version, + EncryptedData: encryptedData, + Created: 1, + Updated: 1, + }, nil +} +func (m *model) update(namespace, name string, version int64, encryptedData []byte) error { + for _, v := range m.entries { + if v.namespace == namespace && v.name == name && v.version == version { + v.encryptedData = encryptedData + return nil + } + } + + return encryption.ErrUnexpectedNumberOfRowsAffected +} + +func (m *model) get(namespace, name string, version int64) (*contracts.EncryptedValue, error) { + for _, v := range m.entries { + if v.namespace == namespace && v.name == name && v.version == version { + return &contracts.EncryptedValue{ + Namespace: namespace, + Name: name, + Version: version, + EncryptedData: v.encryptedData, + Created: 1, + Updated: 1, + }, nil + } + } + + return nil, encryption.ErrEncryptedValueNotFound +} +func (m *model) delete(namespace, name string, version int64) error { + m.entries = slices.DeleteFunc(m.entries, func(v *entry) bool { + return v.namespace == namespace && v.name == name && v.version == version + }) + return nil +} diff --git a/pkg/storage/secret/encryption/query.go b/pkg/storage/secret/encryption/query.go index 1e6a8b13609..25f21cb77ba 100644 --- a/pkg/storage/secret/encryption/query.go +++ b/pkg/storage/secret/encryption/query.go @@ -55,7 +55,8 @@ func (r createEncryptedValue) Validate() error { type readEncryptedValue struct { sqltemplate.SQLTemplate Namespace string - UID string + Name string + Version int64 } // Validate is only used if we use `dbutil` from `unifiedstorage` @@ -67,7 +68,8 @@ func (r readEncryptedValue) Validate() error { type updateEncryptedValue struct { sqltemplate.SQLTemplate Namespace string - UID string + Name string + Version int64 EncryptedData []byte Updated int64 } @@ -81,7 +83,8 @@ func (r updateEncryptedValue) Validate() error { type deleteEncryptedValue struct { sqltemplate.SQLTemplate Namespace string - UID string + Name string + Version int64 } // Validate is only used if we use `dbutil` from `unifiedstorage` diff --git a/pkg/storage/secret/encryption/query_test.go b/pkg/storage/secret/encryption/query_test.go index 371d5bfc887..51e078d8e25 100644 --- a/pkg/storage/secret/encryption/query_test.go +++ b/pkg/storage/secret/encryption/query_test.go @@ -20,7 +20,8 @@ func TestEncryptedValueQueries(t *testing.T) { SQLTemplate: mocks.NewTestingSQLTemplate(), Row: &EncryptedValue{ Namespace: "ns", - UID: "abc123", + Name: "n1", + Version: 1, EncryptedData: []byte("secret"), Created: 1234, Updated: 5678, @@ -34,7 +35,8 @@ func TestEncryptedValueQueries(t *testing.T) { Data: &readEncryptedValue{ SQLTemplate: mocks.NewTestingSQLTemplate(), Namespace: "ns", - UID: "abc123", + Name: "n1", + Version: 1, }, }, }, @@ -44,7 +46,8 @@ func TestEncryptedValueQueries(t *testing.T) { Data: &updateEncryptedValue{ SQLTemplate: mocks.NewTestingSQLTemplate(), Namespace: "ns", - UID: "abc123", + Name: "n1", + Version: 1, EncryptedData: []byte("secret"), Updated: 5679, }, @@ -56,7 +59,8 @@ func TestEncryptedValueQueries(t *testing.T) { Data: &deleteEncryptedValue{ SQLTemplate: mocks.NewTestingSQLTemplate(), Namespace: "ns", - UID: "abc123", + Name: "n1", + Version: 1, }, }, }, diff --git a/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_create-create.sql b/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_create-create.sql index 079496061ca..7a886b1c6ee 100755 --- a/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_create-create.sql +++ b/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_create-create.sql @@ -1,12 +1,14 @@ INSERT INTO `secret_encrypted_value` ( - `uid`, `namespace`, + `name`, + `version`, `encrypted_data`, `created`, `updated` ) VALUES ( - 'abc123', 'ns', + 'n1', + 1, '[115 101 99 114 101 116]', 1234, 5678 diff --git a/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_delete-delete.sql b/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_delete-delete.sql index fb492bcd0c9..6c1dc38ac79 100755 --- a/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_delete-delete.sql +++ b/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_delete-delete.sql @@ -1,4 +1,6 @@ DELETE FROM `secret_encrypted_value` -WHERE `namespace` = 'ns' AND - `uid` = 'abc123' +WHERE + `namespace` = 'ns' AND + `name` = 'n1' AND + `version` = 1 ; diff --git a/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_read-read.sql b/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_read-read.sql index 70ce871fe47..bb9faddb8ff 100755 --- a/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_read-read.sql +++ b/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_read-read.sql @@ -1,11 +1,14 @@ SELECT - `uid`, `namespace`, + `name`, + `version`, `encrypted_data`, `created`, `updated` FROM `secret_encrypted_value` -WHERE `namespace` = 'ns' AND - `uid` = 'abc123' +WHERE + `namespace` = 'ns' AND + `name` = 'n1' AND + `version` = 1 ; diff --git a/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_update-update.sql b/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_update-update.sql index c54acd9a2e7..cf3d6897a64 100755 --- a/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_update-update.sql +++ b/pkg/storage/secret/encryption/testdata/mysql--encrypted_value_update-update.sql @@ -3,6 +3,8 @@ UPDATE SET `encrypted_data` = '[115 101 99 114 101 116]', `updated` = 5679 -WHERE `namespace` = 'ns' AND - `uid` = 'abc123' +WHERE + `namespace` = 'ns' AND + `name` = 'n1' AND + `version` = 1 ; diff --git a/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_create-create.sql b/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_create-create.sql index 1c8a8934e88..23d9a0f2331 100755 --- a/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_create-create.sql +++ b/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_create-create.sql @@ -1,12 +1,14 @@ INSERT INTO "secret_encrypted_value" ( - "uid", "namespace", + "name", + "version", "encrypted_data", "created", "updated" ) VALUES ( - 'abc123', 'ns', + 'n1', + 1, '[115 101 99 114 101 116]', 1234, 5678 diff --git a/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_delete-delete.sql b/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_delete-delete.sql index 380e44b1d68..98a7a677f94 100755 --- a/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_delete-delete.sql +++ b/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_delete-delete.sql @@ -1,4 +1,6 @@ DELETE FROM "secret_encrypted_value" -WHERE "namespace" = 'ns' AND - "uid" = 'abc123' +WHERE + "namespace" = 'ns' AND + "name" = 'n1' AND + "version" = 1 ; diff --git a/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_read-read.sql b/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_read-read.sql index 77d325ac85a..4bbe1000ce6 100755 --- a/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_read-read.sql +++ b/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_read-read.sql @@ -1,11 +1,14 @@ SELECT - "uid", "namespace", + "name", + "version", "encrypted_data", "created", "updated" FROM "secret_encrypted_value" -WHERE "namespace" = 'ns' AND - "uid" = 'abc123' +WHERE + "namespace" = 'ns' AND + "name" = 'n1' AND + "version" = 1 ; diff --git a/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_update-update.sql b/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_update-update.sql index 8db8bb3aa6a..b2b5e30ecac 100755 --- a/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_update-update.sql +++ b/pkg/storage/secret/encryption/testdata/postgres--encrypted_value_update-update.sql @@ -3,6 +3,8 @@ UPDATE SET "encrypted_data" = '[115 101 99 114 101 116]', "updated" = 5679 -WHERE "namespace" = 'ns' AND - "uid" = 'abc123' +WHERE + "namespace" = 'ns' AND + "name" = 'n1' AND + "version" = 1 ; diff --git a/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_create-create.sql b/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_create-create.sql index 1c8a8934e88..23d9a0f2331 100755 --- a/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_create-create.sql +++ b/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_create-create.sql @@ -1,12 +1,14 @@ INSERT INTO "secret_encrypted_value" ( - "uid", "namespace", + "name", + "version", "encrypted_data", "created", "updated" ) VALUES ( - 'abc123', 'ns', + 'n1', + 1, '[115 101 99 114 101 116]', 1234, 5678 diff --git a/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_delete-delete.sql b/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_delete-delete.sql index 380e44b1d68..98a7a677f94 100755 --- a/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_delete-delete.sql +++ b/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_delete-delete.sql @@ -1,4 +1,6 @@ DELETE FROM "secret_encrypted_value" -WHERE "namespace" = 'ns' AND - "uid" = 'abc123' +WHERE + "namespace" = 'ns' AND + "name" = 'n1' AND + "version" = 1 ; diff --git a/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_read-read.sql b/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_read-read.sql index 77d325ac85a..4bbe1000ce6 100755 --- a/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_read-read.sql +++ b/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_read-read.sql @@ -1,11 +1,14 @@ SELECT - "uid", "namespace", + "name", + "version", "encrypted_data", "created", "updated" FROM "secret_encrypted_value" -WHERE "namespace" = 'ns' AND - "uid" = 'abc123' +WHERE + "namespace" = 'ns' AND + "name" = 'n1' AND + "version" = 1 ; diff --git a/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_update-update.sql b/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_update-update.sql index 8db8bb3aa6a..b2b5e30ecac 100755 --- a/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_update-update.sql +++ b/pkg/storage/secret/encryption/testdata/sqlite--encrypted_value_update-update.sql @@ -3,6 +3,8 @@ UPDATE SET "encrypted_data" = '[115 101 99 114 101 116]', "updated" = 5679 -WHERE "namespace" = 'ns' AND - "uid" = 'abc123' +WHERE + "namespace" = 'ns' AND + "name" = 'n1' AND + "version" = 1 ; diff --git a/pkg/storage/secret/metadata/decrypt_store.go b/pkg/storage/secret/metadata/decrypt_store.go index 93d212299cb..d2aea92f54b 100644 --- a/pkg/storage/secret/metadata/decrypt_store.go +++ b/pkg/storage/secret/metadata/decrypt_store.go @@ -94,17 +94,17 @@ func (s *decryptStorage) Decrypt(ctx context.Context, namespace xkube.Namespace, // The auth token will not necessarily have the permission to read the secure value metadata, // but we still need to do it to inspect the `decrypters` field, hence the actual `authorize` // function call happens after this. - sv, err := s.secureValueMetadataStorage.ReadForDecrypt(ctx, namespace, name) + sv, err := s.secureValueMetadataStorage.Read(ctx, namespace, name, contracts.ReadOpts{}) if err != nil { return "", contracts.ErrDecryptNotFound } - decrypterIdentity, authorized := s.decryptAuthorizer.Authorize(ctx, name, sv.Decrypters) + decrypterIdentity, authorized := s.decryptAuthorizer.Authorize(ctx, name, sv.Spec.Decrypters) if !authorized { return "", contracts.ErrDecryptNotAuthorized } - keeperConfig, err := s.keeperMetadataStorage.GetKeeperConfig(ctx, namespace.String(), sv.Keeper, contracts.ReadOpts{}) + keeperConfig, err := s.keeperMetadataStorage.GetKeeperConfig(ctx, namespace.String(), sv.Spec.Keeper, contracts.ReadOpts{}) if err != nil { return "", contracts.ErrDecryptFailed } @@ -114,7 +114,7 @@ func (s *decryptStorage) Decrypt(ctx context.Context, namespace xkube.Namespace, return "", contracts.ErrDecryptFailed } - exposedValue, err := keeper.Expose(ctx, keeperConfig, namespace.String(), contracts.ExternalID(sv.ExternalID)) + exposedValue, err := keeper.Expose(ctx, keeperConfig, namespace.String(), name, sv.Status.Version) if err != nil { return "", contracts.ErrDecryptFailed } diff --git a/pkg/storage/secret/metadata/secure_value_store.go b/pkg/storage/secret/metadata/secure_value_store.go index 3653c6006df..829a48481a7 100644 --- a/pkg/storage/secret/metadata/secure_value_store.go +++ b/pkg/storage/secret/metadata/secure_value_store.go @@ -197,6 +197,7 @@ func (s *secureValueMetadataStorage) getLatestVersion(ctx context.Context, names return &version, nil } +// TODO: can this method + queries be removed? func (s *secureValueMetadataStorage) ReadForDecrypt(ctx context.Context, namespace xkube.Namespace, name string) (*contracts.DecryptSecureValue, error) { start := time.Now() ctx, span := s.tracer.Start(ctx, "SecureValueMetadataStorage.ReadForDecrypt", trace.WithAttributes( diff --git a/pkg/storage/secret/migrator/migrator.go b/pkg/storage/secret/migrator/migrator.go index 6ee7209bc8a..ee2a5a1c975 100644 --- a/pkg/storage/secret/migrator/migrator.go +++ b/pkg/storage/secret/migrator/migrator.go @@ -118,17 +118,21 @@ func (*SecretDB) AddMigration(mg *migrator.Migrator) { Indices: []*migrator.Index{}, // TODO: add indexes based on the queries we make. }) - tables = append(tables, migrator.Table{ + encryptedValueTable := migrator.Table{ Name: TableNameEncryptedValue, Columns: []*migrator.Column{ {Name: "namespace", Type: migrator.DB_NVarchar, Length: 253, Nullable: false}, // Limit enforced by K8s. - {Name: "uid", Type: migrator.DB_NVarchar, Length: 36, IsPrimaryKey: true}, // Fixed size of a UUID. + {Name: "name", Type: migrator.DB_NVarchar, Length: 253, Nullable: false}, + {Name: "version", Type: migrator.DB_BigInt, Nullable: false}, {Name: "encrypted_data", Type: migrator.DB_Blob, Nullable: false}, {Name: "created", Type: migrator.DB_BigInt, Nullable: false}, {Name: "updated", Type: migrator.DB_BigInt, Nullable: false}, }, - Indices: []*migrator.Index{}, // TODO: add indexes based on the queries we make. - }) + Indices: []*migrator.Index{ + {Cols: []string{"namespace", "name", "version"}, Type: migrator.UniqueIndex}, + }, + } + tables = append(tables, encryptedValueTable) // Initialize all tables for t := range tables {