[libvirt] [PATCH v2 00/14] Add TLS support for migration

[John Ferlan]

On 02/28/2017 08:48 AM, Jiri Denemark wrote:
> On Thu, Feb 23, 2017 at 13:42:02 -0500, John Ferlan wrote:
>> AFAIU - removal of the objects would remove the migration tls-creds,
>> tls-hostname settings.
> 
> This would be a very strange behavior.
> 
>> I left the cfg->migrateTLS in for now - it's very simple to remove, but
>> there would still need to be a key on something to ensure the migrateTLS
>> environment has been properly configured since that would mean the default
>> environment would need to be used/configured. Setting up the default
>> environment is keyed off having the migrateTLS defined. That's all part
>> of the qemu_conf reading logic.
> 
> I still don't see the value in keeping migrate_tls option in qemu.conf.
> If we want to check the environment is setup correctly we should check
> the certificates are in place rather than relying on the option. The
> error reported when migrate_tls is set, but the environment is not
> prepared is:
> 
>     internal error: unable to execute QEMU command 'object-add': Unable
>     to access credentials /etc/pki/qemu/ca-cert.pem: No such file or
>     directory
> 
> Not ideal but at least it names the file which is missing.
> 

In my latest branch I've removed migrate_tls.

>> John Ferlan (14):
>>   qemu: Introduce qemuDomainSecretInfoNew
>>   qemu: Introduce qemuDomainSecretMigratePrepare
>>   qemu: Move exit monitor calls in failure paths
>>   qemu: Refactor hotplug to introduce qemuDomain{Add|Del}TLSObjects
>>   qemu: Refactor qemuDomainGetChardevTLSObjects to converge code
>>   qemu: Move qemuDomainSecretChardevPrepare call
>>   qemu: Move qemuDomainPrepareChardevSourceTLS call
>>   qemu: Introduce qemuDomainGetTLSObjects
> 
> The eight patches above are mostly OK except for some small issues.

OK - I will separate them out with the latest adjustments from review
and repost.

> 
> But the rest of the series needs more work...
> 
>>   qemu: Add TLS params to _qemuMonitorMigrationParams
>>   Add new migration flag VIR_MIGRATE_TLS
>>   qemu: Create #define for TLS configuration setup.
>>   conf: Introduce migrate_tls_x509_cert_dir
>>   qemu: Set up the migrate TLS objects for target
>>   qemu: Set up the migration TLS objects for source
> 
> The code should check whether QEMU actually supports TLS migration,
> otherwise you will get
> 
>     internal error: unable to execute QEMU command
>     'migrate-set-parameters': Invalid parameter 'tls-creds'
> 

Hmm...  I see tls-creds added to migrate-set-parameters in 2.7 while
they were added to ChardevSocket in 2.6... Ugh.  Have to refresh my
recollection of how to get the answer I need from capabilities.

> As I mentioned in my v1 review, we should always set the parameters if
> QEMU supports them to make sure they don't contain any leftovers from a
> previous migration.

I see from a quick scan of the qemu code though that it appears as if
the code checks for a non null value being passed:

params->has_tls_creds = !!s->parameters.tls_creds;
params->has_tls_hostname = !!s->parameters.tls_hostname;

So in order to "allow" clearing the tls_creds and tls_hostname, what
would one do?  The clearing would be necessary since a target of a
migration will become a source for a migration and the tls-creds object
would be different (using endpoint={server|client}).

This would be the I used TLS on one migration, but not on the next type
operation for the same domain. Doesn't seem we can assume that TLS will
be used for every migration since the desire is to have usage controlled
via API option and not host config option

> I also said we should properly clean the TLS stuff
> somewhere inside qemuProcessRecoverMigration*. And if we do it in
> addition to properly cleaning up everything in the Finish and Confirm
> phases, we should not need to store migrateTLS in the status XML. We can
> just always do the cleanup if QEMU supports TLS migration.
> 
> 
> Anyway, TLS migration doesn't work even if it is properly configured:
> 
>     operation failed: migration job: unexpectedly failed
> 
> And the destination QEMU log contains:
> 
>     qemu-system-x86_64: No TLS credentials with id 'objmigrate_tls0'
> 
> The reason is an interesting flow of QMP commands issued by libvirt
> during the Prepare phase:
> 
>     {
>         "execute": "object-add",
>         "arguments": {
>             "qom-type": "tls-creds-x509",
>             "id": "objmigrate_tls0",
>             "props": {
>                 "dir": "/etc/pki/libvirt",
>                 "endpoint": "server",
>                 "verify-peer": false
>             }
>         },
>         "id": "libvirt-21"
>     }
> 
>     {
>         "execute": "migrate-set-parameters",
>         "arguments": {
>             "tls-creds": "objmigrate_tls0"
>         },
>         "id": "libvirt-26"
>     }
> 
>     {
>         "execute": "migrate-incoming",
>         "arguments": {
>             "uri": "tcp:[::]:49152"
>         },
>         "id": "libvirt-27"
>     }
> 
>     {
>         "execute": "object-del",
>         "arguments": {
>             "id": "objmigrate_tls0"
>         },
>         "id": "libvirt-28"
>     }
> 
> The error reported after you deleted the objmigrate_tls0 object doesn't
> seem to confirm your idea about migration parameters begin unset when
> the object is removed.

OK - well obviously there's still quite a bit for me to understand about
the migration model.

> 
> Please, test your series before you send a new version of it.
> 

Yep - something I noted in my cover letter - the need for a test
environment... Hopefully I'll find a kind soul that will allow me to
have access to an already configured environment to test with...

John