Skip to content
This repository has been archived by the owner on Jul 7, 2023. It is now read-only.

feat(Dockerfile): Run as nonroot user #32

Merged
merged 4 commits into from
Jun 4, 2020

Conversation

jchauncey
Copy link

@jchauncey jchauncey commented Jun 2, 2020

This pr moves the final image from scratch to distroless/static:nonroot

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 2, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @jchauncey!

It looks like this is your first PR to kubernetes-sigs/dashboard-metrics-scraper 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/dashboard-metrics-scraper has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot requested review from floreks and jeefy June 2, 2020 15:47
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 2, 2020
@jchauncey
Copy link
Author

still need to test that this actually works

@jeefy
Copy link
Member

jeefy commented Jun 2, 2020

Unfortunately this won't work with scratch because of the file permissions with tmp (just verified)

That said, if you change the base image from "scratch" to "distroless", this should work:

FROM gcr.io/distroless/base AS final

I think that change on top of your user change will shore things up. :)

@jchauncey
Copy link
Author

yeah i tried starting it locally and noticed the tmp error. We could just use distroless:nonroot

@jchauncey
Copy link
Author

Ok i updated to use nonroot distroless which seems it may have fixed the tmp issue.

@jchauncey
Copy link
Author

fixes #16

@jeefy
Copy link
Member

jeefy commented Jun 2, 2020

This is great, thanks for tackling this!

/approve
/assign @maciaszczykm @floreks

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jchauncey, jeefy

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2020
@floreks
Copy link
Contributor

floreks commented Jun 2, 2020

Is there any actual reason to use distroless instead of scratch? The container itself does not enforce privileges. We do restrict container privileges in our deployment files already.

https://github.com/kubernetes/dashboard/blob/5a289e51a60958bafacb5266dc602ca4bb565806/aio/deploy/recommended.yaml#L288-L292

@jchauncey
Copy link
Author

Our customers (aks) are having this clusters scanned with the twistlock tool and its flagging this image (along with a few others) as being created with a nonroot user. Even though you set the user at runtime that isnt picked up by the scanning tool and I believe this is failing the scan as its marked a high severity.

@jeefy
Copy link
Member

jeefy commented Jun 2, 2020 via email

@floreks
Copy link
Contributor

floreks commented Jun 2, 2020

Why I like scratch is because it does not add any additional layers to the final image. It is basically an empty image with just the binary. Distroless images have all this additional stuff that we do not need and will most probably never need. If some static check fails because it cannot see nonroot user inside dockerfile then we can simply add passwd file and change user without changing the base image. We've already had this discussion inside dashboard repository kubernetes/dashboard#4301.

@sozercan
Copy link

sozercan commented Jun 2, 2020

distroless/static only have these components: https://github.com/GoogleContainerTools/distroless/blob/master/base/README.md

  • ca-certificates and /tmp are already being used here
  • as you mentioned, /etc/passwd for adding (nonroot) user so the container itself doesn't get flagged

@floreks
Copy link
Contributor

floreks commented Jun 3, 2020

We don't really need ca-certificates or tmp dir already inside the image. I'd only add passwd to the current Dockerfile, then change user and leave it as is.

@jchauncey
Copy link
Author

I must be doing something wrong then because I cant seem to get that to work. It complains about writing to /tmp.

@floreks
Copy link
Contributor

floreks commented Jun 3, 2020

tmp dir is handled by our yaml deployment file.

* also removes /tmp and ca-certificates from the final image
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 3, 2020
@jchauncey
Copy link
Author

Ok new commit that removes /tmp and ca-certificates as well as creates a user in the builder stage and copies it to the final image. went ahead and pushed this commit to see if people are ok with this approach. still need to verify it works

@jchauncey
Copy link
Author

Ok i verified that the pod starts and runs with no errors in the logs with the latest commit

@floreks
Copy link
Contributor

floreks commented Jun 4, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit 5bfe3a1 into kubernetes-sigs:master Jun 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants