Nginx create logs from master process, but recreate them receiving SIGUSR1 (reopen logs signal) from worker process running under nginx:nginx. So log directory should be chowned to root:nginx by default. Write permission for nginx group is not necessary only r-x. If not, then log files are not created or information isn't written to them. For example logrotate default config for nginx stops nginx logging. To repeat the issue: 1) Nginx should be running. 2) (Re)move any logfile. 3) kill -USR1 `cat /run/nginx.pid` What we get: log file isn't created or zero sized and error message for each logfile like: open() "/var/log/nginx/error_log" failed (13: Permission denied) in error_log. What we should get: normal reopening of log files.
*argh* this is really annoying since I'd like to avoid having to force ownership on directories. Usually one wants to change the group of the directory to some privileged user's group to give access to log files.
I'd consider this a bug in nginx since this gives the unprivileged nginx-user write access to the log files since they are chown'ed to nginx:root upon SIGUSR1. But I still don't understand how/when the owner change from root:root to nginx:root happens...
This is definetely an inconsistency of nginx: on startup the worker process (running as user nginx) gets the file-descriptors from the master process (running as user root) while forking. On SIGUSR1 the master reopens the log files and changes ownership such that the worker can reopen the logs himself. From a security perspective I would say that the master should rather pass the new file descriptors to the worker instead which would solve this issue.
(In reply to Tiziano Müller from comment #3) > This is definetely an inconsistency of nginx: Can you report it to upstream ( http://trac.nginx.org/nginx/report or http://forum.nginx.org/list.php?2 )? I don't think that I'm skilled enough to make it convincingly.
ok, upstream explained it and accepted the bug report, this is now on their todo-list. I'd suggest to change the provided logrotate-config to always do a full-config reload instead of a fast log-reopen which solves the problem for the Gentoo installation. Furthermore I will be adding an elog entry stating the above such that people who care less for security but more for performance (a full config reload seems to restart workers and may fail if the user changed the settings but didn't check them). Any objections?
I couldn't find the elog entry in the latest nginx ebuild. Can you confirm that /etc/logrotate.d/nginx should use -HUP instead of -USR1 ? I'm also curious why that isn't the default since nginx keeps logging to the old log file with -USR1. Why is upstream so unconcerned? Shouldn't the current state be considered broken?
Hi, mhh... is there really a problem? I noticed the problem from reading the nginx ml (http://mailman.nginx.org/pipermail/nginx/2013-July/039778.html) which I can confirm. My logs in /var/log/nginx/error_log are also full of 2013/07/18 15:50:27 [emerg] 4469#0: open() "/var/log/nginx/other_vhosts_access.log" failed (13: Permission denied) 2013/07/18 15:50:27 [emerg] 4467#0: open() "/var/log/nginx/error_log" failed (13: Permission denied) These errors can be triggered by running # kill -USR1 `cat /run/nginx.pid` Then I noticed, that the permission was wrong: # ls -l /var/log [...] drwxr-x--- 2 root root 4096 May 26 21:03 nginx I did a test on a vanilla gentoo system and after emerging nginx-1.4.1-r5 I saw the following permission: # ls -l /var/log [...] drwx------ 2 nginx nginx 4096 Jul 18 15:12 nginx I changed permission on the first system, where I saw the permission denied error messages and re-run # kill -USR1 `cat /run/nginx.pid` Now, I don't see the error anymore. And I am also unable to reproduce https://bugs.gentoo.org/show_bug.cgi?id=473036#c0 with the fixed permission. So actual the problem for me looks like, that the update nginx ebuild did not fix permission on existing installations, which is wrong. But when you correct the permissions of the existing /var/log/nginx folder, there shouldn't be a problem. Or did I miss the point?
I think it's me that missed the point. Can anyone confirm that the appropriate fix is changing the ownership and permissions of /var/log/nginx?
Can anyone confirm that drwx------ nginx:nginx are currently the default permissions/ownership for /var/log/nginx? I would need apache:apache though since that's how my reverse-proxy nginx runs.
I can confirm, that chmod 700 and nginx:nginx are the current default permissions for /var/log/nginx, which are working fine with nginx. P.S: It shouldn't be required to run nginx as apache user on the same box. At least when nginx acts as reverse proxy. But this is OT and has nothing to do with that bug report. I'd suggest to create a thread on the forum or ML.
Thanks a lot Thomas. I'm running as nginx:nginx now. Not sure why I thought apache:apache was necessary. Fixed up the /var/log/nginx permissions/ownership too.
Is this still an issue? The latest version of the nginx ebuild (1.4.1-r5) sets 0700 perms on /var/log/nginx, which on USR1 then are logged (error_log) to as root:nginx.
If you do a fresh installation I did not experience any issues, because the correct permissions are set (like you described). But if you are running a previous version which set wrong permissions, pkg_postinst will only fix the world-readable issue (by setting /var/log/nginx to o-rwx) but it will not fix the ownership. So because /var/log/nginx is still owned by root:root, but world cannot write into /var/log/nginx anymore, nginx cannot write, too. You have to correct ownership on your own if you are upgrading from a previous version with wrong permissions. @ Maintainer: Is there a reason why you only handle file permission and not ownership in pkg_postinst?
Could we at least have an ewarn about the issue?
(In reply to Thomas D. from comment #13) > If you do a fresh installation I did not experience any issues, because the > correct permissions are set (like you described). > > But if you are running a previous version which set wrong permissions, > pkg_postinst will only fix the world-readable issue (by setting > /var/log/nginx to o-rwx) but it will not fix the ownership. So because > /var/log/nginx is still owned by root:root, but world cannot write into > /var/log/nginx anymore, nginx cannot write, too. You have to correct > ownership on your own if you are upgrading from a previous version with > wrong permissions. > > @ Maintainer: > Is there a reason why you only handle file permission and not ownership in > pkg_postinst? Yes: At least in our installations we set permissions and ownership very specific. Having an ebuild overwriting all of that would interrupt services. Therefore I am always trying to do only the least invasive solution.
How about an ewarn that lists the files with different ownership/permission than default?
Created attachment 359510 [details, diff] Check permission/ownership in pkg_postinst (In reply to Tiziano Müller from comment #15) > (In reply to Thomas D. from comment #13) > > @ Maintainer: > > Is there a reason why you only handle file permission and not ownership in > > pkg_postinst? > > Yes: At least in our installations we set permissions and ownership very > specific. Having an ebuild overwriting all of that would interrupt services. > Therefore I am always trying to do only the least invasive solution. I like your 'least invasive solution' approach but in this situation I think it is wrong or at least inconsistent: You decided to add a permission fix. That's OK and I agree with your decision. But this fix will *break* old installations, because when you change permission to 700, the nginx user/group is now unable to write to that directory (your are breaking the previous setup). I would call this an incomplete fix: If you change permission that way (and it is required to do that), it is also required to adjust ownership. So your decision to do only the least invasive solution to prevent service interruption will actually cause service interruption. Following your argumentation you should have never changed permissions from existing installations, instead you should only have displayed ewarns telling the user what to do/check. Now we are in a situation where we probably have multiple nginx installations which don't log as expected. Sadly enough, many people will only notice if they will need a log: "I have a nginx problem" -"Check your logs" "My logs are fine, no errors.." <== RIGHT! Because nginx is unable to log due to this permission fix. User will only notice, if he/she will read nginx main log. ;) => I agree with A. Person: If we don't want to fix ownership, like we fixed permission (and as I said I don't understand why, for me the current fix is just incomplete) we should at least add a check to pkg_postinst to check permission and ownership and display an ewarn if they differ from nginx default permission. I created a patch which will do that. Maybe the wording can be improved but the rest should be clear. For the future we may want to add an additional environment variable to suppress this check for people running a custom setup.
(In reply to Thomas D. from comment #17) > Created attachment 359510 [details, diff] [details, diff] > Check permission/ownership in pkg_postinst > > (In reply to Tiziano Müller from comment #15) > > (In reply to Thomas D. from comment #13) > > > @ Maintainer: > > > Is there a reason why you only handle file permission and not ownership in > > > pkg_postinst? > > > > Yes: At least in our installations we set permissions and ownership very > > specific. Having an ebuild overwriting all of that would interrupt services. > > Therefore I am always trying to do only the least invasive solution. > > I like your 'least invasive solution' approach but in this situation I think > it is wrong or at least inconsistent: > > You decided to add a permission fix. That's OK and I agree with your > decision. > > But this fix will *break* old installations, because when you change > permission to 700, the nginx user/group is now unable to write to that > directory (your are breaking the previous setup). I would call this an > incomplete fix: If you change permission that way (and it is required to do > that), it is also required to adjust ownership. > > So your decision to do only the least invasive solution to prevent service > interruption will actually cause service interruption. Well, I didn't expect that nginx behaves different than any other software I now with regard to log-files: when a software can create log files when starting it, I expect it to be able to recreate them without problems after a logrotate. > > Following your argumentation you should have never changed permissions from > existing installations, instead you should only have displayed ewarns > telling the user what to do/check. > > Now we are in a situation where we probably have multiple nginx > installations which don't log as expected. Sadly enough, many people will > only notice if they will need a log: > > "I have a nginx problem" > -"Check your logs" > "My logs are fine, no errors.." <== RIGHT! Because nginx is unable to log > due to this permission fix. User will only notice, if he/she will read nginx > main log. ;) > > > => I agree with A. Person: If we don't want to fix ownership, like we fixed > permission (and as I said I don't understand why, for me the current fix is > just incomplete) we should at least add a check to pkg_postinst to check > permission and ownership and display an ewarn if they differ from nginx > default permission. > > > I created a patch which will do that. Maybe the wording can be improved but > the rest should be clear. Is there a problem with the other (socket) dirs too? Otherwise your patch does too much. Besides, the easiest check to see whether the user should probably change the owner ship is something like this in pkg_postinst: su -c 'touch /var/log/nginx/error.log' nginx if [ $? -ne 0 ] ; then ewarn "Please change the owner/group on /var/log/nginx such that the nginx user has 'rx' permission on that directory." ewarn "Otherwise you may end up with empty logs after a logrotate" fi
(In reply to Tiziano Müller from comment #18) > Is there a problem with the other (socket) dirs too? Otherwise your patch > does too much. Yes, the patch is checking a lot. I cannot tell you for sure if there is a socket problem for old installation like there is a problem with /var/log/nginx or not, but I don't expect one: You moved the TMP dir with 1.4.1-r2. After that, no changes happen so far. > Besides, the easiest check to see whether the user should probably change > the owner ship is something like this in pkg_postinst: > > su -c 'touch /var/log/nginx/error.log' nginx > if [ $? -ne 0 ] ; then > ewarn "Please change the owner/group on /var/log/nginx such that the > nginx user has 'rx' permission on that directory." > ewarn "Otherwise you may end up with empty logs after a logrotate" > fi Can we depend on sys-apps/shadow on all supported platforms? That's why I choose 'stat'. But yes, the check for "${NGINX_HOME_TMP}"/{client,proxy,fastcgi,scgi,uwsgi} should be removed, because they wouldn't match nginx default permissions (=if you remove these folders, nginx would re-create them on start with other permissions/ownership; therefore I am not sure why the ebuild is creating these folders).
In nginx-1.4.2-r1 we warn the user about the permissions if the nginx user can not read or change into the log-directory (or if the log dir is not there, or if shadow is not present, etc.) I therefore consider this as fixed.
