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

Incompatibility with PSPs using read-only allowed host paths #7755

Open
louisblin opened this issue Feb 4, 2022 · 7 comments
Open

Incompatibility with PSPs using read-only allowed host paths #7755

louisblin opened this issue Feb 4, 2022 · 7 comments
Labels
area/executor solution/workaround There's a workaround, might not be great, but exists type/feature Feature request type/security Security related

Comments

@louisblin
Copy link

Summary

Argo Workflows seems to be incompatible with Pod Security Policies (PSPs) specifying read-only allowed host paths. As mentioned in this thread, the init / wait containers surrounding a workflow container mount the same volumes as the main container, but without respecting their read/write mode. For environments using PSPs with read-only allowed host paths, it becomes impossible to run workflows that use volume mounts (as sidecar containers will violate the PSP).

Looking at the code mentioned above, the author claims that mounts need to be read/write to allow overlapping mount paths. However, the main container will already need to mount paths in read/write mode if they overlap, so I don't see why the sidecar containers needs to modify this setting. Am I missing something or could we simply remove this override?

Argo Workflows install: version v3.2.6, on a vanilla k8s cluster with the K8SAPI executor.

Diagnostics

To replicate the issue, here's a sample PSP with a read-only allowed host path:

$ kubectl apply -f - <<EOF
apiVersion: policy/v1beta1
kind: PodSecurityPolicy
metadata:
  name: readonly-hostpaths
spec:
  allowedHostPaths:
  - pathPrefix: /etc/ssl/certs/
    readOnly: true
  volumes:
  - '*'

  runAsUser:
    rule: RunAsAny
  runAsGroup:
    rule: RunAsAny
  fsGroup:
    rule: RunAsAny
  supplementalGroups:
    rule: RunAsAny
  seLinux:
    rule: RunAsAny
EOF

And a workflow that will violate it:

$ argo submit - <<EOF
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: hello-world-
spec:
  entrypoint: entrypoint
  templates:
  - name: entrypoint
    script:
      image: alpine
      source: |
        ls -lha /etc/ssl/certs/
      volumeMounts:
      - mountPath: /etc/ssl/certs/
        name: ssl-certs
        readOnly: true
    volumes:
    - hostPath:
        path: /etc/ssl/certs/
      name: ssl-certs
EOF

After inspecting the workflow in the UI, you get the following message, which indicates that the sidecar container is trying to mount the ssl-certs volume in write mode.

pods "hello-world-bq8pl" is forbidden: unable to validate against any pod security policy: [spec.containers[0].volumeMounts[0].readOnly: Invalid value: false: must be read-only]

This can be confirmed by disabling the PSP and inspecting the pod spec that is created.


Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.

@sarabala1979
Copy link
Member

sarabala1979 commented Feb 4, 2022

@louisblin you use podSpecPatch as workaround to set appropriate values. Can you try and let us know?

@sarabala1979 sarabala1979 added the solution/workaround There's a workaround, might not be great, but exists label Feb 4, 2022
@louisblin
Copy link
Author

louisblin commented Feb 4, 2022

Thanks for the hint @sarabala1979!

It does work, but the patch is particularly unreadable and I can't expect my users to understand this. Problems are:

  • Custom patch for each combination of volume mounts
  • Patch size grows with the number of mounts
  • Makes an assumption about the magic /mainctrfs path prefix

Here's what it looks like:

podSpecPatch: |
  {"containers":[{"name":"wait","volumeMounts":[{"mountPath":"/mainctrfs/etc/ssl/certs","name":"ssl-certs","readOnly":true}]}],"initContainers":[{"name":"init","volumeMounts":[{"mountPath":"/mainctrfs/etc/ssl/certs","name":"ssl-certs","readOnly":true}]}]}

I would very much like to avoid doing this. Is there any reason why the init / wait containers can't mount these volumes using the same mode as the main container? It would be great to remove this line unless there's a good reason for keeping it.

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the problem/stale This has not had a response in some time label Mar 2, 2022
@louisblin
Copy link
Author

Any thoughts on removing the line I mentioned, @sarabala1979 or @alexec?

@stale stale bot removed the problem/stale This has not had a response in some time label Mar 2, 2022
@alexec alexec added type/feature Feature request type/security Security related and removed type/bug triage labels Mar 10, 2022
@alexec
Copy link
Contributor

alexec commented Mar 10, 2022

Would you be interested in investingg and submitting a PR to see if the change works?

@louisblin
Copy link
Author

Absolutely, I'll give that a try!

louisblin referenced this issue Mar 10, 2022
* Implements PNS (Process Namespace Sharing) executor
* Adds limited support for Kubelet/K8s API artifact collection by mirroring volume mounts to wait sidecar
* Adds validation to detect when output artifacts are not supported by the executor
* Adds ability to customize executor from workflow-controller-configmap (e.g. add environment variables, append command line args such as loglevel)
* Fixes an issue where daemon steps were not getting terminated properly
louisblin pushed a commit to louisblin/argo-workflows that referenced this issue Mar 10, 2022
As discussed in argoproj#7755, the `init` / `wait` containers surrounding a
workflow container mount the same volumes as the `main` container, but
without respecting their read/write mode. For environments using PSPs
with read-only allowed host paths, it becomes impossible to run workflows
that use volume mounts (as sidecar containers will violate the PSP).

The original code author (@jessesuen) claims that mounts need to be
read/write to allow overlapping mount paths. However, the `main`
container will already need to mount paths in read/write mode if they
overlap, so there does not seem to be a good reason for keeping this.
louisblin pushed a commit to louisblin/argo-workflows that referenced this issue Mar 10, 2022
As discussed in argoproj#7755, the `init` / `wait` containers surrounding a
workflow container mount the same volumes as the `main` container, but
without respecting their read/write mode. For environments using PSPs
with read-only allowed host paths, it becomes impossible to run workflows
that use volume mounts (as sidecar containers will violate the PSP).

The original code author (@jessesuen) claims that mounts need to be
read/write to allow overlapping mount paths. However, the `main`
container will already need to mount paths in read/write mode if they
overlap, so there does not seem to be a good reason for keeping this.
louisblin pushed a commit to louisblin/argo-workflows that referenced this issue Mar 10, 2022
…#7755

As discussed in argoproj#7755, the `init` / `wait` containers surrounding a
workflow container mount the same volumes as the `main` container, but
without respecting their read/write mode. For environments using PSPs
with read-only allowed host paths, it becomes impossible to run workflows
that use volume mounts (as sidecar containers will violate the PSP).

The original code author (@jessesuen) claims that mounts need to be
read/write to allow overlapping mount paths. However, the `main`
container will already need to mount paths in read/write mode if they
overlap, so there does not seem to be a good reason for keeping this.
louisblin pushed a commit to louisblin/argo-workflows that referenced this issue Mar 10, 2022
…#7755

As discussed in argoproj#7755, the `init` / `wait` containers surrounding a
workflow container mount the same volumes as the `main` container, but
without respecting their read/write mode. For environments using PSPs
with read-only allowed host paths, it becomes impossible to run workflows
that use volume mounts (as sidecar containers will violate the PSP).

The original code author (@jessesuen) claims that mounts need to be
read/write to allow overlapping mount paths. However, the `main`
container will already need to mount paths in read/write mode if they
overlap, so there does not seem to be a good reason for keeping this.

Signed-off-by: louisblin <[email protected]>
louisblin pushed a commit to louisblin/argo-workflows that referenced this issue Mar 10, 2022
…#7755

As discussed in argoproj#7755, the `init` / `wait` containers surrounding a
workflow container mount the same volumes as the `main` container, but
without respecting their read/write mode. For environments using PSPs
with read-only allowed host paths, it becomes impossible to run workflows
that use volume mounts (as sidecar containers will violate the PSP).

The original code author (@jessesuen) claims that mounts need to be
read/write to allow overlapping mount paths. However, the `main`
container will already need to mount paths in read/write mode if they
overlap, so there does not seem to be a good reason for keeping this.

Fixes argoproj#7755

Signed-off-by: louisblin <[email protected]>
louisblin added a commit to louisblin/argo-workflows that referenced this issue Mar 10, 2022
…#7755

As discussed in argoproj#7755, the `init` / `wait` containers surrounding a
workflow container mount the same volumes as the `main` container, but
without respecting their read/write mode. For environments using PSPs
with read-only allowed host paths, it becomes impossible to run workflows
that use volume mounts (as sidecar containers will violate the PSP).

The original code author (@jessesuen) claims that mounts need to be
read/write to allow overlapping mount paths. However, the `main`
container will already need to mount paths in read/write mode if they
overlap, so there does not seem to be a good reason for keeping this.

Fixes argoproj#7755

Signed-off-by: louisblin <[email protected]>
louisblin added a commit to louisblin/argo-workflows that referenced this issue Mar 10, 2022
…#7755

As discussed in argoproj#7755, the `init` / `wait` containers surrounding a
workflow container mount the same volumes as the `main` container, but
without respecting their read/write mode. For environments using PSPs
with read-only allowed host paths, it becomes impossible to run workflows
that use volume mounts (as sidecar containers will violate the PSP).

The original code author (@jessesuen) claims that mounts need to be
read/write to allow overlapping mount paths. However, the `main`
container will already need to mount paths in read/write mode if they
overlap, so there does not seem to be a good reason for keeping this.

Fixes argoproj#7755

Signed-off-by: louisblin <[email protected]>
louisblin added a commit to louisblin/argo-workflows that referenced this issue Mar 10, 2022
…#7755

As discussed in argoproj#7755, the `init` / `wait` containers surrounding a
workflow container mount the same volumes as the `main` container, but
without respecting their read/write mode. For environments using PSPs
with read-only allowed host paths, it becomes impossible to run workflows
that use volume mounts (as sidecar containers will violate the PSP).

The original code author (@jessesuen) claims that mounts need to be
read/write to allow overlapping mount paths. However, the `main`
container will already need to mount paths in read/write mode if they
overlap, so there does not seem to be a good reason for keeping this.

Fixes argoproj#7755

Signed-off-by: louisblin <[email protected]>
louisblin added a commit to louisblin/argo-workflows that referenced this issue Mar 10, 2022
…#7755

As discussed in argoproj#7755, the `init` / `wait` containers surrounding a
workflow container mount the same volumes as the `main` container, but
without respecting their read/write mode. For environments using PSPs
with read-only allowed host paths, it becomes impossible to run workflows
that use volume mounts (as sidecar containers will violate the PSP).

The original code author (@jessesuen) claims that mounts need to be
read/write to allow overlapping mount paths. However, the `main`
container will already need to mount paths in read/write mode if they
overlap, so there does not seem to be a good reason for keeping this.

Fixes argoproj#7755

Signed-off-by: louisblin <[email protected]>
louisblin added a commit to louisblin/argo-workflows that referenced this issue Mar 10, 2022
…#7755

As discussed in argoproj#7755, the `init` / `wait` containers surrounding a
workflow container mount the same volumes as the `main` container, but
without respecting their read/write mode. For environments using PSPs
with read-only allowed host paths, it becomes impossible to run workflows
that use volume mounts (as sidecar containers will violate the PSP).

The original code author (@jessesuen) claims that mounts need to be
read/write to allow overlapping mount paths. However, the `main`
container will already need to mount paths in read/write mode if they
overlap, so there does not seem to be a good reason for keeping this.

Fixes argoproj#7755

Signed-off-by: Louis Blin <[email protected]>
@louisblin
Copy link
Author

Would you be interested in investingg and submitting a PR to see if the change works?

All tests are passing in #8128. Do you feel this needs more validation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/executor solution/workaround There's a workaround, might not be great, but exists type/feature Feature request type/security Security related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants