-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nginx: do not run anything as root #56255
Conversation
8b03dda
to
bfe61e1
Compare
сс @infinisil cc @Mic92 |
cc4163e
to
2e4ff94
Compare
6a2910a
to
03f6607
Compare
532a80b
to
7d910ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't add the RLIMIT
options.
Returned |
@flokli yes, correct work:
|
Acme tests still pass after merging this in, so should be good to go. Thanks! |
@tbenst mentioned this broke nginx, as it's not able to open the log file:
When checking on a NixOS 19.09 machine however, I couldn't reproduce log files being owned by root. @NinjaTrappeur , @asymmetric, any thoughts? |
Before update -
After this PR -
Need remove log files or change owner to |
This should do the trick:
|
If so, then it should probably be a `z ${cfg.stateDir}/logs/*` 6?? ...` - we don't want to set the executable bit on log files.
However, I also saw some `${cfg.stateDir}/*_temp` directories and am not certain if we want to chown/chmod them too, and with which permissions.
|
This commit breaks It's because the permissions on the edit: I believe this is because I didn't originally have tldr; I think this is a configuration issue on my end and it shouldn't affect other users who've set it up properly. I'll confirm later when I've had a chance to check this. edit2: yes, it's as I explained. This was just a configuration error on my end that manifested once the change to the |
I have the suspicion that this file broke running nginx as root: When you manually configure
Here, the nginx master process The directory permissions are:
Because In other words, it seems like the lines 2a413da#diff-795fa65a7c41a479084de826e4e2e65cR672
do not have the desired effect: Yes, they change the parent directory, but if nginx then drops privileges as it did before, child dirs cannot be written to with the dropped perms. |
@nh2 which also implied, nginx won't be able to write to logs directory, as it is not Looks like |
I'm having an issue very similar to this, but with My workaround for now has been to just SSH to my server and
(which are the directories that nginx appears to create) in addition to the already existing:
? I'm very new to this whole NixOS thing, so maybe there's a better way of achieving what I'm trying to accomplish. |
@nh2 @sumnerevans since 50d6e93, there should be a |
@flokli I suspect that should help with @sumnerevans issue, but it seems that it wouldn't help with mine, since as described above, |
I misspoke in my last comment. The user is |
Sorry, I'm now confused. Specifying |
That is what was there before:
Yes. Done in #84391. |
Motivation for this change
Reopened and updated this PR #51551
Change location pidFile from /var/spool/nginx/log/nginx.pid to /run/nginx/nginx.pid
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)