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

security: refactor gid and uid #284

Closed
wants to merge 2 commits into from

Conversation

moray95
Copy link

@moray95 moray95 commented May 2, 2020

  • Moves defaults for server.gid and server.uid to values.yaml.
  • Adds missing documentation for the fields.

Motivation:

Currently, it's not possible to have an "empty" group or user id in pods. Overriding server.gid or server.uid to null doesn't work because of the default clause. However, on Openshift the pods will be assigned a proper group and user depending on security context constraints that applies to it.

From Managing Security Context Constraints:

When a container or pod does not request a user ID under which it should be run, the effective UID depends on the SCC that emits this pod. Because restricted SCC is granted to all authenticated users by default, it will be available to all users and service accounts and used in most cases.

@hashicorp-cla
Copy link

hashicorp-cla commented May 2, 2020

CLA assistant check
All committers have signed the CLA.

@tvoran tvoran added chart Area: helm chart enhancement New feature or request openshift labels May 5, 2020
values.yaml Outdated Show resolved Hide resolved
@moray95 moray95 force-pushed the security-context branch from d51a3f6 to 74f1653 Compare May 5, 2020 18:03
@jasonodonnell
Copy link
Contributor

jasonodonnell commented May 5, 2020

@moray95 I think we'll want unit tests covering the null use case. Have you tested this on a live system where the values are null? I'm wondering if we need to wrap the security contexts with if statements to exclude these values if they aren't set to an integer value?

You can find the unit tests in the test/unit directory.

@moray95
Copy link
Author

moray95 commented May 5, 2020

@jasonodonnell Thanks for the feedback. I'll be writing the tests asap. I have tested a deployment with null values on Openshift 4.3 and worked just fine. I don't think verifying the types is very common on Helm charts, but could be added. TBH I would prefer taking the whole securityContext from values instead of just the user and group ids, as done on many other charts. However, I didn't attempt such a change since it would break backwards compatibility. I could change the PR to use this method if you would be interested.

@moray95 moray95 force-pushed the security-context branch from 49d740f to e45893a Compare May 6, 2020 14:08
@moray95
Copy link
Author

moray95 commented May 6, 2020

@jasonodonnell Tests for the null case have been added.

@tvoran tvoran added this to the 0.6.0 milestone May 22, 2020
@jasonodonnell
Copy link
Contributor

This functionality expanded into more OpenShift support in #319, closing for duplication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart Area: helm chart enhancement New feature or request openshift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants