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

nextcloud: restrict web server support to nginx; stop sharing nginx user/group; improve setup service #93584

Merged
merged 7 commits into from Aug 6, 2020

Conversation

@DavHau
Copy link
Contributor

DavHau commented Jul 21, 2020

Things done

(This description is heavily outdated. Plans changed a lot due to the discussions. See below)

  • user and group used by all nextcloud related services are now unified and configurable. If nginx is enabled, the group will be forced to nginx and an error is raised if group is set manually.
  • nginx is enabled by default now. (In favor of having a working default configuration)
  • phpfpm also uses configured nextcloud user + group
  • In the setup service instead of mkdir + chown now install is used to setup the directories.
    This has the disadvantage that the group of nextcloud cannot be changed anymore on an existing installation without having to fix the ownership of files manually. But it think chowning all files on every single startup is bad since it's a very intense operation and can take very long depending on the amount of files stored. Also the chown operation itself might not be allowed depending on the filesystem. I for example use a sshfs mount for my nextcloud files where this fails

fixes #69261
fixes #48881

  • 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 nixpkgs-review --run "nixpkgs-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.
Copy link
Contributor

aanderse left a comment

Here are some initial thoughts I had which might be a starting point for further discussion.

nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved
default = "nextcloud";
description = "User of the nextcloud service";
};
group = mkOption {

This comment has been minimized.

@aanderse

aanderse Jul 21, 2020 Contributor

Another possibility here would be to keep running nextcloud services (including php-fpm) as a non web server user (like the current nextcloud user) and instead add the nextcloud group to your web servers extraGroups option, like so:

mkIf cfg.nginx.enable {
  users.users.nginx.extraGroups = [ cfg.group ]; # in other words "nextcloud"
}

The reason for this is that the nginx user (and group) likely has access to very sensitive files, such as certificates. If nginx can read nextcloud files (which you want) that is much less of a risk than having nextcloud able to read nginx files - a serious security concern. This obviously requires that your nextcloud files are g+r.

This comment has been minimized.

@DavHau

DavHau Jul 22, 2020 Author Contributor

Sounds like a good idea. The files created by nextcloud seem to be g+r, so this shouldn't be an issue.

This comment has been minimized.

@DavHau

DavHau Jul 26, 2020 Author Contributor

I wonder how to best change the user/group of nextcloud without breaking existing installations. It would require a one time service run as root which changes the ownership. And I would need to have some state file to save that this setup has been completed.
Alternatively I could leave the default user.group to be nginx if system.stateVersion <= 20.03, but i don't really like that approach.

This comment has been minimized.

@DavHau

DavHau Jul 27, 2020 Author Contributor

Forget about it. Since it looks like the user will still be nextcloud, only the group needs to be changed from nginx to nextcloud which is easy to do in the current setup. Also no state needs to be stored.

@aanderse aanderse requested a review from Ma27 Jul 21, 2020
@Ma27
Copy link
Member

Ma27 commented Jul 21, 2020

Did anyone of you test what happens when deploying this configuration to an existing Nextcloud instance? Also, what's supposed to happen if cfg.user changes (or someone switches e.g. from nginx to httpd?).

- remove optons cfg.user, cfg.groups
- add option `serverUser` which is required when not using nginx
- add `serverUser` to nextcloud group
- set user/group to "nextcloud" for nextcloud services
- make setup-service non-root
@DavHau
Copy link
Contributor Author

DavHau commented Jul 26, 2020

please check 6ee3004.

  • removed optons cfg.user, cfg.groups
  • add option serverUser which is required when not using nginx
  • add serverUser to nextcloud group
  • set user/group to "nextcloud" for nextcloud services
  • make setup-service non-root

Did not yet implement a solution for fixing group ownership of files for existing installations. Your suggestions are welcome.

@aanderse
Copy link
Contributor

aanderse commented Jul 26, 2020

@DavHau for context how are you trying to run nextcloud? caddy?

@DavHau
Copy link
Contributor Author

DavHau commented Jul 26, 2020

Sorry my description of what I changed was not very precise.

All nextcloud services now use the user and group "nextcloud" (hard-coded). The web server user must therefore be part of the the nextcloud group.

The webserver user can be configured via cfg.serverUser.
This user will be automatically added to the nextcloud group and it will be used for listen.owner of phpfpm. Also its group will automatically be used for listen.group

cfg.serverUserdefaults to the nginx user if nginx is enabled. Otherwise it must be specified manually.

An example configuration using nginx would be:
(if nothing specified nginx will be enabled and its user added to the nextcloud group)

{ pkgs, ... }:
{
  networking.firewall.allowedTCPPorts = [80];
  services.nextcloud = {
    enable = true;
    hostName = "10.233.1.2";
    config.adminpass = "test";
    package = pkgs.nextcloud19;
  };
}

A configuration for an alternative webserver could look like this:

{ pkgs, ... }:
{
  networking.firewall.allowedTCPPorts = [80];
  services.nextcloud = {
    enable = true;
    hostName = "10.233.1.2";
    config.adminpass = "test";
    package = pkgs.nextcloud19;
    
    # disable nginx and set serverUser
    nginx.enable = false;
    serverUser = "caddy";

    # caddy config
    ...
  };
}
@aanderse
Copy link
Contributor

aanderse commented Jul 26, 2020

@DavHau by having a configurable web server user in this module it seems we're just circling back around to #93070.

@Ma27 I suggest we remove the nginx.enable option entirely and just take it as a given: this module is for nginx only.

@DavHau
Copy link
Contributor Author

DavHau commented Jul 27, 2020

  • removed option nextcloud.nginx.enable. Nginx is now the only supported web server.
  • modified nextcloud-setup service to fix group ownership of nextcloud files if they don't match.
    All installations prior to this PR will be fixed by this. (changed from nginx -> nextcloud).
Copy link
Contributor

aanderse left a comment

Looking good 👍

nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved
Co-authored-by: Aaron Andersen <aaron@fosslib.net>
@DavHau DavHau changed the title nextcloud: configurable user and group, enable nginx, improve setup service nextcloud: restrict web server support to nginx; stop sharing nginx user/group; improve setup service Jul 27, 2020
@DavHau DavHau requested a review from aanderse Jul 29, 2020
Copy link
Member

Ma27 left a comment

The implementation seems fine: nginx is in most of the services in our tree that serve HTTP the default choice and now it's possible here to opt-out by disabling it.
Also did a brief test of the altered functionality and it appears fine to me.

However there are a few things I'd like to get resolved before merging this:

  • We should mention this in our release-notes. When having a big library in your nextcloud, the chown will take some time, so admins should be aware of this before deploying the new config.

  • Removed options must be documented with mkRemovedOptionModule.

  • The nextcloud-specific tests don't evaluate because of the removed nginx.enable option.

nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved
@DavHau DavHau force-pushed the DavHau:nextcloud-improvements branch from 753d8a9 to 8516f4f Aug 3, 2020
@DavHau
Copy link
Contributor Author

DavHau commented Aug 3, 2020

Things done:

  • deprecated services.nextcloud.nginx.enable option via mkRemovedOptionModule
  • replaced chown with chgrp
  • fixed tests by removing use of option services.nextcloud.nginx.enable
  • use mkDefault for services.nginx.enable

We should mention this in our release-notes. When having a big library in your nextcloud, the chown will take some time, so admins should be aware of this before deploying the new config.

I don't think this will be necessary. Up until this point the setup service always did a chown an all nextcloud files on each startup. Therefore the new implementation doesn't bring any performance drawbacks compared to the old one.

@DavHau DavHau force-pushed the DavHau:nextcloud-improvements branch from 8516f4f to ca916e8 Aug 3, 2020
@DavHau DavHau requested a review from aanderse Aug 4, 2020
Copy link
Contributor

aanderse left a comment

In general, LGTM 👍 I have not tested and don't use this module, though, so need someone like @Ma27 to approve as well.

Copy link
Member

Ma27 left a comment

LGTM 👍

  • Did anyone test this with live data yet? I just checked whether the "migration" to the new config works fine in a temporary VM network.
  • We should mention this in the release notes. The rollout of the new config can take quite long due to the recursive chgrp.
Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Aug 6, 2020
Derived from NixOS#93584
@Ma27
Ma27 approved these changes Aug 6, 2020
@Ma27 Ma27 merged commit 50d8cdb into NixOS:master Aug 6, 2020
16 checks passed
16 checks passed
tests tests
Details
tests
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="128dbb3"; rev="128dbb31cca3ba479396c6b65946e2e6503c0f8d"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="128dbb3"; rev="128dbb31cca3ba479396c6b65946e2e6503c0f8d"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="128dbb3"; rev="128dbb31cca3ba479396c6b65946e2e6503c0f8d"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="128dbb3"; rev="128dbb31cca3ba479396c6b65946e2e6503c0f8d"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="128dbb3"; rev="128dbb31cca3ba479396c6b65946e2e6503c0f8d"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="128dbb3"; rev="128dbb31cca3ba479396c6b65946e2e6503c0f8d"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="128dbb3"; rev="128dbb31cca3ba479396c6b65946e2e6503c0f8d"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@ajs124
Copy link
Member

ajs124 commented Aug 6, 2020

This broke then manual: log. Plz fix, kthxbye. Or wait for a 10-12h, then I'll probably get around to it myself.

@aszlig
Copy link
Member

aszlig commented Aug 7, 2020

@ajs124: Fixed in 1365b9a.

@DavHau
Copy link
Contributor Author

DavHau commented Aug 7, 2020

Thanks @ajs124 .
@Ma27 I thought we still wanted to improve the phrase The nextcloud module dropped support for other webservers than nginx..
But now the PR has already been merged. Should I make another commit ontop of this PR and repopen it?

@Ma27
Copy link
Member

Ma27 commented Aug 8, 2020

@aszlig @ajs124 sorry for that!
@DavHau yeah feel free to do that :) Otherwise I'll take care of this next week.

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Aug 10, 2020
Follow-up for NixOS#93584[1]. This change adds a simple example how to use
`Nextcloud` with `httpd`.

[1] NixOS#93584 (comment)
Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Aug 11, 2020
Derived from NixOS#93584
@Ma27
Copy link
Member

Ma27 commented Aug 12, 2020

Just for the record, this PR caused another regression in #95259 which got fixed in fddeb7c.

I'm fully aware that I merged this too early which is why I want to openly apologize to everyone who went into trouble because of the (now fixed) regressions I caused with the merge!

wchresta added a commit to wchresta/nixpkgs that referenced this pull request Aug 17, 2020
Follow-up for NixOS#93584[1]. This change adds a simple example how to use
`Nextcloud` with `httpd`.

[1] NixOS#93584 (comment)
evanjs added a commit to evanjs/nixos_cfg that referenced this pull request Aug 19, 2020
The web server for the nextcloud module is now restricted to nginx.
For more information, see: NixOS/nixpkgs#93584
Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Aug 28, 2020
Derived from NixOS#93584
Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Aug 29, 2020
Derived from NixOS#93584
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.