nixos/syncoid: enable N:N dataset mappings

[ju1m]

This PR is ready for reviews.

Motivation for this change

As proposed in a past comment, this PR changes the syncoid.service so that it uses DynamicUser=.

This move to DynamicUser= should safely allow (to the best of my knowledge, but please double check) to send the same dataset to multiple targets.

Things done

• nixos/syncoid: set TZ envvar
• nixos/syncoid: allow @timer syscalls
• nixos/syncoid: add destroy to localTargetAllow's default
• nixos/syncoid: innocuous cleanups
• nixos/syncoid: use DynamicUser=
  • If nftables is enabled, the set @nixos-syncoid-uids can be used for UID-based dynamic egress filtering of the SSH connections. ExecStartPre= (resp. ExecStopPost=) will add (resp. remove) in that set the dynamic UID of the syncoid process so that it can be used with something like: networking.nftables.ruleset = "table inet filter { chain output-net { skuid @nixos-syncoid-uids meta l4proto tcp accept } }";
  • ssh is hacked using BindReadOnlyPaths= to workaround a bug in bash's own malloc implementation bash: update patches #146463 (comment) , this could be removed after 2ef6252 has hit master.
• nixos/sanoid: add utilities useful to syncoid: make mbuffer and lzop findable over ssh.
• Test sending in parallel a dataset to two different targets.
• Add support for LoadCredentialEncrypted=.

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 21.11 Release Notes (or backporting 21.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[numinit]

Are all sends still done in parallel? I'm observing this ZFS bug after updating to 21.11.

openzfs/zfs#13040

[ju1m]

@numinit, yes, nothing change for that: each entry in commands generates a systemd service running a syncoid.

[numinit]

@ju1m Gotcha. I may have an API idea for how to allow users to run certain sends in series, I'll toy around with it. Going from all in series to all in parallel was some whiplash for my particular setup and I'm not sure if it was documented anywhere.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/731

[lopsided98]

Sorry for ignoring this PR, but I have started to feel like I can't keep up with the changes to this module.

Perhaps this is explained in some previous discussion, but what do you mean by N:N dataset mappings? Could you provide an example config, and perhaps add it to the NixOS test?

I also am concerned about how DynamicUser interacts with delegated permissions. What happens if the power fails or the machine crashes in the middle of running syncoid? Wouldn't that cause the permissions to never be removed, and potentially later given to another DynamicUser service that reuses the same UID?

[ju1m]

@lopsided98

About DynamicUser=

Although "UID/GIDs are recycled after a unit is terminated.", "a service by the same name is likely to always use the same numeric UID". So if a crash occurs during the service's time, the next run will use and remove the permissions.
You can run multiple times commands like these to test the stability of the dynamic UID/GIDs:

# sysctl restart syncoid-backup-mermet.sourcephile.fr-rpool-var-mail.service

# zfs allow losurdo/backup/mermet/var/mail
---- Permissions on losurdo/backup/mermet/var/mail -------------------
Local+Descendent permissions:
        user _du134c26c001915cde change-key,compression,create,destroy,mount,mountpoint,receive,rollback

# id _du134c26c001915cde
uid=63562(_du134c26c001915cde) gid=63562(_du134c26c001915cde) groupes=63562(_du134c26c001915cde)

# while systemctl is-active syncoid-backup-mermet.sourcephile.fr-rpool-var-mail.service; do sleep 1; done

# id _du134c26c001915cde
id: '_du134c26c001915cde': no such user

About N:N dataset mappings

With the current syncoid.service it is not possible to reliably send the same dataset to multiple locations because there is a race condition on setting/removing the permissions between services since the same UID/GID are shared by the syncoid services. This constraint is removed by using DynamicUser= and because the generated services have different names hence different UID/GIDs.
I've added a test showcasing this. Note that it reuses a snapshot taken by sanoid: it would not work if syncoid were used to take the snapshot, because then there could be a conflict in their name when they're taken the same second.

[lopsided98]

Thanks for the explanation and the test!

Regarding the UID issue, I'm not totally comfortable with relying on "likely" behavior for security. The blog post you linked describes UID reuse as a performance optimization, rather than guaranteed behavior. In the documentation, the sentence after the one you quoted specifically notes reuse is not guaranteed: "Care should be taken that any processes running as part of a unit for which dynamic users/groups are enabled do not leave files or directories owned by these users/groups around, as a different unit might get the same UID/GID assigned later on, and thus gain access to these files or directories."

Turning this into an exploitable vulnerability requires a combination of several seemingly unlikely scenarios, but many attacks consist of methods of turning unlikely events into likely ones. Therefore, I don't feel comfortable approving this PR yet. Ideally we could find some solution to truly fix the problem, but at least I would like feedback from others on this issue.

[ju1m]

@lopsided98 yes it's a "likely", but please let's dive in a bit to understand why. I hope that I won't make too many mistakes nor make it too hard to follow.

AFAIU, the stability of the UID/GID is not the side-effect of a secondary performance optimization, it is a conscious goal to minimize the number of chown()s on directories like StateDirectory=, so IMHO this need for stable UID/GIDs will never go away. Sure the logic might change but then the crash would need to happen during that precise systemd update (very unlikely).

The current logic (in pick_uid()) to reach that goal cannot be perfect, it is limited by the quality of the hash (SipHash currently) and the maximum size of the pool (usually ~4000).

PHASE_SUGGESTED,  /* the first phase, reusing directory ownership UIDs */
PHASE_HASHED,     /* the second phase, deriving a UID from the username by hashing */
PHASE_RANDOM,     /* the last phase, randomly picking UIDs */

So, AFAIU, to cause a security problem one would need to:

1. Act when booting just after a crash of systemd which happened while a syncoid service was running (can happen, power outages happen and syncoid services usually run for a few minutes each hour).
2. Take control of a vulnerable service and act before that syncoid service runs again (usually one hour).
3. That vulnerable service must have the Unix (and maybe AppArmor/SELinux) permissions to access /dev/zfs (by default NixOS comes without AppArmor/SELinux and with rw-rw-rw- root:root, too bad).
4. That vulnerable service's name must have a hash clashing with the hash of:
   1. Either the name of that syncoid service.
   2. Or the name of another running service, and must then acquire by chance the UID of that syncoid service (100 chances out of ~4000 usually)
      (all this sounds virtually impossible, only the root user controls the service name, not a non-root vulnerable service, otherwise it's already game over anyway)

Possible mitigations

• Restrict /dev/zfs to rw-rw---- root:zfs and add syncoid/sanoid services and other legitimate users to the zfs Unix group.

[lopsided98]

Thank you for looking into this. I now agree that the risk seems acceptable, although I'd still like someone else to concur (I can't merge, so that's pretty much inevitable anyway).

[ju1m]

• Rebase after 2c3f605 and 2e751c0

[ju1m]

• Add support for LoadCredentialEncrypted=.

[mweinelt]

We're locking and closing this pull request, because through the rebase you inadvertently requested many maintainers for review, which subscribed them to update notifications, resulting in unnecessary spam inside everyone's inbox. While we can remove their review requests, we sadly cannot unsubscribe anyone.

Please create a new pull request and for the next time remember to set your PR to draft status before rebasing. In draft status, you can preview the list of maintainers that are about to be requested for review, which allows you to sidestep this issue.