From dbe815ee684a60fcdf8e86191f9116e53aaa2a51 Mon Sep 17 00:00:00 2001 From: Dana Axinte <53751979+dana-axinte@users.noreply.github.com> Date: Mon, 16 Jun 2025 14:37:36 +0100 Subject: [PATCH] SecretsManager: keepers with secure values credentials (#106761) * SecretsManager: keepers with secure values Co-authored-by: Matheus Macabu Co-authored-by: Dana Axinte <53751979+dana-axinte@users.noreply.github.com> * Keepers: Refactor extract secure values remove extra helper functions Co-authored-by: Matheus Macabu --------- Co-authored-by: Matheus Macabu --- pkg/storage/secret/metadata/keeper_model.go | 34 ++++ pkg/storage/secret/metadata/keeper_store.go | 146 ++++++++++++++++++ pkg/storage/secret/metadata/query.go | 15 +- pkg/storage/secret/metadata/query_test.go | 10 ++ .../mysql--secure_value_listByName-list.sql | 9 ++ ...postgres--secure_value_listByName-list.sql | 9 ++ .../sqlite--secure_value_listByName-list.sql | 8 + pkg/tests/apis/secret/keeper_test.go | 2 - 8 files changed, 230 insertions(+), 3 deletions(-) create mode 100755 pkg/storage/secret/metadata/testdata/mysql--secure_value_listByName-list.sql create mode 100755 pkg/storage/secret/metadata/testdata/postgres--secure_value_listByName-list.sql create mode 100755 pkg/storage/secret/metadata/testdata/sqlite--secure_value_listByName-list.sql diff --git a/pkg/storage/secret/metadata/keeper_model.go b/pkg/storage/secret/metadata/keeper_model.go index 53843e7bf4a..ae8ef7bb908 100644 --- a/pkg/storage/secret/metadata/keeper_model.go +++ b/pkg/storage/secret/metadata/keeper_model.go @@ -246,3 +246,37 @@ func toProvider(keeperType secretv0alpha1.KeeperType, payload string) secretv0al return nil } } + +// extractSecureValues extracts unique securevalues referenced by the keeper, if any. +func extractSecureValues(kp *secretv0alpha1.Keeper) map[string]struct{} { + switch { + case kp.Spec.AWS != nil: + secureValues := make(map[string]struct{}, 0) + + if kp.Spec.AWS.AccessKeyID.SecureValueName != "" { + secureValues[kp.Spec.AWS.AccessKeyID.SecureValueName] = struct{}{} + } + + if kp.Spec.AWS.SecretAccessKey.SecureValueName != "" { + secureValues[kp.Spec.AWS.SecretAccessKey.SecureValueName] = struct{}{} + } + + return secureValues + + case kp.Spec.Azure != nil: + if kp.Spec.Azure.ClientSecret.SecureValueName != "" { + return map[string]struct{}{kp.Spec.Azure.ClientSecret.SecureValueName: {}} + } + + // GCP does not reference secureValues. + case kp.Spec.GCP != nil: + return nil + + case kp.Spec.HashiCorp != nil: + if kp.Spec.HashiCorp.Token.SecureValueName != "" { + return map[string]struct{}{kp.Spec.HashiCorp.Token.SecureValueName: {}} + } + } + + return nil +} diff --git a/pkg/storage/secret/metadata/keeper_store.go b/pkg/storage/secret/metadata/keeper_store.go index 4fa1572a594..78458201e89 100644 --- a/pkg/storage/secret/metadata/keeper_store.go +++ b/pkg/storage/secret/metadata/keeper_store.go @@ -48,6 +48,11 @@ func (s *keeperMetadataStorage) Create(ctx context.Context, keeper *secretv0alph } err = s.db.Transaction(ctx, func(ctx context.Context) error { + // Validate before inserting that any `secureValues` referenced exist and do not reference other third-party keepers. + if err := s.validateSecureValueReferences(ctx, keeper); err != nil { + return err + } + result, err := s.db.ExecContext(ctx, query, req.GetArgs()...) if err != nil { return fmt.Errorf("inserting row: %w", err) @@ -132,6 +137,11 @@ func (s *keeperMetadataStorage) Update(ctx context.Context, newKeeper *secretv0a var newRow *keeperDB err := s.db.Transaction(ctx, func(ctx context.Context) error { + // Validate before updating that any `secureValues` referenced exists and does not reference other third-party keepers. + if err := s.validateSecureValueReferences(ctx, newKeeper); err != nil { + return err + } + // Read old value first. oldKeeperRow, err := s.read(ctx, newKeeper.Namespace, newKeeper.Name, contracts.ReadOpts{ForUpdate: true}) if err != nil { @@ -258,6 +268,142 @@ func (s *keeperMetadataStorage) List(ctx context.Context, namespace xkube.Namesp return keepers, nil } +// validateSecureValueReferences checks that all secure values referenced by the keeper exist and are not referenced by other third-party keepers. +// It is used by other methods inside a transaction. +func (s *keeperMetadataStorage) validateSecureValueReferences(ctx context.Context, keeper *secretv0alpha1.Keeper) error { + usedSecureValues := extractSecureValues(keeper) + + // No secure values are referenced, return early. + if len(usedSecureValues) == 0 { + return nil + } + + // SQL templates do not support maps. + usedSecureValuesList := make([]string, 0, len(usedSecureValues)) + for sv := range usedSecureValues { + usedSecureValuesList = append(usedSecureValuesList, sv) + } + + reqSecureValue := listByNameSecureValue{ + SQLTemplate: sqltemplate.New(s.dialect), + Namespace: keeper.Namespace, + UsedSecureValues: usedSecureValuesList, + } + + querySecureValueList, err := sqltemplate.Execute(sqlSecureValueListByName, reqSecureValue) + if err != nil { + return fmt.Errorf("execute template %q: %w", sqlSecureValueListByName.Name(), err) + } + + rows, err := s.db.QueryContext(ctx, querySecureValueList, reqSecureValue.GetArgs()...) + if err != nil { + return fmt.Errorf("executing query: %w", err) + } + defer func() { _ = rows.Close() }() + + // DTO for `sqlSecureValueListByName` query result, only what we need. + type listByNameResult struct { + Name string + Keeper *string + } + + secureValueRows := make([]listByNameResult, 0) + for rows.Next() { + var row listByNameResult + if err := rows.Scan(&row.Name, &row.Keeper); err != nil { + return fmt.Errorf("error reading secret value row: %w", err) + } + + secureValueRows = append(secureValueRows, row) + } + + if err := rows.Err(); err != nil { + return fmt.Errorf("secret value rows error: %w", err) + } + + // If not all secure values being referenced exist, return an error with the missing ones. + if len(secureValueRows) != len(usedSecureValues) { + // We are guaranteed that the returned `secureValueRows` are a subset of `usedSecureValues`, + // so we don't need to check the other way around. + missing := make(map[string]struct{}, len(usedSecureValues)) + for sv := range usedSecureValues { + missing[sv] = struct{}{} + } + + for _, svRow := range secureValueRows { + delete(missing, svRow.Name) + } + + return contracts.NewErrKeeperInvalidSecureValues(missing) + } + + // If all secure values exist, we need to guarantee that the third-party keeper is not referencing another third-party, + // it must reference only the system keeper (when keeper=null) to keep the dependency tree flat (n=1). + keeperNames := make([]string, 0, len(secureValueRows)) + keeperSecureValues := make(map[string][]string, 0) + + for _, svRow := range secureValueRows { + // Using the system keeper (null). + if svRow.Keeper == nil { + continue + } + + keeperNames = append(keeperNames, *svRow.Keeper) + keeperSecureValues[*svRow.Keeper] = append(keeperSecureValues[*svRow.Keeper], svRow.Name) + } + + // We didn't find any secure values that reference third-party keepers. + if len(keeperNames) == 0 { + return nil + } + + reqKeeper := listByNameKeeper{ + SQLTemplate: sqltemplate.New(s.dialect), + Namespace: keeper.Namespace, + KeeperNames: keeperNames, + } + + qKeeper, err := sqltemplate.Execute(sqlKeeperListByName, reqKeeper) + if err != nil { + return fmt.Errorf("template %q: %w", sqlKeeperListByName.Name(), err) + } + + keepersRows, err := s.db.QueryContext(ctx, qKeeper, reqKeeper.GetArgs()...) + if err != nil { + return fmt.Errorf("listing by name %q: %w", qKeeper, err) + } + defer func() { _ = keepersRows.Close() }() + + thirdPartyKeepers := make([]string, 0) + for keepersRows.Next() { + var name string + if err := keepersRows.Scan(&name); err != nil { + return fmt.Errorf("error reading keeper row: %w", err) + } + + thirdPartyKeepers = append(thirdPartyKeepers, name) + } + + if err := rows.Err(); err != nil { + return fmt.Errorf("third party keeper rows error: %w", err) + } + + // Found secureValueNames that are referenced by third-party keepers. + if len(thirdPartyKeepers) > 0 { + invalidSecureValues := make(map[string]string, 0) + + for _, keeperName := range thirdPartyKeepers { + for _, svName := range keeperSecureValues[keeperName] { + invalidSecureValues[svName] = keeperName + } + } + + return contracts.NewErrKeeperInvalidSecureValuesReference(invalidSecureValues) + } + + return nil +} + func (s *keeperMetadataStorage) GetKeeperConfig(ctx context.Context, namespace string, name *string, opts contracts.ReadOpts) (secretv0alpha1.KeeperConfig, error) { // Check if keeper is the systemwide one. if name == nil { diff --git a/pkg/storage/secret/metadata/query.go b/pkg/storage/secret/metadata/query.go index 1b0b35fcbb1..a47c0ffdae6 100644 --- a/pkg/storage/secret/metadata/query.go +++ b/pkg/storage/secret/metadata/query.go @@ -21,7 +21,8 @@ var ( sqlKeeperList = mustTemplate("keeper_list.sql") sqlKeeperDelete = mustTemplate("keeper_delete.sql") - sqlKeeperListByName = mustTemplate("keeper_listByName.sql") + sqlKeeperListByName = mustTemplate("keeper_listByName.sql") + sqlSecureValueListByName = mustTemplate("secure_value_listByName.sql") sqlSecureValueRead = mustTemplate("secure_value_read.sql") sqlSecureValueList = mustTemplate("secure_value_list.sql") @@ -119,6 +120,18 @@ func (r listByNameKeeper) Validate() error { return nil // TODO } +// This is used at keeper store to validate create & update operations +type listByNameSecureValue struct { + sqltemplate.SQLTemplate + Namespace string + UsedSecureValues []string +} + +// Validate is only used if we use `dbutil` from `unifiedstorage` +func (r listByNameSecureValue) Validate() error { + return nil // TODO +} + /******************************/ /**-- Secure Value Queries --**/ /******************************/ diff --git a/pkg/storage/secret/metadata/query_test.go b/pkg/storage/secret/metadata/query_test.go index 93cdf1fed02..e07a8844844 100644 --- a/pkg/storage/secret/metadata/query_test.go +++ b/pkg/storage/secret/metadata/query_test.go @@ -105,6 +105,16 @@ func TestKeeperQueries(t *testing.T) { }, }, }, + sqlSecureValueListByName: { + { + Name: "list", + Data: listByNameSecureValue{ + SQLTemplate: mocks.NewTestingSQLTemplate(), + Namespace: "ns", + UsedSecureValues: []string{"a", "b"}, + }, + }, + }, }, }) } diff --git a/pkg/storage/secret/metadata/testdata/mysql--secure_value_listByName-list.sql b/pkg/storage/secret/metadata/testdata/mysql--secure_value_listByName-list.sql new file mode 100755 index 00000000000..91ac79af6ae --- /dev/null +++ b/pkg/storage/secret/metadata/testdata/mysql--secure_value_listByName-list.sql @@ -0,0 +1,9 @@ +SELECT + `name`, + `keeper` +FROM + `secret_secure_value` +WHERE `namespace` = 'ns' AND + `name` IN ('a', 'b') +FOR UPDATE +; diff --git a/pkg/storage/secret/metadata/testdata/postgres--secure_value_listByName-list.sql b/pkg/storage/secret/metadata/testdata/postgres--secure_value_listByName-list.sql new file mode 100755 index 00000000000..93a449fa17a --- /dev/null +++ b/pkg/storage/secret/metadata/testdata/postgres--secure_value_listByName-list.sql @@ -0,0 +1,9 @@ +SELECT + "name", + "keeper" +FROM + "secret_secure_value" +WHERE "namespace" = 'ns' AND + "name" IN ('a', 'b') +FOR UPDATE +; diff --git a/pkg/storage/secret/metadata/testdata/sqlite--secure_value_listByName-list.sql b/pkg/storage/secret/metadata/testdata/sqlite--secure_value_listByName-list.sql new file mode 100755 index 00000000000..48ae01800ae --- /dev/null +++ b/pkg/storage/secret/metadata/testdata/sqlite--secure_value_listByName-list.sql @@ -0,0 +1,8 @@ +SELECT + "name", + "keeper" +FROM + "secret_secure_value" +WHERE "namespace" = 'ns' AND + "name" IN ('a', 'b') +; diff --git a/pkg/tests/apis/secret/keeper_test.go b/pkg/tests/apis/secret/keeper_test.go index 971295746ce..c81a8544ba7 100644 --- a/pkg/tests/apis/secret/keeper_test.go +++ b/pkg/tests/apis/secret/keeper_test.go @@ -134,7 +134,6 @@ func TestIntegrationKeeper(t *testing.T) { }) t.Run("and updating the keeper to reference securevalues that does not exist returns an error", func(t *testing.T) { - t.Skip("skipping because storing credentials as securevalues is not implemented yet") newRaw := helper.LoadYAMLOrJSONFile("testdata/keeper-aws-generate.yaml") newRaw.SetName(raw.GetName()) newRaw.Object["spec"].(map[string]any)["aws"] = map[string]any{ @@ -184,7 +183,6 @@ func TestIntegrationKeeper(t *testing.T) { }) t.Run("creating a keeper that references securevalues that does not exist returns an error", func(t *testing.T) { - t.Skip("skipping because storing credentials as securevalues is not implemented yet") testDataKeeper := helper.LoadYAMLOrJSONFile("testdata/keeper-aws-generate.yaml") testDataKeeper.Object["spec"].(map[string]any)["aws"] = map[string]any{ "accessKeyId": map[string]any{