diff --git a/pkg/clusteragent/admission/mutate/cwsinstrumentation/cws_instrumentation.go b/pkg/clusteragent/admission/mutate/cwsinstrumentation/cws_instrumentation.go index 4edece5ae6ef..942cf10e9140 100644 --- a/pkg/clusteragent/admission/mutate/cwsinstrumentation/cws_instrumentation.go +++ b/pkg/clusteragent/admission/mutate/cwsinstrumentation/cws_instrumentation.go @@ -16,6 +16,7 @@ import ( "fmt" "path/filepath" "strconv" + "strings" "time" "github.com/wI2L/jsondiff" @@ -447,22 +448,59 @@ func containerHasReadonlyRootfs(container corev1.Container) bool { return false } -func (ci *CWSInstrumentation) hasReadonlyRootfs(pod *corev1.Pod, container string) bool { +func findContainerByName(pod *corev1.Pod, container string) *corev1.Container { // check in the init containers - for _, c := range pod.Spec.InitContainers { + for i, c := range pod.Spec.InitContainers { if c.Name == container { - return containerHasReadonlyRootfs(c) + return &pod.Spec.InitContainers[i] } } // check the other containers - for _, c := range pod.Spec.Containers { + for i, c := range pod.Spec.Containers { if c.Name == container { - return containerHasReadonlyRootfs(c) + return &pod.Spec.Containers[i] } } + return nil +} - return false +func volumeMountContainsPath(mountPath, target string) bool { + mountPath = filepath.Clean(mountPath) + target = filepath.Clean(target) + if mountPath == target { + return true + } + if mountPath == "/" { + return true + } + return strings.HasPrefix(target, mountPath+"/") +} + +func (ci *CWSInstrumentation) isDirectoryReadOnly(pod *corev1.Pod, containerName, directory string) bool { + container := findContainerByName(pod, containerName) + if container == nil { + return true + } + // find the most specific VolumeMount that contains the directory + var matched *corev1.VolumeMount + matchedLen := -1 + for i, mnt := range container.VolumeMounts { + mountPath := filepath.Clean(mnt.MountPath) + if !volumeMountContainsPath(mountPath, directory) { + continue + } + if len(mountPath) > matchedLen { + matchedLen = len(mountPath) + matched = &container.VolumeMounts[i] + } + } + // if we found a VolumeMount, check its ReadOnly flag + if matched != nil { + return matched.ReadOnly + } + // otherwise, fall back to the container's SecurityContext + return containerHasReadonlyRootfs(*container) } func (ci *CWSInstrumentation) getPod(apiClient kubernetes.Interface, name string, ns string) (*corev1.Pod, error) { @@ -543,6 +581,14 @@ func (ci *CWSInstrumentation) injectCWSCommandInstrumentation(exec *corev1.PodEx var cwsInstrumentationRemotePath string + // check if the input container exists, or select the default one to which the user will be redirected + container, err := podcmd.FindOrDefaultContainerByName(pod, exec.Container, true, nil) + if err != nil { + log.Errorf("Ignoring exec request into %s: invalid container: %v", mutatecommon.PodString(pod), err) + metrics.CWSExecMutationAttempts.Inc(ci.mode.String(), "false", cwsInvalidInputContainerReason) + return false, errors.New(metrics.InvalidInput) + } + switch ci.mode { case InitContainer: cwsInstrumentationRemotePath = filepath.Join(cwsMountPath, "cws-instrumentation") @@ -566,10 +612,10 @@ func (ci *CWSInstrumentation) injectCWSCommandInstrumentation(exec *corev1.PodEx } cwsInstrumentationRemotePath = filepath.Join(cwsMountPath, cwsInstrumentationRemotePath) } else { - // check if the target pod has a read only filesystem - if readOnly := ci.hasReadonlyRootfs(pod, exec.Container); readOnly { + // if we're not using a shared volume, we need to make sure the directory is writable + if ci.isDirectoryReadOnly(pod, container.Name, ci.directoryForRemoteCopy) { // readonly rootfs containers can't be instrumented - log.Errorf("Ignoring exec request into %s, container %s has read only rootfs. Try enabling admission_controller.cws_instrumentation.remote_copy.mount_volume", mutatecommon.PodString(pod), exec.Container) + log.Errorf("Ignoring exec request into %s, directory %q is not writable in container %s that has readonly rootfs. Try enabling admission_controller.cws_instrumentation.remote_copy.mount_volume", mutatecommon.PodString(pod), ci.directoryForRemoteCopy, container.Name) metrics.CWSExecMutationAttempts.Inc(ci.mode.String(), "false", cwsReadonlyFilesystemReason) return false, errors.New(metrics.InvalidInput) } @@ -598,14 +644,6 @@ func (ci *CWSInstrumentation) injectCWSCommandInstrumentation(exec *corev1.PodEx return false, errors.New(metrics.InvalidInput) } - // check if the input container exists, or select the default one to which the user will be redirected - container, err := podcmd.FindOrDefaultContainerByName(pod, exec.Container, true, nil) - if err != nil { - log.Errorf("Ignoring exec request into %s: invalid container: %v", mutatecommon.PodString(pod), err) - metrics.CWSExecMutationAttempts.Inc(ci.mode.String(), "false", cwsInvalidInputContainerReason) - return false, errors.New(metrics.InvalidInput) - } - // copy CWS instrumentation directly to the target container if err := ci.injectCWSCommandInstrumentationRemoteCopy(pod, container.Name, cwsInstrumentationLocalPath, cwsInstrumentationRemotePath); err != nil { log.Warnf("Ignoring exec request into %s: remote copy failed: %v", mutatecommon.PodString(pod), err) diff --git a/pkg/clusteragent/admission/mutate/cwsinstrumentation/cws_instrumentation_test.go b/pkg/clusteragent/admission/mutate/cwsinstrumentation/cws_instrumentation_test.go index 4dfb93a21755..e90081747f21 100644 --- a/pkg/clusteragent/admission/mutate/cwsinstrumentation/cws_instrumentation_test.go +++ b/pkg/clusteragent/admission/mutate/cwsinstrumentation/cws_instrumentation_test.go @@ -1085,3 +1085,277 @@ func Test_initCWSInitContainerResources(t *testing.T) { }) } } + +// boolPtr returns a pointer to the provided bool value. +func boolPtr(b bool) *bool { + return &b +} + +// newPodWithContainer returns a pod whose single regular container is built +// from the provided container. +func newPodWithContainer(c corev1.Container) *corev1.Pod { + return &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{c}, + }, + } +} + +func Test_isDirectoryReadOnly(t *testing.T) { + const ( + targetContainer = "target" + directory = "/tmp" + ) + + tests := []struct { + name string + pod *corev1.Pod + container string + directory string + want bool + }{ + { + name: "no VolumeMount, no SecurityContext -> writable", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + }), + container: targetContainer, + directory: directory, + want: false, + }, + { + name: "no VolumeMount, ReadOnlyRootFilesystem=false -> writable", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + SecurityContext: &corev1.SecurityContext{ + ReadOnlyRootFilesystem: boolPtr(false), + }, + }), + container: targetContainer, + directory: directory, + want: false, + }, + { + name: "no VolumeMount, ReadOnlyRootFilesystem=true -> read-only (fallback)", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + SecurityContext: &corev1.SecurityContext{ + ReadOnlyRootFilesystem: boolPtr(true), + }, + }), + container: targetContainer, + directory: directory, + want: true, + }, + { + name: "VolumeMount on /tmp writable wins over RO rootfs (system-probe case)", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + SecurityContext: &corev1.SecurityContext{ + ReadOnlyRootFilesystem: boolPtr(true), + }, + VolumeMounts: []corev1.VolumeMount{ + {Name: "tmpdir", MountPath: "/tmp", ReadOnly: false}, + }, + }), + container: targetContainer, + directory: directory, + want: false, + }, + { + name: "VolumeMount on /tmp read-only wins over writable rootfs", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + VolumeMounts: []corev1.VolumeMount{ + {Name: "tmpdir", MountPath: "/tmp", ReadOnly: true}, + }, + }), + container: targetContainer, + directory: directory, + want: true, + }, + { + name: "VolumeMount on parent path / covers /tmp", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + SecurityContext: &corev1.SecurityContext{ + ReadOnlyRootFilesystem: boolPtr(true), + }, + VolumeMounts: []corev1.VolumeMount{ + {Name: "rootfs", MountPath: "/", ReadOnly: false}, + }, + }), + container: targetContainer, + directory: directory, + want: false, + }, + { + name: "most specific VolumeMount wins: / rw + /tmp ro -> /tmp ro", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + VolumeMounts: []corev1.VolumeMount{ + {Name: "rootfs", MountPath: "/", ReadOnly: false}, + {Name: "tmpdir", MountPath: "/tmp", ReadOnly: true}, + }, + }), + container: targetContainer, + directory: directory, + want: true, + }, + { + name: "sibling VolumeMount does not cover /tmp (no false positive)", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + SecurityContext: &corev1.SecurityContext{ + ReadOnlyRootFilesystem: boolPtr(true), + }, + VolumeMounts: []corev1.VolumeMount{ + {Name: "tmpfoo", MountPath: "/tmpfoo", ReadOnly: false}, + }, + }), + container: targetContainer, + directory: directory, + want: true, + }, + { + name: "child VolumeMount does not cover parent /tmp", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + SecurityContext: &corev1.SecurityContext{ + ReadOnlyRootFilesystem: boolPtr(true), + }, + VolumeMounts: []corev1.VolumeMount{ + {Name: "subdir", MountPath: "/tmp/foo", ReadOnly: false}, + }, + }), + container: targetContainer, + directory: directory, + want: true, + }, + { + name: "VolumeMount mountPath with trailing slash is normalized", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + SecurityContext: &corev1.SecurityContext{ + ReadOnlyRootFilesystem: boolPtr(true), + }, + VolumeMounts: []corev1.VolumeMount{ + {Name: "tmpdir", MountPath: "/tmp/", ReadOnly: false}, + }, + }), + container: targetContainer, + directory: directory, + want: false, + }, + { + name: "container name does not exist -> read-only (safe default)", + pod: newPodWithContainer(corev1.Container{ + Name: "other", + VolumeMounts: []corev1.VolumeMount{ + {Name: "tmpdir", MountPath: "/tmp", ReadOnly: false}, + }, + }), + container: targetContainer, + directory: directory, + want: true, + }, + { + name: "init container is also inspected", + pod: &corev1.Pod{ + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: targetContainer, + VolumeMounts: []corev1.VolumeMount{ + {Name: "tmpdir", MountPath: "/tmp", ReadOnly: false}, + }, + SecurityContext: &corev1.SecurityContext{ + ReadOnlyRootFilesystem: boolPtr(true), + }, + }, + }, + }, + }, + container: targetContainer, + directory: directory, + want: false, + }, + { + name: "non-default directory falls back when no mount covers it", + pod: newPodWithContainer(corev1.Container{ + Name: targetContainer, + SecurityContext: &corev1.SecurityContext{ + ReadOnlyRootFilesystem: boolPtr(true), + }, + VolumeMounts: []corev1.VolumeMount{ + {Name: "tmpdir", MountPath: "/tmp", ReadOnly: false}, + }, + }), + container: targetContainer, + directory: "/var/run", + want: true, + }, + } + + ci := &CWSInstrumentation{} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ci.isDirectoryReadOnly(tt.pod, tt.container, tt.directory) + require.Equal(t, tt.want, got) + }) + } +} + +func Test_volumeMountContainsPath(t *testing.T) { + tests := []struct { + name string + mountPath string + target string + want bool + }{ + {"exact match", "/tmp", "/tmp", true}, + {"mountPath is parent", "/", "/tmp", true}, + {"mountPath is multi-level parent", "/var", "/var/run/foo", true}, + {"mountPath equals target with trailing slash", "/tmp/", "/tmp", true}, + {"target with trailing slash", "/tmp", "/tmp/", true}, + {"sibling prefix is not a match", "/tmpfoo", "/tmp", false}, + {"target sibling of mount", "/tmp", "/tmpfoo", false}, + {"child mount does not cover parent", "/tmp/foo", "/tmp", false}, + {"unrelated paths", "/var", "/tmp", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.want, volumeMountContainsPath(tt.mountPath, tt.target)) + }) + } +} + +func Test_findContainerByName(t *testing.T) { + pod := &corev1.Pod{ + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + {Name: "init"}, + }, + Containers: []corev1.Container{ + {Name: "app"}, + {Name: "sidecar"}, + }, + }, + } + + t.Run("found in regular containers", func(t *testing.T) { + c := findContainerByName(pod, "sidecar") + require.NotNil(t, c) + require.Equal(t, "sidecar", c.Name) + }) + + t.Run("found in init containers", func(t *testing.T) { + c := findContainerByName(pod, "init") + require.NotNil(t, c) + require.Equal(t, "init", c.Name) + }) + + t.Run("not found returns nil", func(t *testing.T) { + require.Nil(t, findContainerByName(pod, "missing")) + }) +}