Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set SCC's RunAsUser as MustRunAsRange #254

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sigv
Copy link

@sigv sigv commented Mar 8, 2024

Proposed changes

Upstream Helm Chart is removing explicit runAsUser value from the Deployment and DaemonSet resources. This practically means the UID will be inherited from image's Dockerfile.

Users on vanilla Kubernetes clusters will not observe a change in behavior, unless they have exotic configurations.

However, OpenShift does have additional security measures. It suggests using randomized UIDs/GIDs for workloads. To enable this, the custom Security Context Constraint resources are being updated. The MustRunAsRange policy is utilized with pre-allocated values (no explicit range min/max), which effectively allows OpenShift to pick its own ranges.

Relates to nginxinc/kubernetes-ingress#5422.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@sigv sigv requested a review from a team as a code owner March 8, 2024 01:49
@sigv
Copy link
Author

sigv commented Mar 8, 2024

My test was very basic local run.

Just deployed a fresh OpenShift cluster (v4.15.0) in AWS, and then essentially

kubectl apply -f resources/scc.yaml
make install
make run

# for some reason had to manually create the ClusterRole
# should I be worried about this? but not seeing how my change can be related.
kubectl apply -f bundle/manifests/nginx-ingress-operator-nginx-ingress-admin_rbac.authorization.k8s.io_v1_clusterrole.yaml

kubectl apply -f config/samples/charts_v1alpha1_nginxingress.yaml

The Controller was spun up with runAsUser: 1001010000 (generated UID as would be expected).
Not seeing what could directly break here 🤔

Let me know what further experiments could be preferable.

@sigv sigv changed the title Set RunAsUser as MustRunAsRange Set SCC's RunAsUser as MustRunAsRange Mar 8, 2024
@sigv sigv force-pushed the runAsUserNull branch 2 times, most recently from 151224f to 5f4c1c6 Compare March 14, 2024 17:31
@sigv sigv force-pushed the runAsUserNull branch 3 times, most recently from 4b7ead3 to 66fe779 Compare March 27, 2024 19:18
@vepatel
Copy link
Contributor

vepatel commented Apr 8, 2024

Hey @sigv, only scc yamls are exclusive to this operator repo, rest of the files are synced from https://github.com/nginxinc/kubernetes-ingress/tree/main/charts/nginx-ingress, so any change needed will have to go there initially

@sigv
Copy link
Author

sigv commented Apr 10, 2024

Understandable - this change is blocked on nginxinc/kubernetes-ingress#3665.

The chart must be updated for SCC to be updated. Without this, startup would fail as SCC requires dynamic range, but chart enforces specific UID.

@vepatel
Copy link
Contributor

vepatel commented Apr 10, 2024

thanks @sigv, left a comment there in NIC pr

@vepatel
Copy link
Contributor

vepatel commented Apr 18, 2024

@sigv you can remove the changes from helm directory as well, keep the scc ones as they're specific to operator

@sigv
Copy link
Author

sigv commented Apr 18, 2024

Reverted helm-charts to main branch state as requested. ✅

Upstream Helm Chart is removing explicit `runAsUser` value from the
Deployment and DaemonSet resources. This practically means the UID
will be inherited from image's Dockerfile.

Users on vanilla Kubernetes clusters will not observe a change in
behavior, unless they have exotic configurations.

However, OpenShift does have additional security measures. It suggests
using randomized UIDs/GIDs for workloads. To enable this, the custom
Security Context Constraint resources are being updated.
The `MustRunAsRange` policy is utilized with pre-allocated values
(no explicit range min/max), which effectively allows OpenShift to
pick its own ranges.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants