vnstat: add option to start vnstatd #19809
Conversation
|
@KaiHa, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @bjornfor and @offlinehacker to be potential reviewers. |
| cfg = config.services.vnstat; | ||
| in { | ||
| options.services.vnstat = { | ||
| enable = mkEnableOption "If enabled, vnstatd will be started to update the network usage statistics."; |
There was a problem hiding this comment.
This will render poorly. mkEnableOption already adds some boilerplate text to the description. Also, I think the consensus is to phase out usage of mkEnableOption.
| config = mkIf cfg.enable { | ||
| systemd.services.vnstat = { | ||
| description = "vnStat network traffic monitor"; | ||
| path = [ pkgs.coreutils ]; |
There was a problem hiding this comment.
If this is only for the kill call below, path is unnecessary.
| path = [ pkgs.coreutils ]; | ||
| wantedBy = [ "multi-user.target" ]; | ||
| serviceConfig.ExecStart = "${pkgs.vnstat}/bin/vnstatd -n"; | ||
| serviceConfig.ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID"; |
There was a problem hiding this comment.
Given that this runs as root, would you consider constraining the capabilities available to the main process and restrain access to the filesystem (e.g., protect home at least).
There was a problem hiding this comment.
@joachifm can you elaborate on this, how would you do this? Is there documentation or examples you can point me to? I have seen that vnstatd supports being run as a different user, maybe this simplifies things.
There was a problem hiding this comment.
systemd supports lots of mechanisms for isolating services. For example, you can do
let # for syntax highlighting only
serviceConfig = {
ProtectHome = true;
PrivateDevices = true;
PrivateTmp = true;
}to have the service run with an empty /home, a minimal /dev and a private /tmp. See systemd.exec(5) and systemd.resource-control(5) for details. As long as the service is running with full privileges, these directives mainly serve to reduce the risk of accidental data loss/information leaking, so it'd be better if it also ran under an unprivileged account.
|
Please change the target branch to master |
|
@joachifm OK, I have changed the base to master and did some cleanups. Furthermore is vnstatd now run as unprivileged user, this should prevent it from wreak havoc. |
| after = [ "network.target" ]; | ||
| wantedBy = [ "multi-user.target" ]; | ||
| unitConfig.documentation = "man:vnstatd(1) man:vnstat(1) man:vnstat.conf(5)"; | ||
| preStart = "chmod 755 /var/lib/vnstat"; |
There was a problem hiding this comment.
You can do serviceConfig.PermissionsStartOnly = true to run preStart as root, otherwise it might fail if ownership/permissions ever get out of whack.
There was a problem hiding this comment.
That chmod is there so others have read permissions on /var/lib/vnstat/
As long as vnstatd is the owner of /var/lib/vnstat I believe that chmod will succeed. If /var/lib/vnstat is owned by someone else, a chmod alone is not sufficient to fix this. Should I add also a chown into the preStart rule? For me that would feel a little weird.
There was a problem hiding this comment.
I'm thinking of the case where vnstatd no longer owns the folder, yes. I don't think it's weird to ensure consistent ownership, why do you think so?
There was a problem hiding this comment.
By weird I mean, /var/lib/vnstat is the home directory of vnstatd. If it is not owned by vnstatd then someone must have willingly changed the owner of /var/lib/vnstat. And where to draw the line, should I also create /var/lib/vnstat if someone has deleted it? Do you get what I mean?
| in { | ||
| options.services.vnstat = { | ||
| enable = mkOption { | ||
| default = false; |
There was a problem hiding this comment.
Please specify type = types.bool.
| enable = mkOption { | ||
| default = false; | ||
| description = '' | ||
| Wether to enable update of network usage statistics via vnstatd. |
Motivation for this change
Without a started vnstatd daemon vnstat can not monitor the network usage.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)