[virt-tools-list] [virt-install PATCH 3/7] virtinst: cli: Provide a default value for the 'policy' argument

[Cole Robinson]

On 6/10/19 7:58 AM, Erik Skultety wrote:
> On Thu, Jun 06, 2019 at 04:42:03PM -0400, Cole Robinson wrote:
>> On 6/6/19 6:00 AM, Erik Skultety wrote:
>>> Policy is a 4-byte bitfield used to turn on/off certain behaviour within
>>> the SEV firmware. For a detailed table of supported flags, see
>>> https://libvirt.org/formatdomain.html#launchSecurity.
>>> Most of the flags are related to advanced features (some of them don't
>>> even exist at the moment), except for the first 2 bits which determine
>>> whether debug mode should be turned on and whether the same key should
>>> be used to encrypt memory of multiple guests respectively.
>>>
>>> >From security POV, most users will probably want separate keys for
>>> individual guests, thus the value 0x03 was selected as the policy
>>> default.
>>>
>>> Signed-off-by: Erik Skultety <eskultet at redhat.com>
>>> ---
>>>  tests/clitest.py |  1 +
>>>  virtinst/cli.py  | 25 ++++++++++++++++++++++++-
>>>  2 files changed, 25 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/clitest.py b/tests/clitest.py
>>> index c0efabf7..3958871e 100644
>>> --- a/tests/clitest.py
>>> +++ b/tests/clitest.py
>>> @@ -877,6 +877,7 @@ c.add_invalid("--disk none --location nfs:example.com/fake --nonetworks")  # Usi
>>>
>>>  c = vinst.add_category("kvm-x86_64-launch-security", "--disk none --noautoconsole")
>>>  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
>>> +c.add_valid("--boot uefi --machine q35 --launch-security sev,reduced_phys_bits=1,cbitpos=47 --connect " + utils.URIs.kvm_amd_q35)  # Default policy == 0x0003 will be used
>>>  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)
>>> diff --git a/virtinst/cli.py b/virtinst/cli.py
>>> index 0a76b074..4621b3ae 100644
>>> --- a/virtinst/cli.py
>>> +++ b/virtinst/cli.py
>>> @@ -3847,13 +3847,27 @@ class ParserLaunchSecurity(VirtCLIParser):
>>>      guest_propname = "launchSecurity"
>>>      remove_first = "type"
>>>
>>> +    def set_policy_cb(self, inst, val, virtarg):
>>> +        inst.policy = ""
>>> +
>>> +        try:
>>> +            int(val, 16)
>>> +        except ValueError as e:
>>> +            fail(_("Invalid value for argument '%s' of --launch-security: %s") %
>>> +                    (virtarg.propname, str(e)))
>>> +
>>> +        if not val.startswith("0x"):
>>> +            inst.policy = "0x"
>>> +
>>> +        inst.policy = inst.policy + val
>>> +
>>
>> Does libvirt validate this for us? If so I'd rather leave it up to
>> libvirt and just pass the value on. And the magic of turning BF into
>> 0xBF doesn't seem necessary to support, as an example you'd need to add
>> an explicit test case to cover both formats
>>
>>>      @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("policy", "policy", cb=cls.set_policy_cb)
>>>          cls.add_arg("session", "session")
>>>          cls.add_arg("dh_cert", "dh_cert")
>>>
>>> @@ -3862,6 +3876,15 @@ class ParserLaunchSecurity(VirtCLIParser):
>>>
>>>          if not type_:
>>>              fail(_("Missing mandatory attribute 'type' for --launch-security"))
>>> +        elif type_ == "sev":
>>> +
>>> +            # 'policy' is a mandatory 4-byte argument for the SEV firmware,
>>> +            # if missing, let's use 0x03 which, according to the table at
>>> +            # https://libvirt.org/formatdomain.html#launchSecurity:
>>> +            # (bit 0) - disables the debugging mode
>>> +            # (bit 1) - disables encryption key sharing across multiple guests
>>> +            if 'policy' not in self.optdict:
>>> +                self.optdict['policy'] = "0x03"
>>>
>>>      def _parse(self, inst):
>>>          if not inst.conn.is_qemu():
>>>
>>
>> Rather than in the cli parser for similar virt-xml reasons, this should
>> be in the launchSecurity class in a set_defaults function which will get
>> called in guest.py. See features.set_defaults for an example
>>
>> Though in general I think we should ask if this default value is good
>> enough for virt-install then why isn't it set as a default in libvirt.
>> Putting it in libvirt potentially simplifies all API users. If there's
>> good reasons why this isn't a default in libvirt, like maybe there's
> 
> Libvirt and defaults don't usually go well together. Virt-manager (and
> virt-install for that matter) are meant to be user friendly, so having a
> default that will work for most users is IMO okay to do, but in libvirt, we
> don't want to do this, because on the management layer you may have a use case
> for a shared key (a customer pool or whatnot), or you don't want the machine to
> be migrated, libvirt is too low level for policies and decisions like this, you
> want to do this at the management layer.
> 

Alright let's leave it in, but with finding a way to move it to
set_defaults like I suggested above.

Thanks,
Cole

> 
>> downsides to using this value, then we need to ask why that's okay for
>> virt-install to use it rather than require the user to be explicit.
>>
>> Thanks,
>> Cole


- Cole