Skip to content

Conversation

@vijtrip2
Copy link
Contributor

@vijtrip2 vijtrip2 commented Jan 13, 2022

Description of changes:

Before

  • OpenShift enforces nonRoot security posture by running container images using random UID and does not allow specifying runAsUser field in deployment spec.
  • Before this change, ACK generated artifacts enforced nonRoot security posture using SecurityContext's runAsUser and runAsNonRoot field, while the container image ran as root by default

Now

  • With this change, ACK controller images now run by default with non-root User(1000)
  • Removed runAsUser field from deployment templates. This change keeps the existing functionality because K8s pod inherit UserId from Image metadata and ACK controller still runs with non-root user(1000) [Tested and Validated]

runAsUser: "The UID to run the entrypoint of the container process. Defaults to user specified in image metadata if unspecified. " - Official Documentation

  • Removing runAsUser unblock OpenShift installation and allows OpenShift to run container using random UserId
  • Keeping runAsNonRoot field, keeps the validation in place that ACK controller does not run as root. This validation helps security posture for both OpenShift and helm/kustomize installation.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@vijtrip2
Copy link
Contributor Author

/cc @acornett21

@ack-bot
Copy link
Collaborator

ack-bot commented Jan 13, 2022

@vijtrip2: GitHub didn't allow me to request PR reviews from the following users: acornett21.

Note that only aws-controllers-k8s members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @acornett21

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.

@acornett21
Copy link
Contributor

@vijtrip2 I'm not sure OpenShift allows this, I think the deployment/container are a bonded pair and a change in either would require an SecurityContextConstraints to exist to allow this.

@vijtrip2
Copy link
Contributor Author

@vijtrip2 I'm not sure OpenShift allows this, I think the deployment/container are a bonded pair and a change in either would require an SecurityContextConstraints to exist to allow this.

I saw post about bitnami non-root container that runs on open-shift and they are built as default non-root images. I have image for you that you can use to test this locally. Following up offline on slack.

@acornett21
Copy link
Contributor

@vijtrip2 It seems in this case the UID that OpenShift generates for the namespace does get propigated all the way to the node -> pod -> container and having User 1000 in the container does not override that anywhere, which is what I was concerned of. I think unblocks us for now and we can iterate on this later if need be.

Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for the deep dive on this @vijtrip2!

@jaypipes
Copy link
Collaborator

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2022
@ack-bot
Copy link
Collaborator

ack-bot commented Jan 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, jaypipes, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-bot ack-bot merged commit 0057599 into aws-controllers-k8s:main Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants