[virt-tools-list] [virt-install PATCH 2/7] virtinst: cli: Introduce parser support for SEV launch security
Cole Robinson
crobinso at redhat.com
Mon Jun 10 15:35:37 UTC 2019
On 6/10/19 3:40 AM, Erik Skultety wrote:
> 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.
>
I meant adding the common ('--connect ' + utils.URIs.kvm_amd_q35) into
the second argument of the add_category call, but if that's not flexible
for your needs then please ignore, I might have missed something
>>
>>> +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).
>
Gotchya
>>
>>> + 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.
>
For having a local type_ variable in a function, I wouldn't mind, and
overriding builtin type in a case like that is more dangerous. But this
location is part of the internal API where naming impacts other code,
and as nearly every other ./@type prop in other objects is named 'type',
I prefer the regular 'type' naming
Thanks,
Cole
More information about the virt-tools-list
mailing list