From 51c99eeff1c372ca4b5dcd9b35d0b058e9dbf404 Mon Sep 17 00:00:00 2001 From: Patrick Seidensal Date: Mon, 31 Aug 2026 13:37:22 +0200 Subject: [PATCH] fix(agent): apply pod-security labels from namespaceLabels again namespaceLabels in fleet.yaml exists so that a bundle can label the namespace it deploys into, and setting Pod Security Admission labels that way is what SURE-5906 (#1484) asked for. Since #5156 the agent dropped every pod-security.kubernetes.io/ key from the options and skipped those keys when pruning, so labels a user declares are silently ignored (SURE-11931, #5550). That filter was the fix for GHSA-864g-863m-vcvq, where a bundle could weaken PSA enforcement on its target namespace because the agent applied namespaceLabels with its own cluster-admin credentials. #5351 changed that: the namespace is patched as the deployment's service account, so downstream RBAC decides what a bundle may write to it, and the prefix filter is no longer the control it was. The filter never covered the whole path either. The same label reaches the same API call, under the same credentials, when it is declared on a Namespace object in the bundle, or when a pre-provisioned namespace is adopted with helm.takeOwnership. It also matched on the key alone, so it refused tightening enforcement, and setting warn or audit levels, just as it refused weakening. Pod-security labels are therefore treated like every other namespace label again: applied when declared, pruned when they are not. Dropping the pruning exemption as well keeps un-setting working, which that exemption broke for these keys specifically. Deployments that resolve to no service account still patch the namespace as the agent, which is cluster admin. Restricting those requires pinning a service account, in the bundle or through a Policy. Refers to #5550 --- internal/cmd/agent/deployer/deployer.go | 37 ++++------- internal/cmd/agent/deployer/deployer_test.go | 69 +++++++++++++------- 2 files changed, 56 insertions(+), 50 deletions(-) diff --git a/internal/cmd/agent/deployer/deployer.go b/internal/cmd/agent/deployer/deployer.go index 5d6c9406c6..6b80a5b0cf 100644 --- a/internal/cmd/agent/deployer/deployer.go +++ b/internal/cmd/agent/deployer/deployer.go @@ -257,7 +257,7 @@ func (d *Deployer) setNamespaceLabelsAndAnnotations(ctx context.Context, bd *fle if desiredLabels == nil { desiredLabels = make(map[string]string) } - addLabelsFromOptions(log.FromContext(ctx), desiredLabels, bd.Spec.Options.NamespaceLabels) + addLabelsFromOptions(desiredLabels, bd.Spec.Options.NamespaceLabels) } desiredAnnotations := maps.Clone(ns.Annotations) if bd.Spec.Options.NamespaceAnnotations != nil { @@ -328,36 +328,21 @@ func fetchNamespace(ctx context.Context, c client.Client, releaseID string) (*co return ns, nil } -const podSecurityLabelPrefix = "pod-security.kubernetes.io/" - -// addLabelsFromOptions updates nsLabels to contain labels from optLabels, while preserving -// the `kubernetes.io/metadata.name` label added by Kubernetes when creating the namespace -// and any existing `pod-security.kubernetes.io/*` labels. Labels with the -// `pod-security.kubernetes.io/` prefix in optLabels are ignored. +// addLabelsFromOptions updates nsLabels to contain the labels from optLabels, while preserving +// the `kubernetes.io/metadata.name` label added by Kubernetes when creating the namespace. // -// This filtering is intentionally unconditional and independent of the -// service-account impersonation used for the namespace patch: it must also hold -// for deployments that run as the agent (no service account pinned), where -// there is no downstream RBAC gating at all. It is the only safeguard against a -// bundle escalating pod-security enforcement on its target namespace. To set -// pod-security labels on a namespace, declare them on the Namespace resource in -// the bundle instead. -func addLabelsFromOptions(logger logr.Logger, nsLabels map[string]string, optLabels map[string]string) { - for k, v := range optLabels { - if strings.HasPrefix(k, podSecurityLabelPrefix) { - logger.V(1).Info("Ignoring label from options", "label", k) - continue - } - nsLabels[k] = v - } +// Security-sensitive labels such as `pod-security.kubernetes.io/*` are not +// filtered here. The namespace is patched as the deployment's service account +// (see namespaceClient), so which labels a bundle may set on its target +// namespace is gated by the downstream RBAC of that account. Deployments that +// resolve to no service account still run as the agent, so restricting them +// requires pinning a service account, either in the bundle or through a Policy. +func addLabelsFromOptions(nsLabels map[string]string, optLabels map[string]string) { + maps.Copy(nsLabels, optLabels) // Delete labels not defined in the options. // Keep the `kubernetes.io/metadata.name` label as it is added by kubernetes when creating the namespace. - // Keep pod-security.kubernetes.io/ labels as they are managed by cluster administrators. for k := range nsLabels { - if strings.HasPrefix(k, podSecurityLabelPrefix) { - continue - } if _, ok := optLabels[k]; k != corev1.LabelMetadataName && !ok { delete(nsLabels, k) } diff --git a/internal/cmd/agent/deployer/deployer_test.go b/internal/cmd/agent/deployer/deployer_test.go index ff81a15b8b..a2d02f99c3 100644 --- a/internal/cmd/agent/deployer/deployer_test.go +++ b/internal/cmd/agent/deployer/deployer_test.go @@ -8,8 +8,6 @@ import ( fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" - "github.com/go-logr/logr" - corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -306,8 +304,8 @@ func TestSetNamespaceLabelsAndAnnotationsError(t *testing.T) { // TestSetNamespaceLabelsAndAnnotations_NoUpdateWhenAlreadyCorrect verifies that // updateNamespace is not called when the namespace already reflects the desired state. // This guards against the broken reflect.DeepEqual check that compared raw option -// labels to ns.Labels; ns.Labels always includes kubernetes.io/metadata.name and -// may include preserved pod-security labels, so a direct equality check never holds. +// labels to ns.Labels; ns.Labels always includes kubernetes.io/metadata.name, so a +// direct equality check never holds. func TestSetNamespaceLabelsAndAnnotations_NoUpdateWhenAlreadyCorrect(t *testing.T) { bd := &fleet.BundleDeployment{Spec: fleet.BundleDeploymentSpec{ Options: fleet.BundleDeploymentOptions{ @@ -348,13 +346,13 @@ func TestSetNamespaceLabelsAndAnnotations_NoUpdateWhenAlreadyCorrect(t *testing. } } -func TestAddLabelsFromOptions_PodSecurityLabelsFiltered(t *testing.T) { +func TestAddLabelsFromOptions_PodSecurityLabels(t *testing.T) { tests := map[string]struct { nsLabels map[string]string optLabels map[string]string expectedLabels map[string]string }{ - "pod-security.kubernetes.io labels in optLabels are not applied to namespace": { + "pod-security.kubernetes.io labels in optLabels are applied to the namespace": { nsLabels: map[string]string{"kubernetes.io/metadata.name": "ns"}, optLabels: map[string]string{ "pod-security.kubernetes.io/enforce": "privileged", @@ -363,15 +361,17 @@ func TestAddLabelsFromOptions_PodSecurityLabelsFiltered(t *testing.T) { "safe-label": "value", }, expectedLabels: map[string]string{ - "kubernetes.io/metadata.name": "ns", - "safe-label": "value", + "kubernetes.io/metadata.name": "ns", + "pod-security.kubernetes.io/enforce": "privileged", + "pod-security.kubernetes.io/audit": "privileged", + "pod-security.kubernetes.io/warn": "privileged", + "safe-label": "value", }, }, - "existing pod-security.kubernetes.io labels on namespace are preserved": { + "existing pod-security.kubernetes.io labels on the namespace are overwritten": { nsLabels: map[string]string{ "kubernetes.io/metadata.name": "ns", "pod-security.kubernetes.io/enforce": "baseline", - "pod-security.kubernetes.io/audit": "baseline", }, optLabels: map[string]string{ "pod-security.kubernetes.io/enforce": "privileged", @@ -379,11 +379,23 @@ func TestAddLabelsFromOptions_PodSecurityLabelsFiltered(t *testing.T) { }, expectedLabels: map[string]string{ "kubernetes.io/metadata.name": "ns", - "pod-security.kubernetes.io/enforce": "baseline", - "pod-security.kubernetes.io/audit": "baseline", + "pod-security.kubernetes.io/enforce": "privileged", "app-label": "value", }, }, + "pod-security.kubernetes.io labels not in optLabels are removed like any other label": { + nsLabels: map[string]string{ + "kubernetes.io/metadata.name": "ns", + "pod-security.kubernetes.io/audit": "baseline", + }, + optLabels: map[string]string{ + "app-label": "value", + }, + expectedLabels: map[string]string{ + "kubernetes.io/metadata.name": "ns", + "app-label": "value", + }, + }, "non-security labels work normally": { nsLabels: map[string]string{ "kubernetes.io/metadata.name": "ns", @@ -397,7 +409,7 @@ func TestAddLabelsFromOptions_PodSecurityLabelsFiltered(t *testing.T) { "new-label": "new-value", }, }, - "pod-security.kubernetes.io labels with custom suffixes are also filtered": { + "pod-security.kubernetes.io labels with version suffixes are applied as well": { nsLabels: map[string]string{"kubernetes.io/metadata.name": "ns"}, optLabels: map[string]string{ "pod-security.kubernetes.io/enforce-version": "v1.25", @@ -405,15 +417,17 @@ func TestAddLabelsFromOptions_PodSecurityLabelsFiltered(t *testing.T) { "safe-label": "value", }, expectedLabels: map[string]string{ - "kubernetes.io/metadata.name": "ns", - "safe-label": "value", + "kubernetes.io/metadata.name": "ns", + "pod-security.kubernetes.io/enforce-version": "v1.25", + "pod-security.kubernetes.io/audit-version": "v1.25", + "safe-label": "value", }, }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - addLabelsFromOptions(logr.Discard(), test.nsLabels, test.optLabels) + addLabelsFromOptions(test.nsLabels, test.optLabels) if len(test.nsLabels) != len(test.expectedLabels) { t.Errorf("expected %d labels, got %d: %v", len(test.expectedLabels), len(test.nsLabels), test.nsLabels) @@ -427,7 +441,12 @@ func TestAddLabelsFromOptions_PodSecurityLabelsFiltered(t *testing.T) { } } -func TestSetNamespaceLabelsAndAnnotations_PodSecurityLabelsPreserved(t *testing.T) { +// TestSetNamespaceLabelsAndAnnotations_PodSecurityLabelsApplied verifies that +// pod-security labels declared in namespaceLabels reach the namespace, which is +// what SURE-5906 (#1484) added the option for. Setting them is gated by the +// downstream RBAC of the deployment's service account, not by a filter in the +// agent. +func TestSetNamespaceLabelsAndAnnotations_PodSecurityLabelsApplied(t *testing.T) { bd := &fleet.BundleDeployment{Spec: fleet.BundleDeploymentSpec{ Options: fleet.BundleDeploymentOptions{ NamespaceLabels: map[string]string{ @@ -466,14 +485,16 @@ func TestSetNamespaceLabelsAndAnnotations_PodSecurityLabelsPreserved(t *testing. t.Fatalf("unexpected error: %v", err) } - if result.Labels["pod-security.kubernetes.io/enforce"] != "restricted" { - t.Errorf("pod-security.kubernetes.io/enforce: got %s, want restricted", result.Labels["pod-security.kubernetes.io/enforce"]) - } - if result.Labels["pod-security.kubernetes.io/audit"] != "restricted" { - t.Errorf("pod-security.kubernetes.io/audit: got %s, want restricted", result.Labels["pod-security.kubernetes.io/audit"]) + expected := map[string]string{ + "pod-security.kubernetes.io/enforce": "privileged", + "pod-security.kubernetes.io/audit": "privileged", + "pod-security.kubernetes.io/warn": "privileged", + "app-label": "value", } - if result.Labels["app-label"] != "value" { - t.Errorf("app-label: got %s, want value", result.Labels["app-label"]) + for k, v := range expected { + if result.Labels[k] != v { + t.Errorf("%s: got %s, want %s", k, result.Labels[k], v) + } } }