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
nixos/nginx: Sandbox the service using systemd #60646
Conversation
- nginx is not running as root anymore (Closes #56255) - Prevents nginx from accessing the filesystem - Enables most sandboxing options systemd offers - Moves the state directory to /var/lib
is this similair to #56255 ? |
Oh, just in case the question arises. I switched to systemd's |
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'm glad to see another attempt at this. @dasJ do you have a test plan you will document here? Do you have any volunteers to run this module in a variety of production environments? 19.09 is a bit off still so now is a great time to get documented testing underway. See my comments from the last attempt: #56255 (comment)
I still don't use nginx so am not sure how valid my comments/concerns are, but I'd like to be really sure that the testing involved with this PR is solid as it sets the precedent for apache httpd.
@@ -442,10 +437,10 @@ in | |||
"; | |||
}; | |||
|
|||
stateDir = mkOption { | |||
default = "/var/spool/nginx"; | |||
stateDirName = mkOption { |
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.
Is hard coding the state directory to /var/lib/nginx
a bad idea?
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.
Systemd only gives me the possibility to name a directory under /usr/lib as state directory. Absolute paths and .. don't work. Using another directory means I'd have to take care about mkdir, chown and chmod myself. This way, systemd does all that for me.
The only tradeoff is that the directory is moved from /var/spool, but /var/lib is the more appropriate location imo. Maybe we can get rid of the logs directory as NixOS's nginx logs errors to stderr and has no access log enabled by default.
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.
Yes I'm all for locking the state directory down under /var/lib/
my question was asking if the directory name under /var/lib/
needs to be an option? Could the state directory not be hard coded to /var/lib/nginx
(in other words StateDirectory = "nginx";
? Is there value to the user in allowing choice of the directory name here?
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 is possible to leave use custom folder?
@aanderse as I have never written any nixpkgs documentation, what would be an appropriate place? Just the changelog? |
@dasJ documentation regarding testing efforts wouldn't belong in the manual. Using this PR thread would be a sufficient place in my opinion. You could create a checkbox list of test scenarios and mark them as complete as people (yourself included) report back on this PR thread with what their test case is, how/when they ran it, and maybe some details relevant to showing how the test case worked/why the test case is positive evidence/what overall aspect is being tested. Just leave it up to the individual testing to write their report here and if anyone has more questions about the testing that occurred they could ask for clarification. I might try to write up some examples of test cases I'd like to see in the apache scenario for reference/example when I get a chance. NOTE: One more time I'd like to clarify that I have almost zero experience with |
I'll do the writeup later. In the meantime, you can test this change using: {
services.nginx = {
preStart = "";
stateDir = "/var/lib/nginx";
appendConfig = "pid /run/nginx/nginx.pid;";
};
systemd.services.nginx.serviceConfig = {
# User and group
User = "nginx";
Group = "nginx";
# Filesystem access
ProtectSystem = "strict";
ProtectHome = true;
PrivateTmp = true;
PrivateDevices = true;
ProtectKernelTunables = true;
ProtectKernelModules = true;
ProtectControlGroups = true;
StateDirectory = "nginx";
StateDirectoryMode = 750;
RuntimeDirectory = "nginx";
RuntimeDirectoryMode = 750;
# Capabilities
CapabilityBoundingSet = [ "CAP_NET_BIND_SERVICE" ];
AmbientCapabilities = [ "CAP_NET_BIND_SERVICE" ];
NoNewPrivileges = true;
# Misc.
LockPersonality = true;
RestrictRealtime = true;
PrivateMounts = true;
MemoryDenyWriteExecute = true;
} nginx will warn that the |
@@ -45,7 +45,7 @@ let | |||
'')); | |||
|
|||
configFile = pkgs.writers.writeNginxConfig "nginx.conf" '' | |||
user ${cfg.user} ${cfg.group}; | |||
pid /run/nginx/nginx.pid; |
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.
why is the stateDir configurable but the runtimeDir not?
As a quick example here are some of the things I would like to see tested for
While some of this might seem like overkill I think it is important to keep in mind that apache was designed to run as root and weird bugs can pop up in places you might never expect when you run software in ways which were not intended. We don't want to push a change like this into production and break a bunch of production machines when people upgrade to 19.09. Please consider what the |
Group = cfg.group; | ||
# Filesystem access | ||
ProtectSystem = "strict"; | ||
ProtectHome = true; |
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.
What impact will this have on things like mod_userdir? Will this break any nginx
modules?
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 using systemd sandboxing options and also not running nginx as root, but I think some of these changes will cause too much breakage. In particular, there are lots of reasons that nginx needs read and write access to arbitrary parts of the filesystem. Just looking through my configuration, I identified a couple of things that will break with no satisfactory workaround that I can see.
Also, these restrictions risk confusing users as they have to use nginx differently than they are used to on other operating systems.
Note that I have not actually tested this PR on any of my systems, so I could be wrong about some of the breakages.
# Capabilities | ||
CapabilityBoundingSet = [ "CAP_NET_BIND_SERVICE" ]; | ||
AmbientCapabilities = [ "CAP_NET_BIND_SERVICE" ]; | ||
NoNewPrivileges = true; |
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.
Technically redundant, because NoNewPrivileges
is implied by a number of the other options, but maybe it is better to be explicit.
ProtectSystem = "strict"; | ||
ProtectHome = true; | ||
PrivateTmp = true; | ||
PrivateDevices = true; |
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 this will break my configuration which logs to journald using /dev/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.
Shouldn't this still work with PrivateDevices=yes
? https://github.com/systemd/systemd/blob/master/src/core/namespace.c#L734-L736
ProtectKernelTunables = true; | ||
ProtectKernelModules = true; | ||
ProtectControlGroups = true; | ||
StateDirectory = [ "${cfg.stateDirName} ${cfg.stateDirName}/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.
Shouldn't this be a list with two elements, rather than a single string with a space?
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.
We just pass that through to systemd, and systemd splits that with spaces.
User = cfg.user; | ||
Group = cfg.group; | ||
# Filesystem access | ||
ProtectSystem = "strict"; |
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.
This will break existing cache setups, including mine. I keep my cache in a folder under /var/cache
(for no particular technical reason, but because it seemed like the proper place). One solution is to set CacheDirectory="nginx"
and make users either place their caches in /var/cache/nginx
or add another CacheDirectory
to the service themselves. That seems better than putting caches in the state directory.
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 usually use socket files in /var/run
to communicate with services that I am reverse proxying with nginx and I believe this will break this functionality.
Since it has been hard to implement this in the past and there are still a lot of question marks, what about making sandboxing opt-in behind some nginx.sandboxing.enable option? That way the brave can begin testing that config (the option should be copiously documented) without forcing all the problems to be found and fixed up front. |
@chreekat the brave are free to include this module in their own configurations and report results back here. Given how easy it is to replace a module in your own configuration.nix I fail to see the value of including such an experimental option on such an important piece of software. I'm happy to hear reasoning on differing opinions, though. |
That makes sense, people can indeed include this module. My reason for suggesting inclusion was knowing how important it is to break down patches into something that can get merged before they rot or the submitter runs out of steam. But of course the approach I suggested might not be workable. :) One reason to add an option for such an experimental feature, though, would be to increase discoverability. I only found this PR because I'm taking a random walk through the repo; who else might be out there who could try this and report back if only they knew about it? Would the additional feedback justify giving people a sharp edge to play with? |
I think discourse as well as irc might be a better place to find people who will be willing to test this and report results back. Once an organized test plan which shows a fair bit of coverage for the spread of features available in nginx has been documented here I would suggest then trying to "recruit" people for testing on these mediums. |
The only downside is that the worker processes are now able to read the certificates.
Motivation for this change
nginx is running on most of my servers on a well-known port and is therefore a great attack vector. Restricting it as much as possible should help around some zero-day exploits.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)