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.

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/phpfpm: run as non root user for enhanced security #73223

Open
wants to merge 2 commits into
base: master
from

Conversation

@aanderse
Copy link
Contributor

aanderse commented Nov 11, 2019

Motivation for this change

Security.

NOTE: I'm not specifically familiar with linux capabilities beyond the man page so if I'm missing anything in AmbientCapabilities or CapabilitiesBoundingSet please let me know.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@dasJ

This comment has been minimized.

Copy link
Member

dasJ commented Nov 11, 2019

@aanderse I don't have time to review this rn, but I'll do so later. In the meantime: CapabilityBoundingSet is a set of capabilities the service may have at some point, this includes setuid or setcap gained capabilities.
AmbientCapabilities is a set of capabilities the service will just get when spawned and not running as root

Example: If your service wants to bind a privileged port (< 1024), and is not running as root, you need to add CAP_NET_BIND_SERVICE to CapabilityBoundingSet (because the process will need it at some point) and AmbientCapabilities (because the process is not running as root and would otherwise not have it).

@aanderse aanderse force-pushed the aanderse:phpfpm branch from 1da11ee to 1c46e21 Nov 11, 2019
@aanderse aanderse requested a review from flokli Nov 11, 2019
@aanderse aanderse force-pushed the aanderse:phpfpm branch from 1c46e21 to 3a138cd Nov 11, 2019
@aanderse aanderse changed the title nixos/phpfpm: run as non root user for enhanced security WIP: nixos/phpfpm: run as non root user for enhanced security Nov 11, 2019
@aanderse

This comment has been minimized.

Copy link
Contributor Author

aanderse commented Nov 11, 2019

Having some issues getting this to work properly with socket activation. Will get back to it when I have some time. Any help/testing appreciated.

Copy link
Contributor

florianjacob left a comment

Socket activation, yeah! 🥳

@florianjacob

This comment has been minimized.

Copy link
Contributor

florianjacob commented Nov 18, 2019

Having some issues getting this to work properly with socket activation. Will get back to it when I have some time. Any help/testing appreciated.

How do the problems manifest? From my previous socket activation experience, everything seems to be allright? :/

@aanderse aanderse force-pushed the aanderse:phpfpm branch from 3a138cd to d570d3d Nov 22, 2019
@aanderse

This comment has been minimized.

Copy link
Contributor Author

aanderse commented Nov 22, 2019

How do the problems manifest? From my previous socket activation experience, everything seems to be allright? :/

After circling back to this I have found the problem! Another case of PEBKAC seems to be the culprit, as I can't reproduce any problems now and everything is running smoothly...

Next steps:

  • test without using sockets
  • have other people test
@flokli

This comment has been minimized.

Copy link
Contributor

flokli commented Nov 23, 2019

@aanderse I think we should add some phpfpm-specific tests to nixos/tests (we currently only test this via other modules which enable it indirectly).

Those should include both connection via unix file sockets and tcp.

@aanderse aanderse force-pushed the aanderse:phpfpm branch 3 times, most recently from 7eef418 to ee20f29 Nov 23, 2019
@aanderse

This comment has been minimized.

Copy link
Contributor Author

aanderse commented Nov 23, 2019

@GrahamcOfBorg test phpfpm
@GrahamcOfBorg test limesurvey matomo
@GrahamcOfBorg test mediawiki roundcube
@GrahamcOfBorg test moodle
@GrahamcOfBorg test wordpress
@GrahamcOfBorg test nextcloud.with-postgresql-and-redis

@aanderse aanderse force-pushed the aanderse:phpfpm branch from ee20f29 to 2f117e2 Nov 23, 2019
@aanderse aanderse changed the title WIP: nixos/phpfpm: run as non root user for enhanced security nixos/phpfpm: run as non root user for enhanced security Nov 23, 2019
@aanderse

This comment has been minimized.

Copy link
Contributor Author

aanderse commented Nov 23, 2019

Sorry for the noise up until this point. I believe this PR is ready for review now. Thanks for the help so far.

nixos/tests/phpfpm.nix Show resolved Hide resolved
site2 = {
user = config.services.httpd.user;
group = config.services.httpd.group;
settings = {

This comment has been minimized.

Copy link
@flokli

flokli Nov 23, 2019

Contributor

we might want to run that sites phpfpm pool as another user.

This comment has been minimized.

Copy link
@aanderse

aanderse Nov 23, 2019

Author Contributor

In general I agree but for a simple test I'm less motivated. If you feel like this serves for good documentation I'll gladly accept a suggestion or commit.

This comment has been minimized.

Copy link
@flokli

flokli Nov 25, 2019

Contributor

I can include add this to the test, once I fully grasped the rest :-)

};
};

systemd.tmpfiles.rules = [

This comment has been minimized.

Copy link
@flokli

flokli Nov 23, 2019

Contributor

can you use the right user and groups here?

This comment has been minimized.

Copy link
@aanderse

aanderse Nov 23, 2019

Author Contributor

You'll have to explain why root isn't the right user in this case :-\ I'm not in the habit of making source code mutable or owned by the executing user if there is no need to.

This comment has been minimized.

Copy link
@flokli

flokli Nov 25, 2019

Contributor

Looking at wordpress and friends, I seems quite common to have php code owned by the process executing it. If we look at extending the test to run the php interpreter as another user than the webserver, having the htdoc files owned by the specific php interpreter might make sense to mimic that behaviour.

This comment has been minimized.

Copy link
@aanderse

aanderse Nov 25, 2019

Author Contributor

Whenever a user can execute and modify its own code in NixOS it is either: a compromise on security for code ease/clarity, the user requires the ability to modify its own code (usually because of configuration, cache, etc...), or the author has maybe just hasn't thought about it. Generally we don't have the problem too much in nixpkgs because most code is immutable, and when we do have mutable code it usually falls into the category of configuration/cache that is required to be mutable.

Perhaps having those two .php files packaged in the nix store would make the situation more acceptable and familiar... 🤔

@flokli flokli force-pushed the aanderse:phpfpm branch from 2f117e2 to 2909d87 Nov 25, 2019
Copy link
Contributor

flokli left a comment

I rebased on latest master to fix the conflicts.

However, looking at the test updates, this looks we also switch to socket activation by default.

Depending on the environment, some people still might want to autostart the phpfpm pools during boot. Can we add a release note entry, explaining how to get them to autostart (is it just systemd.targets.phpfmp.wantedBy = "multi-user.target"?)

@aanderse

This comment has been minimized.

Copy link
Contributor Author

aanderse commented Nov 25, 2019

I rebased on latest master to fix the conflicts.

However, looking at the test updates, this looks we also switch to socket activation by default.

Depending on the environment, some people still might want to autostart the phpfpm pools during boot. Can we add a release note entry, explaining how to get them to autostart (is it just systemd.targets.phpfmp.wantedBy = "multi-user.target"?)

Definitely. Thank you for bringing that point up, it had slipped by me. Consider me on it.

@aanderse aanderse force-pushed the aanderse:phpfpm branch 2 times, most recently from b8cc042 to 2141386 Nov 28, 2019
@aanderse

This comment has been minimized.

Copy link
Contributor Author

aanderse commented Nov 29, 2019

@flokli I brought the slices and targets back in, and modified such that if you want a socket activated service to start right away you can do it per pool by adding this to your configuration: systemd.services.phpfpm-pool.wantedBy = [ "multi-user.target" ];

How does it look now?

@flokli flokli force-pushed the aanderse:phpfpm branch from 2141386 to 1970826 Dec 14, 2019
@flokli

This comment has been minimized.

Copy link
Contributor

flokli commented Dec 14, 2019

Rebased on latest master.

With all the back and forth w.r.t restarting socket units (#73871), it might make sense to keep systemd-powered socket activation outside this PR.

@aanderse, WDYT?

@aanderse

This comment has been minimized.

Copy link
Contributor Author

aanderse commented Dec 14, 2019

It seems that the correct way to make phpfpm run as a non root user is to use systemd. This PR doesn't really aim to accomplish much beyond running as a non root user. I'm happy to let this sit for a while because I don't think there is a great way to accomplish this without systemd sockets.

@dasJ

This comment has been minimized.

Copy link
Member

dasJ commented Dec 20, 2019

@aanderse I was playing around with php-fpm and I have it kind of running without socket units. While socket units really are the best solution, I'm working around that with the following systemd service options:

User = "kimai";
AmbientCapabilities = "CAP_CHOWN";

This way, php-fpm is allowed to chown the socket to any user. However, this gets really nasty with the permissions of /run/phpfpm, which is why I believe the best way to solve this are socket units.

@aanderse

This comment has been minimized.

Copy link
Contributor Author

aanderse commented Dec 20, 2019

@dasJ unfortunately chown could be called on anything by any .php script then. I'm hoping to get back to this after my apache pr is merged.

@@ -149,20 +146,25 @@ in {
imports = [
(mkRemovedOptionModule [ "services" "phpfpm" "poolConfigs" ] "Use services.phpfpm.pools instead.")
(mkRemovedOptionModule [ "services" "phpfpm" "phpIni" ] "")
];
(mkRemovedOptionModule [ "services" "phpfpm" "listen" ] "Reference the read-only option config.services.phpfpm.pools.<name>.socket to access the path of your socket.")

This comment has been minimized.

Copy link
@Izorkin

Izorkin Dec 24, 2019

Contributor

missing
];

This comment has been minimized.

Copy link
@aanderse

aanderse Dec 25, 2019

Author Contributor

Looks like a change to master merged poorly? Will fix shortly.

"listen.group" = poolOpts.group;
"listen.mode" = "0660";
"user" = poolOpts.user;
"group" = poolOpts.group;

This comment has been minimized.

Copy link
@Izorkin

Izorkin Dec 24, 2019

Contributor

This options not used

дек 24 19:50:05 NixOS-Test php-fpm[6321]: [NOTICE] [pool test-02] 'user' directive is ignored when FPM is not running as root
дек 24 19:50:05 NixOS-Test php-fpm[6321]: [NOTICE] [pool test-02] 'group' directive is ignored when FPM is not running as root

This comment has been minimized.

Copy link
@Izorkin

Izorkin Dec 24, 2019

Contributor

After delete these lines - Service stops starting.
Worked with this lines:

          RuntimeDirectory = "phpfpm-{pool}";
          RuntimeDirectoryMode = "0750";

With RuntimeDirectory = "phpfpm"; and 2 pools the directory /run/phpfpm is recreated. Need use "phpfpm-{pool}";

This comment has been minimized.

Copy link
@Izorkin

Izorkin Dec 24, 2019

Contributor

my example patch

diff --git a/nixos/modules/services/web-servers/phpfpm/default.nix b/nixos/modules/services/web-servers/phpfpm/default.nix
index 31f4ae5445f..9e0177c22c8 100644
--- a/nixos/modules/services/web-servers/phpfpm/default.nix
+++ b/nixos/modules/services/web-servers/phpfpm/default.nix
@@ -5,8 +5,6 @@ with lib;
 let
   cfg = config.services.phpfpm;

-  runtimeDir = "/run/phpfpm";
-
   toStr = value:
     if true == value then "yes"
     else if false == value then "no"
@@ -42,7 +40,7 @@ let
       options = {
         socket = mkOption {
           type = types.str;
-          default = "${runtimeDir}/${name}.sock";
+          default = "/run/phpfpm-${name}/${name}.sock";
           readOnly = true;
           description = ''
             Path to the unix socket file on which to accept FastCGI requests.
@@ -136,8 +134,6 @@ let
           "listen.owner" = poolOpts.user;
           "listen.group" = poolOpts.group;
           "listen.mode" = "0660";
-          "user" = poolOpts.user;
-          "group" = poolOpts.group;
         };
       };
     };
@@ -147,6 +143,7 @@ in {
     (mkRemovedOptionModule [ "services" "phpfpm" "poolConfigs" ] "Use services.phpfpm.pools instead.")
     (mkRemovedOptionModule [ "services" "phpfpm" "phpIni" ] "")
     (mkRemovedOptionModule [ "services" "phpfpm" "listen" ] "Reference the read-only option config.services.phpfpm.pools.<name>.socket to access the path of your socket.")
+  ];

   options = {
     services.phpfpm = {
@@ -329,6 +326,8 @@ in {
           KillMode = mkIf createSocket "process";
           User = poolOpts.user;
           Group = poolOpts.group;
+          RuntimeDirectory = "phpfpm-${pool}";
+          RuntimeDirectoryMode = "0750";
         };
       }
     ) cfg.pools;

This comment has been minimized.

Copy link
@aanderse

aanderse Dec 25, 2019

Author Contributor

listen.owner, listen.group, listen.mode, user, and group are all used by the nixos module and will not be used by phpfpm anymore. The reason they remain is documentation. If those values aren't there users can be confused what is happening. Tricky because systemd is taking care of everything that phpfpm used to... but better than leaving users without any idea IMO.

This comment has been minimized.

Copy link
@aanderse

aanderse Dec 25, 2019

Author Contributor

RuntimeDirectory shouldn't be needed anymore because systemd socket is taking care of directory and socket creation.

This comment has been minimized.

Copy link
@Izorkin

Izorkin Dec 25, 2019

Contributor

Options user and group writed to phpfpm configuration:
cat /nix/store/...-phpfpm-test-01.conf

[global]
daemonize = no
error_log = syslog
rlimit_core = unlimited
rlimit_files = 131072


[test-01]
group = nginx
listen = /run/phpfpm-test-01/test-01.sock
listen.group = nginx
listen.mode = 0660
listen.owner = nginx
pm = dynamic
pm.max_children = 20
pm.max_requests = 400
pm.max_spare_servers = 12
pm.min_spare_servers = 2
pm.start_servers = 4
user = nginx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.