> On 1 Sep 2026, at 12:48 PM, Peter Krempa <[email protected]> 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 <[email protected]> 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?
