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

nixos/transmission: Require encryption by default #87920

Closed
wants to merge 1 commit into from
Closed

Conversation

@L-as
Copy link
Contributor

@L-as L-as commented May 16, 2020

Motivation for this change

This seems like the sane default, since you typically always want your traffic to be encrypted.
This is kind of a breaking change, but I assume that people who have not explicitly set their settings most likely just don't know that encryption isn't enforced by default.

Things done
  • 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.
This seems like the sane default, since you typically always want your traffic to be encrypted.
This *is* kind of a breaking change, but I assume that people who have not explicitly set their settings most likely just don't know that encryption isn't enforced by default.
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented May 16, 2020

Hi,

I'm a transmission user and I checked what's currently my encryption value in my live settings.json and it's 1 which seems like upstream's default:

https://github.com/transmission/transmission/wiki/Editing-Configuration-Files#misc

This default makes sense to me. Why should we enforce encryption? BTW, this module is written so badly that it needs a thorough rewrite. The reason is that it's setting other defaults to the settings.json which are impossible to overwrite without almost rewriting the module in your own configuration.nix entirely. Your change just adds such an impossible to override settings value to the list.

I once tried to enable my self overriding such a default in #76552 but I closed it in favor of a better PR which I don't have time yet to write because I need to write an RFC for declarative systemd-tmpfiles management, per https://discourse.nixos.org/t/nixpkgs-policy-as-for-systemd-prestart-setup-scripts-vs-systemd-tmpfiles/5839 . Also, the settings of the default and the user's need to be merged somehow, probably with the // operator...

Other issue (I haven't opened it but I noted it to myself) with the module, is that passwords are impossible to set in a manner that doesn't put them world readable in the store. That's because the preStart script is hardcoded and the settingsFile is written to the store unconditionally:

settingsFile = pkgs.writeText "settings.json" (builtins.toJSON fullSettings);
# for users in group "transmission" to have access to torrents
fullSettings = { umask = 2; download-dir = downloadDir; incomplete-dir = incompleteDir; } // cfg.settings;
preStart = pkgs.writeScript "transmission-pre-start" ''
#!${pkgs.runtimeShell}
set -ex
for DIR in "${homeDir}" "${settingsDir}" "${fullSettings.download-dir}" "${fullSettings.incomplete-dir}"; do
mkdir -p "$DIR"
done
chmod 755 "${homeDir}"
chmod 700 "${settingsDir}"
chmod ${downloadDirPermissions} "${fullSettings.download-dir}" "${fullSettings.incomplete-dir}"
cp -f ${settingsFile} ${settingsDir}/settings.json
'';

Anyway, I'm sorry but I vote against your change, not only because it's a change to upstream's which I think was set like that for a good reason, but also because the module can't handle overrides to the defaults by the user.

@L-as
Copy link
Contributor Author

@L-as L-as commented May 16, 2020

As for whether encryption should be set by default: This will only affect people who have not touched their settings option at all. It isn't what upstream has set as the default obviously, but at least I don't think it's a good default.
I assume that most people, at least I do, want their torrent traffic to be encrypted unconditionally.

Also, I am pretty sure that you can change the default. The only defaults that do not disappear when you specify the settings as {} is umask, download-dir, and incomplete-dir. If you look at fullSettings, you can see that it uses // already, and in a // b, the values in b are prioritized.

The other issues I don't know about, unfortunately.

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented May 16, 2020

The only defaults that do not disappear when you specify the settings as {} is umask, download-dir, and incomplete-dir.

That is correct. I thought that all of them are unoverrideable because these settings specifically are those that their defaults have always bothered me.

As for whether encryption should be set by default: This will only affect people who have not touched their settings option at all. It isn't what upstream has set as the default obviously, but at least I don't think it's a good default.

For as far as I know, Nixpkgs' policy regarding such dilemmas, is to go with upstream's original intentions. Perhaps you should write a PR upstream? I imagine that if you'll do that they'll be able to explain why isn't encryption set to 2 by default.

It's just that I imagine this might brake downloads for some people if they are connected to trackers or other clients which don't support encryption at all.

@L-as L-as closed this May 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.