[Bug]: cnpg.io/podPatch is decoded into the already-built Pod: patching allowPrivilegeEscalation makes the container privileged, and an inserted container inherits the one it displaced #11742

Is there an existing issue already for this bug?

• I have searched for an existing issue, and could not find anything. I believe this is a new bug.

I have read the troubleshooting guide

• I have read the troubleshooting guide and I think this is a new bug.

I am running a supported version of CloudNativePG

• I am running a supported version of CloudNativePG.

Version

1.30.1 (operator image ghcr.io/cloudnative-pg/cloudnative-pg:1.30.1, Helm chart cloudnative-pg-0.29.1); reproduced on main at 21e24f6 as well

What version of Kubernetes are you using?

1.35.7

What is your Kubernetes environment?

Cloud: Azure AKS

How did you install the operator?

Helm

What happened?

cnpg.io/podPatch can produce an instance Pod that differs from the patch. A field written by the patch can change a field the patch did not touch, or be lost. An added container can also inherit fields from the container that was at its index before.

buildInstance (pkg/specs/pods.go) serializes the built Pod, applies the JSON patch, and then unmarshals the result back into that same *corev1.Pod:

if err = json.Unmarshal(serializedObject, pod); err != nil {

encoding/json reuses what is already in the target: existing slice elements and non-nil pointers are decoded into, not replaced. Two things in the built Pod make that visible.

1. Shared pointers in the default security context. GetSecurityContext (pkg/specs/containers.go) points two fields at each local:

   • RunAsNonRoot and ReadOnlyRootFilesystem both point to trueValue;
   • Privileged and AllowPrivilegeEscalation both point to falseValue.

   Decoding a patched value of one of the pair writes through the shared pointer, so the other reads the same value. Whichever key comes later in the patched JSON wins for both.

2. Slice elements are decoded in place. A container added at an index that is already occupied, e.g. /spec/initContainers/0 in front of bootstrap-controller, is decoded into the existing element. It keeps every field its own JSON does not set, such as volumeMounts and resources. That element's security context pointers are also the shared ones from point 1.

Reproduction on main (21e24f6). This test, put in pkg/specs, compares the NewInstance result with the same patch's JSON decoded into a new corev1.Pod:

package specs

import (
	"context"
	"encoding/json"
	"testing"

	jsonpatch "github.com/evanphx/json-patch/v5"
	corev1 "k8s.io/api/core/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

	apiv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1"
	"github.com/cloudnative-pg/cloudnative-pg/pkg/utils"
)

// The pod NewInstance returns for a podPatch, beside the pod the same patch
// gives when its JSON is decoded into a new Pod.
func patchedBoth(t *testing.T, patch string) (got, want *corev1.Pod) {
	t.Helper()
	cluster := apiv1.Cluster{
		ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "default", Annotations: map[string]string{utils.PodPatchAnnotationName: patch}},
		Status:     apiv1.ClusterStatus{Image: "postgres:18"},
	}
	got, err := NewInstance(context.Background(), cluster, 1)
	if err != nil {
		t.Fatal(err)
	}
	cluster.Annotations = nil
	plain, err := NewInstance(context.Background(), cluster, 1)
	if err != nil {
		t.Fatal(err)
	}
	raw, _ := json.Marshal(plain)
	p, err := jsonpatch.DecodePatch([]byte(patch))
	if err != nil {
		t.Fatal(err)
	}
	if raw, err = p.Apply(raw); err != nil {
		t.Fatal(err)
	}
	want = &corev1.Pod{}
	if err := json.Unmarshal(raw, want); err != nil {
		t.Fatal(err)
	}
	return got, want
}

func b(p *bool) any {
	if p == nil {
		return nil
	}
	return *p
}

func TestPodPatchRepro(t *testing.T) {
	t.Run("allowPrivilegeEscalation", func(t *testing.T) {
		got, want := patchedBoth(t, `[{"op":"replace","path":"/spec/containers/0/securityContext/allowPrivilegeEscalation","value":true}]`)
		g, w := got.Spec.Containers[0].SecurityContext, want.Spec.Containers[0].SecurityContext
		t.Logf("postgres container: privileged got=%v want=%v; allowPrivilegeEscalation got=%v want=%v", b(g.Privileged), b(w.Privileged), b(g.AllowPrivilegeEscalation), b(w.AllowPrivilegeEscalation))
	})
	t.Run("runAsNonRoot", func(t *testing.T) {
		got, want := patchedBoth(t, `[{"op":"replace","path":"/spec/containers/0/securityContext/runAsNonRoot","value":false}]`)
		g, w := got.Spec.Containers[0].SecurityContext, want.Spec.Containers[0].SecurityContext
		t.Logf("postgres container: runAsNonRoot got=%v want=%v; readOnlyRootFilesystem got=%v want=%v", b(g.RunAsNonRoot), b(w.RunAsNonRoot), b(g.ReadOnlyRootFilesystem), b(w.ReadOnlyRootFilesystem))
	})
	t.Run("initContainer added first", func(t *testing.T) {
		got, want := patchedBoth(t, `[{"op":"add","path":"/spec/initContainers/0","value":{"name":"first","image":"busybox:1.37","command":["true"],"securityContext":{"runAsNonRoot":false,"runAsUser":0,"readOnlyRootFilesystem":true,"allowPrivilegeEscalation":false,"capabilities":{"drop":["ALL"]}}}}]`)
		g, w := got.Spec.InitContainers[0], want.Spec.InitContainers[0]
		t.Logf("initContainers: got %d (%s, %s) want %d (%s, %s)", len(got.Spec.InitContainers), got.Spec.InitContainers[0].Name, got.Spec.InitContainers[1].Name, len(want.Spec.InitContainers), want.Spec.InitContainers[0].Name, want.Spec.InitContainers[1].Name)
		t.Logf("\"first\": readOnlyRootFilesystem got=%v want=%v; runAsNonRoot got=%v want=%v", b(g.SecurityContext.ReadOnlyRootFilesystem), b(w.SecurityContext.ReadOnlyRootFilesystem), b(g.SecurityContext.RunAsNonRoot), b(w.SecurityContext.RunAsNonRoot))
		t.Logf("\"first\": volumeMounts got=%d want=%d; env got=%d want=%d; resources.requests got=%v want=%v", len(g.VolumeMounts), len(w.VolumeMounts), len(g.Env), len(w.Env), g.Resources.Requests, w.Resources.Requests)
	})
}

$ go test ./pkg/specs/ -run '^TestPodPatchRepro$' -v
=== RUN   TestPodPatchRepro
=== RUN   TestPodPatchRepro/allowPrivilegeEscalation
podpatch_repro_test.go:59: postgres container: privileged got=true want=false; allowPrivilegeEscalation got=true want=true
=== RUN   TestPodPatchRepro/runAsNonRoot
podpatch_repro_test.go:64: postgres container: runAsNonRoot got=true want=false; readOnlyRootFilesystem got=true want=true
=== RUN   TestPodPatchRepro/initContainer_added_first
podpatch_repro_test.go:69: initContainers: got 2 (first, bootstrap-controller) want 2 (first, bootstrap-controller)
podpatch_repro_test.go:70: "first": readOnlyRootFilesystem got=true want=true; runAsNonRoot got=true want=false
podpatch_repro_test.go:71: "first": volumeMounts got=4 want=0; env got=0 want=0; resources.requests got=map[] want=map[]
--- PASS: TestPodPatchRepro (0.00s)
ok  	github.com/cloudnative-pg/cloudnative-pg/pkg/specs	0.674s

• Replacing only /spec/containers/0/securityContext/allowPrivilegeEscalation with true makes the postgres container privileged: true.
• Replacing only runAsNonRoot with false is lost: it reads true, because readOnlyRootFilesystem: true is decoded after it through the same pointer.
• An init container added at index 0 inherits bootstrap-controller's 4 volume mounts, and its own runAsNonRoot: false is lost.

How we hit it: we add an init container through podPatch (it sets the clock of a Kata pod VM, with runAsNonRoot: false, runAsUser: 0, readOnlyRootFilesystem: true, only SYS_TIME added), at /spec/initContainers/0. The created Pod had that container with readOnlyRootFilesystem: false, and our ValidatingAdmissionPolicy, which requires a read-only root filesystem, rejected the Pod. The databases whose Clusters got the patch had no instance Pod until we changed it. Appending the container (/spec/initContainers/-) avoids point 2 but not point 1.

This is not a security boundary issue, as far as we can tell: whoever can set cnpg.io/podPatch can already write any security context. But the Pod is not the one written, and Pod Security Admission or an admission policy may reject it, or let a more privileged container through than the patch asked for.

Expected: the instance Pod is the built Pod with the JSON patch applied, nothing else. Decoding the patched JSON into a new Pod would give that:

patched := &corev1.Pod{}
if err = json.Unmarshal(serializedObject, patched); err != nil {
	return nil, fmt.Errorf("while deserializing pod to JSON: %w", err)
}
pod = patched

Separately, GetSecurityContext giving each field its own pointer (e.g. ptr.To(true)) would keep anyone who mutates the returned struct from hitting point 1.

Cluster resource

apiVersion: postgresql.cnpg.io/v1
kind: Cluster
metadata:
  name: c
  annotations:
    cnpg.io/podPatch: '[{"op":"replace","path":"/spec/containers/0/securityContext/allowPrivilegeEscalation","value":true}]'
spec:
  instances: 1
  storage:
    size: 1Gi

Relevant log output

Not applicable: the Pod is built differently from the patch before it reaches the API server (see the reproduction above).
In our case the API server then rejected it: our ValidatingAdmissionPolicy requires `readOnlyRootFilesystem: true` on every container of these pods, and the init container added by the patch came out with `false`.

Code of Conduct

• I agree to follow this project's Code of Conduct