From 537ac8ec6899a558840195ec8c0285d8436b6c1a Mon Sep 17 00:00:00 2001 From: Matheus Macabu Date: Tue, 19 Aug 2025 16:55:52 +0200 Subject: [PATCH] Secrets: Validate name/namespace with standard K8s validator (#109868) * Secrets: Validate name/namespace with standard K8s validator * Secrets: Simplify error message for mismatched owner inline secure values --- .../apis/secret/contracts/secure_value.go | 2 +- .../apis/secret/inline/inline_secure_value.go | 5 +- pkg/registry/apis/secret/validator/keeper.go | 23 +++++- .../apis/secret/validator/keeper_test.go | 74 +++++++++++++++++++ .../apis/secret/validator/secure_value.go | 28 ++++--- .../secret/validator/secure_value_test.go | 56 +++++++++++++- 6 files changed, 169 insertions(+), 19 deletions(-) diff --git a/pkg/registry/apis/secret/contracts/secure_value.go b/pkg/registry/apis/secret/contracts/secure_value.go index fa1eae74133..2427430d082 100644 --- a/pkg/registry/apis/secret/contracts/secure_value.go +++ b/pkg/registry/apis/secret/contracts/secure_value.go @@ -11,7 +11,7 @@ import ( ) // The maximum size of a secure value in bytes when written as raw input. -const SECURE_VALUE_RAW_INPUT_MAX_SIZE_BYTES = 24576 // 24 KiB +const SecureValueRawInputMaxSizeBytes = 24576 // 24 KiB type DecryptSecureValue struct { Keeper *string diff --git a/pkg/registry/apis/secret/inline/inline_secure_value.go b/pkg/registry/apis/secret/inline/inline_secure_value.go index 08ce9f3068b..8ad810532c4 100644 --- a/pkg/registry/apis/secret/inline/inline_secure_value.go +++ b/pkg/registry/apis/secret/inline/inline_secure_value.go @@ -118,10 +118,7 @@ func (s *LocalInlineSecureValueService) isSecureValueOwnedByResource(ctx context return true, nil // The secure value is owned by the same owner reference, pass! } - return false, fmt.Errorf( - "secure value %s is not owned by %s/%s/%s/%s but by %s/%s/%s", - name, owner.APIGroup, owner.APIVersion, owner.Kind, owner.Name, actualOwner.APIVersion, actualOwner.Kind, actualOwner.Name, - ) + return false, fmt.Errorf("secure value %s is not owned by %s/%s/%s/%s", name, owner.APIGroup, owner.APIVersion, owner.Kind, owner.Name) } // not owned diff --git a/pkg/registry/apis/secret/validator/keeper.go b/pkg/registry/apis/secret/validator/keeper.go index 64afa94821a..737ef355794 100644 --- a/pkg/registry/apis/secret/validator/keeper.go +++ b/pkg/registry/apis/secret/validator/keeper.go @@ -3,6 +3,7 @@ package validator import ( "strings" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/admission" @@ -19,12 +20,26 @@ func ProvideKeeperValidator() contracts.KeeperValidator { } func (v *keeperValidator) Validate(keeper *secretv1beta1.Keeper, oldKeeper *secretv1beta1.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) + + // General validations. + if err := validation.IsDNS1123Subdomain(keeper.Name); len(err) > 0 { + errs = append( + errs, + field.Invalid(field.NewPath("metadata", "name"), keeper.Name, strings.Join(err, ",")), + ) + } + if err := validation.IsDNS1123Subdomain(keeper.Namespace); len(err) > 0 { + errs = append( + errs, + field.Invalid(field.NewPath("metadata", "namespace"), keeper.Name, strings.Join(err, ",")), + ) } - errs := make(field.ErrorList, 0) + // Only validate Create and Update for now. + if operation != admission.Create && operation != admission.Update { + return errs + } if keeper.Spec.Description == "" { errs = append(errs, field.Required(field.NewPath("spec", "description"), "a `description` is required")) diff --git a/pkg/registry/apis/secret/validator/keeper_test.go b/pkg/registry/apis/secret/validator/keeper_test.go index 0722ed81c40..1dcb51b3e31 100644 --- a/pkg/registry/apis/secret/validator/keeper_test.go +++ b/pkg/registry/apis/secret/validator/keeper_test.go @@ -1,9 +1,11 @@ package validator import ( + "strings" "testing" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/admission" "k8s.io/utils/ptr" @@ -11,11 +13,13 @@ import ( ) func TestValidateKeeper(t *testing.T) { + objectMeta := metav1.ObjectMeta{Name: "test", Namespace: "test"} validator := ProvideKeeperValidator() t.Run("when creating a new keeper", func(t *testing.T) { t.Run("the `description` must be present", func(t *testing.T) { keeper := &secretv1beta1.Keeper{ + ObjectMeta: objectMeta, Spec: secretv1beta1.KeeperSpec{ Aws: &secretv1beta1.KeeperAWSConfig{ AccessKeyID: secretv1beta1.KeeperCredentialValue{ValueFromEnv: "some-value"}, @@ -33,6 +37,7 @@ func TestValidateKeeper(t *testing.T) { t.Run("only one `keeper` must be present", func(t *testing.T) { keeper := &secretv1beta1.Keeper{ + ObjectMeta: objectMeta, Spec: secretv1beta1.KeeperSpec{ Description: "short description", Aws: &secretv1beta1.KeeperAWSConfig{}, @@ -49,6 +54,7 @@ func TestValidateKeeper(t *testing.T) { t.Run("at least one `keeper` must be present", func(t *testing.T) { keeper := &secretv1beta1.Keeper{ + ObjectMeta: objectMeta, Spec: secretv1beta1.KeeperSpec{ Description: "description", }, @@ -61,6 +67,7 @@ func TestValidateKeeper(t *testing.T) { t.Run("aws keeper validation", func(t *testing.T) { validKeeperAWS := &secretv1beta1.Keeper{ + ObjectMeta: objectMeta, Spec: secretv1beta1.KeeperSpec{ Description: "description", Aws: &secretv1beta1.KeeperAWSConfig{ @@ -126,6 +133,7 @@ func TestValidateKeeper(t *testing.T) { t.Run("azure keeper validation", func(t *testing.T) { validKeeperAzure := &secretv1beta1.Keeper{ + ObjectMeta: objectMeta, Spec: secretv1beta1.KeeperSpec{ Description: "description", Azure: &secretv1beta1.KeeperAzureConfig{ @@ -193,6 +201,7 @@ func TestValidateKeeper(t *testing.T) { t.Run("gcp keeper validation", func(t *testing.T) { validKeeperGCP := &secretv1beta1.Keeper{ + ObjectMeta: objectMeta, Spec: secretv1beta1.KeeperSpec{ Description: "description", Gcp: &secretv1beta1.KeeperGCPConfig{ @@ -223,6 +232,7 @@ func TestValidateKeeper(t *testing.T) { t.Run("hashicorp keeper validation", func(t *testing.T) { validKeeperHashiCorp := &secretv1beta1.Keeper{ + ObjectMeta: objectMeta, Spec: secretv1beta1.KeeperSpec{ Description: "description", HashiCorpVault: &secretv1beta1.KeeperHashiCorpConfig{ @@ -267,4 +277,68 @@ func TestValidateKeeper(t *testing.T) { }) }) }) + + t.Run("invalid name", func(t *testing.T) { + keeper := &secretv1beta1.Keeper{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: objectMeta.Namespace, + }, + Spec: secretv1beta1.KeeperSpec{ + Description: "description", + HashiCorpVault: &secretv1beta1.KeeperHashiCorpConfig{ + Address: "http://address", + Token: secretv1beta1.KeeperCredentialValue{ + ValueFromConfig: "config.path.value", + }, + }, + }, + } + + keeper.Name = "" + errs := validator.Validate(keeper, nil, admission.Delete) + require.Len(t, errs, 1) + require.Equal(t, "metadata.name", errs[0].Field) + + keeper.Name = "invalid/name-" + errs = validator.Validate(keeper, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.name", errs[0].Field) + + keeper.Name = strings.Repeat("a", 253+1) + errs = validator.Validate(keeper, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.name", errs[0].Field) + }) + + t.Run("invalid namespace", func(t *testing.T) { + keeper := &secretv1beta1.Keeper{ + ObjectMeta: metav1.ObjectMeta{ + Name: objectMeta.Name, + }, + Spec: secretv1beta1.KeeperSpec{ + Description: "description", + HashiCorpVault: &secretv1beta1.KeeperHashiCorpConfig{ + Address: "http://address", + Token: secretv1beta1.KeeperCredentialValue{ + ValueFromConfig: "config.path.value", + }, + }, + }, + } + + keeper.Namespace = "" + errs := validator.Validate(keeper, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.namespace", errs[0].Field) + + keeper.Namespace = "invalid/namespace-" + errs = validator.Validate(keeper, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.namespace", errs[0].Field) + + keeper.Namespace = strings.Repeat("a", 253+1) + errs = validator.Validate(keeper, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.namespace", errs[0].Field) + }) } diff --git a/pkg/registry/apis/secret/validator/secure_value.go b/pkg/registry/apis/secret/validator/secure_value.go index 17445b984df..0df885bff08 100644 --- a/pkg/registry/apis/secret/validator/secure_value.go +++ b/pkg/registry/apis/secret/validator/secure_value.go @@ -38,17 +38,27 @@ func (v *secureValueValidator) Validate(sv, oldSv *secretv1beta1.SecureValue, op } // General validations. - if sv.Name == "" { - errs = append(errs, field.Required(field.NewPath("metadata", "name"), "a `name` is required")) - } - if sv.Namespace == "" { - errs = append(errs, field.Required(field.NewPath("metadata", "namespace"), "a `namespace` is required")) - } - - if sv.Spec.Value != nil && len(*sv.Spec.Value) > contracts.SECURE_VALUE_RAW_INPUT_MAX_SIZE_BYTES { + if err := validation.IsDNS1123Subdomain(sv.Name); len(err) > 0 { errs = append( errs, - field.TooLong(field.NewPath("spec", "value"), len(*sv.Spec.Value), contracts.SECURE_VALUE_RAW_INPUT_MAX_SIZE_BYTES), + field.Invalid(field.NewPath("metadata", "name"), sv.Name, strings.Join(err, ",")), + ) + + return errs + } + if err := validation.IsDNS1123Subdomain(sv.Namespace); len(err) > 0 { + errs = append( + errs, + field.Invalid(field.NewPath("metadata", "namespace"), sv.Name, strings.Join(err, ",")), + ) + + return errs + } + + if sv.Spec.Value != nil && len(*sv.Spec.Value) > contracts.SecureValueRawInputMaxSizeBytes { + errs = append( + errs, + field.TooLong(field.NewPath("spec", "value"), len(*sv.Spec.Value), contracts.SecureValueRawInputMaxSizeBytes), ) } diff --git a/pkg/registry/apis/secret/validator/secure_value_test.go b/pkg/registry/apis/secret/validator/secure_value_test.go index bd977c6c2e0..88a6220db54 100644 --- a/pkg/registry/apis/secret/validator/secure_value_test.go +++ b/pkg/registry/apis/secret/validator/secure_value_test.go @@ -69,7 +69,7 @@ func TestValidateSecureValue(t *testing.T) { t.Run("`value` cannot exceed 24576 bytes", func(t *testing.T) { sv := validSecureValue.DeepCopy() - sv.Spec.Value = ptr.To(secretv1beta1.NewExposedSecureValue(strings.Repeat("a", contracts.SECURE_VALUE_RAW_INPUT_MAX_SIZE_BYTES+1))) + sv.Spec.Value = ptr.To(secretv1beta1.NewExposedSecureValue(strings.Repeat("a", contracts.SecureValueRawInputMaxSizeBytes+1))) sv.Spec.Ref = nil errs := validator.Validate(sv, nil, admission.Create) @@ -288,4 +288,58 @@ func TestValidateSecureValue(t *testing.T) { require.Len(t, errs, 1) require.Equal(t, "spec.decrypters", errs[0].Field) }) + + t.Run("invalid name", func(t *testing.T) { + sv := &secretv1beta1.SecureValue{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: objectMeta.Namespace, + }, + Spec: secretv1beta1.SecureValueSpec{ + Description: "description", + Ref: ptr.To("ref"), + }, + } + + sv.Name = "" + errs := validator.Validate(sv, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.name", errs[0].Field) + + sv.Name = "invalid/name-" + errs = validator.Validate(sv, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.name", errs[0].Field) + + sv.Name = strings.Repeat("a", 253+1) + errs = validator.Validate(sv, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.name", errs[0].Field) + }) + + t.Run("invalid namespace", func(t *testing.T) { + sv := &secretv1beta1.SecureValue{ + ObjectMeta: metav1.ObjectMeta{ + Name: objectMeta.Name, + }, + Spec: secretv1beta1.SecureValueSpec{ + Description: "description", + Ref: ptr.To("ref"), + }, + } + + sv.Namespace = "" + errs := validator.Validate(sv, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.namespace", errs[0].Field) + + sv.Namespace = "invalid/namespace-" + errs = validator.Validate(sv, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.namespace", errs[0].Field) + + sv.Namespace = strings.Repeat("a", 253+1) + errs = validator.Validate(sv, nil, admission.Create) + require.Len(t, errs, 1) + require.Equal(t, "metadata.namespace", errs[0].Field) + }) }