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(config): add default security context #25

Merged
merged 1 commit into from
Jul 20, 2020

Conversation

micnncim
Copy link
Member

In general, we should configure securityContext to keep least privileges, like using non-root. The users may miss securityContext without default one configured, and also it would be cumbersome to configure them for each kustomization patch. That's why I believe this base kustomization should have a default securityContext.

And also, I've found @ezimanyi agreed with this idea in Spinnaker Slack #kleat channel.

Copy link
Contributor

@maggieneterval maggieneterval left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @micnncim! These seem like reasonable defaults, especially since based on the Slack conversation you referenced, @kskewes has been running Spinnaker with these defaults without any issues. It looks like @kskewes' securityContext configuration also includes privileged: false and capabilities: drop: ["ALL"]: are these redundant or was there any other reason for excluding them?

@micnncim
Copy link
Member Author

@maggieneterval Thank you for your review!

First, I've dropped privileged: false because .spec.template.spec.containers[].securityContext.privileged's default value is false. I believe we don't need to explicitly specify the default value.

Second, I've dropped capabilities: drop: ["ALL"] because I think not many users acknowledge this configuration (itself or how it's important) although it would be important to properly configure the POSIX capabilities. I guess such advanced users would configure the property by themselves. Actually, I'm going to do it with my kustomization patches.

However, I follow you if you have different, rational opinions to add them 😄

@maggieneterval
Copy link
Contributor

@micnncim That all makes sense; you are definitely more familiar with what would be a reasonable set of securityContext defaults than I am. We can always change these later if other rational opinions are raised 😄

@maggieneterval maggieneterval self-requested a review July 20, 2020 16:50
@ezimanyi ezimanyi merged commit 2a7ac57 into spinnaker:master Jul 20, 2020
@karlskewes
Copy link
Contributor

We haven't yet tried setting a different user to the one defined in the Dockerfile's (100, 101, 33 Deck). I guess could be file permissions but if you're tested it please let us know. :)

Suggest adding to spec.securityContext:

      securityContext:
        fsGroup: 1000

This is required when Kubernetes mounts in tokens like the AWS IRSA token into containers, otherwise they get mounted owned by root and clouddriver can't read them.
Similar story for other volume mounting though.

Since fsGroup field is specified, all processes of the container are also part of the supplementary group ID 2000. The owner for volume /data/demo and any files created in that volume will be Group ID 2000.

@micnncim
Copy link
Member Author

Oh, nice! I agree with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants