systemd integration for encryption support

[aerusso]

Motivation and Context

Per conversation in #8750, modifications should be made to the mounting framework used in systemd to allow for encrypted datasets keys to be loaded.

Description

Modify zfs-mount-generator to produce a dependency on a new zfs-import-key-*.service, dynamically created to call zfs load-key for the encryption root before attempting to mount any encrypted datasets.

These dynamically generated units RequiresMountsFor on the keyfile, if present, or calls systemd-ask-password if a passphrase is requested.

This patch includes suggestions from @Fabian-Gruenbichler, @rlaager, and @ryanjaeb.

How Has This Been Tested?

I have NOT tested this.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.

[rlaager]

Is there a white space issue here? (I’m on mobile, so it may just be displaying weird.)

[rlaager]

What happens if the passphrase is entered wrong? Does this need to be an sh loop of, say, up to 3 times?

[aerusso]

I dropped the indentation back to let this fit on a single line, within the style guideline's length limitation. I'll change it to anything that's legible, though.

As for the passphrase issue, I think that should be considered a bug/feature request for systemd, where they can solve this issue for everyone, once and for all, instead of having everyone think about doing this, and maintaining it, themselves.

[rlaager]

systemd has no way of knowing if the passphrase entered was correct. I suppose the protocol could be changed from outputting the password to exec()ing something (in our case zfs load-key) and checking its exit status. But for now, we need to work with what exists.

The sh code isn't particularly complicated, but it's probably too much for a one-liner in the unit. Here's a commit (on a systemd-integration branch on rlaager/zfs) which adds a systemd-load-key script and uses it: rlaager@a894797

I tested the script standalone. I tested that it gets installed to the right directory.

[aerusso]

I looked into automatically restarting the unit up to three times on failure, but doing this for oneshot services is not allowed, for some reason. A PR to get that changed stalled years ago, so I think we'll need to implement this functionality ourselves.

I adapted your script, and did include it as a one-liner in the service file, in the interest of making downstream maintainer's lives easier (i.e., to avoid needing to ensure another path is available at some point in the boot process).

I also added an error code return after three failed attempts.

[ryanjaeb]

I'm not familiar with how zfs-mount-generator works. Would it be possible to get a high level overview of what a regular user like me would need to do to have a dataset unlocked on boot? I'm assuming if I have an encrypted dataset, /tank/enc, the generator will give me a unit for zfs-load-key-tank-enc.service. Would that be disabled by default and I enable it if I want the dataset unlocked on boot?

I ask because I'll have datasets that I don't want unlocked on boot.

[rlaager]

if I have an encrypted dataset, /tank/enc, the generator will give me a unit for zfs-load-key-tank-enc.service.

Yes.

Would that be disabled by default and I enable it if I want the dataset unlocked on boot?

I believe all the units from the generator are enabled. (I'm not sure that it is even possible for a systemd generator to generate disabled units.)

I ask because I'll have datasets that I don't want unlocked on boot.

If they aren't going to be unlocked on boot, then they cannot be mounted. I believe the consensus from #8750 was that you should mark those datasets as canmount=noauto. That will prevent zfs-mount-generator from creating units to load the key and mount the dataset.

[ryanjaeb]

If they aren't going to be unlocked on boot, then they cannot be mounted. I believe the consensus from #8750 was that you should mark those datasets as canmount=noauto. That will prevent zfs-mount-generator from creating units to load the key and mount the dataset.

That's perfect for anything I can think of. Anything I don't want mounted on boot is going to end up underneath a single dataset, so I can set canmount=noauto once and everything else will "just work". I didn't look at the code really close, but the intended strategy seems good to me. Thanks!

[Fabian-Gruenbichler]

Reusing the mount generator seems like a good idea :) I am not convinced it can replace zfs-mount.service altogether (there are people that don't want to run ZED, there might be cases where the cache file is outdated for whatever reason, ...) so IMHO we still need some way to get prompting into zfs-mount.service as well.

I think I'd prefer a template unit for loading keys (parametrized by dataset/encroot). The RequiresMountsFor= directive could then be added as override (UNIT.d snippet) by the generator, and regular users could also re-use the unit if they don't want to go full-blown generator.

Whether we ship a helper script that is used in the units that wraps systemd-ask-password and zfs-load-key together, or whether we add (./configure-able) systemd-ask-password support into zfs-load-key directly seems like not much of a difference, but the latter is more clean and enables proper prompting across the board (including easily skipping prompting in case the key has already been loaded some other way)..

So I'd propose the following:

• new parameter to specify number of prompt attempts for zfs load-key, if prompting is needed
• new parameter to prompt via systemd-ask-password, if prompting is needed

#[email protected]
[Unit]
Description=Load ZFS key for dataset %i
Documentation=man:zfs-mount-generator(8)
DefaultDependencies=no
Wants=zfs-import.target
After=zfs-import.target
[Service]
Type=oneshot
RemainAfterExit=yes
ExecStart=@sbindir@/zfs load-key --count 3 --systemd '%I'
ExecStop=@sbindir@/zfs unload-key '%I'

#[email protected]/source.conf
[Unit]
SourcePath=/some/zfs-list-cachefile

#[email protected]/mounts.conf
[Unit]
RequiresMountFor=/path/to/keyfile

The generator would then just ln -s "$(systemd-escape --template=zfs-load-key@service ${dataset})" "$req_dir" (which is equivalent to enabling the instance) and create the .d snippet files if needed.

zfs-mount.service could then simply get a zfs load-key ExecStartPre.

Final, semi-unrelated question: what about zvols? They might be referenced in non-ZFS .mount units, and would thus also benefit from a zfs-load-key@ unit that could just be (manually) instanced..

[aerusso]

@Fabian-Gruenbichler I also would prefer compiling support for systemd-ask-password into zfs load-key, and I think it would probably make sense to push the same support into zfs mount -l in the same way. Then we directly solve the zfs-mount.service issue by just adding the -l --systemd options.

As for the compressed zvols, I think it makes sense to also have zfs-mount-generator produce the appropriate encryption root units as well, but let the user systemctl enable the templates if they're not explicitly required by another canmount=auto dataset.

Another thing that's come up, if we're going to enable zfs-mount-generator by default, is how do we manage automatically adding the zfs-list.d pool files? We already clear them on pool export, but what if we shut down a machine, with the pool imported, and then remove the pool hardware? This is something I'll often do, perhaps out of laziness. It will lead to lots of failed mount units in systemd, and could be a pain point for users.

[Fabian-Gruenbichler]

On Tue, Jun 04, 2019 at 08:58:45AM -0700, Antonio Russo wrote: @Fabian-Gruenbichler I also would prefer compiling support for `systemd-ask-password` into `zfs load-key`, and I think it would probably make sense to push the same support into `zfs mount -l` in the same way. Then we directly solve the `zfs-mount.service` issue by just adding the `-l --systemd` options.

sounds good to me. the ask-passwork API is not exposed by systemd except via systemd-ask-password unfortunately..

As for the compressed zvols, I think it makes sense to also have `zfs-mount-generator` produce the appropriate encryption root units as well, but let the user `systemctl enable` the templates if they're not explicitly required by another `canmount=auto` dataset.

the zvol support is only needed if you have another .mount unit for the FS that is on top of the zvol, and only if that .mount unit is supposed to get started on boot. I think involving the mount generator is probably not needed, if I only need to do ```systemctl enable [email protected]``` and optionally, add dependencies to this instance (for the keyfile) and to the ```.mount``` unit (for ordering/dependency tracking).

Another thing that's come up, if we're going to enable `zfs-mount-generator` by default, is how do we manage automatically adding the `zfs-list.d` pool files? We already clear them on pool export, but what if we shut down a machine, with the pool imported, and then remove the pool hardware? This is something I'll often do, perhaps out of laziness. It will lead to lots of failed mount units in systemd, and could be a pain point for users.

Document that if you plan on doing that, you should either - export the pool manually before shutting down (IIRC, dracut does an export in the final stage of the shutdown?) - set canmount appropriately for the to-be-removed pool to prune the cachefile automatically via ZED - edit the cache files manually to remove the problematic entries we could also introduce an additional mechanism to ignore certain pools/datasets/..., either via another (set of?) file(s), or via the kernel command line (which is both available in systemd units via "ConditionKernelCommandLine" directives, and in the generator via "/proc/cmdline"). most of the above can also be done manually by the admin inside the initrd or bootloader, in case the first hands-off boot attempt fails because things fall apart too much.

[aerusso]

If we move down this path, we need to decide how to modify the behavior of lib/libzfs/libzfs_crypto.c:get_key_material and get_key_material_raw. get_key_material_raw is pulling the keys from its fd parameter, which is opened in get_key_material.

We shouldn't be hard-coding for a particular method of getting keys, so we probably want to send get_material_raw a command to execute (i.e., systemd-ask-password). That will presumably mean modifying the libzfs API to allow for that information to be passed. On the upside, it will allow easy fixes for both zfs mount -l and zfs load-key with some parameter like --interactive-password='systemd-ask-password --id=%id'. But that is starting to feel a little over-engineered.

[Fabian-Gruenbichler]

If we move down this path, we need to decide how to modify the behavior of lib/libzfs/libzfs_crypto.c:get_key_material and get_key_material_raw. get_key_material_raw is pulling the keys from its fd parameter, which is opened in get_key_material.

We shouldn't be hard-coding for a particular method of getting keys, so we probably want to send get_material_raw a command to execute (i.e., systemd-ask-password). That will presumably mean modifying the libzfs API to allow for that information to be passed. On the upside, it will allow easy fixes for both zfs mount -l and zfs load-key with some parameter like --interactive-password='systemd-ask-password --id=%id'. But that is starting to feel a little over-engineered.

that would probably mean doing something like
get_key_material_ext

which then calls a helper script (in our case, wrapping systemd-ask-password) with

• dataset/fsname
• keyformat
• whether this is a second prompt to check for typos (again)
• whether this is a new key (newkey)
  that returns the key material on stdout?

dataset/fsname, newkey and again could also be combined into a single prompt message parameter - I think keeping the dataset parameter on its own would still make sense, since the helper script might need it to know which key to return if it's non-interactive ;)

that would also allow opening up other storages (such as vault, pass, ...) to be easily used?

OTOH, just writing the single systemd-ask-password helper now with just two parameters (dataset, prompt message) would enable us to go forward with this PR (with ExecStartPre and zfs load-key in zfs-mount.service), and still do the rework with the full feature-set afterwards as a separate thing?

[behlendorf]

@aerusso @Fabian-Gruenbichler @rlaager and @ryanjaeb, how would you like to move forward with this? The proposed solution seems reasonable to me, but I'm unclear if there was a consensus reached. If you're happy with this approach can we get the PR rebased and the conflict resolved.

[rlaager]

I re-reviewed this and marked my review concerns as resolved.

In re-reading the history here, it sounds like the remaining concerns relate to a combination of:

1. Whether ZED should be required. If not, we still need to address prompting in zfs-mount.service. That could be done with a script like I previously proposed, or it could use the following:
2. Whether we should integrate systemd-ask-password support into zfs load-key, possibly with some indirection where we don't hard-code systemd-ask-password but give zfs load-key some flag like --interactive. I don't feel particularly strongly about it, but I doubt the latter is much harder, and it does ad flexibility that may be very useful in other environments, especially on other OSes.
3. @Fabian-Gruenbichler is proposing that we use a template unit that the generator enables instances of as opposed to having the generator create units. That would allow people to use the template without having to use the generator. If this is a use case that anyone cares about, that seems fine to do. It doesn't add much complexity. This assumes that the RequiresMountsFor bit can be added with a unit.d drop-in, which I agree should work.

@aerusso What do you think of adding --interactive to zfs load-key (and maybe also a --retry or something, as some --interactive commands could handle their own retries), switching to using a template [email protected], updating the generator accordingly, and wiring in a zfs load-key -a --interactive ... for zfs-mount.service? Does that address everything @Fabian-Gruenbichler mentioned?

[ryanjaeb]

@behlendorf I have no preference. The originally proposed solution looks like it would work find for me, but my usage is pretty simple since I'm only unlocking volumes with a passphrase that's stored in a file.

I don't know enough about everything involved to offer a well informed opinion on any of the interactive stuff.

[aerusso]

@rlaager I basically agree with everything here. I haven't had a chance to dig into zfs load-key, and unfortunately I cannot make it a high priority right now. It might make sense to merge a partial solution, and open a new issue to address the situation for people who do not/cannot use zed.

[Fabian-Gruenbichler]

@rlaager summarized it quite nicely, and IMHO everything besides the decision on whether to use template unit instances or proper units can be done in a follow-up PR.

I can volunteer for the zfs-load-key part starting at the end of next week.

[codecov]

Codecov Report

Merging #8848 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8848      +/-   ##
==========================================
+ Coverage   78.62%   78.62%   +<.01%     
==========================================
  Files         401      401              
  Lines      120158   120158              
==========================================
+ Hits        94476    94480       +4     
+ Misses      25682    25678       -4

Flag	Coverage Δ
#kernel	79.46% <ø> (+0.04%)	⬆️
#user	66.27% <ø> (+0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5db313...dfce298. Read the comment docs.

[behlendorf]

Thanks for the status feedback. Then I'll get this change integrated and we'll address the remaining work in a followup PR.

[rlaager]

@rlaager summarized it quite nicely, and IMHO everything besides the decision on whether to use template unit instances or proper units can be done in a follow-up PR.

It seems to me that the generator could be changed later to use the template units instead of writing out units.

[rlaager]

I changed my review from "Requested changes" to "Approved" to stop blocking this. I haven't actually run this, but the code looks reasonable to me.

[behlendorf]

@aerusso can you confirm that you've tested this locally.

[aerusso]

@behlendorf I tested this more thoroughly a few weeks ago. I just compiled it again, and a cursory test has it working. I also rebased onto master and squashed down to just two commits.

The second commit enables the zed script, but users will still need to touch /etc/zfs/zfs-list.cache/<poolname> before zfs-mount-generator will actually do anything. There were concerns raised in #9016 over scalability with large numbers of volumes. I'm leaning towards not including that commit, given those concerns, and fixing that in another PR.

Regarding templates:

@rlaager summarized it quite nicely, and IMHO everything besides the decision on whether to use template unit instances or proper units can be done in a follow-up PR.

Getting proper mount dependence information into the templates is tricky (read: I don't see how that is possible). I don't think that should block this PR, and we can add that simplification later if it can be done.

[rlaager]

Getting proper mount dependence information into the templates is tricky (read: I don't see how that is possible).

The suggestion was that the generator write out drop-in files to add the dependencies to the units that need them:

/run/systemd/generator/[email protected]/mounts.conf
[Unit]
RequiresMountFor=/path/to/keyfile

[aerusso]

@rlaager Yes, that's correct. I suppose what I mean is, I don't see how to do this without using a drop-in, and I'm not sure what value there is to using a template and a generated drop-in, compared to just a single generated unit.

For instance, there are twice as many files used in the drop-in case. Does that affect performance? I think we'd need to do a benchmark if we wanted to target that performance enhancement.

[behlendorf]

Thanks for verifying this locally. Then I'm going to go ahead and integrate this and we'll followup as appropriate with additional PRs.