> On 2 Oct 2026, at 5:59 PM, Peter Krempa <[email protected]> wrote:
>
> !-------------------------------------------------------------------|
> CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> On Mon, Sep 07, 2026 at 08:55:20 +0000, Abhisek Panda wrote:
>> Libvirt by default attempt to use the TLS-PSK-enabled VM migration,
>> if the both source and destination support the tls-creds-psk object.
>> The source host adds the pre-shared key in the migration cookie if the
>> source supports the tls-creds-psk object. Upon parsing the migration
>> cookie, the destination host checks for the same capability 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_capabilities.c | 2 +
>> src/qemu/qemu_capabilities.h | 1 +
>> 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 | 98 +++++++++++++++++++
>> src/qemu/qemu_migration.h | 3 +
>> src/qemu/qemu_migration_cookie.c | 79 ++++++++++++++-
>> src/qemu/qemu_migration_cookie.h | 5 +
>> src/qemu/qemu_process.c | 3 +
>> .../caps_10.0.0_aarch64.xml | 1 +
>> .../caps_10.0.0_ppc64.xml | 1 +
>> .../caps_10.0.0_s390x.xml | 1 +
>> .../caps_10.0.0_x86_64+amdsev.xml | 1 +
>> .../caps_10.0.0_x86_64.xml | 1 +
>> .../caps_10.1.0_s390x.xml | 1 +
>> .../caps_10.1.0_x86_64+inteltdx.xml | 1 +
>> .../caps_10.1.0_x86_64.xml | 1 +
>> .../caps_10.2.0_aarch64.xml | 1 +
>> .../caps_10.2.0_x86_64+mshv.xml | 1 +
>> .../caps_10.2.0_x86_64.xml | 1 +
>> .../caps_11.0.0_aarch64.xml | 1 +
>> .../caps_11.0.0_s390x.xml | 1 +
>> .../caps_11.0.0_x86_64+sgx.xml | 1 +
>> .../caps_11.0.0_x86_64.xml | 1 +
>> .../caps_11.1.0_aarch64.xml | 1 +
>> .../caps_11.1.0_s390x.xml | 1 +
>> .../caps_11.1.0_x86_64.xml | 1 +
>> tests/qemucapabilitiesdata/caps_7.2.0_ppc.xml | 1 +
>> .../caps_7.2.0_x86_64+hvf.xml | 1 +
>> .../caps_7.2.0_x86_64.xml | 1 +
>> .../caps_8.0.0_x86_64.xml | 1 +
>> .../qemucapabilitiesdata/caps_8.1.0_s390x.xml | 1 +
>> .../caps_8.1.0_x86_64.xml | 1 +
>> .../caps_8.2.0_aarch64.xml | 1 +
>> .../caps_8.2.0_armv7l.xml | 1 +
>> .../caps_8.2.0_loongarch64.xml | 1 +
>> .../qemucapabilitiesdata/caps_8.2.0_s390x.xml | 1 +
>> .../caps_8.2.0_x86_64.xml | 1 +
>> .../qemucapabilitiesdata/caps_9.0.0_sparc.xml | 1 +
>> .../caps_9.0.0_x86_64.xml | 1 +
>> .../caps_9.1.0_riscv64.xml | 1 +
>> .../qemucapabilitiesdata/caps_9.1.0_s390x.xml | 1 +
>> .../caps_9.1.0_x86_64.xml | 1 +
>> .../caps_9.2.0_aarch64+hvf.xml | 1 +
>> .../qemucapabilitiesdata/caps_9.2.0_s390x.xml | 1 +
>> .../caps_9.2.0_x86_64+amdsev.xml | 1 +
>> .../caps_9.2.0_x86_64.xml | 1 +
>> tests/qemumigrationcookiexmltest.c | 12 ++-
>> 51 files changed, 249 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index bc2b0412dc..3db4710dee 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -776,6 +776,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
>> "arm-smmuv3.cmdqv", /* QEMU_CAPS_ARM_SMMUV3_CMDQV */
>> "iothread.poll-weight", /* QEMU_CAPS_IOTHREAD_POLL_WEIGHT */
>> "win-dmp.guest-aware", /* QEMU_CAPS_WIN_DMP_GUEST_AWARE */
>> + "tls-creds-psk", /* QEMU_CAPS_OBJECT_TLS_CREDS_PSK */
>> );
>>
>>
>> @@ -1490,6 +1491,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[]
>> = {
>> { "uefi-vars-sysbus", QEMU_CAPS_DEVICE_UEFI_VARS },
>> { "monitor-qmp", QEMU_CAPS_OBJECT_MONITOR_QMP },
>> { "arm-smmuv3", QEMU_CAPS_DEVICE_ARM_SMMUV3 },
>> + { "tls-creds-psk", QEMU_CAPS_OBJECT_TLS_CREDS_PSK },
>
> Any reason why the capability is introduced? I didn't see it mentioned
> in our previous conversation.
>
> In case if the support can be compiled out it's okay to add a
> capability, but new capabilities need to go into a separate commit
> because they make existing commits really massive and hard to backport.
>
> So if the capability is needed you'll have to split the hunks that add
> the capability and the detection into a separate commit which will need
> to preceed this one which makes use of it.
The rationale behind adding this capability was to check if the source and
destination’s
QEMU supports PSK before attempting to perform PSK-based migration. Since
Libvirt’s
minimum supported QEMU version is now >= 7.2, and tls-creds-psk objects have
been
supported since QEMU 3.0, we can safely assume this capability exists on both
the source
and destination and drop this capability stuff.
>
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index 28303fd8f0..1e432cdc41 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -750,6 +750,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
>> syntax-check */
>> QEMU_CAPS_ARM_SMMUV3_CMDQV, /* arm-smmuv3.cmdqv */
>> QEMU_CAPS_IOTHREAD_POLL_WEIGHT, /* -object iothread.poll-weight */
>> QEMU_CAPS_WIN_DMP_GUEST_AWARE, /* 'win-dmp' is offered only to a guest
>> that can use it */
>> + QEMU_CAPS_OBJECT_TLS_CREDS_PSK, /* -object tls-creds-psk */
>>
>> QEMU_CAPS_LAST /* this must always be the last item */
>> } virQEMUCapsFlags;
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index cca7776b80..f5c5437c89 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 b700829342..2db3440514 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 a4e5f92840..f4d470553d 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1990,6 +1990,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 23e99dc68c..053140f7ec 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
>> @@ -139,6 +139,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 8498568623..5548d6f7bd 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;
>
> So, recently we've had a few reports about "security" bugs regarding
> directories owned by "qemu:qemu" which are filled by virtqemud with some
> objects.
>
> That makes me think two things:
> 1) does this need to be in a separate directory? We really should have
> a per-VM directory for all the per-instance helper files which will
> live in /run so that it's removed on reboot
>
> 2) the directory shouldn't be owned by qemu:qemu, but rather root:root
> for the system instance and have 0711 mode so that qemu can open the
> files but not create them.
>
> I know this is a more general thing, as our directory in /run/ is
> nonsensically split to e.g. /run/libvirt/qemu/channel/$VMID rather than
> have /run/libvirt/qemu/(possibly some subdir such as 'instance'/$ID'
> which will include all the files.
>
> For this series, what definitely applies is the ownership and
> permissions point.
>
>
>
>> + }
>>
>> qemu_driver->inhibitor = virInhibitorNew(
>> VIR_INHIBITOR_WHAT_SHUTDOWN,
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 4a43ab83b0..b9971286fd 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,89 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef
>> *def)
>> }
>>
>>
>> +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 (!(shortName = virDomainDefGetShortName(vm->def)))
>
> This failure ...
>
>> + 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);
>
> ... has the same implication.
>
>
>> +}
>> +
>> +
>> +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 (!(shortName = virDomainDefGetShortName(vm->def))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Could not get the shortened domain name for
>> domain: %1$s"),
>> + vm->def->name);
>> + 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) {
>
> Same here, the directory IMO doesn't need to be owned by qemu because
> qemu will never attempt to create a file here. It needs to be accessible
> by 'other' though.
>
>
>> + virReportSystemError(errno,
>> + _("Could not create the directory %1$s for
>> storing PSKs"),
>> + dir_path);
>> + goto error;
>> + }
>> +
>> + if (virFileRewrite(key_path, S_IRUSR, cfg->user,
>> + cfg->group, qemuPersistTLSPSKHelper,
>> + tlsPSK) < 0)
>> + goto error;
>> +
>> + return 0;
>> +
>> + error:
>> + qemuMigrationDeletePSKDir(driver, vm);
>> + return -1;
>> +}
>> +
>> +
>> static int
>> qemuDomainGetMigrationBlockers(virDomainObj *vm,
>> int asyncJob,
>> @@ -2722,6 +2806,9 @@ qemuMigrationSrcBeginXML(virDomainObj *vm,
>> if (priv->origCPU)
>> cookieFlags |= QEMU_MIGRATION_COOKIE_CPU;
>>
>> + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_PSK))
>> + cookieFlags |= QEMU_MIGRATION_COOKIE_TLS_PSK;
>> +
>> if (!(flags & VIR_MIGRATE_OFFLINE))
>> cookieFlags |= QEMU_MIGRATION_COOKIE_CAPS;
>>
>> @@ -2738,6 +2825,9 @@ qemuMigrationSrcBeginXML(virDomainObj *vm,
>> cookieFlags) < 0)
>> return NULL;
>>
>> + if (mig->tlsPSK && qemuMigrationPersistPSK(driver, vm, mig->tlsPSK) < 0)
>> + return NULL;
>> +
>> if (xmlin) {
>> g_autoptr(virDomainDef) def = NULL;
>>
>> @@ -4232,6 +4322,8 @@ qemuMigrationSrcConfirmPhase(virQEMUDriver *driver,
>> privJob->stats.mig.downtime = privMigJob->stats.mig.downtime;
>> }
>>
>> + qemuMigrationDeletePSKDir(driver, vm);
>> +
>> if (flags & VIR_MIGRATE_OFFLINE)
>> return 0;
>>
>> @@ -5274,6 +5366,7 @@ qemuMigrationSrcRun(virQEMUDriver *driver,
>>
>> error:
>> virErrorPreserveLast(&orig_err);
>> + qemuMigrationDeletePSKDir(driver, vm);
>>
>> if (qemuDomainObjIsActive(vm)) {
>> int reason;
>> @@ -7029,6 +7122,8 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver,
>> QEMU_MIGRATION_COOKIE_STATS) < 0)
>> VIR_WARN("Unable to encode migration cookie");
>>
>> + qemuMigrationDeletePSKDir(driver, vm);
>> +
>> qemuMigrationDstComplete(driver, vm, inPostCopy,
>> VIR_ASYNC_JOB_MIGRATION_IN, vm->job);
>>
>> @@ -7039,6 +7134,8 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver,
>> * overwrites it. */
>> virErrorPreserveLast(&orig_err);
>>
>> + qemuMigrationDeletePSKDir(driver, vm);
>> +
>> if (qemuDomainObjIsActive(vm)) {
>> if (doKill) {
>> qemuProcessStop(vm, VIR_DOMAIN_SHUTOFF_FAILED,
>> @@ -7197,6 +7294,7 @@ qemuMigrationProcessUnattended(virQEMUDriver *driver,
>> else
>> qemuMigrationSrcComplete(driver, vm, job);
>>
>> + qemuMigrationDeletePSKDir(driver, vm);
>> qemuMigrationJobFinish(vm);
>>
>> if (!virDomainObjIsActive(vm))
>> diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
>> index 7e9410e1f7..ce15f8024b 100644
>> --- a/src/qemu/qemu_migration.h
>> +++ b/src/qemu/qemu_migration.h
>> @@ -279,3 +279,6 @@ int
>> qemuMigrationAnyRefreshStatus(virDomainObj *vm,
>> virDomainAsyncJob asyncJob,
>> virDomainJobStatus *status);
>> +
>> +void
>> +qemuMigrationDeletePSKDir(virQEMUDriver *driver, virDomainObj *vm);
>> diff --git a/src/qemu/qemu_migration_cookie.c
>> b/src/qemu/qemu_migration_cookie.c
>> index 7311a8294b..0cf1ab9590 100644
>> --- a/src/qemu/qemu_migration_cookie.c
>> +++ b/src/qemu/qemu_migration_cookie.c
>> @@ -20,6 +20,7 @@
>>
>> #include <gnutls/gnutls.h>
>> #include <gnutls/x509.h>
>> +#include <inttypes.h>
>>
>> #include "locking/domain_lock.h"
>> #include "virerror.h"
>> @@ -27,6 +28,7 @@
>> #include "virnetdevopenvswitch.h"
>> #include "virstring.h"
>> #include "virutil.h"
>> +#include "virsecureerase.h"
>>
>> #include "qemu_domain.h"
>> #include "qemu_migration_cookie.h"
>> @@ -52,6 +54,7 @@ VIR_ENUM_IMPL(qemuMigrationCookieFlag,
>> "allowReboot",
>> "capabilities",
>> "block-dirty-bitmaps",
>> + "psk",
>> );
>>
>>
>> @@ -165,6 +168,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 +581,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 +941,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 +1450,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 +1531,29 @@ qemuMigrationCookieFormat(qemuMigrationCookie *mig,
>> qemuMigrationCookieAddCaps(mig, dom, party) < 0)
>> return -1;
>>
>> + if (flags & QEMU_MIGRATION_COOKIE_TLS_PSK) {
>> + switch (party) {
>> + case QEMU_MIGRATION_SOURCE:
>> + if (qemuMigrationCookieAddTLSPSK(mig, driver, dom) < 0)
>> + return -1;
>> + break;
>> + case QEMU_MIGRATION_DESTINATION:
>> + if (!virQEMUCapsGet(priv->qemuCaps,
>> QEMU_CAPS_OBJECT_TLS_CREDS_PSK)) {
>
> IMO any sane build of qemu must have TLS enabled nowadays. I wonder if
> it even makes sense to have this capability. If tls can be compiled out,
> techincally it'd mean that there could be such a setup but IMO that
> would be still insane to have a unencrypted connection.
>
> I'd be okay just assuming that any qemu we support has TLS support. On
> the offchance that someone uses an insane build they will get an error,
> but I don't think I'd care for such setups.
IIUC, Libvirt will automatically enforce TLS-PSK encryption for migrations
between
two newer Libvirt hosts, even if the user does not specify the VIR_MIGRATE_TLS
flag.
Consequently, TLS-X509 would only be triggered during a forward migration from
an older
Libvirt host to a newer Libvirt destination (when VIR_MIGRATE_TLS is set). In
that scenario,
the older source wouldn't include the pre-shared key in the migration cookie,
naturally
bypassing the PSK pathway.
I am assuming we do not need to account for newer-to-older migrations.
I wanted to ask a follow up question regarding this 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 it as per our above discussed
design.
2. If the configuration parameter is not set, then we attempt PSK if
VIR_MIGRATE_TLS flag
is specified.
>
>
>> + if (mig->tlsPSK)
>> + virSecureEraseString(mig->tlsPSK);
>> + mig->flags &= ~QEMU_MIGRATION_COOKIE_TLS_PSK;
>> + g_clear_pointer(&mig->tlsPSK, g_free);
>> + }
>> + break;
>> + }
>> + }
>> +
>> 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;
>> }
>>
>> 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;
>> };
