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 pod security context #177

Merged
merged 7 commits into from
Sep 23, 2023
Merged

Added pod security context #177

merged 7 commits into from
Sep 23, 2023

Conversation

jonasbg
Copy link
Contributor

@jonasbg jonasbg commented Sep 18, 2023

What I've done:
Added podSecurityContext which is translated to spec.containers[].securityContext as the regular securityContext only applied to spec.securityContext.

Added SecurityContext to:

  • juice-shop
  • juice-balancer
  • progress-watchdog
  • juice-cleaner

Updated guides/production-notes/production-notes.md with examples of best-practice for a security-context setup.

Git history: I tried to add NetworkPolicy, which I did for juice-shop. But I think that is a bigger PR and that I should add it for all those other deployments as well. So I took it out, so that this PR is just about podSecurityContext.

@jonasbg
Copy link
Contributor Author

jonasbg commented Sep 18, 2023

I have not been able to make the juice-shop podSecurityContext to readOnlyRootFilesystem:true, even when trying to mount /data/chatbot as tempfs in memory. Should perhaps add that to the documentation if someone doesn't know how to fix?

@jonasbg jonasbg changed the title DRAFT: Hardened security policies (SecurityContext and NetworkPolicy) DRAFT: Added pod security context Sep 18, 2023
@jonasbg jonasbg changed the title DRAFT: Added pod security context Added pod security context Sep 18, 2023
@jonasbg jonasbg marked this pull request as ready for review September 18, 2023 08:01
Copy link
Member

@J12934 J12934 left a comment

Choose a reason for hiding this comment

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

Hi @jonasbg 👋
Sorry for the late(ish) reply.

Thank you for the PR, looks really nice 🙌
Two minor things:

  • In the pr you are mixing podSecurityContext and podsecurityContext, i guess there is no reason to do so? Sticking to just podSecurityContext should fit in nicely to the other values that we have :)
  • If possible I'd say we make the values in the prod notes the default values. Should work for basically anybody and then users don't need to do anything to get the extra security benefit. That was my goal with issue Add Default securityContext Configuration #62

Regarding the other points:

  • readOnlyRootFs for juice shop: yeah I guess this might be bit harder there are a handful of files written on juice shop startup, some unfortunately in folder which already contain other files. This makes a read only fs a bit tricky. I'd skip it for now. We might be able to change the juice-shop behavior a bit to make readOnlyRootFs easier
  • net policies: very nice that you started looking into it. would be very cool to have a seperate pr for it 🚀 I started cleaning up the labels for the different pods multi-juicer consists of to make this easier but haven't started yet. any contributions are as highly welcome :)

Copy link
Member

@J12934 J12934 left a comment

Choose a reason for hiding this comment

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

Ok, slight change of plans :D
Will merge this now and (after slightly fixing the value names) release this in a feature release.

Will then use the production release not security context values as defaults in the next major / breaking release.

@J12934 J12934 merged commit aedc272 into juice-shop:main Sep 23, 2023
3 checks passed
@J12934
Copy link
Member

J12934 commented Sep 23, 2023

Urgh no another change of plans... 🥴
The naming right now is super confusing.

Before we only had securityContext which configured the securityContext on pod level.
With this pr it's now also possible to configure it on container level, but the additional property is called podSecurityContext. Which is really inconsistent and confusing.

I'm changing them to containerSecurityContext and podSecurityContext like it's done in the bitnami helm chart.
This will then be a breaking release but i guess it's better than confusing helm values. I'll also set them by default then.

J12934 added a commit that referenced this pull request Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants