> On 20 Aug 2026, at 6:25 PM, Peter Krempa <[email protected]> wrote:
>
> !-------------------------------------------------------------------|
> CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> 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 <[email protected]>
>> ---
>> 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?
>
Acknowledged. Since ‘vm’ or ‘vm->def’ are guaranteed to be non NULL, I will drop
the unnecessary checks. If virDomainDefGetShortName returns NULL, I will
explicitly report VIR_ERR_INTERNAL_ERROR to keep error reporting consistent.
>
>> +
>> + 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?
The caller guarantees tlsPSK is present when invoking this, so I have removed
the tlsPSK
check entirely.
>
>> + 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.
Acknowledged.
>
> 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.
Acknowledged. I will update this to check ‘if mig->tlsPSK’ directly instead of
Re-checking the flag.
>
>
>> + 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.
Acknowledged. The cookie cleanup path will handle freeing and erasing sensitive
string.
>
>> + }
>> + }
>> +
>> 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.
Acknowledged. qemuMigrationDeletePSKDir will check for the existence of
the PSKDir. If it is present then we clean it up else we just return. Therefore,
we do not need any explicit check on the flag.
>
>
>> +
>> 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
Acknowledged
>
>> +
>> 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
Acknowledged
>
>> +
>> 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.
Acknowledged
>
>
>> +
>> 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?
My intention was to prevent the raw PSK key string embedded in XML from being
written to debug logs.
>
>> 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.
I have the following understanding of the pre-shared key:
1. QEMU added support for tls-creds-psk objects back in v3.0. Since libvirt now
targets
QEMU >= 7.2, we can safely rely on tls-creds-psk support being present
unconditionally
on both src and dst without needing feature capability probes.
2. Following our previous discussion in v2 on letting the destination decide
whether to use
PSK or certificates, the current patch had destination prefer x509 whenever
x509 certs were available
(qemuMigrationServerCertsExists), falling back to PSK otherwise. However, as
you rightly noted,
this assumes src has certs ready, which isn't guaranteed.
So moving forward we have 2 design choices?
1. The source sends mig->tlsPSK along with an x509_present flag in the
migration cookie.
The destination uses x509 only if both sides have certs; otherwise, it falls
back to PSK.
2. If `mig->tlsPSK` is present in the cookie, the destination always selects
PSK for the session,
bypassing the X.509 check entirely.
Since, both the source and destination supports tls-creds-psk, I am confused
with the statement:
“IMO we want to use PSK If available“. Can you please explain this in detail?
>
>> + 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;
>>
>
