On Wed, Oct 26, 2022 at 03:34:00PM +0300, Dov Murik wrote:
>
>
> 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@xxxxxxxxxx>
> >>> ---
> >>
> >>> 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.
I'm slightly adverse to adding custom scripts, because I find it very
compelling for the LUKS unlock solution to be something that is possible
using only stuff in the base systemd implementation. That gives us a
guarantee that it will work in any distro that supports systemd (of a
new enough version). This would imply that custom script would need
to be added to systemd, at which point we might as well just avoid the
script.
> > 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.
Hmm, yes, the problem is more general than just the trailing
NUL. Machine generated recovery keys can contain arbitrary
embedded NULs, which is fundamentally incompatible with the
grub patches today.
So I guess we really do need to invent a new GUID explicitly
for LUKS keys supporting the full 8-bit binary space, and
document at the existing Grub targetted GUID is a limited
design.
> > 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-initramfs
>
> But there's no keyscript option in systemd...
The keyfile path can, however, point to a AF_UNIX socket path.
So you can have a script on the other end of the path that does
anything at all. Still as per my earlier point, I find it very
appealing to have a solution not reliant on us writing an
extension script.
So I'm again inclined to change my mind and say we SHOULD invent
a new GUID for full 8-bit binary LUKS unlock keys, distinct from
the previous grub GUID.
> > 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/6e4a249dfbf6fae4a0ec636ae2fac8c0aa368467/src/kbc_modules/offline_sev_kbc/mod.rs#L17
> [3] https://github.com/confidential-containers/attestation-agent/blob/6e4a249dfbf6fae4a0ec636ae2fac8c0aa368467/src/kbc_modules/online_sev_kbc/mod.rs#L28
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
