feat: rootless container & small improvements#1373
feat: rootless container & small improvements#1373MrRubberDucky wants to merge 2 commits intoFuzzyGrim:devfrom
Conversation
Hopefully this is descriptive enough to explain what was changed. All container logs from processes are accessible within container in `/tmp` now so no easy direct logs in stdout. Dockerfile: - Create an user abc:abc with UID and GID of 1000 - Change ownership of directories to abc:abc and set more restrictive file & directory permissions - Remove shadow package after installation - Drop sed -i command for nginx IPv6, instead it now runs as dual-stack (it works fine in my setup, though I can revert this change if desired) - Run python manage.py collectstatic --noinput as abc:abc user instead of root - Container runs entrypoint as abc:abc user (1000:1000) supervisord.conf: - Remove user elevation so supervisord doesn't panic on launch - Remove /dev/stderr, /dev/stdout from log directives as it needs root privileges to attach to them - Save temporary supervisord logs to /tmp - Remove IPv6 environment variable from nginx command nginx.conf: - Make nginx write it's logs and files to /tmp to avoid permission denied issues (supervisord still redirects output from console to /tmp) - Listen on both `:8000` and `[::]:8000` src/config/settings.py - Added `USE_X_FORWARDED_HOST = True` as otherwise django was not trusting Caddy's headers after reverse proxying, breaking SSO auth flow by setting callback URL to local IP address.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the container to run as a non-root user, moves permission management from the entrypoint script to the Dockerfile, and updates Nginx and Supervisor configurations for rootless operation. Several critical issues were identified, including a syntax error in the Supervisor Nginx command and the redirection or removal of log outputs, which would make logs inaccessible via standard container logging tools. Additionally, the unconditional IPv6 configuration in Nginx may cause startup failures in some environments, and the security implications of enabling USE_X_FORWARDED_HOST should be carefully reviewed.
| command=nginx -c $CONF -g "daemon off;"' | ||
| priority=1 |
There was a problem hiding this comment.
The nginx command contains an undefined variable $CONF and a trailing single quote, which will cause the process to fail to start. Additionally, removing stdout_logfile and stderr_logfile settings prevents logs from appearing in docker logs. Even in rootless containers, processes can typically write to their own stdout/stderr. If permission issues occur with device files, using /proc/self/fd/1 and /proc/self/fd/2 is a better alternative than writing to temporary files in /tmp.
command=nginx -c /etc/nginx/nginx.conf -g "daemon off;"
priority=1
stdout_logfile=/dev/stdout
stdout_logfile_maxbytes=0
stderr_logfile=/dev/stderr
stderr_logfile_maxbytes=0
| user abc; | ||
| worker_processes auto; | ||
| worker_processes auto; | ||
| error_log /tmp/error.log; |
There was a problem hiding this comment.
Redirecting error_log to a file in /tmp makes it invisible to the container engine's logging system (e.g., docker logs). It is recommended to keep this pointing to /dev/stderr. If permission issues occur in rootless mode, consider using /proc/self/fd/2.
error_log /dev/stderr warn;
| uwsgi_temp_path /tmp/uwsgi_temp; | ||
| scgi_temp_path /tmp/scgi_temp; | ||
|
|
||
| access_log /tmp/access.log; |
There was a problem hiding this comment.
Redirecting access_log to a file in /tmp makes it invisible to the container engine's logging system. It is recommended to keep this pointing to /dev/stdout. If permission issues occur in rootless mode, consider using /proc/self/fd/1.
access_log /dev/stdout main;
| command=gunicorn --control-socket /tmp/gunicorn.ctl --config python:config.gunicorn config.wsgi:application | ||
| user=abc | ||
| priority=5 |
There was a problem hiding this comment.
Removing stdout_logfile and stderr_logfile prevents logs from appearing in docker logs. It is recommended to restore these settings pointing to /dev/stdout and /dev/stderr (with maxbytes=0) for all programs in this configuration.
command=gunicorn --control-socket /tmp/gunicorn.ctl --config python:config.gunicorn config.wsgi:application
priority=5
stdout_logfile=/dev/stdout
stdout_logfile_maxbytes=0
stderr_logfile=/dev/stdout
stderr_logfile_maxbytes=0
| [supervisord] | ||
| nodaemon=true | ||
| user=root | ||
| logfile=/tmp/supervisord.conf |
There was a problem hiding this comment.
The log file for supervisord is named supervisord.conf, which is confusing as it shares the same name as the configuration file. It should ideally be named supervisord.log.
logfile=/tmp/supervisord.log
| server { | ||
| listen 8000; | ||
| listen [::]:8000; |
There was a problem hiding this comment.
Adding listen [::]:8000; unconditionally might cause Nginx to fail to start on systems where IPv6 is disabled. The previous implementation used a conditional check to handle this. Consider making this optional or ensuring the environment supports IPv6.
| if BASE_URL: | ||
| CSRF_COOKIE_PATH = BASE_URL + "/" | ||
|
|
||
| USE_X_FORWARDED_HOST = True |
There was a problem hiding this comment.
Enabling USE_X_FORWARDED_HOST can be a security risk as it allows clients to spoof the Host header, which Django uses to construct absolute URLs. Ensure that your upstream proxy (like Nginx or Caddy) is configured to strictly set or strip this header before passing it to the application.
|
Ah the clanker had a field trip here, we truly live in the future. Not gonna resolve these (as I don't really know how and I don't wanna accidentally nuke this PR with LLM changing stuff 'cus it misunderstood what I've said) as they're non-issues and are done the way they are due to supervisord logic doing funky things while trying to access IPv6 is generally fine anyways as rootless networks do both IPv4 and IPv6 even if it's disabled on host and any IPv4 connection is properly resolved because nginx & rootful / rootless networking is just clever like that. It could be reverted though at the request of a maintainer, or changed back to how it was, I don't mind. This would also replace #1264 as I set entrypoint.sh permissions here to Edit: I've been thinking if it's worthwhile dumping logs to /tmp/logs instead, or finding out how nginx rootless image does it if logs being displayed on |
...in supervisord.conf. Undefined variable voes. nginx probably ignored it and tried it's own directory even if it was empty, otherwise I don't understand why it wouldn't fail here.
1 participant
This PR makes container run as a rootless user by default. Fixes #1120 and replaces PR #1128
Following files were modified:
Changes
Container now runs as user & group
abc:abcwith UID & GID of1000, similar to aforementioned PR. It also takes care of permission inside the container, ensuring that abc user can write to directories it needs. All logs get written to/tmpas rootless user can't access/dev/stdout,/dev/stderr- supervisor writes stdout and stderr of apps to/tmp/appname-stdout---supervisord-randomstring.logso I suppose it's not that big of an issue.There's one problem and it's with nginx trying to write an
/var/lib/nginx/logs/error.logall the time but it just seems to be a hard-coded value and any errors are still logged to supervisord stdout log files if they occur. Since this isn't considered fatal, I see it as a pointless thing to try to fix.Added
USE_X_FORWARDED_HOSTto/src/config/settings.pyas otherwise I couldn't get django to trust Caddy'sX Forwarded Forheader. This will be more useful for people that will reverse proxy their Yamtrack container to the web and want SSO to work properly. Without it, SSO redirect gets built wrongly and instead of the domain from the headers, it used LAN IP address for redirect, causing invalid callback URL error.Modified
nginx.confto listen on both IPv4 and IPv6 by default. Currently the Dockerfile creates another IPv6 configuration file then uses an undocumented environment variable to switch between them, which seems pointless considering that nginx supports dual-stack connections just fine. Additionally, all relevant temp and log files are written to/tmpto allow it to run rootless without barrage of permission errors or having to constantly mount everything to be owned under current user.entrypoint.sh was reduced to just two simple commands: the simple migration python command and supervisord exec.
PGIDandPUIDare unused variables now.Potential problems arising from this PR
SQLite installs with bind volumes will need to
chowntheir volumes to be owned by rootless user inside container, otherwise they will get eitherout of memory(happens when SQLite can't write to the directory, or to the database), or errors such asaccess denied. User ownership doesn't matter for any other directory as the permissions are allowing enough with them beingr-x.Did you test it?
Yup. It works fine, nothing else to say really.
What's the benefit?
You can now run the container as any rootless user and as read only, as long as you fix relevant directory ownership yourself. This gets rid of the convenience aspect but in turn it increases security as container init and processes running under it are ran via non-root user with strict permissions that prevent modification to
entrypoint.shand anything copied from/src/*If you will be using external database, you can run it as read only easily without suffering through permission errors. Here's my current Quadlet as an example that runs as my user with randomized user namespace and read only.
...and no, this wasn't written by LLM before anyone asks.