Re: [libvirt] [PATCH v3 6/7] qemu: Set up the migrate TLS objects for target

On Fri, Mar 17, 2017 at 14:39:00 -0400, John Ferlan wrote: > If the migration flags indicate this migration will be using TLS, > then set up the destination during the prepare phase once the target > domain has been started to add the TLS objects to perform the migration. > > This will create at least an "-object tls-creds-x509,endpoint=server,..." > and potentially an "-object secret,..." to handle the passphrase response Looks like a leftover from previous versions where the objects were not hotplugged on both sides of migration.

> to access the TLS credentials. The alias/id used for the TLS objects > will contain "libvirt_migrate" as a mechanism to signify that libvirt > started the migration on the target (reaping benefits possibly). > > Once the objects are created, the code will set the "tls-creds" and > "tls-hostname" migration parameters to signify usage of TLS. > > During the Finish phase we'll be sure to attempt to clear the > migration parameters and delete those objects (whether or not they > were created). > > Since incoming migrations that don't reach the Finish stage will be > killed in qemuProcessRecoverMigrationIn and the only purpose at that > point would be to free memory, it's not necessary to set up any sort > of recovery. Additionally, subsequent migrations will check if the > migration parameters are set and adjust them appropriately if for > some reason libvirtd restarts after setting the Finish marker, but > before actually resetting the environment. Sure, migrations will do that, but what about snapshots, saving a domain to a file and similar stuff which internally uses migration too. Do we properly reset the parameters for them too? I think we should delete the objects and reset migration parameters in qemuProcessRecoverMigrationIn anyway.

> > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/qemu/qemu_domain.c | 7 +- > src/qemu/qemu_domain.h | 91 +++++++----- > src/qemu/qemu_migration.c | 344 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 403 insertions(+), 39 deletions(-) ... > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h > index 1f266bf..1dd3b1c 100644 > --- a/src/qemu/qemu_domain.h > +++ b/src/qemu/qemu_domain.h ... > @@ -246,47 +283,18 @@ struct _qemuDomainObjPrivate { > > /* note whether memory device alias does not correspond to slot number */ > bool memAliasOrderMismatch; > -}; > > -# define QEMU_DOMAIN_PRIVATE(vm) \ > - ((qemuDomainObjPrivatePtr) (vm)->privateData) > + /* for migrations using TLS with a secret (not to be saved in our */ > + /* private XML). */ > + qemuDomainSecretInfoPtr migSecinfo; > > -/* Type of domain secret */ > -typedef enum { > - VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0, > - VIR_DOMAIN_SECRET_INFO_TYPE_AES, /* utilize GNUTLS_CIPHER_AES_256_CBC */ > - > - VIR_DOMAIN_SECRET_INFO_TYPE_LAST > -} qemuDomainSecretInfoType; > - > -typedef struct _qemuDomainSecretPlain qemuDomainSecretPlain; > -typedef struct _qemuDomainSecretPlain *qemuDomainSecretPlainPtr; > -struct _qemuDomainSecretPlain { > - char *username; > - uint8_t *secret; > - size_t secretlen; > + /* Used when fetching/storing the current 'tls-creds' migration setting */ > + /* (not to be saved in our private XML). */ > + char *migTLSAlias; I'm not quite sure why we need to store the original value.

> }; ... > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 66a5062..42074f0 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -85,6 +85,8 @@ VIR_ENUM_IMPL(qemuMigrationCompressMethod, QEMU_MIGRATION_COMPRESS_LAST, > "mt", > ); > > +#define QEMU_MIGRATION_TLS_ALIAS_BASE "libvirt_migrate" > + > enum qemuMigrationCookieFlags { > QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS, > QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE, > @@ -1488,6 +1490,164 @@ qemuMigrationEatCookie(virQEMUDriverPtr driver, > return NULL; > } > > +/* qemuMigrationCheckTLSCreds > + * @driver: pointer to qemu driver > + * @vm: domain object > + * @asyncJob: migration job to join > + * > + * Query the migration parameters looking for the 'tls-creds' parameter. > + * The parameter was initially supported in QEMU 2.7; however, there was > + * no mechanism provided to clear the parameter. For QEMU 2.9, a change > + * was made to allow setting the parameter to an empty string in order > + * to clear. An additional change was made to initialize the parameter > + * to the empty string. Although still not perfect since it's possible > + * that a pre-2.9 release set the string to something and we should not How would this happen? QEMU won't magically set it to something by itself. And we don't need to care about someone messing up with the monitor or command line manually.

Anyway, I don't see a real value in repeating the story over and over. We just ask for the default parameter values and if TLS parameters are not there, they are not supported from our point of view. That's pretty clear and version agnostic.

> + * set it to the empty string, at least it's better than nothing. So let's > + * check if the parameter has been set to something to detect the whether > + * the parameter exists. If it's been set to something, then save the > + * value in our private domain structures so that future decision makers > + * can decide how they should proceed based upon the setting. > + * > + * Returns 0 if we were able to successfully fetch the params and > + * additionally if the tls-creds parameter exists, saves it in the > + * private domain structure. Returns -1 on failure. > + */ > +static int > +qemuMigrationCheckTLSCreds(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + qemuDomainAsyncJob asyncJob) > +{ > + int ret = -1; > + qemuDomainObjPrivatePtr priv = vm->privateData; > + qemuMonitorMigrationParams migParams = { 0 }; > + > + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) > + goto cleanup; > + > + if (qemuMonitorGetMigrationParams(priv->mon, &migParams) < 0) > + goto cleanup; > + > + /* NB: Could steal NULL pointer too! Let caller decide what to do. */ > + VIR_STEAL_PTR(priv->migTLSAlias, migParams.migrateTLSAlias); > + > + ret = 0; > + > + cleanup: > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + ret = -1; > + > + qemuMigrationParamsClear(&migParams); > + > + return ret; > +} > + > + > +/* qemuMigrationCheckSetupTLS > + * @conn: Connection pointer > + * @driver: pointer to qemu driver > + * @vm: domain object > + * @cfg: configuration pointer > + * @asyncJob: migration job to join > + * > + * Check if TLS is possible and set up the environment. Assumes the caller > + * desires to use TLS (e.g. caller found VIR_MIGRATE_TLS flag). > + * > + * Ensure the qemu.conf has been properly configured to add an entry for > + * "migrate_tls_x509_cert_dir". Also check if the "tls-creds" parameter > + * was present from a query of migration parameters > + * > + * Returns 0 on success, -1 on error/failure > + */ > +static int > +qemuMigrationCheckSetupTLS(virConnectPtr conn, > + virQEMUDriverPtr driver, > + virQEMUDriverConfigPtr cfg, > + virDomainObjPtr vm, > + qemuDomainAsyncJob asyncJob) > +{ > + qemuDomainObjPrivatePtr priv = vm->privateData; > + > + if (!cfg->migrateTLSx509certdir) { > + virReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("host migration TLS directory not configured")); > + return -1; > + } > + > + if (qemuMigrationCheckTLSCreds(driver, vm, asyncJob) < 0) > + return -1; > + > + if (!priv->migTLSAlias) { > + virReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("get/set empty migration parameter 'tls-creds' is " > + "not supported")); Heh, it took me a while to understand what you were trying to say and it is certainly not something we'd want to tell users who enable TLS with old QEMU. How about "TLS migration is not supported with this QEMU binary"? And the code should be VIR_ERR_OPERATION_UNSUPPORTED.

> + return -1; > + } > + > + /* If there's a secret, then grab/store it now using the connection */ > + if (cfg->migrateTLSx509secretUUID && > + !(priv->migSecinfo = > + qemuDomainSecretInfoTLSNew(conn, priv, QEMU_MIGRATION_TLS_ALIAS_BASE, > + cfg->migrateTLSx509secretUUID))) > + return -1; > + > + return 0; > +} > + > + > +/* qemuMigrationAddTLSObjects > + * @driver: pointer to qemu driver > + * @vm: domain object > + * @cfg: configuration pointer > + * @tlsListen: server or client > + * @asyncJob: Migration job to join > + * @tlsAlias: alias to be generated for TLS object > + * @secAlias: alias to be generated for a secinfo object > + * @migParams: migration parameters to set > + * > + * Create the TLS objects for the migration and set the migParams value > + * > + * Returns 0 on success, -1 on failure > + */ > +static int > +qemuMigrationAddTLSObjects(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virQEMUDriverConfigPtr cfg, > + bool tlsListen, > + qemuDomainAsyncJob asyncJob, > + char **tlsAlias, > + char **secAlias, > + qemuMonitorMigrationParamsPtr migParams) > +{ > + qemuDomainObjPrivatePtr priv = vm->privateData; > + virJSONValuePtr tlsProps = NULL; > + virJSONValuePtr secProps = NULL; > + > + if (qemuDomainGetTLSObjects(priv->qemuCaps, priv->migSecinfo, > + cfg->migrateTLSx509certdir, tlsListen, > + cfg->migrateTLSx509verify, > + QEMU_MIGRATION_TLS_ALIAS_BASE, > + &tlsProps, tlsAlias, &secProps, secAlias) < 0) > + return -1; > + > + /* 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 alias) or > + * some other error path between now and perform . */ > + qemuDomainDelTLSObjects(driver, vm, asyncJob, *secAlias, *tlsAlias); > + > + /* Add the migrate TLS objects to the domain */ Useless comment. > + if (qemuDomainAddTLSObjects(driver, vm, asyncJob, *secAlias, &secProps, > + *tlsAlias, &tlsProps) < 0) > + return -1; > + > + /* Set the param used for 'tls-creds' */ And this one too.

> + if (VIR_STRDUP(migParams->migrateTLSAlias, *tlsAlias) < 0) > + return -1; > + > + return 0; > +} > + > + > static void > qemuMigrationStoreDomainState(virDomainObjPtr vm) > { > @@ -3530,6 +3690,47 @@ qemuMigrationParamsFree(qemuMonitorMigrationParamsPtr *migParams) > } > > > +/* qemuMigrationSetEmptyTLSParams > + * @priv: Pointer to private domain data > + * @migParams: Pointer to a migration parameters block > + * > + * If the qemuMigrationCheckTLSCreds query finds a non empty alias and it > + * is set to the alias that libvirt set, then we need to set the migration > + * parameters to "" in order to force clearing the TLS values from our > + * previous migration that may not have been cleared properly if libvirtd > + * restarted during the finish phase before the ResetTLSParams was run. > + * > + * Returns 0 on success, -1 on failure > + */ > +static int > +qemuMigrationSetEmptyTLSParams(qemuDomainObjPrivatePtr priv, > + qemuMonitorMigrationParamsPtr migParams) > +{ > + char *tlsAlias = NULL; > + > + if (priv->migTLSAlias) { > + if (*priv->migTLSAlias == '\0') > + return 0; > + > + if (!(tlsAlias = > + qemuAliasTLSObjFromSrcAlias(QEMU_MIGRATION_TLS_ALIAS_BASE))) > + return -1; > + > + if (STRNEQ(priv->migTLSAlias, tlsAlias)) { > + VIR_FREE(tlsAlias); > + return 0; Why? We should just always reset the parameters if VIR_MIGRATE_TLS is not set. No matter what was the previous value.

> + } > + VIR_FREE(tlsAlias); > + > + if (VIR_STRDUP(migParams->migrateTLSAlias, "") < 0 || > + VIR_STRDUP(migParams->migrateTLSHostname, "") < 0) > + return -1; > + } > + > + return 0; > +} > + > + > qemuMonitorMigrationParamsPtr > qemuMigrationParams(virTypedParameterPtr params, > int nparams, > @@ -3601,6 +3802,110 @@ qemuMigrationSetParams(virQEMUDriverPtr driver, > } > > > +/* qemuMigrationResetTLSParams > + * @driver: pointer to qemu driver > + * @vm: domain object > + * @asyncJob: migration job to join > + * @tlsAlias: alias used for TLS object > + * > + * If we configured the migration TLS params, then let's clear the setting > + * of the tls-creds and tls-hostname. > + * > + * Returns 0 on success, -1 on failure with error message set > + */ > +static int > +qemuMigrationResetTLSParams(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + qemuDomainAsyncJob asyncJob, > + const char *tlsAlias) > +{ > + int ret = -1; > + qemuDomainObjPrivatePtr priv = vm->privateData; > + qemuMonitorMigrationParams migParams = { 0 }; > + > + if (!priv->migTLSAlias) > + return 0; > + > + if (STREQ_NULLABLE(priv->migTLSAlias, tlsAlias)) { > + if (VIR_STRDUP(migParams.migrateTLSAlias, "") < 0 || > + VIR_STRDUP(migParams.migrateTLSHostname, "") < 0) > + goto cleanup; > + > + if (qemuMigrationSetParams(driver, vm, asyncJob, &migParams) < 0) > + goto cleanup; > + } > + > + ret = 0; > + > + cleanup: > + qemuMigrationParamsClear(&migParams); > + return ret; > +} And how does this differ from qemuMigrationSetEmptyTLSParams? Well, one function generates its own tlsAlias while the other gets it as a parameter, but we should not check it anyway... Oh I see, Reset actually sends the parameters to QEMU. It could be just a convenient wrapper around SetEmpty then.

> + > + > +/* qemuMigrationDeconstructTLS > + * @driver: pointer to qemu driver > + * @vm: domain object > + * @asyncJob: migration job to join > + * @tlsAlias: alias generated for TLS object > + * @secAlias: alias generated for a secinfo object > + * > + * Deconstruct all the setup possibly done for TLS - various objects, secinfo, > + * and migration parameters. > + * > + * Returns 0 on success, -1 on failure > + */ > +static int > +qemuMigrationDeconstructTLS(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + qemuDomainAsyncJob asyncJob, > + const char *tlsAlias, > + const char *secAlias) > +{ > + qemuDomainObjPrivatePtr priv = vm->privateData; > + > + qemuDomainDelTLSObjects(driver, vm, asyncJob, secAlias, tlsAlias); > + qemuDomainSecretInfoFree(&priv->migSecinfo); > + > + return qemuMigrationResetTLSParams(driver, vm, asyncJob, tlsAlias); > +} > + > + > +/* qemuMigrationResetTLS > + * @driver: pointer to qemu driver > + * @vm: domain object > + * @asyncJob: migration job to join > + * > + * Wrapper to qemuMigrationDeconstructTLS that generates the expected > + * tlsAlias and secAlias for migration paths without them set (e.g. Finish) > + * > + * Returns 0 on success, -1 on failure > + */ > +static int > +qemuMigrationResetTLS(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + qemuDomainAsyncJob asyncJob) Can't you just create a single qemuMigrationResetTLS function which takes both aliases and generates them if they are NULL? I know you like the "Deconstruct" part of the function name, but it all just looks more complicated than it needs to be.

> +{ > + char *tlsAlias = NULL; > + char *secAlias = NULL; > + int ret; > + > + /* NB: If either or both fail to allocate memory we can still proceed > + * since the next time we migrate another deletion attempt will be > + * made after successfully generating the aliases. */ > + tlsAlias = qemuAliasTLSObjFromSrcAlias(QEMU_MIGRATION_TLS_ALIAS_BASE); > + secAlias = qemuDomainGetSecretAESAlias(QEMU_MIGRATION_TLS_ALIAS_BASE, > + false); > + > + ret = qemuMigrationDeconstructTLS(driver, vm, asyncJob, tlsAlias, secAlias); > + > + VIR_FREE(tlsAlias); > + VIR_FREE(secAlias); > + > + return ret; > +} > + > + > static int > qemuMigrationPrepareAny(virQEMUDriverPtr driver, > virConnectPtr dconn, ... > @@ -3829,6 +4137,32 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, > compression, &migParams) < 0) > goto stopjob; > > + /* Migrations using TLS need to add the "tls-creds-x509" object and > + * set the migration TLS parameters */ > + if (flags & VIR_MIGRATE_TLS) { > + cfg = virQEMUDriverGetConfig(driver); > + if (qemuMigrationCheckSetupTLS(dconn, driver, cfg, vm, > + QEMU_ASYNC_JOB_MIGRATION_IN) < 0) > + goto stopjob; > + > + if (qemuMigrationAddTLSObjects(driver, vm, cfg, true, > + QEMU_ASYNC_JOB_MIGRATION_IN, > + &tlsAlias, &secAlias, &migParams) < 0) > + goto stopjob; > + > + /* Force reset of 'tls-hostname', just in case */ The "just in case" part is confusing. What about replacing it with something about tls-hostname making no sense on destination?

> + if (VIR_STRDUP(migParams.migrateTLSHostname, "") < 0) > + goto stopjob; > + > + } else { > + /* If we support setting the tls-creds, be sure to always reset > + * the migration parameters when this migration isn't using TLS */ Exactly, "always" is the important part, it doesn't say "if something" as in the current qemuMigrationSetEmptyTLSParams implementation. > + if ((qemuMigrationCheckTLSCreds(driver, vm, s/((/(/ > + QEMU_ASYNC_JOB_MIGRATION_IN) < 0) || s/0)/0/ > + (qemuMigrationSetEmptyTLSParams(priv, &migParams) < 0)) s/(//; s/0)/0/

> + goto stopjob; > + } > + > if (STREQ_NULLABLE(protocol, "rdma") && > virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 0) { > goto stopjob; ... Jirka