From b46e305cb2f22f67f962ba24c73aeac41801dcba Mon Sep 17 00:00:00 2001 From: Dana Axinte <53751979+dana-axinte@users.noreply.github.com> Date: Tue, 1 Jul 2025 08:40:26 +0100 Subject: [PATCH] SecureValues: Remove actor prefix from decrypters (#107433) Co-authored-by: Matheus Macabu --- .../secret/reststorage/secure_value_rest.go | 32 +++++++--- .../reststorage/secure_value_rest_test.go | 64 ++++++++++--------- .../secure-value-default-generate.yaml | 4 +- .../testdata/secure-value-generate.yaml | 4 +- 4 files changed, 59 insertions(+), 45 deletions(-) diff --git a/pkg/registry/apis/secret/reststorage/secure_value_rest.go b/pkg/registry/apis/secret/reststorage/secure_value_rest.go index 0aed12aac53..7ea856509ea 100644 --- a/pkg/registry/apis/secret/reststorage/secure_value_rest.go +++ b/pkg/registry/apis/secret/reststorage/secure_value_rest.go @@ -13,6 +13,7 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/endpoints/request" @@ -308,7 +309,7 @@ func validateSecureValueUpdate(sv, oldSv *secretv0alpha1.SecureValue) field.Erro return errs } -// validateDecrypters validates that (if populated) the `decrypters` must match "actor_{name}" and must be unique. +// validateDecrypters validates that (if populated) the `decrypters` must be unique. func validateDecrypters(decrypters []string, decryptersAllowList map[string]struct{}) field.ErrorList { errs := make(field.ErrorList, 0) @@ -326,8 +327,17 @@ func validateDecrypters(decrypters []string, decryptersAllowList map[string]stru decrypterNames := make(map[string]struct{}, 0) for i, decrypter := range decrypters { + decrypter = strings.TrimSpace(decrypter) + if decrypter == "" { + errs = append( + errs, + field.Invalid(field.NewPath("spec", "decrypters", "["+strconv.Itoa(i)+"]"), decrypter, "decrypters cannot be empty if specified"), + ) + + continue + } + // 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( @@ -341,17 +351,19 @@ func validateDecrypters(decrypters []string, decryptersAllowList map[string]stru 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}`"), - ) + // Use the same validation as labels for the decrypters. + if verrs := validation.IsValidLabelValue(decrypter); len(verrs) > 0 { + for _, verr := range verrs { + errs = append( + errs, + field.Invalid(field.NewPath("spec", "decrypters", "["+strconv.Itoa(i)+"]"), decrypter, verr), + ) + } continue } - if _, exists := decrypterNames[name]; exists { + if _, exists := decrypterNames[decrypter]; exists { errs = append( errs, field.Invalid(field.NewPath("spec", "decrypters", "["+strconv.Itoa(i)+"]"), decrypter, "decrypters must be unique"), @@ -360,7 +372,7 @@ func validateDecrypters(decrypters []string, decryptersAllowList map[string]stru continue } - decrypterNames[name] = struct{}{} + decrypterNames[decrypter] = 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 index 1cde8d63816..1656ca7a1fc 100644 --- a/pkg/registry/apis/secret/reststorage/secure_value_rest_test.go +++ b/pkg/registry/apis/secret/reststorage/secure_value_rest_test.go @@ -22,7 +22,7 @@ func TestValidateSecureValue(t *testing.T) { Description: "description", Value: "value", Keeper: &keeper, - Decrypters: []string{"actor_app1", "actor_app2"}, + Decrypters: []string{"app1", "app2"}, }, } @@ -187,8 +187,8 @@ func TestValidateSecureValue(t *testing.T) { Description: "description", Ref: &ref, Decrypters: []string{ - "actor_app1", - "actor_app1", + "app1", + "app1", }, }, } @@ -198,33 +198,8 @@ func TestValidateSecureValue(t *testing.T) { require.Equal(t, "spec.decrypters.[1]", errs[0].Field) }) - t.Run("`decrypters` must match the expected format", func(t *testing.T) { - ref := "ref" - sv := &secretv0alpha1.SecureValue{ - Spec: secretv0alpha1.SecureValueSpec{ - Description: "description", 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": {}} + allowList := map[string]struct{}{"app1": {}, "app2": {}} decrypters := slices.Collect(maps.Keys(allowList)) t.Run("no matches, returns an error", func(t *testing.T) { @@ -233,7 +208,7 @@ func TestValidateSecureValue(t *testing.T) { Spec: secretv0alpha1.SecureValueSpec{ Description: "description", Ref: &ref, - Decrypters: []string{"actor_app3"}, + Decrypters: []string{"app3"}, }, } @@ -284,10 +259,37 @@ func TestValidateSecureValue(t *testing.T) { }) }) + t.Run("`decrypters` must be a valid label value", func(t *testing.T) { + decrypters := []string{ + "", // invalid + "is/this/valid", // invalid + "is this valid", // invalid + "is.this.valid", + "is-this-valid", + "is_this_valid", + "0isthisvalid9", + "isthisvalid9", + "0isthisvalid", + "isthisvalid", + } + + ref := "ref" + sv := &secretv0alpha1.SecureValue{ + Spec: secretv0alpha1.SecureValueSpec{ + Description: "description", Ref: &ref, + + Decrypters: decrypters, + }, + } + + errs := ValidateSecureValue(sv, nil, admission.Create, nil) + require.Len(t, errs, 3) + }) + t.Run("`decrypters` cannot have more than 64 items", func(t *testing.T) { decrypters := make([]string, 0, 64+1) for i := 0; i < 64+1; i++ { - decrypters = append(decrypters, fmt.Sprintf("actor_app%d", i)) + decrypters = append(decrypters, fmt.Sprintf("app%d", i)) } ref := "ref" diff --git a/pkg/tests/apis/secret/testdata/secure-value-default-generate.yaml b/pkg/tests/apis/secret/testdata/secure-value-default-generate.yaml index 5bc9a368723..dec2cd8611b 100644 --- a/pkg/tests/apis/secret/testdata/secure-value-default-generate.yaml +++ b/pkg/tests/apis/secret/testdata/secure-value-default-generate.yaml @@ -11,5 +11,5 @@ spec: description: This is a secret value: this is super duper secure decrypters: - - actor_k6 - - actor_synthetic-monitoring + - k6 + - synthetic-monitoring diff --git a/pkg/tests/apis/secret/testdata/secure-value-generate.yaml b/pkg/tests/apis/secret/testdata/secure-value-generate.yaml index 2743a32650b..158052350c9 100644 --- a/pkg/tests/apis/secret/testdata/secure-value-generate.yaml +++ b/pkg/tests/apis/secret/testdata/secure-value-generate.yaml @@ -12,5 +12,5 @@ spec: keeper: my-keeper-1 value: super duper secure decrypters: - - actor_k6 - - actor_synthetic-monitoring + - k6 + - synthetic-monitoring