ZFS: Request credentials only for selected pools #93395
Conversation
| ${optionalString cfgZfs.requestEncryptionCredentials '' | ||
| ${optionalString (if isBool cfgZfs.requestEncryptionCredentials | ||
| then cfgZfs.requestEncryptionCredentials | ||
| else cfgZfs.requestEncryptionCredentials != []) '' | ||
| ${packages.zfsUser}/sbin/zfs list -rHo name,keylocation ${pool} | while IFS=$'\t' read ds kl; do | ||
| (case "$kl" in | ||
| (${if isBool cfgZfs.requestEncryptionCredentials | ||
| then "" # requestEncryptionCredentials was true, so we loop over all (we insert no check) | ||
| else '' | ||
| if ! echo '${concatStringsSep cfgZfs.requestEncryptionCredentials "\n"}' | grep -qx "$ds"; then | ||
| continue | ||
| fi | ||
| ''} | ||
| case "$kl" in |
There was a problem hiding this comment.
I haven't tested this bit yet, because I only have a single pool and this only runs for non-root pools. I plan to do this in the next few days in a VM.
There was a problem hiding this comment.
I have tested this in a VM with one extra pool and it works well. I've also had a look at nixos/tests/zfs.nix, whether there is an easy way to test this functionality but it doesn't look like it is possible to perform reboots of the test VMs.
There was a problem hiding this comment.
It is actually possible
nixpkgs/nixos/tests/consul.nix
Line 161 in 5507932
Do you want to implement that?
There was a problem hiding this comment.
There is shutdown() as well as start().
hmenke
force-pushed
the
zfs
branch
2 times, most recently
from
6cd5d09
to
6b5134e
Compare
|
@Mic92 Thanks again for your input. This is now ready for the next round of review (or merge). |
|
All those should not exercise encryption though: @GrahamcOfBorg test zfs.stable zfs.unstable zfs.installer |
This change introduces more fine-grained requestEncryptionCredentials. While previously when requestEncryptionCredentials = true, the credentials for all imported pools and all datasets in these imported pools were requested, it is now possible to select exactly the pools and datasets for which credentials should be requested. It is still possible to set requestEncryptionCredentials = true, which continues to act as a wildcard for all pools and datasets, so the change is backwards compatible.
| ${packages.zfsUser}/sbin/zfs list -rHo name,keylocation ${pool} | while IFS=$'\t' read ds kl; do | ||
| (case "$kl" in | ||
| (${optionalString (!isBool cfgZfs.requestEncryptionCredentials) '' | ||
| if ! echo '${concatStringsSep "\n" cfgZfs.requestEncryptionCredentials}' | grep -qFx "$ds"; then |
There was a problem hiding this comment.
I just realized another corner case that I have fixed now. We need the -F flag for grep to not interpret the string as a regex because we only want exact matches. It is unlikely that users have . or other special characters in their dataset names, but they are allowed by ZFS and we should account for that.
There was a problem hiding this comment.
I feel like this shouldn't be done with grep like this. Instead, it seems like we ought to have two different code blocks: One for if it's a bool, and one for if it's a list. If it's a bool, then use the old code. If it's a list, then pluck all the elements of the list whose names begin with "${pool}/" and put load-key commands for each of them. This can be done at Nix eval time without annoying bash-isms, and doesn't require regex'ing every dataset against every list element on boot.
There was a problem hiding this comment.
@ElvishJerricco I don't think that would work. you still need to figure out at runtime if systemd-ask-password can be used.
There was a problem hiding this comment.
After thinking a bit about it using grep here is the method that requires the least amount of code.
This change introduces more fine-grained requestEncryptionCredentials.
While previously when requestEncryptionCredentials = true, the
credentials for all imported pools and all datasets in these imported
pools were requested, it is now possible to select exactly the pools and
datasets for which credentials should be requested.
It is still possible to set requestEncryptionCredentials = true, which
continues to act as a wildcard for all pools and datasets, so the change
is backwards compatible.
Motivation for this change
Fixes #93389
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)