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

zfs: create zfs_data pool with LUKS encryption #940

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vunnyso
Copy link
Contributor

@vunnyso vunnyso commented Dec 20, 2024

Description of changes

With this patch we create zfs_data pool with LUKS encryption enabled and modify postBostCommands accordingly.

Checklist for things done

  • Summary of the proposed changes in the PR description
  • More detailed description in the commit message(s)
  • Commits are squashed into relevant entities - avoid a lot of minimal dev time commits in the PR
  • Contribution guidelines followed
  • Ghaf documentation updated with the commit - https://tiiuae.github.io/ghaf/
  • PR linked to architecture documentation and requirement(s) (ticket id)
  • Test procedure described (or includes tests). Select one or more:
    • Tested on Lenovo X1 x86_64
    • Tested on Jetson Orin NX or AGX aarch64
    • Tested on Polarfire riscv64
  • Author has run make-checks and it passes
  • All automatic Github Action checks pass - see actions
  • Author has added reviewers and removed PR draft status
  • Change requires full re-installation
  • Change can be updated with nixos-rebuild ... switch

Instructions for Testing

  • List all targets that this applies to:
  • Is this a new feature
    • List the test steps to verify:
  • If it is an improvement how does it impact existing functionality?

Difference

Branch ZFS and disk details
main image
This PR image

Test steps

  1. Make sure now there are two pools called as zfs_root and zfs_data and there is NO zfspool using below command.

      [ghaf@ghaf-host:~]$ zpool list 
      NAME       SIZE  ALLOC   FREE  CKPOINT  EXPANDSZ   FRAG    CAP  DEDUP    HEALTH  ALTROOT
      zfs_data   396G  1.13M   396G        -         -     0%     0%  1.00x    ONLINE  -
      zfs_root  29.5G  6.80G  22.7G        -         -     0%    23%  1.00x    ONLINE  -
    
  2. To verify zfs_data is luks encrypted run below command

     [ghaf@ghaf-host:~]$ sudo nix-shell -p cryptsetup --run "cryptsetup status zfs_data" 
      /dev/mapper/zfs_data is active and is in use.
        type:    LUKS2
        cipher:  aes-xts-plain64
        keysize: 512 bits
        key location: keyring
        device:  /dev/sda5
        sector size:  512
        offset:  32768 sectors
        size:    833105935 sectors
        mode:    read/write
    

Improvements

* With this PR everytime user need to enter password when system boots.
* Change decryption mechanism to Yubikey/TPM.

@vunnyso vunnyso temporarily deployed to internal-build-workflow December 20, 2024 08:28 — with GitHub Actions Inactive
@vunnyso vunnyso requested a review from Mic92 December 20, 2024 08:29
@vunnyso vunnyso temporarily deployed to internal-build-workflow December 20, 2024 08:38 — with GitHub Actions Inactive
pool = "zfs_root";
};
};
zfs_data = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you are still using this post boot script, than this should be just an empty partition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If using post boot script, we keep empty partition and we do something similar to this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. We need to detect if the device already has luks headers and if only format if this is not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed, now we have better control on password with post boot script.
My only concern is once I use luksFormat on zfs_data partition all zfs meta data (pools, datasets) info is lost, because of that I need to create pool and datasets again in post boot script.

With this patch we create zfs_data pool with LUKS
encryption enabled and modify postBostCommands
accordingly.

Signed-off-by: Vunny Sodhi <[email protected]>
Signed-off-by: Vunny Sodhi <[email protected]>
@vunnyso vunnyso temporarily deployed to internal-build-workflow January 2, 2025 12:42 — with GitHub Actions Inactive
@vunnyso vunnyso requested a review from unbel13ver January 3, 2025 07:52
@vunnyso vunnyso marked this pull request as ready for review January 7, 2025 08:04
@vunnyso vunnyso requested a review from brianmcgillion January 7, 2025 08:04
Comment on lines +57 to +58
echo -n $pswd | cryptsetup luksFormat --type luks2 -q "$ZFS_LOCATION"
echo -n $pswd | cryptsetup luksOpen "$ZFS_LOCATION" "$ZFS_POOLNAME" --persistent
Copy link
Collaborator

@Mic92 Mic92 Jan 8, 2025

Choose a reason for hiding this comment

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

We can actually generate password randomly because we can use the TPM to unlock the disk.

# Create pool, datasets as luksFormat will erase pools, ZFS datasets stored on that partition
zpool create -o ashift=12 -O compression=lz4 -O acltype=posixacl -O xattr=sa -f "$ZFS_POOLNAME" /dev/mapper/"$ZFS_POOLNAME"
zfs create -o quota=30G "$ZFS_POOLNAME"/vm_storage
zfs create -o quota=10G -o mountpoint=none "$ZFS_POOLNAME"/reserved
Copy link
Collaborator

@Mic92 Mic92 Jan 8, 2025

Choose a reason for hiding this comment

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

What is this used for? If one wants to reserve space, there is refreservation. It's a zfs property

@@ -44,6 +44,10 @@
boot = {
initrd.availableKernelModules = [ "zfs" ];
supportedFilesystems = [ "zfs" ];
zfs.extraPools = [ "zfs_data" ];
initrd.luks.devices.zfs_data = {
device = "/dev/disk/by-partlabel/disk-disk1-zfs_data";
Copy link
Collaborator

@Mic92 Mic92 Jan 8, 2025

Choose a reason for hiding this comment

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

I think you also need this for tpm decryption:

Suggested change
device = "/dev/disk/by-partlabel/disk-disk1-zfs_data";
device = "/dev/disk/by-partlabel/disk-disk1-zfs_data";
crypttabExtraOpts = [ "tpm2-device=auto" ];

@Mic92
Copy link
Collaborator

Mic92 commented Jan 8, 2025

For encrypted swap we could also do the same tpm encryption dance.
It's documented here: https://wiki.archlinux.org/title/Dm-crypt/Swap_encryption#Using_a_TPM
man systemd-hibernate-resume-generator will check for a resume= on the kernel command line (https://www.freedesktop.org/software/systemd/man/latest/systemd-hibernate-resume-generator.html)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants