Skip to content

Add support for TPM- and Tang-based disk encryption #83

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.

Sign up for GitHub

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

Merged
pothos merged 1 commit into from
Mar 13, 2024
Merged

Add support for TPM- and Tang-based disk encryption #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions dracut/30ignition/coreos-metadata-wrapper
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions dracut/30ignition/ignition-wrapper
View file
Open in desktop

This file was deleted.

64 changes: 55 additions & 9 deletions dracut/30ignition/module-setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,58 @@ install() {
# Flatcar: add 66-azure-storage.rules and 90-cloud-storage.rules
inst_rules 60-cdrom_id.rules 66-azure-storage.rules 90-cloud-storage.rules

# Flatcar: add wrappers for Ignition and coreos-metadata (afterburn)
# which rely on the earlier /sysusr/usr mount
inst_script "$moddir/coreos-metadata-wrapper" \
"/usr/bin/coreos-metadata"
inst_script "$moddir/ignition-wrapper" \
"/usr/bin/ignition"
inst_script "$moddir/setfiles-wrapper" \
"/usr/bin/setfiles"
}
# Flatcar: add symlinks for dependencies of Ignition, coreos-metadata (afterburn), and
# Clevis. This saves space in the initramfs image by replacing files with symlinks to
# the previously mounted /sysusr/.
for executable in \
/usr/bin/clevis-decrypt-sss \
/usr/bin/clevis-decrypt-tang \
/usr/bin/clevis-decrypt-tpm2 \
/usr/bin/clevis-decrypt \
/usr/bin/clevis-encrypt-sss \
/usr/bin/clevis-encrypt-tang \
/usr/bin/clevis-encrypt-tpm2 \
/usr/bin/clevis-luks-bind \
/usr/bin/clevis-luks-common-functions \
/usr/bin/clevis-luks-list \
/usr/bin/clevis-luks-unlock \
/usr/bin/clevis \
/usr/bin/coreos-metadata \
/usr/bin/curl \
/usr/bin/ignition \
/usr/bin/jose \
/usr/bin/luksmeta \
/usr/bin/mktemp \
/usr/bin/pwmake \
/usr/bin/sort \
/usr/bin/tail \
/usr/bin/tpm2_createprimary \
/usr/bin/tpm2_create \
/usr/bin/tpm2_flushcontext \
/usr/bin/tpm2_load \
/usr/bin/tpm2_pcrlist \
/usr/bin/tpm2_pcrread \
/usr/bin/tpm2_unseal \
/usr/lib/systemd-reply-password \
/usr/local/libexec/clevis-luks-askpass \
Copy link
Contributor

@ader1990 ader1990 Mar 5, 2024

Choose a reason for hiding this comment

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

See the note from the upstream module: https://github.com/coreos/ignition/blob/main/dracut/30ignition/module-setup.sh, more precisely https://github.com/coreos/ignition/blob/main/dracut/30ignition/module-setup.sh#L49. Can we reuse the upstream clevis module for this purpose in order to not diverge from the upstream?

Copy link
Member

@pothos pothos Mar 7, 2024

Choose a reason for hiding this comment

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

Or we could add a comment here that points to the upstream clevis module to have a chance of finding divergence more easily .
If we import the clevis module we would still add the same contents we have here, I'm both ok with having it imported and patched or with having things here.

Copy link
Author

@simoncampion simoncampion Mar 9, 2024

Choose a reason for hiding this comment

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

If we import the clevis module

To clarify, the clevis module for dracut from app-crypt/clevis is included in the initramfs. One way to see this is that the module 40clevis-unlock currently lists it as a dependency here. Unfortunately, the clevis module will not unlock disks in Flatcar's initramfs though as explained in this comment. That is why we have our own 40clevis-unlock dracut module in addition the clevis module. The lines of code you commented on replace files installed by the clevis module, such as /usr/bin/tpm2_unseal, with symlinks to the same files under /sysusr/. That is, we can remove the symlinking logic here and things will work just fine. My understanding was that replacing files with symlinks was meant to avoid blowing up the initramfs image unnecessarily, given that the files we need are in /sysusr/ anyways. But I started working on this PR after the symlinking had been introduced and maybe I am misunderstanding and the symlinking was originally introduced under the assumption that the clevis module would not be installed, in which case the symlinks would be essential for the unlocking to work.

Some options on how to proceed:

  1. keep both clevis and 40clevis-unlock as they currently are, and add a comment noting that we diverge from upstream 30ignition here
  2. keep both clevis and 40clevis-unlock as they currently are, but do not overwrite the real files with symlinks, instead using the files from the clevis module; this would keep us us closer to upstream 30ignition (if I understood correctly, this was @ader1990's suggestion)
  3. do not install the clevis module and instead make sure that all files that 40clevis-unlock depends on are present in /sysusr/usr and symlinked
  4. do not install 40clevis-unlock and instead extend the Flatcar patch of app-crypt/clevis so that the clevis module manages to unlock disks on its own by registering a dracut hook

I do not have particularly strong opinions on how to proceed, so let me know which option you prefer. I am not sure how straightforward option 4 would be, and once we use a dracut hook rather than our own systemd unit for unlocking, we might not be able to switch off initramfs unlocking using the kernel cmdline (without further patching of app-crypt/clevis).

Copy link
Member

@pothos pothos Mar 10, 2024

Choose a reason for hiding this comment

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

Option 1 sounds ok, or? We should keep the symlinks for reducing the initrd size impact of this addition. About the systemd-cryptsetup integration: I think that can also be triggered through a luks.uuid= cmdline parameter instead of /etc/crypttab - maybe it would work to write this out to grub.cfg through a helper? Jeremi also noted that https://www.freedesktop.org/software/systemd/man/latest/systemd-gpt-auto-generator.html exists and could work. Anyway, I think the current way is ok even if we iterate on it. Once we document things we ideally shouldn't change them in a backwards-incompatible way anymore, though.

Copy link
Author

@simoncampion simoncampion Mar 11, 2024

Choose a reason for hiding this comment

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

Re symlinks: Agreed. I will add a comment explaining that we diverge from 30ignition by symlinking to save space. I will also check that we really symlink everything we can.

Re how to unlock in initramfs: I like the idea of using rd.luks.uuid/name or rd.auto to trigger disk unlock through systemd-cryptsetup in the initramfs. I tried setting these cmdline parameter instead of rd.clevis_unlock on an image built on the current version in this PR, and the initramfs unlock worked fine.

Suggestion:

  • Remove the custom rd.clevis_unlock parameter
  • Remove the custom initramfs systemd unit to unlock disks
  • Keep the generator that enables systemd-networkd, but rename the parameter to something more general, such as rd.network=1, and make the generator add a dependency from dracut-initqueue (or some other unit that runs early enough) to systemd-networkd, systemd-resolved, etc.
  • For now, do not automatically add the command line parameters for initramfs unlocking to grub.cfg. Instead, require the appropriate parameters to be set in the Ignition config by the user.

I'm pretty sure I can push those changes until tomorrow end of day, so it shouldn't delay the merge too much. I think this approach is better than the current approach because

  • we stick to standard rd.* parameters rather than introducing a Flatcar-specific rd.clevis_unlock parameter,
  • we get rid of the custom systemd unit in our initramfs, so less code for us to maintain,
  • the current solution also requires the user to set command line arguments to enable initramfs unlock, so nothing is lost through the proposed switch to rd.* params

Copy link
Member

@pothos pothos Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With an encrypted rootfs it didn't work, and I still had to pass luks.uuid= or it would hang.

The problem seems to be that the gpt auto generator only runs as "late" generator.
Edit: Maybe this is the problem:

This generator will only look for the root partition on the same physical disk where the EFI System Partition (ESP) is located. Note that support from the boot loader is required: the EFI variable LoaderDevicePartUUID
       of the 4a67b082-0a4c-41cf-b6c7-440b29bb8c4f vendor UUID is used to determine from which partition, and hence the disk, from which the system was booted. If the boot loader does not set this variable, this generator
       will not be able to detect the root partition. See the Boot Loader Interface[3] for detail

But it looks like it might work once we have this bli module https://wiki.archlinux.org/title/GRUB#LoaderDevicePartUUID
But root= must also not be specified on the cmdline.
… so I guess we could write a custom generator that runs the systemd gpt auto generator with the right environment in a mount namespace

Copy link
Member

@pothos pothos Mar 11, 2024

Choose a reason for hiding this comment

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

Thinking about it instead of adding a hack to run the gpt auto generator we could have a custom generator that that looks up the luks UDID based on the by-partlabel/ROOT entry and then runs the systemd-cryptsetup-generator with a kernel cmdline bind-mounted in a mount namespace

Copy link
Author

@simoncampion simoncampion Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I pushed commits that implement the switch to using the systemd-cryptsetup unlocking in the initramfs and rd.* arguments, as described here.

From the user's perspective, here's how it works:

  • for non-root disks, the user simply adds this to their Butane config:
luks:
- name: dataencrypted
  device: "/dev/disk/by-partlabel/data"
  clevis:
    tpm2: true
  • for root disks, the user must additionally set the UUID of the LUKS device and add a kernel parameter to trigger unlock in the initramfs, and, if network-bound encryption is used, a kernel parameter to start systemd-networkd in the initramfs:
kernel_arguments:
  should_exist:
  - rd.luks.name=12345678-9abc-def0-1234-56789abcdef0=rootencrypted
  - rd.networkd=1
luks:
- name: rootencrypted
  device: "/dev/disk/by-partlabel/root"
  uuid: 12345678-9abc-def0-1234-56789abcdef0
  clevis:
    tang:
      - url: "http://tang.mycompany.com"
        thumbprint: "Hk1VN2eKhzaVqWhXtXxXIGF5LRZt4cBWWb07I1-a0NX"

See also the changes to the automated tests in the mantle repository that I just pushed here. These tests pass with the new version of the bootengine.

From an implementation perspective, the only bootengine code we now add is the generator that implements adding a dependency on systemd-networkd etc. if rd.networkd=1 is set. I deleted the custom unlock service.

Personally, I feel better about this solution than about the previous one; I think this solution is user-friendly and adds less complexity on the Flatcar side, by reusing systemd-cryptsetup.

From my perspective, the only thing left to do (other than getting the initramfs image size under control---see the thread below) is to consider removing the need for the user to set kernel_arguments in their Butane for root disk encryption. I think it'd probably be okay to merge as is and then later remove the need for kernel_arguments as an optimization, but I'd also be happy to push another commit that adds this optimization before we merge. Let me know what you think.

Regarding how to optimize, what you suggest would be a way to solve the problem, I think:

we could have a custom generator that that looks up the luks UDID based on the by-partlabel/ROOT entry and then runs the systemd-cryptsetup-generator with a kernel cmdline bind-mounted in a mount namespace

I'm not sure how big of a problem that is, but note that this solution would not work for Butane configs that do not label their root partition at all or label it something other than ROOT. As far as I know, it is no requirement to use this label for the root partition. Of course, we could document that users need to use this label for unlocking in the initramfs to work.

Copy link
Member

@pothos pothos Mar 12, 2024

Choose a reason for hiding this comment

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

Thank you, that looks good.

The partition label already is ROOT and won't change, when the partition is reformatted with luks, or? In any case it's easy to require using a partition label and it's nicer than having to provide the uuid upfront. Anyway, I think this is a follow-up improvement.

Copy link
Author

@simoncampion simoncampion Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The partition label already is ROOT and won't change, when the partition is reformatted with luks, or?

I was thinking about the case where the root partition is moved to a new partition on a disk other than the OS disk, e.g. as described here in the documentation. In the case where the root partition on the OS disk is used, you are right.

In any case, I 100% agree with you that we could just require the user to use this partition label.

/usr/libexec/clevis-luks-generic-unlocker \
/usr/sbin/setfiles \
; do
directory="$(dirname "$executable")"
filename="$(basename "$executable")"

wrapper_name="${filename}-wrapper"
cat <<EOF > /tmp/${filename}-wrapper
#!/bin/sh

LD_LIBRARY_PATH=/sysusr/usr/lib64 exec "/sysusr${executable}" "\$@"
EOF
chmod +x /tmp/${filename}-wrapper

inst_script "/tmp/${filename}-wrapper" \
"/usr/bin/$filename"

rm /tmp/${filename}-wrapper
done

}
2 changes: 0 additions & 2 deletions dracut/30ignition/setfiles-wrapper
View file
Open in desktop

This file was deleted.

10 changes: 10 additions & 0 deletions dracut/40networkd-dependency/module-setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/bin/bash

depends() {
echo systemd
}

install() {
inst_simple "$moddir/networkd-dependency-generator" \
"$systemdutildir/system-generators/networkd-dependency-generator"
}
17 changes: 17 additions & 0 deletions dracut/40networkd-dependency/networkd-dependency-generator
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#!/bin/bash

set -e

UNIT_DIR="$1"

if cat /proc/cmdline | grep -qw "rd.networkd=1"; then
mkdir -p "${UNIT_DIR}/dracut-initqueue.service.d"

cat >${UNIT_DIR}/dracut-initqueue.service.d/networkd_dependency.conf <<EOF
# Automatically generated by networkd-dependency-generator

[Unit]
Wants=systemd-networkd.service systemd-resolved.service network-online.target
After=systemd-networkd.service systemd-resolved.service network-online.target
EOF
fi