Non-root support for volumes #12944

pmorie · 2015-08-19T21:47:07Z

This is a WIP proposal for handing concerns around volume ownership and pods running containers as non-root UIDs.

I'm still hacking around to see what kind of changes I think will be necessary but I think the use-case and analysis are probably ready to begin discussion around.

@thockin @vishh @smarterclayton @ncdc

k8s-bot · 2015-08-19T21:49:43Z

GCE e2e build/test failed for commit 7ffc2e6.

pmorie · 2015-08-19T21:50:48Z

pkg/kubelet/volumes.go

+		return nil
+	}
+
+	// Do images need to be pulled here?


For the record, I think this will need to be a function of the container runtime, because the image pull happens behind a Runtime interface method call.

k8s-bot · 2015-08-20T18:48:16Z

GCE e2e build/test failed for commit eceffd5a364f09b3c8e73aa2ce6182fe461a66e2.

ghost · 2015-08-25T19:10:59Z

@pmorie Re SELinux in this PR and #9844 what if we do the following:

Set all volumes to use the :z flag by default (for nfs the default label can be svirt_sandbox_file_t)
An optional improvement later can be to use the :Z flag where appropriate.
Add a SecurityContext feild to volumes so we can avoid having to infer explicit context from the first container in a pod a la WIP: Emptydir and security context #9844. If this field is specified the :z :Z flags are not used and files are labeled with the given context. Users who wish to avoid generic context can use this.

This would give good defaults and the options to change them for more strict security. WDYT ?

smarterclayton · 2015-08-25T19:53:51Z

There's a discussion going on to do a pod security context - that has
mostly superseded the idea of a security context to volumes (volumes would
inherit from the pod security policy).

On Tue, Aug 25, 2015 at 4:11 PM, Sami Wagiaalla notifications@github.com
wrote:

@pmorie https://github.com/pmorie Re SELinux in this PR and #9844
#9844 what if we do the
following:

Set all volumes to use the :z flag by default (for nfs the default
label can be svirt_sandbox_file_t)

An optional improvement later can be to use the :Z flag where
appropriate.

Add a SecurityContext feild to volumes so we can avoid having to
infer explicit context from the first container in a pod a la WIP: Emptydir and security context #9844
WIP: Emptydir and security context #9844. If this field is
specified the :z :Z flags are not used and files are labeled with the given
context. Users who wish to avoid generic context can use this.

This would give good defaults and the options to change them for more
strict security. WDYT ?

—
Reply to this email directly or view it on GitHub
#12944 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift

ghost · 2015-08-26T14:04:50Z

There's a discussion going on to do a pod security context
Ah yes.. I missed that PR.. pod level security context makes sense

pmorie · 2015-08-27T05:04:58Z

@smarterclayton @swagiaal I don't want to use :z at all anymore, because it relaxes the SELinux context of the volume so that it is usable from an SELinux standpoint by any container.

k8s-bot · 2015-08-27T05:05:36Z

GCE e2e build/test failed for commit d63eabd4b3668241f80057b3251c4e7b935ce97e.

pmorie · 2015-08-27T05:08:29Z

docs/proposals/volumes.md

+3.  The builders for distributed file systems should return the correct values based on the `Manage`
+    field of the volume source and the SELinux support of that volume type.
+
+TODO: persistent volumes


@markturansky, does this concern fall under the 'dynamic provisioning' work?

I will read this entire PR carefully to understand how it will work with PV and then provide feedback.

pmorie · 2015-08-27T05:10:47Z

@thockin @vishh @smarterclayton @swagiaal PTAL, made some major revisions and additions to this tonight.

k8s-bot · 2015-08-27T05:11:24Z

GCE e2e build/test failed for commit 7450cbc.

pmorie · 2015-08-27T16:33:03Z

@childsb @wattsteve PTAL

childsb · 2015-08-27T17:37:09Z

i'd like to see 2nd order multi-tenancy addressed. Or the use case of existing storage/LDAP system with UID/GIDs that may overlap with UID/GIDs on the host or other containers. If the decision is to specifically not allow it should be stated (i think it should be allowed)

pmorie · 2015-08-27T17:47:10Z

@childsb I'm not exactly clear on what '2nd order' multitenancy is -- what would be first vs. second order?

pmorie · 2015-08-27T17:48:07Z

@childsb I am planning to add a section on cluster-wide UID/GID provisioning, just want to make sure I know what you're looking for.

childsb · 2015-08-27T17:55:28Z

@pmorie i would define 1st/2nd order tenancy like:
1st order - user submitting the POD to kube
2nd order - user logging into the application running on top of kube that the first user submitted

pmorie · 2015-08-27T18:28:32Z

@childsb Is this the same topic we've previously discussed? You brought up examples of hadoop, tomcat, etc.

childsb · 2015-08-27T18:39:56Z

@pmorie yes same issue

pmorie · 2015-08-27T18:44:49Z

@childsb Okay, I will address that in the section I add about that. TL;DR: You should make sure your containers run as a single UID, handling multiple UIDs within containers is not going to be supported.

ghost · 2015-08-27T20:22:48Z

I don't want to use :z at all anymore, because it relaxes the SELinux context of the volume so that it is usable from an SELinux standpoint by any container.

If you do not use :z you must generate pod level MCS labels and override the container ones

ghost · 2015-08-27T20:28:55Z

docs/proposals/volumes.md

+
+The Kubelet must analyze the pod spec to determine which UIDs need to use which volumes.  If a
+container's security context's `RunAsUser` field is not set, the Kubelet must inspect the image via
+the container runtime to determine which UID the image will run as.  Once the list of UIDs that need


I think the kubelet should avoid inspecting container images. If RunAsUser is not specified then rely on the GID to grant volume permissions

I agree, and will change it. I don't think there should be any ownership determination. We already inspect the image though for the RunAsNonRoot feature.

We discussed this before and it is part of the policy because it matches
end user expectation. Images are late bound to nodes, so it has to be
enforced at the kubelet.

You can represent the policy you describe already.

On Aug 27, 2015, at 5:29 PM, Sami Wagiaalla notifications@github.com
wrote:

In docs/proposals/volumes.md
#12944 (comment):

+1. When does Kubernetes need to manage the ownership of a volume?
+2. How does Kubernetes determine the ownership of a volume?
+
+#### When to determine ownership
+
+Whether Kubernetes should manage the ownership of a volume for a distributed filesystem depends upon
+both the file system type and the cluster operator's policy. For example:
+
+1. Some organizations will manage ownership of volumes externally to the cluster
+2. It is not possible to securely chown or chmod paths within some distributed filesystems
+
+#### How to determine ownership
+
+The Kubelet must analyze the pod spec to determine which UIDs need to use which volumes. If a
+container's security context's RunAsUser field is not set, the Kubelet must inspect the image via
+the container runtime to determine which UID the image will run as. Once the list of UIDs that need

I think the kubelet should avoid inspecting container images. If RunAsUser
is not specified then rely on the GID to grant volume permissions

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/12944/files#r38142257.

I meant this in the context of RunAsNonRoot - late binding the image requires late binding the policy

pmorie · 2015-09-28T17:29:54Z

@thockin @smarterclayton PTAL

k8s-bot · 2015-09-28T18:04:37Z

Unit, integration and GCE e2e build/test failed for commit d096d83b7eee0b20b06a09fa7f10b4bb5d82c399.

pmorie · 2015-09-29T22:31:07Z

@thockin I updated the doc based on our discussion today. Since the SELinux one is going to have similar copy, would you care to review this one first? If we're over the hump on this one, I will update the SELinux proposal as well.

k8s-bot · 2015-09-29T22:57:54Z

Unit, integration and GCE e2e test build/test passed for commit 26d10817f78323d7d8a5c1a03bc33d1d8ea80542.

k8s-github-robot · 2015-09-30T00:09:28Z

Labelling this PR as size/XL

thockin · 2015-09-30T16:37:14Z

docs/proposals/volumes.md

+
+If the list of UIDs that need to use a volume includes both root and non-root users, supplemental
+groups can be applied to enable sharing volumes between containers.  The ownership and permissions
+`root:<supplemental group> 0770` will make a volume usable from both containers running as root and


you need the mode to be 02770 on the dir - if the group sticky bit is not set users can write to the volume as themselves but the process's FSGID is what is used.

I'm going to say we want the setgid bit set for now. I'm still thinking through all the implications of sticky bit. I think we want it, trying to determine if we should set it on all volumes.

thockin · 2015-09-30T16:56:44Z

just a couple nits, then LGTM.

I don't want to re-review, so feel free to self-lgtm this :)

pmorie · 2015-09-30T22:35:12Z

By the power vested in me by #12944 (comment), I declare this PR LGTM.

pmorie · 2015-09-30T22:35:25Z

Thanks for the review @thockin

k8s-bot · 2015-09-30T23:10:02Z

Unit, integration and GCE e2e build/test failed for commit 32b6646.

pmorie · 2015-09-30T23:34:36Z

@k8s-bot test this

k8s-bot · 2015-10-01T00:42:43Z

Unit, integration and GCE e2e build/test failed for commit 32b6646.

k8s-github-robot · 2015-10-01T12:53:06Z

Automatic merge from submit-queue

Auto commit by PR queue bot

googlebot added the cla: yes label Aug 19, 2015

pmorie reviewed Aug 19, 2015
View reviewed changes

pmorie mentioned this pull request Aug 19, 2015

Pod-level security context proposal #12823

Merged

pmorie force-pushed the nonroot-volumes-proposal branch from 7ffc2e6 to eceffd5 Compare August 20, 2015 18:36

k8s-github-robot assigned dchen1107 Aug 21, 2015

pmorie mentioned this pull request Aug 27, 2015

[RFC] adding a proposal for api groups #12951

Merged

pmorie force-pushed the nonroot-volumes-proposal branch from eceffd5 to d63eabd Compare August 27, 2015 05:03

pmorie reviewed Aug 27, 2015
View reviewed changes

pmorie force-pushed the nonroot-volumes-proposal branch from d63eabd to 7450cbc Compare August 27, 2015 05:09

ghost reviewed Aug 27, 2015
View reviewed changes

pmorie force-pushed the nonroot-volumes-proposal branch from 880b0d6 to d096d83 Compare September 28, 2015 17:29

pmorie mentioned this pull request Sep 28, 2015

Security contexts and volumes #7925

Closed

pmorie force-pushed the nonroot-volumes-proposal branch from d096d83 to 26d1081 Compare September 29, 2015 22:20

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 30, 2015

thockin reviewed Sep 30, 2015
View reviewed changes

Proposal for pod-level supplemental group and volume ownership mangement

32b6646

pmorie force-pushed the nonroot-volumes-proposal branch from 26d1081 to 32b6646 Compare September 30, 2015 22:24

pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2015

pmorie changed the title ~~WIP: Non-root support for volumes~~ Non-root support for volumes Sep 30, 2015

pmorie added the e2e-not-required label Sep 30, 2015

k8s-github-robot pushed a commit that referenced this pull request Oct 1, 2015

Merge pull request #12944 from pmorie/nonroot-volumes-proposal

b840357

Auto commit by PR queue bot

k8s-github-robot merged commit b840357 into kubernetes:master Oct 1, 2015

pmorie mentioned this pull request Oct 9, 2015

FSGroup implementation #15352

Merged

xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016

Merge pull request kubernetes#12944 from pmorie/nonroot-volumes-proposal

69881ec

Auto commit by PR queue bot

Non-root support for volumes #12944

Non-root support for volumes #12944

Conversation

pmorie commented Aug 19, 2015

k8s-bot commented Aug 19, 2015

Choose a reason for hiding this comment

k8s-bot commented Aug 20, 2015

ghost commented Aug 25, 2015

smarterclayton commented Aug 25, 2015

ghost commented Aug 26, 2015

pmorie commented Aug 27, 2015

k8s-bot commented Aug 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie commented Aug 27, 2015

k8s-bot commented Aug 27, 2015

pmorie commented Aug 27, 2015

childsb commented Aug 27, 2015

pmorie commented Aug 27, 2015

pmorie commented Aug 27, 2015

childsb commented Aug 27, 2015

pmorie commented Aug 27, 2015

childsb commented Aug 27, 2015

pmorie commented Aug 27, 2015

ghost commented Aug 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie commented Sep 28, 2015

k8s-bot commented Sep 28, 2015

pmorie commented Sep 29, 2015

k8s-bot commented Sep 29, 2015

k8s-github-robot commented Sep 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Sep 30, 2015

pmorie commented Sep 30, 2015

pmorie commented Sep 30, 2015

k8s-bot commented Sep 30, 2015

pmorie commented Sep 30, 2015

k8s-bot commented Oct 1, 2015

k8s-github-robot commented Oct 1, 2015