Conversation
|
Created a staging project on OBS for Tumbleweed: home:pushman:BCI:Staging:Tumbleweed:Tumbleweed-3167 Build ResultsRepository
Repository
Repository
Repository
Build succeeded ✅ To run BCI-tests against this PR, use the following command: OS_VERSION=tumbleweed TARGET=custom BASEURL=registry.opensuse.org/home/pushman/bci/staging/tumbleweed/tumbleweed-3167/ tox -- -n autoThe following images can be pulled from the staging project:
|
|
Created a staging project on OBS for 7: home:pushman:BCI:Staging:SLE-15-SP7:7-3167 Build ResultsRepository
Repository
Repository
Repository
Repository
Repository
Repository
Repository
Build succeeded ✅ To run BCI-tests against this PR, use the following command: OS_VERSION=15.7 TARGET=custom BASEURL=registry.opensuse.org/home/pushman/bci/staging/sle-15-sp7/7-3167/ tox -- -n autoThe following images can be pulled from the staging project:
|
|
Created a staging project on OBS for 16.0: home:pushman:BCI:Staging:16.0:16.0-3167 Build ResultsRepository
Repository
Repository
Repository
Repository
Repository
Repository
Repository
Build succeeded ✅ To run BCI-tests against this PR, use the following command: OS_VERSION=16.0 TARGET=custom BASEURL=registry.opensuse.org/home/pushman/bci/staging/16.0/16.0-3167/ tox -- -n autoThe following images can be pulled from the staging project:
|
871cffd to
e43e062
Compare
e43e062 to
ca2996d
Compare
| entrypoint_log "$0: Removed 'user' directive for unprivileged worker." | ||
|
|
||
| # Ensure PID path is set to /tmp/nginx.pid | ||
| sed -i 's,^#\?\s*pid\s\+.*;$,pid /tmp/nginx.pid;,' /etc/nginx/nginx.conf |
There was a problem hiding this comment.
why are we setting it to /tmp rather than a safe location like /run/nginx or /run ?
There was a problem hiding this comment.
We are starting the nginx container as unprevileged user.
/run directory and /var/run directory is owned by root.
There was a problem hiding this comment.
But nothing prevents you from having /run/nginx/nginx.pid , you can create the folder with the nginx user, which it seems you already do with chown -R nginx:nginx /var/run/.
I would also move this to be a default location, not just in case it is a non-root user. Since it would not make any difference if it is owned by nginx anyway.
There was a problem hiding this comment.
pid path is set to /var/run/nginx/nginx.pid
|
@rcmadhankumar can you please also include the (adjusted) readme update from #3071 ? |
ca2996d to
77e787a
Compare
Added relevant readme update. |
77e787a to
74167bd
Compare
alexandrevicenzi
left a comment
There was a problem hiding this comment.
Are we seding the user provided file as well (volume mount)? It does not make sense to change is such cases, only if it is the default config we ship, otherwise, we could potentially break the user config.
| entrypoint_log "$0: Removed 'user' directive for unprivileged worker." | ||
|
|
||
| # Ensure PID path is set to /tmp/nginx.pid | ||
| sed -i 's,^#\?\s*pid\s\+.*;$,pid /tmp/nginx.pid;,' /etc/nginx/nginx.conf |
There was a problem hiding this comment.
But nothing prevents you from having /run/nginx/nginx.pid , you can create the folder with the nginx user, which it seems you already do with chown -R nginx:nginx /var/run/.
I would also move this to be a default location, not just in case it is a non-root user. Since it would not make any difference if it is owned by nginx anyway.
| chown -R nginx:nginx /etc/nginx; \ | ||
| chown -R nginx:nginx /var/run/nginx; \ | ||
| install -d -o nginx -g nginx /tmp/client_temp /tmp/proxy_temp /tmp/fastcgi_temp /tmp/uwsgi_temp /tmp/scgi_temp; \ | ||
| chown -R nginx:nginx /tmp; \ |
There was a problem hiding this comment.
well, /tmp must be owned by root, it has the sticky bit set though. why are you changing this here?
There was a problem hiding this comment.
Sure, Now changing only the permissions of the subdirectories inside /tmp created in the run time to run the image in root less mode.
| entrypoint_log "$0: Removed 'user' directive for unprivileged worker." | ||
|
|
||
| sed -i 's/listen \(.*\)80;/listen \18080;/' /etc/nginx/conf.d/default.conf 2>/dev/null || \ | ||
| sed -i 's/listen \(.*\)80;/listen \18080;/' /etc/nginx/nginx.conf 2>/dev/null || true |
There was a problem hiding this comment.
this could be mounted over though by a user to provide a different configuration file? we should at least check before sed'ing whether there is a change necessary?
There was a problem hiding this comment.
True that. Sedding config file now only if it uses port 80 in rootless mode. This prevents if user wants to use different port than 8080 in rootless mode.
| ## Running nginx as a non-root user | ||
| To run the image as a less privileged user using the `nginx` user, do the following: | ||
| ```ShellSession | ||
| $ podman run -it --user nginx --rm -p 8080:8080 -v /path/to/html/:/srv/www/htdocs/:Z -v $PWD/nginx.conf:/etc/nginx/nginx.conf:Z {{ image.pretty_reference }} |
There was a problem hiding this comment.
why exactly do we still need nginx.conf to be mounted over from the host here? where is it coming from?
There was a problem hiding this comment.
We don't necessarily mount the config file. Updated the readme.
| ln -sf /dev/stderr /var/log/nginx/error.log; \ | ||
| chown -R nginx:nginx /var/cache/nginx; \ | ||
| chown -R nginx:nginx /etc/nginx; \ | ||
| chown -R nginx:nginx /var/run/nginx; \ |
There was a problem hiding this comment.
so we're expecting the uid to be the one of nginx? otherwise these changes wouldn't seem to make sense?
There was a problem hiding this comment.
Yes it was. Now modified the code so the rootless user ID can be anything.
| # Ensure PID path is set to /var/run/nginx.pid for both privileged and unprivileged users | ||
| sed -i 's,^#\?\s*pid\s\+.*;$,pid /var/run/nginx/nginx.pid;,' /etc/nginx/nginx.conf | ||
| # modify temp paths for both privileged and unprivileged users | ||
| sed -i "/^http {/a \ proxy_temp_path /tmp/proxy_temp;\n client_body_temp_path /tmp/client_temp;\n fastcgi_temp_path /tmp/fastcgi_temp;\n uwsgi_temp_path /tmp/uwsgi_temp;\n scgi_temp_path /tmp/scgi_temp;\n" /etc/nginx/nginx.conf |
There was a problem hiding this comment.
that's a backward incompatible change, isn't it? we cannot do it that way unconditionally.
There was a problem hiding this comment.
Yes, Updated the code.
dirkmueller
left a comment
There was a problem hiding this comment.
need to preserve compatibility with previous version of the nginx container
c6f69b9 to
50d77cf
Compare
|
modified the code by keeping following expectations in mind:
|
50d77cf to
0154f56
Compare
| fi | ||
| fi | ||
|
|
||
| CURRENT_UID=$(id -u) |
There was a problem hiding this comment.
This code is going to vanish as soon as update-files.sh is executed. docker-entrypoint.sh is taken from upstream. I completely missed that before.
This piece of code should be in /docker-entrypoint.d/ and could be called unprivileged-mode.sh.
0154f56 to
99dea49
Compare
| {DOCKERFILE_RUN} set -euo pipefail; mkdir -p /var/cache/nginx /var/run/nginx /tmp/client_temp /tmp/proxy_temp /tmp/fastcgi_temp /tmp/uwsgi_temp /tmp/scgi_temp;\ | ||
| ln -sf /dev/stdout /var/log/nginx/access.log;\ | ||
| ln -sf /dev/stderr /var/log/nginx/error.log;\ | ||
| chmod -R 777 /var/cache/nginx /etc/nginx /var/run/nginx /var/log/nginx /tmp/client_temp /tmp/proxy_temp /tmp/fastcgi_temp /tmp/uwsgi_temp /tmp/scgi_temp; |
There was a problem hiding this comment.
a chmod -R 0777 is kinda ugly, can we chown it to the nginx user instead? we should then set that as a stableuid and update the README accordingly.
Made necessary changes to the nginx image so that it can be simply run as non root by passing
--user=nginxflag.fixes: #2924