Fix permissions error #298 and improve permissions in general

[zwimer]

This PR cleans up permission in dockerentry.sh in 3 ways:

1. Fixes Bug: dockerentry.sh chmod +x's all files in config. #298 by chmod-ing directories to 750, then effectively chmod-ing files to 640, but without removing any existing executable (x) permissions. I.e. executables will remain executables (to user and group)
2. These permissions should be set regardless of if DOCKER_NONROOT is set. This is both for consistency but also ensures consistency should a user try DOCKER_NONROOT then revert
3. chown when DOCKER_NONROOT is not set; this is in case a user switches from using DOCKER_NONROOT then back.

[Forceu]

Thanks for the PR. Wouldn't that remove the executable bit from /app/gokapi?

[zwimer]

From:

git checkout fix-298-permissions-error
docker build -t foo .
docker run --rm --name foo -d foo
docker exec -it foo /bin/ls -la /app

I see

total 49368
drwxr-x---    1 root     root            42 Aug 29 17:49 .
drwxr-xr-x    1 root     root            14 Aug 29 17:52 ..
-rw-r-----    1 root     root             0 Aug 29 17:41 .isdocker
-rwxr-x---    1 root     root      50544824 Aug 29 17:49 gokapi
-rwxr-x---    1 root     root           538 Aug 29 17:49 run.sh

So gokapi is user and group executable.

[spaghetti-coder]

In combination with UMASK PR, the result looks like this:

• given UMASK=0077, Gokapi creates files in /app/data with 0600 permission in runtime
• with the container restart, the permission on existing files forcefully changes to 0640. What is the point of UMASK than?

[zwimer]

This is a different PR from the linked PR, UMASK does not exist for this PR. If the others is merged first it may need to be modified but since it's not it does not. The current behavior is chmod -R 700 /app, which has the negative effect of +xing files into executables.

After the linked PR is merged, this can be changed if there is desire. To be honest I'm not sure why there is a chmod here in the first place as chown is more relevant in changing ownership during DOCKER_NONROOT. Ideally we can remove this line entirely and just assume previous permissions are correct. But @Forceu will have to weigh in on that.

[spaghetti-coder]

This is a different PR from the linked PR, UMASK does not exist for this PR.

Sorry, just taking an overall picture. And agree that chmod -R 700 /app is irrelevant.

To be honest I'm not sure why there is a chmod here

Agree with that. Excluding some edge cases changing permission and ownership is not a container entrypoint job.

[zwimer]

@Forceu I left the chmod in before to avoid breaking in case there was something I was unaware of, but unless there is a reason, can I remove it?

[spaghetti-coder]

Overall opinion. This MR introduces unnecessary complexity that also brings some issues.

1. find ... chmod ... is performed before chown, i.e. changing permissions before ownership is ensured.
2. 1 fails in a very basic scenario docker run -u 99 ..., because executables in the /app directory are owned by root.
3. chown -R ... /app fails in the same scenario that 2.
4. 1 and 2 change /app directory, which is not meant to be changed. The targets are config and data dirs.

Meanwhile the problem it targets can be solved by most standard docker practice without any changes to entrypoint

chown -R <UID>:<GID> config data \
&& docker container run -u <UID>:<GID> \
  -v "$(pwd)/config:/app/config" -v "$(pwd)/data:/app/data"
  ...

Permissions are not supposed to be manipulated in this manner, it's either umask setting suggested in #297, or no control at all

[zwimer]

1. find ... chmod ... is performed before chown, i.e. changing permissions before ownership is ensured.

Good catch, I'll fix that.

2. 1 fails in a very basic scenario docker run -u 99 ..., because executables in the /app directory are owned by root.

If it is owned by 99, it should work just fine. If /app is owned by root or other, I think this should fail since the program can't modify it. With that said, -e (as it wasn't in master) is not set so right now it shouldn't halt execution.

3. chown -R ... /app fails in the same scenario that 2.

No, it would work if the directory was already owned by that UID/GID. chowning something you already own is not an error. Otherwise it should fail for the same reason as mentioned in 2. With that said, -e (as it wasn't in master) is not set so right now it shouldn't halt execution.

4. 1 and 2 change /app directory, which is not meant to be changed. The targets are config and data dirs.

This does the exact same thing the current master branch does, it targets /app

[spaghetti-coder]

If it is owned by 99, it should work just fine. If /app is owned by root or other, I think this should fail since the program can't modify it.

In the current image /app is always owned by root on entrypoint stage. Unless it's a volume owned by somebody else

$ docker container run -ti -u 69 f0rc3/gokapi sh
/app $ id
uid=69 gid=0(root) groups=0(root)
/app $ ls -dl /app/
drwxr-xr-x    1 root     root          4096 Aug 29 12:49 /app/
/app $ chown -R $(id -u):$(id -g) /app
chown: /app/.isdocker: Operation not permitted
chown: /app/gokapi: Operation not permitted
chown: /app/run.sh: Operation not permitted
chown: /app: Operation not permitted
chown: /app: Operation not permitted

But again...
You are trying to solve a problem that doesn't exist. The problem is what you logged initially in #298, chmod -R 700 /app within $DOCKER_NONROOT = true block. Should it be changed? Your suggestion is definitely a behavioral change, but IMO it's also a breaking change. Depends on the author's vision on how data is used. The real problem is the whole $DOCKER_NONROOT = true block.

But what I can say for sure, there are no issues outside of $DOCKER_NONROOT = true block. The image performs fine with the conventional docker approach for non-root user (-u whatever), which is way more safe and cloud providers friendly than DOCKER_NONROOT tricks. With the chowning / chmodding and -u <non-root> it will produce unnecessary noise in the logs in best way. In worst it will also modify permissions, which is one more behavioral change now outside of $DOCKER_NONROOT = true block.

Just no need to fix what is not broken

[zwimer]

I see the current behavior as broken. The behavior you are describing is a no-op and log spam in the bad case, and rectifying ownership if it works, i.e. if root or if /app is owned externally. I suppose we could do-away with the log spam via 2>/dev/null though.

As for permissions, I agree- it'd be better to remove that line but since I don't know why they were set in the master branch I'm leaving it in unless the author says it is fine to do away with them.

If we end up documenting that the correct way to run the binary as non-root is -u in docker, I'd be a lot more in agreeance with you- and personally I agree that is a better solution. But right now I'm operating under the view that it is not, as the documentation implies the supported way is:

If you do not want the binary to run as the root user in the container, you can set the environment variable DOCKER_NONROOT to true.

[zwimer]

If you make a PR, and test it, removing the DOCKER_NONROOT stuff, updating documentation, and removing the permissions change (and that doesn't break anything), I'll support it though. I agree it is a better solution, it's just not what gokapi has right now.

[Forceu]

Thank you for your feedback, both sides are very valuable! Personally I do not have extensive experience with Docker, especially as I am mostly using non-root Podman instances.

I would be more than happy to accept changes to the current way the Docker image is run and to the documentation.
Are there any security concerns at the moment? And what is the "proper" Docker way to handle this issue?

Also sorry for closing the PR, that was a misclick.

[zwimer]

I think we both agree, uness there is some need to have root for setup when you start the container, the best way would be:

1. Setting UID/GID via docker directly, such as docker's --user flag instead of via an env var
2. Not setting permissions on mounted directories / files on startup, unless there is a specific reason to do so (remove the chmod -R +x unless it was put there because something has to be +x)
3. Not having docker env variables determine if the image is root or not (remove DOCKER_NONROOT)
4. Update documentation to reflect this all

The current setup determines root based on an env variable. If you need root initially or something, this works perfectly as you can do:

# do_root_stuff
# su-exec gokapi actual_command

In that case, the use of -u should be discouraged as the implication of that variable existing is that it is the way to run as non-root as opposed to -u as it is necessary to have root to start for some reason or another.

A caveat here is that when using --user, /etc/passwd, /etc/groups, etc, are not populated, meaning commands that require users and groups to be named within those files are likely to fail. But as long as you don't use such commands and rely on UID/GID rather than user/group names, this shouldn't be much of an issue.

[zwimer]

@spaghetti-coder If I missed anything above please let us know

[Forceu]

So it would be enough to simply run exec /app/gokapi "$@" in the Docker container as an entry point (basically what was done before introducing the option to run as non-root), if the user runs the docker container with the --user parameter? Are there any other implications?
The Gokapi container ships with a minimal Alpine base.

[spaghetti-coder]

If you make a PR, and test it,

Works for me.

And what is the "proper" Docker way to handle this issue?

Will shape a ticket shortly and will try to implement.

So it would be enough to simply run exec /app/gokapi "$@" in the Docker container as an entry point (basically what was done before introducing the option to run as non-root), if the user runs the docker container with the --user parameter? Are there any other implications?

With major version yes, but for now we need to keep backward compatibility.

[zwimer]

@Forceu @spaghetti-coder This is partially tangential, partially not due to the existing PR, but one thing that would be nice to have here is UMASK support. I.e.

if [ -n "$UMASK" ]; then umask "$UMASK"; fi 
exec /app/gokapi "$@" 

And maybe, optionally:

1. if [ -n "$DOCKER_NONROOT" ]; then >&2 echo "This methods is no longer supported; see <link to documentation>"; exit 1; fi or something along those lines.
2. Verify mounted directories in /app/ are owned by $(id -u):$(id -g) and error or warn if not.

[zwimer]

@spaghetti-coder If you do make a PR, if you wouldn't mind commenting "Conflicts with " on the other two PRs. I'll leave them open until yours is merged in case it ends up not being so, so the comment would be appreciated. Also ideally link the related issues the PRs address

[spaghetti-coder]

@zwimer Sure

but one thing that would be nice to have here is UMASK support

Was my intention, but turned out to be not that simple #325. At least not in this scope

[spaghetti-coder]

Conflicts with #327