nixos/nextcloud: use LoadCredential to read secrets #367433

networkException · 2024-12-22T19:14:15Z

This pull request adds support for using systemd's LoadCredential= feature to read various secret files used by nextcloud service units.

Previously credentials had to be readable by the nextcloud user, this is now no longer required.

The nextcloud-occ wrapper script has been adjusted to use systemd-run for loading credentials when being called from outside a service.

See the individual commits for more details.

Depends on #365442
Continuation of #152141

Things done

Add a 👍 reaction to pull requests you find important.

networkException · 2024-12-22T19:23:59Z

This should be generally well tested already so reviews are encouraged, undrafting when

I've run the tests against master in addition to nixos-unstable
I've tested this on my deployment
nixos/tests/nextcloud: fix notify_push test; nixos/nextcloud-notify_push: add nextcloudUrl option #365442 is merged

nixos/tests/nextcloud/with-declarative-redis-and-secrets.nix

nixos/modules/services/web-apps/nextcloud.nix

SuperSandro2000 · 2024-12-22T20:00:20Z

nixos/modules/services/web-apps/nextcloud.nix

+  runtimeSystemdCredentials = []
+    ++ (lib.optional (cfg.config.dbpassFile != null) "dbpass:${cfg.config.dbpassFile}")
+    ++ (lib.optional (cfg.config.objectstore.s3.enable) "s3_secret:${cfg.config.objectstore.s3.secretFile}")
+    ++ (lib.optional (cfg.config.objectstore.s3.sseCKeyFile != null) "s3_sse_c_key:${cfg.config.objectstore.s3.sseCKeyFile}");


Suggested change

runtimeSystemdCredentials = []

++ (lib.optional (cfg.config.dbpassFile != null) "dbpass:${cfg.config.dbpassFile}")

++ (lib.optional (cfg.config.objectstore.s3.enable) "s3_secret:${cfg.config.objectstore.s3.secretFile}")

++ (lib.optional (cfg.config.objectstore.s3.sseCKeyFile != null) "s3_sse_c_key:${cfg.config.objectstore.s3.sseCKeyFile}");

runtimeSystemdCredentials = lib.optional (cfg.config.dbpassFile != null) "dbpass:${cfg.config.dbpassFile}"

++ lib.optional (cfg.config.objectstore.s3.enable) "s3_secret:${cfg.config.objectstore.s3.secretFile}"

++ lib.optional (cfg.config.objectstore.s3.sseCKeyFile != null) "s3_sse_c_key:${cfg.config.objectstore.s3.sseCKeyFile}";

I prefer consistent indentation

nixos/modules/services/web-apps/nextcloud.nix

SuperSandro2000 · 2024-12-22T20:44:46Z

nixos/modules/services/web-apps/nextcloud-notify_push.nix

+      nextcloud-notify_push_setup = {
+        wantedBy = [ "multi-user.target" ];
+        requiredBy = [ "nextcloud-notify_push.service" ];
+        after = [ "nextcloud-notify_push.service" ];
+        serviceConfig = {
+          Type = "oneshot";
+          User = "nextcloud";
+          Group = "nextcloud";
+          ExecStart = "${lib.getExe cfgN.occ} notify_push:setup ${cfg.nextcloudUrl}/push";
+          LoadCredential = config.systemd.services.nextcloud-cron.serviceConfig.LoadCredential;
+        };
      };


This will definitely not work. notify_push:setup verifies if the configuration is correct and checks if the domain and trusted proxy settings are correct. It is not similar to nextcloud-setup and must be run after the notify_push service is up. If it fails the service is none functional.

I'm not too invested in reducing how many credentials the long running process can read. The ordering works just fine like this with after and Type=notify.

In general having nextcloud-notify_push_setup.service compared to postStart just changes which service fails in case something goes wrong

This will definitely not work. notify_push:setup verifies if the configuration is correct and checks if the domain and trusted proxy settings are correct

@SuperSandro2000 I think @networkException is right here and I fail to understand what'll break exactly either.
Please elaborate.

Ma27 · 2024-12-22T20:53:28Z

Thanks!
Will try to review in the upcoming days.

…-and-secrets This patch adds a subtest and corresponding configuration to with-declarative-redis-and-secrets to test for nextcloud notify_push to be working, just as in with-postgresql-and-redis. As notify_push needs to connect to the database, including it in this test checks that it can read the dbpassFile properly.

This patch replaces the use of writeScriptBin for the nextcloud-occ script with writeShellApplication, enabling shell checking. This patch also updates various invocations of the script to use lib.getExe.

…closer to phpfpm config

nixos/modules/services/web-apps/nextcloud.nix

Ma27 · 2024-12-29T16:46:29Z

nixos/modules/services/web-apps/nextcloud.nix

          # On Nextcloud ≥ 26, it is not necessary to patch the database files to prevent
          # an automatic creation of the database user.
          environment.NC_setup_create_db_user = lib.mkIf (nextcloudGreaterOrEqualThan "26") "false";
        };
        nextcloud-cron = {
          after = [ "nextcloud-setup.service" ];
+          # FIXME: In contrast to the occ wrapper script running phpCli directly will not
+          #        set NEXTCLOUD_CONFIG_DIR correctly currently.


Can you explain why you added a fixme for that?

Maybe a note is better, I added this after removing the explicit (somewhat duplicate) variable from the nextcloud-update-db service

Ma27 · 2024-12-29T16:55:32Z

nixos/modules/services/web-apps/nextcloud.nix

+            umask 0077
+            # NOTE: The phpfpm runtime directory is currently preserved
+            #       between restarts.
+            rm -rf /run/phpfpm/nextcloud-credentials/


I think it would be nicer to use $RUNTIME_DIRECTORY/nextcloud-credentials/ here.
It's rather unlikely that you ever change serviceConfig.RuntimeDirectory in a phpfpm unit, but with that change we'd be safe here.

Also... doesn't this mean that other phpfpm instances can see these values (even though only readable as root, so not from PHP code)?

I think it would be nicer to use $RUNTIME_DIRECTORY/nextcloud-credentials/ here.

I don't think this currently applies here but $RUNTIME_DIRECTORY has the pesky detail that you can technically specify multiple directories which will end up as /run/a:/run/b in the variable

Also... doesn't this mean that other phpfpm instances can see these values (even though only readable as root, so not from PHP code)?

Yes sadly thats the case, briefly as root and then as nextcloud. I don't know phpfpm good enough to know what would happen if the service were rootless, but that would be much cleaner here

I don't think this currently applies here but $RUNTIME_DIRECTORY has the pesky detail that you can technically specify multiple directories which will end up as /run/a:/run/b in the variable

Meh, makes perfect sense for a variable name in signular :/
but good point, I'll retract that.

Yes sadly thats the case, briefly as root and then as nextcloud. I don't know phpfpm good enough to know what would happen if the service were rootless, but that would be much cleaner here

My main problem is that the runtime directory is used for each phpfpm service, i.e. each PHP application on the same host will have this in its runtime directory (sure, different user then, but this smells like a disaster waiting to happen).

But given that we can specify multiple runtime directories, wouldn't it be an option to add another directory to the phpfpm-nextcloud service and place the secrets into that dir?

But given that we can specify multiple runtime directories, wouldn't it be an option to add another directory to the phpfpm-nextcloud service and place the secrets into that dir?

Sure, good point. I'll work on this in a bit

I've added a separate runtime directory to the service (https://github.com/NixOS/nixpkgs/compare/dedb0ac10499c7406ce55399db5ac7fdf3568907..4d4d3672a52c630e4a3f7970b5c3aa1bb2beda00). Note that sadly RuntimeDirectoryPreserve=true as well as the mode also applies. I also don't know a way to avoid the lib.mkForce here.

Ma27 · 2024-12-29T17:05:54Z

nixos/modules/services/web-apps/nextcloud-notify_push.nix

+      nextcloud-notify_push_setup = {
+        wantedBy = [ "multi-user.target" ];
+        requiredBy = [ "nextcloud-notify_push.service" ];
+        after = [ "nextcloud-notify_push.service" ];
+        serviceConfig = {
+          Type = "oneshot";
+          User = "nextcloud";
+          Group = "nextcloud";
+          ExecStart = "${lib.getExe cfgN.occ} notify_push:setup ${cfg.nextcloudUrl}/push";
+          LoadCredential = config.systemd.services.nextcloud-cron.serviceConfig.LoadCredential;
+        };
      };


This will definitely not work. notify_push:setup verifies if the configuration is correct and checks if the domain and trusted proxy settings are correct

@SuperSandro2000 I think @networkException is right here and I fail to understand what'll break exactly either.
Please elaborate.

Ma27 · 2024-12-29T18:37:05Z

nixos/modules/services/web-apps/nextcloud.nix

+            # NOTE: This early returns the script when nextcloud is in maintenance mode
+            #       or needs `occ upgrade`. Using ExecCondition= is not possible here
+            #       because it doesn't work with systemd credentials.
+            ${lib.getExe occ} status -e; [ $? -ne 0 ] && exit 0


Hmm, doesn't this fail the service since script adds a set -e?

I've changed the implementation to use status --output=json, feels nicer compared to -e anyways: https://github.com/NixOS/nixpkgs/compare/4d4d3672a52c630e4a3f7970b5c3aa1bb2beda00..3d64615e8a60490360283fad2ed7b8bca6f29372

Ma27 · 2024-12-30T15:47:34Z

nixos/modules/services/web-apps/nextcloud.nix

+          script = ''
+            # NOTE: This early returns the script when nextcloud is in maintenance mode
+            #       or needs `occ upgrade`. Using ExecCondition= is not possible here
+            #       because it doesn't work with systemd credentials.


fwiw opened systemd/systemd#35788 to discuss whether we could change this upstream.

This patch adds support for using systemd's LoadCredential feature to read various secret files used by nextcloud service units. Previously credentials had to be readable by the nextcloud user, this is now no longer required. The nextcloud-occ wrapper script has been adjusted to use systemd-run for loading credentials when being called from outside a service. In detail this change touches various details of the module: - The nix_read_secret() php function now takes the name of a file relative to the path specified in the CREDENTIALS_DIRECTORY environment variable. - The nix_read_secret() now exits with error code 1 instead of throwing a RuntimeException as this will properly error out the nextcloud-occ script - Only the nextcloud-setup service unit has the adminpass credential added in addition to the other credentials - Uses of ExecCondition= in nextcloud-cron and nextcloud-update-db have been replaced by a shell conditional as ExecCondition currently doesn't support credentials - The phpfpm-nextcloud service now runs a preStart script to make the credentials it gets readable by the nextcloud user as the unit runs as root but the php process itself as nextcloud. - To invoke occ notify_push:setup when using nextcloud notify_push a new service has been added that replaces the preStart script in nextcloud-notify_push.service. This has been done as the main executable only needs the database password credential. Co-authored-by: lassulus <[email protected]>

This patch changes the implementation of the subtests to check for redis' cache being non empty to only run redis-cli and jq in a shell and assert the returned length in python. This fixes jq "len" simply not compiling and makes sure regressions get noticed.

github-actions bot 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/` labels Dec 22, 2024

networkException requested review from SuperSandro2000, Ma27 and Lassulus December 22, 2024 19:18

networkException force-pushed the nextcloud-loadcredential branch from b15113c to 6587fd2 Compare December 22, 2024 19:21

github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 22, 2024

SuperSandro2000 reviewed Dec 22, 2024

View reviewed changes

nixos/tests/nextcloud/with-declarative-redis-and-secrets.nix Outdated Show resolved Hide resolved

SuperSandro2000 reviewed Dec 22, 2024

View reviewed changes

nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved

SuperSandro2000 reviewed Dec 22, 2024

View reviewed changes

nixos/modules/services/web-apps/nextcloud.nix Show resolved Hide resolved

SuperSandro2000 reviewed Dec 22, 2024

View reviewed changes

networkException added 3 commits December 22, 2024 22:10

nixos/nextcloud: use writeShellApplication for nextcloud-occ

c4ada7d

This patch replaces the use of writeScriptBin for the nextcloud-occ script with writeShellApplication, enabling shell checking. This patch also updates various invocations of the script to use lib.getExe.

nixos/nextcloud: move systemd service overrides for phpfpm-nextcloud …

30a23e1

…closer to phpfpm config

networkException force-pushed the nextcloud-loadcredential branch 2 times, most recently from 6c60763 to 6fece8a Compare December 22, 2024 22:12

networkException marked this pull request as ready for review December 23, 2024 16:27

Ma27 reviewed Dec 29, 2024

View reviewed changes

networkException force-pushed the nextcloud-loadcredential branch from fd5180a to dedb0ac Compare December 29, 2024 17:38

Ma27 reviewed Dec 29, 2024

View reviewed changes

Ma27 reviewed Dec 30, 2024

View reviewed changes

networkException force-pushed the nextcloud-loadcredential branch from dedb0ac to 4d4d367 Compare January 1, 2025 00:08

networkException and others added 3 commits January 1, 2025 02:14

nixos/tests/nextcloud: use lib instead of pkgs.lib wherever trivial

acdb8de

networkException force-pushed the nextcloud-loadcredential branch from 4d4d367 to 3d64615 Compare January 1, 2025 01:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/nextcloud: use LoadCredential to read secrets #367433

nixos/nextcloud: use LoadCredential to read secrets #367433

networkException commented Dec 22, 2024 •

edited

Loading

networkException commented Dec 22, 2024 •

edited

Loading

SuperSandro2000 Dec 22, 2024

networkException Jan 1, 2025

SuperSandro2000 Dec 22, 2024

networkException Dec 22, 2024

Ma27 Dec 29, 2024

Ma27 commented Dec 22, 2024

Ma27 Dec 29, 2024

networkException Dec 29, 2024

Ma27 Dec 29, 2024

Ma27 Dec 29, 2024

networkException Dec 29, 2024

networkException Dec 29, 2024

Ma27 Dec 29, 2024

networkException Dec 30, 2024

networkException Jan 1, 2025

Ma27 Dec 29, 2024

Ma27 Dec 29, 2024

networkException Jan 1, 2025

Ma27 Dec 30, 2024

nixos/nextcloud: use LoadCredential to read secrets #367433

Are you sure you want to change the base?

nixos/nextcloud: use LoadCredential to read secrets #367433

Conversation

networkException commented Dec 22, 2024 • edited Loading

Things done

networkException commented Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ma27 commented Dec 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

networkException commented Dec 22, 2024 •

edited

Loading

networkException commented Dec 22, 2024 •

edited

Loading