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

Docker: Allow selection of (unprivileged) UID/GID at build time #418

Merged
merged 7 commits into from
Apr 3, 2022

Conversation

lkubb
Copy link
Contributor

@lkubb lkubb commented Oct 9, 2021

I created a pull request to discuss my proposal in a dedicated thread and allow merging, if desired. I hope that's fine and I'm doing everything correctly, as I said, I'm mostly a lone lurker on here. Previous discussion is in #345, which is related. I made the newbie mistake and commited to main, so I had to clean up a little, which is why the previous links are a bit borked.

This PR introduces the option to create a Docker container running as an unprivileged user while retaining the previous default of running as root, and while still being based on FROM scratch. This avoids a breaking change for existing users and keeps the image as lean as it is. UID/GID can optionally be specified as well.

Setting the build argument RUNAS to anything will trigger the unprivileged mode. If it's unset, Docker will happily default to root in both COPY --chown=${RUNAS} and USER ${RUNAS}. To avoid an error when the glob /tmp/useradd/* returns nothing, I had to create a dummy file when building for root. The (relatively speaking) resource-heavy steps of compiling can be reused from cache when building different variants of the image.

  • docker build -t transfersh-root . will result in an image which runs the container as root. The only change from before is the dummy file /etc/unused.
  • docker build -t transfersh-user --build-arg RUNAS=plsanythingbutroot . will result in an image that runs the container with the default UID:GID of 5000:5000.
  • docker build -t transfersh-user-custom --build-arg RUNAS=verycustomsir --build-arg PUID=1337 --build-arg PGID=1338 . will result in an image that runs the container with UID:GID of 1337:1338.

Apart from the good conscience of running as an unprivileged process, this also allows easy selection of an ID, e.g. when mapping NFS-mounted folders as the base dir. Furthermore, it would be possible to provide a separate official image like transfersh:noroot without much hassle. Current users wanting to switch manually would have to run a simple sudo chown -R $UID:$GID ./data on their host.

Fixes #100.

@aspacca
Copy link
Collaborator

aspacca commented Oct 16, 2021

@lkubb , great!

are you able to update the documentation as well: https://github.com/dutchcoders/transfer.sh#docker? (adding an how to build step)

the best would be to add as well another github workflow (https://github.com/dutchcoders/transfer.sh/blob/main/.github/workflows/build-docker-images.yml) in order to build the non-root image :)

@uptimejeff
Copy link

Thanks for your work on this... running as root didn't work for me either :-)

Original build command from directions
docker build -t transfersh-user --build-args RUNAS=plsanythingbutroot .
Which returns
unknown flag: --build-args

Switched to arg (singular) and ran
docker build -t transfersh-user --build-args RUNAS=plsanythingbutroot .
The build ends here

Step 11/18 : LABEL maintainer="Andrea Spacca <andrea.spacca@gmail.com>"
unexpected EOF

@lkubb
Copy link
Contributor Author

lkubb commented Oct 21, 2021

Sorry, I was tired when I wrote the description above. It should say --build-arg, no plural. :) Adding a how to and workflow is on my plate, just have a lot of stuff to do atm.

Edit: Oh, saw you switched to singular and it didn't build, that's strange. I will look into it. Which Docker version are you running? And can you reproducibly build the current Dockerfile, but not the one in this PR?

@lkubb
Copy link
Contributor Author

lkubb commented Mar 24, 2022

Sorry for the long delay. I rebased on current master (correction: main), finally added documentation and updated the container build workflow.

Please take a close look, especially if the changes to the workflow seem reasonable since this is my first time working with them. Thanks for your patience :)

README.md Outdated Show resolved Hide resolved
@aspacca
Copy link
Collaborator

aspacca commented Apr 2, 2022

hi @lkubb

thank you very much and I apologise as well for taking a little time to review.

I would just ask you to address the comment I left: I will wait a couple of days and then merge anyway and "fix" on my own :)

@lkubb
Copy link
Contributor Author

lkubb commented Apr 3, 2022

No worries, I'm glad I'm in the position to contribute and you're happy with the result. :) Just addressed your comment and rebased again on main. Cheers!

@aspacca aspacca merged commit bb0891c into dutchcoders:main Apr 3, 2022
@lkubb lkubb deleted the docker-no-root branch April 3, 2022 18:07
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.

[Security Issue] Docker containers' processes run as root
3 participants