> On 10 Jul 2026, at 8:23 PM, Peter Krempa <[email protected]> wrote: > > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > On Tue, Jun 23, 2026 at 06:26:00 +0000, Abhisek Panda wrote: >> For enabling a TLS-PSK-enabled VM migration, we rely on the >> VIR_MIGRATE_TLS migration flag and existence of ca-cert.pem on the > > These comments are based on the commit message only: > > I don't think the source should make this decision. IMO the source > should always generate and offer a PSK key to the destination even if it > has an x509 certificate avaliable. > > The destination can then make the decision. The destination side will > certainly signal if it supports it by formatting the field back in the > cookie so the source knows which one to use. > > I'm also wondering if we should just use PSK instead of x509 even if > both sides support it. >
In v3 patch series, the destination side signals if the migration will utilize TLS PSK-based authentication or TLS X.509-based authentication via the migration cookie. IMHO, using a pre-shared key (PSK) is preferable to X.509 certificates in VM migration. This is because each migration session utilizes a unique key, any potential key compromise is strictly contained to that migration session. In addition, we can reduce our reliance on a certificate authority. >> source. If the migration flag is set and ca-cert.pem doesn't exist >> on the source, then Libvirt fallbacks to PSK-based migration >> instead of X.509. Subsequently, it handles the generation, persistent >> storage, and cleanup of pre-shared keys on both source and destination. >> >> For a migration session, Libvirt generates a random key of the >> specified length, and then stores the content, "qemu:<random key>", at >> <runtime_state_dir>/<vm_uuid>/keys.psk on the source host. >> 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, and then persistently store it exactly the same >> way as the source. Upon migration completion or any failure, both >> source and destination Libvirt deletes the directory containing the >> session's keys.psk. >> >> Signed-off-by: Abhisek Panda <[email protected]> >> --- >> src/qemu/qemu_migration.c | 53 ++++++++++++ >> src/qemu/qemu_migration_cookie.c | 125 +++++++++++++++++++++++++++++ >> src/qemu/qemu_migration_cookie.h | 5 ++ >> tests/qemumigrationcookiexmltest.c | 12 +-- >> 4 files changed, 190 insertions(+), 5 deletions(-) >> >> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c >> index 4a43ab83b0..72e13f854b 100644 >> --- a/src/qemu/qemu_migration.c >> +++ b/src/qemu/qemu_migration.c >> @@ -1503,6 +1503,35 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef >> *def) >> } >> >> >> +static bool >> +qemuMigrationCACertExists(virQEMUDriver *driver) >> +{ >> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); >> + g_autofree char *cert_path = g_strdup_printf("%s/ca-cert.pem", >> cfg->migrateTLSx509certdir); >> + if (!virFileExists(cert_path)) >> + return false; > > just 'return virFileExists(cert_path);' > > >> + >> + return true; >> +} >> + >> + >> +static void >> +qemuMigrationDeletePSKDir(virQEMUDriver *driver, virDomainObj *vm) >> +{ >> + char uuidstr[VIR_UUID_STRING_BUFLEN]; >> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); >> + g_autofree char *dir_path = NULL; >> + >> + virUUIDFormat(vm->def->uuid, uuidstr); >> + dir_path = g_strdup_printf("%s/%s", cfg->stateDir, uuidstr); > > So this would put the keys to /run/libvirt/qemu/UUID. That directory is > organized a bit different (there are dirs based on type of object they > store and then dirs identifying the VM by '$ID-$VMNAME' instead of > UUID). > > Please keep it consistent. Acknowledged in v3. > > >> + >> + if (virFileIsDir(dir_path) && >> + virFileDeleteTree(dir_path) < 0) >> + VIR_WARN("Failed to delete the directory %s containing the >> pre-shared keys for migration of domain %s", >> + dir_path, vm->def->name); >> +} >> + >> + >> static int >> qemuDomainGetMigrationBlockers(virDomainObj *vm, >> int asyncJob, >> @@ -2725,6 +2754,10 @@ qemuMigrationSrcBeginXML(virDomainObj *vm, >> if (!(flags & VIR_MIGRATE_OFFLINE)) >> cookieFlags |= QEMU_MIGRATION_COOKIE_CAPS; >> >> + if ((flags & VIR_MIGRATE_TLS) && >> + !qemuMigrationCACertExists(driver)) >> + cookieFlags |= QEMU_MIGRATION_COOKIE_TLS_PSK; > > As said above, the source should always offer PSK. In fact once this is > merged I'll modify it to always offer PSK so that even if > VIR_MIGRATE_TLS is not used but both sides support PSK they will encrypt > the communication. > > We need to switch to safe channels by default. > IIUC, enabling TLS during VM migration will incur performance overhead, which some users might not want. Therefore, IMHO we can keep it as a configurable parameter. > >> + >> if (!(mig = qemuMigrationCookieNew(vm->def, priv->origname))) >> return NULL; >> >> @@ -4232,6 +4265,9 @@ qemuMigrationSrcConfirmPhase(virQEMUDriver *driver, >> privJob->stats.mig.downtime = privMigJob->stats.mig.downtime; >> } >> >> + if ((flags & VIR_MIGRATE_TLS) && !qemuMigrationCACertExists(driver)) >> + qemuMigrationDeletePSKDir(driver, vm); > > the directory should IMO be deleted always. the function seems to not > produce eerror when it doesn't exist. > > >> + >> if (flags & VIR_MIGRATE_OFFLINE) >> return 0; >> >> @@ -5275,6 +5311,9 @@ qemuMigrationSrcRun(virQEMUDriver *driver, >> error: >> virErrorPreserveLast(&orig_err); >> >> + if ((flags & VIR_MIGRATE_TLS) && !qemuMigrationCACertExists(driver)) >> + qemuMigrationDeletePSKDir(driver, vm); > > Same here. > > There should be also a single place where we can put this cleanup > though. Upon migration completion, we execute qemuMigrationSrcConfirmPhase on the source and qemuMigrationDstFinishActive on the destination. I feel that we need to keep invoking qemuMigrationDeletePSKDir in separate places. > >> + >> if (qemuDomainObjIsActive(vm)) { >> int reason; >> virDomainState state = virDomainObjGetState(vm, &reason); >> @@ -7029,6 +7068,9 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, >> QEMU_MIGRATION_COOKIE_STATS) < 0) >> VIR_WARN("Unable to encode migration cookie"); >> >> + if (flags & VIR_MIGRATE_TLS) >> + qemuMigrationDeletePSKDir(driver, vm); > > As you see if the logic changes places can forget this. > > >> + >> qemuMigrationDstComplete(driver, vm, inPostCopy, >> VIR_ASYNC_JOB_MIGRATION_IN, vm->job); >> >> @@ -7039,6 +7081,9 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, >> * overwrites it. */ >> virErrorPreserveLast(&orig_err); >> >> + if (flags & VIR_MIGRATE_TLS) >> + qemuMigrationDeletePSKDir(driver, vm); >> + >> if (qemuDomainObjIsActive(vm)) { >> if (doKill) { >> qemuProcessStop(vm, VIR_DOMAIN_SHUTOFF_FAILED, >> @@ -7197,6 +7242,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); >> + >> qemuMigrationJobFinish(vm); >> >> if (!virDomainObjIsActive(vm)) >> diff --git a/src/qemu/qemu_migration_cookie.c >> b/src/qemu/qemu_migration_cookie.c >> index 7311a8294b..7734966983 100644 >> --- a/src/qemu/qemu_migration_cookie.c >> +++ b/src/qemu/qemu_migration_cookie.c >> @@ -20,9 +20,11 @@ >> >> #include <gnutls/gnutls.h> >> #include <gnutls/x509.h> >> +#include <inttypes.h> >> >> #include "locking/domain_lock.h" >> #include "virerror.h" >> +#include "virfile.h" >> #include "virlog.h" >> #include "virnetdevopenvswitch.h" >> #include "virstring.h" >> @@ -52,6 +54,7 @@ VIR_ENUM_IMPL(qemuMigrationCookieFlag, >> "allowReboot", >> "capabilities", >> "block-dirty-bitmaps", >> + "psk", >> ); >> >> >> @@ -149,6 +152,66 @@ >> G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuMigrationBlockDirtyBitmapsDisk, >> qemuMigrationBlockDirtyBitmapsDiskFree); >> >> >> +static int >> +qemuPersistTLSPSKHelper(int pskFD, >> + const char *pskPath, >> + const void *opaque) >> +{ >> + const char *key = opaque; >> + g_autofree char *psk_content = NULL; >> + >> + psk_content = g_strdup_printf("qemu:%s", key); > > > just do another safewrite call here to write the prefix to avoid extra > copy of the secret. > >> + >> + if (safewrite(pskFD, psk_content, strlen(psk_content)) < 0) { >> + virReportSystemError(errno, >> + _("Unable to write the pre-shared key to file >> '%1$s'"), >> + pskPath); >> + return -1; >> + } >> + >> + return 0; >> +} >> + >> + >> +static int >> +qemuMigrationPersistPSK(qemuMigrationCookie *mig, virQEMUDriverConfig *cfg) >> +{ >> + char uuidstr[VIR_UUID_STRING_BUFLEN]; >> + g_autofree char *dir_path = NULL; >> + g_autofree char *key_path = NULL; >> + >> + virUUIDFormat(mig->uuid, uuidstr); >> + dir_path = g_strdup_printf("%s/%s", cfg->stateDir, uuidstr); >> + key_path = g_strdup_printf("%s/keys.psk", dir_path); > > See note about naming above. > > >> + >> + 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 (mig->tlsPSK) { >> + if (virFileRewrite(key_path, S_IRUSR, cfg->user, >> + cfg->group, qemuPersistTLSPSKHelper, >> + mig->tlsPSK) < 0) >> + goto error; >> + } else { >> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", >> + _("The pre-shared key for TLS-PSK migration is >> missing in the migration cookie")); >> + goto error; >> + } >> + >> + return 0; >> + >> + error: >> + if (virFileExists(dir_path)) >> + virFileDeleteTree(dir_path); >> + return -1; >> +} >> + >> + >> void >> qemuMigrationCookieFree(qemuMigrationCookie *mig) >> { >> @@ -165,6 +228,7 @@ qemuMigrationCookieFree(qemuMigrationCookie *mig) >> g_free(mig->name); >> g_free(mig->lockState); >> g_free(mig->lockDriver); >> + g_free(mig->tlsPSK); >> g_clear_pointer(&mig->jobData, virDomainJobDataFree); >> virCPUDefFree(mig->cpu); >> qemuMigrationCookieCapsFree(mig->caps); >> @@ -575,6 +639,48 @@ qemuMigrationCookieAddCaps(qemuMigrationCookie *mig, >> } >> >> >> +static int >> +qemuMigrationCookieAddTLSPSK(qemuMigrationCookie *mig, virQEMUDriver >> *driver) >> +{ >> + gnutls_datum_t psk_key = {NULL, 0}; >> + g_autofree char *key = NULL; >> + size_t key_len; >> + int ret; >> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); >> + >> + 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; >> + } >> + >> + mig->tlsPSK = g_steal_pointer(&key); >> + mig->flags |= QEMU_MIGRATION_COOKIE_TLS_PSK; >> + >> + ret = qemuMigrationPersistPSK(mig, cfg); >> + if (ret < 0) { >> + gnutls_free(psk_key.data); >> + g_free(mig->tlsPSK); >> + mig->tlsPSK = NULL; > > Use g_clear_pointer to fee & set to NULL instead of open coding. > Acknowledged in v3. > >> + return -1; >> + } >> + >> + gnutls_free(psk_key.data); >> + return 0; >> +} >> + >> + >> static void >> qemuMigrationCookieGraphicsXMLFormat(virBuffer *buf, >> qemuMigrationCookieGraphics *grap) >> @@ -890,6 +996,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) >> + virBufferAsprintf(buf, "<migration-key>%s</migration-key>\n", >> mig->tlsPSK); >> + >> virBufferAdjustIndent(buf, -2); >> virBufferAddLit(buf, "</qemu-migration>\n"); >> return 0; >> @@ -1396,6 +1505,10 @@ qemuMigrationCookieXMLParse(qemuMigrationCookie *mig, >> qemuMigrationCookieBlockDirtyBitmapsParse(ctxt, mig) < 0) >> return -1; >> >> + if (flags & QEMU_MIGRATION_COOKIE_TLS_PSK) { >> + mig->tlsPSK = virXPathString("string(./migration-key[1])", ctxt); >> + } >> + >> return 0; >> } >> >> @@ -1471,6 +1584,10 @@ qemuMigrationCookieFormat(qemuMigrationCookie *mig, >> qemuMigrationCookieAddCaps(mig, dom, party) < 0) >> return -1; >> >> + if (flags & QEMU_MIGRATION_COOKIE_TLS_PSK && >> + qemuMigrationCookieAddTLSPSK(mig, driver) < 0) >> + return -1; >> + >> if (qemuMigrationCookieXMLFormat(driver, priv->qemuCaps, &buf, mig) < 0) >> return -1; >> >> @@ -1494,6 +1611,8 @@ qemuMigrationCookieParse(virQEMUDriver *driver, >> unsigned int flags) >> { >> g_autoptr(qemuMigrationCookie) mig = NULL; >> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); >> + int ret; >> >> /* Parse & validate incoming cookie (if any) */ >> if (cookiein && cookieinlen && >> @@ -1537,6 +1656,12 @@ qemuMigrationCookieParse(virQEMUDriver *driver, >> } >> } >> >> + if ((flags & QEMU_MIGRATION_COOKIE_TLS_PSK) && mig->tlsPSK) { >> + ret = qemuMigrationPersistPSK(mig, cfg); >> + if (ret < 0) >> + return NULL; >> + } >> + >> if (vm && flags & QEMU_MIGRATION_COOKIE_STATS && mig->jobData && >> vm->job->current) >> mig->jobData->operation = vm->job->current->operation; >> >> diff --git a/src/qemu/qemu_migration_cookie.h >> b/src/qemu/qemu_migration_cookie.h >> index 254372234d..fd3b4c5a56 100644 >> --- a/src/qemu/qemu_migration_cookie.h >> +++ b/src/qemu/qemu_migration_cookie.h >> @@ -35,6 +35,7 @@ typedef enum { >> QEMU_MIGRATION_COOKIE_FLAG_ALLOW_REBOOT, >> QEMU_MIGRATION_COOKIE_FLAG_CAPS, >> QEMU_MIGRATION_COOKIE_FLAG_BLOCK_DIRTY_BITMAPS, >> + QEMU_MIGRATION_COOKIE_FLAG_TLS_PSK, >> >> QEMU_MIGRATION_COOKIE_FLAG_LAST >> } qemuMigrationCookieFlags; >> @@ -53,6 +54,7 @@ typedef enum { >> QEMU_MIGRATION_COOKIE_CPU = (1 << QEMU_MIGRATION_COOKIE_FLAG_CPU), >> QEMU_MIGRATION_COOKIE_CAPS = (1 << QEMU_MIGRATION_COOKIE_FLAG_CAPS), >> QEMU_MIGRATION_COOKIE_BLOCK_DIRTY_BITMAPS = (1 << >> QEMU_MIGRATION_COOKIE_FLAG_BLOCK_DIRTY_BITMAPS), >> + QEMU_MIGRATION_COOKIE_TLS_PSK = (1 << >> QEMU_MIGRATION_COOKIE_FLAG_TLS_PSK), >> } qemuMigrationCookieFeatures; >> >> typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics; >> @@ -171,6 +173,9 @@ struct _qemuMigrationCookie { >> >> /* If flags & QEMU_MIGRATION_COOKIE_BLOCK_DIRTY_BITMAPS */ >> GSList *blockDirtyBitmaps; >> + >> + /* If flags & QEMU_MIGRATION_COOKIE_TLS_PSK */ >> + char *tlsPSK; >> }; >> >> >> diff --git a/tests/qemumigrationcookiexmltest.c >> b/tests/qemumigrationcookiexmltest.c >> index bc0f68b8c5..ee91c5d8b1 100644 >> --- a/tests/qemumigrationcookiexmltest.c >> +++ b/tests/qemumigrationcookiexmltest.c >> @@ -161,7 +161,7 @@ testQemuMigrationCookieParse(const void *opaque) >> } >> >> /* set all flags so that formatter attempts to format everything */ >> - data->cookie->flags = ~0; >> + data->cookie->flags = ~QEMU_MIGRATION_COOKIE_TLS_PSK; >> >> if (qemuMigrationCookieXMLFormat(&driver, >> priv->qemuCaps, > > > Any reason why the test is skipping this? I'd expect it to just work. In v2 patch series, the test will pass even if we do not skip QEMU_MIGRATION_COOKIE_TLS_PSK. In v3 patch series, we have added a unit test case to validate the migration-key element in the migration cookie.
