-
-
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 #51551
Conversation
32eecb4
to
0acedfe
Compare
|
||
# The nginx configuration test might have created log files as root, | ||
# make sure the nginx user/group can write to them. | ||
chown -R ${cfg.user}:${cfg.group} ${cfg.stateDir} |
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.
"chown" must be ran before "${cfg.package}/bin/nginx ... -t", otherwise an error will be observed. More details in #48875 (review)
or move ${cfg.preStart}
My example PreStart script to check permissions
test -d "${cfg.stateDir}" || mkdir -m 750 "${cfg.stateDir}"
test -d "${cfg.stateDir}/logs" || mkdir -m 750 -p "${cfg.stateDir}/logs"
test `stat -c %a:%U:%G "${cfg.stateDir}"` = "750:root:${cfg.group}" || (chmod 750 "${cfg.stateDir}" && chown root:${cfg.group} "${cfg.stateDir}")
test `stat -c %a:%U:%G "${cfg.stateDir}/logs"` = "750:root:${cfg.group}" || (chmod 750 "${cfg.stateDir}/logs" && chown root:${cfg.group} "${cfg.stateDir}/logs")
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.
There is already a chown in the default cfg.preStart -- and yes, it's a bit ugly that it needs to be done before and after, but I couldn't convince the nginx binary to not create the log files as root:root when in test mode. I toyed with running the nginx config test unprivileged, but unfortunately this then failed because the config test fails to bind 80/443.
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 use this method:
services.logrotate = {
config = ''
${cfgNginxSpool}/logs/access.log
${cfgNginxSpool}/logs/error.log
{
create 0640 ${cfg.user} ${cfg.group}
postrotate
[ ! -f /run/nginx.pid ] || kill -USR1 `cat /run/nginx.pid`
endscript
}
'';
};
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.
Please check thats variant:
preStart = mkOption {
type = types.lines;
default = ''
test -d "${cfg.stateDir}" || mkdir -m 750 "${cfg.stateDir}"
test -d "${cfg.stateDir}/logs" || mkdir -m 750 -p "${cfg.stateDir}/logs"
test `stat -c %a:%U:%G "${cfg.stateDir}"` = "750:${cfg.user}:${cfg.group}" || (chmod 750 "${cfg.stateDir}" && chown ${cfg.user}:${cfg.group} "${cfg.stateDir}")
test `stat -c %a:%U:%G "${cfg.stateDir}/logs"` = "750:${cfg.user}:${cfg.group}" || (chmod 750 "${cfg.stateDir}/logs" && chown ${cfg.user}:${cfg.group} "${cfg.stateDir}/logs")
${pkgs.su-exec}/bin/su-exec ${cfg.user}:${cfg.group} /run/wrappers/bin/nginx -c ${configFile} -p ${cfg.stateDir} -t
'';
...
systemd.services.nginx = {
preStart =
''
${cfg.preStart}
'';
...
security.wrappers.nginx = {
source = "${cfg.package}/bin/nginx";
capabilities = "cap_net_bind_service+eip";
owner = cfg.user;
group = cfg.group;
permissions = "u+rx,g+x";
};
Instead of fixing permissions of files created by "nginx -t", we can run "nginx -t" with dropped privileges, using su-exec. In this case nginx will create log files with correct permissions. So all we need to do is create /var/spool/nginx writeable for nginx (maybe even for nginx group).
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 like this, hadn't thought of using a wrapper to add cap_net_bind_service! Done.
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.
There is no setuid binary:
-r-x--x--- 1 nginx nginx 17704 Dec 6 01:18 nginx
That binary is however able to listen on <1024 ports through capabilities. I think this could just be mode 500 though. Let me change that.
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.
It was a mistake to expose preStart
as an option in #48875 in the first place. Now we have to do ugly workarounds.
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.
My prestart scripts normal worked, not ugly.
nginx -t running from root incorrectly sets permissions log
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.
@Mic92 please make sure you're looking at the latest version of this PR -- it has close to no changes to the preStart script. This comment thread was started on an older version.
@Izorkin I don't understand why the preStart script permissions dance should change in any way from what is currently in master. The current version in this PR does the right thing.
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.
@delroth i have no deprecation in PR.
This needs a changelog since our acme certificates are not readable by default
|
@Mic92 agreed, I'll work on some documentation for the change. I expect some configs have been relying on the fact that the process was starting as root -- but I don't know yet how common that is or how hard to fix that is though (and I guess we won't really know until this is pushed to users...). Note that for the ACME case, if you use the useACME = true; option of nginx vhosts, the permissions should already be nginx:nginx and not root:root. |
I think served files should be mostly ok because they have to be already readable by |
83b5830
to
6034985
Compare
Drafted a changelog entry. English is not my first language, so suggestions are welcome :-) |
6034985
to
76f6c55
Compare
If delete the "nginx -t" service will work normally? |
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 would rather prefer starting nginx as root then having a setuid binary, where every member of the nginx
group could start nginx as the nginx user.
76f6c55
to
df5e4c5
Compare
@Mic92 as mentioned in the comment thread, there never was a setuid binary. |
Ok. Then can you rename the setcap wrapper to something else but |
@@ -630,17 +629,30 @@ in | |||
} | |||
]; | |||
|
|||
security.wrappers.nginx = { |
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.
@delroth
You can rename to security.wrappers.test-nginx ?
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.
Done.
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.
Huh what's the reason behind this? This has nothing to do with testing
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.
"chown" must be ran before "${cfg.package}/bin/nginx ... -t", otherwise an error will be observed. More details in #48875 (review) and #51551 (comment)
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.
@infinisil it does have to do with testing, because this wrapper for the nginx binary is only used for the config testing done at pre-startup. The reason why it needs a wrapper is that for some reason the nginx config test checks if ports can be listened on, and this fails for privileged ports without the proper cap. In serving situations, that cap is given by the systemd service. In pre-exec the cap is not provided, hence the need for this wrapper (which is only runnable by user nginx, so doesn't provide a universal cap privesc).
It's not very elegant and probably a better way would be to convince upstream to have an option to not do this listen port check. But frankly this is more effort than I'm willing to put into this -- way more effort than is required to rebase this PR in my local fork to get the benefits of it myself.
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.
@delroth there is still such a method https://github.com/nixcloud/nixcloud-webservices/blob/master/modules/web/webserver/lib/nginx_check_config.nix
Instead tell systemd to run the service as the requested user/group with the capability to listen on reserved ports (< 1024).
df5e4c5
to
0a427cc
Compare
@Mic92 this variant normal? |
I'm dropping this change, it seems to be going nowhere. If someone is motivated to follow through with the comments, feel free to pick it up. So far it's been less painful to rebase this change regularly in my local nixpkgs fork than to figure out how to upstream this... |
Instead tell systemd to run the service as the requested user/group with
the capability to listen on reserved ports (< 1024).
Motivation for this change
Defense in depth: less root processes running on the system.
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)