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/nzbget: cfg.configFile/cfg.dataDir owned by cfg.user/cfg.group #58928

Closed

Conversation

cmacrae
Copy link
Contributor

@cmacrae cmacrae commented Apr 3, 2019

Motivation for this change

These changes ensure, via the preStart script, that the configFile and dataDir
are owned by the user & group that nzbget has been configured to run with.
Without this, when the config template is copied over to facilitate
initial startup, it's done so with root permissions. This causes the
service to fail during the startup script (grep is used to inspect the
file for some values). The dataDir is similar circumstances; without these
changes, it's created with root permissions, which means the service cannot
read or write data there, if it's running as a different user.

Example of failure scenario

Mar 31 21:50:25 slim systemd[1]: Starting NZBGet Daemon...
Mar 31 21:50:25 slim 2f3n6zr1r5q4xplvnd6zizpknlhcha14-unit-script-nzbget-pre-start[2615]: /var/lib/nzbget/nzbget.conf not found. Copying default config /nix/store/q8q5af4cxypc75dhp223jl89amfhbn00-nzbget-20.0/s>
Mar 31 21:50:25 slim 2f3n6zr1r5q4xplvnd6zizpknlhcha14-unit-script-nzbget-pre-start[2615]: Setting temporary $MAINDIR variable in default config required in order to allow nzbget to complete initial start
Mar 31 21:50:25 slim 2f3n6zr1r5q4xplvnd6zizpknlhcha14-unit-script-nzbget-pre-start[2615]: Remember to change this to a proper value once NZBGet startup has been completed
Mar 31 21:50:25 slim 2xgi9aacik9f2q5wyyqa1vswd6izy9vg-unit-script-nzbget-start[2618]: grep: /var/lib/nzbget/nzbget.conf: Permission denied
Mar 31 21:50:25 slim 2xgi9aacik9f2q5wyyqa1vswd6izy9vg-unit-script-nzbget-start[2618]: In /var/lib/nzbget/nzbget.conf, valid ConfigTemplate not found; falling back to ConfigTemplate=/nix/store/q8q5af4cxypc75dhp>
Mar 31 21:50:25 slim 2xgi9aacik9f2q5wyyqa1vswd6izy9vg-unit-script-nzbget-start[2618]: grep: /var/lib/nzbget/nzbget.conf: Permission denied
Mar 31 21:50:25 slim 2xgi9aacik9f2q5wyyqa1vswd6izy9vg-unit-script-nzbget-start[2618]: In /var/lib/nzbget/nzbget.conf, valid WebDir not found; falling back to WebDir=/nix/store/q8q5af4cxypc75dhp223jl89amfhbn00->
Mar 31 21:50:25 slim 2xgi9aacik9f2q5wyyqa1vswd6izy9vg-unit-script-nzbget-start[2618]: nzbget.conf(0): Could not open file /var/lib/nzbget/nzbget.conf
Mar 31 21:50:25 slim systemd[1]: nzbget.service: Control process exited, code=exited status=1
Mar 31 21:50:25 slim systemd[1]: nzbget.service: Failed with result 'exit-code'.
Mar 31 21:50:25 slim systemd[1]: Failed to start NZBGet Daemon.

Things done

Ensure the configFile and dataDir are owned by the user & group the service has been configured to run with.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

These changes ensure, via the preStart script, that the configFile and
dataDir are owned by the user & group that nzbget has been configured
to run with.

Without this, when the config template is copied over to facilitate
initial startup, it's done so with root permissions. This causes the
service to fail during the startup script (grep is used to inspect the
file for some values). The dataDir is similar circumstances; without
these changes, it's created with root permissions, which means the
service cannot read or write data there, if it's running as a
different user.
@@ -61,10 +61,14 @@ in {
p7zip
];
preStart = ''
if [ ! -d ${cfg.dataDir} ]; then
echo "${cfg.dataDir} directory not found. Creating it."
install -d -o ${cfg.user} -g ${cfg.group} ${cfg.dataDir}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use systemd.tmpfiles instead for doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand a little on why systemd.tmpfiles might be a better approach here?
The resulting directory is intended to be "long-lasting".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the name tmpfiles is a bit misleading. It can be used for non-temporary files as well, which is in fact the main use of it in NixOS. Because this approach is declarative, it is preferred in NixOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, great. Thanks for the suggestion, I'll take a look!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and actually, there's serviceConfig.StateDirectory too, which is preferred over tmpfiles if it works (it's not as flexible as tmpfiles, but I don't think that flexibility is needed for this specific case).

@aanderse
Copy link
Member

aanderse commented Apr 5, 2019

@cmacrae since you're familiar with this service and the change you are making is at least somewhat related would you possibly mind taking a look at #53852 for nzbget?

@cmacrae
Copy link
Contributor Author

cmacrae commented Apr 9, 2019

Sure @aanderse, might as well catch it now :)

@aanderse
Copy link
Member

@cmacrae oops! I completely forgot about this PR the other day when I happened to notice the nzbget service is broken in 19.03. I ended up fixing it with @flokli in #60019.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants