diff --git a/internal/cmd/agent/deployer/deployer.go b/internal/cmd/agent/deployer/deployer.go index 034917d8df..e10923a357 100644 --- a/internal/cmd/agent/deployer/deployer.go +++ b/internal/cmd/agent/deployer/deployer.go @@ -240,7 +240,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 { @@ -311,36 +311,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 8b4bf84a14..4cdfd840c2 100644 --- a/internal/cmd/agent/deployer/deployer_test.go +++ b/internal/cmd/agent/deployer/deployer_test.go @@ -5,8 +5,6 @@ import ( "errors" "testing" - "github.com/go-logr/logr" - fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" corev1 "k8s.io/api/core/v1" @@ -305,8 +303,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{ @@ -347,13 +345,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", @@ -362,15 +360,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", @@ -378,11 +378,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", @@ -396,7 +408,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", @@ -404,15 +416,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) @@ -426,7 +440,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{ @@ -465,14 +484,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) + } } } func TestIsStateAccepted(t *testing.T) {