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@xxxxxxxxxx> >>> --- >>> 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 _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
