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

apache-httpd: do not run anything as root #56304

Closed
wants to merge 1 commit into from

Conversation

Izorkin
Copy link
Contributor

@Izorkin Izorkin commented Feb 24, 2019

Motivation for this change

Disable running Apache-httpd from root

Please recheck config.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 24, 2019
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need add something to release notes talking about this change. Both certbot & security.acme.certs generate root only readable certs by default, which will cause breakage. In the release notes you should explain what steps people need to take to ensure their apache servers won't break because of this.

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Izorkin After some more review this will break a bunch of the apache submodules so they will need to be modified/updated for this as well. Quick example: https://github.com/NixOS/nixpkgs/blob/774fd3e6f2c06f5ee1531b5606f139b15be19b68/nixos/modules/services/web-servers/apache-httpd/limesurvey.nix#L189

@Izorkin
Copy link
Contributor Author

Izorkin commented Feb 28, 2019

@aanderse to create the database postgres require root privileges. I don't know how to get around it.

@aanderse
Copy link
Member

@aanderse to create the database postgres require root privileges. I don't know how to get around it.

@Izorkin That definitely is a common problem in many NixOS modules! MySQL is great because you can use ensureDatabases and ensureUsers for services. See https://discourse.nixos.org/t/postgresql-module-vs-mysql-module/2243/3 for my proposal to fix this problem.

@Izorkin
Copy link
Contributor Author

Izorkin commented Feb 28, 2019

Also in the limesurvey module, the rule does not work systemd.tmpfiles.rules. How can I fix it?

diff --git a/nixos/modules/services/web-servers/apache-httpd/limesurvey.nix b/nixos/modules/services/web-servers/apache-httpd/limesurvey.nix
index 5c387700a5d..016bb0db57c 100644
--- a/nixos/modules/services/web-servers/apache-httpd/limesurvey.nix
+++ b/nixos/modules/services/web-servers/apache-httpd/limesurvey.nix
@@ -177,12 +177,13 @@ in rec {
     };
   };
 
+  systemd.tmpfiles.rules = [
+    "d '${config.dataDir}' 0750 ${serverInfo.serverConfig.user} ${serverInfo.serverConfig.group} - -"
+  ];
+
   startupScript = pkgs.writeScript "limesurvey_startup.sh" ''

Maybe in the Add option enableRootlessMode = false; ?
If you do not need a submodule, turn on enableRootlessMode = true;

@aanderse
Copy link
Member

@Izorkin I imagine you would have to do something like config.systemd.tmpfiles.rules instead of just systemd.tmpfiles.rules, but I haven't tested that.

@Izorkin
Copy link
Contributor Author

Izorkin commented Feb 28, 2019

This option does not work.

@aanderse
Copy link
Member

aanderse commented Mar 2, 2019

This option does not work.

I see the problem. The way these submodules are called I don't believe they can modify the global config (anyone please correct me if I'm wrong). I can imagine there would be some hacky workarounds someone could write to allow subservices to modify the config, but that seems like the wrong path to go down.

There has been discussion on the state of subservices in the past, but it is a complicated topic and never seems to end up with a solution. See #6960, #18977, and #22067.

I'm of the opinion that several of these subservices (specifically the ones that use php) shouldn't exist as apache subservices, but as their own modules utilizing phpfpm. If limesurvey was moved to a module of its own and used phpfpm this problem wouldn't exist, and limesurvey could be served up over either apache, nginx, or any other web server which supports this.

The remaining subservices which aren't using php look like they probably only require minimal changes (if any) to work without any root privileges which is probably a good idea anyways... Anyone who maintains their own private subservices outside of nixpkgs could have problems with this, but I think that risk is minimal and with documentation in the release notes that isn't a big concern.

I think several things need to happen before this PR can go much further, but I'm interested in helping out so I might take a stab at converting a php subservice to its own module and seeing how that goes.

  • in no way required, but would be awesome if this change was made first: https://discourse.nixos.org/t/postgresql-module-vs-mysql-module/2243/3
  • foswiki startupScript to be modified to assume "vardir" already exists, and fail with an error message otherwise dropped
  • limesurvey moved to its own module and utilize phpfpm, documentation updated, and for bonus points a nixos test written
  • mediawiki moved to its own module and utilize phpfpm, documentation updated, and for bonus points a nixos test written (nixos/mediawiki: init service to replace httpd subservice #62748)
  • phabricator moved to its own module and utilize phpfpm, documentation updated, and for bonus points a nixos test written (note a phd module exists which could then be merged with the new phabricator module) dropped
  • trac startupScript calls mkdir /var/trac which needs to be called some other way... or this could be moved to its own module and from that module modify services.httpd like other modules do (see nagios) dropped
  • wordpress moved to its own module and utilize phpfpm, documentation updated, and for bonus points a nixos test written
  • zabbix subservice could probably just be moved into the existing zabbix module (nixos/zabbix: overhaul package & module #63844)
  • tomcat-connector can hopefully just be dropped, or integrated right into the httpd module (nixos/httpd: drop tomcat-connector httpd subservice #64052)

@Izorkin thoughts?

@Izorkin
Copy link
Contributor Author

Izorkin commented Mar 14, 2019

Updated to new variant.
Limesurvey worked.
Please recheck.

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Mar 14, 2019
@Izorkin
Copy link
Contributor Author

Izorkin commented Mar 14, 2019

MediaWiki worked!

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jun 2, 2019
@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 3, 2019

Rebased PR

@aanderse
Copy link
Member

Use of CAP_SETUID would cause serious security problems in combination with enablePerl and enablePHP. Potential other problems as well.

I'm working on a set of PRs and intend to include this functionality before the 20.03 release.

@aanderse aanderse closed this Nov 12, 2019
@Izorkin Izorkin deleted the httpd-rootless branch November 12, 2019 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants