Conversation
d4984ad to
dc84cc2
Compare
dc84cc2 to
35bc83d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
@nu-nu-ko
If you are allowed, I'd like to be a contributor, thank you!
Do you mean during tests? |
This comment was marked as outdated.
This comment was marked as outdated.
|
I've seen this in When you use But it doesn't happen the other way, if |
This comment was marked as outdated.
This comment was marked as outdated.
|
Is it weird if there were two separate sets of options?
|
|
Also, related and unrelated. I was seeing that the web ui and other services were being unresponsive after a couple of hours. I thought that the IO of qbittorrent was bringing the system down, but after 2 days of trying everything, it was my network device's firmware. 😅 I'll try to retest it this week. |
nevivurn
left a comment
There was a problem hiding this comment.
Afaik, it is possible to pass in the webui and torrenting port via command line arguments, and I assume these arguments take precedence over other configuration options. This should let us implement openFirewall without forcing users to use the declarative config.
a better way to set CapabilityBoundingSet and SystemCallFilter to restrict all.
CapabilityBoundingSet is an allowlist, the strictest setting is just the empty string. Although, given that we 1) don't run qbittorrent as root, 2) set NoNewPrivileges, 3) don't set AmbientCapabilities, omitting it entirely is probably fine.
I personally think @system-service is a good enough default for SystemCallFilter.
I don't understand why it sets PrivateTmp to false
This comment in the upstream PR mentions adding torrents through the command line. Not sure if this is actually true.
ProtectSystem needs to be disabled if we aren't using declaritiveConfig
I don't think ProtectSystem should be enabled here. It will break users with per-category/torrent save paths, no matter what.
imo, we don't need lock down every option by default. Users that want to further harden their system can easily add these options in their own nixos configuration.
d60b403 to
bc0a267
Compare
This comment was marked as outdated.
This comment was marked as outdated.
nevivurn
left a comment
There was a problem hiding this comment.
I don't see why we wouldn't set it regardless personally, leaving it empty seems to do nothing as according to systemd-analyze security qbittorrent on my system.
👍
I agree to some extent, id much rather every service does at least up to "breaks as little as possible"
I completely agree, we can enable everything that probably won't break users.
There are still a couple options left that I feel like are a tad bit too opinionated, namely
- non-default torrenting port
- umask 0066
This comment was marked as outdated.
This comment was marked as outdated.
Ah, did not realize the empty list wasn't doing anything.
The bittorrent port is chosen randomly by default, and it is quite unexpected for nixos to fix an arbitrary port number by default. Similar with the web port, I don't see a reason for the NixOS module to deviate from the upstream default. With this module, it would be trivial for users to change it as needed, anyway.
I think this would break users trying to access downloaded files without needing to run as the qbittorrent user, likely a common use case. Plus, if we're going for paranoid settings, why is it not 0077? |
e3913e0 to
c289233
Compare
c289233 to
07d92b9
Compare
aa2f5a4 to
145b6e3
Compare
|
|
Another friendly ping to @eclairevoyant and @zhaofengli. There are still requested changes from you. Please check if the changes are already implemented or still necessary. Thanks! |
This comment was marked as resolved.
This comment was marked as resolved.
|
I don't see any reason to hold this PR any longer and I will merge it soon unless someone raises any blocker. Any improvements can be done in follow up PRs. |
Ah, I wasn't aware of that. My bad.
Great! Thanks. |
nixos/qbittorrent: add default serverConfig & fix test Migrate to runTest Replace lib.optional with lib.optionals nixos/qbittorrent: update release notes to 2511
f37d884 to
84d174e
Compare
I've squashed the commits, should be ready now, thank you. |
As discussed in PR NixOS#287923. The author agreed to add me as a maintainer to the module after merging.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-25.05
git worktree add -d .worktree/backport-287923-to-release-25.05 origin/release-25.05
cd .worktree/backport-287923-to-release-25.05
git switch --create backport-287923-to-release-25.05
git cherry-pick -x 84d174e312870ccefb9ba0dd11532bb2a58773db |
|
This adds a module and should therefore not be backported IIUC |
|
Why can't new modules be backported? It's not a breaking change. Of course if we need to make some breaking changes to make it usable, that should be done first. |
From CONTRIBUTING.md:
I believe this module should just work on 25.05. But will run the tests to make sure. |
I seem to remember having a trivial module addition be denied on the basis that any change to the options is potentially breaking, but I can't find it nor can I find a reference to this being a precedent, so I guess it's probably fine. |
|
For those who can't wait for the module to be backported, you can always import it from the unstable branch to your config. I am assuming you are using flake and have an unstable entry in the inputs: nixpkgs-unstable.url = "github:nixos/nixpkgs/nixos-unstable";and you are passing the inputs to your config with Then you can casually import the needed module: imports = [
"${inputs.nixpkgs-unstable.outPath}/nixos/modules/services/torrent/qbittorrent.nix"
];The only problem could be that When the time comes, you can just remove the |
As discussed in PR NixOS#287923. The author agreed to add me as a maintainer to the module after merging. (cherry picked from commit 380cd59)
you could also just grab pkgs.vuetorrent from unstable as well to those wanting a backport, I'm not sure about the process there but if anyones willing to explain what (if anything) needs done to do so I'd be happy to. This has been in a working-enough state for quite some time so I'd assume itd be just fine |
|
Literally just cherry-pick the commits on top of the |
| inherit (cfg) user group; | ||
| }; | ||
| "${cfg.profileDir}/qBittorrent/config/qBittorrent.conf"."L+" = mkIf (cfg.serverConfig != { }) { | ||
| mode = "1400"; |
There was a problem hiding this comment.
I noticed that qBittorrent.conf is created with mode = 1400. Why is the sticky bit set?
Also, it looks like permissions are ignored for symlinks anyway: https://www.man7.org/linux/man-pages/man5/tmpfiles.d.5.html#:~:text=L%2C%20L+%2C%20L?.
There was a problem hiding this comment.
Why is the sticky bit set?
if my memory is right, setting it made qbittorrent unable to overwrite the symlink or otherwise within it with a directory/file.
it looks like permissions are ignored for symlinks anyway
if this is the case for the stickybit also then I'm not sure why I observed this behavior.
anyone willing to confirm qbittorrents behavior is consistent with/without this set, please do share.
There was a problem hiding this comment.
A PR was made a couple days ago, changing this behavior. We could continue this discussion there: #482534.
Description of changes
create a module to use qbittorrent as a service.
user notes
in ui settings changes
stateful service config management is possible, simply dont set
serverConfigsetting
serverConfigwill still allow settings to be changed while the service runs but rebuilds, reboots etc will lose those changes that arent defined inserverConfigpassword formatting
the password format that qbittorrent expects can be generated using this tool ( thanks Fea )
it can also be gotten from running the service independently and grabbing the formatted string out of the generated file after the setup phase
alternative UI's
custom webuis managed with nix are possible, examples with VueTorrent.
fetchzip to get VueTorrent from the source.
from nixpkgs.
use before merge example
assumes you have this (https://github.com/fsnkty/nixpkgs/tree/init-nixos-qbittorrent) branch as an input named
qbitnice to haves / future PR ideas
gendeepINImigrationVersionin service config before handThings done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.