Skip to content

Conversation

jcpunk
Copy link
Contributor

@jcpunk jcpunk commented May 5, 2025

This should resolve the regression from #3684

@jkroepke
Copy link
Collaborator

jkroepke commented May 5, 2025

@andreas-heatly are you able to test this to prevent an outage on your side?

@Kimi450
Copy link

Kimi450 commented May 5, 2025

I tested the restart usecase explained in #3699 and this fixes that at least

  1. Install the chart
    helm upgrade -i my-release grafana/grafana --set initChownData.enabled=true --set persistence.enabled=true --wait
  2. Once the pod is running, delete it
  3. Observe that it cannot restart because the initcontainer fails with
chown: /var/lib/grafana/csv: Permission denied
chown: /var/lib/grafana/png: Permission denied
chown: /var/lib/grafana/pdf: Permission denied
19:52:16 dell-e7440 [--0] ~  $
  1. updated the deployment with the changes proposed here
  2. new pod comes up

@markussiebert
Copy link

markussiebert commented May 9, 2025

Tested this - does not work with psp:

message: 'pods "grafana-64f4f78b5f-xkwr8" is forbidden: violates PodSecurity "baseline:latest":                               
      non-default capabilities (container "init-chown-data" must not include "DAC_READ_SEARCH"                                    
      in securityContext.capabilities.add)'

Don't think we have to give away more than the baseline policy?!

@jkroepke
Copy link
Collaborator

jkroepke commented May 9, 2025

baseline policy

Generally, chown is a privileged operation. If you want to be compliant with baseline, just disable the initContainer. In modern Kubernetes environment, its generally not recommend to use that approach.

@jcpunk
Copy link
Contributor Author

jcpunk commented May 9, 2025

Breaking compat with baseline is not something I'm interested in.

There is some sort of strange interaction between the init-container and the pvc. I don't think I'll have time to look at this for a while... I'll close this PR out.

Interested folks at least have a hint something is tricky with the explicit readOnlyRootFilesystem: false.

@jcpunk jcpunk closed this May 9, 2025
@jkroepke jkroepke reopened this May 9, 2025
@jkroepke
Copy link
Collaborator

jkroepke commented May 9, 2025

Breaking compat with baseline is not something I'm interested in.

Sorry to interrupt here, but the chown init container will never be compatible with baseline.

Changing file owner is an operation done by the root account. This is normally done by securityContext.fsGroups. In production deployments, this initContainer not be not used.

But it's still valuable to increase the hardening in that context.

@markussiebert
Copy link

But I don't understand - shouldn't the default be the hardened? I am only here, because of the issues. Never decided to activate the init container. Never had any psp issues until the cap of this pr was added. And chown alone doesn't violate the psp.

@jkroepke
Copy link
Collaborator

But how the PSP can affect your deployment, if chmod container is not enabled on your deployment?

@markussiebert
Copy link

The default is enable chown

@jkroepke
Copy link
Collaborator

Thats the root issue. this should be not enabled by default.

| `initChownData.image.pullPolicy` | init-chown-data container image pull policy | `IfNotPresent` |
| `initChownData.resources` | init-chown-data pod resource requests & limits | `{}` |
| `initChownData.securityContext` | init-chown-data pod securityContext | `{"readOnlyRootFilesystem": false, "runAsNonRoot": false}`, "runAsUser": 0, "seccompProfile": {"type": "RuntimeDefault"}, "capabilities": {"add": ["CHOWN"], "drop": ["ALL"]}}` |
| `initChownData.securityContext` | init-chown-data pod securityContext | `{"readOnlyRootFilesystem": true, "runAsNonRoot": false}`, "runAsUser": 0, "seccompProfile": {"type": "RuntimeDefault"}, "capabilities": {"add": ["CHOWN", "DAC_READ_SEARCH"], "drop": ["ALL"]}}` |
Copy link

Choose a reason for hiding this comment

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

Looks like the formatting is incorrect

Suggested change
| `initChownData.securityContext` | init-chown-data pod securityContext | `{"readOnlyRootFilesystem": true, "runAsNonRoot": false}`, "runAsUser": 0, "seccompProfile": {"type": "RuntimeDefault"}, "capabilities": {"add": ["CHOWN", "DAC_READ_SEARCH"], "drop": ["ALL"]}}` |
| `initChownData.securityContext` | init-chown-data pod securityContext | `{"readOnlyRootFilesystem": true, "runAsNonRoot": false, "runAsUser": 0, "seccompProfile": {"type": "RuntimeDefault"}, "capabilities": {"add": ["CHOWN", "DAC_READ_SEARCH"], "drop": ["ALL"]}}` |

Copy link
Collaborator

Choose a reason for hiding this comment

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

@marquiz are you interested into pick up this PR?

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