Re: [virt-install PATCH 2/7] virtinst: cli: Introduce parser support for SEV launch security

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
>>> ---
>>>  ...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

_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list



[Index of Archives]     [Linux Virtualization]     [KVM Development]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]     [Video 4 Linux]

  Powered by Linux