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

campaigns: create files/directories that might be mounted into the container with global permissions #365

Closed
LawnGnome opened this issue Nov 4, 2020 · 0 comments · Fixed by #366
Assignees
Labels
bug Something isn't working team/code-search

Comments

@LawnGnome
Copy link
Contributor

LawnGnome commented Nov 4, 2020

Scenario

  1. The user is running on an environment where UIDs are mapped directly into Docker containers (so Linux, not macOS).
  2. The user is not running as root, but as a normal user (eg 1000).
  3. The container being run does not run as root, but runs as a different user that does not match the user's UID outside the container. (eg 2000)

Therefore, anything we naïvely bind mount into the container may have permission issues.

Shell scripts

As part of executing steps, src-cli creates a temporary file with the shell script to be executed and volume mounts it into the Docker container. This file is created using ioutil.TempFile, which creates all files with permission 0600 as the current user.

Therefore, we end up in a scenario where there's a file owned by 1000, with permission 0600, that is being executed by a shell in a container running as 2000, and that fails with a permission error.

There's no particular reason that temporary file needs to be 0600, so we should create it as 0644 and sidestep this issue entirely.

Workspaces

Similarly, src-cli unzips the repository being modified into a temporary directory. This directory and its children are created with minimal permissions, and are subject to the umask, both of which are likely to prevent writing to the workspace in a mixed-UID environment like that described above.

This one's a bit ickier: the easiest fix here (which is what we'll do to start with) is to replicate the user permissions out to the group/other bits on the workspace's files and directories, which essentially matches what Git does (check the user executable bit and nothing else), but it has the potential for subtle breakage if the steps that are being run assume more about the permission structure of the repository.

It may be possible to probe the container's UID and use ACLs to get more fine-grained behaviour, but this has some pretty tricky scenarios if there are multiple UIDs at play: if the first step runs as 2000 and the second step as 2001, we'd have to manipulate the ACL on the fly.

Finally, the other option I considered was user namespaces, but since these make changes to the global behaviour of Docker and require a dockerd option, that's a non-starter.

@LawnGnome LawnGnome added bug Something isn't working team/code-search labels Nov 4, 2020
@LawnGnome LawnGnome added this to the Campaigns Sprint 4 milestone Nov 4, 2020
@LawnGnome LawnGnome self-assigned this Nov 4, 2020
@LawnGnome LawnGnome changed the title campaigns: create temporary files that might be mounted into the container with global read permissions campaigns: create files/directories that might be mounted into the container with global read permissions Nov 4, 2020
@LawnGnome LawnGnome changed the title campaigns: create files/directories that might be mounted into the container with global read permissions campaigns: create files/directories that might be mounted into the container with global permissions Nov 4, 2020
LawnGnome added a commit that referenced this issue Nov 4, 2020
This fixes #365 by ensuring that files and workspaces mounted into
campaign containers are world readable, writable, and executable as
appropriate.
LawnGnome added a commit that referenced this issue Nov 5, 2020
This fixes #365 by ensuring that files and workspaces mounted into
campaign containers are world readable, writable, and executable as
appropriate.
scjohns pushed a commit that referenced this issue Apr 24, 2023
This fixes #365 by ensuring that files and workspaces mounted into
campaign containers are world readable, writable, and executable as
appropriate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working team/code-search
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant