-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: add missing resources, securityContext and env entries #13210
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
Conversation
This pull request introduces an insecure Helm chart change that writes untrusted
Arbitrary Content Injection (RCE) in
|
Vulnerability | Arbitrary Content Injection (RCE) |
---|---|
Description | The Helm chart allows arbitrary content from .Values.localsettingspy to be directly injected into a ConfigMap named {{ $fullName }}-localsettingspy . This ConfigMap is then mounted as /app/dojo/settings/local_settings.py in the celery-beat , celery-worker , and django application containers. In Django applications, local_settings.py is typically imported and executed as Python code during application startup. An attacker with permissions to set Helm values can inject malicious Python code into .Values.localsettingspy , which will be executed by the application, leading to Remote Code Execution (RCE). |
{{ toYaml .Values.localsettingspy | indent 4 }} |
All finding details can be found in the DryRun Security Dashboard.
@kiblik I came across some missing gaps from my last chart PR, should be quite straightforward and I hope I didn't miss anything this time |
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.
Couple of comments
Now I'm thinking, there are Pod SecurityContext and Container SecurityContext. Some parts overlap (e.g., $ curl https://raw.githubusercontent.com/yannh/kubernetes-json-schema/refs/heads/master/master/securitycontext.json | jq '.properties | keys '
[
"allowPrivilegeEscalation",
"appArmorProfile",
"capabilities",
"privileged",
"procMount",
"readOnlyRootFilesystem",
"runAsGroup",
"runAsNonRoot",
"runAsUser",
"seLinuxOptions",
"seccompProfile",
"windowsOptions"
]
$ curl https://raw.githubusercontent.com/yannh/kubernetes-json-schema/refs/heads/master/master/podsecuritycontext.json | jq '.properties | keys '
[
"appArmorProfile",
"fsGroup",
"fsGroupChangePolicy",
"runAsGroup",
"runAsNonRoot",
"runAsUser",
"seLinuxChangePolicy",
"seLinuxOptions",
"seccompProfile",
"supplementalGroups",
"supplementalGroupsPolicy",
"sysctls",
"windowsOptions"
] It would be good idea to separate them. I'm not sure how it is usually done in other projects. |
okay, did some (not small) changes to split pod <> container security Contexts and some other changes (annotations -> extraAnnotations to align with labels, add them everywhere). I'll need more time to see where I added a regression in the tests Also, release notes are pending but that will come later 🙏 |
@fernandezcuesta, can you please rebase from |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Conflicts have been resolved. A maintainer will review the pull request shortly. |
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.
Just one change ({}
vs. nil
).
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.
Approved
Description
This is a complement of #12691 which addressed some Helm chart refactoring, but failed to allow securityContext as well as mounted volumes and resources for all containers and initContainers.
For example, when PSS policies are enforced, we may need to specify
readOnlyRootFilesystem: false
and mount tmpfs volues in all (init)containers. This PR closes the gaps left from #12691.Test results
Tested in a production instance from a fork.
Documentation
n/a