Hardening container #254
Hardening container #254
Conversation
- No longer runs as root - Runtime image is now "from scratch" - Standard principle of least privilege security caps in deployment manifest
ack-bot
added
the
do-not-merge/work-in-progress
ack-bot
added
the
needs-ok-to-test
|
Hi @philnichol. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/ok-to-test |
ack-bot
added
ok-to-test
vijtrip2
left a comment
vijtrip2
left a comment
There was a problem hiding this comment.
one single comment, otherwise looks good (I will test it locally once just for proof and then approve :) )
vijtrip2
left a comment
vijtrip2
left a comment
There was a problem hiding this comment.
This is excellent stuff! Thank you!
I tested it locally by building ecr-controller image, installing using new helm chart template, creating an ecr repository and deleting it. Everything works fine! :)
I will let another ACK team member give this a look as well but LGTM.
P.S. two more files need update, Dockerfile.local and templates/config/controller/deployment.yaml.tpl but I can handle that in separate PR.
| @@ -1,5 +1,11 @@ | |||
| # Base image to use for the final stage | |||
| ARG base_image=public.ecr.aws/amazonlinux/amazonlinux:2 | |||
There was a problem hiding this comment.
nit: Please remove this base_image arg as it is no longer used.
|
/ok-to-test |
|
thanks @vijtrip2 ! Have addressed your feedback and included the files you mentioned. Also just realised since this is security-related it probably should have been raised via a more private medium, apologies for that, won't happen again |
|
/ok-to-test |
ack-bot
removed
the
do-not-merge/work-in-progress
|
/retest |
jaypipes
left a comment
jaypipes
left a comment
There was a problem hiding this comment.
Firstly, @philnichol thank you so much for your PR and for raising awareness of this important topic. I really appreciate it!
That said, I'd like to request that for this particular PR, we remove any changes to the Dockerfiles and only update the deployment.yaml files/templates.
I've just had a conversation with @micahhausler (who is my go-to person to talk about container build/toolchain security) and I'd like ACK to move away from the fat AL2 container image base and into the distroless-like EKS-D minimal AL2-based base non-root container image available here:
https://gallery.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-nonroot
(source code for the above image is here: https://github.com/aws/eks-distro-build-tooling/tree/main/eks-distro-base)
The reason I'd like to do this is twofold:
-
The EKS-D/EKS-Anywhere team has expertise in build toolchain provenance and security and all aspects of the EKS-D build toolchain are audited and owned by AMZN
-
We (the ACK team) should take advantage of the work that the EKS-D team has done over the last 2 years and reduce our overall surface area (for both supply-chain attacks as well as simply the amount of work it takes to keep a build chain up to date and secure).
Therefore, @philnichol if we could slim this particular PR down to just the changes to the deployment manifests (which are awesome, BTW!) and tackle the refactoring of the base image in a separate PR, that would be great. Thank you!
|
@jaypipes thanks for the feedback! Great point, makes sense to use the work that you guys already have on minimal container images 😄 , I've removed the Dockerfiles from this PR |
|
/lgtm thanks @philnichol |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jaypipes, philnichol, vijtrip2 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 |
4 participants
Issue #, if available:
Relates aws-controllers-k8s/community#1112
Description of changes:
No longer runs as root, runs as nobody instead, since runtime is from scratch I've added a "dummy" /etc/shadow fileRuntime image is now "from scratch" since we don't need much other than ca-certs and the binary itself (eg. curl, vim, etc)This is a draft since there's still stuff missing, and not sure if you would want to go in a different direction
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.