Run all containers as non-root user #3237

bradhoekstra · 2019-02-15T15:55:17Z

Related to #3001

Proposed Changes

Run all containers as a non-root user

Release Note

Serving containers no longer run as root

knative-prow-robot

@bradhoekstra: 0 warnings.

In response to this:

Related to #3001

Proposed Changes

Run all containers as a non-root user

Release Note
Serving containers no longer run as root

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.

bradhoekstra · 2019-02-15T16:41:36Z

/assign @mattmoor

vagababov

Should there be an integration test validating this? Akin to contract e2e tests?

pkg/reconciler/v1alpha1/revision/resources/queue.go

config/controller.yaml

bradhoekstra · 2019-02-15T18:49:33Z

Should there be an integration test validating this? Akin to contract e2e tests?

That seems like a good idea, I can file an issue to follow up on that.

config/webhook.yaml

bradhoekstra · 2019-02-15T19:33:19Z

/hold

config/activator.yaml

config/autoscaler.yaml

config/webhook.yaml

bradhoekstra · 2019-02-25T20:35:09Z

I brought up the idea of building/publishing a nonroot image directly in distroless, but it was turned down: GoogleContainerTools/distroless#306.

If we want a nonroot base image, it looks like we will have to build and publish that ourselves.

pmorie · 2019-03-05T13:47:09Z

Does ko have a facility for controlling image metadata?

mattmoor · 2019-03-06T04:27:06Z

@pmorie No, I'd expect ko to inherit this from the base image.

pmorie · 2019-05-23T15:21:29Z

Circling back on this - I am not sure where to take things from here. @mattmoor do you have any idea whether GoogleContainerTools/distroless#235 will be actionable?

dprotaso · 2019-05-29T19:16:17Z

@pmorie Action taken GoogleContainerTools/distroless#368

dprotaso · 2019-06-19T20:20:07Z

Bump @bradhoekstra GoogleContainerTools/distroless#368 merged so you should have non-root distroless images to leverage now.

If you've moved onto other things let us know and I can close/redo this PR.

bradhoekstra · 2019-06-19T20:41:44Z

I can update this PR to use the new tags.

.ko.yaml

bradhoekstra · 2019-06-20T21:33:40Z

Rebase and try again...

bradhoekstra · 2019-06-20T21:38:51Z

The only thing that I can think may be happening is that another test run is uploading to the same repository and colliding with this one creating a race.

Can the boskos project be shared by multiple jobs at once?

mattmoor · 2019-06-21T00:33:25Z

/retest

mattmoor · 2019-06-21T03:36:11Z

@bradhoekstra the failure looks legit:

service.go:141: Successfully created Service should-run-as-user-container-default-giwossyq
user_test.go:93: uid = 0, want: 65532
user_test.go:99: euid = 0, want: 65532

mattmoor · 2019-06-21T03:45:57Z

This is puzzling because if I deploy this image with the .ko.yaml I get:

		"user": {
			"uid": 65532,
			"euid": 65532,
			"gid": 65532,
			"egid": 65532
		}

mattmoor · 2019-06-21T04:12:32Z

/retest

mattmoor · 2019-06-21T04:43:56Z

FWIW, this passed for me locally, so I'm puzzled. One thing that comes to mind is that maybe .ko.yaml was ignored, but IDK how the prior base image would have been picked up...

markusthoemmes

This looks awesome throughout! I've got one question left regarding the securityContext and one small nit.

pkg/reconciler/revision/resources/queue.go

markusthoemmes · 2019-06-21T07:02:59Z

config/controller.yaml

@@ -64,6 +64,8 @@ spec:
          value: config-observability
        - name: METRICS_DOMAIN
          value: knative.dev/serving
+        securityContext:
+          allowPrivilegeEscalation: false


Can you elaborate on why this is needed to be set explicitly?

This stops the user from being able to sudo or otherwise elevate privileges, without it starting the container as a non-root user does not mean the user cannot become root afterwards.

A good explanation is here: https://kubernetes.io/docs/concepts/policy/pod-security-policy/#privilege-escalation

bradhoekstra · 2019-06-21T11:51:44Z

Somehow the tests passed now, maybe it was an issue due to project sharing?

bradhoekstra · 2019-06-21T11:59:02Z

A thought: if the integration problem is due to the project sharing then merging this change may cause all sort of issues for PR integration tests until all PRs have picked up this change.

Change-Id: I77370d1f198276f59e6033d8515d2c438c8f47c7

mattmoor

/lgtm

I don't think that's the problem, I suspect that we should log the raw data payload for debugging...

mattmoor · 2019-06-21T14:00:43Z

/approve

knative-prow-robot · 2019-06-21T14:00:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bradhoekstra, mattmoor

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:

~~OWNERS~~ [mattmoor]

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

bradhoekstra · 2019-06-21T17:41:08Z

/retest

bradhoekstra · 2019-06-21T17:48:22Z

/retest

bradhoekstra · 2019-06-21T18:11:05Z

/retest

mattmoor · 2019-06-21T18:37:33Z

/retest

yu2003w · 2019-06-22T07:33:52Z

when deploying latest serving code from master branch, encounter errors as below,

Warning  Failed     7s (x4 over 22s)  kubelet, 172.16.17.35  Error: container has runAsNonRoot and image has non-numeric user (nonroot), cannot verify user is non-root

In my environment, PodSecurityPolicy is enabled.

yu2003w · 2019-06-22T10:51:50Z

.ko.yaml

-baseImageOverrides:
-        github.com/knative/serving/test/test_images/runtime-unprivileged: jenkins/jenkins:lts-slim
+# Use :nonroot base image for all containers
+defaultBaseImage: gcr.io/distroless/static:nonroot


If PodSecurityPolicy is enabled, this base image doesn't work.
Hit error as

"Warning Failed 7s (x4 over 22s) kubelet, 172.16.17.35 Error: container has runAsNonRoot and image has non-numeric user (nonroot), cannot verify user is non-root"

mattmoor · 2019-06-22T15:43:03Z

Sounds like we should enable PodSecurityPolicy in our test setup.

GoogleContainerTools/distroless#375 should fix this.

dprotaso · 2019-06-24T13:53:14Z

@yu2003w what's your exact pod security policy?

yu2003w · 2019-06-24T14:10:00Z

@dprotaso I have submitted PR.#4500

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 15, 2019

knative-prow-robot reviewed Feb 15, 2019

View reviewed changes

knative-prow-robot requested review from dgerd and tcnghia February 15, 2019 15:55

bradhoekstra marked this pull request as ready for review February 15, 2019 16:41

knative-prow-robot assigned mattmoor Feb 15, 2019

vagababov reviewed Feb 15, 2019

View reviewed changes

dgerd reviewed Feb 15, 2019

View reviewed changes

pkg/reconciler/v1alpha1/revision/resources/queue.go Outdated Show resolved Hide resolved

config/controller.yaml Outdated Show resolved Hide resolved

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 15, 2019

mattmoor reviewed Feb 15, 2019

View reviewed changes

config/webhook.yaml Show resolved Hide resolved

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2019

dprotaso reviewed Feb 16, 2019

View reviewed changes

config/activator.yaml Outdated Show resolved Hide resolved

pmorie reviewed Feb 18, 2019

View reviewed changes

config/activator.yaml Outdated Show resolved Hide resolved

config/autoscaler.yaml Show resolved Hide resolved

config/webhook.yaml Show resolved Hide resolved

bradhoekstra mentioned this pull request Feb 20, 2019

Publish :nonroot versions of images GoogleContainerTools/distroless#306

Closed

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 19, 2019

bradhoekstra force-pushed the non-root branch from 34f699c to 65fc0bf Compare June 19, 2019 21:10

knative-prow-robot added the area/API API objects and controllers label Jun 19, 2019

jonjohnsonjr reviewed Jun 19, 2019

View reviewed changes

.ko.yaml Outdated Show resolved Hide resolved

bradhoekstra force-pushed the non-root branch from 65fc0bf to dc9592c Compare June 19, 2019 21:51

bradhoekstra force-pushed the non-root branch from cb55835 to c6ca957 Compare June 20, 2019 21:33

markusthoemmes reviewed Jun 21, 2019

View reviewed changes

markusthoemmes mentioned this pull request Jun 21, 2019

Added proper PodSecurityPolicy, role, rolebinding to run e2e tests in namespace serving-tests #4278

Closed

Use pkg.ptr

1535f2b

Change-Id: I77370d1f198276f59e6033d8515d2c438c8f47c7

mattmoor reviewed Jun 21, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2019

knative-prow-robot merged commit aa48603 into knative:master Jun 21, 2019

yu2003w reviewed Jun 22, 2019

View reviewed changes

mattmoor mentioned this pull request Jun 22, 2019

Enable PodSecurityPolicy in our test environment #4480

Closed

yu2003w mentioned this pull request Jun 22, 2019

e2e/TestMinScale failed due to lack of proper psp when admission controller is enabled #4276

Closed

Run all containers as non-root user #3237

Run all containers as non-root user #3237

Conversation

bradhoekstra commented Feb 15, 2019

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

bradhoekstra commented Feb 15, 2019

vagababov left a comment

Choose a reason for hiding this comment

bradhoekstra commented Feb 15, 2019

bradhoekstra commented Feb 15, 2019

bradhoekstra commented Feb 25, 2019

pmorie commented Mar 5, 2019

mattmoor commented Mar 6, 2019

pmorie commented May 23, 2019

dprotaso commented May 29, 2019

dprotaso commented Jun 19, 2019

bradhoekstra commented Jun 19, 2019

bradhoekstra commented Jun 20, 2019

bradhoekstra commented Jun 20, 2019

mattmoor commented Jun 21, 2019

mattmoor commented Jun 21, 2019

mattmoor commented Jun 21, 2019

mattmoor commented Jun 21, 2019

mattmoor commented Jun 21, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

markusthoemmes Jun 21, 2019

Choose a reason for hiding this comment

bradhoekstra Jun 21, 2019

Choose a reason for hiding this comment

bradhoekstra commented Jun 21, 2019

bradhoekstra commented Jun 21, 2019

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor commented Jun 21, 2019

knative-prow-robot commented Jun 21, 2019

bradhoekstra commented Jun 21, 2019

bradhoekstra commented Jun 21, 2019

bradhoekstra commented Jun 21, 2019

mattmoor commented Jun 21, 2019

yu2003w commented Jun 22, 2019 • edited Loading

yu2003w Jun 22, 2019

Choose a reason for hiding this comment

mattmoor commented Jun 22, 2019

dprotaso commented Jun 24, 2019 • edited Loading

yu2003w commented Jun 24, 2019

yu2003w commented Jun 22, 2019 •

edited

Loading

dprotaso commented Jun 24, 2019 •

edited

Loading