feat(docker): run service under node user

[ChrisRomp]

Fixes #783

Run under the existing node user, which as a UID/GID of 1000.

[coveralls]

Pull Request Test Coverage Report for Build 917561288

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 17.555%

---

Totals
Change from base Build 892292172:	0.0%
Covered Lines:	2024
Relevant Lines:	11899

---

💛 - Coveralls

[robertsLando]

LGTM for me. I also would like to know @chrisns thoughts on this

[robertsLando]

I've run the container and it looks like it's ok, but I haven't tested it with a controller attached.

Could you try to also check it works with a controller attached (real use case)?

[chrisns]

why do we need to chown everything?

[ChrisRomp]

Maybe we don't. I'll try it without.

[chrisns]

I feel like it may need some seccomp config to talk to the zwave interface be it usb/serial etc if it's not root too?

[ChrisRomp]

As-is it doesn't work because config/store needs to open up some .json files as read/write, and all of the files have a 755 mask. Even with that path mounted as a volume doesn't work because the parent /usr/src/app/store folder has the same mask.

So we'll either need to chmod the files at container build time (testing this), or chown them to node. Any preference, @chrisns?

I'm still open to working out how to make the UID/GID for the node user available to modification at runtime by environment vars, but that will require some additional things added to the container to run some scripts as root at startup to modify the user.

[chrisns]

the issue is that we've not defined the VOLUMES more clearly which may have made this slightly less painful

a startup script to chmod kinda defeats the point since it'll need to run as root.

you could introduce a breaking change that checks and bails out early if it fails with some helpful feedback to fix the permissions or do user mapping, user mapping would be preferable anyway to avoid clashing with other things on the host machine

for the minute you could achieve all of what you're setting out to do with an update to the docs, kube+docker-compose config with what we recommend you run it with e.g.

export HOST_UID=123 # some random number that won't clash, or use UID 0 if you want to run just the container namespacing but still root host mapping (ugly)
docker run -u $HOST_UID:1000 -v $(pwd)/data:/data

or whatever

gonna move this PR to a draft if thats ok? until we've got a clear answer that works

[ChrisRomp]

I'll fire up this image on my home controller later today.

[ChrisRomp]

This introduces a breaking change using this approach. If the previous /user/src/app/store was mounted as root and now we run as node (even if we copy with the --chown=node:node argument), the old files are still owned by root and node gets permission denied trying to update anything in store (since that volume isn't available at container build time).

The fix for this is the same as the fix for making the UID/GID for the node user dynamic, and that's to trigger a container startup script to set everything the way it "should be" before launching the node app (UID/GID and chown the app or store folder(s). I still think we should chown app to the node user at that time, but we could possibly just worry about the store folder and leave everything else as root (it would be faster, anyway, since we can skip node_modules). It shouldn't matter at that point if the perms are set properly.

I can either keep this PR open and update it later with that change (good for keeping context), or I can open a new one at that time.

[chrisns]

the issue is that we've not defined the VOLUMES more clearly which may have made this slightly less painful

a startup script to chmod kinda defeats the point since it'll need to run as root.

you could introduce a breaking change that checks and bails out early if it fails with some helpful feedback to fix the permissions or do user mapping, user mapping would be preferable anyway to avoid clashing with other things on the host machine

for the minute you could achieve all of what you're setting out to do with an update to the docs, kube+docker-compose config with what we recommend you run it with e.g.

export HOST_UID=123 # some random number that won't clash, or use UID 0 if you want to run just the container namespacing but still root host mapping (ugly)
docker run -u $HOST_UID:1000 -v $(pwd)/data:/data

or whatever

gonna move this PR to a draft if thats ok? until we've got a clear answer that works

[ChrisRomp]

Sounds good.

[robertsLando]

This introduces a breaking change using this approach. If the previous /user/src/app/store was mounted as root and now we run as node (even if we copy with the --chown=node:node argument), the old files are still owned by root and node gets permission denied trying to update anything in store (since that volume isn't available at container build time).

Is there a way to prevent this kind of breaking change? maybe a script that fix the permissions on files on store on startup?

[chrisns]

@robertsLando no. See my response

[robertsLando]

If not I'm not 100% sure I want this, as it would break ALL the instances. We should find a workaround

[chrisns]

@robertsLando Yes, hence why I suggested (only) a change to docs unless we can think of a better way and moved this to draft :)

[ChrisRomp]

Yes, I'm confident if I do this the "right way" instead of just changing the running user to node it'll work. Give me a little bit of time to work on this (between other things, as you know) and I'll document and demonstrate it. :)

[robertsLando]

@ChrisRomp Ok no problem :)

[robertsLando]

news here?

[ChrisRomp]

Working on a couple of angles. Been super swamped lately and changed jobs, but I wanted to try a couple of things to either make it work or put it away. I'll know something soon, or just close the PR if I decide I don't have time right now to do it right.

The thing is I can do it, but I don't want to bloat the container image, so I'm looking for the lightest way of accomplishing the goal. Also, it's further complicated by the fact that the container currently runs as root, so everyone running it today has files under store owned by root, which I'll have to manage as well.

First I'm going to try and have it just run under UID 1000 which is the node user in the base container. Then if that works alright, I'll see about making it variable based on the original issue, #783.

Would you want to see this as a separate Dockerfile, or just see this behavior integrated into the main Dockerfile?

[ChrisRomp]

One other - way simpler - option to consider would be to just have the Dockerfile add USER node before CMD, and it would work fine on new installs. Something to keep in mind for future containers, anyway.

Existing users would have to chown -R 1000:1000 /path/to/store/volume first. That's the breaking change mentioned previously. Or they could fix it from Docker as: docker run --user 0 -v [volume]:/usr/src/app/store zwavejs/zwavejs2mqtt:[tag] chown -R 1000:1000 /usr/src/app/store before running the image after this change.

But to fix it without introducing a breaking change, I'm just looking for a lightweight init system that will run at container startup as root every time the container boots to chown the files to the specified user, then let the CMD stuff take place as the non-root user. Lots of Alpine-based containers do this, but I'm evaluating the bloat cost.

[ChrisRomp]

Adding this for reference: https://github.com/nodejs/docker-node/blob/main/docs/BestPractices.md#non-root-user

By default, Docker runs container as root which inside of the container can pose as a security issue. You would want to run the container as an unprivileged user wherever possible. The node images provide the node user for such purpose.

[ChrisRomp]

Ok here's my lightest-weight approach:

1. Add entrypoint.sh script which configures permissions for the node user (script runs as root), then executes the node app as the node user (exec su -p node -c "node bin/www").
2. Copy the entrypoint.sh script to the container at /
3. Update Dockerfile to exclude the container-scripts directory (I can relocate this if desired)

I've added a Dockerfile.nonroot that sets the entrypoint to /entrypoint.sh so this can be evaluated in isolation, but I think if everyone is ok with this approach we can put it in the main Dockerfile.

FYI I'm running this in my home now as my main zwavejs2mqtt instance. It successfully remapped the ownership of the files under store. I only use the zwavejs server (not doing MQTT here), however.

(looks like my linter killed a bunch of trailing spaces in Dockerfile -- I can put them back to minimize the diff if you'd like)

[ChrisRomp]

(requested change/review out of date)

[robertsLando]

Looks ok to me, I would like to know @chrisns opinion too

[chrisns]

What you've done is an anti pattern and I'm not sure really what it's achieving. And worse if you run docker -u 1000 which is the right answer the entry point will fail.
I stand behind previous suggestion that the answer here really is additional documentation not code
Sorry

[ChrisRomp]

Let me start by saying I'm super appreciative of all the work that you all put into this project (and zwave-js), and I use this at home. My work here is trying to improve it and take some of that load off of your shoulders. But I've maxed out my spare bandwidth on this for now, so I'm unable to put more time in right now (hopefully I'll be able to again sometime in the near future), so I'm going to close out this PR.

With so many platforms making it easier for slightly-more-than-average tech savvy users enabled to run Docker containers, I'm going to suggest that the anti-pattern was having the container run as root in the first place, and that the users who know better don't need the documentation to tell them so; the users who don't know any better won't read the documentation to that level of detail. I'm not sure why there doesn't seem to be an appetite to fix this now, but that's not my call, and I can work around this issue my own way on my own systems.

The "right" fix is to add the USER node directive before the CMD in the Dockerfile, but that will mess up users who have run this previously as their volume will have all of its files owned by root. Maybe at your next major version change -- a breaking change by semantic versioning guidelines -- that would be a good time to introduce such a change. In the meantime it might be worthwhile to have a separate Docker tag that runs as the non-root user, per the container base image's own guidelines.

[chrisns]

Hey,
Sorry if my last reply was blunt, your contributions and thoughts are really welcomed and valued.

I agree to a degree with a lot of your points around running as non-root by default is preferable, and a breaking change applied via semver may be possible and for the greater good.

However my reservations with applying this to novice users is largely around Docker and the linux user/volume mapping that occurs and how it might actually cause more problems that would be harder for someone to fix.

If the USER 1000 is set in the Dockerfile, then the files that docker writes will be owned by user 1000 on the host too, depending on the configuration of the host (and other containers running on the host) you could have all sorts of unintended side effects because of that.

It's only the pesky state that makes this difficult.

Alternative container runtimes such as podman do a much better job of isolation in this regard.

[ChrisRomp]

I don't take blunt as bad, no worries. I really am just out of time and it's entirely your call how this operates. I was looking for a way to chip in a little while back and saw an open issue I felt I could take on. If time permits I'll be back trolling your issues list for more later on.

If the USER 1000 is set in the Dockerfile, then the files that docker writes will be owned by user 1000 on the host too, depending on the configuration of the host (and other containers running on the host) you could have all sorts of unintended side effects because of that.

You could make the same argument for the files being owned by root on the host as well, not to go round-and-round. 😉

[robertsLando]

@ChrisRomp I'm not against this to land, I just want to be sure to don't introduce any breaking change for users.

[github-actions]

This pull-request is stale because it has been open 90 days with no activity. Remove the stale label or comment or this will be closed in 5 days. To ignore this pull-request entirely you can add the no-stale label

[github-actions]

This pull-request is now closed due to inactivity, you can of course reopen or reference this pull-request if you see fit.