chore(fix): moved the validation further up the chain (#6735)

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

apis/externalsecrets/v1/externalsecret_validator.go

@@ -61,7 +61,7 @@ func validateExternalSecret(es *ExternalSecret) (admission.Warnings, error) {
 		errs = errors.Join(errs, errors.New("either data or dataFrom should be specified"))
 	}
 
-	if err := validatePrivilegedTemplate(es); err != nil {
+	if err := validatePrivilegedTemplate(es.Spec.Target.Template); err != nil {
 		errs = errors.Join(errs, err)
 	}
 
@@ -130,8 +130,7 @@ func validatePolicies(es *ExternalSecret) error {
 
 // validatePrivilegedTemplate rejects templates with specific types and annotations combinations
 // to prevent users from creating long-lived tokens beyond the scope of the defined RBAC.
-func validatePrivilegedTemplate(es *ExternalSecret) error {
-	tpl := es.Spec.Target.Template
+func validatePrivilegedTemplate(tpl *ExternalSecretTemplate) error {
 	if tpl == nil {
 		return nil
 	}
@@ -152,6 +151,17 @@ func validatePrivilegedTemplate(es *ExternalSecret) error {
 	return nil
 }
 
+// ValidateSecretTemplate applies every template restriction that must hold when an
+// ExternalSecret renders into a Secret. The admission webhook reaches these rules through
+// validateExternalSecret; the controller calls this so the same set is enforced when no
+// webhook sits in front of it.
+func ValidateSecretTemplate(tpl *ExternalSecretTemplate) error {
+	return errors.Join(
+		validatePrivilegedTemplate(tpl),
+		ValidateSecretTemplateFromTargets(tpl),
+	)
+}
+
 // 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.

pkg/controllers/externalsecret/externalsecret_controller_template.go

@@ -38,6 +38,13 @@ import (
 // * secret via es.data or es.dataFrom (if template.MergePolicy is Merge, or there is no template)
 // * existing secret keys (if CreationPolicy is Merge or CreateOrMerge).
 func (r *Reconciler) ApplyTemplate(ctx context.Context, es *esv1.ExternalSecret, secret *v1.Secret, dataMap map[string][]byte) error {
+	// the admission webhook rejects these templates already, but a cluster
+	// running with failurePolicy=Ignore or without the webhook must not render them either.
+	// this runs before any mutation, so a rejected template leaves the secret untouched.
+	if err := esv1.ValidateSecretTemplate(es.Spec.Target.Template); err != nil {
+		return err
+	}
+
 	// update metadata (labels, annotations, finalizers) of the secret
 	if err := setMetadata(secret, es); err != nil {
 		return err
@@ -56,12 +63,6 @@ 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

@@ -79,6 +79,78 @@ func TestApplyTemplateRejectsPathStyleTemplateFromTarget(t *testing.T) {
 			require.Error(t, err)
 			assert.Contains(t, err.Error(), "is not allowed when targeting a Secret")
 			assert.NotEqual(t, v1.SecretTypeServiceAccountToken, secret.Type)
+			assert.NotContains(t, secret.Annotations, v1.ServiceAccountNameKey)
+		})
+	}
+}
+
+func TestApplyTemplateRejectsPrivilegedTemplate(t *testing.T) {
+	literal := "irrelevant"
+	tests := []struct {
+		name     string
+		template *esv1.ExternalSecretTemplate
+		wantErr  string
+	}{
+		{
+			name: "service account token type with a service account annotation",
+			template: &esv1.ExternalSecretTemplate{
+				EngineVersion: esv1.TemplateEngineV2,
+				Type:          v1.SecretTypeServiceAccountToken,
+				Metadata: esv1.ExternalSecretTemplateMetadata{
+					Annotations: map[string]string{
+						v1.ServiceAccountNameKey: "kube-system-admin-sa",
+					},
+				},
+			},
+			wantErr: `template.type="kubernetes.io/service-account-token" with annotation "kubernetes.io/service-account.name" is not allowed`,
+		},
+		{
+			name: "service account token type with a templateFrom annotations target",
+			template: &esv1.ExternalSecretTemplate{
+				EngineVersion: esv1.TemplateEngineV2,
+				Type:          v1.SecretTypeServiceAccountToken,
+				TemplateFrom: []esv1.TemplateFrom{
+					{Literal: &literal, Target: esv1.TemplateTargetAnnotations},
+				},
+			},
+			wantErr: `template.type="kubernetes.io/service-account-token" with templateFrom target="Annotations" is not allowed`,
+		},
+		{
+			name: "bootstrap token type",
+			template: &esv1.ExternalSecretTemplate{
+				EngineVersion: esv1.TemplateEngineV2,
+				Type:          v1.SecretTypeBootstrapToken,
+			},
+			wantErr: `template.type="bootstrap.kubernetes.io/token" is not allowed`,
+		},
+	}
+	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,
+			}
+
+			es := &esv1.ExternalSecret{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-es", Namespace: "default"},
+				Spec: esv1.ExternalSecretSpec{
+					Target: esv1.ExternalSecretTarget{
+						Name:     "test-secret",
+						Template: tt.template,
+					},
+				},
+			}
+
+			secret := &v1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-secret", Namespace: "default"},
+			}
+
+			err := r.ApplyTemplate(context.Background(), es, secret, map[string][]byte{})
+
+			require.EqualError(t, err, tt.wantErr)
+			assert.Empty(t, secret.Type)
+			assert.NotContains(t, secret.Annotations, v1.ServiceAccountNameKey)
 		})
 	}
 }