[libvirt] [PATCH v6 6/9] libvirt: add new public API to get launch security info

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:31PM -0500, Brijesh Singh wrote:
> The API can be used outside the libvirt to get the launch security
> information. When SEV is enabled, the API can be used to get the
> measurement of the launch process.
>
> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
> ---
>  include/libvirt/libvirt-domain.h | 17 ++++++++++++++
>  src/driver-hypervisor.h          |  7 ++++++
>  src/libvirt-domain.c             | 48 ++++++++++++++++++++++++++++++++++++++++
>  src/libvirt_public.syms          |  5 +++++
>  4 files changed, 77 insertions(+)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index d7cbd187969d..f252d18da72f 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -4764,4 +4764,21 @@ int virDomainSetLifecycleAction(virDomainPtr domain,
>                                  unsigned int action,
>                                  unsigned int flags);
>
> +/**
> + * Launch Security API
> + */
> +
> +/**
> + * VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT:
> + *
> + * Macro represents the launch measurement of the SEV guest,
> + * as VIR_TYPED_PARAM_STRING.
> + */
> +#define VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT "sev-measurement"

fails make syntax-check because of indentation, should be "# define ..."

...

> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 95df3a0dbc7b..5ccae5da8883 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -785,4 +785,9 @@ LIBVIRT_4.1.0 {
>          virStoragePoolLookupByTargetPath;
>  } LIBVIRT_3.9.0;
>
> +LIBVIRT_4.4.0 {
> +    global:
> +        virDomainGetLaunchSecurityInfo;
> +} LIBVIRT_4.1.0;

Will most probably become 4.5.0 :(

Technically, I don't have any notes related to the functional changes,
therefore I'd give you my RB, however, I still find the naming confusing and I
can't think of something better. What if one day we'll actually be able to/need
to modify the configuration for some reason, we should reserve a name like this
for future modifications of launch-security data of the guest. Next, you're
preparing for adding support for some kind of setter in the virsh command, any
idea of what the setter data might be? Because I can imagine that you'd still
want to perform a measurement, but want to send additional arguments, to the
remote side's firmware to change the behaviour of the measurement and you can't
do this with a simple flag, you also need typed params for that which means
wou'd end up with something like:

int
virDomainLaunchSecurityMeasure(virDomainPtr domain,
                               virTypedParamsPtr send_params,
                               unsigned int nsend_params,
                               virTypedParamsPtr *recv_params,
                               unsigned int nrecv_params);

And you can both send additional arguments as well as receive arguments
according to the remote implementation, it's IMHO safer, but it's extremely
ugly at the same time and we're hitting the 'one function does all' kind of
scenario which we also shouldn't do. At the same time though, we could say that
any arguments that you might need to alter the result of the measurement should
be available prior to launching the guest in its XML config file, that way,
you'll never need anything apart from the recv_params,recv_nparams and the name
you're suggesting can stay, since only by using flags you can tell libvirt
daemon whether you want to work with the info in the config or take a
measurement or something else.

This was just me thinking out loud and would like to get some input from you
and/or other reviewer because even though I'm okay with the code, I don't want
to make a decision we can't take back just yet.

Erik