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

[Cole Robinson]

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
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