chore(fix): template abuse for secret values (#6730)

* chore(fix): template abuse for secret values

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

* addressed review comments

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

---------

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Co-authored-by: Jean-Philippe Evrard <jean-philippe.evrard+rochepub@external.roche.com>

apis/externalsecrets/v1/externalsecret_types.go

@@ -160,7 +160,8 @@ type TemplateFrom struct {
 	Secret    *TemplateRef `json:"secret,omitempty"`
 
 	// Target specifies where to place the template result.
-	// For Secret resources, common values are: "Data", "Annotations", "Labels".
+	// For Secret resources the accepted values are empty, "Data", "Annotations" and "Labels";
+	// any other value is rejected because it would allow writes to privileged Secret fields.
 	// For custom resources (when spec.target.manifest is set), this supports
 	// nested paths like "spec.database.config" or "data".
 	// +optional
@@ -186,7 +187,8 @@ const (
 	TemplateScopeKeysAndValues TemplateScope = "KeysAndValues"
 )
 
-// These constants are provided for convenience but Target accepts any string.
+// These constants are the only Target values accepted when the ExternalSecret renders
+// into a Secret. Custom resource targets additionally accept nested paths.
 const (
 	TemplateTargetData        = "Data"
 	TemplateTargetAnnotations = "Annotations"

apis/externalsecrets/v1/externalsecret_validator.go

@@ -65,6 +65,10 @@ func validateExternalSecret(es *ExternalSecret) (admission.Warnings, error) {
 		errs = errors.Join(errs, err)
 	}
 
+	if err := validateTemplateFromTarget(es); err != nil {
+		errs = errors.Join(errs, err)
+	}
+
 	for _, ref := range es.Spec.DataFrom {
 		if err := validateExtractFindGenerator(ref); err != nil {
 			errs = errors.Join(errs, err)
@@ -148,6 +152,55 @@ func validatePrivilegedTemplate(es *ExternalSecret) error {
 	return nil
 }
 
+// isManifestSecretTarget reports whether the ExternalSecret renders into a core/v1 Secret.
+// That is the default target, and it is also reachable through an explicit manifest
+// reference naming a Secret.
+func isManifestSecretTarget(es *ExternalSecret) bool {
+	manifest := es.Spec.Target.Manifest
+	if manifest == nil {
+		return true
+	}
+	return manifest.APIVersion == "v1" && manifest.Kind == "Secret"
+}
+
+// validateTemplateFromTarget restricts templateFrom targets whenever the ExternalSecret
+// renders into a Secret.
+func validateTemplateFromTarget(es *ExternalSecret) error {
+	if !isManifestSecretTarget(es) {
+		return nil
+	}
+
+	return ValidateSecretTemplateFromTargets(es.Spec.Target.Template)
+}
+
+// ValidateSecretTemplateFromTargets restricts templateFrom targets to the well-known Secret
+// fields. The templating engine treats any other value as a dotted path into the rendered
+// object, which would let a user write privileged top-level fields such as type, immutable
+// or metadata.ownerReferences and so sidestep validatePrivilegedTemplate. Nested paths
+// remain available for custom resource targets.
+func ValidateSecretTemplateFromTargets(tpl *ExternalSecretTemplate) error {
+	if tpl == nil {
+		return nil
+	}
+
+	var errs error
+	for _, tf := range tpl.TemplateFrom {
+		switch {
+		case tf.Target == "",
+			strings.EqualFold(tf.Target, TemplateTargetData),
+			strings.EqualFold(tf.Target, TemplateTargetAnnotations),
+			strings.EqualFold(tf.Target, TemplateTargetLabels):
+			continue
+		}
+
+		errs = errors.Join(errs, fmt.Errorf(
+			"templateFrom target=%q is not allowed when targeting a Secret, must be one of %q, %q or %q",
+			tf.Target, TemplateTargetData, TemplateTargetAnnotations, TemplateTargetLabels))
+	}
+
+	return errs
+}
+
 func validateDuplicateKeys(es *ExternalSecret, errs error) error {
 	if es.Spec.Target.DeletionPolicy == DeletionPolicyRetain {
 		seenKeys := make(map[string]struct{})

apis/externalsecrets/v1/externalsecret_validator_test.go

@@ -375,6 +375,179 @@ either data or dataFrom should be specified`,
 				},
 			},
 		},
+		{
+			name: "templateFrom type target is rejected",
+			obj: &ExternalSecret{
+				Spec: ExternalSecretSpec{
+					DataFrom: []ExternalSecretDataFromRemoteRef{
+						{
+							SourceRef: &StoreGeneratorSourceRef{
+								GeneratorRef: &GeneratorRef{},
+							},
+						},
+					},
+					Target: ExternalSecretTarget{
+						Template: &ExternalSecretTemplate{
+							Metadata: ExternalSecretTemplateMetadata{
+								Annotations: map[string]string{
+									corev1.ServiceAccountNameKey: "victim-sa",
+								},
+							},
+							TemplateFrom: []TemplateFrom{
+								{Target: "type"},
+							},
+						},
+					},
+				},
+			},
+			expectedErr: `templateFrom target="type" is not allowed when targeting a Secret, must be one of "Data", "Annotations" or "Labels"`,
+		},
+		{
+			name: "templateFrom nested metadata.annotations target is rejected",
+			obj: &ExternalSecret{
+				Spec: ExternalSecretSpec{
+					DataFrom: []ExternalSecretDataFromRemoteRef{
+						{
+							SourceRef: &StoreGeneratorSourceRef{
+								GeneratorRef: &GeneratorRef{},
+							},
+						},
+					},
+					Target: ExternalSecretTarget{
+						Template: &ExternalSecretTemplate{
+							Type: corev1.SecretTypeServiceAccountToken,
+							TemplateFrom: []TemplateFrom{
+								{Target: "metadata.annotations"},
+							},
+						},
+					},
+				},
+			},
+			expectedErr: `templateFrom target="metadata.annotations" is not allowed when targeting a Secret, must be one of "Data", "Annotations" or "Labels"`,
+		},
+		{
+			name: "templateFrom mixed case nested annotations target is rejected",
+			obj: &ExternalSecret{
+				Spec: ExternalSecretSpec{
+					DataFrom: []ExternalSecretDataFromRemoteRef{
+						{
+							SourceRef: &StoreGeneratorSourceRef{
+								GeneratorRef: &GeneratorRef{},
+							},
+						},
+					},
+					Target: ExternalSecretTarget{
+						Template: &ExternalSecretTemplate{
+							Type: corev1.SecretTypeServiceAccountToken,
+							TemplateFrom: []TemplateFrom{
+								{Target: "Metadata.Annotations.kubernetes.io/service-account.name"},
+							},
+						},
+					},
+				},
+			},
+			expectedErr: `templateFrom target="Metadata.Annotations.kubernetes.io/service-account.name" is not allowed when targeting a Secret, must be one of "Data", "Annotations" or "Labels"`,
+		},
+		{
+			name: "templateFrom immutable target is rejected",
+			obj: &ExternalSecret{
+				Spec: ExternalSecretSpec{
+					DataFrom: []ExternalSecretDataFromRemoteRef{
+						{
+							SourceRef: &StoreGeneratorSourceRef{
+								GeneratorRef: &GeneratorRef{},
+							},
+						},
+					},
+					Target: ExternalSecretTarget{
+						Template: &ExternalSecretTemplate{
+							TemplateFrom: []TemplateFrom{
+								{Target: "immutable"},
+							},
+						},
+					},
+				},
+			},
+			expectedErr: `templateFrom target="immutable" is not allowed when targeting a Secret, must be one of "Data", "Annotations" or "Labels"`,
+		},
+		{
+			name: "templateFrom well-known targets are allowed in any case",
+			obj: &ExternalSecret{
+				Spec: ExternalSecretSpec{
+					DataFrom: []ExternalSecretDataFromRemoteRef{
+						{
+							SourceRef: &StoreGeneratorSourceRef{
+								GeneratorRef: &GeneratorRef{},
+							},
+						},
+					},
+					Target: ExternalSecretTarget{
+						Template: &ExternalSecretTemplate{
+							TemplateFrom: []TemplateFrom{
+								{Target: ""},
+								{Target: "data"},
+								{Target: TemplateTargetData},
+								{Target: "labels"},
+								{Target: TemplateTargetLabels},
+								{Target: "annotations"},
+								{Target: TemplateTargetAnnotations},
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "templateFrom nested target is allowed for a custom resource manifest",
+			obj: &ExternalSecret{
+				Spec: ExternalSecretSpec{
+					DataFrom: []ExternalSecretDataFromRemoteRef{
+						{
+							SourceRef: &StoreGeneratorSourceRef{
+								GeneratorRef: &GeneratorRef{},
+							},
+						},
+					},
+					Target: ExternalSecretTarget{
+						Manifest: &ManifestReference{
+							APIVersion: "argoproj.io/v1alpha1",
+							Kind:       "Application",
+						},
+						Template: &ExternalSecretTemplate{
+							TemplateFrom: []TemplateFrom{
+								{Target: "spec.database.config"},
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "templateFrom nested target is rejected for a manifest naming a Secret",
+			obj: &ExternalSecret{
+				Spec: ExternalSecretSpec{
+					DataFrom: []ExternalSecretDataFromRemoteRef{
+						{
+							SourceRef: &StoreGeneratorSourceRef{
+								GeneratorRef: &GeneratorRef{},
+							},
+						},
+					},
+					Target: ExternalSecretTarget{
+						Manifest: &ManifestReference{
+							APIVersion: "v1",
+							Kind:       "Secret",
+						},
+						Template: &ExternalSecretTemplate{
+							TemplateFrom: []TemplateFrom{
+								{Target: "type"},
+							},
+						},
+					},
+				},
+			},
+			expectedErr: `templateFrom target="type" is not allowed when targeting a Secret, must be one of "Data", "Annotations" or "Labels"`,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

apis/externalsecrets/v1beta1/externalsecret_validator.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -135,7 +136,7 @@ func validatePrivilegedTemplate(es *ExternalSecret) error {
 			return fmt.Errorf("template.type=%q with annotation %q is not allowed", corev1.SecretTypeServiceAccountToken, corev1.ServiceAccountNameKey)
 		}
 		for _, tf := range tpl.TemplateFrom {
-			if tf.Target == TemplateTargetAnnotations {
+			if strings.EqualFold(string(tf.Target), string(TemplateTargetAnnotations)) {
 				return fmt.Errorf("template.type=%q with templateFrom target=%q is not allowed", corev1.SecretTypeServiceAccountToken, TemplateTargetAnnotations)
 			}
 		}

apis/externalsecrets/v1beta1/externalsecret_validator_test.go

@@ -271,6 +271,29 @@ either data or dataFrom should be specified`,
 			},
 			expectedErr: `template.type="kubernetes.io/service-account-token" with templateFrom target="Annotations" is not allowed`,
 		},
+		{
+			name: "service account token template with lowercase templateFrom annotations target is rejected",
+			obj: &ExternalSecret{
+				Spec: ExternalSecretSpec{
+					DataFrom: []ExternalSecretDataFromRemoteRef{
+						{
+							SourceRef: &StoreGeneratorSourceRef{
+								GeneratorRef: &GeneratorRef{},
+							},
+						},
+					},
+					Target: ExternalSecretTarget{
+						Template: &ExternalSecretTemplate{
+							Type: corev1.SecretTypeServiceAccountToken,
+							TemplateFrom: []TemplateFrom{
+								{Target: "annotations"},
+							},
+						},
+					},
+				},
+			},
+			expectedErr: `template.type="kubernetes.io/service-account-token" with templateFrom target="Annotations" is not allowed`,
+		},
 		{
 			name: "service account token template with templateFrom data target is allowed",
 			obj: &ExternalSecret{

config/crds/bases/external-secrets.io_clusterexternalsecrets.yaml

@@ -771,7 +771,8 @@ spec:
                                   default: Data
                                   description: |-
                                     Target specifies where to place the template result.
-                                    For Secret resources, common values are: "Data", "Annotations", "Labels".
+                                    For Secret resources the accepted values are empty, "Data", "Annotations" and "Labels";
+                                    any other value is rejected because it would allow writes to privileged Secret fields.
                                     For custom resources (when spec.target.manifest is set), this supports
                                     nested paths like "spec.database.config" or "data".
                                   type: string

config/crds/bases/external-secrets.io_clusterpushsecrets.yaml

@@ -649,7 +649,8 @@ spec:
                               default: Data
                               description: |-
                                 Target specifies where to place the template result.
-                                For Secret resources, common values are: "Data", "Annotations", "Labels".
+                                For Secret resources the accepted values are empty, "Data", "Annotations" and "Labels";
+                                any other value is rejected because it would allow writes to privileged Secret fields.
                                 For custom resources (when spec.target.manifest is set), this supports
                                 nested paths like "spec.database.config" or "data".
                               type: string

config/crds/bases/external-secrets.io_externalsecrets.yaml

@@ -750,7 +750,8 @@ spec:
                               default: Data
                               description: |-
                                 Target specifies where to place the template result.
-                                For Secret resources, common values are: "Data", "Annotations", "Labels".
+                                For Secret resources the accepted values are empty, "Data", "Annotations" and "Labels";
+                                any other value is rejected because it would allow writes to privileged Secret fields.
                                 For custom resources (when spec.target.manifest is set), this supports
                                 nested paths like "spec.database.config" or "data".
                               type: string

config/crds/bases/external-secrets.io_pushsecrets.yaml

@@ -571,7 +571,8 @@ spec:
                           default: Data
                           description: |-
                             Target specifies where to place the template result.
-                            For Secret resources, common values are: "Data", "Annotations", "Labels".
+                            For Secret resources the accepted values are empty, "Data", "Annotations" and "Labels";
+                            any other value is rejected because it would allow writes to privileged Secret fields.
                             For custom resources (when spec.target.manifest is set), this supports
                             nested paths like "spec.database.config" or "data".
                           type: string

deploy/crds/bundle.yaml

@@ -721,7 +721,8 @@ spec:
                                     default: Data
                                     description: |-
                                       Target specifies where to place the template result.
-                                      For Secret resources, common values are: "Data", "Annotations", "Labels".
+                                      For Secret resources the accepted values are empty, "Data", "Annotations" and "Labels";
+                                      any other value is rejected because it would allow writes to privileged Secret fields.
                                       For custom resources (when spec.target.manifest is set), this supports
                                       nested paths like "spec.database.config" or "data".
                                     type: string
@@ -2240,7 +2241,8 @@ spec:
                                 default: Data
                                 description: |-
                                   Target specifies where to place the template result.
-                                  For Secret resources, common values are: "Data", "Annotations", "Labels".
+                                  For Secret resources the accepted values are empty, "Data", "Annotations" and "Labels";
+                                  any other value is rejected because it would allow writes to privileged Secret fields.
                                   For custom resources (when spec.target.manifest is set), this supports
                                   nested paths like "spec.database.config" or "data".
                                 type: string
@@ -14180,7 +14182,8 @@ spec:
                                 default: Data
                                 description: |-
                                   Target specifies where to place the template result.
-                                  For Secret resources, common values are: "Data", "Annotations", "Labels".
+                                  For Secret resources the accepted values are empty, "Data", "Annotations" and "Labels";
+                                  any other value is rejected because it would allow writes to privileged Secret fields.
                                   For custom resources (when spec.target.manifest is set), this supports
                                   nested paths like "spec.database.config" or "data".
                                 type: string
@@ -15412,7 +15415,8 @@ spec:
                             default: Data
                             description: |-
                               Target specifies where to place the template result.
-                              For Secret resources, common values are: "Data", "Annotations", "Labels".
+                              For Secret resources the accepted values are empty, "Data", "Annotations" and "Labels";
+                              any other value is rejected because it would allow writes to privileged Secret fields.
                               For custom resources (when spec.target.manifest is set), this supports
                               nested paths like "spec.database.config" or "data".
                             type: string

docs/api/spec.md

@@ -12157,7 +12157,8 @@ string
 <td>
 <em>(Optional)</em>
 <p>Target specifies where to place the template result.
-For Secret resources, common values are: &ldquo;Data&rdquo;, &ldquo;Annotations&rdquo;, &ldquo;Labels&rdquo;.
+For Secret resources the accepted values are empty, &ldquo;Data&rdquo;, &ldquo;Annotations&rdquo; and &ldquo;Labels&rdquo;;
+any other value is rejected because it would allow writes to privileged Secret fields.
 For custom resources (when spec.target.manifest is set), this supports
 nested paths like &ldquo;spec.database.config&rdquo; or &ldquo;data&rdquo;.</p>
 </td>

pkg/controllers/externalsecret/externalsecret_controller_template.go

@@ -56,6 +56,12 @@ func (r *Reconciler) ApplyTemplate(ctx context.Context, es *esv1.ExternalSecret,
 		return nil
 	}
 
+	// defense in depth: the admission webhook rejects these targets already, but a cluster
+	// running with failurePolicy=Ignore or without the webhook must not render them either.
+	if err := esv1.ValidateSecretTemplateFromTargets(es.Spec.Target.Template); err != nil {
+		return err
+	}
+
 	// set the secret type if it is defined in the template, otherwise keep the existing type
 	if es.Spec.Target.Template.Type != "" {
 		secret.Type = es.Spec.Target.Template.Type

pkg/controllers/externalsecret/externalsecret_controller_template_test.go

@@ -0,0 +1,118 @@
+/*
+Copyright © The ESO Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package externalsecret
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/scheme"
+	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
+)
+
+func TestApplyTemplateRejectsPathStyleTemplateFromTarget(t *testing.T) {
+	tests := []struct {
+		name   string
+		target string
+	}{
+		{name: "type", target: "type"},
+		{name: "nested annotations path", target: "metadata.annotations"},
+		{name: "mixed case nested annotations path", target: "Metadata.Annotations"},
+		{name: "immutable", target: "immutable"},
+		{name: "owner references", target: "metadata.ownerReferences"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_ = esv1.AddToScheme(scheme.Scheme)
+			r := &Reconciler{
+				Client: fakeclient.NewClientBuilder().WithScheme(scheme.Scheme).Build(),
+				Scheme: scheme.Scheme,
+			}
+
+			literal := "kubernetes.io/service-account-token"
+			es := &esv1.ExternalSecret{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-es", Namespace: "default"},
+				Spec: esv1.ExternalSecretSpec{
+					Target: esv1.ExternalSecretTarget{
+						Name: "test-secret",
+						Template: &esv1.ExternalSecretTemplate{
+							EngineVersion: esv1.TemplateEngineV2,
+							Metadata: esv1.ExternalSecretTemplateMetadata{
+								Annotations: map[string]string{
+									v1.ServiceAccountNameKey: "victim-sa",
+								},
+							},
+							TemplateFrom: []esv1.TemplateFrom{
+								{Literal: &literal, Target: tt.target},
+							},
+						},
+					},
+				},
+			}
+
+			secret := &v1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-secret", Namespace: "default"},
+			}
+
+			err := r.ApplyTemplate(context.Background(), es, secret, map[string][]byte{})
+
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "is not allowed when targeting a Secret")
+			assert.NotEqual(t, v1.SecretTypeServiceAccountToken, secret.Type)
+		})
+	}
+}
+
+func TestApplyTemplateAllowsWellKnownTemplateFromTargets(t *testing.T) {
+	_ = esv1.AddToScheme(scheme.Scheme)
+	r := &Reconciler{
+		Client: fakeclient.NewClientBuilder().WithScheme(scheme.Scheme).Build(),
+		Scheme: scheme.Scheme,
+	}
+
+	dataLiteral := "greeting: hello"
+	annotationLiteral := "team: platform"
+	es := &esv1.ExternalSecret{
+		ObjectMeta: metav1.ObjectMeta{Name: "test-es", Namespace: "default"},
+		Spec: esv1.ExternalSecretSpec{
+			Target: esv1.ExternalSecretTarget{
+				Name: "test-secret",
+				Template: &esv1.ExternalSecretTemplate{
+					EngineVersion: esv1.TemplateEngineV2,
+					TemplateFrom: []esv1.TemplateFrom{
+						{Literal: &dataLiteral, Target: "data"},
+						{Literal: &annotationLiteral, Target: esv1.TemplateTargetAnnotations},
+					},
+				},
+			},
+		},
+	}
+
+	secret := &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{Name: "test-secret", Namespace: "default"},
+	}
+
+	require.NoError(t, r.ApplyTemplate(context.Background(), es, secret, map[string][]byte{}))
+	assert.Equal(t, []byte("hello"), secret.Data["greeting"])
+	assert.Equal(t, "platform", secret.Annotations["team"])
+}