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.

Sign up for GitHub

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

Jump to bottom

Apply UID/GID defaults from image #3665

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

Apply UID/GID defaults from image #3665

wants to merge 1 commit into from
+0 −13

Conversation

sigv
Copy link
Contributor

@sigv sigv commented Mar 19, 2023 •
edited
Loading

Proposed changes

build/Dockerfile specifies USER 101 for common target, which is re-applied into the final images. Helm Chart/Manifests do not need to specify UID explicitly, and can instead use the image's UID. (PodSecurityContext v1 core specifies runAsUser defaults to user specified in image metadata if unspecified.)

The existing runAsNonRoot: true flag (already in place) will ensure during runtime that the image is configured with a custom user ID.

This is notably helpful for users running OpenShift, because OpenShift attempts to enforce custom UID/GID ranges for individual namespaces as part of restricted-v2 Security Context Constraint. When removing hard-coded values from manifests, OpenShift will be able to assign its own UID/GID.

In practice, this means a different model of configuring file system permissions. OpenShift assigns the container process GID 0 as supplemental to assist with that. Locations that are expected to be written to must be owned by GID 0, with group write permissions. Previous changes to main have ensured that is the case.

Init container copying files is not a concern, as we will have the same UID as owner there as the main NIC container.

Reference: A Guide to OpenShift and UIDs

Closes #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 main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@sigv sigv requested a review from a team as a code owner March 19, 2023 14:59
@github-actions github-actions bot added the helm_chart Pull requests that update the Helm Chart label Mar 19, 2023
@sigv sigv force-pushed the runAsUserNull branch 2 times, most recently from e9847eb to 9a78b51 Compare March 20, 2023 15:32
@codecov
Copy link

codecov bot commented Mar 20, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.35%. Comparing base (8a2c9a9) to head (f38706e).
Report is 257 commits behind head on main.

❗ Current head f38706e differs from pull request most recent head ee75176. Consider uploading reports for the commit ee75176 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3665      +/-   ##
==========================================
+ Coverage   52.32%   52.35%   +0.03%     
==========================================
  Files          61       59       -2     
  Lines       17502    16880     -622     
==========================================
- Hits         9158     8838     -320     
+ Misses       8015     7747     -268     
+ Partials      329      295      -34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sigv sigv force-pushed the runAsUserNull branch 6 times, most recently from a3a1fc0 to f38706e Compare March 21, 2023 20:25
@sigv
Copy link
Contributor Author

sigv commented Mar 22, 2023 •
edited
Loading

Not sure if CI / Smoke Tests (alpine, policies, 1.26.2) failure of internally observed 502 Bad Gateway response is relevant to this PR. 🤔

Will double check if I can repro. Tests being re-run helped. I do not see how this PR can cause a transient test failure.

@sigv sigv marked this pull request as draft March 22, 2023 06:22
@sigv sigv marked this pull request as ready for review March 22, 2023 13:51
@sigv sigv force-pushed the runAsUserNull branch 12 times, most recently from ab538f7 to 67a9b99 Compare March 30, 2023 06:08
@sigv sigv force-pushed the runAsUserNull branch 4 times, most recently from b975e9b to 5344586 Compare April 3, 2023 20:53
@sigv sigv force-pushed the runAsUserNull branch 3 times, most recently from f4b93df to d407700 Compare March 20, 2024 17:40
@sigv sigv force-pushed the runAsUserNull branch 4 times, most recently from 11dc22e to c9a02e5 Compare March 27, 2024 19:17
@sigv sigv force-pushed the runAsUserNull branch 5 times, most recently from 690f861 to cec777a Compare April 10, 2024 06:38
@vepatel
Copy link
Contributor

vepatel commented Apr 10, 2024

Hi @sigv can you create a new issue for this as the one linked in description seems incomplete and WIP, we can then prioritise and review the PR, thanks!

@vepatel vepatel requested review from oseoin and pdabelf5 April 10, 2024 12:05
@sigv
Copy link
Contributor Author

sigv commented Apr 18, 2024

@vepatel, opened a fresh #5422 for this scope. 🤞🏻

@sigv sigv force-pushed the runAsUserNull branch 3 times, most recently from f3b7352 to f291fb8 Compare April 24, 2024 14:48
@sigv
Apply UID/GID defaults from image
ee75176 
`build/Dockerfile` specifies `USER 101` for `common` target,
which is re-applied into the final images. Helm Chart/Manifests
do not need to specify UID explicitly, and can instead use
the image's UID. (PodSecurityContext v1 core specifies `runAsUser`
defaults to user specified in image metadata if unspecified.)

The existing `runAsNonRoot: true` flag (already in place) will ensure
during runtime that the image is configured with a custom user ID.

This is notably helpful for users running OpenShift, because OpenShift
attempts to enforce custom UID/GID ranges for individual namespaces
as part of `restricted-v2` Security Context Constraint. When removing
hard-coded values from manifests, OpenShift will be able to assign its
own UID/GID.

In practice, this means a different model of configuring file system
permissions. OpenShift assigns the container process GID 0 as
supplemental to assist with that. Locations that are expected to be
written to must be owned by GID 0, with group write permissions.
Previous changes to `main` have ensured that is the case.

Init container copying files is not a concern, as we will
have the same UID as owner there as the main NIC container.

Reference: https://cloud.redhat.com/blog/a-guide-to-openshift-and-uids
@github-actions github-actions bot removed the documentation Pull requests/issues for documentation label May 8, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 7, 2024

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label Aug 7, 2024
@j1m-ryan
Copy link
Member

Hi @sigv, now that securityContext is configurably using helm, is this still required?

@github-actions github-actions bot removed the stale Pull requests/issues with no activity label Aug 13, 2024
@github-actions GitHub Actions
Copy link

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ADubhlaoich ADubhlaoich ADubhlaoich left review comments

@brianehlert brianehlert brianehlert left review comments

@lucacome lucacome lucacome left review comments

@oseoin oseoin Awaiting requested review from oseoin

@pdabelf5 pdabelf5 Awaiting requested review from pdabelf5

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
helm_chart Pull requests that update the Helm Chart stale Pull requests/issues with no activity
Projects
NGINX Ingress Controller
Status: Todo ☑
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support OpenShift's built-in restricted-v2 Security Context Constraint
7 participants
@sigv @andrescolodrero @brianehlert @vepatel @j1m-ryan @lucacome @ADubhlaoich