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

Allow running command containers as non-root user with EBS backed (and other) workspace volumes #453

Merged
merged 6 commits into from
Dec 18, 2024

Conversation

patrobinson
Copy link
Contributor

@patrobinson patrobinson commented Dec 13, 2024

When using a storage provisioner, such as aws-ebs-csi, the default permission mask for the volume mount is 755 and there's no way to change it.

That means when you specify a workspace from one of these provisioners you get a volume owned by root, which is not-writable by non-root users:

config.yaml snipped

config:
  agent-config:
    hooksVolume:
      name: buildkite-hooks
      configMap:
        defaultMode: 493
        name: buildkite-agent-hooks
  workspace-volume:
    ephemeral:
      volumeClaimTemplate:
        spec:
          accessModes: ["ReadWriteOnce"]
          resources:
            requests:
              storage: 1Gi
    name: workspace-volume

configMap pre-checkout hook:

#!/bin/sh

ls -l /
ls -l /workspace
ls -l /workspace/sockets || true

Screenshot 2024-12-13 at 3 13 02 pm

This means using a command container that has the USER set to something other than root isn't possible.
Firstly our agent can't start, because it needs access to write a job-api socket file. You get:

Error: Error setting up Job API: creating job API server: creating socket server: creating socket directory: mkdir /workspace/sockets: permission denied

This can be worked around by mounting an in-memory container to the /workspace/sockets directory with the following config snipped:

  pod-spec-patch:
    containers:
      - name: container-0
        volumeMounts:
          - name: socket-dir
            mountPath: /workspace/sockets
    volumes:
      - name: socket-dir
        emptyDir:
          medium: Memory

But the command container still has no ability to write to /workspace directory, which is a common use case.

We already handle this issue within the checkout container by ensuring all files are created as the same user as the SecurityContext provided https://github.com/buildkite/agent-stack-k8s/blob/main/internal/controller/scheduler/scheduler.go#L837-L838

But since the volume as mounted as 755, the checkout container has no permission to write to the directory.

I tried to patch this by telling the checkout container to change ownership of the directory to the relevant user before it changes users,

                checkoutContainer.Args = []string{fmt.Sprintf(`set -exufo pipefail
 addgroup -g %d buildkite-agent
 adduser -D -u %d -G buildkite-agent -h /workspace buildkite-agent
+chown -R %d:%d /workspace
 su buildkite-agent -c "%s && buildkite-agent-entrypoint bootstrap"`,
+                       podGroup,
+                       podUser,
                        podGroup,
                        podUser,
                        gitConfigCmd,

But it causes the agent to fail to start with:

2024-12-13 04:00:30 INFO   Build Path doesn't exist, creating it (/workspace/build)
fatal: failed to create builds path: mkdir /workspace/build: permission denied

So I landed on this solution, copying the existing implementation that checkout uses to ensure it runs as root then chown the directory in the init container.

Result:
Screenshot 2024-12-13 at 3 25 38 pm

Most storage providers (i.e. aws-ebs-csi) create the volume with 755 permissions, which means our checkout and command container can't write to the /workspace directory
So we must give that user permission explicitly
chown -R %d /workspace`,
podUser,
podUser,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be refactored, it's pretty similar to the createCheckoutContainer code

But gathering early feedback first.

@patrobinson patrobinson changed the title Ensure /workspace is owned by user/group in security context Allow running command containers as non-root user with EBS (and other) volumes Dec 13, 2024
@patrobinson patrobinson changed the title Allow running command containers as non-root user with EBS (and other) volumes Allow running command containers as non-root user with EBS backed (and other) workspace volumes Dec 13, 2024
@DrJosh9000 DrJosh9000 self-requested a review December 16, 2024 00:31
Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

This seems like a good solution.

internal/controller/scheduler/scheduler.go Show resolved Hide resolved
internal/controller/scheduler/scheduler.go Outdated Show resolved Hide resolved
internal/controller/scheduler/scheduler.go Outdated Show resolved Hide resolved
@patrobinson patrobinson marked this pull request as ready for review December 17, 2024 03:09
@patrobinson
Copy link
Contributor Author

@DrJosh9000 thanks, I've marked this as ready for review

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

LGTM with one name change. I also have some minor cleanup suggestions but these also apply to the existing createCheckoutContainer, so feel free to split off into a followup.

internal/controller/scheduler/scheduler.go Outdated Show resolved Hide resolved
internal/controller/scheduler/scheduler.go Outdated Show resolved Hide resolved
internal/controller/scheduler/scheduler.go Outdated Show resolved Hide resolved
internal/controller/scheduler/scheduler.go Outdated Show resolved Hide resolved
@patrobinson patrobinson merged commit 91f1b33 into main Dec 18, 2024
1 check passed
@patrobinson patrobinson deleted the chown-workspace-to-poduser branch December 18, 2024 06:41
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.

2 participants