Skip to content

Change Docker image to non-root usage - #477

Open
Apfelwurm wants to merge 1 commit into
orangecoding:masterfrom
Apfelwurm:master
Open

Apfelwurm wants to merge 1 commit into
orangecoding:masterfrom
Apfelwurm:master

Conversation

@Apfelwurm

@Apfelwurm Apfelwurm commented Sep 27, 2026 •

Copy link
Copy Markdown

What does this PR do?

This PR is intended to migrate fredy to a rootless docker image, since it is pretty sad that this is still the default.
Since the last attempts seem to have died because of migration problems, i added a small docker entrypoint for now, which takes care of chowning everything nessecary to the node user, before then starting tini via setpriv on the node user.
I have tested running the master locally, created a job, stopped it and ran my image and the instance and the job seems to work just fine.
When this is rolled out for a while, the entrypoint script could be removed again and be replaced with a single USER node line before the old entrypoint.

Related issue

There seemed to be a few older ones: #217 #214 and #207 , but it seems they have been rolled back.

AI disclosure (required)

Fredy accepts AI assisted contributions, but they have to be declared. An
undeclared AI PR will be closed. Tick exactly one box:

  • ai:none - I wrote this myself. No AI generated code, text or commit messages.
  • ai:assisted - AI helped with parts of it (autocomplete, refactoring, tests, docs). I reviewed and understand every line.
  • ai:generated - AI produced most or all of this PR. I reviewed it, but it is largely machine written.

If you ticked ai:assisted or ai:generated, the three answers below are
mandatory. Keep them on the same line as the label.

Which AI: GPT-5
How much: Generating the setpriv command and generating the test additions
Why: to make sure passing of the CMD line works flawlessly with the right escaping and build the tests quicker

Checklist

  • yarn test:offline passes (or yarn test if the change touches a live provider)
  • yarn lint and yarn format:check pass
  • The change is useful for everybody, not a custom tweak for my own setup
  • I have read and answered the AI disclosure above honestly

@Apfelwurm
Apfelwurm marked this pull request as draft September 27, 2026 23:12
@Apfelwurm
Apfelwurm marked this pull request as ready for review September 27, 2026 23:40
@orangecoding

Copy link
Copy Markdown
Owner

@Apfelwurm There are test failures. Can you please fix them?

@Apfelwurm
Apfelwurm force-pushed the master branch 2 times, most recently from 858c2c4 to 7c20609 Compare September 28, 2026 21:12
@Apfelwurm

Copy link
Copy Markdown
Author

Sorry for that, did not expect tests for the Dockerfile in the application test suite :D Changed/Added the ct ones with AI, so they check everything we have in there in regards to the startup as of right now.
Also rebased it, so it should be ready to merge now.

@orangecoding

Copy link
Copy Markdown
Owner

@Apfelwurm

Thanks for picking this up again! This is the first attempt that actually handles the migration, wich is what killed the earlier tries (see #214, #217 and #281).

I built the image and played around with it a bit. Fresh volumes work, and so does upgrading from root-owned volumes. tini is PID 1 running as node, and the browser launches fine. Nice.

A few things I ran into though:

  1. set -e + the unconditional chown -R: with a read-only /conf mount or cap_drop: ALL the container now dies on start. Both of those work on master right now.
  2. Starting it with --user 1000:1000 (or runAsUser in k8s) fails with setpriv: initgroups failed: Operation not permitted. That's pretty much the setup people who ask for non-root will use.
  3. setpriv doesn't touch the env, so node runs with HOME=/root. Chromium complains about /root/.local/share/pki/nssdb, and anything that uses os.homedir() ends up in a dir it can't write to. The XDG vars work around it, but just exporting HOME is simpler.

Something like this should cover all three:

#!/bin/sh
set -e

if [ "$(id -u)" = "0" ]; then
  find /db /conf \! -user node -exec chown node:node {} + || echo "WARN: could not fix ownership of /db or /conf" >&2
  export HOME=/home/node
  exec setpriv --reuid=node --regid=node --init-groups /usr/bin/tini -g -- "$@"
fi

exec /usr/bin/tini -g -- "$@"

With that, hardened setups can just run with --user 1000:1000, and the XDG vars can go. The tests need a small update for it.

I'd also keep the entrypoint for good instead of switching to USER node later. Anyone who skips the release with the chown would run straight into #281 again.

Two small things:

  • please add *.sh text eol=lf to .gitattributes, otherwise Windows checkouts get CRLF and the script fails with "no such file or directory"
  • missing newline at the end of the script

Once thats in, I'll take it through develop first so it ends up in the pre-release image and goes through the compose healthcheck (the PR CI doesn't build the docker image).

@Apfelwurm

Copy link
Copy Markdown
Author

Thanks @orangecoding for your valid thoughts and additions :)
I have implemented your changes (with the slight difference to also check for the group in the find command, so the group is also always setted). I also added a small check on startup, that checks if any files have wrong ownership when running at another user than root, and the container then fails with a message what is happening and how to fix this, which should make sure edecases are handled with a good user response. Also tested this behaviour manually to make sure it works
I have updated my instance to this version, and everything seems to work just fine.

Let me know if there is anything else to do :)

@orangecoding

Copy link
Copy Markdown
Owner

@Apfelwurm
Thanks, looks really good now! I rebuilt it and went through the same scenarios again. Fresh volumes, upgrade from root-owned volumes, read-only /conf, --user 1000:1000 (also together with cap_drop: ALL) all work, HOME is right and the nssdb warning is gone.

One thing left: the --user check looks at ownership, not at whether the dirs are writable. With group-writable volumes (thats the usual k8s fsGroup setup, /db and /conf owned by root:1000 with 2775) it now refuses to start, while master runs fine there. Checking writability fixes that and still catches the root-owned case:

foreign="$(find /db /conf \! -writable 2>/dev/null | head -n 5)"

The error text and the test need a tiny tweak for it then.

Small nit while your at it: the chown -R $uid:$gid hint reads like any uid works, but anything other than 1000 still fails because the browser binary lives in /home/node. Maybe just say 1000:1000 there.

After that I'll take it into develop.

@Apfelwurm
Apfelwurm marked this pull request as draft October 2, 2026 23:54
@Apfelwurm
Apfelwurm marked this pull request as ready for review October 2, 2026 23:57
@Apfelwurm

Copy link
Copy Markdown
Author

done :)

@orangecoding

Copy link
Copy Markdown
Owner

@Apfelwurm great. Now please change the destination of the merge from master to develop, then I can merge it ;)

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