Просмотр исходного кода

fix(pushsecret): reject an empty source label selector

An empty selector under spec.selector.secret resolves to labels.Everything(),
not labels.Nothing(), so a PushSecret that renders "selector: {}" pushes every
Secret in the namespace to the provider, with no error anywhere. The realistic
trigger is a chart whose matchLabels block renders empty because a value was
missing.

Add CEL validation to PushSecretSecret: a selector must set matchLabels or
matchExpressions, and exactly one of name or selector may be set. The second
rule closes a related sharp edge in the same struct, since setting both
silently ignored the selector; the reconciler tests name first. ClusterPushSecret
embeds the same type, so it is covered too.

CEL runs only on write, and validation ratcheting keeps an existing object valid
through later updates, so admission alone would leave any cluster that already
has the misconfiguration with it permanently. resolveSecrets therefore also
rejects a selector that resolves to empty.

Covered by an envtest suite against a live API server and a unit test for the
reconciler guard. make test.crds is a one-directional snapshot gate and does not
see added validation rules, so it is not evidence here.

Refs: external-secrets/external-secrets#6945
Signed-off-by: Alexander Chernov <alexander@chernov.it>
Alexander Chernov 1 день назад
Родитель
Сommit
f4ee9487f7

+ 4 - 0
apis/externalsecrets/v1alpha1/pushsecret_types.go

@@ -122,6 +122,7 @@ type PushSecretSpec struct {
 }
 
 // PushSecretSecret defines a Secret that will be used as a source for pushing to providers.
+// +kubebuilder:validation:XValidation:rule="has(self.name) != has(self.selector)",message="exactly one of name or selector must be set"
 type PushSecretSecret struct {
 	// Name of the Secret.
 	// The Secret must exist in the same namespace as the PushSecret manifest.
@@ -132,7 +133,10 @@ type PushSecretSecret struct {
 	Name string `json:"name,omitempty"`
 
 	// Selector chooses secrets using a labelSelector.
+	// It must not be empty: an empty selector resolves to labels.Everything(),
+	// which would push every Secret in the namespace to the provider.
 	// +optional
+	// +kubebuilder:validation:XValidation:rule="has(self.matchLabels) && size(self.matchLabels) > 0 || has(self.matchExpressions) && size(self.matchExpressions) > 0",message="selector must set matchLabels or matchExpressions"
 	Selector *metav1.LabelSelector `json:"selector,omitempty"`
 }
 

+ 12 - 1
config/crds/bases/external-secrets.io_clusterpushsecrets.yaml

@@ -468,7 +468,10 @@ spec:
                             pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
                             type: string
                           selector:
-                            description: Selector chooses secrets using a labelSelector.
+                            description: |-
+                              Selector chooses secrets using a labelSelector.
+                              It must not be empty: an empty selector resolves to labels.Everything(),
+                              which would push every Secret in the namespace to the provider.
                             properties:
                               matchExpressions:
                                 description: matchExpressions is a list of label selector
@@ -513,7 +516,15 @@ spec:
                                 type: object
                             type: object
                             x-kubernetes-map-type: atomic
+                            x-kubernetes-validations:
+                            - message: selector must set matchLabels or matchExpressions
+                              rule: has(self.matchLabels) && size(self.matchLabels)
+                                > 0 || has(self.matchExpressions) && size(self.matchExpressions)
+                                > 0
                         type: object
+                        x-kubernetes-validations:
+                        - message: exactly one of name or selector must be set
+                          rule: has(self.name) != has(self.selector)
                     type: object
                   template:
                     description: Template defines a blueprint for the created Secret

+ 12 - 1
config/crds/bases/external-secrets.io_pushsecrets.yaml

@@ -391,7 +391,10 @@ spec:
                         pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
                         type: string
                       selector:
-                        description: Selector chooses secrets using a labelSelector.
+                        description: |-
+                          Selector chooses secrets using a labelSelector.
+                          It must not be empty: an empty selector resolves to labels.Everything(),
+                          which would push every Secret in the namespace to the provider.
                         properties:
                           matchExpressions:
                             description: matchExpressions is a list of label selector
@@ -436,7 +439,15 @@ spec:
                             type: object
                         type: object
                         x-kubernetes-map-type: atomic
+                        x-kubernetes-validations:
+                        - message: selector must set matchLabels or matchExpressions
+                          rule: has(self.matchLabels) && size(self.matchLabels) >
+                            0 || has(self.matchExpressions) && size(self.matchExpressions)
+                            > 0
                     type: object
+                    x-kubernetes-validations:
+                    - message: exactly one of name or selector must be set
+                      rule: has(self.name) != has(self.selector)
                 type: object
               template:
                 description: Template defines a blueprint for the created Secret resource.

+ 20 - 2
deploy/crds/bundle.yaml

@@ -2068,7 +2068,10 @@ spec:
                               pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
                               type: string
                             selector:
-                              description: Selector chooses secrets using a labelSelector.
+                              description: |-
+                                Selector chooses secrets using a labelSelector.
+                                It must not be empty: an empty selector resolves to labels.Everything(),
+                                which would push every Secret in the namespace to the provider.
                               properties:
                                 matchExpressions:
                                   description: matchExpressions is a list of label selector requirements. The requirements are ANDed.
@@ -2111,7 +2114,13 @@ spec:
                                   type: object
                               type: object
                               x-kubernetes-map-type: atomic
+                              x-kubernetes-validations:
+                                - message: selector must set matchLabels or matchExpressions
+                                  rule: has(self.matchLabels) && size(self.matchLabels) > 0 || has(self.matchExpressions) && size(self.matchExpressions) > 0
                           type: object
+                          x-kubernetes-validations:
+                            - message: exactly one of name or selector must be set
+                              rule: has(self.name) != has(self.selector)
                       type: object
                     template:
                       description: Template defines a blueprint for the created Secret resource.
@@ -15497,7 +15506,10 @@ spec:
                           pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
                           type: string
                         selector:
-                          description: Selector chooses secrets using a labelSelector.
+                          description: |-
+                            Selector chooses secrets using a labelSelector.
+                            It must not be empty: an empty selector resolves to labels.Everything(),
+                            which would push every Secret in the namespace to the provider.
                           properties:
                             matchExpressions:
                               description: matchExpressions is a list of label selector requirements. The requirements are ANDed.
@@ -15540,7 +15552,13 @@ spec:
                               type: object
                           type: object
                           x-kubernetes-map-type: atomic
+                          x-kubernetes-validations:
+                            - message: selector must set matchLabels or matchExpressions
+                              rule: has(self.matchLabels) && size(self.matchLabels) > 0 || has(self.matchExpressions) && size(self.matchExpressions) > 0
                       type: object
+                      x-kubernetes-validations:
+                        - message: exactly one of name or selector must be set
+                          rule: has(self.name) != has(self.selector)
                   type: object
                 template:
                   description: Template defines a blueprint for the created Secret resource.

+ 33 - 0
docs/api/pushsecret.md

@@ -27,6 +27,39 @@ stringData:
   best-pokemon-dst: "PIKACHU is the really best!"
 ```
 
+## Selecting the source
+
+`spec.selector` names the source of the data to push. Set exactly one of
+`spec.selector.secret` or `spec.selector.generatorRef`.
+
+Under `spec.selector.secret`, set exactly one of:
+
+- **`name`** (string): a single Secret in the same namespace as the PushSecret.
+- **`selector`** (object): a label selector matching one or more Secrets in the same namespace. Every match is pushed.
+
+```yaml
+spec:
+  selector:
+    secret:
+      # Either a single Secret by name
+      name: my-secret
+
+      # Or a label selector. It must match on something.
+      selector:
+        matchLabels:
+          push-to-provider: "true"
+```
+
+The selector must set a non-empty `matchLabels` or `matchExpressions`. An empty
+selector is rejected, because an empty label selector matches *every* Secret in
+the namespace rather than none, which would push the whole namespace to the
+provider. The usual way to hit this is a chart whose `matchLabels` block renders
+empty when a value is missing.
+
+Note that this only rules out the empty case. A selector is still yours to get
+right: an expression such as `{key: absent-label, operator: DoesNotExist}` is
+accepted and does match every Secret in the namespace.
+
 ## DataTo
 
 The `spec.dataTo` field enables bulk pushing of secrets without explicit per-key configuration. This is useful when you need to push multiple related secrets and want to avoid verbose YAML.

+ 3 - 1
docs/api/spec.md

@@ -15857,7 +15857,9 @@ Kubernetes meta/v1.LabelSelector
 </td>
 <td>
 <em>(Optional)</em>
-<p>Selector chooses secrets using a labelSelector.</p>
+<p>Selector chooses secrets using a labelSelector.
+It must not be empty: an empty selector resolves to labels.Everything(),
+which would push every Secret in the namespace to the provider.</p>
 </td>
 </tr>
 </tbody>

+ 6 - 0
pkg/controllers/pushsecret/pushsecret_controller.go

@@ -652,6 +652,12 @@ func (r *Reconciler) resolveSecrets(ctx context.Context, ps *esapi.PushSecret) (
 		if err != nil {
 			return nil, err
 		}
+		// An empty selector resolves to labels.Everything(). CEL rejects that at
+		// admission, but objects written before that rule still reach here, so
+		// fail closed rather than pushing every Secret in the namespace.
+		if labelSelector.Empty() {
+			return nil, errors.New("secret selector is empty, which would select every Secret in the namespace")
+		}
 
 		var secretList v1.SecretList
 		err = r.List(ctx, &secretList, &client.ListOptions{LabelSelector: labelSelector, Namespace: ps.Namespace})

+ 102 - 0
pkg/controllers/pushsecret/pushsecret_controller_resolve_test.go

@@ -0,0 +1,102 @@
+/*
+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 pushsecret
+
+import (
+	"context"
+	"testing"
+
+	"github.com/go-logr/logr"
+	"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"
+
+	"github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
+)
+
+// CEL rejects an empty selector at admission, but objects written before that
+// rule still reach the reconciler, so the guard is tested on its own.
+func TestResolveSecretsRejectsEmptySelector(t *testing.T) {
+	tests := []struct {
+		name     string
+		selector *metav1.LabelSelector
+		want     []string
+		wantErr  string
+	}{
+		{
+			name:     "empty selector selects nothing, not everything",
+			selector: &metav1.LabelSelector{},
+			wantErr:  "secret selector is empty",
+		},
+		{
+			name:     "empty matchLabels is still empty",
+			selector: &metav1.LabelSelector{MatchLabels: map[string]string{}},
+			wantErr:  "secret selector is empty",
+		},
+		{
+			name:     "a populated selector still resolves",
+			selector: &metav1.LabelSelector{MatchLabels: map[string]string{"push": "true"}},
+			want:     []string{"tagged"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			require.NoError(t, v1alpha1.AddToScheme(scheme.Scheme))
+			tagged := &v1.Secret{ObjectMeta: metav1.ObjectMeta{
+				Name: "tagged", Namespace: "default", Labels: map[string]string{"push": "true"},
+			}}
+			untagged := &v1.Secret{ObjectMeta: metav1.ObjectMeta{
+				Name: "untagged", Namespace: "default",
+			}}
+			r := &Reconciler{
+				Client: fakeclient.NewClientBuilder().WithScheme(scheme.Scheme).
+					WithObjects(tagged, untagged).Build(),
+				Scheme: scheme.Scheme,
+				Log:    logr.Discard(),
+			}
+
+			ps := &v1alpha1.PushSecret{
+				ObjectMeta: metav1.ObjectMeta{Name: "ps", Namespace: "default"},
+				Spec: v1alpha1.PushSecretSpec{
+					Selector: v1alpha1.PushSecretSelector{
+						Secret: &v1alpha1.PushSecretSecret{Selector: tt.selector},
+					},
+				},
+			}
+
+			got, err := r.resolveSecrets(context.Background(), ps)
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.wantErr)
+				assert.Nil(t, got)
+
+				return
+			}
+
+			require.NoError(t, err)
+			names := make([]string, 0, len(got))
+			for _, s := range got {
+				names = append(names, s.Name)
+			}
+			assert.ElementsMatch(t, tt.want, names)
+		})
+	}
+}

+ 168 - 0
pkg/controllers/pushsecret/pushsecret_validation_test.go

@@ -0,0 +1,168 @@
+/*
+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 pushsecret
+
+import (
+	"context"
+	"fmt"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
+	ctest "github.com/external-secrets/external-secrets/pkg/controllers/commontest"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+const (
+	errNotExclusive = "exactly one of name or selector"
+	errEmptySelect  = "selector must set matchLabels or matchExpressions"
+)
+
+// newPushSecretSpec builds a minimal PushSecret spec whose only interesting
+// part is the source selector under test.
+func newPushSecretSpec(src *v1alpha1.PushSecretSecret) v1alpha1.PushSecretSpec {
+	return v1alpha1.PushSecretSpec{
+		SecretStoreRefs: []v1alpha1.PushSecretStoreRef{
+			{Name: "a-store", Kind: "SecretStore"},
+		},
+		Selector: v1alpha1.PushSecretSelector{Secret: src},
+	}
+}
+
+var _ = Describe("PushSecret source selector validation", func() {
+	var namespace string
+	var counter int
+
+	BeforeEach(func() {
+		var err error
+		namespace, err = ctest.CreateNamespace("test-ns", k8sClient)
+		Expect(err).ToNot(HaveOccurred())
+	})
+
+	// The suite runs a live reconciler, so an accepted object would otherwise
+	// keep retrying against a SecretStore that does not exist.
+	create := func(src *v1alpha1.PushSecretSecret) error {
+		counter++
+		ps := &v1alpha1.PushSecret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      fmt.Sprintf("validation-%d", counter),
+				Namespace: namespace,
+			},
+			Spec: newPushSecretSpec(src),
+		}
+		err := k8sClient.Create(context.Background(), ps)
+		if err == nil {
+			DeferCleanup(func() {
+				Expect(k8sClient.Delete(context.Background(), ps)).To(Succeed())
+			})
+		}
+
+		return err
+	}
+
+	It("accepts a name on its own", func() {
+		Expect(create(&v1alpha1.PushSecretSecret{Name: "some-secret"})).To(Succeed())
+	})
+
+	It("accepts a selector with matchLabels", func() {
+		Expect(create(&v1alpha1.PushSecretSecret{
+			Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"push": "true"}},
+		})).To(Succeed())
+	})
+
+	It("accepts a selector with only matchExpressions", func() {
+		Expect(create(&v1alpha1.PushSecretSecret{
+			Selector: &metav1.LabelSelector{
+				MatchExpressions: []metav1.LabelSelectorRequirement{
+					{Key: "push", Operator: metav1.LabelSelectorOpExists},
+				},
+			},
+		})).To(Succeed())
+	})
+
+	// Guards the operator precedence of the two && clauses joined by ||: an
+	// empty matchLabels must not veto a populated matchExpressions.
+	It("accepts an empty matchLabels beside a populated matchExpressions", func() {
+		Expect(create(&v1alpha1.PushSecretSecret{
+			Selector: &metav1.LabelSelector{
+				MatchLabels: map[string]string{},
+				MatchExpressions: []metav1.LabelSelectorRequirement{
+					{Key: "push", Operator: metav1.LabelSelectorOpExists},
+				},
+			},
+		})).To(Succeed())
+	})
+
+	// An empty selector resolves to labels.Everything(), which would push every
+	// Secret in the namespace to the provider. It must not reach the reconciler.
+	It("rejects an empty selector", func() {
+		err := create(&v1alpha1.PushSecretSecret{Selector: &metav1.LabelSelector{}})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring(errEmptySelect))
+	})
+
+	It("rejects a selector whose matchLabels is empty", func() {
+		err := create(&v1alpha1.PushSecretSecret{
+			Selector: &metav1.LabelSelector{MatchLabels: map[string]string{}},
+		})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring(errEmptySelect))
+	})
+
+	It("rejects a selector whose matchExpressions is empty", func() {
+		err := create(&v1alpha1.PushSecretSecret{
+			Selector: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{}},
+		})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring(errEmptySelect))
+	})
+
+	// Both set is silently name-wins in the reconciler, so the manifest never
+	// meant what it says.
+	It("rejects name and selector together", func() {
+		err := create(&v1alpha1.PushSecretSecret{
+			Name:     "some-secret",
+			Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"push": "true"}},
+		})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring(errNotExclusive))
+	})
+
+	It("rejects neither name nor selector", func() {
+		err := create(&v1alpha1.PushSecretSecret{})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring(errNotExclusive))
+	})
+
+	// ClusterPushSecret embeds the same type, so the rules have to reach the
+	// template too.
+	It("rejects an empty selector in a ClusterPushSecret template", func() {
+		cps := &v1alpha1.ClusterPushSecret{
+			ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("validation-cluster-%d", counter)},
+			Spec: v1alpha1.ClusterPushSecretSpec{
+				PushSecretSpec: newPushSecretSpec(&v1alpha1.PushSecretSecret{
+					Selector: &metav1.LabelSelector{},
+				}),
+			},
+		}
+		err := k8sClient.Create(context.Background(), cps)
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring(errEmptySelect))
+	})
+})