zfs-mount-generator: Skip loading already loaded key #9529

vozhyk- · 2019-10-29T21:37:58Z

Don't ask for the password / try to load the key
if the key for the encryptionroot is already loaded.

Motivation and Context

This resolves #9495.

Description

Make the generated zfs-load-key-<pool>.service check if the encryptionroot's keystatus is available (meaning that the key is loaded) and return success if it is.

How Has This Been Tested?

I've tested the change manually on top of 0.8.2 (in a fork with libbe and beadm ported from illumos; commit https://gitlab.com/linux-be/zfs/commits/85ef14c587e7cf358a8434ec31fa9ed2799247e7) on the Sabayon system from #9495.

After the initramfs has loaded the key for the root pool (te), zfs-load-key-te.service does not ask for the password and does not fail:

systemctl status zfs-load-key-te.service

# systemctl status zfs-load-key-te.service
● zfs-load-key-te.service - Load ZFS key for te
   Loaded: loaded (/etc/zfs/zfs-list.cache/te; generated)
   Active: active (exited) since Thu 2019-11-07 14:37:45 CET; 5min ago
     Docs: man:zfs-mount-generator(8)
 Main PID: 40379 (code=exited, status=0/SUCCESS)
    Tasks: 0 (limit: 4915)
   Memory: 0B
   CGroup: /system.slice/zfs-load-key-te.service

Nov 07 14:37:45 vozhx-te systemd[1]: Starting Load ZFS key for te...
Nov 07 14:37:45 vozhx-te systemd[1]: Started Load ZFS key for te.

With a file-based pool (testpool), the generated zfs-load-key-testpool.service does not ask for the passphrase when the key is already loaded, but does ask for one and loads the key when it is not loaded:

results with a file-based pool

# truncate -s100M /tmp/testpool
# touch /etc/zfs/zfs-list.cache/testpool
# zpool create -O encryption=aes-256-gcm -O keyformat=passphrase testpool /tmp/testpool

Enter passphrase: 
Re-enter passphrase: 
# systemctl daemon-reload
# systemctl start zfs-load-key-testpool
# systemctl status zfs-load-key-testpool
● zfs-load-key-testpool.service - Load ZFS key for testpool
   Loaded: loaded (/etc/zfs/zfs-list.cache/testpool; generated)
   Active: active (exited) since Thu 2019-11-07 14:40:29 CET; 4s ago
     Docs: man:zfs-mount-generator(8)
  Process: 54248 ExecStart=/bin/sh -c set -eu;keystatus="$$(/sbin/zfs get -H -o value keystatus "testpool")";[ "$$keystatus" = "unavailable" ] || exit 0;count=0;while [ $$count -lt 3 ];do >
 Main PID: 54248 (code=exited, status=0/SUCCESS)

Nov 07 14:40:29 vozhx-te systemd[1]: Starting Load ZFS key for testpool...
Nov 07 14:40:29 vozhx-te systemd[1]: Started Load ZFS key for testpool.
# zfs unmount testpool
# zfs unload-key testpool
# systemctl start zfs-load-key-testpool
# systemctl restart zfs-load-key-testpool
Enter passphrase for testpool: ********
# systemctl status zfs-load-key-testpool
● zfs-load-key-testpool.service - Load ZFS key for testpool
   Loaded: loaded (/etc/zfs/zfs-list.cache/testpool; generated)
   Active: active (exited) since Thu 2019-11-07 14:40:55 CET; 3s ago
     Docs: man:zfs-mount-generator(8)
  Process: 54433 ExecStart=/bin/sh -c set -eu;keystatus="$$(/sbin/zfs get -H -o value keystatus "testpool")";[ "$$keystatus" = "unavailable" ] || exit 0;count=0;while [ $$count -lt 3 ];do >
 Main PID: 54433 (code=exited, status=0/SUCCESS)

Nov 07 14:40:51 vozhx-te systemd[1]: Starting Load ZFS key for testpool...
Nov 07 14:40:55 vozhx-te systemd[1]: Started Load ZFS key for testpool.

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.

vozhyk- · 2019-10-29T21:52:39Z

I considered using ExecCondition= in the unit instead of adding the check to ExecStart=, but, according to the manpage, failure of the check would mean the unit is not relevant for the running system, and the unit might get "garbage-collected". This is undesirable, as it might become useful once e.g. the user unloads the key for the dataset.

rlaager

LGTM

behlendorf · 2019-10-29T22:57:01Z

@aerusso you may want to have a look at this as well.

codecov · 2019-10-30T06:00:04Z

Codecov Report

Merging #9529 into master will increase coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9529      +/-   ##
==========================================
+ Coverage   79.01%   79.17%   +0.15%     
==========================================
  Files         418      416       -2     
  Lines      123686   123661      -25     
==========================================
+ Hits        97736    97909     +173     
+ Misses      25950    25752     -198

Flag	Coverage Δ
#kernel	`79.76% <ø> (ø)`	⬆️
#user	`66.93% <ø> (+0.22%)`	⬆️

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 ae38e00...fbdcd34. Read the comment docs.

aerusso · 2019-10-30T13:23:43Z

This looks good to me. I waffled back and forth about testing for unavailable vs available, i.e., maybe test for

"[ \"\$\$keystatus\" = \"unavailable\" ] || exit 0;"\

Which more narrowly proceeds only if it can actually do something about it, and succeeds otherwise (also, a cursory glance through man zfs doesn't indicate when none will be returned---is that referring to the - returned for unencrypted datasets?).

tcaputi

I'm not super familiar with systemd, but the general idea looks right to me.

rlaager

From: #9495 (comment)

This problem also exists in dracut module in the mount-zfs.sh. There should be a check included to see if zfs encryption key was already loaded.

behlendorf · 2019-11-01T20:28:27Z

@vozhyk- when you get a chance can you update the dracut script with a similar fix.

vozhyk- · 2019-11-03T17:26:52Z

"[ \"\$\$keystatus\" = \"unavailable\" ] || exit 0;"\

@aerusso Indeed, this makes more sense. This should prevent trying to load the key for an unencrypted dataset (which would also fail) if the unit is ever generated for one.

@vozhyk- when you get a chance can you update the dracut script with a similar fix.

@rlaager @behlendorf I've added similar checks to all the places dracut and initramfs tries to load a key. Always doing the check is useful, as the user might have loaded the key manually or by other means before the script gets called.

That said, I cannot test the dracut and initramfs changes, as all of my systems use genkernel for the initramfs.

rlaager

This looks good other than the tabs to spaces change needed.

contrib/dracut/90zfs/zfs-load-key.sh.in

Don't ask for the password / try to load the key if the key for the encryptionroot is already loaded. Closes openzfs#9495 Signed-off-by: Witaut Bajaryn <[email protected]>

The user might have loaded the key manually or by other means before the scripts get called. Issue openzfs#9495 Signed-off-by: Witaut Bajaryn <[email protected]>

rlaager

LGTM.

vozhyk- · 2019-11-09T14:25:40Z

Can this be added to 0.8-release?

Don't ask for the password / try to load the key if the key for the encryptionroot is already loaded. The user might have loaded the key manually or by other means before the scripts get called. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Tom Caputi <[email protected]> Reviewed-by: Richard Laager <[email protected]> Signed-off-by: Witaut Bajaryn <[email protected]> Closes openzfs#9495 Closes openzfs#9529

Don't ask for the password / try to load the key if the key for the encryptionroot is already loaded. The user might have loaded the key manually or by other means before the scripts get called. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Tom Caputi <[email protected]> Reviewed-by: Richard Laager <[email protected]> Signed-off-by: Witaut Bajaryn <[email protected]> Closes #9495 Closes #9529

behlendorf requested review from rlaager and tcaputi October 29, 2019 21:54

behlendorf added the Status: Code Review Needed Ready for review and testing label Oct 29, 2019

rlaager approved these changes Oct 29, 2019

View reviewed changes

tcaputi approved these changes Oct 30, 2019

View reviewed changes

rlaager suggested changes Oct 30, 2019

View reviewed changes

vozhyk- force-pushed the zfs-mount-generator-ignore-already-loaded-key_master branch from 76d7d4b to b50c0ec Compare November 3, 2019 17:28

rlaager suggested changes Nov 4, 2019

View reviewed changes

contrib/dracut/90zfs/zfs-load-key.sh.in Outdated Show resolved Hide resolved

vozhyk- added 2 commits November 4, 2019 10:56

zfs-mount-generator: Skip loading already loaded key

b3d3edf

Don't ask for the password / try to load the key if the key for the encryptionroot is already loaded. Closes openzfs#9495 Signed-off-by: Witaut Bajaryn <[email protected]>

dracut, initramfs: Skip loading already loaded key

fbdcd34

The user might have loaded the key manually or by other means before the scripts get called. Issue openzfs#9495 Signed-off-by: Witaut Bajaryn <[email protected]>

vozhyk- force-pushed the zfs-mount-generator-ignore-already-loaded-key_master branch from b50c0ec to fbdcd34 Compare November 4, 2019 09:57

behlendorf approved these changes Nov 6, 2019

View reviewed changes

behlendorf requested a review from rlaager November 6, 2019 19:46

rlaager approved these changes Nov 7, 2019

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 8, 2019

behlendorf merged commit 6c7023a into openzfs:master Nov 8, 2019

vozhyk- deleted the zfs-mount-generator-ignore-already-loaded-key_master branch November 9, 2019 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zfs-mount-generator: Skip loading already loaded key #9529

zfs-mount-generator: Skip loading already loaded key #9529

vozhyk- commented Oct 29, 2019 •

edited

Loading

vozhyk- commented Oct 29, 2019

rlaager left a comment

behlendorf commented Oct 29, 2019

codecov bot commented Oct 30, 2019 •

edited

Loading

aerusso commented Oct 30, 2019

tcaputi left a comment

rlaager left a comment

behlendorf commented Nov 1, 2019

vozhyk- commented Nov 3, 2019

rlaager left a comment

rlaager left a comment

vozhyk- commented Nov 9, 2019

zfs-mount-generator: Skip loading already loaded key #9529

zfs-mount-generator: Skip loading already loaded key #9529

Conversation

vozhyk- commented Oct 29, 2019 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

vozhyk- commented Oct 29, 2019

rlaager left a comment

Choose a reason for hiding this comment

behlendorf commented Oct 29, 2019

codecov bot commented Oct 30, 2019 • edited Loading

Codecov Report

aerusso commented Oct 30, 2019

tcaputi left a comment

Choose a reason for hiding this comment

rlaager left a comment

Choose a reason for hiding this comment

behlendorf commented Nov 1, 2019

vozhyk- commented Nov 3, 2019

rlaager left a comment

Choose a reason for hiding this comment

rlaager left a comment

Choose a reason for hiding this comment

vozhyk- commented Nov 9, 2019

vozhyk- commented Oct 29, 2019 •

edited

Loading

codecov bot commented Oct 30, 2019 •

edited

Loading