Re: [PATCH v3 2/7] qemu: Manage a pre-shared key's lifecycle

On Wed, Jul 29, 2026 at 08:58:54 +0000, Abhisek Panda wrote:

Libvirt falls back to the TLS-PSK-enabled VM migration, if the VIR_MIGRATE_TLS flag is set but the destination host lacks necessary X.509 credentials (ca-cert.pem, server-cert.pem and server-key.pem). The source host unconditionally adds the pre-shared key in the migration cookie if the VIR_MIGRATE_TLS flag is set. Upon parsing the migration cookie, the destination host checks for the presence of X.509 credentials and informs the source host whether to use TLS X.509 or TLS PSK during VM migration via the migration cookie.

For a migration session, Libvirt generates a random key of the specified length, and then stores the content, "qemu:<random key>", at <tls_psk_state_dir>/$ID-$VMNAME/keys.psk on the source host. This is because QEMU's tls-creds-psk object does not accept a raw key string as a parameter, it only accepts a dir argument pointing to a directory from which it can read the key file. Subsequently, it sends the key to destination by embedding it within the migration cookie. The destination's Libvirt extracts the key from the migration cookie. Upon migration completion or any failure, both source and destination Libvirt must delete the directory containing the session's keys.psk.

Signed-off-by: Abhisek Panda <abhisek.panda1@nutanix.com> --- src/qemu/qemu_conf.c | 4 + src/qemu/qemu_conf.h | 1 + src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 6 ++ src/qemu/qemu_migration.c | 119 +++++++++++++++++++++++++++++ src/qemu/qemu_migration_cookie.c | 94 ++++++++++++++++++++++- src/qemu/qemu_migration_cookie.h | 5 ++ tests/qemumigrationcookiexmltest.c | 18 +++-- 9 files changed, 241 insertions(+), 8 deletions(-)

[...]

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4a43ab83b0..3d6e472443 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c

[...]

+static int +qemuMigrationPersistPSK(virQEMUDriver *driver, virDomainObj *vm, const char *tlsPSK) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autofree char *dir_path = NULL; + g_autofree char *key_path = NULL; + g_autofree char *shortName = NULL; + + if (!vm || !vm->def || !(shortName = virDomainDefGetShortName(vm->def))) + return -1;

This error path mixes situations where a libvirt error is raised (below) and when no error is reported. We don't allow that because the caller can't then know if an error was reported, thus all code paths must report error or all must not report error. How can 'vm' or 'vm->def' even be NULL here? Does this check even make sense?

+ + dir_path = g_strdup_printf("%s/%s", cfg->tlsPSKStateDir, shortName); + key_path = g_strdup_printf("%s/keys.psk", dir_path); + + if (virDirCreate(dir_path, 0700, cfg->user, cfg->group, + VIR_DIR_CREATE_ALLOW_EXIST) < 0) { + virReportSystemError(errno, + _("Could not create the directory %1$s for storing PSKs"), + dir_path); + goto error; + } + /> + if (tlsPSK) { + if (virFileRewrite(key_path, S_IRUSR, cfg->user, + cfg->group, qemuPersistTLSPSKHelper, + tlsPSK) < 0) + goto error; + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("The pre-shared key for TLS-PSK migration is not provided"));

Can this happen? Why would the caller then call this function? And why would you create the directory first before reporting this?

+ goto error; + } + + return 0; + + error: + qemuMigrationDeletePSKDir(driver, vm); + return -1; +} + + static int qemuDomainGetMigrationBlockers(virDomainObj *vm, int asyncJob, @@ -2718,6 +2804,7 @@ qemuMigrationSrcBeginXML(virDomainObj *vm, qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; g_autoptr(qemuMigrationCookie) mig = NULL; + int ret;

if (priv->origCPU) cookieFlags |= QEMU_MIGRATION_COOKIE_CPU; @@ -2725,6 +2812,9 @@ qemuMigrationSrcBeginXML(virDomainObj *vm, if (!(flags & VIR_MIGRATE_OFFLINE)) cookieFlags |= QEMU_MIGRATION_COOKIE_CAPS;

+ if (flags & VIR_MIGRATE_TLS) + cookieFlags |= QEMU_MIGRATION_COOKIE_TLS_PSK; + if (!(mig = qemuMigrationCookieNew(vm->def, priv->origname))) return NULL;

@@ -2738,6 +2828,15 @@ qemuMigrationSrcBeginXML(virDomainObj *vm, cookieFlags) < 0) return NULL;

+ if ((flags & VIR_MIGRATE_TLS) && mig->tlsPSK) { + ret = qemuMigrationPersistPSK(driver, vm, mig->tlsPSK);

So, since this is the only caller and it's guaranteed that tlsPSK exists and 'vm' is non, null here, all the nonsense checks I pointed out above can be removed. Also the PSK should be written to the disk at any point when it will be used, which is decided before, so the flag check of VIR_MIGRATE_TLS doesn't make sense.

+ if (ret < 0) { + virSecureEraseString(mig->tlsPSK); + g_clear_pointer(&mig->tlsPSK, g_free); + return NULL;

This belongs to a common cleanup path after failed/completed migration, not this random location.

+ } + } + if (xmlin) { g_autoptr(virDomainDef) def = NULL;

@@ -4232,6 +4331,9 @@ qemuMigrationSrcConfirmPhase(virQEMUDriver *driver, privJob->stats.mig.downtime = privMigJob->stats.mig.downtime; }

+ if (flags & VIR_MIGRATE_TLS) + qemuMigrationDeletePSKDir(driver, vm); + if (flags & VIR_MIGRATE_OFFLINE) return 0;

@@ -5275,6 +5377,9 @@ qemuMigrationSrcRun(virQEMUDriver *driver, error: virErrorPreserveLast(&orig_err);

+ if (flags & VIR_MIGRATE_TLS) + qemuMigrationDeletePSKDir(driver, vm);

IMO this should't be gated by the flag check but rather by whether the PSK dir was set up and all of it inside qemuMigrationDeletePSKDir so that any furher change doesn't need to fix all callers.

+ if (qemuDomainObjIsActive(vm)) { int reason; virDomainState state = virDomainObjGetState(vm, &reason); @@ -7029,6 +7134,9 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, QEMU_MIGRATION_COOKIE_STATS) < 0) VIR_WARN("Unable to encode migration cookie");

+ if (flags & VIR_MIGRATE_TLS) + qemuMigrationDeletePSKDir(driver, vm);

ditto

+ qemuMigrationDstComplete(driver, vm, inPostCopy, VIR_ASYNC_JOB_MIGRATION_IN, vm->job);

@@ -7039,6 +7147,9 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, * overwrites it. */ virErrorPreserveLast(&orig_err);

+ if (flags & VIR_MIGRATE_TLS) + qemuMigrationDeletePSKDir(driver, vm);

same here

+ if (qemuDomainObjIsActive(vm)) { if (doKill) { qemuProcessStop(vm, VIR_DOMAIN_SHUTOFF_FAILED, @@ -7197,6 +7308,14 @@ qemuMigrationProcessUnattended(virQEMUDriver *driver, else qemuMigrationSrcComplete(driver, vm, job);

+ /* + * Attempt to clean up the directory containing the pre-shared keys + * for the domain. Since, we cannot determine if the migration has + * enabled the VIR_MIGRATE_TLS flag with pre-shared keys, we clean up + * the directory unconditionally. + */ + qemuMigrationDeletePSKDir(driver, vm);

And here you then don't need the comment.

+ qemuMigrationJobFinish(vm);

if (!virDomainObjIsActive(vm)) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 7311a8294b..1a7b9361da 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c

@@ -149,6 +153,17 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuMigrationBlockDirtyBitmapsDisk, qemuMigrationBlockDirtyBitmapsDiskFree);

+static bool +qemuMigrationServerCertsExists(virQEMUDriver *driver) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autofree char *cacert_path = g_strdup_printf("%s/ca-cert.pem", cfg->migrateTLSx509certdir); + g_autofree char *key_path = g_strdup_printf("%s/server-key.pem", cfg->migrateTLSx509certdir); + g_autofree char *cert_path = g_strdup_printf("%s/server-cert.pem", cfg->migrateTLSx509certdir); + return virFileExists(cacert_path) && virFileExists(key_path) && virFileExists(cert_path); +} + + void qemuMigrationCookieFree(qemuMigrationCookie *mig) { @@ -165,6 +180,9 @@ qemuMigrationCookieFree(qemuMigrationCookie *mig) g_free(mig->name); g_free(mig->lockState); g_free(mig->lockDriver); + if (mig->tlsPSK) + virSecureEraseString(mig->tlsPSK); + g_free(mig->tlsPSK);

So you do have a common cleanup path.

g_clear_pointer(&mig->jobData, virDomainJobDataFree); virCPUDefFree(mig->cpu); qemuMigrationCookieCapsFree(mig->caps); @@ -575,6 +593,51 @@ qemuMigrationCookieAddCaps(qemuMigrationCookie *mig, }

+static int +qemuMigrationCookieAddTLSPSK(qemuMigrationCookie *mig, + virQEMUDriver *driver, + virDomainObj *vm) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivate *priv = vm->privateData; + gnutls_datum_t psk_key = {NULL, 0}; + g_autofree char *key = NULL; + size_t key_len; + int ret; + + /* Generate the pre-shared key exactly once for a migration session*/ + if (priv->migTLSPSK) { + mig->tlsPSK = g_strdup(priv->migTLSPSK); + mig->flags |= QEMU_MIGRATION_COOKIE_TLS_PSK; + return 0; + } + + ret = gnutls_key_generate(&psk_key, cfg->migrateTLSPSKLength); + if (ret < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Generation of a pre-shared key failed")); + return -1; + } + key_len = (psk_key.size*2) + 1; + key = g_new0(char, key_len); + + ret = gnutls_hex_encode(&psk_key, key, &key_len); + if (ret < 0) { + gnutls_free(psk_key.data); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Hex encoding of a PSK key failed")); + return -1; + } + + priv->migTLSPSK = g_strdup(key); + mig->tlsPSK = g_steal_pointer(&key); + mig->flags |= QEMU_MIGRATION_COOKIE_TLS_PSK; + + gnutls_free(psk_key.data); + return 0; +} + + static void qemuMigrationCookieGraphicsXMLFormat(virBuffer *buf, qemuMigrationCookieGraphics *grap) @@ -890,6 +953,9 @@ qemuMigrationCookieXMLFormat(virQEMUDriver *driver, if (mig->flags & QEMU_MIGRATION_COOKIE_BLOCK_DIRTY_BITMAPS) qemuMigrationCookieBlockDirtyBitmapsFormat(buf, mig->blockDirtyBitmaps);

+ if ((mig->flags & QEMU_MIGRATION_COOKIE_TLS_PSK) && mig->tlsPSK) + virBufferAsprintf(buf, "<migration-key>%s</migration-key>\n", mig->tlsPSK); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</qemu-migration>\n"); return 0; @@ -1396,6 +1462,12 @@ qemuMigrationCookieXMLParse(qemuMigrationCookie *mig, qemuMigrationCookieBlockDirtyBitmapsParse(ctxt, mig) < 0) return -1;

+ if (flags & QEMU_MIGRATION_COOKIE_TLS_PSK) { + mig->tlsPSK = virXPathString("string(./migration-key[1])", ctxt); + if (mig->tlsPSK) + mig->flags |= QEMU_MIGRATION_COOKIE_TLS_PSK; + } + return 0; }

@@ -1471,14 +1543,17 @@ qemuMigrationCookieFormat(qemuMigrationCookie *mig, qemuMigrationCookieAddCaps(mig, dom, party) < 0) return -1;

+ if (flags & QEMU_MIGRATION_COOKIE_TLS_PSK && + party == QEMU_MIGRATION_SOURCE && + qemuMigrationCookieAddTLSPSK(mig, driver, dom) < 0) + return -1; + if (qemuMigrationCookieXMLFormat(driver, priv->qemuCaps, &buf, mig) < 0) return -1;

*cookieoutlen = virBufferUse(&buf) + 1; *cookieout = virBufferContentAndReset(&buf);

- VIR_DEBUG("cookielen=%d cookie=%s", *cookieoutlen, *cookieout); -

Why is this debug statement deleted?

return 0; }

@@ -1537,6 +1613,20 @@ qemuMigrationCookieParse(virQEMUDriver *driver, } }

+ if (flags & QEMU_MIGRATION_COOKIE_TLS_PSK) { + if (!qemuMigrationServerCertsExists(driver)) { + if (!mig->tlsPSK) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("destination host has no X.509 certificates configured for migration and source host did not provide a pre-shared key")); + return NULL; + } + } else {

So, if the destination has x509 certs this signals that PSK is not supported? What if the source doesn't have them. IMO we want to use PSK if available.

+ virSecureEraseString(mig->tlsPSK); + mig->flags &= ~QEMU_MIGRATION_COOKIE_TLS_PSK; + g_clear_pointer(&mig->tlsPSK, g_free); + } + } + if (vm && flags & QEMU_MIGRATION_COOKIE_STATS && mig->jobData && vm->job->current) mig->jobData->operation = vm->job->current->operation;