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.
>
> 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-initramfs
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/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
-Dov
