Add KEP for SELinux label change

[jsafrane]

This KEP tries to speed up the way how volumes (incl. persistent volumes) are made available to Pods on systems with SELinux in enforcing mode.

Current way includes recursive relabeling of all files on a volume before a container can be started. This is slow for large volumes.

Familiarity with fsGroupChangePolicy KEP is suggested

[jsafrane]

Passed the first round of internal review. Adding more people from the community:

Explicitly @tallclair @derekwaynecarr for kubelet expertise and strong security background.
@haircommander @rhatdan for CRI and SELinux
@kubernetes/sig-node-proposals
@kubernetes/sig-storage-pr-reviews

[haircommander]

LGTM from runtime side 👍
cc @mrunalp @umohnani8 @saschagrunert to see if they have any comments

[derekwaynecarr]

@sjenning or @mrunalp can you help do an initial review?

[saschagrunert]

LGTM too (unfortunately I'm not a SELinux expert)

[rhatdan]

It should be noted that not all file systems support SELinux labeling. Specifically file systems without XAttr support. For these cases the volume needs to be mounted with the context mount or the container has to be run without SELinux separation. In the future we are shipping UDICA (https://github.com/containers/udica) which is a mechanism to have more then on container process type. "container_t", This would allow users to have access to random labels on the system, but the policy needs to be installed on the system.

[jsafrane]

Thinking again about shared volumes, I needed to add CSIDriver.Spec.SupportsSELinux to tell kubelet that it should not use mount -o context=xyz for volumes provided by a particular CSI driver. For some CSI drivers (e.g. NFS-client, mount -o context=A nfs-server.com:/export/vol1 <somewhere> sets SELinux context of all mounts from the same NFS share. mount -o context=B nfs-server.com:/export/vol2 <somewhere else> will fail, because the NFS export is already mounted with context A.

Not sure if SupportsSELinux is a great name.

[gnufied]

You mean default to Always right?

[jsafrane]

No, this is Kubernetes feature gate.

[gnufied]

Another option is perhaps - to have a field called StorageIsolationPolicy or SecurityPolicy and define a enum for it with possible value of selinux for now. In future this field could be expanded to include other security policies such as apparmour or something else in windows.

[jsafrane]

As I pointed above, the name (and actually also its description) is misleading. The flag is uses to determine if mount -o context on a volume can affect other volumes of the same CSI driver on the same node.

If nfs-client provisioner had a CSI driver, mounting one of its volume with -o context=A sets the context for all other volumes on the node, because they come from the same NFS export.

If nfs provisioner had a CSI driver, mounting one of its volume with -o context=A does not affect the other volumes, because they use separate NFS exports [here I am not 100% sure what ganesha actually does, but I believe there are other NFS servers where it would work].

So the flag is actually about the server and independence of its volumes, not about SELinux or apparmor. It needs a better name.

[jsafrane]

Added more description for SupportsSELinux and table with examples.

[msau42]

Following my review comment for the fsgroup field, I think it would be better to have explicit enums for the 3 behaviors.

[jsafrane]

I renamed the field to SELinuxMountSupported to emphasize that it is only about mounts with -o context. It reduces number of cases to handle in kubelet / CSI volume plugin. If the mounted filesystem supports SELinux or not is always autodetected by presence of seclabel mount option, as it is already done now.

Therefore the values are true or false. nil means false. Do you still want enum? It would be something line "Supported" and "Unsupported", which looks odd.

[msau42]

We can debate naming later, but I prefer something like "Recursive" instead of "Always", because we will relabel in all cases (if driver supports), regardless of what value is set here.

[msau42]

What about the case where the driver doesn't support it and we don't want to recursively relabel?

[jsafrane]

This is currently autodetected by presence of seclabel mount option. If it's there after NodePublish, the mounted volume supports SELinux and CRI relabels, even in Kubernetes 1.18 and earlier.

This behavior will be the same with this KEP. I haven't heard requests not to relabel when seclabel is present.

[msau42]

mostly lgtm. just some nits

[msau42]

Should we mention here that this option may still fallback to recursive mode if the driver doesn't support volume mount mode?

[jsafrane]

Added this:

Kubernetes may fall back to policy "Always" if a storage backed does not support this policy.

[msau42]

nit: This isn't a "regression". Users using the default mode (today's behavior) will continue to work. It's a difference in behavior for the new mode.

[jsafrane]

reworded to:

The only different behavior is when two pods with different SELinux context use the same volume, but different SubPath - they are working with Always policy, as the container runtime relabeled only the subpaths, with OnVolumeMount the whole volume must have the same context.

[jsafrane]

Updated and squashed everything to a single commit.

[msau42]

LGTM will let @tallclair also review

[tallclair]

Suggested change

	This KEP tries to speed up the way how volumes (incl. persistent volumes) are made available to Pods on systems with SELinux in enforcing mode.
	This KEP tries to speed up the way that volumes (incl. persistent volumes) are made available to Pods on systems with SELinux in enforcing mode.

[tallclair]

nit: I don't think this KEP is relevant to sig-auth.

[jsafrane]

removed

[tallclair]

What do you think about combining SELinuxRelabelPolicy and FSGroupChangePolicy into a single option? For example, something like this:

const (
  // The heuristic policy acts like setting both the OnVolumeMount policy and the OnRootMismatch policy.
  HeuristicVolumeChangePolicy VolumeChangePolicy = "Heuristic"
  RecursiveVolumeChangePolicy VolumeChangePolicy = "Recursive"
)

type PodSeucrityContext struct {
  ...
  VolumeChangePolicy *VolumeChangePolicy
  ...
}

The motivation is that these settings seem very closely related, and would probably typically be set together. This decreases the flexibility, but simplifies the API and feature usage.

[gnufied]

We are targeting beta for fsGroupChangePolicy feature this quarter - https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/695-skip-permission-change . If we were to rename the field, we will have to probably put a halt on that. Nothing too problematic, I think it is fine if it stays alpha in one more release but just wanted to give heads up.

[gnufied]

Thinking through - there are some downsides of combining selinux and fsGroup options which are worth considering:

1. Having them behind one option kind of makes API hard to understand without reading the documentation in detail. IMO this is lot of detail to hide behind one field which is not very self-explanatory.
2. There are use cases where one may want to set OnRootMistach but Always for selinux or vice-versa. For example for a volume driver that supports selinux(such as gce-pd), using OnVolumeMount is perfectly fine default for all use cases even when you bring a volume with data on it. On other hand there could be cases where a user may have to use Always as default fsGroupChangePolicy but not Always selinux policy.

[jsafrane]

I see benefits of a single field, however, "Heuristic" won't work well with fsGroup and a PV that's used also outside of Kubernetes, without fsGroup knowledge. If Heuristic was used, mounting -o context will work without issues, but skipping fsGroup chown / chmod may not work, because the top dir may have the right owner, while something outside of Kubernetes did create files on the volume with a wrong owner.

So, user may want full fsGroup change, but skip SELinux. I admit it may be a minor artificial use case, still, I don't want to paint us in the corner by combining the fields together.

[jsafrane]

Added as "considered alternative".

[tallclair]

OK, this reasoning makes sense to me. It's unfortunate though, as the majority of the time I forsee the non-"always" option being used when the volume is too large and the recursive rewriting is too slow. It's unfortunate that these implementation details are getting surfaced in the API.

[msau42]

In the SELinux case, would anyone want the Recursive approach over the VolumeMount approach? I'm wondering if we can get rid of the SELinux policy in PodSecurityContext, and only use the CSIDriver field to figure out what to do.

[jsafrane]

In the SELinux case, would anyone want the Recursive approach over the VolumeMount approach

I can think only about shared volumes + subpaths, they behave a bit differently with OnMount.

BTW, there is still the first alternative - follow fsGroup / OnRootMismatch. Then we can merge the API easily, save lot of code and make everything consistent... Just the speed will suffer, as kubelet has to relabel the volume when used for the first time.

[msau42]

The first alternative of having the same behavior of the fsgroup change policy sounds nice if that means we can also combine the API fields into one.

[msau42]

I think the penalty of relabeling on the first mount is acceptable.

[jsafrane]

/retest

[msau42]

Discussed offline, we'll continue for alpha as proposed, but

• We'll keep fsgroup policy in alpha for 1.19
• consider if we can merge the two APIs before going to beta

[msau42]

/lgtm
/approve
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, msau42

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:

• keps/sig-storage/OWNERS [jsafrane,msau42]

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