From 4c59219adbff5e1aa857d231c395fd3fca3b4623 Mon Sep 17 00:00:00 2001 From: Matheus Macabu Date: Wed, 19 Mar 2025 16:31:10 +0100 Subject: [PATCH] SecretsManager: Add reststorage logic with validations (#102464) * SecretsManager: rename keeper + securevalues rest file * SecretsManager: add rest of xkube helper methods * SecretsManager: add domain errors to contracts * SecretsManager: copy over Keeper reststorage from feature branch * SecretsManager: copy over SecureValue reststorage from feature branch --------- Co-authored-by: PoorlyDefinedBehaviour Co-authored-by: Dana Axinte <53751979+dana-axinte@users.noreply.github.com> Co-authored-by: Michael Mandrus --- pkg/registry/apis/secret/contracts/keeper.go | 6 + .../apis/secret/contracts/secure_value.go | 6 + .../apis/secret/reststorage/keeper.go | 128 ------- .../apis/secret/reststorage/keeper_rest.go | 356 ++++++++++++++++++ .../secret/reststorage/keeper_rest_test.go | 332 ++++++++++++++++ .../apis/secret/reststorage/secure_value.go | 129 ------- .../secret/reststorage/secure_value_rest.go | 315 ++++++++++++++++ .../reststorage/secure_value_rest_test.go | 269 +++++++++++++ pkg/registry/apis/secret/xkube/annotations.go | 24 ++ pkg/registry/apis/secret/xkube/errors.go | 9 + 10 files changed, 1317 insertions(+), 257 deletions(-) delete mode 100644 pkg/registry/apis/secret/reststorage/keeper.go create mode 100644 pkg/registry/apis/secret/reststorage/keeper_rest.go create mode 100644 pkg/registry/apis/secret/reststorage/keeper_rest_test.go delete mode 100644 pkg/registry/apis/secret/reststorage/secure_value.go create mode 100644 pkg/registry/apis/secret/reststorage/secure_value_rest.go create mode 100644 pkg/registry/apis/secret/reststorage/secure_value_rest_test.go create mode 100644 pkg/registry/apis/secret/xkube/annotations.go create mode 100644 pkg/registry/apis/secret/xkube/errors.go diff --git a/pkg/registry/apis/secret/contracts/keeper.go b/pkg/registry/apis/secret/contracts/keeper.go index 0f9ab5e62bb..edd996ed4e9 100644 --- a/pkg/registry/apis/secret/contracts/keeper.go +++ b/pkg/registry/apis/secret/contracts/keeper.go @@ -2,12 +2,18 @@ package contracts import ( "context" + "errors" secretv0alpha1 "github.com/grafana/grafana/pkg/apis/secret/v0alpha1" "github.com/grafana/grafana/pkg/registry/apis/secret/xkube" "k8s.io/apimachinery/pkg/apis/meta/internalversion" ) +var ( + ErrKeeperNotFound = errors.New("keeper not found") +) + +// KeeperMetadataStorage is the interface for wiring and dependency injection. type KeeperMetadataStorage interface { Create(ctx context.Context, keeper *secretv0alpha1.Keeper) (*secretv0alpha1.Keeper, error) Read(ctx context.Context, namespace xkube.Namespace, name string) (*secretv0alpha1.Keeper, error) diff --git a/pkg/registry/apis/secret/contracts/secure_value.go b/pkg/registry/apis/secret/contracts/secure_value.go index 5a848731872..a3109e1d44f 100644 --- a/pkg/registry/apis/secret/contracts/secure_value.go +++ b/pkg/registry/apis/secret/contracts/secure_value.go @@ -2,12 +2,18 @@ package contracts import ( "context" + "errors" secretv0alpha1 "github.com/grafana/grafana/pkg/apis/secret/v0alpha1" "github.com/grafana/grafana/pkg/registry/apis/secret/xkube" "k8s.io/apimachinery/pkg/apis/meta/internalversion" ) +var ( + ErrSecureValueNotFound = errors.New("secure value not found") +) + +// SecureValueMetadataStorage is the interface for wiring and dependency injection. type SecureValueMetadataStorage interface { Create(ctx context.Context, sv *secretv0alpha1.SecureValue) (*secretv0alpha1.SecureValue, error) Read(ctx context.Context, namespace xkube.Namespace, name string) (*secretv0alpha1.SecureValue, error) diff --git a/pkg/registry/apis/secret/reststorage/keeper.go b/pkg/registry/apis/secret/reststorage/keeper.go deleted file mode 100644 index 7803d28d1ad..00000000000 --- a/pkg/registry/apis/secret/reststorage/keeper.go +++ /dev/null @@ -1,128 +0,0 @@ -package reststorage - -import ( - "context" - "fmt" - - "k8s.io/apimachinery/pkg/apis/meta/internalversion" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/apiserver/pkg/admission" - "k8s.io/apiserver/pkg/endpoints/request" - "k8s.io/apiserver/pkg/registry/rest" - - "github.com/grafana/grafana/pkg/apimachinery/utils" - secretv0alpha1 "github.com/grafana/grafana/pkg/apis/secret/v0alpha1" - "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" -) - -var ( - _ rest.Scoper = (*KeeperRest)(nil) - _ rest.SingularNameProvider = (*KeeperRest)(nil) - _ rest.Getter = (*KeeperRest)(nil) - _ rest.Lister = (*KeeperRest)(nil) - _ rest.Storage = (*KeeperRest)(nil) - _ rest.Creater = (*KeeperRest)(nil) - _ rest.Updater = (*KeeperRest)(nil) - _ rest.GracefulDeleter = (*KeeperRest)(nil) -) - -// KeeperRest is an ddimplementation of CRUDL operations on a `keeper` backed by TODO. -type KeeperRest struct { - storage contracts.KeeperMetadataStorage - resource utils.ResourceInfo - tableConverter rest.TableConvertor -} - -// NewKeeperRest is a returns a constructed `*KeeperRest`. -func NewKeeperRest(storage contracts.KeeperMetadataStorage, resource utils.ResourceInfo) *KeeperRest { - return &KeeperRest{storage, resource, resource.TableConverter()} -} - -// New returns an empty `*Keeper` that is used by the `Create` method. -func (s *KeeperRest) New() runtime.Object { - return s.resource.NewFunc() -} - -// Destroy is called when? [TODO] -func (s *KeeperRest) Destroy() {} - -// NamespaceScoped returns `true` because the storage is namespaced (== org). -func (s *KeeperRest) NamespaceScoped() bool { - return true -} - -// GetSingularName is used by `kubectl` discovery to have singular name representation of resources. -func (s *KeeperRest) GetSingularName() string { - return s.resource.GetSingularName() -} - -// NewList returns an empty `*KeeperList` that is used by the `List` method. -func (s *KeeperRest) NewList() runtime.Object { - return s.resource.NewListFunc() -} - -// ConvertToTable is used by Kubernetes and converts objects to `metav1.Table`. -func (s *KeeperRest) ConvertToTable(ctx context.Context, object runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) { - return s.tableConverter.ConvertToTable(ctx, object, tableOptions) -} - -// List calls the inner `store` (persistence) and returns a list of `Keepers` within a `namespace` filtered by the `options`. -func (s *KeeperRest) List(ctx context.Context, options *internalversion.ListOptions) (runtime.Object, error) { - _, ok := request.NamespaceFrom(ctx) - if !ok { - return nil, fmt.Errorf("missing namespace") - } - - return &secretv0alpha1.KeeperList{Items: make([]secretv0alpha1.Keeper, 0)}, nil -} - -// Get calls the inner `store` (persistence) and returns a `Keeper` by `name`. -func (s *KeeperRest) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { - _, ok := request.NamespaceFrom(ctx) - if !ok { - return nil, fmt.Errorf("missing namespace") - } - - return nil, s.resource.NewNotFound(name) -} - -// Create a new `Keeper`. Does some validation and allows empty `name` (generated). -func (s *KeeperRest) Create( - ctx context.Context, - obj runtime.Object, - createValidation rest.ValidateObjectFunc, - options *metav1.CreateOptions, -) (runtime.Object, error) { - return nil, nil -} - -// Update a `Keeper`'s `value`. The second return parameter indicates whether the resource was newly created. -func (s *KeeperRest) Update( - ctx context.Context, - name string, - objInfo rest.UpdatedObjectInfo, - createValidation rest.ValidateObjectFunc, - updateValidation rest.ValidateObjectUpdateFunc, - forceAllowCreate bool, - options *metav1.UpdateOptions, -) (runtime.Object, bool, error) { - return nil, false, nil -} - -// Delete calls the inner `store` (persistence) in order to delete the `Keeper`. -// The second return parameter `bool` indicates whether the delete was intant or not. It always is for `Keepers`. -func (s *KeeperRest) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) { - _, ok := request.NamespaceFrom(ctx) - if !ok { - return nil, false, fmt.Errorf("missing namespace") - } - - return nil, true, nil -} - -// ValidateKeeper does basic spec validation of a keeper. -func ValidateKeeper(keeper *secretv0alpha1.Keeper, operation admission.Operation) field.ErrorList { - return nil -} diff --git a/pkg/registry/apis/secret/reststorage/keeper_rest.go b/pkg/registry/apis/secret/reststorage/keeper_rest.go new file mode 100644 index 00000000000..77926723e78 --- /dev/null +++ b/pkg/registry/apis/secret/reststorage/keeper_rest.go @@ -0,0 +1,356 @@ +package reststorage + +import ( + "context" + "errors" + "fmt" + "strings" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/registry/rest" + + "github.com/grafana/grafana/pkg/apimachinery/utils" + secretv0alpha1 "github.com/grafana/grafana/pkg/apis/secret/v0alpha1" + "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" + "github.com/grafana/grafana/pkg/registry/apis/secret/xkube" +) + +var ( + _ rest.Scoper = (*KeeperRest)(nil) + _ rest.SingularNameProvider = (*KeeperRest)(nil) + _ rest.Getter = (*KeeperRest)(nil) + _ rest.Lister = (*KeeperRest)(nil) + _ rest.Storage = (*KeeperRest)(nil) + _ rest.Creater = (*KeeperRest)(nil) + _ rest.Updater = (*KeeperRest)(nil) + _ rest.GracefulDeleter = (*KeeperRest)(nil) +) + +// KeeperRest is an implementation of CRUDL operations on a `keeper` backed by TODO. +type KeeperRest struct { + storage contracts.KeeperMetadataStorage + resource utils.ResourceInfo + tableConverter rest.TableConvertor +} + +// NewKeeperRest is a returns a constructed `*KeeperRest`. +func NewKeeperRest(storage contracts.KeeperMetadataStorage, resource utils.ResourceInfo) *KeeperRest { + return &KeeperRest{storage, resource, resource.TableConverter()} +} + +// New returns an empty `*Keeper` that is used by the `Create` method. +func (s *KeeperRest) New() runtime.Object { + return s.resource.NewFunc() +} + +// Destroy is called when? [TODO] +func (s *KeeperRest) Destroy() {} + +// NamespaceScoped returns `true` because the storage is namespaced (== org). +func (s *KeeperRest) NamespaceScoped() bool { + return true +} + +// GetSingularName is used by `kubectl` discovery to have singular name representation of resources. +func (s *KeeperRest) GetSingularName() string { + return s.resource.GetSingularName() +} + +// NewList returns an empty `*KeeperList` that is used by the `List` method. +func (s *KeeperRest) NewList() runtime.Object { + return s.resource.NewListFunc() +} + +// ConvertToTable is used by Kubernetes and converts objects to `metav1.Table`. +func (s *KeeperRest) ConvertToTable(ctx context.Context, object runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) { + return s.tableConverter.ConvertToTable(ctx, object, tableOptions) +} + +// List calls the inner `store` (persistence) and returns a list of `Keepers` within a `namespace` filtered by the `options`. +func (s *KeeperRest) List(ctx context.Context, options *internalversion.ListOptions) (runtime.Object, error) { + namespace, ok := request.NamespaceFrom(ctx) + if !ok { + return nil, fmt.Errorf("missing namespace") + } + + keepersList, err := s.storage.List(ctx, xkube.Namespace(namespace), options) + if err != nil { + return nil, fmt.Errorf("failed to list keepers: %w", err) + } + + return keepersList, nil +} + +// Get calls the inner `store` (persistence) and returns a `Keeper` by `name`. +func (s *KeeperRest) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { + namespace, ok := request.NamespaceFrom(ctx) + if !ok { + return nil, fmt.Errorf("missing namespace") + } + + kp, err := s.storage.Read(ctx, xkube.Namespace(namespace), name) + if err != nil { + if errors.Is(err, contracts.ErrKeeperNotFound) { + return nil, s.resource.NewNotFound(name) + } + return nil, fmt.Errorf("failed to read keeper: %w", err) + } + + return kp, nil +} + +// Create a new `Keeper`. Does some validation and allows empty `name` (generated). +func (s *KeeperRest) Create( + ctx context.Context, + obj runtime.Object, + createValidation rest.ValidateObjectFunc, + options *metav1.CreateOptions, +) (runtime.Object, error) { + kp, ok := obj.(*secretv0alpha1.Keeper) + if !ok { + return nil, fmt.Errorf("expected Keeper for create") + } + + if err := createValidation(ctx, obj); err != nil { + return nil, err + } + + createdKeeper, err := s.storage.Create(ctx, kp) + if err != nil { + var kErr xkube.ErrorLister + if errors.As(err, &kErr) { + return nil, apierrors.NewInvalid(kp.GroupVersionKind().GroupKind(), kp.Name, kErr.ErrorList()) + } + + return nil, fmt.Errorf("failed to create keeper: %w", err) + } + + return createdKeeper, nil +} + +// Update a `Keeper`'s `value`. The second return parameter indicates whether the resource was newly created. +func (s *KeeperRest) Update( + ctx context.Context, + name string, + objInfo rest.UpdatedObjectInfo, + createValidation rest.ValidateObjectFunc, + updateValidation rest.ValidateObjectUpdateFunc, + forceAllowCreate bool, + options *metav1.UpdateOptions, +) (runtime.Object, bool, error) { + oldObj, err := s.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + return nil, false, err + } + + // Makes sure the UID and ResourceVersion are OK. + // TODO: this also makes it so the labels and annotations are additive, unless we check and remove manually. + newObj, err := objInfo.UpdatedObject(ctx, oldObj) + if err != nil { + return nil, false, fmt.Errorf("k8s updated object: %w", err) + } + + // The current supported behavior for `Update` is to replace the entire `spec` with the new one. + // Each provider-specific setting of a keeper lives at the top-level, so it makes it possible to change a provider + // during an update. Otherwise both old and new providers would be merged in the `newObj` which is not allowed. + if err := updateValidation(ctx, newObj, oldObj); err != nil { + return nil, false, err + } + + newKeeper, ok := newObj.(*secretv0alpha1.Keeper) + if !ok { + return nil, false, fmt.Errorf("expected Keeper for update") + } + + // TODO: do we need to do this here again? Probably not, but double-check! + newKeeper.Annotations = xkube.CleanAnnotations(newKeeper.Annotations) + + // Current implementation replaces everything passed in the spec, so it is not a PATCH. Do we want/need to support that? + updatedKeeper, err := s.storage.Update(ctx, newKeeper) + if err != nil { + var kErr xkube.ErrorLister + if errors.As(err, &kErr) { + return nil, false, apierrors.NewInvalid(newKeeper.GroupVersionKind().GroupKind(), newKeeper.Name, kErr.ErrorList()) + } + + return nil, false, fmt.Errorf("failed to update keeper: %w", err) + } + + return updatedKeeper, false, nil +} + +// Delete calls the inner `store` (persistence) in order to delete the `Keeper`. +// The second return parameter `bool` indicates whether the delete was intant or not. It always is for `Keepers`. +func (s *KeeperRest) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) { + namespace, ok := request.NamespaceFrom(ctx) + if !ok { + return nil, false, fmt.Errorf("missing namespace") + } + + if err := s.storage.Delete(ctx, xkube.Namespace(namespace), name); err != nil { + return nil, false, fmt.Errorf("failed to delete keeper: %w", err) + } + + return nil, true, nil +} + +// ValidateKeeper does basic spec validation of a keeper. +func ValidateKeeper(keeper *secretv0alpha1.Keeper, operation admission.Operation) field.ErrorList { + // Only validate Create and Update for now. + if operation != admission.Create && operation != admission.Update { + return nil + } + + errs := make(field.ErrorList, 0) + + if keeper.Spec.Title == "" { + errs = append(errs, field.Required(field.NewPath("spec", "title"), "a `title` is required")) + } + + // Only one keeper type can be configured. Return early and don't validate the specific keeper fields. + if err := validateKeepers(keeper); err != nil { + errs = append(errs, err) + + return errs + } + + // TODO: Improve SQL keeper validation. + // SQL keeper is not allowed to use `secureValueName` in credentials fields to avoid depending on another keeper. + if keeper.IsSqlKeeper() { + if keeper.Spec.SQL.Encryption.AWS != nil { + if keeper.Spec.SQL.Encryption.AWS.AccessKeyID.SecureValueName != "" { + errs = append(errs, field.Forbidden(field.NewPath("spec", "aws", "accessKeyId"), "secureValueName cannot be used with SQL keeper")) + } + + if keeper.Spec.SQL.Encryption.AWS.SecretAccessKey.SecureValueName != "" { + errs = append(errs, field.Forbidden(field.NewPath("spec", "aws", "secretAccessKey"), "secureValueName cannot be used with SQL keeper")) + } + } + + if keeper.Spec.SQL.Encryption.Azure != nil && keeper.Spec.SQL.Encryption.Azure.ClientSecret.SecureValueName != "" { + errs = append(errs, field.Forbidden(field.NewPath("spec", "azure", "clientSecret"), "secureValueName cannot be used with SQL keeper")) + } + + if keeper.Spec.SQL.Encryption.HashiCorp != nil && keeper.Spec.SQL.Encryption.HashiCorp.Token.SecureValueName != "" { + errs = append(errs, field.Forbidden(field.NewPath("spec", "hashicorp", "token"), "secureValueName cannot be used with SQL keeper")) + } + } + + if keeper.Spec.AWS != nil { + if err := validateCredentialValue(field.NewPath("spec", "aws", "accessKeyId"), keeper.Spec.AWS.AccessKeyID); err != nil { + errs = append(errs, err) + } + + if err := validateCredentialValue(field.NewPath("spec", "aws", "secretAccessKey"), keeper.Spec.AWS.SecretAccessKey); err != nil { + errs = append(errs, err) + } + } + + if keeper.Spec.Azure != nil { + if keeper.Spec.Azure.KeyVaultName == "" { + errs = append(errs, field.Required(field.NewPath("spec", "azure", "keyVaultName"), "a `keyVaultName` is required")) + } + + if keeper.Spec.Azure.TenantID == "" { + errs = append(errs, field.Required(field.NewPath("spec", "azure", "tenantId"), "a `tenantId` is required")) + } + + if keeper.Spec.Azure.ClientID == "" { + errs = append(errs, field.Required(field.NewPath("spec", "azure", "clientId"), "a `clientId` is required")) + } + + if err := validateCredentialValue(field.NewPath("spec", "azure", "clientSecret"), keeper.Spec.Azure.ClientSecret); err != nil { + errs = append(errs, err) + } + } + + if keeper.Spec.GCP != nil { + if keeper.Spec.GCP.ProjectID == "" { + errs = append(errs, field.Required(field.NewPath("spec", "gcp", "projectId"), "a `projectId` is required")) + } + + if keeper.Spec.GCP.CredentialsFile == "" { + errs = append(errs, field.Required(field.NewPath("spec", "gcp", "credentialsFile"), "a `credentialsFile` is required")) + } + } + + if keeper.Spec.HashiCorp != nil { + if keeper.Spec.HashiCorp.Address == "" { + errs = append(errs, field.Required(field.NewPath("spec", "hashicorp", "address"), "a `address` is required")) + } + + if err := validateCredentialValue(field.NewPath("spec", "hashicorp", "token"), keeper.Spec.HashiCorp.Token); err != nil { + errs = append(errs, err) + } + } + + return errs +} + +func validateKeepers(keeper *secretv0alpha1.Keeper) *field.Error { + availableKeepers := map[string]bool{ + "sql": keeper.Spec.SQL != nil, + "aws": keeper.Spec.AWS != nil, + "azure": keeper.Spec.Azure != nil, + "gcp": keeper.Spec.GCP != nil, + "hashicorp": keeper.Spec.HashiCorp != nil, + } + + configuredKeepers := make([]string, 0) + + for keeperKind, notNil := range availableKeepers { + if notNil { + configuredKeepers = append(configuredKeepers, keeperKind) + } + } + + if len(configuredKeepers) == 0 { + return field.Required(field.NewPath("spec"), "at least one `keeper` must be present") + } + + if len(configuredKeepers) > 1 { + return field.Invalid( + field.NewPath("spec"), + strings.Join(configuredKeepers, " & "), + "only one `keeper` can be present at a time but found more", + ) + } + + return nil +} + +func validateCredentialValue(path *field.Path, credentials secretv0alpha1.CredentialValue) *field.Error { + availableOptions := map[string]bool{ + "secureValueName": credentials.SecureValueName != "", + "valueFromEnv": credentials.ValueFromEnv != "", + "valueFromConfig": credentials.ValueFromConfig != "", + } + + configuredCredentials := make([]string, 0) + + for credentialKind, notEmpty := range availableOptions { + if notEmpty { + configuredCredentials = append(configuredCredentials, credentialKind) + } + } + + if len(configuredCredentials) == 0 { + return field.Required(path, "one of `secureValueName`, `valueFromEnv` or `valueFromConfig` must be present") + } + + if len(configuredCredentials) > 1 { + return field.Invalid( + path, + strings.Join(configuredCredentials, " & "), + "only one of `secureValueName`, `valueFromEnv` or `valueFromConfig` must be present at a time but found more", + ) + } + + return nil +} diff --git a/pkg/registry/apis/secret/reststorage/keeper_rest_test.go b/pkg/registry/apis/secret/reststorage/keeper_rest_test.go new file mode 100644 index 00000000000..abc1ac52b63 --- /dev/null +++ b/pkg/registry/apis/secret/reststorage/keeper_rest_test.go @@ -0,0 +1,332 @@ +package reststorage + +import ( + "testing" + + secretv0alpha1 "github.com/grafana/grafana/pkg/apis/secret/v0alpha1" + "github.com/stretchr/testify/require" + "k8s.io/apiserver/pkg/admission" +) + +func TestValidateKeeper(t *testing.T) { + t.Run("when creating a new keeper", func(t *testing.T) { + t.Run("the `title` must be present", func(t *testing.T) { + keeper := &secretv0alpha1.Keeper{ + Spec: secretv0alpha1.KeeperSpec{ + SQL: &secretv0alpha1.SQLKeeperConfig{}, + }, + } + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.title", errs[0].Field) + }) + }) + + t.Run("only one `keeper` must be present", func(t *testing.T) { + keeper := &secretv0alpha1.Keeper{ + Spec: secretv0alpha1.KeeperSpec{ + Title: "title", + SQL: &secretv0alpha1.SQLKeeperConfig{}, + AWS: &secretv0alpha1.AWSKeeperConfig{}, + Azure: &secretv0alpha1.AzureKeeperConfig{}, + GCP: &secretv0alpha1.GCPKeeperConfig{}, + HashiCorp: &secretv0alpha1.HashiCorpKeeperConfig{}, + }, + } + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec", errs[0].Field) + }) + + t.Run("at least one `keeper` must be present", func(t *testing.T) { + keeper := &secretv0alpha1.Keeper{ + Spec: secretv0alpha1.KeeperSpec{ + Title: "title", + }, + } + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec", errs[0].Field) + }) + + t.Run("aws keeper validation", func(t *testing.T) { + validKeeperAWS := &secretv0alpha1.Keeper{ + Spec: secretv0alpha1.KeeperSpec{ + Title: "title", + AWS: &secretv0alpha1.AWSKeeperConfig{ + AWSCredentials: secretv0alpha1.AWSCredentials{ + AccessKeyID: secretv0alpha1.CredentialValue{ + ValueFromEnv: "some-value", + }, + SecretAccessKey: secretv0alpha1.CredentialValue{ + SecureValueName: "some-value", + }, + KMSKeyID: "optional", + }, + }, + }, + } + + t.Run("`accessKeyId` must be present", func(t *testing.T) { + t.Run("at least one of the credential value must be present", func(t *testing.T) { + keeper := validKeeperAWS.DeepCopy() + keeper.Spec.AWS.AccessKeyID = secretv0alpha1.CredentialValue{} + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.aws.accessKeyId", errs[0].Field) + }) + + t.Run("at most one of the credential value must be present", func(t *testing.T) { + keeper := validKeeperAWS.DeepCopy() + keeper.Spec.AWS.AccessKeyID = secretv0alpha1.CredentialValue{ + SecureValueName: "a", + ValueFromEnv: "b", + ValueFromConfig: "c", + } + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.aws.accessKeyId", errs[0].Field) + }) + }) + + t.Run("`secretAccessKey` must be present", func(t *testing.T) { + t.Run("at least one of the credential value must be present", func(t *testing.T) { + keeper := validKeeperAWS.DeepCopy() + keeper.Spec.AWS.SecretAccessKey = secretv0alpha1.CredentialValue{} + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.aws.secretAccessKey", errs[0].Field) + }) + + t.Run("at most one of the credential value must be present", func(t *testing.T) { + keeper := validKeeperAWS.DeepCopy() + keeper.Spec.AWS.SecretAccessKey = secretv0alpha1.CredentialValue{ + SecureValueName: "a", + ValueFromEnv: "b", + ValueFromConfig: "c", + } + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.aws.secretAccessKey", errs[0].Field) + }) + }) + }) + + t.Run("azure keeper validation", func(t *testing.T) { + validKeeperAzure := &secretv0alpha1.Keeper{ + Spec: secretv0alpha1.KeeperSpec{ + Title: "title", + Azure: &secretv0alpha1.AzureKeeperConfig{ + AzureCredentials: secretv0alpha1.AzureCredentials{ + KeyVaultName: "kv-name", + TenantID: "tenant-id", + ClientID: "client-id", + ClientSecret: secretv0alpha1.CredentialValue{ + ValueFromConfig: "config.path.value", + }, + }, + }, + }, + } + + t.Run("`keyVaultName` must be present", func(t *testing.T) { + keeper := validKeeperAzure.DeepCopy() + keeper.Spec.Azure.KeyVaultName = "" + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.azure.keyVaultName", errs[0].Field) + }) + + t.Run("`tenantId` must be present", func(t *testing.T) { + keeper := validKeeperAzure.DeepCopy() + keeper.Spec.Azure.TenantID = "" + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.azure.tenantId", errs[0].Field) + }) + + t.Run("`clientId` must be present", func(t *testing.T) { + keeper := validKeeperAzure.DeepCopy() + keeper.Spec.Azure.ClientID = "" + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.azure.clientId", errs[0].Field) + }) + + t.Run("`clientSecret` must be present", func(t *testing.T) { + t.Run("at least one of the credential value must be present", func(t *testing.T) { + keeper := validKeeperAzure.DeepCopy() + keeper.Spec.Azure.ClientSecret = secretv0alpha1.CredentialValue{} + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.azure.clientSecret", errs[0].Field) + }) + + t.Run("at most one of the credential value must be present", func(t *testing.T) { + keeper := validKeeperAzure.DeepCopy() + keeper.Spec.Azure.ClientSecret = secretv0alpha1.CredentialValue{ + SecureValueName: "a", + ValueFromEnv: "b", + ValueFromConfig: "c", + } + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.azure.clientSecret", errs[0].Field) + }) + }) + }) + + t.Run("gcp keeper validation", func(t *testing.T) { + validKeeperGCP := &secretv0alpha1.Keeper{ + Spec: secretv0alpha1.KeeperSpec{ + Title: "title", + GCP: &secretv0alpha1.GCPKeeperConfig{ + GCPCredentials: secretv0alpha1.GCPCredentials{ + ProjectID: "project-id", + CredentialsFile: "/path/to/credentials/file.json", + }, + }, + }, + } + + t.Run("`projectId` must be present", func(t *testing.T) { + keeper := validKeeperGCP.DeepCopy() + keeper.Spec.GCP.ProjectID = "" + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.gcp.projectId", errs[0].Field) + }) + + t.Run("`credentialsFile` must be present", func(t *testing.T) { + keeper := validKeeperGCP.DeepCopy() + keeper.Spec.GCP.CredentialsFile = "" + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.gcp.credentialsFile", errs[0].Field) + }) + }) + + t.Run("hashicorp keeper validation", func(t *testing.T) { + validKeeperHashiCorp := &secretv0alpha1.Keeper{ + Spec: secretv0alpha1.KeeperSpec{ + Title: "title", + HashiCorp: &secretv0alpha1.HashiCorpKeeperConfig{ + HashiCorpCredentials: secretv0alpha1.HashiCorpCredentials{ + Address: "http://address", + Token: secretv0alpha1.CredentialValue{ + ValueFromConfig: "config.path.value", + }, + }, + }, + }, + } + + t.Run("`address` must be present", func(t *testing.T) { + keeper := validKeeperHashiCorp.DeepCopy() + keeper.Spec.HashiCorp.Address = "" + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.hashicorp.address", errs[0].Field) + }) + + t.Run("`token` must be present", func(t *testing.T) { + t.Run("at least one of the credential value must be present", func(t *testing.T) { + keeper := validKeeperHashiCorp.DeepCopy() + keeper.Spec.HashiCorp.Token = secretv0alpha1.CredentialValue{} + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.hashicorp.token", errs[0].Field) + }) + + t.Run("at most one of the credential value must be present", func(t *testing.T) { + keeper := validKeeperHashiCorp.DeepCopy() + keeper.Spec.HashiCorp.Token = secretv0alpha1.CredentialValue{ + SecureValueName: "a", + ValueFromEnv: "b", + ValueFromConfig: "c", + } + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "spec.hashicorp.token", errs[0].Field) + }) + }) + }) + + t.Run("sql keeper validation", func(t *testing.T) { + t.Run("does not allow usage of `secureValueName` in credentials", func(t *testing.T) { + providers := []struct { + name string + enc secretv0alpha1.Encryption + expectedErrors int + }{ + { + name: "aws", + enc: secretv0alpha1.Encryption{ + AWS: &secretv0alpha1.AWSCredentials{ + AccessKeyID: secretv0alpha1.CredentialValue{ + SecureValueName: "not-empty", + }, + SecretAccessKey: secretv0alpha1.CredentialValue{ + SecureValueName: "not-empty", + }, + }, + }, + expectedErrors: 2, + }, + { + name: "azure", + enc: secretv0alpha1.Encryption{ + Azure: &secretv0alpha1.AzureCredentials{ + ClientSecret: secretv0alpha1.CredentialValue{ + SecureValueName: "not-empty", + }, + }, + }, + expectedErrors: 1, + }, + { + name: "hashicorp", + enc: secretv0alpha1.Encryption{ + HashiCorp: &secretv0alpha1.HashiCorpCredentials{ + Token: secretv0alpha1.CredentialValue{ + SecureValueName: "not-empty", + }, + }, + }, + expectedErrors: 1, + }, + } + + for _, tc := range providers { + t.Run("when using credentials for "+tc.name, func(t *testing.T) { + keeper := &secretv0alpha1.Keeper{ + Spec: secretv0alpha1.KeeperSpec{ + Title: "title", + SQL: &secretv0alpha1.SQLKeeperConfig{Encryption: &tc.enc}, + }, + } + + errs := ValidateKeeper(keeper, admission.Create) + require.Len(t, errs, tc.expectedErrors) + }) + } + }) + }) +} diff --git a/pkg/registry/apis/secret/reststorage/secure_value.go b/pkg/registry/apis/secret/reststorage/secure_value.go deleted file mode 100644 index 10dc2695170..00000000000 --- a/pkg/registry/apis/secret/reststorage/secure_value.go +++ /dev/null @@ -1,129 +0,0 @@ -package reststorage - -import ( - "context" - "fmt" - - "k8s.io/apimachinery/pkg/apis/meta/internalversion" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/apiserver/pkg/admission" - "k8s.io/apiserver/pkg/endpoints/request" - "k8s.io/apiserver/pkg/registry/rest" - - "github.com/grafana/grafana/pkg/apimachinery/utils" - secretv0alpha1 "github.com/grafana/grafana/pkg/apis/secret/v0alpha1" - "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" -) - -var ( - _ rest.Scoper = (*SecureValueRest)(nil) - _ rest.SingularNameProvider = (*SecureValueRest)(nil) - _ rest.Getter = (*SecureValueRest)(nil) - _ rest.Lister = (*SecureValueRest)(nil) - _ rest.Storage = (*SecureValueRest)(nil) - _ rest.Creater = (*SecureValueRest)(nil) - _ rest.Updater = (*SecureValueRest)(nil) - _ rest.GracefulDeleter = (*SecureValueRest)(nil) -) - -// SecureValueRest is an implementation of CRUDL operations on a `securevalue` backed by a persistence layer `store`. -type SecureValueRest struct { - storage contracts.SecureValueMetadataStorage - resource utils.ResourceInfo - tableConverter rest.TableConvertor -} - -// NewSecureValueRest is a returns a constructed `*SecureValueRest`. -func NewSecureValueRest(storage contracts.SecureValueMetadataStorage, resource utils.ResourceInfo) *SecureValueRest { - return &SecureValueRest{storage, resource, resource.TableConverter()} -} - -// New returns an empty `*SecureValue` that is used by the `Create` method. -func (s *SecureValueRest) New() runtime.Object { - return s.resource.NewFunc() -} - -// Destroy is called when? [TODO] -func (s *SecureValueRest) Destroy() {} - -// NamespaceScoped returns `true` because the storage is namespaced (== org). -func (s *SecureValueRest) NamespaceScoped() bool { - return true -} - -// GetSingularName is used by `kubectl` discovery to have singular name representation of resources. -func (s *SecureValueRest) GetSingularName() string { - return s.resource.GetSingularName() -} - -// NewList returns an empty `*SecureValueList` that is used by the `List` method. -func (s *SecureValueRest) NewList() runtime.Object { - return s.resource.NewListFunc() -} - -// ConvertToTable is used by Kubernetes and converts objects to `metav1.Table`. -func (s *SecureValueRest) ConvertToTable(ctx context.Context, object runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) { - return s.tableConverter.ConvertToTable(ctx, object, tableOptions) -} - -// List calls the inner `store` (persistence) and returns a list of `securevalues` within a `namespace` filtered by the `options`. -func (s *SecureValueRest) List(ctx context.Context, options *internalversion.ListOptions) (runtime.Object, error) { - _, ok := request.NamespaceFrom(ctx) - if !ok { - return nil, fmt.Errorf("missing namespace") - } - - return &secretv0alpha1.SecureValueList{Items: make([]secretv0alpha1.SecureValue, 0)}, nil -} - -// Get calls the inner `store` (persistence) and returns a `securevalue` by `name`. It will NOT return the decrypted `value`. -func (s *SecureValueRest) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { - _, ok := request.NamespaceFrom(ctx) - if !ok { - return nil, fmt.Errorf("missing namespace") - } - - return nil, s.resource.NewNotFound(name) -} - -// Create a new `securevalue`. Does some validation and allows empty `name` (generated). -func (s *SecureValueRest) Create( - ctx context.Context, - obj runtime.Object, - createValidation rest.ValidateObjectFunc, - options *metav1.CreateOptions, -) (runtime.Object, error) { - return nil, nil -} - -// Update a `securevalue`'s `value`. The second return parameter indicates whether the resource was newly created. -// Currently does not support "create on update" functionality. If the securevalue does not yet exist, it returns an error. -func (s *SecureValueRest) Update( - ctx context.Context, - name string, - objInfo rest.UpdatedObjectInfo, - createValidation rest.ValidateObjectFunc, - updateValidation rest.ValidateObjectUpdateFunc, - forceAllowCreate bool, - options *metav1.UpdateOptions, -) (runtime.Object, bool, error) { - return nil, false, nil -} - -// Delete calls the inner `store` (persistence) in order to delete the `securevalue`. -// The second return parameter `bool` indicates whether the delete was instant or not. It always is for `securevalues`. -func (s *SecureValueRest) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) { - _, ok := request.NamespaceFrom(ctx) - if !ok { - return nil, false, fmt.Errorf("missing namespace") - } - - return nil, true, nil -} - -// ValidateSecureValue does basic spec validation of a securevalue. -func ValidateSecureValue(sv, oldSv *secretv0alpha1.SecureValue, operation admission.Operation, decryptersAllowList map[string]struct{}) field.ErrorList { - return nil -} diff --git a/pkg/registry/apis/secret/reststorage/secure_value_rest.go b/pkg/registry/apis/secret/reststorage/secure_value_rest.go new file mode 100644 index 00000000000..f0b043108e9 --- /dev/null +++ b/pkg/registry/apis/secret/reststorage/secure_value_rest.go @@ -0,0 +1,315 @@ +package reststorage + +import ( + "context" + "errors" + "fmt" + "strconv" + "strings" + + "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/registry/rest" + + "github.com/grafana/grafana/pkg/apimachinery/utils" + secretv0alpha1 "github.com/grafana/grafana/pkg/apis/secret/v0alpha1" + "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" + "github.com/grafana/grafana/pkg/registry/apis/secret/xkube" +) + +var ( + _ rest.Scoper = (*SecureValueRest)(nil) + _ rest.SingularNameProvider = (*SecureValueRest)(nil) + _ rest.Getter = (*SecureValueRest)(nil) + _ rest.Lister = (*SecureValueRest)(nil) + _ rest.Storage = (*SecureValueRest)(nil) + _ rest.Creater = (*SecureValueRest)(nil) + _ rest.Updater = (*SecureValueRest)(nil) + _ rest.GracefulDeleter = (*SecureValueRest)(nil) +) + +// SecureValueRest is an implementation of CRUDL operations on a `securevalue` backed by a persistence layer `store`. +type SecureValueRest struct { + storage contracts.SecureValueMetadataStorage + resource utils.ResourceInfo + tableConverter rest.TableConvertor +} + +// NewSecureValueRest is a returns a constructed `*SecureValueRest`. +func NewSecureValueRest(storage contracts.SecureValueMetadataStorage, resource utils.ResourceInfo) *SecureValueRest { + return &SecureValueRest{storage, resource, resource.TableConverter()} +} + +// New returns an empty `*SecureValue` that is used by the `Create` method. +func (s *SecureValueRest) New() runtime.Object { + return s.resource.NewFunc() +} + +// Destroy is called when? [TODO] +func (s *SecureValueRest) Destroy() {} + +// NamespaceScoped returns `true` because the storage is namespaced (== org). +func (s *SecureValueRest) NamespaceScoped() bool { + return true +} + +// GetSingularName is used by `kubectl` discovery to have singular name representation of resources. +func (s *SecureValueRest) GetSingularName() string { + return s.resource.GetSingularName() +} + +// NewList returns an empty `*SecureValueList` that is used by the `List` method. +func (s *SecureValueRest) NewList() runtime.Object { + return s.resource.NewListFunc() +} + +// ConvertToTable is used by Kubernetes and converts objects to `metav1.Table`. +func (s *SecureValueRest) ConvertToTable(ctx context.Context, object runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) { + return s.tableConverter.ConvertToTable(ctx, object, tableOptions) +} + +// List calls the inner `store` (persistence) and returns a list of `securevalues` within a `namespace` filtered by the `options`. +func (s *SecureValueRest) List(ctx context.Context, options *internalversion.ListOptions) (runtime.Object, error) { + namespace, ok := request.NamespaceFrom(ctx) + if !ok { + return nil, fmt.Errorf("missing namespace") + } + + secureValueList, err := s.storage.List(ctx, xkube.Namespace(namespace), options) + if err != nil { + return nil, fmt.Errorf("failed to list secure values: %w", err) + } + + return secureValueList, nil +} + +// Get calls the inner `store` (persistence) and returns a `securevalue` by `name`. It will NOT return the decrypted `value`. +func (s *SecureValueRest) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { + namespace, ok := request.NamespaceFrom(ctx) + if !ok { + return nil, fmt.Errorf("missing namespace") + } + + sv, err := s.storage.Read(ctx, xkube.Namespace(namespace), name) + if err != nil { + if errors.Is(err, contracts.ErrSecureValueNotFound) { + return nil, s.resource.NewNotFound(name) + } + + return nil, fmt.Errorf("failed to read secure value: %w", err) + } + + return sv, nil +} + +// Create a new `securevalue`. Does some validation and allows empty `name` (generated). +func (s *SecureValueRest) Create( + ctx context.Context, + obj runtime.Object, + createValidation rest.ValidateObjectFunc, + options *metav1.CreateOptions, +) (runtime.Object, error) { + sv, ok := obj.(*secretv0alpha1.SecureValue) + if !ok { + return nil, fmt.Errorf("expected SecureValue for create") + } + + if err := createValidation(ctx, obj); err != nil { + return nil, err + } + + createdSecureValue, err := s.storage.Create(ctx, sv) + if err != nil { + return nil, fmt.Errorf("failed to create secure value: %w", err) + } + + return createdSecureValue, nil +} + +// Update a `securevalue`'s `value`. The second return parameter indicates whether the resource was newly created. +// Currently does not support "create on update" functionality. If the securevalue does not yet exist, it returns an error. +func (s *SecureValueRest) Update( + ctx context.Context, + name string, + objInfo rest.UpdatedObjectInfo, + createValidation rest.ValidateObjectFunc, + updateValidation rest.ValidateObjectUpdateFunc, + forceAllowCreate bool, + options *metav1.UpdateOptions, +) (runtime.Object, bool, error) { + oldObj, err := s.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + return nil, false, err + } + + // Makes sure the UID and ResourceVersion are OK. + // TODO: this also makes it so the labels and annotations are additive, unless we check and remove manually. + newObj, err := objInfo.UpdatedObject(ctx, oldObj) + if err != nil { + return nil, false, fmt.Errorf("k8s updated object: %w", err) + } + + if err := updateValidation(ctx, newObj, oldObj); err != nil { + return nil, false, err + } + + newSecureValue, ok := newObj.(*secretv0alpha1.SecureValue) + if !ok { + return nil, false, fmt.Errorf("expected SecureValue for update") + } + + // TODO: do we need to do this here again? Probably not, but double-check! + newSecureValue.Annotations = xkube.CleanAnnotations(newSecureValue.Annotations) + + // Current implementation replaces everything passed in the spec, so it is not a PATCH. Do we want/need to support that? + updatedSecureValue, err := s.storage.Update(ctx, newSecureValue) + if err != nil { + return nil, false, fmt.Errorf("failed to update secure value: %w", err) + } + + return updatedSecureValue, false, nil +} + +// Delete calls the inner `store` (persistence) in order to delete the `securevalue`. +// The second return parameter `bool` indicates whether the delete was instant or not. It always is for `securevalues`. +func (s *SecureValueRest) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) { + namespace, ok := request.NamespaceFrom(ctx) + if !ok { + return nil, false, fmt.Errorf("missing namespace") + } + + if err := s.storage.Delete(ctx, xkube.Namespace(namespace), name); err != nil { + return nil, false, fmt.Errorf("delete secure value: %w", err) + } + + return nil, true, nil +} + +// ValidateSecureValue does basic spec validation of a securevalue. +func ValidateSecureValue(sv, oldSv *secretv0alpha1.SecureValue, operation admission.Operation, decryptersAllowList map[string]struct{}) field.ErrorList { + errs := make(field.ErrorList, 0) + + // Operation-specific field validation. + switch operation { + case admission.Create: + errs = validateSecureValueCreate(sv) + + // If we plan to support PATCH-style updates, we shouldn't be requiring fields to be set. + case admission.Update: + errs = validateSecureValueUpdate(sv, oldSv) + + case admission.Delete: + case admission.Connect: + } + + // General validations. + if errs := validateDecrypters(sv.Spec.Decrypters, decryptersAllowList); len(errs) > 0 { + return errs + } + + return errs +} + +// validateSecureValueCreate does basic spec validation of a securevalue for the Create operation. +func validateSecureValueCreate(sv *secretv0alpha1.SecureValue) field.ErrorList { + errs := make(field.ErrorList, 0) + + if sv.Spec.Title == "" { + errs = append(errs, field.Required(field.NewPath("spec", "title"), "a `title` is required")) + } + + if sv.Spec.Keeper == "" { + errs = append(errs, field.Required(field.NewPath("spec", "keeper"), "a `keeper` is required")) + } + + if sv.Spec.Value == "" && sv.Spec.Ref == "" { + errs = append(errs, field.Required(field.NewPath("spec"), "either a `value` or `ref` is required")) + } + + if sv.Spec.Value != "" && sv.Spec.Ref != "" { + errs = append(errs, field.Forbidden(field.NewPath("spec"), "only one of `value` or `ref` can be set")) + } + + return errs +} + +// validateSecureValueUpdate does basic spec validation of a securevalue for the Update operation. +func validateSecureValueUpdate(sv, oldSv *secretv0alpha1.SecureValue) field.ErrorList { + errs := make(field.ErrorList, 0) + + // For updates, an `old` object is required. + if oldSv == nil { + errs = append(errs, field.InternalError(field.NewPath("spec"), errors.New("old object is nil"))) + + return errs + } + + // Only validate if one of the fields is being changed/set. + if sv.Spec.Value != "" || sv.Spec.Ref != "" { + if oldSv.Spec.Ref != "" && sv.Spec.Value != "" { + errs = append(errs, field.Forbidden(field.NewPath("spec"), "cannot set `value` when `ref` was already previously set")) + } + + if oldSv.Spec.Ref == "" && sv.Spec.Ref != "" { + errs = append(errs, field.Forbidden(field.NewPath("spec"), "cannot set `ref` when `value` was already previously set")) + } + } + + // Keeper cannot be changed. + if sv.Spec.Keeper != oldSv.Spec.Keeper { + errs = append(errs, field.Forbidden(field.NewPath("spec"), "the `keeper` cannot be changed")) + } + + return errs +} + +// validateDecrypters validates that (if populated) the `decrypters` must match "actor_{name}" and must be unique. +func validateDecrypters(decrypters []string, decryptersAllowList map[string]struct{}) field.ErrorList { + errs := make(field.ErrorList, 0) + + decrypterNames := make(map[string]struct{}, 0) + + for i, decrypter := range decrypters { + // Allow List: decrypters must match exactly and be in the allowed list to be able to decrypt. + // This means an allow list item should have the format "actor_{name}" and not just "{name}". + if len(decryptersAllowList) > 0 { + if _, exists := decryptersAllowList[decrypter]; !exists { + errs = append( + errs, + field.Invalid(field.NewPath("spec", "decrypters", "["+strconv.Itoa(i)+"]"), decrypter, fmt.Sprintf("allowed values: %v", decryptersAllowList)), + ) + + return errs + } + + continue + } + + actor, name, found := strings.Cut(strings.TrimSpace(decrypter), "_") + if !found || actor != "actor" || name == "" { + errs = append( + errs, + field.Invalid(field.NewPath("spec", "decrypters", "["+strconv.Itoa(i)+"]"), decrypter, "a decrypter must have the format `actor_{name}`"), + ) + + continue + } + + if _, exists := decrypterNames[name]; exists { + errs = append( + errs, + field.Invalid(field.NewPath("spec", "decrypters", "["+strconv.Itoa(i)+"]"), decrypter, "decrypters must be unique"), + ) + + continue + } + + decrypterNames[name] = struct{}{} + } + + return errs +} diff --git a/pkg/registry/apis/secret/reststorage/secure_value_rest_test.go b/pkg/registry/apis/secret/reststorage/secure_value_rest_test.go new file mode 100644 index 00000000000..3002be94706 --- /dev/null +++ b/pkg/registry/apis/secret/reststorage/secure_value_rest_test.go @@ -0,0 +1,269 @@ +package reststorage + +import ( + "fmt" + "maps" + "slices" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apiserver/pkg/admission" + + secretv0alpha1 "github.com/grafana/grafana/pkg/apis/secret/v0alpha1" +) + +func TestValidateSecureValue(t *testing.T) { + t.Run("when creating a new securevalue", func(t *testing.T) { + validSecureValue := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Title: "title", + Value: "value", + Keeper: "keeper", + Decrypters: []string{"actor_app1", "actor_app2"}, + }, + } + + t.Run("the `title` must be present", func(t *testing.T) { + sv := validSecureValue.DeepCopy() + sv.Spec.Title = "" + + errs := ValidateSecureValue(sv, nil, admission.Create, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec.title", errs[0].Field) + }) + + t.Run("the `keeper` must be present", func(t *testing.T) { + sv := validSecureValue.DeepCopy() + sv.Spec.Keeper = "" + + errs := ValidateSecureValue(sv, nil, admission.Create, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec.keeper", errs[0].Field) + }) + + t.Run("either a `value` or `ref` must be present but not both", func(t *testing.T) { + sv := validSecureValue.DeepCopy() + sv.Spec.Value = "" + sv.Spec.Ref = "" + + errs := ValidateSecureValue(sv, nil, admission.Create, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec", errs[0].Field) + + sv.Spec.Value = "value" + sv.Spec.Ref = "value" + + errs = ValidateSecureValue(sv, nil, admission.Create, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec", errs[0].Field) + }) + }) + + t.Run("when updating a securevalue", func(t *testing.T) { + t.Run("when trying to switch from a `value` (old) to a `ref` (new), it returns an error", func(t *testing.T) { + oldSv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Ref: "", // empty `ref` means a `value` was present. + }, + } + + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Ref: "ref", + }, + } + + errs := ValidateSecureValue(sv, oldSv, admission.Update, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec", errs[0].Field) + }) + + t.Run("when trying to switch from a `ref` (old) to a `value` (new), it returns an error", func(t *testing.T) { + oldSv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Ref: "non-empty", + }, + } + + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Value: "value", + }, + } + + errs := ValidateSecureValue(sv, oldSv, admission.Update, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec", errs[0].Field) + }) + + t.Run("when both `value` and `ref` are set, it returns an error", func(t *testing.T) { + oldSv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Ref: "non-empty", + }, + } + + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Value: "value", + Ref: "ref", + }, + } + + errs := ValidateSecureValue(sv, oldSv, admission.Update, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec", errs[0].Field) + + oldSv = &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Value: "non-empty", + }, + } + + errs = ValidateSecureValue(sv, oldSv, admission.Update, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec", errs[0].Field) + }) + + t.Run("when no changes are made, it returns no errors", func(t *testing.T) { + oldSv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Title: "old-title", + }, + } + + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Title: "new-title", + }, + } + + errs := ValidateSecureValue(sv, oldSv, admission.Update, nil) + require.Empty(t, errs) + }) + + t.Run("when the old object is `nil` it returns an error", func(t *testing.T) { + sv := &secretv0alpha1.SecureValue{} + + errs := ValidateSecureValue(sv, nil, admission.Update, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec", errs[0].Field) + }) + + t.Run("when trying to change the `keeper`, it returns an error", func(t *testing.T) { + oldSv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Keeper: "a-keeper", + }, + } + + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Keeper: "another-keeper", + }, + } + + errs := ValidateSecureValue(sv, oldSv, admission.Update, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec", errs[0].Field) + }) + }) + + t.Run("`decrypters` must have unique items", func(t *testing.T) { + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Title: "title", Keeper: "keeper", Ref: "ref", + + Decrypters: []string{ + "actor_app1", + "actor_app1", + }, + }, + } + + errs := ValidateSecureValue(sv, nil, admission.Create, nil) + require.Len(t, errs, 1) + require.Equal(t, "spec.decrypters.[1]", errs[0].Field) + }) + + t.Run("`decrypters` must match the expected format", func(t *testing.T) { + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Title: "title", Keeper: "keeper", Ref: "ref", + + Decrypters: []string{ + "app1", + "_app1", + "actr_app1", + "actor_ ", + "actor_", + }, + }, + } + + errs := ValidateSecureValue(sv, nil, admission.Create, nil) + require.Len(t, errs, len(sv.Spec.Decrypters)) + + for i, err := range errs { + require.Equal(t, fmt.Sprintf("spec.decrypters.[%d]", i), err.Field) + require.Contains(t, err.Error(), "a decrypter must have the format `actor_{name}`") + } + }) + + t.Run("when set, the `decrypters` must be one of the allowed in the allow list", func(t *testing.T) { + allowList := map[string]struct{}{"actor_app1": {}, "actor_app2": {}} + decrypters := slices.Collect(maps.Keys(allowList)) + + t.Run("no matches, returns an error", func(t *testing.T) { + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Title: "title", Keeper: "keeper", Ref: "ref", + + Decrypters: []string{"actor_app3"}, + }, + } + + errs := ValidateSecureValue(sv, nil, admission.Create, allowList) + require.Len(t, errs, 1) + }) + + t.Run("no decrypters, returns no error", func(t *testing.T) { + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Title: "title", Keeper: "keeper", Ref: "ref", + + Decrypters: []string{}, + }, + } + + errs := ValidateSecureValue(sv, nil, admission.Create, allowList) + require.Empty(t, errs) + }) + + t.Run("one match, returns no errors", func(t *testing.T) { + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Title: "title", Keeper: "keeper", Ref: "ref", + + Decrypters: []string{decrypters[0]}, + }, + } + + errs := ValidateSecureValue(sv, nil, admission.Create, allowList) + require.Empty(t, errs) + }) + + t.Run("all matches, returns no errors", func(t *testing.T) { + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Title: "title", Keeper: "keeper", Ref: "ref", + + Decrypters: decrypters, + }, + } + + errs := ValidateSecureValue(sv, nil, admission.Create, allowList) + require.Empty(t, errs) + }) + }) +} diff --git a/pkg/registry/apis/secret/xkube/annotations.go b/pkg/registry/apis/secret/xkube/annotations.go new file mode 100644 index 00000000000..21e7cbbac74 --- /dev/null +++ b/pkg/registry/apis/secret/xkube/annotations.go @@ -0,0 +1,24 @@ +package xkube + +import "github.com/grafana/grafana/pkg/apimachinery/utils" + +var ( + // Exclude these annotations + skipAnnotations = map[string]bool{ + "kubectl.kubernetes.io/last-applied-configuration": true, // force server side apply + utils.AnnoKeyCreatedBy: true, + utils.AnnoKeyUpdatedBy: true, + utils.AnnoKeyUpdatedTimestamp: true, + } +) + +func CleanAnnotations(anno map[string]string) map[string]string { + copy := make(map[string]string) + for k, v := range anno { + if skipAnnotations[k] { + continue + } + copy[k] = v + } + return copy +} diff --git a/pkg/registry/apis/secret/xkube/errors.go b/pkg/registry/apis/secret/xkube/errors.go new file mode 100644 index 00000000000..3de10eeffcf --- /dev/null +++ b/pkg/registry/apis/secret/xkube/errors.go @@ -0,0 +1,9 @@ +package xkube + +import "k8s.io/apimachinery/pkg/util/validation/field" + +// ErrorLister is an interface compatible with errors that also returns a list of Kubernetes field errors. +type ErrorLister interface { + error + ErrorList() field.ErrorList +}