-
Notifications
You must be signed in to change notification settings - Fork 96
Add non-root support for PodImpersonation #973
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds non-root support for PodImpersonation by removing explicit root user configurations (RunAsUser and RunAsGroup) from init containers and the proxy container. The intent is to allow these containers to inherit security context settings from the pod level, enabling the code to work with pods that have non-root security contexts. Additionally, file paths are changed from /root/.kube/config to /home/shell/.kube/config to match the expected home directory for non-root users.
Key changes:
- Removed explicit RunAsUser and RunAsGroup settings from init and proxy containers
- Changed KUBECONFIG paths from
/root/.kube/configto/home/shell/.kube/config - Modified SecurityContext handling to preserve or inherit container settings
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/podimpersonation/podimpersonation.go | Modified SecurityContext handling for init and proxy containers to support non-root execution; updated KUBECONFIG paths from /root to /home/shell |
| go.mod | Added Kubernetes version replace directives to pin all k8s.io dependencies to v0.34.1/v1.34.2 |
| go.sum | Updated checksums for new and updated dependencies including extensive k8s.io module additions |
| .gitignore | Added .idea directory to ignore list and reordered entries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| securityContext = container.SecurityContext | ||
|
|
||
| //We must override ReadOnlyRootFilesystem value to false for same reason as above. | ||
| securityContext.ReadOnlyRootFilesystem = &f |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SecurityContext handling logic may override settings from the original container's SecurityContext. If container.SecurityContext is not nil, this code directly assigns it to the init container, which could include settings like RunAsUser, RunAsNonRoot, AllowPrivilegeEscalation, etc. that may not be appropriate for the init container's purpose. Consider creating a new SecurityContext that only copies specific required fields from the container's SecurityContext rather than using it directly.
| securityContext = container.SecurityContext | |
| //We must override ReadOnlyRootFilesystem value to false for same reason as above. | |
| securityContext.ReadOnlyRootFilesystem = &f | |
| // Create a copy of the container's SecurityContext so that updates | |
| // for the init container do not mutate the original container's settings. | |
| scCopy := *container.SecurityContext | |
| // We must override ReadOnlyRootFilesystem value to false for same reason as above. | |
| scCopy.ReadOnlyRootFilesystem = &f | |
| securityContext = &scCopy |
| pod.Spec.InitContainers = append(pod.Spec.InitContainers, v1.Container{ | ||
| Name: "init-kubeconfig-volume", | ||
| Image: image, | ||
| Command: []string{"sh", "-c", fmt.Sprintf("cp %s %s && chown %d %s/config", cfgVMount.MountPath, vmount.MountPath, shellUser, vmount.MountPath)}, |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init container command uses chown to change file ownership to UID 1000, but without explicitly setting RunAsUser, the init container will inherit the pod's SecurityContext. If the pod is configured to run as a non-root user other than UID 1000, the chown command may fail due to insufficient permissions. Consider either preserving the RunAsUser: 0 for the init container (to allow chown to work), or changing the approach to not require chown (e.g., using fsGroup).
| if container.SecurityContext != nil { | ||
| securityContext = container.SecurityContext | ||
|
|
||
| //We must override ReadOnlyRootFilesystem value to false for same reason as above. |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment mentions "same reason as above" but there is no clear reference to what "above" means in this context since the comment was likely moved. The comment should be more self-contained and explicit about why ReadOnlyRootFilesystem must be set to false (i.e., because the init container needs to write to the emptyDir volume).
| //We must override ReadOnlyRootFilesystem value to false for same reason as above. | |
| // We must override ReadOnlyRootFilesystem to false because the init container needs | |
| // to write to the emptyDir volume when copying and changing permissions on the | |
| // kubeconfig file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Related to: rancher/rancher#53280
Issue
When calling
PodImpersonation.CreatePodif the inputpod *v1.Podhas non-root SecurityContext the added containers do not match the pod and cause errors.Solution
Modify
PodImpersonationto ensure it will not add explicit root user configs. This way it will inherit the values from the Pod's config and can be run as non-Root.Testing
Tested local branch/package alongside my PR to Rancher using the newest Shell RC.
Other Thoughts
I'm not sure what other context this code is used in so there may be issues related to that blindspot. If we assume the context always relates to the
rancher/shellimage then this is less concerning. Since we can always know that the/home/shellexists for non-root access.