Re: [libvirt PATCH v2 09/12] tools: support generating SEV secret injection tables

On 26/10/2022 12:59, Daniel P. Berrangé wrote: > On Tue, Oct 25, 2022 at 07:38:43PM -0400, Cole Robinson wrote: >> On 10/19/22 6:17 AM, Daniel P. Berrangé wrote: >>> It is possible to build OVMF for SEV with an embedded Grub that can >>> fetch LUKS disk secrets. This adds support for injecting secrets in >>> the required format. >>> >>> Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; >>> --- >> >>> diff --git a/tools/virt-qemu-sev-validate b/tools/virt-qemu-sev-validate >>> index 5ce5763d5b..2d15edb933 100755 >>> --- a/tools/virt-qemu-sev-validate >>> +++ b/tools/virt-qemu-sev-validate >>> @@ -36,16 +36,19 @@ >>> >>> import abc >>> import argparse >>> -from base64 import b64decode >>> +from base64 import b64decode, b64encode >>> from hashlib import sha256 >>> import hmac >>> import logging >>> +import os >>> import re >>> import socket >>> from struct import pack >>> import sys >>> import traceback >>> from uuid import UUID >>> +from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes >>> + >>> >>> from lxml import etree >>> import libvirt >>> @@ -573,7 +576,26 @@ class KernelTable(GUIDTable): >>> return entries >>> >>> >>> -class ConfidentialVM(object): >>> +class SecretsTable(GUIDTable): >>> + >>> + TABLE_GUID = UUID('{1e74f542-71dd-4d66-963e-ef4287ff173b}').bytes_le >>> + DISK_PW_GUID = UUID('{736869e5-84f0-4973-92ec-06879ce3da0b}').bytes_le >>> + >>> + def __init__(self): >>> + super().__init__(guid=self.TABLE_GUID, >>> + lenlen=4) >>> + self.disk_password = None >>> + >>> + def load_disk_password(self, path): >>> + with open(path, 'rb') as fh: >>> + self.disk_password = fh.read() >>> + >>> + def entries(self): >>> + return self.build_entry(self.DISK_PW_GUID, >>> + self.disk_password + bytes([0]), 4) >>> + >> >> This bytes([0]) NUL byte ends up in the efi_secret /sys path. Dropping >> it doesn't seem to impact injecting the secret at all > > The specs/sev-guest-firmware.rst files does not specify anything in > particular about the injected secret format. The rules for the format > will vary according to the GUID used for the entry. In this case what > I've called the DISK_PW_GUID is the same as the GUID used in James > Bottomley's grub patch: > > https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00260.html > > In particular note > > + /* > + * the passphrase must be a zero terminated string because the > + * password routines call grub_strlen () to find its size > + */ > > As written, the code just passes around a pointer to the data in the > secret table, so it can't simply add a NUL terminator itself. > > See also James' python demo script for injection which adds a NUL: > > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03691.html > >> FWIW once that's dropped, getting automatic luks unlock is really simple >> with /etc/crypttab + kernel 5.19 >> >> sed -i -e "s| none | >> /sys/kernel/security/secrets/coco/736869e5-84f0-4973-92ec-06879ce3da0b >> |g" /etc/crypttab >> dracut --force --add-drivers efi_secret >> shutdown -r now I'm not an initrd expert, but maybe there's a way to run an early script (before cryptsetup) that will peel that terminating NUL: head -c -1 /sys/kernel/security/secrets/coco/736869e5-84f0-4973-92ec-06879ce3da0b > /tmp/luks_key and then use /tmp/luks_key in /etc/crypttab. Of course /tmp/luks_key isn't a secure place, so need to find the correct location for this.

> Now one option here is to invent a different GUID and call it the > "CRYTPTAB_PW_GUID", and have the host side injection tools not use > a NUL terminator for this option. and then modify the Grub patch (option 1 below) to read from the new GUID, copy the value, and add the NUL byte. > > What I don't like about this is that in both cases we're just passing > in the same data - a passphrase for unlocking a LUKS keyslot. > > IMHO, we should have a "LUKS_PASSPHARSE_GUID" secret entry which can > be consumed by any software that wants to unlock LUKS, not have > software specific GUIDs. > > The crypttab keyfiles can contain arbitrary bytes, so we can't just > drop the trailing NUL as that might be relevant key material. > Note that grub and cryptsetup are not identical -- I think that (currently) the grub patches do not support binary passphrases (that may include NUL bytes), whereas cryptsetup keyfile can be have binary content. But I'm not following grub-devel closely.

> Reading cryttab(5) I see that /etc/crypttab supports an option > called 'keyfile-size', which could be used to truncate the file. > That's not at all user friendly for us, as the user pw data can be > arbitrary length and when building the disk image template we > don't know how many bytes are sensitive.IOW, we can't predict > what keyfile-size to use. > > So it seems we have a few options > > 1. modify grub patches to remove the requirement for NUL > termination > 2. modify systemd to add a 'keyfile-nul-terminated' option > to instruct it to stip a trailing NUL from key-file. > 3. modify systemd to add a 'efi-secret=UUID' option and > ignore key-file field entirely Debian's crypttab supports keyscript=/path/to/script which should output the passphrase to stdout. This is very generic and can allow us to: 1. check a few GUIDs (fallback) 2. drop terminating NUL 3. for SNP-style late attestation: fetch attestation report, contact the KBS, and retrieve the key (see AMD's example [1]) [1] https://github.com/AMDESE/sev-guest/tree/main/attestation/cryptsetup-init... But there's no keyscript option in systemd...

> Incidentally I think we should document well known secret GUIDs > in specs/sev-guest-firmware.rst. The only others I know are used in confidential-containers attestation agent (offline_sev_kbc [2] and online_sev_kbc [3]). [2] https://github.com/confidential-containers/attestation-agent/blob/6e4a249... [3] https://github.com/confidential-containers/attestation-agent/blob/6e4a249...