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

feat: add securityContext for pod and container #357

Merged

Conversation

ChrisFraun
Copy link
Contributor

What: Deployments can have security settings in their manifest on two levels: pod and container. However, there are some capabilities only configurable in one of the respective levels(https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.23/#securitycontext-v1-core). This PR sets a default configuration for container securityContext, which drops all POSIX capabilities and denies privilege escalation and for pod securityContext adds user, group fsGroup and supplementalGroups and also denies root usage.
These are and should be standard settings in the context of Kubernetes. It also adds the possibility of running vault-injector in a Kubernetes environment without PSP (to be removed in v1.25 https://kubernetes.io/docs/concepts/security/pod-security-policy/), but with OpenPolicyAgent (possibly the PSP substitute) with the same capabilities as a restricted PSP instead.

This PR sets the respective settings to the values.yaml and is defaulting them as well. With this they can be adopted if it is needed.

@ChrisFraun ChrisFraun requested a review from slok as a code owner July 25, 2022 08:24
@ChrisFraun
Copy link
Contributor Author

any thoughts @slok ? :)

@peteroruba
Copy link

We are also waiting for this to get merged

@bfennane
Copy link

bfennane commented Oct 6, 2022

+1, we are waiting for this too.

@slok
Copy link
Owner

slok commented Oct 22, 2022

Hi @ChrisFraun, thanks for the PR!

I will try reviewing this next week, however, I would prefer to not have this as a default, but as an option. I think that most people lean towards not having all this complexity by default.

Also, could you add some tests please 🙏

Security is important, but simplicity too, it all depends on the context you are running things :)

@ChrisFraun
Copy link
Contributor Author

I understand!
whats better than simply basic security by default then (which can even be modified if needed)?
can you point me out where exactly I should write those tests?

@slok
Copy link
Owner

slok commented Oct 24, 2022

Here are the tests: https://github.com/slok/sloth/tree/main/deploy/kubernetes/helm/sloth/tests

I don't want to derail the PR in another kind of discussion about security philosophy, I just going to say that security adds complexity (always, and at multiple layers), and context matters (people forget about this).

If this default doesn't have any side effects, why Kubernetes doesn't set this as a default when not specifying anything? I'm not, the one forcing users to select their pod security if Kubernetes doesn't do it either.

Don't get me wrong, I'm not saying security is not important, security it's so important matter that at the moment I would need this kind of security level, I would not be comfortable depending on a random third-party helm chart to control my cluster security, I would use webhooks and/or controllers to have that sorted at a cluster level.

@ChrisFraun
Copy link
Contributor Author

True, we can leave the settings empty as well. For me the most important that the contexts are set correctly for pod and containers respectively.

@ChrisFraun
Copy link
Contributor Author

Not completely sure if I did it right. I ran the test commands make test locally and it seems to work. Would be grateful for a look and some feedback @slok! :)

@slok
Copy link
Owner

slok commented Oct 25, 2022

I think that this may help you to have a better feedback loop :)

cd ./deploy/kubernetes/helm/sloth/tests/ && go test -v ./...

@ChrisFraun
Copy link
Contributor Author

PASS ok github.com/slok/sloth/helm 0.537s :)

@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2022

Codecov Report

Merging #357 (1afdf91) into main (932d116) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #357   +/-   ##
=======================================
  Coverage   75.58%   75.58%           
=======================================
  Files          17       17           
  Lines        1491     1491           
=======================================
  Hits         1127     1127           
  Misses        286      286           
  Partials       78       78           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Owner

@slok slok left a comment

Choose a reason for hiding this comment

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

You go it passing finally! Congrats :) Many thanks for the contribution @ChrisFraun.

I see minor things that I could change myself afterward (like test location), so it doesn't make sense to block this PR even more time.

🚀

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.

5 participants