[libvirt] [PATCH v5 09/10] virsh: implement new command for launch security

[Erik Skultety]

On Wed, Apr 04, 2018 at 07:36:37AM -0500, Brijesh Singh wrote:
>
>
> On 4/3/18 9:32 AM, Erik Skultety wrote:
> > On Mon, Apr 02, 2018 at 09:18:55AM -0500, Brijesh Singh wrote:
> >> Add new 'launch-security' command, the command can be used to get or set
> >> the launch security information when booting encrypted VMs.
> >>
> >> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
> >> ---
> >>  tools/virsh-domain.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 84 insertions(+)
> >>
> >> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> >> index 2b775fc..4dca191 100644
> >> --- a/tools/virsh-domain.c
> >> +++ b/tools/virsh-domain.c
> >> @@ -13877,6 +13877,84 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd)
> >>      return ret >= 0;
> >>  }
> >>
> >> +/*
> >> + * "launch-security" command
> >> + */
> >> +static const vshCmdInfo info_launch_security[] = {
> >> +    {.name = "help",
> >> +        .data = N_("Get or set launch-security information")
> >> +    },
> >> +    {.name = "desc",
> >> +        .data = N_("Get or set the current launch-security information for a guest"
> >> +                   " domain.\n"
> >> +                   "    To get the launch-security information use following command: \n\n"
> >> +                   "    virsh # launch-security <domain>")
> > As John has pointed out, you might want to shorten ^these 2 lines, however, I
> > think it makes sense to make it obvious that running without any
> > arguments/options this behaves like a getter, otherwise it's going to behave
> > like a setter, right? (it's a common practice in libvirt, so nothing against
> > conceptually).
> >
>
> Yes, without any command line it should be getter otherwise settter.
> Currently, we don't have anything in setter yet.
>
> >> +    },
> >> +    {.name = NULL}
> >> +};
> >> +
> >> +static const vshCmdOptDef opts_launch_security[] = {
> >> +    VIRSH_COMMON_OPT_DOMAIN_FULL(0),
> >> +    {.name = "get",
> >> +     .type = VSH_OT_STRING,
> >> +     .help = N_("Show the launch-security info")
> >> +    },
> >> +    VIRSH_COMMON_OPT_DOMAIN_CONFIG,
> >> +    VIRSH_COMMON_OPT_DOMAIN_LIVE,
> >> +    VIRSH_COMMON_OPT_DOMAIN_CURRENT,
> >> +    {.name = NULL}
> >> +};
> > Sorry if I missed the obvious, but what exactly is the --get <string> supposed
> > to do?
>
> The command will return a measurement of encrypted image. The
> measurement value can be used by guest owner to validate the image
> before it launches the VM. In a typical scenario we may have something
> like this:
>
> # virsh create guest.xml --paused
> # virsh launch-security --domain guest
> // validate the measurement obtained through above command
> if measurement is wrong then;
>  destory the guest
> else
>  // optionally inject secret in guest using virsh launch-security set <....>
>  resume the guest

Yeah, but since you agreed that the command behaves like a getter without any
parameters, I don't see a reason for the --get <string> parameter, that's why I
asked about it. Also, the name of the command should be changed since this way,
it's a bit deceptive as it hints that it queries/modifies the launch-security
data which you got from querying qemu capabilities within the domain XML.
I'm also going to respond to the other email to this patch I sent yesterday,
because it's irrelevant given the fact that I got confused by the name (even
though I saw what API it called) and created a false association.
Other than what's already been noted, the logic in the virsh command itself
looks solid, not much to it since the setter part will come later I presume.

Erik