Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add zfskeys script from and for FreeBSD #12444

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ltning
Copy link

@ltning ltning commented Jul 28, 2021

Note: The below is mostly CCed from the FreeBSD commit at cgit.freebsd.org

Motivation and Context

ZFS in FreeBSD 13 supports encryption, but for the use case where keys are available in plaintext on disk there is no mechanism for automatically loading keys on startup.

Description

This script will, by default, look for any dataset with encryption and keylocation prefixed with file://. It will attempt to unlock, timing
out after 10 seconds for each dataset found. User can optionally specify explicitly which datasets to attempt to unlock.

Also supports (optionally by force) unmounting filesystems and unloading associated keys.

How Has This Been Tested?

This has been tested in various in-house environments, preproduction and production, with a mix of non-encrypted, plaintext-key-on-disk-encrypted and offline-key-encrypted datasets. Has also been tested in combination with GELI-encrypted pools, making sure the startup sequence is correct.

Script itself has been subjected to shellcheck, and is already accepted upstream in FreeBSD.

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)
  • Platform-specific supporting scripts

Checklist:

Signed-off-by: Eirik Øverby <ltning-github@anduin.net>
@ghost ghost added this to In progress in FreeBSD via automation Aug 3, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will you be taking care of removing the duplicate from FreeBSD and switching the build to use this copy instead?

Let's put this in etc/rc.d instead, to line up with etc/init.d and hook it up for local installs from the repo as well. We might as well include the other zfs-related rc scripts at that point, though perhaps excluding zfsd (which should probably move to this repo at some point as well).

@ltning
Copy link
Author

ltning commented Aug 3, 2021

Will you be taking care of removing the duplicate from FreeBSD and switching the build to use this copy instead?

It was @allanjude who asked me to open this PR here; I'm just a lowly contributor with no commit bits so I think that's a discussion for others than me :)

Let's put this in etc/rc.d instead, to line up with etc/init.d and hook it up for local installs from the repo as well. We might as well include the other zfs-related rc scripts at that point, though perhaps excluding zfsd (which should probably move to this repo at some point as well).

Yes, fwiw I think it should be all (the rc scripts) or nothing. However (and related to the previous point) I'm uncertain if it should be done now or aligned with some future FreeBSD release; in the meantime having them exist both places may be good for the other (Free)BSD siblings who may or may not be using the OpenZFS port rather than what's in base - and in case changes are made here that base isn't yet ready for.

@ghost
Copy link

ghost commented Aug 3, 2021

Now is the perfect time IMO. Better to make the switch when we're not near an impending release.

@tonynguien tonynguien added the Status: Code Review Needed Ready for review and testing label Aug 5, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be moved into etc/rc.d and hooked into the build so it gets installed by make install. You may add other rc scripts as well, if necessary, but probably at this point most people on FreeBSD are running a version that includes scripts that more or less work already.

@ghost ghost added Status: Revision Needed Changes are required for the PR to be accepted and removed Status: Code Review Needed Ready for review and testing labels Sep 13, 2022
@grahamperrin
Copy link
Contributor

Additionally, from the Userland Configuration Changes section of FreeBSD 13.2-RELEASE Release Notes:

The zfskeys startup script supports autoloading of keys stored on ZFS. 2411090f6940 (Sponsored by Klara Inc.)

Context (seeking zfskeys in the releng/13.2 branch):

0a4f7dbd9e94 timeout: Move from /usr/bin to /bin
2411090f6940 zfskeys: Support autoloading of keys stored on ZFS
7584921e6d33 RELNOTES: Note addition of zfskeys
e8baa0024251 rc.conf: Document zfskeys
73f55c5a38bf etc/defaults/rc.conf: set default of zfskeys_enable to NO
4a10f984d8e5 OptionalObsoleteFiles.inc: Add rc.d/zfskeys
c88c1274ca0c rc: Hook zfskeys to the build
e81b2348d210 Add zfskeys rc.d script for auto-loading encryption keys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Revision Needed Changes are required for the PR to be accepted
Projects
FreeBSD
In progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants