Skip to content
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.

Sign up for GitHub

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

Jump to bottom

nixos/nginx: Sandbox the service using systemd #60646

Closed
wants to merge 1 commit into from
Closed

nixos/nginx: Sandbox the service using systemd #60646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions nixos/doc/manual/release-notes/rl-1909.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,34 @@
the module for some time and so was removed as cleanup.
</para>
</listitem>
<listitem>
<para>
The nginx web server previously started its master process as root
privileged, then ran worker processes as a less privileged identity. This
was changed to start all of nginx as a less privileged user (defined by
<option>services.nginx.user</option> and
<option>services.nginx.group</option>). As a consequence, all files that
are needed for nginx to run (included configuration fragments, SSL
certificates and keys, etc.) must now be readable by this less privileged
identity.
Additionally, systemd sandboxing is now in place to prevent nginx from writing
to the filesystem.
</para>
</listitem>
<listitem>
<para>
The option <option>services.nginx.stateDiretory</option> was changed to
<option>services.nginx.stateDirectoryName</option> which means the
nginx state directory is always under <literal>/var/lib/</literal>.
This also means the old state directory needs to be moved from <literal>/var/spool/nginx</literal>,
for example using
<programlisting>
services.nginx.preStart = ''
cp -r /var/spool/nginx/* /var/lib/nginx/
'';
</programlisting>
</para>
</listitem>
</itemizedlist>
</section>

Expand Down
43 changes: 31 additions & 12 deletions nixos/modules/services/web-servers/nginx/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ let
''));

configFile = pkgs.writers.writeNginxConfig "nginx.conf" ''
user ${cfg.user} ${cfg.group};
pid /run/nginx/nginx.pid;
Copy link
Member

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?

error_log ${cfg.logError};
daemon off;

Expand Down Expand Up @@ -361,12 +361,7 @@ in

preStart = mkOption {
type = types.lines;
default = ''
test -d ${cfg.stateDir}/logs || mkdir -m 750 -p ${cfg.stateDir}/logs
test `stat -c %a ${cfg.stateDir}` = "750" || chmod 750 ${cfg.stateDir}
test `stat -c %a ${cfg.stateDir}/logs` = "750" || chmod 750 ${cfg.stateDir}/logs
chown -R ${cfg.user}:${cfg.group} ${cfg.stateDir}
'';
default = "";
description = "
Shell commands executed before the service's nginx is started.
";
Expand Down Expand Up @@ -442,10 +437,10 @@ in
";
};

stateDir = mkOption {
default = "/var/spool/nginx";
stateDirName = mkOption {
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Contributor

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?

default = "nginx";
description = "
Directory holding all state for nginx to run.
Name of the directory under /var/lib holding nginx's state.
";
};

Expand Down Expand Up @@ -640,14 +635,38 @@ in
preStart =
''
${cfg.preStart}
${cfg.package}/bin/nginx -c ${configFile} -p ${cfg.stateDir} -t
${cfg.package}/bin/nginx -c '${configFile}' -p '/var/lib/${cfg.stateDirName}' -t
'';
serviceConfig = {
ExecStart = "${cfg.package}/bin/nginx -c ${configFile} -p ${cfg.stateDir}";
ExecStart = "${cfg.package}/bin/nginx -c '${configFile}' -p '/var/lib/${cfg.stateDirName}'";
ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";
Restart = "always";
RestartSec = "10s";
StartLimitInterval = "1min";
# User and group
User = cfg.user;
Group = cfg.group;
# Filesystem access
ProtectSystem = "strict";
dasJ marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

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.

Copy link
Contributor

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.

ProtectHome = true;
Copy link
Member

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?

PrivateTmp = true;
PrivateDevices = true;
Copy link
Contributor

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.

Copy link
Contributor

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" ];
Copy link
Contributor

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?

Copy link
Contributor

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.

StateDirectoryMode = 750;
RuntimeDirectory = "nginx";
RuntimeDirectoryMode = 750;
# Capabilities
CapabilityBoundingSet = [ "CAP_NET_BIND_SERVICE" ];
AmbientCapabilities = [ "CAP_NET_BIND_SERVICE" ];
NoNewPrivileges = true;
Copy link
Contributor

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.

# Misc.
LockPersonality = true;
RestrictRealtime = true;
PrivateMounts = true;
MemoryDenyWriteExecute = true;
};
};

Expand Down