Mount generator fixes #10477
Mount generator fixes #10477
Conversation
Drop Before=zfs.mount dependency explicity on generated key-load .service unit. Indeed, the associated mount unit is After=<dataset-key-load>.service. This is thus the mount point which controls at what point it wants to be mounted (Before=zfs-mount.service in stock generator), but this can be an automount point, or triggered by another service. This additional dependency from the key load service is not needed thus. Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com>
We need a stronger dependency between the mount unit and its keyload unit when we know that the dataset is encrypted. If the keyload unit fails, Wants= will still try to mount the dataset, which will then fail. It’s better to show that the failure is due to a dependency failing, the keyload unit, by tighting up the dependency. We can do this as we know that we generate both units in the generator and so, it’s not an optional dependency. BindsTo enable as well that if the keyload unit fails at any point, the associated mountpoint will be then unmounted. Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com>
The unit was failing instead of stopping if someone manually unloaded the key before stopping the unit (zfs unload-key is failing on an unavailable key). Follow a similar logic than for loading the key, checking for the key status before unloading it. Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com>
Codecov Report
@@ Coverage Diff @@
## master #10477 +/- ##
==========================================
+ Coverage 79.65% 79.77% +0.11%
==========================================
Files 393 393
Lines 123861 123861
==========================================
+ Hits 98663 98810 +147
+ Misses 25198 25051 -147
Continue to review full report at Codecov.
|
|
cc: @aerusso |
|
I’m concerned about the idea of stopping the mount unit trying to stop the key-load unit. The key-load unit is for the encryption root, which may have many mounts under it. Stopping one mount unit should not cascade into stopping all the mounts under that encryption root. I think the desired behavior is that stopping the mount unit leaves the key unit running. It is unclear which is the current behavior with this PR, as these seem to conflict:
|
Sorry, this is wrong (probably due to trying to be exhaustive :p) and I meant:
I have fixed the above description.
Hope this is more clear now :) |
| @@ -191,19 +198,19 @@ Documentation=man:zfs-mount-generator(8) | |||
| DefaultDependencies=no | |||
| Wants=${wants} | |||
| After=${after} | |||
| Before=${before} | |||
There was a problem hiding this comment.
If we do this, should we also drop Wants=, After=, and Requires=?
The key load may apply to multiple encryption roots, right? So it's inappropriate for any of the particular mount x-systemd.* options to apply. So my vote is to just drop all of these.
Caveat: I just woke up. So read my arguments very critically.
There was a problem hiding this comment.
FWIW we had some discussions about this way back when: #9649 (comment) (and at least the following comment by @rlaager)
There was a problem hiding this comment.
@didrocks What is the effect of removing this Before= and why did you make that change?
There was a problem hiding this comment.
@didrocks What is the effect of removing this Before= and why did you make that change?
This was done in this commit (e80ded6). To expand a little bit more, the idea is to delay the home user mount once the keyfile is accessible. The keyfile can be made accessible way later than zfs-mount.service (which is what is in the before= here), like via a pam module on login for instance, but can be also be made accessible during the boot process (single user, plymouth asking for passphrase) or so on.
Basically, the idea is to not necessary link those units to zfs-mount.service, which will try to mount it, then, by dependency chain, try loading the key-load unit which will fail at this point, creating spam in logs.
great! I was also unsure about that part.
How would this be implemented? Some changes to the generator that conditionally generate slightly different units depending on a control property? If so, then as a humble ZFS user, I'd very much like to see that upstreamed, even if only to stop other people from trying to reinvent that wheel. Speaking of new properties to modify key-load unit behaviour: Back in January (simpler times!), when I was working on adding the initial control properties, I was considering whether we should add similar properties to control the key-load units' dependencies individually. Seeing how my PR dragged on and got bigger and bigger (we started with two(?) properties and ended with a bunch more), I decided to postpone the idea in the name of future work and niche interest that even I myself didn't have a concrete, real use for back then and settled for a simple solution instead of more 'property explosions'. Apparently it's time to at least reconsider it. |
Indeed, we already has some changes in the generator (like invalid cache update and things hooking up with ZSys) that we want to upstream (for upstreamable one). Right now, the idea is that we have a keystore which will have all keyfile for encrypted directory. However, this keystore is made accessible for the whole system in initramfs, and the children per-user keyfile is made accessible via PAM modules, on login. Then, an automount unit will trigger the mount unit, which triggers key-load service (those are the 2 changes here: to keep a common mount and key-load service) that we BindsTo to a service that make unaccessible the keyfile.
So, you have the scheme over there. I would be glad if we can upstream this whole behavior if possible so that we pilot that and the whole property thing (I saw indeed a big expansion in 0.8.3 of them :)). The idea is really to allow a scheme where mount of encrypted dataset can be on demand, with a timeout to unmount it (which decreases the timeframe some files can be made accessible to other user/process on the system). Edit: We only modify this mount logic generation for encrypted home user datasets controlled by ZSys to limit the impact. Other datasets (encrypted or not) have the pure upstream dependencies. Note: I will be away for the next 3 weeks, which gives time for a second thought on that, but jibel should be around if any further discussion is needed. |
|
After discussing with jibel, an other way think about it without making it too easy for people to shoot in theit feet by tweaking systemd properties via zfs systems is to consider some "modes" on the dataset. For instance, let’s imagine one user property "mountmode" which would be:
That way, we control more the system relationships between units, and this is more robust in general user-wide. |
|
@behlendorf Thanks for the link! We have a mix setup for our PAM authentication (as we need to load from Luks the keyfile that are then used by native ZFS encryption), but I think we can converge the solution. We went with writing a PAM module available at https://github.com/ubuntu/kstore. The idea is to converge our ZFS and non ZFS story for encryption from a Luks store (so that you have all automation we already have from Luks through ZFS), but I think we can take and reuse part of the work made in Is there any chance to move on with this branch (our feature freeze is soon and I would like to avoid distro-patching if possible)? |
|
@didrocks thanks for reminding me about this. Yes, I don't see an issue with merging this soon unless @aerusso @rlaager or @InsanePrawn have concerns about these changes. |
behlendorf
added
Status: Accepted
We need a stronger dependency between the mount unit and its keyload unit when we know that the dataset is encrypted. If the keyload unit fails, Wants= will still try to mount the dataset, which will then fail. It’s better to show that the failure is due to a dependency failing, the keyload unit, by tighting up the dependency. We can do this as we know that we generate both units in the generator and so, it’s not an optional dependency. BindsTo enable as well that if the keyload unit fails at any point, the associated mountpoint will be then unmounted. Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes #10477
The unit was failing instead of stopping if someone manually unloaded the key before stopping the unit (zfs unload-key is failing on an unavailable key). Follow a similar logic than for loading the key, checking for the key status before unloading it. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Richard Laager <rlaager@wiktel.com> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes #10477
|
Thanks a lot! |
Drop Before=zfs.mount dependency explicity on generated key-load .service unit. Indeed, the associated mount unit is After=<dataset-key-load>.service. This is thus the mount point which controls at what point it wants to be mounted (Before=zfs-mount.service in stock generator), but this can be an automount point, or triggered by another service. This additional dependency from the key load service is not needed thus. Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes openzfs#10477
We need a stronger dependency between the mount unit and its keyload unit when we know that the dataset is encrypted. If the keyload unit fails, Wants= will still try to mount the dataset, which will then fail. It’s better to show that the failure is due to a dependency failing, the keyload unit, by tighting up the dependency. We can do this as we know that we generate both units in the generator and so, it’s not an optional dependency. BindsTo enable as well that if the keyload unit fails at any point, the associated mountpoint will be then unmounted. Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes openzfs#10477
The unit was failing instead of stopping if someone manually unloaded the key before stopping the unit (zfs unload-key is failing on an unavailable key). Follow a similar logic than for loading the key, checking for the key status before unloading it. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Richard Laager <rlaager@wiktel.com> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes openzfs#10477
Drop Before=zfs.mount dependency explicity on generated key-load .service unit. Indeed, the associated mount unit is After=<dataset-key-load>.service. This is thus the mount point which controls at what point it wants to be mounted (Before=zfs-mount.service in stock generator), but this can be an automount point, or triggered by another service. This additional dependency from the key load service is not needed thus. Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes openzfs#10477
We need a stronger dependency between the mount unit and its keyload unit when we know that the dataset is encrypted. If the keyload unit fails, Wants= will still try to mount the dataset, which will then fail. It’s better to show that the failure is due to a dependency failing, the keyload unit, by tighting up the dependency. We can do this as we know that we generate both units in the generator and so, it’s not an optional dependency. BindsTo enable as well that if the keyload unit fails at any point, the associated mountpoint will be then unmounted. Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes openzfs#10477
The unit was failing instead of stopping if someone manually unloaded the key before stopping the unit (zfs unload-key is failing on an unavailable key). Follow a similar logic than for loading the key, checking for the key status before unloading it. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Richard Laager <rlaager@wiktel.com> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes openzfs#10477
Drop Before=zfs.mount dependency explicity on generated key-load .service unit. Indeed, the associated mount unit is After=<dataset-key-load>.service. This is thus the mount point which controls at what point it wants to be mounted (Before=zfs-mount.service in stock generator), but this can be an automount point, or triggered by another service. This additional dependency from the key load service is not needed thus. Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes openzfs#10477
We need a stronger dependency between the mount unit and its keyload unit when we know that the dataset is encrypted. If the keyload unit fails, Wants= will still try to mount the dataset, which will then fail. It’s better to show that the failure is due to a dependency failing, the keyload unit, by tighting up the dependency. We can do this as we know that we generate both units in the generator and so, it’s not an optional dependency. BindsTo enable as well that if the keyload unit fails at any point, the associated mountpoint will be then unmounted. Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes openzfs#10477
The unit was failing instead of stopping if someone manually unloaded the key before stopping the unit (zfs unload-key is failing on an unavailable key). Follow a similar logic than for loading the key, checking for the key status before unloading it. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Richard Laager <rlaager@wiktel.com> Co-authored-by: Didier Roche <didrocks@ubuntu.com> Signed-off-by: Didier Roche <didrocks@ubuntu.com> Closes openzfs#10477
This set of changes makes the zfs mount generated units more effective and tight in case of encryption selected.
Motivation and Context
The goal of this PR is to make the systemd zfs generator a little bit more robust against failure, especially when encryption is involved.
Those are done in multiple ways (each in separate commits):
Description
Taking by each point in the commit message:
Make unloading the key more robust
The unit was failing instead of stopping if someone manually unloaded the key before stopping the unit (zfs unload-key is failing on an unavailable key). Follow a similar logic than for loading the key, checking for the key status before unloading it.
BindsTo dataset keyload unit to mount associate unit
We need a stronger dependency between the mount unit and its keyload unit when we know that the dataset is encrypted.
If the keyload unit fails, Wants= will still try to mount the dataset, which will then fail.
It’s better to show that the failure is due to a dependency failing, the keyload unit, by tighting up the dependency. We can do this as we know that we generate both units in the generator and so, it’s not an optional dependency. BindsTo enable as well that if the keyload unit fails at any point, the associated mountpoint will be then unmounted.
Note: Requires could be enough as this is a simple oneshot service, but if it will evolve in a real service, the relationship is better addressed that way.
Ensure mount unit pilots when its ZFS key is loaded
Drop Before=zfs.mount dependency explicity on generated load-key .service unit.
Indeed, the associated mount unit is After=.service.
This is thus the mount point which controls at what point it wants to be mounted (Before=zfs-mount.service in stock generator), but this can be an automount point, or triggered by another service.
This additional dependency from the key load service is not needed thus.
How Has This Been Tested?
We checked after a
systemctl daemon-reloadthat:.conf.d/file for wanting it.Those changes of course only impacts systemd systems with some encrypted datasets, when the user enabled the zfs mount generator via the list cache zed hook.
Types of changes
Checklist:
Signed-off-by.Note: they are indeed multiple commits, but those are small and impacts the same areas of code. We separated them for better articulations of the changes.service
The generator doesn’t have any automated tests, hence this box not checked. However, as previously explained, a lot of manual tests have been processed.
No documentation is impacted AFAIK.