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

add security context that we recommend for the image #32

Merged
merged 12 commits into from
Nov 4, 2021

Conversation

elrido
Copy link
Contributor

@elrido elrido commented Oct 1, 2020

This is an untested change, that I hope will not break any existing setup. I would expect that k8s does run the process as specified in the container with the non-root user ID already and therefore the data gets owned accordingly, but as discussed in #31 it could be made explicit.

@elrido elrido requested a review from bdashrad October 1, 2020 19:23
@bdashrad
Copy link
Collaborator

bdashrad commented Oct 2, 2020

I will review and test this tomorrow at work

Copy link
Collaborator

@bdashrad bdashrad left a comment

Choose a reason for hiding this comment

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

We may want to consider wrapping the securityContext block in a conditional, default off for now, until we release a version tagged docker image to reduce risk of hitting any edge cases.

We also should make this change to the StatefulSet

privatebin/Chart.yaml Outdated Show resolved Hide resolved
privatebin/templates/deployment.yaml Outdated Show resolved Hide resolved
elrido and others added 2 commits October 3, 2020 10:29
Co-authored-by: Brad Clark <bdashrad@gmail.com>
Co-authored-by: Brad Clark <bdashrad@gmail.com>
@elrido
Copy link
Contributor Author

elrido commented Oct 3, 2020

Ok, I've created a tag "1.3.4-alpine3.12-k8s-sec-context" on the image repo, which got auto-built and published to the docker hub. Would I now need to change the Chart.yaml's appversion to this tag or would you prefer to have the conditional in place anyway?

appVersion: 1.3.4

@bdashrad
Copy link
Collaborator

bdashrad commented Oct 5, 2020

I need to think about how to do this properly in the stateful set so we don't break anyone using persistent volumes for storage.

@elrido
Copy link
Contributor Author

elrido commented Oct 6, 2020

Yes, lets better be careful with this change.

Don't know if this is helpful or not: For that k8s setup with an NFS backed persistent volume I had to use an init container in the deployment to set up the permissions (as in the example for the image's README). Maybe if it got used to recursively update the permissions?

      initContainers:
      - name: {{ .Chart.Name }}-permissions
        image: privatebin/chown:1.31.1-musl-1.1.24-r9
        imagePullPolicy: {{ .Values.image.pullPolicy }}
        command: ['-R', '65534:82', '/mnt']
        securityContext:
          runAsUser: 0
          readOnlyRootFilesystem: true
        volumeMounts:
        - mountPath: /mnt
          name: storage
          readOnly: False

Note: The image mentioned above contains only the chown binary built from busybox.

@bdashrad
Copy link
Collaborator

bdashrad commented Oct 6, 2020

That seems to be the common way to deal with file permissions in this situation. For example: https://github.com/helm/charts/blob/master/stable/redis/templates/redis-master-statefulset.yaml#L317

@Punkoivan
Copy link

Hello there.
I would add fsGroup and set to 82.
Without this it breaks on EKS with gp2 as PV and StatefulSet as a kind.

Also ReadWriteMany is not allowed mode within gp2, so made it as a variable in values.
Submitted PR #37
Would be happy to adjust that to fit this one.

Thanks!

@bdashrad bdashrad merged commit 41ed99f into master Nov 4, 2021
@bdashrad bdashrad deleted the securitycontext branch November 4, 2021 19:03
@github-actions github-actions bot mentioned this pull request Nov 22, 2021
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