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

Added a security context to run as non-root user. #1799

Closed
wants to merge 1 commit into from

Conversation

NerdSec
Copy link
Contributor

@NerdSec NerdSec commented Oct 5, 2020

Hello Everyone,

As discussed on the forum with @manics, raising a PR for fixing the security context of the user-placeholder pods.

This is my first of many (hopefully) PR on an OSS project. Do let me know in case I missed something.

Regards,
Nachiket

@welcome
Copy link

welcome bot commented Oct 5, 2020

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/restricted-psp-z2jh-fails-in-local-gitlab-oauth-workflow/6202/5

@consideRatio
Copy link
Member

Welcome and thanks for being an active community member @NerdSec ! ❤️ 🎉

Could you help me review #1798 perhaps? I recently opened a PR that this duplicates in order to resolve this kind of issues for all the Helm chart's pods!

@@ -47,4 +47,6 @@ spec:
image: {{ .Values.prePuller.pause.image.name }}:{{ .Values.prePuller.pause.image.tag }}
resources:
{{- include "jupyterhub.resources" . | nindent 12 }}
securityContext:
runAsUser: 1000
Copy link
Member

Choose a reason for hiding this comment

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

This is a podSecurityContext, which can influence sidecar containers for example from istio which I've seen can lead to issues before, so I think we should set it on the container level only influencing the containers we actually manage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this does make sense. Adding it as a podSecurityContext seems better than for the entire deployment. Maybe we can add the runAsGroup parameter as well to ensure that any changes to the image does not affect us in the future?

The indentation in the code above seems to miss a tab, I think it should be as follows:

  image: {{ .Values.prePuller.pause.image.name }}:{{ .Values.prePuller.pause.image.tag }}
  resources:
    {{- include "jupyterhub.resources" . | nindent 12 }}
  securityContext:
    runAsGroup: 1000
    runAsUser: 1000

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for thinking about this @NerdSec ! I've made it a container securityContext in #1798, and also added runAsGroup there, what do you think?

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

3 participants