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

Dockerfile: don't create gluon user (fixes support for UID != 1000) #2870

Closed
wants to merge 2 commits into from

Conversation

stweil
Copy link
Contributor

@stweil stweil commented Apr 19, 2023

The new user gluon gets UID 1000 and GID 1000 and cannot write to the mounted gluon directory unless the calling user has the same UID.

On Linux only the first non admin user gets UID 1000, so docker and podman fail for all other users.

On macOS there is typically no user with UID 1000, so docker and podman fail for all users.

When no gluon user is created, a default user with the same UID and GID as the calling user is used and everything works fine.

@stweil
Copy link
Contributor Author

stweil commented Apr 19, 2023

This pull request replaces #2862 and #2868. @hafu, does this change work for you? Maybe the original author of Dockerfile can also review my pull request.

@AiyionPrime AiyionPrime added the 2. status: waiting-on-review Awaiting review from the assignee but also interested parties. label Apr 19, 2023
@stweil
Copy link
Contributor Author

stweil commented Apr 19, 2023

@mweinelt, are you the author of Dockerfile? If yes: is the gluon user required, or can it be removed?

@mweinelt
Copy link
Contributor

The build should simply not run as root, not even in a container.

Instead https://docs.podman.io/en/latest/markdown/options/userns.container.html#userns-mode should be used with podman.

@stweil
Copy link
Contributor Author

stweil commented Apr 19, 2023

@mweinelt, that was my first solution (#2862), but it requires a very recent podman, so does not work with Debian stable for example.

With the patch here, docker and podman behave differently: in the docker container the user has UID 0 and GID 0 (= root), but in the podman container the user gets UID and GID from the outside user which is the desired behaviour.

@neocturne
Copy link
Member

Let me check a few things, I also have a branch for improving this, but I still need to verify that my approach works in both Docker and Podman.

Also note that I don't think the commit message of this PR is correct. The Docker image should already work fine for UIDs != 1000. The created passwd entry will be wrong, but that shouldn't matter?

@stweil
Copy link
Contributor Author

stweil commented Apr 19, 2023

The Docker image should already work fine for UIDs != 1000.

Did you test that? In my test on a Debian host it does not.

@stweil
Copy link
Contributor Author

stweil commented Apr 19, 2023

The build should simply not run as root, not even in a container.

For podman it does not run as root. And docker can be called with --user $(id -u):$(id -g). That seems to work, but has the disadvantage that it uses UID and GID without corresponding entries in /etc/passwd and /etc/groups.

The new user `gluon` gets UID 1000 and GID 1000 and cannot write to the
mounted gluon directory unless the calling user has the same UID.

On Linux only the first non admin user gets UID 1000, so docker and podman
fail for all other users.

On macOS there is typically no user with UID 1000, so docker and podman
fail for all users.

When no `gluon` user is created, a default user with the same UID and GID
as the calling user is used and everything works fine.

Signed-off-by: Stefan Weil <[email protected]>
@github-actions github-actions bot added the 3. topic: build This is about the build system label Apr 20, 2023
@stweil
Copy link
Contributor Author

stweil commented Apr 20, 2023

@mweinelt, I added a commit which changes the UID and GID in docker. It no longer runs as root.

Copy link

@hafu hafu 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 to work for podman and docker. The only downside is, that commands in the container are run as root. I don't know if it could be a problem.

@@ -16,7 +16,7 @@ then
elif [ "$(command -v docker)" ]
then
docker build -t "${TAG}" contrib/docker
docker run -it --rm --volume="$(pwd):/gluon" "${TAG}"
docker run -it --rm --user $(id -u):$(id -g) --volume="$(pwd):/gluon" "${TAG}"
Copy link

Choose a reason for hiding this comment

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

Suggested change
docker run -it --rm --user $(id -u):$(id -g) --volume="$(pwd):/gluon" "${TAG}"
docker run -it --rm --user "$(id -u)":"$(id -g)" --volume="$(pwd):/gluon" "${TAG}"

At least shellcheck throws a warning in this part. (Quote this to prevent word splitting)

@hafu
Copy link

hafu commented Apr 20, 2023

I suggest to add --userns=host to the docker run command. If user namespaces are enabled in Docker we run into permission issues with the bind mount. --userns=host disables the remapping.

@stweil
Copy link
Contributor Author

stweil commented Apr 20, 2023

@AiyionPrime, I noticed that you cancelled some CI checks for this PR. That's good, because they cost a lot of computing resources and also waste energy. Maybe it would be good to run them only on demand (triggered by a team member) or for changes in selected files. Even then a smaller subset of checks would typically be sufficient.

@neocturne
Copy link
Member

The Docker image should already work fine for UIDs != 1000.

Did you test that? In my test on a Debian host it does not.

Ah, I meant with Podman, not with Docker - --userns=keep-id even edits the passwd to allow resolving UIDs to usernames etc. I don't think we should change anything for the Podman case.

With Docker, the --user solution provides some improvement, but as no passwd editing happens, this results in an ugly "I have no name!" bash prompt.

@stweil
Copy link
Contributor Author

stweil commented Apr 20, 2023

Podman does not work with the current code and UID != 1000. I noticed that on macOS, and that was my initial motivation to look for a fix.

The default bash prompt with "I have no name" when using docker is ugly. But changing the bash prompt to something which looks nice is very simple. So if that is the only remaining problem, I'd suggest to merge my PR, then discuss which prompt is desired and implement that in one more commit.

@neocturne
Copy link
Member

Podman does not work with the current code and UID != 1000. I noticed that on macOS, and that was my initial motivation to look for a fix.

Ah, I wasn't aware of this interaction between --userid=keep-id and USER.

Removing the USER isn't a nice solution though, as it will break other usecases of the container image that don't involve mounting into the container. Instead, we should not touch the Dockerfile at all (except for the bash prompt maybe) and instead add the --user option to the podman run command as well to override the USER setting.

@stweil
Copy link
Contributor Author

stweil commented Apr 20, 2023

Keeping Dockerfile unmodified would be possible of course, although the created gluon user would not be used by scripts/container.sh. If that is desired, I (or any maintainer) can update the PR accordingly.

I don't know other use cases, so cannot comment on them.

@neocturne
Copy link
Member

Fixed by #2975

@neocturne neocturne closed this Sep 11, 2023
@stweil stweil deleted the podman branch September 11, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. status: waiting-on-review Awaiting review from the assignee but also interested parties. 3. topic: build This is about the build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants