Skip to content

Conversation

@javabrett
Copy link
Contributor

This removes any file-permission ambiguity that can result from a different umask set at git checkout
time on developer's or build machine.

This problem was noticed when running a buildx on MacOS with a umask set of 0077. Git doesn't do anything special with rw attributes, only x, so these are inherited from the checkout user's umask. Since Docker passes these through, this seems to be a flaw in build-portability of Git Checkout -> Docker Build (same build args) -> Image. So my image fails to run as minecraft:minecraft due mainly to missing group/world r permissions ... x that is added currently is not sufficient and will fail immediately with:

/bin/bash: /start-configuration: Permission denied

This PR adds --chmod to all COPY instructions in the Dockerfile, to force consistency. Note that COPY --chmod is buildkit and not plain Docker, but that is already required.

Testing

This command (apologies for the quoting) attempts to ls all the files copied-in to the image, and compare the permissions from the current image with one from a local build.

diff <(docker run --rm --entrypoint /bin/bash itzg/minecraft-server -c "ls -l /etc/sudoers.d /start* /usr/local/bin /health* /tmp /autopause | awk '{print \$1\" \"\$3\" \"\$4\" \"\$9}'") <(docker run --rm --entrypoint /bin/bash minecraft-server -c "ls -l /etc/sudoers.d /start* /usr/local/bin /health* /tmp /autopause | awk '{print \$1\" \"\$3\" \"\$4\" \"\$9}'")

Note the first diff input is from itzg/minecraft-server, the second is local tag minecraft-server.

For a "bad" build with umask 0077, there will be a lot of diffs, and the bad side looks like this:

-rwx------ root root /health.sh
-rwx--x--x root root /start
-rwx--x--x root root /start-autopause
-rwx--x--x root root /start-configuration
-rwx--x--x root root /start-deployAirplane
-rwx--x--x root root /start-deployBukkitSpigot
-rwx--x--x root root /start-deployCanyon
-rwx--x--x root root /start-deployCatserver
-rwx--x--x root root /start-deployCF
-rwx--x--x root root /start-deployCrucible
-rwx--x--x root root /start-deployCustom
-rwx--x--x root root /start-deployFabric
-rwx--x--x root root /start-deployForge
-rwx--x--x root root /start-deployFTBA
-rwx--x--x root root /start-deployLimbo
-rwx--x--x root root /start-deployMagma
-rwx--x--x root root /start-deployMohist
-rwx--x--x root root /start-deployPaper
-rwx--x--x root root /start-deployPurpur
-rwx--x--x root root /start-deploySpongeVanilla
-rwx--x--x root root /start-deployVanilla
-rwx--x--x root root /start-finalExec
-rwx--x--x root root /start-setupEnvVariables
-rwx--x--x root root /start-setupModconfig
-rwx--x--x root root /start-setupModpack
-rwx--x--x root root /start-setupMounts
-rwx--x--x root root /start-setupServerProperties
-rwx--x--x root root /start-setupWorld
-rwx--x--x root root /start-spiget
-rwx--x--x root root /start-utils

/autopause:
total
-rwx--x--x root root autopause-daemon.sh
-rwx--x--x root root autopause-fcns.sh
-rw------- root root knockd-config.cfg
-rwx--x--x root root pause.sh
-rwx--x--x root root resume.sh
...

Note the odd 711 perms.

Built with this PR, there are no diffs between official and locally-built image.

Downside - need to remember to add --chmod to any future COPY commands.

Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

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

Very nice solution. This is good to merge as is, but wanted to hear your thoughts on old chmod +x removal.

This removes any file-permission ambiguity that can result from a different umask set at git checkout
time on developer's or build machine.
@javabrett
Copy link
Contributor Author

Rebased, added two commits accepting feedback. Note that there is now this difference between my image built on MacOS, and the existing master image:

< -rw-r--r-- root root knockd-config.cfg
---
> -rwxr-xr-x root root knockd-config.cfg

I actually started-out with this modifying those RUN ... chmod +x changing to +rw, before realising there were a bunch of other config files missing r, and COPY --chmod seems like the right declarative fix for the copy permissions, so good if we can avoid separate chmods where possible.

@javabrett javabrett requested a review from itzg December 12, 2021 10:57
Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

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

Excellent. Thanks for the changes and the thorough analysis

@itzg itzg merged commit c4aa105 into itzg:master Dec 12, 2021
@javabrett javabrett deleted the copy-with-chmod branch December 14, 2021 10:24
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