nixos/transmission: improvements #350085

diniamo · 2024-10-20T19:04:23Z

As per Transmission's documentation, the umask option should be a string.

Things done

Add a 👍 reaction to pull requests you find important.

nixos/modules/services/torrent/transmission.nix

diniamo · 2024-10-22T21:21:23Z

@ju1m could you help with this PR?

ju1m · 2024-10-23T11:29:13Z

@diniamo thanks for spotting this mistake, type = with types; either str int could be used for backward compatibility, and a warning can be added to issue a deprecation message of int for the next stable version (nixos-25.05) when typeOf cfg.settings.umask == "string".

diniamo · 2024-10-23T15:40:32Z

Does that mean I should remove the release note entry?

ju1m · 2024-10-23T17:10:14Z

@diniamo , sorry I did not notice you had already done all that work, what you did is better, thanks!

ju1m

LGTM

diniamo · 2024-10-23T19:24:51Z

@ju1m just to make sure: I changed serviceConfig.StateDirectoryMode to cfg.downloadDirPermissions as well. It's needed because other things can't access any subdirectories, if they can't access the parent directory. Alternatively, do you have a better solution?

ju1m · 2024-10-23T21:02:09Z

@ju1m just to make sure: I changed serviceConfig.StateDirectoryMode to cfg.downloadDirPermissions as well. It's needed because other things can't access any subdirectories, if they can't access the parent directory. Alternatively, do you have a better solution?

Right, no I would not change StateDirectoryMode= because the better way security-wise is to add users to services.transmission.group, or to set cfg.settings.download-dir & co. to some path where intended users have access to them (since they are bind-mounted into the RootDirectory= using BindPaths=, transmission will be able to access them).

Besides, if cfg.downloadDirPermissions is null (the default) then StateDirectoryMode= would default to 755 instead of the current 750 which would open transmission's state folder read-only to every users. Sure ${cfg.home}/${settingsDir}/settings.json is forced to 600, but yet, AFAICS it's not necessary to relax the 750, just use services.transmission.group, which is allowed read-only.

ju1m · 2024-10-23T21:04:03Z

nixos/modules/services/torrent/transmission.nix

@@ -358,7 +356,7 @@ in
          "transmission/${downloadsDir}"
          "transmission/${watchDir}"
        ];
-        StateDirectoryMode = mkDefault 750;
+        StateDirectoryMode = cfg.downloadDirPermissions;


Please keep 750 and let users add themselves to the transmission group.

diniamo · 2024-10-24T05:43:52Z

How do I add a user to multiple groups declaratively though?

Edit: seems like removing the downloadDirPermissions option worked. Although what happens if you set it to null is not documented.

ju1m · 2024-10-24T10:19:07Z

How do I add a user to multiple groups declaratively though?

users.users."diniamo".extraGroups = [ config.transmission.group ];

Or:

users.groups.${config.transmission.group}.members = [ "diniamo" ];

diniamo · 2024-10-24T13:18:28Z

And do you know why setting downloadDirPermissions to null makes it 755? I'll add a note about that to the description.

ju1m · 2024-10-24T17:05:40Z

And do you know why setting downloadDirPermissions to null makes it 755? I'll add a note about that to the description.

Yes, I explained it in my previous message. But please do not use StateDirectoryMode = cfg.downloadDirPermissions; there's no need to relax 750 since user groups or bind-mounts are a common and more secure way to give access to directories.

diniamo · 2024-10-25T11:10:52Z

How is this?

ju1m · 2024-10-25T21:42:19Z

nixos/modules/services/torrent/transmission.nix

@@ -191,6 +189,10 @@ in
          and [](#opt-services.transmission.settings.watch-dir).
          Note that you may also want to change
          [](#opt-services.transmission.settings.umask).
+
+          If `null`, the home and the download directories become


Hmm, I think you've misunderstood what I tried to say previously, when I wrote:

Besides, if cfg.downloadDirPermissions is null (the default) then StateDirectoryMode= would default to 755 instead of the current 750 which would open transmission's state folder read-only to every users.

I was describing what would happen if we were to use StateDirectoryMode = cfg.downloadDirPermissions; but that's no longer the case, so downloadDirPermissions == null remains a no-op.

Indeed, the description only documents the If not null, case, but adding users to config.transmission.group is unrelated to that option being null or not, it's related to StateDirectoryMode == "750".

Then that option is very misleading. If I set it to 755, then I expect other users to be able to read the download directories, which is not the case. This is very unintuitive, and the option descriptions don't mention this anywhere. It took a long while before I figured out what was going on.

It would be nice if we could avoid wasting others' time. What do you suggest?

IMHO, downloadDirPermissions is a quirk that should not even exist in the first place, since adding users to services.transmission.group is both more secure and more idiomatic.

Yet this option lingers and we have to deal with it, I would suggest to improve its description by explaining the two main ways to give access to the directories that I explained previously:

the better way security-wise is to add users to services.transmission.group,

or to set cfg.settings.download-dir & co. to some path where intended users have access to them [and use settings.umask = "002" and downloadDirPermissions = "755"]

diniamo · 2024-10-26T07:03:10Z

How is this?

I did some minimal testing in a VM, and it seems to be working fine.

ju1m · 2024-10-26T11:29:39Z

nixos/modules/services/torrent/transmission.nix

-          "transmission/${downloadsDir}"
-          "transmission/${watchDir}"
-        ];
-        StateDirectoryMode = mkDefault 750;


Please do not remove StateDirectoryMode nor StateDirectory, otherwise they will not be bound in the RootDirectory.

~~I don't exactly understand the bind mount stuff from the systemd docs. Isn't it enough to only bind the top-level directory (/var/lib/transmission)?~~

Never mind. I'm not digging into this deeper than I have to.

yes, but listing subdirs enforces StateDirectoryMode on them:

Except in case of ConfigurationDirectory=, the innermost specified directories will be owned by the user and group specified in User= and Group=. If the specified directories already exist and their owning user or group do not match the configured ones, all files and directories below the specified directories as well as the directories themselves will have their file ownership recursively changed to match what is configured. As an optimization, if the specified directories are already owned by the right user and group, files and directories below of them are left as-is, even if they do not match what is requested. The innermost specified directories will have their access mode adjusted to the what is specified in RuntimeDirectoryMode=, StateDirectoryMode=, CacheDirectoryMode=, LogsDirectoryMode= and ConfigurationDirectoryMode=.

Note that because "transmission/${downloadsDir}" is necessarily /var/lib/transmission/Downloads, StateDirectoryMode does not adjust modes of cfg.settings.download-dir if it's set to something else than the default "${cfg.home}/${downloadsDir}" (cfg.home being another quirk that lingers).

I see. Well IMO this entire module deserves a rewrite, but that's a lot of effort, so the latest commit should be fine.

Yes, if it were not for backward compatibility we should drop cfg.home to always use /var/lib/transmission, and drop downloadDirPermissions to let systemd create directories with "750" (or "770" if members of the transmission group should be allowed to delete dangling torrent files that transmission did not delete).

Except for that, the rest of the module is not that bad (obviously I'm biased),
amongst the things that come to mind that can be improved security-wise:

With recent systemd giving access to LoadCredential=/LoadCredentialEncrypted= in ExecStartPre=, those should be used instead of directly using ${cfg.credentialsFile}, as in

nixpkgs/nixos/modules/services/misc/radicle.nix

Lines 272 to 282 in 072f2ee

let keyCred = builtins.split ":" "${cfg.privateKeyFile}"; in

if lib.length keyCred > 1

then {

LoadCredentialEncrypted = [ cfg.privateKeyFile ];

# Note that neither %d nor ${CREDENTIALS_DIRECTORY} works in BindReadOnlyPaths=

BindReadOnlyPaths = [ "/run/credentials/radicle-node.service/${lib.head keyCred}:${env.RAD_HOME}/keys/radicle" ];

}

else {

LoadCredential = [ "radicle:${cfg.privateKeyFile}" ];

BindReadOnlyPaths = [ "/run/credentials/radicle-node.service/radicle:${env.RAD_HOME}/keys/radicle" ];

};

Instead of using BindReadOnlyPaths = [ builtins.storeDir ], systemd.services.transmission.confinement should be used to only mount transmission's closure dependencies, as in

nixpkgs/nixos/modules/services/misc/radicle.nix

Lines 102 to 112 in 072f2ee

confinement = {

enable = true;

mode = "full-apivfs";

packages = [

pkgs.gitMinimal

cfg.package

pkgs.iana-etc

(lib.getLib pkgs.nss)

pkgs.tzdata

];

};

diniamo · 2024-10-26T13:46:55Z

Some more stuff, since we are dragging this out anyway.

diniamo · 2024-10-27T13:39:12Z

@ju1m sorry for this taking so long, but could you take a look?

Aleksanaa · 2024-10-28T14:49:48Z

We have to wait until after NixOS 24.11 as this is a breaking change and not a trivial fix.

diniamo · 2024-10-28T16:14:55Z

I don't get it. Why do we have to wait for 24.11?

Aleksanaa · 2024-10-28T16:23:50Z

Because breaking changes are blocked in this period to prepare for the next release. See #339153

diniamo · 2024-10-29T10:21:52Z

@ju1m it looks like I didn't do enough research: transmission 3 (which is the default for the module) still uses the old way of configuring umask (decimal number), while transmission 4 uses the new way (octal number as a string). Should I update the default package as well, or?

ambroisie · 2024-10-29T12:33:07Z

@diniamo should probably be using type = with lib.types; either str int; (which would make it a non-breaking change) to accommodate both versions.

EDIT: maybe adjusting the default to be 2 or "002" depending of the version of cfg.package would be good too.

diniamo · 2024-10-29T12:38:41Z

I was thinking of something like that as well. Thnaks for that suggestion, I'll implement it.

ju1m · 2024-10-29T13:15:50Z

@ju1m it looks like I didn't do enough research: transmission 3 (which is the default for the module) still uses the old way of configuring umask (decimal number), while transmission 4 uses the new way (octal number as a string). Should I update the default package as well, or?

@diniamo, chances are that nixos-25.05 (first NixOS stable release on which this PR will land) may indeed switch to transmission_4 as default for services.transmission, which is currently blocked to transmission_3 for nixos-24.11:

nixpkgs/nixos/doc/manual/release-notes/rl-2411.section.md

Lines 210 to 212 in b166978

    
           - `transmission` package has been aliased with a `trace` warning to `transmission_3`. Since [Transmission 4 has been released last year](https://github.com/transmission/transmission/releases/tag/4.0.0), and Transmission 3 will eventually go away, it was decided perform this warning alias to make people aware of the new version. The `services.transmission.package` defaults to `transmission_3` as well because the upgrade can cause data loss in certain specific usage patterns (examples: [#5153](https://github.com/transmission/transmission/issues/5153), [#6796](https://github.com/transmission/transmission/issues/6796)). Please make sure to back up to your data directory per your usage: 
        
             - `transmission-gtk`: `~/.config/transmission` 
        
             - `transmission-daemon` using NixOS module: `${config.services.transmission.home}/.config/transmission-daemon` (defaults to `/var/lib/transmission/.config/transmission-daemon`)

But that would have to be discussed with people involved in #258058

nixos/transmission: improve code - Remove `with lib;` - Use `{ name = { ... }; }` instead of `{ name.foo = ...; name.bar = ...; }`

diniamo · 2024-10-29T14:49:32Z

In that case, I'll leave updating the default to them, and this should be fine.

diniamo · 2024-10-29T21:45:46Z

@Aleksanaa this is no longer a breaking change.

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 Oct 20, 2024

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Oct 20, 2024

Aleksanaa reviewed Oct 21, 2024

View reviewed changes

nixos/modules/services/torrent/transmission.nix Outdated Show resolved Hide resolved

diniamo force-pushed the fix-transmission branch from beb5ec1 to 99add84 Compare October 21, 2024 11:56

github-actions bot added 8.has: documentation 8.has: changelog labels Oct 21, 2024

diniamo changed the title ~~nixos/transmission: fix settings.umask to be of type str~~ nixos/transmission: fix permissions Oct 22, 2024

ju1m self-requested a review October 23, 2024 17:11

ju1m approved these changes Oct 23, 2024

View reviewed changes

ju1m added needs_merger 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Oct 23, 2024

wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 23, 2024

ju1m suggested changes Oct 23, 2024

View reviewed changes

diniamo force-pushed the fix-transmission branch from b0b6952 to 283ce8d Compare October 25, 2024 11:10

ju1m suggested changes Oct 25, 2024

View reviewed changes

diniamo force-pushed the fix-transmission branch from 283ce8d to f7cdf7b Compare October 26, 2024 07:02

diniamo force-pushed the fix-transmission branch from f7cdf7b to 849298b Compare October 26, 2024 11:13

ju1m suggested changes Oct 26, 2024

View reviewed changes

diniamo force-pushed the fix-transmission branch from 849298b to 6895141 Compare October 26, 2024 12:03

ju1m approved these changes Oct 26, 2024

View reviewed changes

diniamo changed the title ~~nixos/transmission: fix permissions~~ nixos/transmission: improvements Oct 26, 2024

ju1m approved these changes Oct 27, 2024

View reviewed changes

wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 28, 2024

Aleksanaa added the 2.status: wait for branch‐off Waiting for the next Nixpkgs branch‐off label Oct 28, 2024

wegank removed the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package label Oct 29, 2024

diniamo added 3 commits October 29, 2024 15:34

nixos/transmission: improve permission handling and description

cebeec1

nixos/transmission: improve code

89320d7

nixos/transmission: improve code - Remove `with lib;` - Use `{ name = { ... }; }` instead of `{ name.foo = ...; name.bar = ...; }`

nixos/transmission: format with nixfmt-rfc-style

76fb46f

diniamo force-pushed the fix-transmission branch from 66ce4b0 to 94a4001 Compare October 29, 2024 14:40

github-actions bot removed 8.has: documentation 8.has: changelog labels Oct 29, 2024

diniamo force-pushed the fix-transmission branch from 94a4001 to 66115dd Compare October 29, 2024 14:43

nixos/transmission: fix the type of settings.umask

38bc6ac

diniamo force-pushed the fix-transmission branch from 66115dd to 38bc6ac Compare October 29, 2024 14:49

ju1m approved these changes Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/transmission: improvements #350085

nixos/transmission: improvements #350085

diniamo commented Oct 20, 2024

diniamo commented Oct 22, 2024

ju1m commented Oct 23, 2024

diniamo commented Oct 23, 2024

ju1m commented Oct 23, 2024

ju1m left a comment

diniamo commented Oct 23, 2024

ju1m commented Oct 23, 2024

ju1m Oct 23, 2024

diniamo commented Oct 24, 2024 •

edited

Loading

ju1m commented Oct 24, 2024

diniamo commented Oct 24, 2024

ju1m commented Oct 24, 2024

diniamo commented Oct 25, 2024

ju1m Oct 25, 2024 •

edited

Loading

diniamo Oct 26, 2024

ju1m Oct 26, 2024 •

edited

Loading

diniamo commented Oct 26, 2024

ju1m Oct 26, 2024

diniamo Oct 26, 2024 •

edited

Loading

ju1m Oct 26, 2024

diniamo Oct 26, 2024 •

edited

Loading

ju1m Oct 26, 2024

diniamo commented Oct 26, 2024

diniamo commented Oct 27, 2024

Aleksanaa commented Oct 28, 2024

diniamo commented Oct 28, 2024 •

edited

Loading

Aleksanaa commented Oct 28, 2024

diniamo commented Oct 29, 2024

ambroisie commented Oct 29, 2024 •

edited

Loading

diniamo commented Oct 29, 2024

ju1m commented Oct 29, 2024

diniamo commented Oct 29, 2024

diniamo commented Oct 29, 2024

	let keyCred = builtins.split ":" "${cfg.privateKeyFile}"; in
	if lib.length keyCred > 1
	then {
	LoadCredentialEncrypted = [ cfg.privateKeyFile ];
	# Note that neither %d nor ${CREDENTIALS_DIRECTORY} works in BindReadOnlyPaths=
	BindReadOnlyPaths = [ "/run/credentials/radicle-node.service/${lib.head keyCred}:${env.RAD_HOME}/keys/radicle" ];
	}
	else {
	LoadCredential = [ "radicle:${cfg.privateKeyFile}" ];
	BindReadOnlyPaths = [ "/run/credentials/radicle-node.service/radicle:${env.RAD_HOME}/keys/radicle" ];
	};

	confinement = {
	enable = true;
	mode = "full-apivfs";
	packages = [
	pkgs.gitMinimal
	cfg.package
	pkgs.iana-etc
	(lib.getLib pkgs.nss)
	pkgs.tzdata
	];
	};

nixos/transmission: improvements #350085

Are you sure you want to change the base?

nixos/transmission: improvements #350085

Conversation

diniamo commented Oct 20, 2024

Things done

diniamo commented Oct 22, 2024

ju1m commented Oct 23, 2024

diniamo commented Oct 23, 2024

ju1m commented Oct 23, 2024

ju1m left a comment

Choose a reason for hiding this comment

diniamo commented Oct 23, 2024

ju1m commented Oct 23, 2024

ju1m Oct 23, 2024

Choose a reason for hiding this comment

diniamo commented Oct 24, 2024 • edited Loading

ju1m commented Oct 24, 2024

diniamo commented Oct 24, 2024

ju1m commented Oct 24, 2024

diniamo commented Oct 25, 2024

ju1m Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

diniamo Oct 26, 2024

Choose a reason for hiding this comment

ju1m Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

diniamo commented Oct 26, 2024

ju1m Oct 26, 2024

Choose a reason for hiding this comment

diniamo Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

ju1m Oct 26, 2024

Choose a reason for hiding this comment

diniamo Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

ju1m Oct 26, 2024

Choose a reason for hiding this comment

diniamo commented Oct 26, 2024

diniamo commented Oct 27, 2024

Aleksanaa commented Oct 28, 2024

diniamo commented Oct 28, 2024 • edited Loading

Aleksanaa commented Oct 28, 2024

diniamo commented Oct 29, 2024

ambroisie commented Oct 29, 2024 • edited Loading

diniamo commented Oct 29, 2024

ju1m commented Oct 29, 2024

diniamo commented Oct 29, 2024

diniamo commented Oct 29, 2024

diniamo commented Oct 24, 2024 •

edited

Loading

ju1m Oct 25, 2024 •

edited

Loading

ju1m Oct 26, 2024 •

edited

Loading

diniamo Oct 26, 2024 •

edited

Loading

diniamo Oct 26, 2024 •

edited

Loading

diniamo commented Oct 28, 2024 •

edited

Loading

ambroisie commented Oct 29, 2024 •

edited

Loading