nixos/transmission: fixes #258793

[ju1m]

Description of changes

This should fix #258793 (not tested by me).

An alternative would be to only BindReadOnlyPaths="/run/systemd/resolve" when services.resolved.enable == true, but IMHO it's too much a special case, for too little added security (/run/* is still restricted through Unix perms).

Things done

• Use BindPaths= instead of BindReadOnlyPaths= for /run. Transmission may need to read in the host's /run (eg. /run/systemd/resolve) or write in its private /run (eg. /run/host).
• Added mkDefaults to ease the custom settings of a user reporting they had to disable PrivateMounts= and PrivateUsers=.

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[rnhmjoj]

An alternative would be to only BindReadOnlyPaths="/run/systemd/resolve" when services.resolved.enable == true, but IMHO it's too much a special case

Makes sense, that also doesn't scale when lots of modules are interacting with each other.

[puffnfresh]

Haven't tested but seems like a reasonable solution.

[rnhmjoj]

@GrahamcOfBorg test transmission bittorrent

[SuperSandro2000]

This is not required, people can use lib.mkForce to overwrite any of those values

[pbsds]

It makes for a more friendly user interface though. IMO the nixos modules should be considered opinionated defaults at best

[ju1m]

Yes, that's my rationale here. mkDefault gives a hint that a user may want to change them, but that security is by default (as openFirewall defaults to false for almost every service).

[eyJhb]

It still hides it away from the user, and they actively have to find the service file to discover this + it's not even sure it makes a difference for their setup. It's quite a small hint.

[eyJhb]

I'm not sure this PR will change much, as having to use lib.mkForce is fine. The thing that have been bothering me, is the massive amounts of systemd parameters that have been added to the service, in an attempt to satisfy the systemd security analysis (among other things). If anything should be changed, all the hardening parameters should be behind a option, which could be enabled by default.

Which as far as I know, is a issue that should be dealt with somewhere else, and more broadly for nixpkgs itself. :)

[ju1m]

I'm not sure this PR will change much, as having to use lib.mkForce is fine.

mkForce is a big hammer, it does not help to see conflicting values being set for booleans / ints, nor lists being discarded instead of merged. But most of all, I see the use of mkDefault as a hint that the user might want to tweak such setting.

The thing that have been bothering me, is the massive amounts of systemd parameters that have been added to the service, in an attempt to satisfy the systemd security analysis (among other things). If anything should be changed, all the hardening parameters should be behind a option, which could be enabled by default.

I am the main culprit for pushing those hardening settings, I am sorry it bothers you.
Below are bits of context and rationale I can give you.

There was almost no other hardening example to follow in Nixpkgs at the time, so I did what I thought was best to provide a more secure transmission for everyone, a program widely used and extremely exposed to attacks. Including making Apparmor more functional into NixOS which was quite hard to get merged.

To my mind, enabling hardening by default is best achieved by using mkDefault on options that the user might want to change, or requiring to use mkForce on options that the user should not want to change outside very rare use cases. An option to switch on or off all those security options would be too extreme for me.

Maybe some lower level options could be removed to let the systemd logic apply them or not depending on the setting of more high-level options, but this is a huge cognitive overhead and a burden to check when changing them, so I prefer to set them all as shown by the best systemd-analyze we can come up with.

Which as far as I know, is a issue that should be dealt with somewhere else, and more broadly for nixpkgs itself. :)

See for instance #270637
A few years ago there was also a NixOS PR or such trying to define some levels or profiles of hardening, which to my mind (someone knowing systemd options) brought more confusion than anything and is better resolved by systemd.exec's "This option implies ..." documentation, but maybe this approach will be tried again and reach some level of adoption.
I should also mention my failed attempt to build some consensus around a guiding policy for systemd-confinement: https://discourse.nixos.org/t/nixos-policy-regarding-systemd-confinement/18976

[eyJhb]

Sadly I can't add much to your reply, the reasoning makes sense, but at the same time the comment from #270637, is what I lean towards.

[...] These hardening options tend to skew towards making the services barely useable, thanks to systemd-analyze. Instead of #267319 (comment). [...]

I don't think systemd-analyze is the golden standard, or should be used for these cases. And using lib.mkDefault still hides all the hardening away from the end user, where they'll not know they've stumbled upon a service, which fits the comment from #270637 .

Having a single option which is something like services.transmission.enableSystemdHardening, would make it more visible to the user, that hardening is enabled, and would make it much easier to disable, instead of having to throw in 20+ statements, and find their default values. Ie. that it's not able to run in a nixos container, with no real way to just disable the hardening is rather annoying.

That said, I appreciate all the effort you've put into it, I'm just questioning if systemd-analyze should be followed, and if there should be a "turn all this off" flag.

[ju1m]

Having a single option which is something like services.transmission.enableSystemdHardening, would make it more visible to the user, that hardening is enabled

Tools to help for that:

• systemd-analyze security (without a parameter) is a first tool.
• https://discourse.nixos.org/t/systemd-sandboxing-audit/20581

and would make it much easier to disable, instead of having to throw in 20+ statements, and find their default values.

Since such global option is considered useful by you, maybe you could propose it in another PR, I guess it could be made a enableHardening or rather allowHardening option in serviceOptions to be used in a mkIf guard in each service instead of just gathering the settings after a "hardening" comment. Either by slow adoption or by a treewide change. But again I think that « hardening » in itself may be a concept too vague to be useful.

Ie. that it's not able to run in a nixos container, with no real way to just disable the hardening is rather annoying.

I do agree and hope that services.transmission should work in a container, this is the main goal of this PR to fix that. The service was proven to be too much hardened, this PR relaxes it. To resolve the tension between security and usability I chose to first starting with an educated guess for the strongest working security settings, and then relaxing specific part of it when it has brought justified problems into light, like here in the container case. But it's hard to guess all problems in advance. Thanks all of you for your understanding.

[doronbehar]

I'm merging this, as the arguments presented for this change have satisfied me as well, and the debate has stopped long ago.

[eyJhb]

I still don't think it makes any sense. Now we have mkDefault with no explanation on when it makes sense to set it to false, unless this PR or the other issue is found, which makes the changes mute.