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

Abhisek Panda posted 7 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v3 2/7] qemu: Manage a pre-shared key's lifecycle
Posted by Abhisek Panda 1 month, 2 weeks ago
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_conf.c b/src/qemu/qemu_conf.c
index 986f01ddcd..86571e8da6 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -168,6 +168,7 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool privileged,
         cfg->cacheDir = g_strdup_printf("%s/cache/qemu", root);
         cfg->libDir = g_strdup_printf("%s/lib/qemu", root);
         cfg->swtpmStorageDir = g_strdup_printf("%s/lib/swtpm", root);
+        cfg->tlsPSKStateDir = g_strdup_printf("%s/run/psk", root);
 
         cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir);
         cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir);
@@ -187,6 +188,7 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool privileged,
         cfg->stateDir = g_strdup_printf("%s/libvirt/qemu", RUNSTATEDIR);
         cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir);
         cfg->channelTargetDir = g_strdup_printf("%s/channel", cfg->stateDir);
+        cfg->tlsPSKStateDir = g_strdup_printf("%s/psk", cfg->stateDir);
 
         cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR);
 
@@ -214,6 +216,7 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool privileged,
         cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir);
         cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir);
         cfg->channelTargetDir = g_strdup_printf("%s/channel", cfg->stateDir);
+        cfg->tlsPSKStateDir = g_strdup_printf("%s/psk", cfg->stateDir);
 
         cfg->configBaseDir = virGetUserConfigDirectory();
 
@@ -375,6 +378,7 @@ static void virQEMUDriverConfigDispose(void *obj)
     g_free(cfg->dbusStateDir);
     g_free(cfg->rdpStateDir);
     g_free(cfg->vncStateDir);
+    g_free(cfg->tlsPSKStateDir);
 
     g_free(cfg->libDir);
     g_free(cfg->cacheDir);
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index c18aedf59c..06a0b0770c 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -113,6 +113,7 @@ struct _virQEMUDriverConfig {
     char *dbusStateDir;
     char *rdpStateDir;
     char *vncStateDir;
+    char *tlsPSKStateDir;
     /* These two directories are ones QEMU processes use (so must match
      * the QEMU user/group */
     char *libDir;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e1b805d906..41421214df 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1992,6 +1992,7 @@ qemuDomainObjPrivateFree(void *data)
     virObjectUnref(priv->monConfig);
     g_free(priv->lockState);
     g_free(priv->origname);
+    g_free(priv->migTLSPSK);
 
     virChrdevFree(priv->devs);
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 50ab492023..1feeb11bcd 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -148,6 +148,7 @@ struct _qemuDomainObjPrivate {
     char *origname;
     int nbdPort; /* Port used for migration with NBD */
     unsigned short migrationPort;
+    char *migTLSPSK; /* Hex-encoded pre-shared key for TLS-PSK-enabled VM migration session */
     unsigned short backupNBDPort;
     int preMigrationState;
     unsigned long long preMigrationMemlock; /* Original RLIMIT_MEMLOCK in case
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bdc0cff66a..84b017b342 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -664,6 +664,12 @@ qemuStateInitialize(bool privileged,
                              cfg->vncStateDir);
         goto error;
     }
+    if (virDirCreate(cfg->tlsPSKStateDir, 0700, cfg->user, cfg->group,
+                     VIR_DIR_CREATE_ALLOW_EXIST) < 0) {
+        virReportSystemError(errno, _("Failed to create TLS PSK state dir %1$s"),
+                             cfg->tlsPSKStateDir);
+        goto error;
+    }
 
     qemu_driver->inhibitor = virInhibitorNew(
         VIR_INHIBITOR_WHAT_SHUTDOWN,
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
@@ -59,6 +59,7 @@
 #include "virprocess.h"
 #include "virdomainsnapshotobjlist.h"
 #include "virutil.h"
+#include "virsecureerase.h"
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -1503,6 +1504,91 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
 }
 
 
+static void
+qemuMigrationDeletePSKDir(virQEMUDriver *driver, virDomainObj *vm)
+{
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+    qemuDomainObjPrivate *priv = vm->privateData;
+    g_autofree char *dir_path = NULL;
+    g_autofree char *shortName = NULL;
+
+    if (priv->migTLSPSK) {
+        virSecureEraseString(priv->migTLSPSK);
+        g_clear_pointer(&priv->migTLSPSK, g_free);
+    }
+
+    if (!vm || !vm->def || !(shortName = virDomainDefGetShortName(vm->def)))
+        return;
+
+    dir_path = g_strdup_printf("%s/%s", cfg->tlsPSKStateDir, shortName);
+
+    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
+qemuPersistTLSPSKHelper(int pskFD,
+                        const char *pskPath,
+                        const void *opaque)
+{
+    const char *key = opaque;
+
+    if (safewrite(pskFD, "qemu:", 5) < 0 ||
+        safewrite(pskFD, key, strlen(key)) < 0) {
+        virReportSystemError(errno,
+                             _("Unable to write the pre-shared key to file '%1$s'"),
+                             pskPath);
+        return -1;
+    }
+
+    return 0;
+}
+
+
+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;
+
+    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"));
+        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);
+        if (ret < 0) {
+            virSecureEraseString(mig->tlsPSK);
+            g_clear_pointer(&mig->tlsPSK, g_free);
+            return NULL;
+        }
+    }
+
     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);
+
     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);
+
     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);
+
     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);
+
     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
@@ -20,13 +20,16 @@
 
 #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"
 #include "virutil.h"
+#include "virsecureerase.h"
 
 #include "qemu_domain.h"
 #include "qemu_migration_cookie.h"
@@ -52,6 +55,7 @@ VIR_ENUM_IMPL(qemuMigrationCookieFlag,
               "allowReboot",
               "capabilities",
               "block-dirty-bitmaps",
+              "psk",
 );
 
 
@@ -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);
     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);
-
     return 0;
 }
 
@@ -1494,6 +1569,7 @@ qemuMigrationCookieParse(virQEMUDriver *driver,
                          unsigned int flags)
 {
     g_autoptr(qemuMigrationCookie) mig = NULL;
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 
     /* Parse & validate incoming cookie (if any) */
     if (cookiein && cookieinlen &&
@@ -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 {
+            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;
 
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..adf2eb4d08 100644
--- a/tests/qemumigrationcookiexmltest.c
+++ b/tests/qemumigrationcookiexmltest.c
@@ -160,8 +160,12 @@ testQemuMigrationCookieParse(const void *opaque)
         return -1;
     }
 
-    /* set all flags so that formatter attempts to format everything */
-    data->cookie->flags = ~0;
+    /* Set all flags except QEMU_MIGRATION_COOKIE_TLS_PSK so that formatter
+     * attempts to format everything except the migration-key element. This is
+     * because the value of the migration-key element is randomly generated every time
+     * the migration cookie is constructed.
+     */
+    data->cookie->flags = ~QEMU_MIGRATION_COOKIE_TLS_PSK;
 
     if (qemuMigrationCookieXMLFormat(&driver,
                                      priv->qemuCaps,
@@ -225,15 +229,17 @@ testQemuMigrationCookieDom2XML(const char *namesuffix,
          * - lockstate: internals are NULL in tests, causes crash
          * - nbd: monitor not present
          * - dirty bitmaps: monitor not present
+         * - tls-psk: a new key is generated every time the migration cookie is constructed, so we can't test it
          */
         unsigned int cookiePopulateFlagMask = QEMU_MIGRATION_COOKIE_LOCKSTATE |
                                               QEMU_MIGRATION_COOKIE_NBD |
-                                              QEMU_MIGRATION_COOKIE_BLOCK_DIRTY_BITMAPS;
+                                              QEMU_MIGRATION_COOKIE_BLOCK_DIRTY_BITMAPS |
+                                              QEMU_MIGRATION_COOKIE_TLS_PSK;
         data->cookiePopulateFlags = ~cookiePopulateFlagMask;
     }
 
     if (cookieParseFlags == 0)
-        data->cookieParseFlags = ~0;
+        data->cookieParseFlags = ~QEMU_MIGRATION_COOKIE_TLS_PSK;
 
     data->inStatus = g_strconcat(abs_srcdir, "/", domxml, NULL);
 
@@ -279,7 +285,7 @@ testQemuMigrationCookieXML2XML(const char *name,
     int ret = 0;
 
     if (cookieParseFlags == 0)
-        data->cookieParseFlags = ~0;
+        data->cookieParseFlags = ~QEMU_MIGRATION_COOKIE_TLS_PSK;
 
     data->inStatus = g_strconcat(abs_srcdir, "/", statusxml, NULL);
     data->infile = g_strconcat(abs_srcdir, "/qemumigrationcookiexmldata/",
@@ -381,7 +387,7 @@ testQemuMigrationCookieXML2XMLBitmaps(const char *name,
     int ret = 0;
 
     if (cookieParseFlags == 0)
-        data->cookieParseFlags = ~0;
+        data->cookieParseFlags = ~QEMU_MIGRATION_COOKIE_TLS_PSK;
 
     data->inStatus = g_strconcat(abs_srcdir, "/", statusxml, NULL);
     data->infile = g_strconcat(abs_srcdir, "/qemumigrationcookiexmldata/",
-- 
2.43.7
Re: [PATCH v3 2/7] qemu: Manage a pre-shared key's lifecycle
Posted by Peter Krempa via Devel 3 weeks, 5 days ago
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;
>
Re: [PATCH v3 2/7] qemu: Manage a pre-shared key's lifecycle
Posted by Abhisek Panda 2 weeks, 1 day ago

> On 20 Aug 2026, at 6:25 PM, Peter Krempa <pkrempa@redhat.com> 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 <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?
> 

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;
>> 
> 

Re: [PATCH v3 2/7] qemu: Manage a pre-shared key's lifecycle
Posted by Peter Krempa via Devel 2 weeks ago
On Mon, Aug 31, 2026 at 19:26:31 +0000, Abhisek Panda wrote:
> > On 20 Aug 2026, at 6:25 PM, Peter Krempa <pkrempa@redhat.com> wrote:
> > 
> >> +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.

[...]


> The caller guarantees tlsPSK is present when invoking this, so I have removed the tlsPSK
> check entirely.

[...]

> > 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.

[...]

> > 
> > 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.

[..]

> > 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 (flags & VIR_MIGRATE_TLS)
> >> +        qemuMigrationDeletePSKDir(driver, vm);
> > 
> > ditto
> 
> Acknowledged 

[...]

> > 
> > same here
> 
> Acknowledged 

[...]

> > 
> > And here you then don't need the comment.
> 
> Acknowledged 

[...]


Please avoid the acknowledgement of requested changes as seen above
unless you have a followup needing discussion. I almost stopped reading
this message after the fifth acknowledgement, which would mean I'd miss
your question below.


> >> @@ -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.

yup

But you need to keep in mind that either the source or destination can
be an older libvirt version which doesn't support PSK.

> 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 I'd normally prefer to use x509 if it was already set up. The problem
is that it's hard to know if it is set up properly before trying.

Since the setup of PSK is much simpler and much more likely to work the
simplest option is to use PSK as default and use x509 only if one of the
sides doesn't support PSK.

Security-wise this will not be a downgrade because the libvirt
connection needs to be secured too.

As mentioned earlier I want to also enable TLS+PSK always (when
supported) even when the _TLS flag wasn't supplied so that we provide
security by default (that's why I've asked for removing some of the
checks and depending on the actual setup).


> 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.

The issue is that both hosts can have x509 set up incompatibly which
wouldn't work, but with PSK it will.


> 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?

So I think an advanced version of 2 is the correct approach. We should:

1) If both sides support PSK, use PSK. Even when VIR_MIGRATE_TLS is not
specified.
2) If one of the sides doesn't support PSK and VIR_MIGRATE_TLS is
specified, try x509
3) Otherwise - though luck

The use of VIR_MIGRATE_TLS will then gate that the migration will fail
if neither PSK nor x509 is supported.

Re: [PATCH v3 2/7] qemu: Manage a pre-shared key's lifecycle
Posted by Abhisek Panda 5 days, 22 hours ago

> On 1 Sep 2026, at 12:48 PM, Peter Krempa <pkrempa@redhat.com> wrote:
> 
> !-------------------------------------------------------------------|
>  CAUTION: External Email
> 
> |-------------------------------------------------------------------!
> 
> On Mon, Aug 31, 2026 at 19:26:31 +0000, Abhisek Panda wrote:
>>> On 20 Aug 2026, at 6:25 PM, Peter Krempa <pkrempa@redhat.com> wrote:
>>> 
>>>> +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.
> 
> [...]
> 
> 
>> The caller guarantees tlsPSK is present when invoking this, so I have removed the tlsPSK
>> check entirely.
> 
> [...]
> 
>>> 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.
> 
> [...]
> 
>>> 
>>> 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.
> 
> [..]
> 
>>> 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 (flags & VIR_MIGRATE_TLS)
>>>> +        qemuMigrationDeletePSKDir(driver, vm);
>>> 
>>> ditto
>> 
>> Acknowledged
> 
> [...]
> 
>>> 
>>> same here
>> 
>> Acknowledged
> 
> [...]
> 
>>> 
>>> And here you then don't need the comment.
>> 
>> Acknowledged
> 
> [...]
> 
> 
> Please avoid the acknowledgement of requested changes as seen above
> unless you have a followup needing discussion. I almost stopped reading
> this message after the fifth acknowledgement, which would mean I'd miss
> your question below.
> 
> 
>>>> @@ -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.
> 
> yup
> 
> But you need to keep in mind that either the source or destination can
> be an older libvirt version which doesn't support PSK.
> 
>> 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 I'd normally prefer to use x509 if it was already set up. The problem
> is that it's hard to know if it is set up properly before trying.
> 
> Since the setup of PSK is much simpler and much more likely to work the
> simplest option is to use PSK as default and use x509 only if one of the
> sides doesn't support PSK.
> 
> Security-wise this will not be a downgrade because the libvirt
> connection needs to be secured too.
> 
> As mentioned earlier I want to also enable TLS+PSK always (when
> supported) even when the _TLS flag wasn't supplied so that we provide
> security by default (that's why I've asked for removing some of the
> checks and depending on the actual setup).
> 
> 
>> 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.
> 
> The issue is that both hosts can have x509 set up incompatibly which
> wouldn't work, but with PSK it will.
> 
> 
>> 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?
> 
> So I think an advanced version of 2 is the correct approach. We should:
> 
> 1) If both sides support PSK, use PSK. Even when VIR_MIGRATE_TLS is not
> specified.
> 2) If one of the sides doesn't support PSK and VIR_MIGRATE_TLS is
> specified, try x509
> 3) Otherwise - though luck
> 
> The use of VIR_MIGRATE_TLS will then gate that the migration will fail
> if neither PSK nor x509 is supported.
> 

I have pushed v4 of this patch series for review with the advanced version of 2nd design.

Given that with encrypted migration, there will be a performance impact to the live migrations
of the VMs, due to auxiliary encrypt and decrypt operations at the source and destination, and
also due to a lack of MSG_ZEROCOPY support for encrypted migrations in QEMU. Some users
might be running VM migration within a cluster of trusted nodes, for which encrypted migration
might not be a performant solution. Can we gate this behaviour of by-default enablement of
PSK behind a configuration parameter in qemu.conf. This design is as follows:
1. If the configuration parameter is set we do advanced version of 2nd design.
2. If the configuration parameter is not set, then we attempt PSK if VIR_MIGRATE_TLS flag is
    specified and both source and destination supports it. If either of the nodes doesn’t support it
    we fallback to x509.

Would you please let me know your opinion on this design?