nixos/radicale: add settings option #120440

dotlambda · 2021-04-23T18:33:27Z

Motivation for this change

NixOS/rfcs#42

Things done

I'm looking for comments on my approach.
The following points need to be solved:

nixosTests.radicale is broken cause it uses /tmp
This is a breaking change for people who use a non-default filesystem_folder. Is documenting that change sufficient? Should we add an option to add paths to ReadWritePaths?
document breaking changes
Should we merge the two test into one, remove the old one, or keep both files?

nixos/modules/services/networking/radicale.nix

aanderse

Just a few comments to think on. Also, are the old versions of radicale still supported upstream?

aanderse · 2021-04-24T02:55:32Z

nixos/modules/services/networking/radicale.nix

@@ -43,7 +54,7 @@ in
      '';
    };

-    services.radicale.config = mkOption {
+    config = mkOption {


Is this still needed? Presumably for the older versions of radicale? If so, can set add a comment about when you would use which option and how they are mutually exclusive. Maybe an assertion.

settings should also work with older versions, but I kept config for backwards compatibility with existing configuration.nixs

The problem with this is that third-party modules can't rely on settings. Every module that wants to read or write settings options has the chance to not work in case the user doesn't use settings. And it's even worse: A third-party module writing to settings makes the settings == {} check fail, meaning the users config option gets ignored! Because of this I think it might be better to actually remove this option.

And it's even worse: A third-party module writing to settings makes the settings == {} check fail, meaning the users config option gets ignored! Because of this I think it might be better to actually remove this option.

That's not true: If both config and settings are set an assertion is triggerred.
Nevertheless, I'm all in favor of removing config but it might make sense to first mark it as deprecated using lib.warn.

aanderse · 2021-04-24T02:57:22Z

nixos/modules/services/networking/radicale.nix

@@ -84,9 +150,25 @@ in
        ));
        User = "radicale";
        Group = "radicale";
+        StateDirectory = "radicale/collections";


Link to upstream documentation with comment so we can track changes would be useful. Additionally, will these hardening options break older versions of radicale?

They will only break configurations where data is stored at a non-standard location.

nixos/modules/services/networking/radicale.nix

aanderse · 2021-04-24T03:00:01Z

nixos/modules/services/networking/radicale.nix

  cfg = config.services.radicale;

-  confFile = pkgs.writeText "radicale.conf" cfg.config;
+  format = let


Doesn't the ini format work here? That would be much easier to read...

No, that's why I had to create my own. The IP addresses Radicale listens on have to be given as a comma-separated list: https://radicale.org/3.0.html#tutorials/basic-configuration/addresses. I thought requiring users to specify e.g. hosts = "0.0.0.0:5232, [::]:5232" is a little unnatural.

Do we have a (unit) test to ensure our formatter is doing what we expect it to do?

Yes, there is nixos/tests/radicale3.nix.

No no I mean: can we actually format the IPs as we claim? The nixos (integration) test just covers one case as far as I can see. I would love to have a few tests. One case for each of the permissible input values.

And essentially, I'm not doing anything except for replacing lists of strings with comma-separated strings and then I use lib.generators.toINI, which is tested in lib/tests/misc.nix.

No no I mean: can we actually format the IPs as we claim? The nixos (integration) test just covers one case as far as I can see. I would love to have a few tests. One case for each of the permissible input values.

Wouldn't we have to put the generator in lib/ for that? I honestly don't think unit tests are needed for such a simple thing as concatStringsSep ", ".

I extended formats.ini to support this with #121613

Thanks a lot!

dotlambda · 2021-04-24T05:47:17Z

@SuperSandro2000 I did not ping @andir without a reason: I know he uses Radicale: https://github.com/andir/infra/tree/master/config/servers/mail/radicale

SuperSandro2000 · 2021-04-24T06:20:13Z

@SuperSandro2000 I did not ping @andir without a reason: I know he uses Radicale: https://github.com/andir/infra/tree/master/config/servers/mail/radicale

Thats not my fault. He blocked me and when I edit the reviewer list he gets removed.

dotlambda · 2021-04-24T06:20:15Z

Also, are the old versions of radicale still supported upstream?

No, they are not. But using a newer version requires changes to the configuration (and the data format is different between version 1 and the others), so we keep them for old system.stateVersions. I don't know of any vulnerabilities in the old versions.
If we want to get rid of this behaviour, we could require that people set their own package unless the system.stateVersion is new enough.

nixos/modules/services/networking/radicale.nix

andir · 2021-04-24T09:22:37Z

nixos/modules/services/networking/radicale.nix

  cfg = config.services.radicale;

-  confFile = pkgs.writeText "radicale.conf" cfg.config;
+  format = let


Do we have a (unit) test to ensure our formatter is doing what we expect it to do?

andir · 2021-04-24T09:25:10Z

nixos/modules/services/networking/radicale.nix

+      type = "from_file";
+      file = toString rightsFile;
+    };
+
    environment.systemPackages = [ cfg.package ];

    users.users.radicale =
      { uid = config.ids.uids.radicale;
        description = "radicale user";
        home = "/var/lib/radicale";


I think this requires a changelog entry so an admin can put special attention towards this change. It is subtile as the service might start but the data might no longer be "there" if we forgot some case.

Yes, see the list of todos at the very top. I will add a changelog entry once we decided on what the changes will be.

dotlambda · 2021-05-01T10:53:07Z

I applied some more hardening options to the systemd unit. Maybe @mweinelt who's been doing that a lot lately wants to have a look?

dotlambda · 2021-05-01T10:55:57Z

Also, are the old versions of radicale still supported upstream?

No, they are not. But using a newer version requires changes to the configuration (and the data format is different between version 1 and the others), so we keep them for old system.stateVersions. I don't know of any vulnerabilities in the old versions.
If we want to get rid of this behaviour, we could require that people set their own package unless the system.stateVersion is new enough.

I suggest the following: We remove the old package versions from Nixpkgs since keeping them gives a sense of them still being supported. In the NixOS service, we add an assertion that checks whether package is set by hand in case system.stateVersion is not recent enough.

mweinelt · 2021-05-01T11:05:41Z

I applied some more hardening options to the systemd unit. Maybe @mweinelt who's been doing that a lot lately wants to have a look?

I usually add systemd-analyze to the nixos test, so I can revisit the results quickly.

diff --git a/nixos/tests/radicale3.nix b/nixos/tests/radicale3.nix
index cda43b6c06c..0fedaeea705 100644
--- a/nixos/tests/radicale3.nix
+++ b/nixos/tests/radicale3.nix
@@ -74,5 +74,7 @@ in {
 
     with subtest("Test web interface"):
         machine.succeed("curl --fail http://${user}:${password}@localhost:${port}/.web/")
+
+    machine.log(machine.succeed("systemd-analyze security radicale.service"))
   '';
 })

Other than that hardening means you better know what interfaces radicale needs, which I do not.

✗ PrivateNetwork=                    Service has access to the host's …      0.5
✗ RestrictAddressFamilies=~AF_(INET… Service may allocate Internet soc…      0.3
✗ DeviceAllow=                       Service has a device ACL with som…      0.1
✗ IPAddressDeny=                     Service does not define an IP add…      0.2
✗ ProtectProc=                       Service has full access to proces…      0.2
✗ SystemCallFilter=~@privileged      System call allow list defined fo…      0.2
✗ RootDirectory=/RootImage=          Service runs within the host's ro…      0.1
✗ UMask=                             Files created by service are grou…      0.1
✗ ProcSubset=                        Service has full access to non-pr…      0.1

→ Overall exposure level for radicale.service: 1.4 OK :-)

Can't lock down network things further, but DevicePolicy=strict might make sense. Other than that UMask and the procfs stuff looks easy.

dotlambda · 2021-05-01T11:18:19Z

DevicePolicy=strict might make sense

Doesn't work. Radicale definitely needs /dev/random or something.

mweinelt · 2021-05-01T11:19:32Z

DevicePolicy=strict might make sense

Doesn't work. Radicale definitely needs /dev/random or something.

Then you can add that to DeviceAllow= 😀

dotlambda · 2021-05-01T12:22:39Z

DevicePolicy=strict might make sense

Doesn't work. Radicale definitely needs /dev/random or something.

Then you can add that to DeviceAllow=

That's what I did. Turns out is uses syscalls so no dev/urandom needed.
However, I'm surprised that /dev/null is not needed (and I even added a hook to the test): https://github.com/Kozea/Radicale/blob/3.0.6/radicale/storage/multifilesystem/lock.py#L60-L62

dotlambda

The hardening is even more restrictive now, but I think NixOS modules are not supposed to be usable for everyone, they just provide an opinionated default.

dotlambda · 2021-05-01T12:23:40Z

nixos/modules/services/networking/radicale.nix

@@ -84,9 +161,43 @@ in
        ));
        User = "radicale";
        Group = "radicale";
+        StateDirectory = "radicale/collections";
+        # Hardening
+        CapabilityBoundingSet = "";


This needs CAP_NET_BIND_SERVICE if you want to use a port < 1024. How should we handle that?

Best case: Socket activation, if supported.
Second best: Only pass it in if port < 1024.

I don't think python actively requests capabilities, so you'd need to put it into AmbientCapabilities as well.

Best case: Socket activation, if supported.

not yet: Kozea/Radicale#1141

Second best: Only pass it in if port < 1024.

Then we need a new service option because we shouldn't parse settings.server.hosts.
We can make it dependent on bindLocalhost.

Actually, the current implementation doesn't have the permissions to bind to ports < 1024, so they should also be given by hand in the future.

dotlambda · 2021-05-01T12:25:31Z

nixos/modules/services/networking/radicale.nix

+        IPAddressAllow = mkIf bindLocalhost "localhost";
+        IPAddressDeny = mkIf bindLocalhost "any";


Is this problematic? One scenario I can think of is a hook accessing the (non-local) network. Does anyone do that?

I wouldn't feel comfortable putting a hostname in there, but maybe that's just the networker in me speaking. But making it contingent on bindLocalhost sounds like a sensible approach.

It's not actually a hostname, it's a special name interpreted as 127.0.0.0/8 ::1/128 by systemd: https://www.freedesktop.org/software/systemd/man/systemd.resource-control.html#IPAddressAllow=ADDRESS%5B/PREFIXLENGTH%5D%E2%80%A6

SuperSandro2000 · 2021-05-02T07:22:02Z

The hardening is even more restrictive now, but I think NixOS modules are not supposed to be usable for everyone, they just provide an opinionated default.

They should create a good basis to use the package and should be applicable to a or the most common use case(s). It is okay if they don't work for every possible use case but they should supply things like extraConfig to make it easier to adapt them to my use case.

dotlambda · 2021-05-02T08:20:25Z

I suggest the following: We remove the old package versions from Nixpkgs since keeping them gives a sense of them still being supported. In the NixOS service, we add an assertion that checks whether package is set by hand in case system.stateVersion is not recent enough.

I would really appreciate some feedback on this.

dotlambda · 2021-05-03T14:47:54Z

I suggest the following: We remove the old package versions from Nixpkgs since keeping them gives a sense of them still being supported. In the NixOS service, we add an assertion that checks whether package is set by hand in case system.stateVersion is not recent enough.

Instead of an assertion, we might just want to use lib.warn and link to https://github.com/Kozea/Radicale/blob/3.0.6/NEWS.md#upgrade-checklist and https://radicale.org/2.1.html#documentation/migration-from-1xx-to-2xx.

infinisil · 2021-05-03T14:56:56Z

You can use the warnings option to define a warning when e.g. options.services.radicale.config.isDefined is set

nixos/modules/services/networking/radicale.nix

nixos/tests/radicale.nix

The radicale version is no longer chosen automatically based on system.stateVersion because that gave the impression that old versions are still supported.

dotlambda · 2021-05-04T08:24:01Z

From my point of view, this is ready now. Please test whether the hardening breaks your deployments (I documented the most common case in the release notes). Let me know if you think anything needs more documentation.

nixos/modules/services/networking/radicale.nix

dotlambda · 2021-05-08T11:11:37Z

I will merge this soon unless someone complains or prefers that we call the new option config as well and decide which one it is based on the type.

dotlambda added the 9.needs: community feedback label Apr 23, 2021

dotlambda requested review from andir and infinisil April 23, 2021 18:33

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 Apr 23, 2021

dotlambda force-pushed the radicale-settings branch from c56d28c to 0fbed8c Compare April 23, 2021 18:34

dotlambda commented Apr 23, 2021

View reviewed changes

nixos/modules/services/networking/radicale.nix Outdated Show resolved Hide resolved

nixos/modules/services/networking/radicale.nix Outdated Show resolved Hide resolved

ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 labels Apr 23, 2021

SuperSandro2000 requested review from aanderse and removed request for andir April 24, 2021 01:10

aanderse reviewed Apr 24, 2021

View reviewed changes

dotlambda requested a review from andir April 24, 2021 06:17

dotlambda force-pushed the radicale-settings branch 2 times, most recently from 80cebea to 8d3ed4a Compare April 24, 2021 07:42

andir reviewed Apr 24, 2021

View reviewed changes

dotlambda force-pushed the radicale-settings branch 3 times, most recently from e4798a9 to aba0248 Compare May 1, 2021 10:52

dotlambda force-pushed the radicale-settings branch 2 times, most recently from 1807f71 to f6d0cba Compare May 1, 2021 12:19

dotlambda commented May 1, 2021

View reviewed changes

dotlambda force-pushed the radicale-settings branch from f6d0cba to b529613 Compare May 2, 2021 08:31

dotlambda force-pushed the radicale-settings branch from b529613 to f8d41d5 Compare May 3, 2021 16:57

github-actions bot added 8.has: changelog 8.has: documentation This PR adds or changes documentation labels May 3, 2021

dotlambda force-pushed the radicale-settings branch from f8d41d5 to 6ffacb3 Compare May 3, 2021 16:59

dotlambda marked this pull request as ready for review May 3, 2021 17:13

dotlambda force-pushed the radicale-settings branch from 6ffacb3 to 0ad341e Compare May 3, 2021 17:17

infinisil mentioned this pull request May 3, 2021

formats.ini: Introduce listToValue argument #121613

Merged

3 tasks

dotlambda commented May 3, 2021

View reviewed changes

nixos/modules/services/networking/radicale.nix Outdated Show resolved Hide resolved

dotlambda commented May 3, 2021

View reviewed changes

nixos/tests/radicale.nix Show resolved Hide resolved

calendar-cli: init at 0.12.0

870b3a3

dotlambda force-pushed the radicale-settings branch from 0ad341e to fd8cc3d Compare May 4, 2021 08:06

nixos/radicale: add settings option

022c5b0

The radicale version is no longer chosen automatically based on system.stateVersion because that gave the impression that old versions are still supported.

dotlambda force-pushed the radicale-settings branch from fd8cc3d to ec10438 Compare May 4, 2021 08:22

nixos/radicale: harden systemd unit

762be5c

dotlambda force-pushed the radicale-settings branch from ec10438 to 762be5c Compare May 4, 2021 15:43

dotlambda commented May 4, 2021

View reviewed changes

nixos/modules/services/networking/radicale.nix Show resolved Hide resolved

dotlambda mentioned this pull request May 12, 2021

21.05 Feature Freeze #121972

Closed

dotlambda merged commit e611d66 into NixOS:master May 14, 2021

dotlambda deleted the radicale-settings branch May 14, 2021 13:37

		IPAddressAllow = mkIf bindLocalhost "localhost";
		IPAddressDeny = mkIf bindLocalhost "any";

nixos/radicale: add settings option #120440

nixos/radicale: add settings option #120440

Conversation

dotlambda commented Apr 23, 2021 • edited Loading

Motivation for this change

Things done

aanderse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotlambda Apr 24, 2021 • 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

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

infinisil May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotlambda commented Apr 24, 2021

SuperSandro2000 commented Apr 24, 2021 • edited Loading

dotlambda commented Apr 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotlambda Apr 24, 2021 • edited Loading

Choose a reason for hiding this comment

dotlambda commented May 1, 2021

dotlambda commented May 1, 2021

mweinelt commented May 1, 2021 • edited Loading

dotlambda commented May 1, 2021

mweinelt commented May 1, 2021

dotlambda commented May 1, 2021

dotlambda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotlambda May 1, 2021 • 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

SuperSandro2000 commented May 2, 2021

dotlambda commented May 2, 2021

dotlambda commented May 3, 2021

infinisil commented May 3, 2021

dotlambda commented May 4, 2021

dotlambda commented May 8, 2021

dotlambda commented Apr 23, 2021 •

edited

Loading

dotlambda Apr 24, 2021 •

edited

Loading

infinisil May 3, 2021 •

edited

Loading

SuperSandro2000 commented Apr 24, 2021 •

edited

Loading

dotlambda commented Apr 24, 2021 •

edited

Loading

dotlambda Apr 24, 2021 •

edited

Loading

mweinelt commented May 1, 2021 •

edited

Loading

dotlambda May 1, 2021 •

edited

Loading