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

[Erik Skultety]

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.

Erik

> 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