Add support for `no_new_privs` via AllowPrivilegeEscalation by jessfraz · Pull Request #47019 · kubernetes/kubernetes

jessfraz · 2017-06-06T03:49:25Z

What this PR does / why we need it:
Implements kubernetes/community#639
Fixes #38417

Adds AllowPrivilegeEscalation and DefaultAllowPrivilegeEscalation to PodSecurityPolicy.
Adds AllowPrivilegeEscalation to container SecurityContext.

Adds the proposed behavior to kuberuntime, dockershim, and rkt. Adds a bunch of unit tests to ensure the desired default behavior and that when DefaultAllowPrivilegeEscalation is explicitly set.

Tests pass locally with docker and rkt runtimes. There are also a few integration tests with a setuid binary for sanity.

Release note:

Adds AllowPrivilegeEscalation to control whether a process can gain more privileges than it's parent process

dims · 2017-06-06T11:37:22Z

@jessfraz : sorry for a dumb question. Is this feature meant to only be active/available when the kubelet --allow-privileged is true?

jessfraz · 2017-06-06T11:40:03Z

@dims there is a chart in the proposal that might help, no_new_privs will be added when allowPrivilegeEscalation is false, by default this will be when a container is not privileged, adding CAP{_SYS_ADMIN, or when the uid!=0

dims · 2017-06-06T12:03:04Z

@jessfraz got it. thanks!

jessfraz · 2017-06-06T19:37:52Z

test/e2e_node/image_list.go

 	"gcr.io/google_containers/nginx-slim:0.7",
 	"gcr.io/google_containers/serve_hostname:v1.4",
 	"gcr.io/google_containers/netexec:1.7",
+	"r.j3ss.co/no_new_privs:latest",


I will switch this to google_containers before merge don't worry

fixed the image to be on google container registry

pweil- · 2017-06-06T19:59:55Z

@php-coder

spxtr · 2017-07-24T17:59:34Z

/lint

k8s-ci-robot

@spxtr: 7 warnings.

Details

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2017-07-24T18:01:20Z

pkg/api/v1/conversion.go


 	return nil
 }
+func Convert_api_SecurityContext_To_v1_SecurityContext(in *api.SecurityContext, out *v1.SecurityContext, s conversion.Scope) error {


Golint naming: don't use underscores in Go names; func Convert_api_SecurityContext_To_v1_SecurityContext should be ConvertAPISecurityContextToV1SecurityContext. More info.

k8s-ci-robot · 2017-07-24T18:01:20Z

pkg/apis/extensions/v1beta1/conversion.go

 	return nil
 }
+
+func Convert_extensions_PodSecurityPolicySpec_To_v1beta1_PodSecurityPolicySpec(in *extensions.PodSecurityPolicySpec, out *extensionsv1beta1.PodSecurityPolicySpec, s conversion.Scope) error {


Golint naming: don't use underscores in Go names; func Convert_extensions_PodSecurityPolicySpec_To_v1beta1_PodSecurityPolicySpec should be ConvertExtensionsPodSecurityPolicySpecToV1beta1PodSecurityPolicySpec. More info.

k8s-ci-robot · 2017-07-24T18:01:20Z

staging/src/k8s.io/api/core/v1/types.generated.go

-		yyb14 = yyj14 > l
+	var yyj16 int
+	var yyb16 bool
+	var yyhl16 bool = l >= 0


Golint type-inference: should omit type bool from declaration of var yyhl16; it will be inferred from the right-hand side.

k8s-ci-robot · 2017-07-24T18:01:20Z

pkg/kubelet/lifecycle/handlers.go

 	}
 }
+
+func NewNoNewPrivsAdmitHandler(runtime kubecontainer.Runtime) PodAdmitHandler {


Golint comments: exported function NewNoNewPrivsAdmitHandler should have comment or be unexported. More info.

k8s-ci-robot · 2017-07-24T18:01:21Z

pkg/securitycontext/util_test.go

+
+func TestAddNoNewPrivileges(t *testing.T) {
+	var nonRoot int64 = 1000
+	var root int64 = 0


Golint zero-value: should drop = 0 from declaration of var root; it is the zero value.

k8s-ci-robot · 2017-07-24T18:01:21Z

pkg/kubelet/apis/cri/v1alpha1/runtime/api.pb.go

 	return ""
 }

+func (m *LinuxContainerSecurityContext) GetNoNewPrivs() bool {


Golint comments: exported method LinuxContainerSecurityContext.GetNoNewPrivs should have comment or be unexported. More info.

k8s-ci-robot · 2017-07-24T18:01:21Z

staging/src/k8s.io/api/extensions/v1beta1/types.generated.go

-		yyb28 = yyj28 > l
+	var yyj32 int
+	var yyb32 bool
+	var yyhl32 bool = l >= 0


Golint type-inference: should omit type bool from declaration of var yyhl32; it will be inferred from the right-hand side.

spxtr · 2017-07-24T18:02:43Z

Lint comments on generated code aren't too useful 😞

jessfraz · 2017-07-28T15:07:16Z

this is all updated and green :)

tallclair · 2017-07-28T17:35:18Z

/lgtm

jessfraz · 2017-07-29T00:40:37Z

/retest

…

On Jul 28, 2017 20:29, "k8s-ci-robot" ***@***.***> wrote: @jessfraz <https://github.com/jessfraz>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-kops-aws a5e4c6f <a5e4c6f> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/47019/pull-kubernetes-e2e-kops-aws/37905/> /test pull-kubernetes-e2e-kops-aws Full PR test history <https://k8s-gubernator.appspot.com/pr/47019>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/jessfraz>. Please help us cut down on flakes by linking to <https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://github.com/kubernetes/test-infra/blob/master/commands.md>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47019 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYNbAH6PtUVf8iRfE-Y0MPavb0NcJfuks5sSnz8gaJpZM4Nw1Aj> .

jessfraz · 2017-07-31T15:00:23Z

ping @smarterclayton for approval 😇

smarterclayton · 2017-07-31T19:10:52Z

API approved as per the proposal and discussion on the thread.

/approve

k8s-github-robot · 2017-07-31T19:11:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jessfraz, smarterclayton, tallclair

Associated issue: 639

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

Details

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [smarterclayton]
~~docs/OWNERS~~ [smarterclayton]
~~federation/OWNERS~~ [smarterclayton]
~~pkg/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/api/OWNERS~~ [smarterclayton]
~~test/OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jessfraz · 2017-07-31T19:36:14Z

/test pull-kubernetes-bazel

k8s-github-robot · 2017-07-31T23:56:56Z

Automatic merge from submit-queue (batch tested with PRs 49651, 49707, 49662, 47019, 49747)

mkumatag · 2017-08-01T05:05:49Z

test/images/nonewprivs/nnp.c

@@ -0,0 +1,22 @@
+// Copyright 2017 The Kubernetes Authors.


Is there any specif reason why we are using C code? I see even os library in go has Geteuid() which can be used.

Does it matter?

Yes. For multi arch test images we are planning to have all images under kubernetes/tests/images directory for supported architectures like amd64, arm, arm64, ppc64le. So its always very easy to cross build the go code over a C code.

grodrigues3 · 2017-08-16T17:47:43Z

Which sig does this fall under?

jessfraz · 2017-08-16T17:56:22Z

sig-auth

…

On Aug 16, 2017 13:48, "grodrigues3" ***@***.***> wrote: Which sig does this fall under? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47019 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYNbK92W_zltYwO7qZXMwUV2LuPWlcsks5sYyuFgaJpZM4Nw1Aj> .

jessfraz · 2017-08-29T12:58:10Z

Sure thanks

…

On Aug 29, 2017 08:56, "Vyacheslav Semushin" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/apis/extensions/v1beta1/types.go <#47019 (comment)> : > @@ -933,6 +933,15 @@ type PodSecurityPolicySpec struct { // host paths may be used. // +optional AllowedHostPaths []string `json:"allowedHostPaths,omitempty" protobuf:"bytes,15,opt,name=allowedHostPaths"` + // DefaultAllowPrivilegeEscalation controls the default setting for whether a + // process can gain more privileges than it's parent process. + // // +optional @jessfraz <https://github.com/jessfraz> I see that s/it's parent/its parent/ wasn't fixed. Do you want me to prepare PR for that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47019 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYNbBeUnZqwIVlGeDLR3xA0vePnRL7_ks5sdAqSgaJpZM4Nw1Aj> .

php-coder · 2017-08-29T14:13:35Z

pkg/security/podsecuritypolicy/provider_test.go

+func TestValidateAllowPrivilegeEscalation(t *testing.T) {
+	pod := defaultPod()
+	pe := true
+	pod.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation = &pe


I see that tests don't cover that case of using InitContainers? Does this validation work for them? I assume that it should but I don't see any mentions about them in the code.

php-coder · 2017-08-29T14:19:06Z

pkg/kubelet/lifecycle/handlers.go

+	return PodAdmitResult{Admit: true}
+}
+
+func noNewPrivsRequired(pod *v1.Pod) bool {


@jessfraz Should we handle InitContainers here too? Looks like, yes, we should.

Are init containers handled differently that should probably be documented somewhere so it's not overlooked unless I missed it

As far I understand, the most of the code from this PR is handling initContainers correctly because it works only with securityContext. The existing infrastructure is aware about containers/initContainers and executes validation on their SecurityContexts.

But when we're manually inspecting PodSpec we obligated to take care about initContainers. In this particular case, I see that noNewPrivsRequired() will return false for a pod that has initContainers with allowPrivielegeEscalation=false. I'm not sure what will be the consequence of this, perhaps allowPrivielegeEscalation=false won't work for a pod?

php-coder · 2017-08-29T14:32:44Z

staging/src/k8s.io/api/core/v1/types.go

+	// the no_new_privs flag will be set on the container process.
+	// AllowPrivilegeEscalation is true always when the container is:
+	// 1) run as Privileged
+	// 2) has CAP_SYS_ADMIN


@jessfraz Are we missing the case "when a container is not run as root" here?

We got rid of that functionality

And the proposal is outdated now? https://github.com/kubernetes/community/blob/f149433600ff1795521e7d9f2ae8ff3162394c61/contributors/design-proposals/no-new-privs.md#changes-of-securitycontext-objects

Also some tests can be deleted then:

kubernetes/pkg/securitycontext/util_test.go

Lines 203 to 212 in 28f6b3f

"allowPrivilegeEscalation nil nonRoot": {

sc: v1.SecurityContext{

RunAsUser: &nonRoot,

},

},

"allowPrivilegeEscalation nil root": {

sc: v1.SecurityContext{

RunAsUser: &root,

},

},

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 6, 2017

k8s-github-robot assigned saad-ali and mbohlool Jun 6, 2017

jessfraz force-pushed the allowPrivilegeEscalation branch from dbcf72c to 7813f76 Compare June 6, 2017 04:10

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2017

jessfraz added this to the v1.8 milestone Jun 6, 2017

jessfraz force-pushed the allowPrivilegeEscalation branch from f948d60 to d4cac92 Compare June 6, 2017 10:33

jessfraz force-pushed the allowPrivilegeEscalation branch from d4cac92 to 4e223b6 Compare June 6, 2017 11:49

jessfraz force-pushed the allowPrivilegeEscalation branch 4 times, most recently from 12d5ddc to 8e1a8fa Compare June 6, 2017 18:41

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2017

jessfraz force-pushed the allowPrivilegeEscalation branch from 8e1a8fa to 4b159e1 Compare June 6, 2017 19:03

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2017

jessfraz force-pushed the allowPrivilegeEscalation branch from 4b159e1 to a8c7682 Compare June 6, 2017 19:19

jessfraz requested review from smarterclayton, timstclair, vishh and yujuhong June 6, 2017 19:20

jessfraz commented Jun 6, 2017

View reviewed changes

jessfraz requested a review from pweil- June 6, 2017 19:40

k8s-ci-robot reviewed Jul 24, 2017

View reviewed changes

k8s-ci-robot assigned tallclair Jul 28, 2017

mkumatag reviewed Aug 1, 2017

View reviewed changes

mkumatag mentioned this pull request Aug 1, 2017

Multiarch nonewprivs test image #49951

Merged

Random-Liu mentioned this pull request Aug 21, 2017

Update kubernetes containerd/cri#148

Merged

php-coder mentioned this pull request Aug 29, 2017

Pod Security Policy doc is missing AllowPrivilegeEscalation parameter kubernetes/website#5239

Closed

2 tasks

php-coder reviewed Aug 29, 2017

View reviewed changes

php-coder mentioned this pull request Aug 29, 2017

Fix typo in API docs #51536

Merged

jessfraz mentioned this pull request Sep 14, 2017

add docs for AllowPrivilegeEscalation kubernetes/website#5448

Merged

mikkeloscar mentioned this pull request Oct 4, 2017

kubectl logs panics when container has allowPrivilegeEscalation: false #53370

Closed

jonpulsifer mentioned this pull request Oct 12, 2017

Create Audit for AllowPrivilegeEscalation Shopify/kubeaudit#23

Closed

php-coder mentioned this pull request Jan 8, 2018

pkg/securitycontext/util_test.go(TestAddNoNewPrivileges): update tests #57970

Merged

php-coder mentioned this pull request Mar 28, 2018

Add SCC.AllowPrivilegeEscalation (aka no-new-privs) openshift/origin#18398

Closed

AlexeyPerevalov mentioned this pull request Sep 30, 2021

Fix CAP_SYS_ADMIN verification #105237

Closed

liggitt mentioned this pull request Jun 29, 2023

Inconsistency in' securityContext.privileged and allowPrivilegeEscalation behaviors #118822

Closed

	"allowPrivilegeEscalation nil nonRoot": {
	sc: v1.SecurityContext{
	RunAsUser: &nonRoot,
	},
	},
	"allowPrivilegeEscalation nil root": {
	sc: v1.SecurityContext{
	RunAsUser: &root,
	},
	},

Conversation

jessfraz commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dims commented Jun 6, 2017

Uh oh!

jessfraz commented Jun 6, 2017

Uh oh!

dims commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pweil- commented Jun 6, 2017

Uh oh!

spxtr commented Jul 24, 2017

Uh oh!

k8s-ci-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spxtr commented Jul 24, 2017

Uh oh!

jessfraz commented Jul 28, 2017

Uh oh!

tallclair commented Jul 28, 2017

Uh oh!

jessfraz commented Jul 29, 2017 via email

Uh oh!

jessfraz commented Jul 31, 2017

Uh oh!

smarterclayton commented Jul 31, 2017

Uh oh!

k8s-github-robot commented Jul 31, 2017

Uh oh!

jessfraz commented Jul 31, 2017

Uh oh!

k8s-github-robot commented Jul 31, 2017

Uh oh!

mkumatag Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grodrigues3 commented Aug 16, 2017

Uh oh!

jessfraz commented Aug 16, 2017 via email

Uh oh!

jessfraz commented Aug 29, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jessfraz commented Jun 6, 2017 •

edited

Loading

mkumatag Aug 1, 2017 •

edited

Loading