Add support for enforcing read only host paths in PSPs.

[jhorwit2]

What this PR does / why we need it:

This PR adds support for the PSP to enforce that host paths are readonly.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #57371
xref kubernetes/enhancements#5

Special notes for your reviewer:

Release note:

PodSecurityPolicy now supports restricting hostPath volume mounts to be readOnly and under specific path prefixes

/cc @ericchiang @liggitt

[liggitt]

will force volumes matching the path prefix to run as read-only

I don't think this should modify pod specs, which is what that sounds like.

Could be better phrased as "when set to true, will allow host volumes matching the pathPrefix if all volume mounts are readOnly", since you could have the following:

allowedHostPaths:[
{pathPrefix:"/foo", readOnly:true},
{pathPrefix:"/foo/bar", readOnly:false}
]

which would allow a writeable /foo/bar mount

[jhorwit2]

Ah, good catch. I copied the comment style from something does does modify the spec.

[liggitt]

this isn't quite right. see the example at #58647 (comment)

make sure there's a testcase for the overlapping-allowedHostPaths-with-different-readonly-settings scenario

[jhorwit2]

That link isn't going any where specific for me.

[liggitt]

weird, was just a link to the comment above

[liggitt]

still outstanding

[jhorwit2]

Mind looking at the tests I added on the 10th? Is it still outstanding? I thought I covered this there.

[jhorwit2]

@liggitt PTAL. I fixed the overlapping issue and added some test cases to cover it.

[liggitt]

If the container specifically requests to run with a non-read only volume that matches the path prefix then the PSP should deny the pod

this isn't accurate... another allowsHostVolume could allow it

[liggitt]

I'd probably just say when set to true, will allow host volumes matching the pathPrefix if all volume mounts are readOnly

[liggitt]

continueing in "success" cases assumes we'll never add other things that need checking later on in the loop. keep this structured to continue only to short-circuit the current volume checking in case of an error

[jhorwit2]

@liggitt this is keeping the same logic that we had in the util method before. I was trying to decrease the number of changes required to make this change. You'd prefer i set a variable then below check it before emitting every error?

[ericchiang]

I think what Liggitt is saying is that you're now continueing over stuff like flex volume validation further on. I'm personally confused why len(s.psp.Spec.AllowedHostPaths) == 0 would cause you to skip the flex volume checks.

Maybe just change the above if statement to:

if fsType == extensions.HostPath && len(s.psp.Spec.AllowedHostPaths) > 1 {
    // ...
}

[jhorwit2]

@ericchiang It would never skip flex volume checks if the fs type was flex volume because a volume can't be two types at the same time. So if it reached the existing allowed host path check then it must be a host volume and nothing else.

if fsType == extensions.HostPath {
	if len(s.psp.Spec.AllowedHostPaths) == 0 {
		// If no allowed paths are specified then allow any path
		// since host path volumes are allowed.
		continue
	}

[liggitt]

You'd prefer i set a variable then below check it before emitting every error?

No, I'd prefer to only continue early in cases where we added an error. The current change adds continue statements in non-error cases.

It would never skip flex volume checks if the fs type was flex volume because a volume can't be two types at the same time. So if it reached the existing allowed host path check then it must be a host volume and nothing else.

Continuing early in non-error cases assumes all other checks will be specific to other volume types, which is an assumption I'm trying to avoid adding here.

I was trying to decrease the number of changes required to make this change.

I do agree on minimal change... I think it can be contained entirely to this block and the AllowsHostVolumePath helper:

--- a/pkg/security/podsecuritypolicy/provider.go
+++ b/pkg/security/podsecuritypolicy/provider.go
@@ -229,10 +229,27 @@ func (s *simpleProvider) ValidatePodSecurityContext(pod *api.Pod, fldPath *field
                        }

                        if fsType == extensions.HostPath {
-                               if !psputil.AllowsHostVolumePath(s.psp, v.HostPath.Path) {
+                               pathIsAllowed, mustBeReadOnly := psputil.AllowsHostVolumePath(s.psp, v.HostPath.Path)
+                               if !pathIsAllowed {
                                        allErrs = append(allErrs, field.Invalid(
                                                field.NewPath("spec", "volumes").Index(i).Child("hostPath", "pathPrefix"), v.HostPath.Path,
                                                fmt.Sprintf("is not allowed to be used")))
+                               } else if mustBeReadOnly {
+                                       // Ensure all the VolumeMounts that use this volume are readonly
+                                       for i, c := range pod.Spec.InitContainers {
+                                               for j, cv := range c.VolumeMounts {
+                                                       if cv.Name == v.Name && !cv.ReadOnly {
+                                                               allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "initContainers").Index(i).Child("volumeMounts").Index(j).Child("readOnly"), cv.ReadOnly, "must be read-only"))
+                                                       }
+                                               }
+                                       }
+                                       for i, c := range pod.Spec.Containers {
+                                               for j, cv := range c.VolumeMounts {
+                                                       if cv.Name == v.Name && !cv.ReadOnly {
+                                                               allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "containers").Index(i).Child("volumeMounts").Index(j).Child("readOnly"), cv.ReadOnly, "must be read-only"))
+                                                       }
+                                               }
+                                       }
                                }
                        }

--- a/pkg/security/podsecuritypolicy/util/util.go
+++ b/pkg/security/podsecuritypolicy/util/util.go
@@ -173,25 +173,29 @@ func GroupFallsInRange(id int64, rng extensions.GroupIDRange) bool {
        return id >= rng.Min && id <= rng.Max
 }

-// AllowsHostVolumePath is a utility for checking if a PSP allows the host volume path.
-// This only checks the path. You should still check to make sure the host volume fs type is allowed.
-func AllowsHostVolumePath(psp *extensions.PodSecurityPolicy, hostPath string) bool {
+// AllowsHostVolumePath is a utility for checking if a PSP allows the host volume path, and whether volume mounts
+// that use the path are required to be readonly. This only checks the path. You should still check to make sure the host volume fs type is allowed.
+func AllowsHostVolumePath(psp *extensions.PodSecurityPolicy, hostPath string) (pathIsAllowed, mustBeReadOnly bool) {
        if psp == nil {
-               return false
+               return false, false
        }

        // If no allowed paths are specified then allow any path
        if len(psp.Spec.AllowedHostPaths) == 0 {
-               return true
+               return true, false
        }

        for _, allowedPath := range psp.Spec.AllowedHostPaths {
                if hasPathPrefix(hostPath, allowedPath.PathPrefix) {
-                       return true
+                       if !allowedPath.ReadOnly {
+                               return true, false
+                       }
+                       pathIsAllowed = true
+                       mustBeReadOnly = true
                }
        }

-       return false
+       return pathIsAllowed, mustBeReadOnly
 }

[jhorwit2]

/assign @liggitt

[liggitt]

drop the changes to this file

[liggitt]

this got added in this commit, then removed in the next. please squash or fixup to avoid noise

[liggitt]

/kind feature

[liggitt]

/priority important-soon

[liggitt]

oh, I still think we should put language in the PSP doc (either on the pathPrefix or readOnly field) that very explicitly warns about things that would let a pod circumvent the pathPrefix control:

• The PSP must set privileged to false, and set allowedHostPaths[*].readOnly to true, or the container can write to the filesystem in ways that let it traverse the host filesystem outside the pathPrefix

[k8s-github-robot]

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@jhorwit2 @liggitt @tallclair

Pull Request Labels

• sig/auth sig/storage: Pull Request will be escalated to these SIGs if needed.
• priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
• kind/feature: New functionality.

Help

• Additional instructions
• Commands for setting labels

[tallclair]

I somehow missed this PR until today, so I apologize in advance for the late feedback.

AllowedPaths isn't really useful unless ALL paths are marked readonly (as @liggitt referred to above). Also, it might be useful to have this option for other volume types as well. Given that, what do you think of this alternative API:

// ReadOnlyVolumes is a list of volume types that must be mounted read-only.
ReadOnlyVolumes []FSType

This could either be a separate whitelist from Volumes (i.e. a volume type should only be in one list, not both), or a strict subset of Volumes (i.e. put it in Volumes to allow it, and ReadOnlyVolumes to force read only).

WDYT?

[liggitt]

I'm not sure about pinning readonly restrictions to all volumes of a specific type. Are the reasons that require them all to be readonly mostly unique to hostPath?

If we anticipate being able to limit hostPath volumes to particular types (like socket) and pinned paths (not just prefixes) in the future, the per-hostPath construct makes more sense

[liggitt]

/retest

[msau42]

lgtm from storage perspective. I agree with @liggitt that requiring volumes to be read only for other volume types may not be as useful. Especially with PVCs being namespaced.

[dims]

/test pull-kubernetes-e2e-kops-aws

[liggitt]

/retest

[liggitt]

lgtm from storage perspective. I agree with @liggitt that requiring volumes to be read only for other volume types may not be as useful. Especially with PVCs being namespaced.

@tallclair WDYT?

[tallclair]

Ok, I'm convinced. This LGTM.

[liggitt]

docs in kubernetes/website#8898

[liggitt]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhorwit2, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• api/OWNERS [liggitt]
• docs/api-reference/OWNERS [liggitt]
• pkg/apis/OWNERS [liggitt]
• pkg/security/podsecuritypolicy/OWNERS [liggitt]
• staging/src/k8s.io/api/OWNERS [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[liggitt]

/retest

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 64344, 64709, 64717, 63631, 58647). If you want to cherry-pick this change to another branch, please follow the instructions here.