[libvirt] [PATCH 13/13] qemu: Set up the migration TLS objects for source

[Jiri Denemark]

On Fri, Feb 17, 2017 at 14:39:30 -0500, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1300769
> 
> Modify the Begin phase to add the checks to determine whether a migration
> wishes to use TLS and whether it's configured including adding the secret
> into the priv->migSecinfo for the source domain.
> 
> Modify the Perform phase in qemuMigrationRun in order to generate the
> TLS objects to be used for the migration and set the migration channel
> parameters 'tls-creds' and possibly 'tls-hostname' in order to enable TLS.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 448d94e..84eb6a3 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3362,6 +3362,9 @@ qemuMigrationBegin(virConnectPtr conn,
>          goto endjob;
>      }
>  
> +    if (qemuMigrationCheckSetupTLS(driver, conn, vm, flags) < 0)
> +        goto endjob;
> +
>      /* Check if there is any ejected media.
>       * We don't want to require them on the destination.
>       */
> @@ -4709,8 +4712,14 @@ qemuMigrationRun(virQEMUDriverPtr driver,
>  {
>      int ret = -1;
>      unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND;
> +    virQEMUDriverConfigPtr cfg = NULL;
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      qemuMigrationCookiePtr mig = NULL;
> +    virJSONValuePtr tlsProps = NULL;
> +    virJSONValuePtr secProps = NULL;
> +    char *tlsAlias = NULL;
> +    char *tlsHostname = NULL;
> +    char *secAlias = NULL;
>      qemuMigrationIOThreadPtr iothread = NULL;
>      int fd = -1;
>      unsigned long migrate_speed = resource ? resource : priv->migMaxBandwidth;
> @@ -4774,6 +4783,44 @@ qemuMigrationRun(virQEMUDriverPtr driver,
>      if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig, graphicsuri) < 0)
>          VIR_WARN("unable to provide data for graphics client relocation");
>  
> +    /* If we're using TLS attempt to add the objects */

Redundant comment.

> +    if (priv->migrateTLS) {
> +        cfg = virQEMUDriverGetConfig(driver);
> +
> +        if (qemuDomainGetTLSObjects(priv->qemuCaps, priv->migSecinfo,
> +                                    cfg->migrateTLSx509certdir, false,
> +                                    cfg->migrateTLSx509verify,
> +                                    "migrate", &tlsProps, &tlsAlias,
> +                                    &secProps, &secAlias) < 0)
> +            goto cleanup;
> +
> +        /* Ensure the domain doesn't already have the TLS objects defined...
> +         * This should prevent any issues just in case some cleanup wasn't
> +         * properly completed (both src and dst use the same aliases) or
> +         * some other error path between now and perform . */
> +        qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);

This shouldn't be needed if you do what I suggest at the end of this
email, but I agree we can just call it anyway for extra safety.

> +
> +        /* Add the migrate TLS objects to the domain */
> +        if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps,
> +                                    tlsAlias, &tlsProps) < 0)
> +            goto cleanup;
> +
> +        migParams->migrateTLSAlias = tlsAlias;
> +        migParams->migrateTLSAlias_set = true;
> +
> +        /* We need to add the tls-hostname only for special circumstances.
> +         * When using "fd:" or "exec:", qemu needs to know the hostname of
> +         * the target qemu to correctly validate the x509 certificate
> +         * it receives. */
> +        if (STREQ(spec->dest.host.protocol, "fd") ||
> +            STREQ(spec->dest.host.protocol, "exec")) {
> +            if (VIR_STRDUP(tlsHostname, spec->dest.host.name) < 0)
> +                goto cleanup;
> +            migParams->migrateTLSHostname = tlsHostname;
> +            migParams->migrateTLSHostname_set = true;
> +        }
> +    }
> +

Can most of this code be moved into a separate function?

>      if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
>                           QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) {
>          if (mig->nbd) {
> @@ -4954,6 +5001,14 @@ qemuMigrationRun(virQEMUDriverPtr driver,
>              ret = -1;
>      }
>  
> +    qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);
> +    virJSONValueFree(tlsProps);
> +    virJSONValueFree(secProps);
> +    VIR_FREE(tlsAlias);
> +    VIR_FREE(tlsHostname);
> +    VIR_FREE(secAlias);
> +    virObjectUnref(cfg);
> +
>      if (spec->fwdType != MIGRATION_FWD_DIRECT) {
>          if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0)
>              ret = -1;

You store the migrateTLS info in the status XML on the destination host
where libvirtd restart almost always kills the QEMU process. But you
didn't bother storing the flag on the source where the QEMU process
almost always remains running when libvirtd is restarted.

The freshly started libvirtd calls qemuProcessRecoverMigration* to
finish or cancel the ongoing migration and both functions (or functions
which are called from them) need to properly cleanup the TLS objects.

Jirka