NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.19k stars 14.2k forks source link

borgbackup configuration creates borg-repo before filesystems are mounted #54747

Open ToxicFrog opened 5 years ago

ToxicFrog commented 5 years ago

https://github.com/NixOS/nixpkgs/blob/0576b0236940a69118c3cb1ab6406792185c7c1a/nixos/modules/services/backup/borgbackup.nix#L102

If the repo dir is local, it adds an activation script that creates the repo. This is (a) useless, because the backup script will call borg init if necessary before creating a backup, and borg init will create the repo directory, and (b) dangerous, because the activation scripts run before systemd starts, which means if the borg repo is located on a filesystem that doesn't become available until later in the boot process, it will create an empty directory under that mountpoint and prevent the filesystem from being mounted in the first place.

dotlambda commented 5 years ago

Did you read the comment above the line you're referencing? The repo directory needs to exist prior to starting the systemd unit because otherwise, systemd will fail due to a nonexistant directory being specified in ReadWritePaths. The latter is used for additional security. It has the affect that borg can't access any other than the specified paths.

In order to make the unit wait for the appropriate filesystems to be mounted, we could use RequiresMountsFor. Would that solve your problem?

PS: Next time, please ping the maintainer of the respective NixOS module or package.

ToxicFrog commented 5 years ago

I don't think RequiresMountsFor helps here, because that gets checked when the unit starts up, while this directory creation happens at activation time, before systemd starts at all. It might at least mean the unit fails noisily (when that filesystem can't be mounted because the activation script has dirtied its mountpoint) rather than writing to the wrong filesystem, which would be an improvement.

I guess we can't put directory creation in ExecStartPre because RequiresMountsFor is checked before any of the precommands run, right?

Marking the mountpoint as boot critical (by setting neededForBoot) would fix this by ensuring it gets mounted before activation runs at boot, but then (AIUI) boot will fail outright if the filesystem can't be mounted, which is not a good tradeoff if (e.g.) you keep your backups on a removable drive that is only sometimes connected.

The entry in ReadWritePaths could be made optional, but in that case I don't think the borg service has any way of creating it, because creating the directory in the first place requires writing to its containing directory.

My inclination would be to move this out of activation entirely and instead make it a one-shot unit that runs after local-fs.target and before any of the borgbackup units run; this is the approach used by the gitolite and ipfs services, ensuring that the directory is created after local filesystems are mounted but before service startup.

As an added bonus, this would let borg init run as part of this unit, rather than needing to be conditionally run inside the backup script itself.

dotlambda commented 5 years ago

I guess we can't put directory creation in ExecStartPre because RequiresMountsFor is checked before any of the precommands run, right?

If I remember correctly, ReadWritePaths must exist in ExecStartPre too so this is probably also the case. Hence, you're right, the one-shot unit is the way to go. We can add the RequiresMountsFor option to it. However, I'm not sure if we should run borg init in that unit since that would make borg have access to the whole system after all.

stale[bot] commented 4 years ago

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.
ToxicFrog commented 4 years ago

This is, in fact, still important to me, but not quite important for me to actually implement it.

stale[bot] commented 3 years ago

I marked this as stale due to inactivity. → More info