On Fri, Mar 17, 2017 at 14:38:55 -0400, John Ferlan wrote: ... "while (0)" is usually used in such macros... not a big deal, but the semicolon must not be there. ... ACK with the semicolon removed. Jirka

On Fri, Mar 17, 2017 at 14:38:58 -0400, John Ferlan wrote: ... Our *Free functions don't usually get double pointers. ... ... This comment could make some sense in the commit message (unlike describing which paths are changed by the patch), but I don't think it's any useful here. Describing that NULL means unsupported and "" means unset would be enough I think. And even better if this is documented inside struct _qemuMonitorMigrationParams. Yeah, this is what I was thinking about (except for the "See query" part). And I still think it would make sense to move it to struct _qemuMonitorMigrationParams. Jirka

Add an asyncJob argument for add/delete TLS Objects. A future patch will add/delete TLS objects from a migration which may hae a job to join. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_hotplug.c | 24 ++++++++++++++++-------- src/qemu/qemu_hotplug.h | 2 ++ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ddcbc5e..9adb04a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1531,6 +1531,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, void qemuDomainDelTLSObjects(virQEMUDriverPtr driver, virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, const char *secAlias, const char *tlsAlias) { @@ -1542,7 +1543,8 @@ qemuDomainDelTLSObjects(virQEMUDriverPtr driver, orig_err = virSaveLastError(); - qemuDomainObjEnterMonitor(driver, vm); + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + goto cleanup; if (tlsAlias) ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); @@ -1552,6 +1554,7 @@ qemuDomainDelTLSObjects(virQEMUDriverPtr driver, ignore_value(qemuDomainObjExitMonitor(driver, vm)); + cleanup: if (orig_err) { virSetError(orig_err); virFreeError(orig_err); @@ -1562,6 +1565,7 @@ qemuDomainDelTLSObjects(virQEMUDriverPtr driver, int qemuDomainAddTLSObjects(virQEMUDriverPtr driver, virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, const char *secAlias, virJSONValuePtr *secProps, const char *tlsAlias, @@ -1574,7 +1578,8 @@ qemuDomainAddTLSObjects(virQEMUDriverPtr driver, if (!tlsAlias && !secAlias) return 0; - qemuDomainObjEnterMonitor(driver, vm); + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; if (secAlias) { rc = qemuMonitorAddObject(priv->mon, "secret", @@ -1601,7 +1606,7 @@ qemuDomainAddTLSObjects(virQEMUDriverPtr driver, virSetError(orig_err); virFreeError(orig_err); } - qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); + qemuDomainDelTLSObjects(driver, vm, asyncJob, secAlias, tlsAlias); return -1; } @@ -1682,8 +1687,8 @@ qemuDomainAddChardevTLSObjects(virConnectPtr conn, goto cleanup; dev->data.tcp.tlscreds = true; - if (qemuDomainAddTLSObjects(driver, vm, *secAlias, &secProps, - *tlsAlias, &tlsProps) < 0) + if (qemuDomainAddTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE, + *secAlias, &secProps, *tlsAlias, &tlsProps) < 0) goto cleanup; ret = 0; @@ -1773,7 +1778,8 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, virSetError(orig_err); virFreeError(orig_err); } - qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); + qemuDomainDelTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE, + secAlias, tlsAlias); goto audit; } @@ -2034,7 +2040,8 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, virFreeError(orig_err); } - qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); + qemuDomainDelTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE, + secAlias, tlsAlias); goto audit; } @@ -2186,7 +2193,8 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, virFreeError(orig_err); } - qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); + qemuDomainDelTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE, + secAlias, tlsAlias); goto audit; } diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 73f2b1f..f06f232 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -36,11 +36,13 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, void qemuDomainDelTLSObjects(virQEMUDriverPtr driver, virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, const char *secAlias, const char *tlsAlias); int qemuDomainAddTLSObjects(virQEMUDriverPtr driver, virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, const char *secAlias, virJSONValuePtr *secProps, const char *tlsAlias, -- 2.9.3

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 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. 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.c b/src/qemu/qemu_domain.c index c239a06..f4636ed 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -782,7 +782,7 @@ qemuDomainSecretAESClear(qemuDomainSecretAES secret) } -static void +void qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo) { if (!*secinfo) @@ -1186,7 +1186,7 @@ qemuDomainSecretInfoNew(virConnectPtr conn, * * Returns qemuDomainSecretInfoPtr or NULL on error. */ -static qemuDomainSecretInfoPtr +qemuDomainSecretInfoPtr qemuDomainSecretInfoTLSNew(virConnectPtr conn, qemuDomainObjPrivatePtr priv, const char *srcAlias, @@ -1677,6 +1677,9 @@ qemuDomainObjPrivateFree(void *data) VIR_FREE(priv->libDir); VIR_FREE(priv->channelTargetDir); + + qemuDomainSecretInfoFree(&priv->migSecinfo); + VIR_FREE(priv->migTLSAlias); qemuDomainMasterKeyFree(priv); VIR_FREE(priv); 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 @@ -175,6 +175,43 @@ VIR_ENUM_DECL(qemuDomainNamespace) bool qemuDomainNamespaceEnabled(virDomainObjPtr vm, qemuDomainNamespace ns); +/* 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; +}; + +# define QEMU_DOMAIN_AES_IV_LEN 16 /* 16 bytes for 128 bit random */ + /* initialization vector */ +typedef struct _qemuDomainSecretAES qemuDomainSecretAES; +typedef struct _qemuDomainSecretAES *qemuDomainSecretAESPtr; +struct _qemuDomainSecretAES { + char *username; + char *alias; /* generated alias for secret */ + char *iv; /* base64 encoded initialization vector */ + char *ciphertext; /* encoded/encrypted secret */ +}; + +typedef struct _qemuDomainSecretInfo qemuDomainSecretInfo; +typedef qemuDomainSecretInfo *qemuDomainSecretInfoPtr; +struct _qemuDomainSecretInfo { + qemuDomainSecretInfoType type; + union { + qemuDomainSecretPlain plain; + qemuDomainSecretAES aes; + } s; +}; + typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { @@ -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; }; -# define QEMU_DOMAIN_AES_IV_LEN 16 /* 16 bytes for 128 bit random */ - /* initialization vector */ -typedef struct _qemuDomainSecretAES qemuDomainSecretAES; -typedef struct _qemuDomainSecretAES *qemuDomainSecretAESPtr; -struct _qemuDomainSecretAES { - char *username; - char *alias; /* generated alias for secret */ - char *iv; /* base64 encoded initialization vector */ - char *ciphertext; /* encoded/encrypted secret */ -}; - -typedef struct _qemuDomainSecretInfo qemuDomainSecretInfo; -typedef qemuDomainSecretInfo *qemuDomainSecretInfoPtr; -struct _qemuDomainSecretInfo { - qemuDomainSecretInfoType type; - union { - qemuDomainSecretPlain plain; - qemuDomainSecretAES aes; - } s; -}; +# define QEMU_DOMAIN_PRIVATE(vm) \ + ((qemuDomainObjPrivatePtr) (vm)->privateData) # define QEMU_DOMAIN_DISK_PRIVATE(disk) \ ((qemuDomainDiskPrivatePtr) (disk)->privateData) @@ -730,6 +738,9 @@ int qemuDomainMasterKeyCreate(virDomainObjPtr vm); void qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv); +void qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo) + ATTRIBUTE_NONNULL(1); + void qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) ATTRIBUTE_NONNULL(1); @@ -739,6 +750,12 @@ bool qemuDomainSecretDiskCapable(virStorageSourcePtr src) bool qemuDomainDiskHasEncryptionSecret(virStorageSourcePtr src) ATTRIBUTE_NONNULL(1); +qemuDomainSecretInfoPtr +qemuDomainSecretInfoTLSNew(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + const char *srcAlias, + const char *secretUUID); + int qemuDomainSecretDiskPrepare(virConnectPtr conn, qemuDomainObjPrivatePtr priv, virDomainDiskDefPtr disk) 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 + * 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")); + 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 */ + if (qemuDomainAddTLSObjects(driver, vm, asyncJob, *secAlias, &secProps, + *tlsAlias, &tlsProps) < 0) + return -1; + + /* Set the param used for 'tls-creds' */ + 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; + } + 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; +} + + +/* 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) +{ + 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, @@ -3623,6 +3928,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, { virDomainObjPtr vm = NULL; virObjectEventPtr event = NULL; + virQEMUDriverConfigPtr cfg = NULL; int ret = -1; int dataFD[2] = { -1, -1 }; qemuDomainObjPrivatePtr priv = NULL; @@ -3636,6 +3942,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, bool stopProcess = false; bool relabel = false; int rv; + char *tlsAlias = NULL; + char *secAlias = NULL; qemuMonitorMigrationParams migParams = { 0 }; virNWFilterReadLockFilterUpdates(); @@ -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 */ + 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 */ + if ((qemuMigrationCheckTLSCreds(driver, vm, + QEMU_ASYNC_JOB_MIGRATION_IN) < 0) || + (qemuMigrationSetEmptyTLSParams(priv, &migParams) < 0)) + goto stopjob; + } + if (STREQ_NULLABLE(protocol, "rdma") && virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 0) { goto stopjob; @@ -3914,6 +4248,9 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, ret = 0; cleanup: + VIR_FREE(tlsAlias); + VIR_FREE(secAlias); + virObjectUnref(cfg); qemuProcessIncomingDefFree(incoming); VIR_FREE(xmlout); VIR_FORCE_CLOSE(dataFD[0]); @@ -3940,6 +4277,10 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, return ret; stopjob: + ignore_value(qemuMigrationDeconstructTLS(driver, vm, + QEMU_ASYNC_JOB_MIGRATION_IN, + tlsAlias, secAlias)); + if (stopProcess) { unsigned int stopFlags = VIR_QEMU_PROCESS_STOP_MIGRATED; if (!relabel) @@ -6415,6 +6756,9 @@ qemuMigrationFinish(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_MIGRATION_IN); } + if (qemuMigrationResetTLS(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) + goto endjob; + qemuMigrationJobFinish(driver, vm); if (!virDomainObjIsActive(vm)) qemuDomainRemoveInactive(driver, vm); -- 2.9.3

On 03/22/2017 12:26 PM, Jiri Denemark wrote: Hmm. thought I adjusted this already... Must've been thinking about it... Why would someone use TLS for snapshots, saving domain to a fail, and other similar stuff? What am I missing? I don't mind adding the extra call. If the TLS parameters *only* get adjusted by libvirt during PrepareAny and Run, but both those phases cause reset of the migration back to the original source on failure *and* Cancel does the reset - then I'm missing the connection. It determines whether or not TLS is possible... It can ensure that when not using TLS for migration that we set the params to "" if they aren't already set that way. Yes it does - 2.9-rc0 initializes it to "" while it was NULL beforehand. See QEMU commit '4af245dc3'. Well I always find it difficult to stumble upon code and wonder why something is being done a particular way. So if I over state what something does, is it really a bad thing? I can work to reduce the repetition though... OK that's fine. Fine - I'll remove them Perhaps I'm over thinking... I guess I was going down the path of clearing them when we didn't set them, but then again - I don't change the "migTLSAlias" so sure I can remove the above 3 checks. One is called from Cancel which wouldn't have the aliases on input while qemuMigrationRun and qemuMigrationPrepareAny... The SetEmptyTLSParams is used to ensure when we're doing a non TLS migration that the tls-creds and tls-hostname if of course the parameters are supported. Guess it wasn't self documenting. I'll look to rework the reset logic. The destination cannot have tls-hostname set - it's a source parameter. If the target was a source and for some reason the parameter was set, then just in case we'll be sure to clear it. OK - here and in the next patch too. John

https://bugzilla.redhat.com/show_bug.cgi?id=1300769 If the migration flags indicate this migration will be using TLS, then while we have connection in the Begin phase check and setup the TLS environment that will be used by virMigrationRun during the Perform phase for the source to configure TLS. This creates at least an "-object tls-creds-x509,endpoint=client,..." and potentially an "-object secret,..." to handle the passphrase response 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 source (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. Since qemuProcessRecoverMigrationOut will cancel outgoing migrations that are still in the QEMU_MIGRATION_PHASE_PERFORM{2|3} stages, there's no need to do anything special as the Perform cleanup and Cancel phases will reset the environment. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 42074f0..5acae6e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3453,6 +3453,7 @@ qemuMigrationBegin(virConnectPtr conn, unsigned long flags) { virQEMUDriverPtr driver = conn->privateData; + virQEMUDriverConfigPtr cfg = NULL; char *xml = NULL; qemuDomainAsyncJob asyncJob; @@ -3486,6 +3487,12 @@ qemuMigrationBegin(virConnectPtr conn, nmigrate_disks, migrate_disks, flags))) goto endjob; + if (flags & VIR_MIGRATE_TLS) { + cfg = virQEMUDriverGetConfig(driver); + if (qemuMigrationCheckSetupTLS(conn, driver, cfg, vm, asyncJob) < 0) + goto endjob; + } + if ((flags & VIR_MIGRATE_CHANGE_PROTECTION)) { /* We keep the job active across API calls until the confirm() call. * This prevents any other APIs being invoked while migration is taking @@ -3502,6 +3509,7 @@ qemuMigrationBegin(virConnectPtr conn, } cleanup: + virObjectUnref(cfg); virDomainObjEndAPI(&vm); return xml; @@ -5010,8 +5018,11 @@ qemuMigrationRun(virQEMUDriverPtr driver, { int ret = -1; unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; + virQEMUDriverConfigPtr cfg = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; qemuMigrationCookiePtr mig = NULL; + char *tlsAlias = NULL; + char *secAlias = NULL; qemuMigrationIOThreadPtr iothread = NULL; int fd = -1; unsigned long migrate_speed = resource ? resource : priv->migMaxBandwidth; @@ -5075,6 +5086,38 @@ qemuMigrationRun(virQEMUDriverPtr driver, if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig, graphicsuri) < 0) VIR_WARN("unable to provide data for graphics client relocation"); + if (flags & VIR_MIGRATE_TLS) { + cfg = virQEMUDriverGetConfig(driver); + + /* Begin/CheckSetupTLS already set up migTLSAlias, the following + * assumes that and adds the TLS objects to the domain. */ + if (qemuMigrationAddTLSObjects(driver, vm, cfg, false, + QEMU_ASYNC_JOB_MIGRATION_OUT, + &tlsAlias, &secAlias, migParams) < 0) + goto cleanup; + + /* We need to add the tls-hostname only for special circumstances, + * e.g. for a fd: or exec: based migration. As it turns out the + * CONNECT_HOST turns into an FD migration (see below). */ + if (spec->destType == MIGRATION_DEST_CONNECT_HOST || + spec->destType == MIGRATION_DEST_FD) { + if (VIR_STRDUP(migParams->migrateTLSHostname, + spec->dest.host.name) < 0) + goto cleanup; + } else { + /* Be sure there's nothing from a previous migration */ + if (VIR_STRDUP(migParams->migrateTLSHostname, "") < 0) + goto cleanup; + } + } else { + /* If we support setting the tls-creds, be sure to always reset + * the migration parameters when this migration isn't using TLS */ + if ((qemuMigrationCheckTLSCreds(driver, vm, + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) || + (qemuMigrationSetEmptyTLSParams(priv, migParams) < 0)) + goto cleanup; + } + if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK | QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) { if (mig->nbd) { @@ -5255,6 +5298,14 @@ qemuMigrationRun(virQEMUDriverPtr driver, ret = -1; } + if (qemuMigrationDeconstructTLS(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT, + tlsAlias, secAlias) < 0) + ret = -1; + + VIR_FREE(tlsAlias); + VIR_FREE(secAlias); + virObjectUnref(cfg); + if (spec->fwdType != MIGRATION_FWD_DIRECT) { if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0) ret = -1; @@ -6958,6 +7009,8 @@ qemuMigrationCancel(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0 || (storage && !blockJobs)) goto endsyncjob; + ignore_value(qemuMigrationResetTLS(driver, vm, QEMU_ASYNC_JOB_NONE)); + if (!storage) { ret = 0; goto cleanup; -- 2.9.3

On 03/22/2017 12:26 PM, Jiri Denemark wrote: I can reword OK - so as I figured out things the RecoverMigrationOut only mattered when certain migration phase requirements were met - that wouldn't happen because the setting and clearing of the parameters is done in the PrepareAny and Run phases as well as Cancel. Sure - something that wasn't obvious on my first pass through the code. In fact having CONNECT_HOST change into FD later on wasn't as obvious to me until I dug through the code. I can change the comment to: /* We need to add tls-hostname whenever QEMU itself does not * connect directly to the destination. NB: The CONNECT_HOST * turns into a FD migration below in qemuMigrationConnect */ and I've fixed the extra () here as well John

Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/news.xml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 3501f89..25d920c 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -39,6 +39,16 @@ the QEMU binary supports it. </description> </change> + <change> + <summary> + qemu: Add support to migrate using TLS + </summary> + <description> + Add the ability to migrate QEMU guests using TLS via a new flag + VIR_MIGRATE_TLS or virsh '--tls' option. Requires using at least + QEMU 2.9.0 in order to work properly. + </description> + </change> </section> <section title="Improvements"> <change> -- 2.9.3