[BUGFIX] Fix for docker(-compose) to allow the app to be run as a regular user.

[justSem]

Description

Fixes #857 and #694

Due to the calling of setpriv as an regular user and some weird env variable stuff which seemed set wrongly to me the application couldn't be launched in docker as a non-root user.
(And really, an application like this shouldn't run as root as it simply doesn't need it)

The proposed fix properly sets the environment variables reflecting the actual UID and GID and asseses this before launching.

An user switching an existing setup from root to non-root user might need to manually chown the /app/data directory once depending on how they set up the storage. This does not involve altering any container files.

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• My code follows the style guidelines of this project
• I ran ESLint and other linters for modified files
  • Not applicable. I modified a bash file ;)
• I have performed a self-review of my own code and tested it
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings

Screenshots (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

[k3rnelpan1c-dev]

While this is certainly one way to handle rootles container or containers that run as non root account due to CLI params (i.e. --user 1001:1001), would it not be a more standard way to let the container run as a non root user by default?

What I mean by that is that most of the entrypoint script could be made obsolete and to simply create a user+group during the container build and ensure that the /data dir gets creted and properly assigned.

As I have to deal with this a lot at work some reference from a platform that enforces the container to run as non root unless you explicitly allow it otherwise: OpenShift has a section in their docs on how to build a container so it supports arbitrary user ids

I know this works, since I ended up building a custom image that uses the recomendation from the docs as well as what Nginx uses for their unpribileged container to test uptime-kuma in an unprivileged enforcing environment.
https://github.com/nginxinc/docker-nginx-unprivileged/blob/main/stable/debian/Dockerfile#L20-L21

However, the question remains if this would be the 'better' way or to keep the entrypoint script and use the fix proposed by this PR.

Either way, these are just my two cents to the matter that came up when I saw that you where faster than me to propose changes 🙂

[justSem]

While this is certainly one way to handle rootles container or containers that run as non root account due to CLI params (i.e. --user 1001:1001), would it not be a more standard way to let the container run as a non root user by default?

What I mean by that is that most of the entrypoint script could be made obsolete and to simply create a user+group during the container build and ensure that the /data dir gets creted and properly assigned.

As I have to deal with this a lot at work some reference from a platform that enforces the container to run as non root unless you explicitly allow it otherwise: OpenShift has a section in their docs on how to build a container so it supports arbitrary user ids

I know this works, since I ended up building a custom image that uses the recomendation from the docs as well as what Nginx uses for their unpribileged container to test uptime-kuma in an unprivileged enforcing environment. https://github.com/nginxinc/docker-nginx-unprivileged/blob/main/stable/debian/Dockerfile#L20-L21

However, the question remains if this would be the 'better' way or to keep the entrypoint script and use the fix proposed by this PR.

Either way, these are just my two cents to the matter that came up when I saw that you where faster than me to propose changes 🙂

You are probably right. My changes are fairly "quick 'n dirty" because I didn't have too much time on my hands to fully dive into what his entrypoint is doing and why. And since I suck in anything node/js related I figured it just might be a requirement for some node stuff, hence I did not investigate any further. So I've edited the script to run as it was when running as root, and like this when it gets invoked as a normal user.

The main reason I proposed these changes is because it created an - as far as I've tested - fully working/functioning uptime-kuma while running as a normal user. Said user would then be given in the docker cli command or the docker-compose file. That gives the user more control over as which user the container runs.

So in the end I think both solutions could be fine to achieve the goal - though I do agree that I already had a feeling that most of the entrypoint's script lines might be obsolete. (But again - since I've got 0 experience in NodeJS I didn't research it)

[k3rnelpan1c-dev]

from what I can tell the scrip has nothing NodeJS specific.
It really just ensures that the /data dir exists and has its owner set 'properly' (with your fix it actually does that, b4 you would have had to specify PUID and PUID) and spawns of the NodeJS process as the same user the data dir now has as its owner.

At least that is what I could gather from it :)

Regardless, all I wanted to do is throw in the comment and open a discussion which way is the preferable one.

Either works since even if you where to create a user within the container build and run by default as that a user could still use the container runtime CLI (Docker, Podman, ...) to overwrite the user and group the container will run as would then lead to the NodeJS process to run as those ids.

There is only one instance where I am not 100% certain how it would react, if a user where to use a docker volume to mount on a pre-created /data dir (created at container build time and owned by the user created at build time) but setting a different UID and GID via the CLI or a thing such as Compose.
The likely 'solution' for that would be to set the /data dir at creation to 777.
(hope that made sense)

[justSem]

There is only one instance where I am not 100% certain how it would react, if a user where to use a docker volume to mount on a pre-created /data dir (created at container build time and owned by the user created at build time) but setting a different UID and GID via the CLI or a thing such as Compose. The likely 'solution' for that would be to set the /data dir at creation to 777. (hope that made sense)

I tested that last scenario. In the case a user has a /data volume which has been created as root (or any other user for that matter) the container will fail to launch because it cannot chown something it has no permission for.
It's of course simple to fix by just chmod-ing the directory yourself before launching the container, but it could be prettier :)

On a sidenote: There's no real use-case to chmod a folder like that to 777 as it brings security implications.
Imho the 'neat' fix in this part would be to alter the entrypoint to firstly check if the permissions are set properly, if not it should try to set them - when that fails it should cleanly exit with a proper error message and instructions as this particular problem is easy to pinpoint and guide the user in.

[k3rnelpan1c-dev]

Thanks for testing my assumed edge case 😅
And yeah I assumed as much that it would run into this issue.
Setting the dir to 777 is NEVER a good idea so my apologies for bringing it up at all, was just throwing it out there for the only way to avoid permission errors I suppose.

I like the idea of giving the entrypoint script the porous of a environment checker and presenting the directions to fix possible issues (i.e. legacy permissions or plain wrong bind mount perms).

[justSem]

Thanks for testing my assumed edge case 😅 And yeah I assumed as much that it would run into this issue. Setting the dir to 777 is NEVER a good idea so my apologies for bringing it up at all, was just throwing it out there for the only way to avoid permission errors I suppose.

I like the idea of giving the entrypoint script the porous of a environment checker and presenting the directions to fix possible issues (i.e. legacy permissions or plain wrong bind mount perms).

I've altered the script to (attempt) to work around this edge-case.
It now assesses ownership, directory and file permissions, and tries to sets them properly.

If it fails it'll exit with an error message instructing on how to resolve the problem.

[Saibamen]

It would be great to have /app/data in variable (used 9 times here)

[justSem]

@Saibamen Thank you for your feedback and approval! :)

[gaby]

@justSem Doesn't running as a regular user break the ability of sending pings? I remember a long time ago someone else tried this and it broke the Docker image.

[justSem]

@justSem Doesn't running as a regular user break the ability of sending pings? I remember a long time ago someone else tried this and it broke the Docker image.

No, ping isn't a privileged unix process.
I run my instance with this script and ping works just fine

Edit: could you reference the issue that would break ping? I'm curious

[k3rnelpan1c-dev]

While I don't run this script yet, I have Uptime-Kuma deployed in the restrictive SCC (Security Context Constraint) of Open Shift and it works just fine.

*I built my own image with a slightly altered version of this script though, since the restricted SCC wont allow for any privileged or root running pod, even if its just at startup.

[gaby]

@justSem Here #488 it was a long time ago though.

[Kurt108]

@k3rnelpan1c-dev could you share your script? Getting Kuma running on Openshift and with a persistent Volume is now as easy as it should be :-o

[openstep]

While I don't run this script yet, I have Uptime-Kuma deployed in the restrictive SCC (Security Context Constraint) of Open Shift and it works just fine.

*I built my own image with a slightly altered version of this script though, since the restricted SCC wont allow for any privileged or root running pod, even if its just at startup.

Hi,

could you please share the info necessary running this in OpenShift?
Many thanks.

[k3rnelpan1c-dev]

For those curious, yes I will share my config for OCP in a new issue, for the rest sorry to have disrupted the PR topic by my comment unexpectedly 😅

Edit: So done, hope everyone curious for OpenShift can find what they are looking for in #1531

[beneiltis]

When are you gonne merge this? :-)

[kaysond]

This would solve my problem of not having root access to my fs!

[justSem]

@Saibamen @louislam Is this going to get merged on any point? Or are there any changes required?

[Saibamen]

@justSem: I don't have write permissions

[SirSirae]

@louislam may someone merge this it would be really helpful ! Thank you !

[kaysond]

Bump. @louislam can we please get this into the next release?

[kaysond]

Bumping again and pinging @louislam

[Marck]

@louislam any change this could come in a release?
I would love to see, I have a volume that is mounted under a non-root user for all my containers use but I cannot use that because this runs as root (which is not needed)

[chakflying]

Obsolete by #4052.