Create no-new-privs support proposal doc.

[xingzhou]

kubernetes/kubernetes#38417
Create no-new-privs support proposal doc under
contributors/design-proposals.

[timstclair]

@kubernetes/sig-node

[timstclair]

@euank @yifan-gu - Does rkt have support for this?

[euank]

rkt has an isolator for it, but does not expose it as a CLI flag at this time.

It's a kinda low-level knob and asking the user to tweak it isn't great.

To change it in rkt, an image you package can indicate whether it should be run this way or not (e.g. when I package nginx I might assert it can run with no-new-privs and rkt will apply it, but when I package an application which uses suid ping I wouldn't set the isolator and rkt wouldn't apply it).

It wouldn't be too difficult to expose it on the rkt cli if we needed to (though I don't think we see much value in that) or for rktlet to tweak that lower level knob.

cc @lucab

[xingzhou]

@timstclair, is that ok to only support docker runtime for now?

[cyphar]

runC does have a knob for this in the runtime-spec, so it would only be rkt that doesn't support this for now.

[xingzhou]

yes, for now, I suggest we can hold on for rkt implementation and just implement the support for other runtimes.

[philips]

cc @lucab @jonboulle

[philips]

Regardless rkt supports no_new_privs since May 2016. So, we can make whatever change are necessary to support this Kubernetes API. I filed a tracking issue against rkt to ensure it is exposed in a way that it can be consumed by cri-rkt. rkt/rkt#3491

[vishh]

I'd expect this feature to be turned on by default and so I don't see a need for exposing a CLI knob.

[thockin]

It's unclear from @jessfraz above whether it can realistically be turned on by default, and even if so, it probably needs some way to disable if, for example, you want to ship ping.

Is this equivalent to mounting NOSUID ?

[thockin]

This is, however, a VERY low-level mechanism-oriented flag. Is there a way we can re-express this with clearer high-level intent instead?

[timstclair]

I'm not a fan of duplicating every field no both the SecurityContext and PodSecurityContext unless it's something that really makes sense at the pod level (I don't think this does). We haven't been consistent about this, so I'm curious what other people think.

@kubernetes/api-review-team @pmorie

[timstclair]

nit: drop the v1., it applies to all versions.

[xingzhou]

let me modify the proposal then.

[timstclair]

(copied from previous doc) Can you elaborate on the use cases for running without noNewPrivileges? The reasons to defaulting to disabled (i.e. noNewPrivileges=false) in docker are discussed here, but I think it's worth reconsidering this choice for Kubernetes. There are backwards compatability isuses to consider, but perhaps this can be resolved with a default PSP on new clusters. /cc @pweil-

[cyphar]

@timstclair set-uid binaries don't work with noNewPrivileges=true -- which could break a lot of things. For example, the reason that ping works in ubuntu Docker images is because ping is a set-uid binary on Ubuntu (if you try to use ping with an opensuse or debian image it will fail because the capability has been disabled even though it has a set-cap bit set).

[cyphar]

I can see there being a benefit to having two containers in the same pod having different noNewPrivileges values (though the benefit to the user is questionable at best) because you might have a tool in a Pod that needs set-uid, but you don't want to enable it for another container in a Pod.

On the other hand, it would be very useful to users if they can make sure that all containers created in a Pod have noNewPrivileges=true set (since you need to set it each time you start a container because it's inherited from the parent, not from the Pod).

[mrunalp]

I agree with @cyphar Best to allow the user the ability to enable/disable this per container and pod. (Probably having the container setting override the pod setting).

[xingzhou]

let me update the proposal by:
• Set the default value of noNewPrivileges as false, as if it is true by default, a lot of programs which depend on suid or sguid would fail.
• Still add noNewPrivileges field to both PodSecurityContext and SecurityContext. The noNewPrivileges=true in PodSecurityContext means all the containers inside a Pod should enable no-new-privs, while the noNewPrivileges in container SecurityContext could override the pod level settings.

@timstclair, not sure if this is ok to you or do we need more discussion on this?

[timstclair]

SGTM

[feiskyer]

cc @mrunalp @cyphar from cri-o

[cyphar]

Is there any more description of what this flag might be extended to accept? Or do you just want to define --security-opt=no-new-privileges for now and we can expand it later?

[xingzhou]

@cyphar, for now, I'm thinking just define --security-opt=no-new-privileges, but we can expand it with other options support. I've discussed this with @timstclair on this part before, maybe we can start another ticket to resolve the kubectl enhancement.

[vishh]

kubectl run is intentionally simple. I don't see a need for exposing this kind of a knob via kubectl.

[jessfraz]

+1 @vishh its not like that flag already exists to support apparmor and seccomp, adding it would only cause confusion and lead people to believe you can pass the same args as the docker cli tbh

[jessfraz]

Also I always hated how we named that flag because it didn't scale well ;)

[timstclair]

Agreed. Let's remove this section from this proposal. If you want to be able to set security options via kubectl run, you can open a separate proposal for that.

[smarterclayton]

I agree with vish, this is not an action we would expose through kubectl. It's probably a candidate for "kubectl set security-context" or something similar though.

[yujuhong]

+1. Please do not expose this directly in kubectl run.

[cyphar]

I like the idea of supporting noNewPrivileges in k8s, so that we can set that bit inside cri-o (because runC also has support for it in the OCI spec). There's just the one question (from the k8s side) on whether it should be defined as a Pod-level concept as well as a container-level one. But yeah, IANAM but LGTM.

[mrunalp]

I suggest that we add a link to https://www.kernel.org/doc/Documentation/prctl/no_new_privs.txt

[xingzhou]

Sure, let me add the link then.

[xingzhou]

@timstclair, @cyphar, @mrunalp, @euank, @feiskyer thanks for your review and comments. I've left some response in this PR and updated the proposal doc, please take a look, thanks!

[cyphar]

Not sure if you want to point to the OCI, but there are the points where you could add the information.

[cyphar]

You can also mention that the OCI runtime spec had support for it as of this pr.

[jessfraz]

Also might mention its on by default in docker

[mrunalp]

@jessfraz It isn't on by default AFAIK. One needs to pass --security-opt=no-new-privileges to enable it. Did this change recently?

[jessfraz]

crap I guess we didn't do that, such sad

[jessfraz]

fwiw there were issues with SELINUX, idk if these have since been fixed in fedora, but maybe its best to not default to on moby/moby#23981 (comment)

[cyphar]

Maybe add a very short section about runC / OCI

###Support in OCI runtimes

Since version 0.3.0 of the OCI runtime specification, a user can specify the `noNewPrivs` boolean flag in the configuration file.

[cyphar]

Aside from my comments, this LGTM.

[xingzhou]

Added OCI support section to the proposal.

[cyphar]

LGTM 👍

[lucab]

I have three concerns on this no-new-privs proposal in its current form:

1. no-new-privs is a very low-level primitive. It is required/recommended for other higher level isolation layers and I'm not sure this should be exposed to the user as-is¹. Is there a way for this to be linked with other sandboxing settings instead?

2. some combinations are not possible. For example, non-root seccomp-2 requires no-new-privs. Same for dropped CAP_SYS_ADMIN. Who is responsible for checking this? What should a runtime do in this case?

3. this proposal defaults to false. This will make some other sandboxing mechanisms moot or impossible to apply. Given the two points above, I wonder if the runtime should be left with some freedom to apply sane defaults (based on other sandboxing settings) instead of trying to enforce false unconditionally.

¹: In its creator's words: no_new_privs is not a sandbox. It's just a way to make it safe for sandboxes and other such weird things processes can do to themselves safe across execve.

[xingzhou]

@lucab, thanks for the comments. Here are some points from my view, for discussion:
• for the impossible security options combinations, instead of applying sane defaults, we can just detect the conflicts and report to user. This will help user better understand what happened in the background.
• As docker and OCI all support no-new-privs, I think it is the best that we can keep the consistent user experience in k8s. This helps user manipulate kubernetes in a more easier way.

[philips]

@xingzhou can you take a swing at documenting all of the potential error cases as outlined by @lucab and document the errors that should be reported and where they will be reported?

[euank]

@xingzhou

for the impossible security options combinations, instead of applying sane defaults, we can just detect the conflicts and report to user. This will help user better understand what happened in the background.

That will lead to making security harder for the user. This complexity means fewer people will use this feature and more people will be less secure.
If it's possible to have more secure defaults, it's a win for everyone.
Well, except that we have to think more now to get it right, but it's worth it!

---

As docker and OCI all support no-new-privs, I think it is the best that we can keep the consistent user experience in k8s. This helps user manipulate kubernetes in a more easier way.

"Docker supports feature X" is not sufficient reason for Kubernetes to support it. Kubernetes' API is explicitly not meant to expose every feature of OCI, or Docker, or any underlying system. Kubernetes is not just a way to run Docker containers, nor is it meant to have the same semantics.

---

I'd prefer a better answer to @lucab's concerns. From your first bullet point, it sounds like the suggestion is to push additional pain onto the user, and I think we should think more carefully about any possible alternatives before doing so.

[xingzhou]

ok, so let me summarize the possible option conflicts first, then we can see what kind of default value design we can make for no-new-privs

[erictune]

New fields on securityContext should have corresponding fields in podSecurityPolicy.
The pod security policy admission controller is a good place to set default values in securityContext, and as a way to control which users can use which securityContext options.

[pweil-]

s/mager/manager

[pweil-]

I think this logic may be getting spread around a bit. I would suggest it be added to the security context provider: https://github.com/kubernetes/kubernetes/blob/bcc783c594652df29c8b7d45ae75f26ab7b5b65a/pkg/securitycontext/provider.go#L54

[yujuhong]

We are in the middle of migrating to the CRI implementation in 1.6, if everything goes well (fingers crossed). In the meantime, I would like to freeze development in the old implementation (pkg/kubelet/dockertools) if possible. I would suggest removing all sections referring the old implementation from this proposal.

p.s. I recently moved all docker-specific code from pkg/securitycontext into pkg/kubelet/dockertools/securitycontext, so the link @pweil- gave above is outdated.

[pweil-]

is defaulting to true so you can maintain backwards compatibility? Any thoughts on defaulting to the secure setting (I guess that would mean the var needs to be something like newPrivs so when it is left off of a pod spec you're not getting any extra permissions? This is how things like hostPID and hostNetwork work.

[euank]

@xingzhou to clarify your last comment, do you mean you're planning to add a summary of options to the proposal and suggest one of them?

[vishh]

What would be the drawback if we set no_new_privs by default for all non privileged containers? Would it break any existing apps? Do we need a knob for sure?

[jessfraz]

I honestly don't think it would break many turning it on by default for non privileged containers, +1

[timstclair]

It would break anything with a suid bit (e.g. ping). Unfortunately this probably breaks enough containers that we can't enable it by default, but we can document that it's a best practice to set it to false in the PodSecurityPolicy.

[timstclair]

Does noNewPrivileges have any meaning for a privileged container? If so, please explain it in the document. If not, please explain how it will be handled (i.e. do we reject it as an invalid config?)

[mrunalp]

@timstclair It should be disabled when privileged is enabled. Agree that behavior should be clarified.

[smarterclayton]

We don't use negative names, and I agree with Tim that this is low level. I'd suggest allowPrivilegeEscalation or something that uses a word to capture the appropriate level of risk. We should not default this in the API, but at the kubelet, and use podsecuritypolicy to set appropriate defaults (which is how we treat these other flags).

[vishh]

By default we run container's as root. suid binaries will be broken only if the containers are run as non root users AFAIK. May be turn it on by default for non-privileged and uid `0` containers? In general suid is bad. I'd recommend making usage of suid opt-in rather than opt-out.

…

On Thu, Jan 5, 2017 at 2:42 PM, Tim St. Clair ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In contributors/design-proposals/no-new-privs.md <#180>: > +While creating a container, kubelet parses the security context object and formats the security option strings for Docker. The security options strings will finally be inserted into `ContainerCreateConfig.HostConfig.SecurityOpt` and passed to Docker. Different Kubernetes runtimes now are using different methods to parse and format the security option strings: +* method `#getSecurityOpts` in `docker_mager_xxxx.go` for Docker runtime +* method `#getContainerSecurityOpts` in `docker_container.go` for CRI + + +##Proposal to support "no new privileges" + +To support "no new privileges" options in Kubernetes, it is proposed to make the following changes: + +###Changes of SecurityContext objects + +Add a new bool type field named `noNewPrivileges` to both `SecurityContext` definition and `PodSecurityContext` definition: +* `noNewPrivileges=true` in `PodSecurityContext` means that all the containers in the pod should be run with `no-new-privileges` enabled. This should be a pod level control of `no-new-privileges` flag. +* `noNewPrivileges` in `SecurityContext` is a container level control of `no-new-privileges` flag, and can override the pod level `noNewPrivileges` setting. + +By default, `noNewPrivileges` is `false`. It would break anything with a suid bit (e.g. ping). Unfortunately this probably breaks enough containers that we can't enable it by default, but we can document that it's a best practice to set it to false in the PodSecurityPolicy. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#180>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGvIKCL4h36I-2JskKm6JKMdSthZ3V68ks5rPXHkgaJpZM4LNwUY> .

[thockin]

I want to echo @euank : "Docker supports feature X" is not sufficient reason for Kubernetes to support it. This seems like a reasonable thing to USE but we should think hard about how we EXPOSE it.

Also, am I mis-reading or is this just a fancy form of the NOSUID moount flag?

[timstclair]

@thockin It's more than just NOSUID. For instance, it can be used to grant a container permission to further restrict child processes through seccomp, without also granting the ability unrestrict child processes. See section 5.7 "A World Without Root" of https://www.nccgroup.trust/us/our-research/understanding-and-hardening-linux-containers/ which was the original inspiration for filing this issue.

That said, maybe the effect of NOSUID is the only one we need to worry about with regards to breaking containers?

[lucab]

Another datapoint in here: NNP may have some implications on (ambient) capabilities as well, discussion happening in runtime-spec opencontainers/runtime-spec#668.

[smarterclayton]

We don't use negative names, and I agree with Tim that this is low level. I'd suggest allowPrivilegeEscalation or something that uses a word to capture the appropriate level of risk. We should not default this in the API, but at the kubelet, and use podsecuritypolicy to set appropriate defaults (which is how we treat these other flags).

[smarterclayton]

I agree with vish, this is not an action we would expose through kubectl. It's probably a candidate for "kubectl set security-context" or something similar though.

[yujuhong]

We are in the middle of migrating to the CRI implementation in 1.6, if everything goes well (fingers crossed). In the meantime, I would like to freeze development in the old implementation (pkg/kubelet/dockertools) if possible. I would suggest removing all sections referring the old implementation from this proposal.

p.s. I recently moved all docker-specific code from pkg/securitycontext into pkg/kubelet/dockertools/securitycontext, so the link @pweil- gave above is outdated.

[yujuhong]

nit: I prefer a higher-level description since the code (e.g., the function/file names) changes frequently.

I think you need to add a new field, e.g., AllowPrivilegeEscalation in the CRI LinuxSandboxSecurityContext. Without the API change, kubelet can't tell the runtime to do this.

[yujuhong]

+1. Please do not expose this directly in kubectl run.

[jessfraz]

@xingzhou can you find time to update this proposal?

[jessfraz]

If not I can carry for you :)

[jessfraz]

I carried this and addressed all the comments and added information with regard to interactions with other linux primitives here: #639

@xingzhou if you want to take this back over please let me know.

[xingzhou]

@jessfraz, sorry for the late response, I'm now not working on this issue, feel free to take over it, thank you!

[jessfraz]

no worries, thanks for all the work you put into this!!