nixos/nextcloud: use LoadCredential to read secrets #367433
Conversation
github-actions
bot
added
6.topic: nixos
networkException
force-pushed
the
nextcloud-loadcredential
branch
from
b15113c to
6587fd2
Compare
github-actions
bot
added
10.rebuild-darwin: 0
|
This should be generally well tested already so reviews are encouraged, undrafting when
|
| # NOTE: The phpfpm runtime directory is currently preserved | ||
| # between restarts. | ||
| rm -rf /run/phpfpm/nextcloud-credentials/ | ||
| mkdir -p /run/phpfpm/nextcloud-credentials/ | ||
| cp $CREDENTIALS_DIRECTORY/* /run/phpfpm/nextcloud-credentials/ |
There was a problem hiding this comment.
As per my comment above the files are not readable by the nextcloud user.
$ pwd
/run/credentials/phpfpm-nextcloud.service
$ ls -lah
total 4.0K
dr-x------ 2 root root 60 Dec 22 20:58 .
drwxr-xr-x 11 root root 220 Dec 22 20:58 ..
-r-------- 1 root root 7 Dec 22 20:58 dbpass
| 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}"); |
There was a problem hiding this comment.
| 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}"; |
| # NOTE: If there are no credentials that are required at runtime then there's no need | ||
| # to load any credentials. |
There was a problem hiding this comment.
IMO we should always execute the script the same way to avoid edge cases and to make testing changes easier.
There was a problem hiding this comment.
If there are no credentials the script would try to use systemd-run from within other services, which fails.
It needs some way of knowing if its running in a service or that it shouldn't expect credentials
There was a problem hiding this comment.
On a first glance I'd be fine with that, but it would be nice to have a test which runs occ from the "outside" to make sure we immediately notice if we're about to break it (that means, a command that fails if secrets are unreadable).
There was a problem hiding this comment.
We have very basic checks for nextcloud-occ everywhere:
nixpkgs/nixos/tests/nextcloud/default.nix
Lines 70 to 71 in 5f81eeb
If you have ideas to expand this let me know
There was a problem hiding this comment.
This is the output of nextcloud-occ status with logging enabled for whenever a file path is being read
function nix_read_secret($file) {
$credential_file = getenv("CREDENTIALS_DIRECTORY") . "/" . $file;
+ error_log($credential_file);
if (!is_readable($credential_file)) {$ nextcloud-occ status
/run/credentials/run-u13.service/dbpass
- installed: true
- version: 30.0.4.1
- versionstring: 30.0.4
- edition:
- maintenance: false
- needsDbUpgrade: false
- productname: Nextcloud
- extendedSupport: false
I've just pushed additional changes that make the script exit with error code 1, the previously thrown RuntimeException would be caught somewhere:
subtest: Ensure nextcloud-occ is working
nextcloud: must succeed: nextcloud-occ status
nextcloud # [ 23.857755] systemd[1]: Starting /nix/store/b1wvkjx96i3s7wblz38ya0zr8i93zbc5-coreutils-9.5/bin/env NEXTCLOUD_CONFIG_DIR=/var/lib/nextcloud/config /nix/store/c6i258wgrwgg8aladc0v97k39cs2sgp6-php-with-extensions-8.3.15/bin/php occ status...
nextcloud # [ 23.897224] systemd[1]: Started /nix/store/b1wvkjx96i3s7wblz38ya0zr8i93zbc5-coreutils-9.5/bin/env NEXTCLOUD_CONFIG_DIR=/var/lib/nextcloud/config /nix/store/c6i258wgrwgg8aladc0v97k39cs2sgp6-php-with-extensions-8.3.15/bin/php occ status.
nextcloud # [ 23.984784] nextcloud-update-db-start[1148]: oc_filecache table updated successfully.
nextcloud # [ 23.985753] nextcloud-update-db-start[1148]: Adding additional systag_by_objectid index to the oc_systemtag_object_mapping table, this can take some time...
nextcloud # [ 24.110756] systemd[1]: run-u4.service: Main process exited, code=exited, status=1/FAILURE
nextcloud # [ 24.112105] systemd[1]: run-u4.service: Failed with result 'exit-code'.
nextcloud: output: Cannot read credential 'dbpass' passed by NixOS, $CREDENTIALS_DIRECTORY is not set!
Test "Ensure nextcloud-occ is working" failed with error: "command `nextcloud-occ status` failed (exit code 1)"
| 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; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Thanks! |
…-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
networkException
force-pushed
the
nextcloud-loadcredential
branch
from
9ed61f1 to
6c60763
Compare
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]>
networkException
force-pushed
the
nextcloud-loadcredential
branch
from
6c60763 to
6fece8a
Compare
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.
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-occwrapper script has been adjusted to usesystemd-runfor loading credentials when being called from outside a service.See the individual commits for more details.
Depends on #365442 (currently included in this branch)
Continuation of #152141
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.