[virt-tools-list] [virt-install PATCH 2/7] virtinst: cli: Introduce parser support for SEV launch security
Erik Skultety
eskultet at redhat.com
Mon Jun 10 07:40:47 UTC 2019
On Thu, Jun 06, 2019 at 04:26:27PM -0400, Cole Robinson wrote:
> On 6/6/19 6:00 AM, Erik Skultety wrote:
> > Introduce both the launchSecurity XML and parser classes. While at it,
> > add launchSecurity as a property instance to the Guest class too.
> >
> > The parser requires the 'type' argument to be mandatory since in the
> > future it will determine different code paths, therefore
> > '--launch-security foo=bar' is incorrect.
> >
>
> Is there anything spec'd out what other type= XML will look like? I'm
> curious
So the initial proposal from Intel on their MKTM contains the following:
<launchSecurity type='mktme'>
<id>m0</id>
<key_type>user</key_type>
<key>samplekey</key>
<encryption_algorithm>aes-xts-128</encryption_algorithm>
</launchSecurity>
...but the patches haven't been reviewed, essentially an RFC only.
>
> > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > ---
> > ...nstall-x86_64-launch-security-sev-full.xml | 63 +++++++++++++++++++
> > tests/clitest.py | 5 ++
> > virtinst/cli.py | 39 ++++++++++++
> > virtinst/domain/__init__.py | 1 +
> > virtinst/domain/launch_security.py | 24 +++++++
> > virtinst/guest.py | 3 +-
> > 6 files changed, 134 insertions(+), 1 deletion(-)
> > create mode 100644 tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev-full.xml
> > create mode 100644 virtinst/domain/launch_security.py
> >
> > diff --git a/tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev-full.xml b/tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev-full.xml
> > new file mode 100644
> > index 00000000..5b87a621
> > --- /dev/null
> > +++ b/tests/cli-test-xml/compare/virt-install-x86_64-launch-security-sev-full.xml
> > @@ -0,0 +1,63 @@
> > +<domain type="kvm">
> > + <name>foobar</name>
> > + <uuid>00000000-1111-2222-3333-444444444444</uuid>
> > + <memory>65536</memory>
> > + <currentMemory>65536</currentMemory>
> > + <vcpu>1</vcpu>
> > + <os>
> > + <type arch="x86_64" machine="q35">hvm</type>
> > + <loader readonly="yes" type="pflash">/usr/share/edk2/ovmf/OVMF_CODE.fd</loader>
> > + <boot dev="hd"/>
> > + </os>
> > + <features>
> > + <acpi/>
> > + <apic/>
> > + <vmport state="off"/>
> > + </features>
> > + <cpu mode="host-model"/>
> > + <clock offset="utc">
> > + <timer name="rtc" tickpolicy="catchup"/>
> > + <timer name="pit" tickpolicy="delay"/>
> > + <timer name="hpet" present="no"/>
> > + </clock>
> > + <pm>
> > + <suspend-to-mem enabled="no"/>
> > + <suspend-to-disk enabled="no"/>
> > + </pm>
> > + <devices>
> > + <emulator>/usr/bin/qemu-kvm</emulator>
> > + <controller type="usb" index="0" model="ich9-ehci1"/>
> > + <controller type="usb" index="0" model="ich9-uhci1">
> > + <master startport="0"/>
> > + </controller>
> > + <controller type="usb" index="0" model="ich9-uhci2">
> > + <master startport="2"/>
> > + </controller>
> > + <controller type="usb" index="0" model="ich9-uhci3">
> > + <master startport="4"/>
> > + </controller>
> > + <interface type="bridge">
> > + <source bridge="eth0"/>
> > + <mac address="00:11:22:33:44:55"/>
> > + <model type="e1000e"/>
> > + </interface>
> > + <console type="pty"/>
> > + <input type="tablet" bus="usb"/>
> > + <graphics type="spice" port="-1" tlsPort="-1" autoport="yes">
> > + <image compression="off"/>
> > + </graphics>
> > + <sound model="ich9"/>
> > + <video>
> > + <model type="qxl"/>
> > + </video>
> > + <redirdev bus="usb" type="spicevmc"/>
> > + <redirdev bus="usb" type="spicevmc"/>
> > + </devices>
> > + <launchSecurity type="sev">
> > + <cbitpos>47</cbitpos>
> > + <reducedPhysBits>1</reducedPhysBits>
> > + <policy>0x0001</policy>
> > + <session>BASE64SESSION</session>
> > + <dhCert>BASE64CERT</dhCert>
> > + </launchSecurity>
> > +</domain>
> > diff --git a/tests/clitest.py b/tests/clitest.py
> > index 01f76b8a..c0efabf7 100644
> > --- a/tests/clitest.py
> > +++ b/tests/clitest.py
> > @@ -874,6 +874,11 @@ c.add_invalid("--nodisks --boot network --arch mips --virt-type kvm") # Invalid
> > c.add_invalid("--nodisks --boot network --paravirt --arch mips") # Invalid arch/virt combo
> > c.add_invalid("--disk none --location nfs:example.com/fake --nonetworks") # Using --location nfs, no longer supported
> >
> > +
> > +c = vinst.add_category("kvm-x86_64-launch-security", "--disk none --noautoconsole")
>
> The surrounding code isn't consistent here but please add two newlines
> between each category. You can fold the duplicate --connect bit into the
> second option of add_category
Not sure I follow with the duplicate '--connect' bit, can you rephrase?
Initially I added --connect into the category so that I don't have to use it
with each use case, and then domcapabilities happened and I needed to test
different failures.
>
> > +c.add_compare("--boot uefi --machine q35 --launch-security type=sev,reduced_phys_bits=1,policy=0x0001,cbitpos=47,dh_cert=BASE64CERT,session=BASE64SESSION --connect " + utils.URIs.kvm_amd_q35, "x86_64-launch-security-sev-full") # Full cmdline
>
> Is the --boot uefi bit important? if not, I suggest folding the
> boilerplate options into add_category so you just test --launch-security
> invocations here. You'll need an install option so --pxe should be fine.
So, yes UEFI is crucial, Brijesh from AMD told me he started the development on
SeaBIOS, but due to secure boot requests switched to Q35, so IIRC SeaBIOS
doesn't support SEV, but Brijesh can surely correct me. Anyhow, my take on this
is that SEV would be a good opportunity to enforce UEFI (along with Q35) and
thus move users from the legacy setups onto modern ones with lots of goodies.
Normally I'd simply put --boot uefi into the category, but I also wanted to
have a negative test on the platform setup (one of the subsequent patches has
the check).
>
> > +c.add_invalid("--launch-security policy=0x0001 --connect " + utils.URIs.kvm_amd_q35) # Missing launch-security 'type'
> > +> c = vinst.add_category("kvm-q35", "--noautoconsole --connect " +
> utils.URIs.kvm_q35)
> > c.add_compare("--boot uefi --disk none", "boot-uefi")
> >
> > diff --git a/virtinst/cli.py b/virtinst/cli.py
> > index 7ba6b211..0a76b074 100644
> > --- a/virtinst/cli.py
> > +++ b/virtinst/cli.py
> > @@ -827,6 +827,12 @@ def add_guest_xml_options(geng):
> > "--qemu-commandline='-display gtk,gl=on'\n"
> > "--qemu-commandline env=DISPLAY=:0.1"))
> >
> > + ParserLaunchSecurity.register()
> > + geng.add_argument("--launch-security", action="append",
> > + help=_("Configure VM launch security (e.g. SEV memory encryption). Ex:\n"
> > + "--launch-security type=sev,cbitpos=47,reduced_phys_bits=1,policy=0x0001,dh_cert=BASE64CERT\n"
> > + "--launch-security sev"))
> > +
> >
>
> We are trying to have virt-install options mirror XML naming, so this
> should be --launchSecurity. We can have --launchsecurity too like
>
> add_argument("--launchSecurity", "--launchsecurity", ...)
Noted.
>
> > def add_boot_options(insg):
> > ParserBoot.register()
> > @@ -3832,6 +3838,39 @@ class ParserHostdev(VirtCLIParser):
> > cls.add_arg("rom.bar", "rom_bar", is_onoff=True)
> >
> >
> > +#############################
> > +# --launch-security parsing #
> > +#############################
> > +
> > +class ParserLaunchSecurity(VirtCLIParser):
> > + cli_arg_name = "launch_security"
> > + guest_propname = "launchSecurity"
> > + remove_first = "type"
> > +
> > + @classmethod
> > + def _init_class(cls, **kwargs):
> > + VirtCLIParser._init_class(**kwargs)
> > + cls.add_arg("type", "type_")
> > + cls.add_arg("cbitpos", "cbitpos")
> > + cls.add_arg("reduced_phys_bits", "reduced_phys_bits")
> > + cls.add_arg("policy", "policy")
> > + cls.add_arg("session", "session")
> > + cls.add_arg("dh_cert", "dh_cert")
> > +
>
> Similarly the suboptions should match XML naming. So the first opts
> should be reducedPhysBits and dhCert, others are fine. Internal naming
> doesn't matter but I have a slight preference for matching XML naming,
> though the codebase isn't remotely consistent on that so it's up to you
Nah, you say consistent, I'll be consistent, I like it too, makes things easier
to look for when grepping the code base.
>
>
> > + def _check_required_opts(self):
> > + type_ = self.optdict.get("type", None)
> > +
> > + if not type_:
> > + fail(_("Missing mandatory attribute 'type' for --launch-security"))
> > +
>
> Generally we don't want this style of validation in the cli parser
> because it's tricky to get right. In this case it will break using
> virt-xml to edit a single non-type --launchSecurity field which should
> be a valid thing to do.
>
> You'll want to move this validation into a 'def validate' function in
> the launch security object and the virt-install cli parser will call it
> by default.
Originally, I wanted to have in in validate, but it didn't occur to me (from
how the validation is done in virt-install) like a proper validation, but sure,
I agree that parser was never a good place to put that in.
>
> > + def _parse(self, inst):
> > + if not inst.conn.is_qemu():
> > + logging.warning("--launch-security is only supported with QEMU")
> > +
>
> Generally I'm not a fan of these types of bits in the cli parser, and
> virt-manager in general. If libvirt supports sev for xen tomorrow then
> this message is now out of date. If libvirt/qemu combo is the only one
> that supports this, then we either let libvirt reject it for us
> (preferred), or we use domaincapabilities content to validate that sev
> is available and error otherwise. But if the latter bit is the only
> option then I think it's better just to not worry about it until real
> users complain about it
Right now, QEMU is the only one supporting it - looking at libvirt code, nothing
will really happen, as libvirt will parse the XML, but since libxl doesn't have
any code touching SEV, it's a NOP, so we can drop the check.
>
> > + self._check_required_opts()> + return super()._parse(inst)
> > +
> > +
> > ###########################
> > # Public virt parser APIs #
> > ###########################
> > diff --git a/virtinst/domain/__init__.py b/virtinst/domain/__init__.py
> > index f942ee59..b7157c9c 100644
> > --- a/virtinst/domain/__init__.py
> > +++ b/virtinst/domain/__init__.py
> > @@ -19,5 +19,6 @@ from .seclabel import DomainSeclabel
> > from .sysinfo import DomainSysinfo
> > from .vcpus import DomainVCPUs
> > from .xmlnsqemu import DomainXMLNSQemu
> > +from .launch_security import DomainLaunchSecurity
> >
> > __all__ = [l for l in locals() if l.startswith("Domain")]
> > diff --git a/virtinst/domain/launch_security.py b/virtinst/domain/launch_security.py
> > new file mode 100644
> > index 00000000..a911712c
> > --- /dev/null
> > +++ b/virtinst/domain/launch_security.py
> > @@ -0,0 +1,24 @@
> > +from ..xmlbuilder import XMLBuilder, XMLProperty
> > +
> > +
> > +class DomainLaunchSecurity(XMLBuilder):
> > + """
> > + Class for generating <launchSecurity> XML element
> > + """
> > +
> > + XML_NAME = "launchSecurity"
> > + _XML_PROP_ORDER = ["type_", "cbitpos", "reduced_phys_bits", "policy",
> > + "session", "dh_cert"]
> > +
> > + type_ = XMLProperty("./@type")
>
> Why the funny naming? if it's to avoid pylint complaining about
> collision with builtin type, I prefer supressing that error
I found this as a recommendation in a few "python guidelines" articles on the
internet, so as not to collide with builtin keywords, it also doesn't confuse
the syntax highlighter. I realized you don't care across the code base and it
works, but if it's not a big issue, I'd rather follow the recommendation.
Thanks,
Erik
More information about the virt-tools-list
mailing list