[libvirt] [PATCH 00/13] Add TLS support for migration
Patches 1, 3 -> 9 are primarily quite a bit of code motion in order to allow reuse of the "core" of the chardev TLS code. Theoretically speaking of course, these patches should work - I don't have a TLS and migration environment to test with, so between following the qemu command model on Daniel's blog and prior experience with the chardev TLS would I added the saving of a flag to the private qemu domain state, although I'm not 100% sure it was necessary. At one time I created the source TLS objects during the Begin phase, but later decided to wait until just before the migration is run. I think the main reason to have the flag would be a restart of libvirtd to let 'something' know migration using TLS was configured. I think it may only be "necessary" in order to repopulate the migSecinfo after libvirtd restart, but it's not entirely clear. By the time I started thinking more about while writing this cover letter it was too late to just remove. Also rather than create the destination host TLS objects on the fly, I modified the command line generation. That model could change to adding the TLS objects once the destination is started and before the params are set for the migration. This 'model' is also going to be used for the NBD, but I figured I'd get this posted now since it was already too long of a series. John Ferlan (13): qemu: Create #define for TLS configuration setup. conf: Introduce migrate_tls_x509_cert_dir qemu: Rename qemuAliasTLSObjFromChardevAlias qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy} 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 qemu: Add TLS params to _qemuMonitorMigrationParams Add new migration flag VIR_MIGRATE_TLS qemu: Set up the migrate TLS objects for target qemu: Set up the migration TLS objects for source include/libvirt/libvirt-domain.h | 8 + src/qemu/libvirtd_qemu.aug | 6 + src/qemu/qemu.conf | 39 +++++ src/qemu/qemu_alias.c | 8 +- src/qemu/qemu_alias.h | 2 +- src/qemu/qemu_command.c | 33 +++- src/qemu/qemu_command.h | 4 +- src/qemu/qemu_conf.c | 43 +++-- src/qemu/qemu_conf.h | 5 + src/qemu/qemu_domain.c | 78 ++++++++++ src/qemu/qemu_domain.h | 89 ++++++----- src/qemu/qemu_hotplug.c | 312 ++++++++++++++++++++----------------- src/qemu/qemu_hotplug.h | 24 +++ src/qemu/qemu_migration.c | 135 ++++++++++++++++ src/qemu/qemu_migration.h | 1 + src/qemu/qemu_monitor.c | 12 +- src/qemu/qemu_monitor.h | 7 + src/qemu/qemu_monitor_json.c | 13 +- src/qemu/qemu_process.c | 4 + src/qemu/test_libvirtd_qemu.aug.in | 4 + tools/virsh-domain.c | 7 + 21 files changed, 625 insertions(+), 209 deletions(-) -- 2.9.3
Create GET_CONFIG_TLS_CERT to set up the TLS for 'chardev' TLS setting. Soon to be reused. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b5b0645..09066e4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -529,22 +529,31 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) goto cleanup; - if (virConfGetValueBool(conf, "chardev_tls", &cfg->chardevTLS) < 0) - goto cleanup; - if (virConfGetValueString(conf, "chardev_tls_x509_cert_dir", &cfg->chardevTLSx509certdir) < 0) - goto cleanup; - if ((rv = virConfGetValueBool(conf, "chardev_tls_x509_verify", &cfg->chardevTLSx509verify)) < 0) - goto cleanup; - if (rv == 0) - cfg->chardevTLSx509verify = cfg->defaultTLSx509verify; - if (virConfGetValueString(conf, "chardev_tls_x509_secret_uuid", - &cfg->chardevTLSx509secretUUID) < 0) - goto cleanup; - if (!cfg->chardevTLSx509secretUUID && cfg->defaultTLSx509secretUUID) { - if (VIR_STRDUP(cfg->chardevTLSx509secretUUID, - cfg->defaultTLSx509secretUUID) < 0) - goto cleanup; - } +#define GET_CONFIG_TLS_CERT(val) \ + do { \ + if (virConfGetValueBool(conf, #val "_tls", &cfg->val## TLS) < 0) \ + goto cleanup; \ + if ((rv = virConfGetValueBool(conf, #val "_tls_x509_verify", \ + &cfg->val## TLSx509verify)) < 0) \ + goto cleanup; \ + if (rv == 0) \ + cfg->val## TLSx509verify = cfg->defaultTLSx509verify; \ + if (virConfGetValueString(conf, #val "_tls_x509_cert_dir", \ + &cfg->val## TLSx509certdir) < 0) \ + goto cleanup; \ + if (virConfGetValueString(conf, \ + #val "_tls_x509_secret_uuid", \ + &cfg->val## TLSx509secretUUID) < 0) \ + goto cleanup; \ + if (!cfg->val## TLSx509secretUUID && \ + cfg->defaultTLSx509secretUUID) { \ + if (VIR_STRDUP(cfg->val## TLSx509secretUUID, \ + cfg->defaultTLSx509secretUUID) < 0) \ + goto cleanup; \ + } \ + } while (false); + + GET_CONFIG_TLS_CERT(chardev); if (virConfGetValueUInt(conf, "remote_websocket_port_min", &cfg->webSocketPortMin) < 0) goto cleanup; -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:18 -0500, John Ferlan wrote:
Create GET_CONFIG_TLS_CERT to set up the TLS for 'chardev' TLS setting. Soon to be reused.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-)
Is it possible to do this as a function and not a macro?
On Mon, Feb 20, 2017 at 08:54:26 +0100, Peter Krempa wrote:
On Fri, Feb 17, 2017 at 14:39:18 -0500, John Ferlan wrote:
Create GET_CONFIG_TLS_CERT to set up the TLS for 'chardev' TLS setting. Soon to be reused.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-)
Is it possible to do this as a function and not a macro?
So, this is going to be reused in the same function. That is okay with me as long as you #undef the macro right away so that it's clear that it's not used anywhere else.
On 02/20/2017 04:17 AM, Peter Krempa wrote:
On Mon, Feb 20, 2017 at 08:54:26 +0100, Peter Krempa wrote:
On Fri, Feb 17, 2017 at 14:39:18 -0500, John Ferlan wrote:
Create GET_CONFIG_TLS_CERT to set up the TLS for 'chardev' TLS setting. Soon to be reused.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-)
Is it possible to do this as a function and not a macro?
So, this is going to be reused in the same function. That is okay with me as long as you #undef the macro right away so that it's clear that it's not used anywhere else.
Right - I forgot the #undef. Conceptually it would be used for "chardev", "migrate", and "nbd" (although I'm wondering if anyone would ever go through the process to set up 3 different TLS envs). John
On Fri, Feb 17, 2017 at 14:39:18 -0500, John Ferlan wrote:
Create GET_CONFIG_TLS_CERT to set up the TLS for 'chardev' TLS setting. Soon to be reused.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b5b0645..09066e4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -529,22 +529,31 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) goto cleanup;
- if (virConfGetValueBool(conf, "chardev_tls", &cfg->chardevTLS) < 0) - goto cleanup; - if (virConfGetValueString(conf, "chardev_tls_x509_cert_dir", &cfg->chardevTLSx509certdir) < 0) - goto cleanup; - if ((rv = virConfGetValueBool(conf, "chardev_tls_x509_verify", &cfg->chardevTLSx509verify)) < 0) - goto cleanup; - if (rv == 0) - cfg->chardevTLSx509verify = cfg->defaultTLSx509verify; - if (virConfGetValueString(conf, "chardev_tls_x509_secret_uuid", - &cfg->chardevTLSx509secretUUID) < 0) - goto cleanup; - if (!cfg->chardevTLSx509secretUUID && cfg->defaultTLSx509secretUUID) { - if (VIR_STRDUP(cfg->chardevTLSx509secretUUID, - cfg->defaultTLSx509secretUUID) < 0) - goto cleanup; - } +#define GET_CONFIG_TLS_CERT(val) \ + do { \ + if (virConfGetValueBool(conf, #val "_tls", &cfg->val## TLS) < 0) \ + goto cleanup; \ + if ((rv = virConfGetValueBool(conf, #val "_tls_x509_verify", \ + &cfg->val## TLSx509verify)) < 0) \ + goto cleanup; \ + if (rv == 0) \ + cfg->val## TLSx509verify = cfg->defaultTLSx509verify; \ + if (virConfGetValueString(conf, #val "_tls_x509_cert_dir", \ + &cfg->val## TLSx509certdir) < 0) \ + goto cleanup; \ + if (virConfGetValueString(conf, \ + #val "_tls_x509_secret_uuid", \ + &cfg->val## TLSx509secretUUID) < 0) \ + goto cleanup; \ + if (!cfg->val## TLSx509secretUUID && \ + cfg->defaultTLSx509secretUUID) { \ + if (VIR_STRDUP(cfg->val## TLSx509secretUUID, \ + cfg->defaultTLSx509secretUUID) < 0) \ + goto cleanup; \ + } \ + } while (false); + + GET_CONFIG_TLS_CERT(chardev);
The following line is missing here: #undef GET_CONFIG_TLS_CERT Otherwise, the patch looks OK. Jirka
Add a new TLS X.509 certificate type - "migrate". This will handle the creation of a TLS certificate capability (and possibly repository) to be used for migrations. Similar to chardev's, credentials will be handled via a libvirt secrets. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 6 ++++++ src/qemu/qemu.conf | 39 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/test_libvirtd_qemu.aug.in | 4 ++++ 5 files changed, 56 insertions(+) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 82bae9e..18679c1 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -54,6 +54,11 @@ module Libvirtd_qemu = | bool_entry "chardev_tls_x509_verify" | str_entry "chardev_tls_x509_secret_uuid" + let migrate_entry = bool_entry "migrate_tls" + | str_entry "migrate_tls_x509_cert_dir" + | bool_entry "migrate_tls_x509_verify" + | str_entry "migrate_tls_x509_secret_uuid" + let nogfx_entry = bool_entry "nographics_allow_host_audio" let remote_display_entry = int_entry "remote_display_port_min" @@ -116,6 +121,7 @@ module Libvirtd_qemu = | vnc_entry | spice_entry | chardev_entry + | migrate_entry | nogfx_entry | remote_display_entry | security_entry diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 97d769d..83d91b6 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -238,6 +238,45 @@ #chardev_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000" +# Enable use of TLS encryption for migration +# +# It is necessary to setup CA and issue a server certificate +# before enabling this. +# +#migrate_tls = 1 + + +# In order to override the default TLS certificate location for migration +# certificates, supply a valid path to the certificate directory. If the +# provided path does not exist then the default_tls_x509_cert_dir path +# will be used. +# +#migrate_tls_x509_cert_dir = "/etc/pki/libvirt-migrate" + + +# The default TLS configuration only uses certificates for the server +# allowing the client to verify the server's identity and establish +# an encrypted channel. +# +# It is possible to use x509 certificates for authentication too, by +# issuing a x509 certificate to every client who needs to connect. +# +# Enabling this option will reject any client who does not have a +# certificate signed by the CA in /etc/pki/libvirt-migrate/ca-cert.pem +# +#migrate_tls_x509_verify = 1 + + +# Uncomment and use the following option to override the default secret +# UUID provided in the default_tls_x509_secret_uuid parameter. +# +# NB This default all-zeros UUID will not work. Replace it with the +# output from the UUID for the TLS secret from a 'virsh secret-list' +# command and then uncomment the entry +# +#migrate_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000" + + # By default, if no graphical front end is configured, libvirt will disable # QEMU audio output since directly talking to alsa/pulseaudio may not work # with various security settings. If you know what you're doing, enable diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 09066e4..a03fcf0 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -555,6 +555,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_CONFIG_TLS_CERT(chardev); + GET_CONFIG_TLS_CERT(migrate); + if (virConfGetValueUInt(conf, "remote_websocket_port_min", &cfg->webSocketPortMin) < 0) goto cleanup; if (cfg->webSocketPortMin < QEMU_WEBSOCKET_PORT_MIN) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index e585f81..ac7badb 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -137,6 +137,11 @@ struct _virQEMUDriverConfig { bool chardevTLSx509verify; char *chardevTLSx509secretUUID; + bool migrateTLS; + char *migrateTLSx509certdir; + bool migrateTLSx509verify; + char *migrateTLSx509secretUUID; + unsigned int remotePortMin; unsigned int remotePortMax; diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index bd25235..3d884e5 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -25,6 +25,10 @@ module Test_libvirtd_qemu = { "chardev_tls_x509_cert_dir" = "/etc/pki/libvirt-chardev" } { "chardev_tls_x509_verify" = "1" } { "chardev_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } +{ "migrate_tls" = "1" } +{ "migrate_tls_x509_cert_dir" = "/etc/pki/libvirt-migrate" } +{ "migrate_tls_x509_verify" = "1" } +{ "migrate_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } { "nographics_allow_host_audio" = "1" } { "remote_display_port_min" = "5900" } { "remote_display_port_max" = "65535" } -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:19 -0500, John Ferlan wrote:
Add a new TLS X.509 certificate type - "migrate". This will handle the creation of a TLS certificate capability (and possibly repository) to be used for migrations. Similar to chardev's, credentials will be handled via a libvirt secrets.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 6 ++++++ src/qemu/qemu.conf | 39 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/test_libvirtd_qemu.aug.in | 4 ++++ 5 files changed, 56 insertions(+)
I'm not a big fan of setting up two sets of X.509 environments, but I guess it could be useful to someone a we could always set both to the same values, right? Jirka
On 02/20/2017 10:13 AM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:19 -0500, John Ferlan wrote:
Add a new TLS X.509 certificate type - "migrate". This will handle the creation of a TLS certificate capability (and possibly repository) to be used for migrations. Similar to chardev's, credentials will be handled via a libvirt secrets.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 6 ++++++ src/qemu/qemu.conf | 39 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/test_libvirtd_qemu.aug.in | 4 ++++ 5 files changed, 56 insertions(+)
I'm not a big fan of setting up two sets of X.509 environments, but I guess it could be useful to someone a we could always set both to the same values, right?
Jirka
Cannot disagree... setting up one is daunting enough ;-)! With this there's going to be 4 and could be 5 if NBD needed it's own (the other 3 being VNC, Spice, and Chardev)... I do have a patch beyond this series "in process" which would do the same for NBD (but I keep thinking it'd be overkill). For those that want just one, they would just define "migrate_tls=1" and then use the "default" environment (default_tls_x509_cert_dir). Perhaps Daniel has more information about multi configured environments and what use they'd be. John
On Mon, Feb 20, 2017 at 10:26:16AM -0500, John Ferlan wrote:
On 02/20/2017 10:13 AM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:19 -0500, John Ferlan wrote:
Add a new TLS X.509 certificate type - "migrate". This will handle the creation of a TLS certificate capability (and possibly repository) to be used for migrations. Similar to chardev's, credentials will be handled via a libvirt secrets.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 6 ++++++ src/qemu/qemu.conf | 39 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/test_libvirtd_qemu.aug.in | 4 ++++ 5 files changed, 56 insertions(+)
I'm not a big fan of setting up two sets of X.509 environments, but I guess it could be useful to someone a we could always set both to the same values, right?
Jirka
Cannot disagree... setting up one is daunting enough ;-)!
With this there's going to be 4 and could be 5 if NBD needed it's own (the other 3 being VNC, Spice, and Chardev)... I do have a patch beyond this series "in process" which would do the same for NBD (but I keep thinking it'd be overkill).
For those that want just one, they would just define "migrate_tls=1" and then use the "default" environment (default_tls_x509_cert_dir).
Perhaps Daniel has more information about multi configured environments and what use they'd be.
I don't think you'd really set up different certs for every QEMU service. In practice I think you'll see either 1 set of certs, or 2 sets of certs. The former if every QEMU network service is only used inside the mgmt LAN, and the latter if you have some network services you need to expose directly to the end user (eg VNC). I think even the latter case is becoming less common though, as I see increasing preference for running a websockets proxy in between the end user & vnc/spice server, at least in the case of cloud - internal only data center virt may still expose the consoles services dirctly to the end user. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Mon, Feb 20, 2017 at 03:30:26PM +0000, Daniel P. Berrange wrote:
On Mon, Feb 20, 2017 at 10:26:16AM -0500, John Ferlan wrote:
On 02/20/2017 10:13 AM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:19 -0500, John Ferlan wrote:
Add a new TLS X.509 certificate type - "migrate". This will handle the creation of a TLS certificate capability (and possibly repository) to be used for migrations. Similar to chardev's, credentials will be handled via a libvirt secrets.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 6 ++++++ src/qemu/qemu.conf | 39 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/test_libvirtd_qemu.aug.in | 4 ++++ 5 files changed, 56 insertions(+)
I'm not a big fan of setting up two sets of X.509 environments, but I guess it could be useful to someone a we could always set both to the same values, right?
Jirka
Cannot disagree... setting up one is daunting enough ;-)!
With this there's going to be 4 and could be 5 if NBD needed it's own (the other 3 being VNC, Spice, and Chardev)... I do have a patch beyond this series "in process" which would do the same for NBD (but I keep thinking it'd be overkill).
BTW, we should *not* add certs for NBD - logically the NBD connections we're managing are just part of the migration data flow - they just happen to be separate TCP connections. IOW the 'migration' certs should always be used for the NBD channels too. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Fri, Feb 17, 2017 at 14:39:19 -0500, John Ferlan wrote:
Add a new TLS X.509 certificate type - "migrate". This will handle the creation of a TLS certificate capability (and possibly repository) to be used for migrations. Similar to chardev's, credentials will be handled via a libvirt secrets.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 6 ++++++ src/qemu/qemu.conf | 39 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/test_libvirtd_qemu.aug.in | 4 ++++ 5 files changed, 56 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 82bae9e..18679c1 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -54,6 +54,11 @@ module Libvirtd_qemu = | bool_entry "chardev_tls_x509_verify" | str_entry "chardev_tls_x509_secret_uuid"
+ let migrate_entry = bool_entry "migrate_tls" + | str_entry "migrate_tls_x509_cert_dir" + | bool_entry "migrate_tls_x509_verify" + | str_entry "migrate_tls_x509_secret_uuid" + let nogfx_entry = bool_entry "nographics_allow_host_audio"
let remote_display_entry = int_entry "remote_display_port_min" @@ -116,6 +121,7 @@ module Libvirtd_qemu = | vnc_entry | spice_entry | chardev_entry + | migrate_entry | nogfx_entry | remote_display_entry | security_entry diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 97d769d..83d91b6 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -238,6 +238,45 @@ #chardev_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000"
+# Enable use of TLS encryption for migration +# +# It is necessary to setup CA and issue a server certificate +# before enabling this. +# +#migrate_tls = 1
Actually what is this option supposed to do? It seems it doesn't do anything but saying "yes, I configured TLS for migration". The TLS usage for migration is turned on by VIR_MIGRATE_TLS flag which suggests the configuration option here is useless. Jirka
On 02/20/2017 11:03 AM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:19 -0500, John Ferlan wrote:
Add a new TLS X.509 certificate type - "migrate". This will handle the creation of a TLS certificate capability (and possibly repository) to be used for migrations. Similar to chardev's, credentials will be handled via a libvirt secrets.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 6 ++++++ src/qemu/qemu.conf | 39 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/test_libvirtd_qemu.aug.in | 4 ++++ 5 files changed, 56 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 82bae9e..18679c1 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -54,6 +54,11 @@ module Libvirtd_qemu = | bool_entry "chardev_tls_x509_verify" | str_entry "chardev_tls_x509_secret_uuid"
+ let migrate_entry = bool_entry "migrate_tls" + | str_entry "migrate_tls_x509_cert_dir" + | bool_entry "migrate_tls_x509_verify" + | str_entry "migrate_tls_x509_secret_uuid" + let nogfx_entry = bool_entry "nographics_allow_host_audio"
let remote_display_entry = int_entry "remote_display_port_min" @@ -116,6 +121,7 @@ module Libvirtd_qemu = | vnc_entry | spice_entry | chardev_entry + | migrate_entry | nogfx_entry | remote_display_entry | security_entry diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 97d769d..83d91b6 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -238,6 +238,45 @@ #chardev_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000"
+# Enable use of TLS encryption for migration +# +# It is necessary to setup CA and issue a server certificate +# before enabling this. +# +#migrate_tls = 1
Actually what is this option supposed to do? It seems it doesn't do anything but saying "yes, I configured TLS for migration". The TLS usage for migration is turned on by VIR_MIGRATE_TLS flag which suggests the configuration option here is useless.
Jirka
It more or less follows the same logic for chardev's which got an additional 'tls="yes"' to allow one to enable/disable an object in a domain on a case by case basis if chardev tls was enabled for the host. See: http://libvirt.org/formatdomain.html#elementsConsole and search down for 'tls' So my feeling while coding was - if I don't supply some sort of way to have an option to allow someone to choose to use the configured TLS environment for the migration, I'd end up being asked to add one since chardev has it. IIRC using 'haveTLS' for chardev's was debated during reviews... Between Pavel and I the code was added and 'massaged' to deal with release differences (that's all a blur in memory though). Regardless, based on Daniel's comment - if this does remain a conf parameter - perhaps the "default" description needs to be adjusted to indicate the usage model described, while the 'chardev' and 'migrate' (if added) specific descriptions would be altered to describe usage for exposure to some end user. I'm not opposed to dropping this and letting the "default" credentials to be used. Makes things easier. Perhaps in the future it could be resurrected if requested. John
On Mon, Feb 20, 2017 at 14:28:42 -0500, John Ferlan wrote:
On 02/20/2017 11:03 AM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:19 -0500, John Ferlan wrote:
+# Enable use of TLS encryption for migration +# +# It is necessary to setup CA and issue a server certificate +# before enabling this. +# +#migrate_tls = 1
Actually what is this option supposed to do? It seems it doesn't do anything but saying "yes, I configured TLS for migration". The TLS usage for migration is turned on by VIR_MIGRATE_TLS flag which suggests the configuration option here is useless.
It more or less follows the same logic for chardev's which got an additional 'tls="yes"' to allow one to enable/disable an object in a domain on a case by case basis if chardev tls was enabled for the host.
See: http://libvirt.org/formatdomain.html#elementsConsole
and search down for 'tls'
So my feeling while coding was - if I don't supply some sort of way to have an option to allow someone to choose to use the configured TLS environment for the migration, I'd end up being asked to add one since chardev has it.
The problem is chardevs and migration are quite different. While you can easily have a default configuration for chardevs and use tls="yes|no" to override it (no tls attribute will just tell libvirt to use the default), it's not really possible with migration API. API flags have not two states (in contrast to three-state boolean attributes in XML). Setting VIR_MIGRATE_TLS flag turns TLS on and using the flag turns TLS off. That is the default value is not used anywhere. If VIR_MIGRATE_TLS flag is used, the code checks migrate_tls is 1. Which translates to "yes, I configured TLS for migration" semantics of the configuration option. In other words, it makes sense to have the configuration option for devices defined in the XML, but using it for migration doesn't make any sense. This would of course mean its parsing would need to be moved out of the macro introduced in 1/13 and used for chardevs only. Jirka
On 02/21/2017 06:43 AM, Jiri Denemark wrote:
On Mon, Feb 20, 2017 at 14:28:42 -0500, John Ferlan wrote:
On 02/20/2017 11:03 AM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:19 -0500, John Ferlan wrote:
+# Enable use of TLS encryption for migration +# +# It is necessary to setup CA and issue a server certificate +# before enabling this. +# +#migrate_tls = 1
Actually what is this option supposed to do? It seems it doesn't do anything but saying "yes, I configured TLS for migration". The TLS usage for migration is turned on by VIR_MIGRATE_TLS flag which suggests the configuration option here is useless.
It more or less follows the same logic for chardev's which got an additional 'tls="yes"' to allow one to enable/disable an object in a domain on a case by case basis if chardev tls was enabled for the host.
See: http://libvirt.org/formatdomain.html#elementsConsole
and search down for 'tls'
So my feeling while coding was - if I don't supply some sort of way to have an option to allow someone to choose to use the configured TLS environment for the migration, I'd end up being asked to add one since chardev has it.
The problem is chardevs and migration are quite different. While you can easily have a default configuration for chardevs and use tls="yes|no" to override it (no tls attribute will just tell libvirt to use the default), it's not really possible with migration API. API flags have not two states (in contrast to three-state boolean attributes in XML). Setting VIR_MIGRATE_TLS flag turns TLS on and using the flag turns TLS off. That is the default value is not used anywhere. If VIR_MIGRATE_TLS flag is used, the code checks migrate_tls is 1. Which translates to "yes, I configured TLS for migration" semantics of the configuration option.
In other words, it makes sense to have the configuration option for devices defined in the XML, but using it for migration doesn't make any sense. This would of course mean its parsing would need to be moved out of the macro introduced in 1/13 and used for chardevs only.
Jirka
So I've circled back around to this... It seems like you're advocating the removal of the VIR_MIGRATE_TLS flag - which is fine. The only question I'd have for that is would there be a scenario/case where TLS for migration was defined on the host, but someone would prefer to not use it for migration. Or perhaps someone sets up the migrate_tls for some external LAN, but sometimes wants to migrate inside the LAN - having to reconfigure migrate_tls each time would seem to be more work than necessary so having a flag and config value would be desirable. IDC either way... Is there a preference? John
On Thu, Feb 23, 2017 at 08:10:18 -0500, John Ferlan wrote:
On 02/21/2017 06:43 AM, Jiri Denemark wrote:
On Mon, Feb 20, 2017 at 14:28:42 -0500, John Ferlan wrote:
On 02/20/2017 11:03 AM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:19 -0500, John Ferlan wrote:
+# Enable use of TLS encryption for migration +# +# It is necessary to setup CA and issue a server certificate +# before enabling this. +# +#migrate_tls = 1
Actually what is this option supposed to do? It seems it doesn't do anything but saying "yes, I configured TLS for migration". The TLS usage for migration is turned on by VIR_MIGRATE_TLS flag which suggests the configuration option here is useless.
It more or less follows the same logic for chardev's which got an additional 'tls="yes"' to allow one to enable/disable an object in a domain on a case by case basis if chardev tls was enabled for the host.
See: http://libvirt.org/formatdomain.html#elementsConsole
and search down for 'tls'
So my feeling while coding was - if I don't supply some sort of way to have an option to allow someone to choose to use the configured TLS environment for the migration, I'd end up being asked to add one since chardev has it.
The problem is chardevs and migration are quite different. While you can easily have a default configuration for chardevs and use tls="yes|no" to override it (no tls attribute will just tell libvirt to use the default), it's not really possible with migration API. API flags have not two states (in contrast to three-state boolean attributes in XML). Setting VIR_MIGRATE_TLS flag turns TLS on and using the flag turns TLS off. That is the default value is not used anywhere. If VIR_MIGRATE_TLS flag is used, the code checks migrate_tls is 1. Which translates to "yes, I configured TLS for migration" semantics of the configuration option.
In other words, it makes sense to have the configuration option for devices defined in the XML, but using it for migration doesn't make any sense. This would of course mean its parsing would need to be moved out of the macro introduced in 1/13 and used for chardevs only.
Jirka
So I've circled back around to this... It seems like you're advocating the removal of the VIR_MIGRATE_TLS flag - which is fine. The only
No. We definitely need VIR_MIGRATE_TLS. I'm advocating the removal of the migrate_tls configuration option. Jirka
On 02/23/2017 09:19 AM, Jiri Denemark wrote:
On Thu, Feb 23, 2017 at 08:10:18 -0500, John Ferlan wrote:
On 02/21/2017 06:43 AM, Jiri Denemark wrote:
On Mon, Feb 20, 2017 at 14:28:42 -0500, John Ferlan wrote:
On 02/20/2017 11:03 AM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:19 -0500, John Ferlan wrote:
+# Enable use of TLS encryption for migration +# +# It is necessary to setup CA and issue a server certificate +# before enabling this. +# +#migrate_tls = 1
Actually what is this option supposed to do? It seems it doesn't do anything but saying "yes, I configured TLS for migration". The TLS usage for migration is turned on by VIR_MIGRATE_TLS flag which suggests the configuration option here is useless.
It more or less follows the same logic for chardev's which got an additional 'tls="yes"' to allow one to enable/disable an object in a domain on a case by case basis if chardev tls was enabled for the host.
See: http://libvirt.org/formatdomain.html#elementsConsole
and search down for 'tls'
So my feeling while coding was - if I don't supply some sort of way to have an option to allow someone to choose to use the configured TLS environment for the migration, I'd end up being asked to add one since chardev has it.
The problem is chardevs and migration are quite different. While you can easily have a default configuration for chardevs and use tls="yes|no" to override it (no tls attribute will just tell libvirt to use the default), it's not really possible with migration API. API flags have not two states (in contrast to three-state boolean attributes in XML). Setting VIR_MIGRATE_TLS flag turns TLS on and using the flag turns TLS off. That is the default value is not used anywhere. If VIR_MIGRATE_TLS flag is used, the code checks migrate_tls is 1. Which translates to "yes, I configured TLS for migration" semantics of the configuration option.
In other words, it makes sense to have the configuration option for devices defined in the XML, but using it for migration doesn't make any sense. This would of course mean its parsing would need to be moved out of the macro introduced in 1/13 and used for chardevs only.
Jirka
So I've circled back around to this... It seems like you're advocating the removal of the VIR_MIGRATE_TLS flag - which is fine. The only
No. We definitely need VIR_MIGRATE_TLS. I'm advocating the removal of the migrate_tls configuration option.
Jirka
For me it's a consistency thing - whenever TLS is used elsewhere in the code the qemu.conf variables are referenced. Now for migration we're going to assume the configuration is set up and can be used. That just seems odd, but if that's how it's felt we should proceed, then fine I can drop the migrate_tls variable, the others would seem to be needed though. That just means the first patch is dropped and the second one removes the migrate_tls, but keeps certDir, verify, and secret uuid. John
On Thu, Feb 23, 2017 at 12:19:46 -0500, John Ferlan wrote:
The problem is chardevs and migration are quite different. While you can easily have a default configuration for chardevs and use tls="yes|no" to override it (no tls attribute will just tell libvirt to use the default), it's not really possible with migration API. API flags have not two states (in contrast to three-state boolean attributes in XML). Setting VIR_MIGRATE_TLS flag turns TLS on and using the flag turns TLS off. That is the default value is not used anywhere. If VIR_MIGRATE_TLS flag is used, the code checks migrate_tls is 1. Which translates to "yes, I configured TLS for migration" semantics of the configuration option.
In other words, it makes sense to have the configuration option for devices defined in the XML, but using it for migration doesn't make any sense. This would of course mean its parsing would need to be moved out of the macro introduced in 1/13 and used for chardevs only.
Jirka
So I've circled back around to this... It seems like you're advocating the removal of the VIR_MIGRATE_TLS flag - which is fine. The only
No. We definitely need VIR_MIGRATE_TLS. I'm advocating the removal of the migrate_tls configuration option.
Jirka
For me it's a consistency thing - whenever TLS is used elsewhere in the code the qemu.conf variables are referenced.
I think the result is actually inconsistent. The existing _tls options are useful and enable TLS for the affected devices by default. But migration is not a device, it's an action and not a device. We don't want to set the default for migration since the usage of TLS is enabled or disabled by the flag passed to migration APIs.
Now for migration we're going to assume the configuration is set up and can be used.
We are not going to assume anything. TLS will not be enabled by default. So if a user explicitly requests TLS by adding a flag, it's their responsibility to make sure the environment is setup correctly. Otherwise migration will just fail. Having to set migrate_tls = 1 in addition to using the flag is not going to give us anything. The environment may still be improperly configured.
I can drop the migrate_tls variable, the others would seem to be needed though. That just means the first patch is dropped and the second one removes the migrate_tls, but keeps certDir, verify, and secret uuid.
Right. Although you can still keep the first patch, you just need to move *_tls out of the macro. But do whathever you think is better. Jirka
It's not really 'Chardev' specific - we can reuse this for other objects. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 8 ++++---- src/qemu/qemu_alias.h | 2 +- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_hotplug.c | 6 +++--- src/qemu/qemu_monitor_json.c | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 8521a44..4cccf23 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -609,17 +609,17 @@ qemuDomainGetSecretAESAlias(const char *srcalias, } -/* qemuAliasTLSObjFromChardevAlias - * @chardev_alias: Pointer to the chardev alias string +/* qemuAliasTLSObjFromSrcAlias + * @srcAlias: Pointer to a source alias string * * Generate and return a string to be used as the TLS object alias */ char * -qemuAliasTLSObjFromChardevAlias(const char *chardev_alias) +qemuAliasTLSObjFromSrcAlias(const char *srcAlias) { char *ret; - ignore_value(virAsprintf(&ret, "obj%s_tls0", chardev_alias)); + ignore_value(virAsprintf(&ret, "obj%s_tls0", srcAlias)); return ret; } diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index dea05cf..300fd4d 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -81,7 +81,7 @@ char *qemuDomainGetMasterKeyAlias(void); char *qemuDomainGetSecretAESAlias(const char *srcalias, bool isLuks); -char *qemuAliasTLSObjFromChardevAlias(const char *chardev_alias) +char *qemuAliasTLSObjFromSrcAlias(const char *srcAlias) ATTRIBUTE_NONNULL(1); char *qemuAliasChardevFromDevAlias(const char *devAlias) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c00a47a..d831d56 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -782,7 +782,7 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, qemuCaps, &props) < 0) goto cleanup; - if (!(objalias = qemuAliasTLSObjFromChardevAlias(inalias))) + if (!(objalias = qemuAliasTLSObjFromSrcAlias(inalias))) goto cleanup; if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("tls-creds-x509", @@ -5098,7 +5098,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, charAlias, qemuCaps) < 0) goto cleanup; - if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias))) + if (!(objalias = qemuAliasTLSObjFromSrcAlias(charAlias))) goto cleanup; virBufferAsprintf(&buf, ",tls-creds=%s", objalias); VIR_FREE(objalias); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2f209f1..8d15eee 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1561,7 +1561,7 @@ qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, tlsProps) < 0) return -1; - if (!(*tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(charAlias))) return -1; dev->data.tcp.tlscreds = true; @@ -4016,7 +4016,7 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, if (chr->source->type == VIR_DOMAIN_CHR_TYPE_TCP && chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) { - if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + if (!(tlsAlias = qemuAliasTLSObjFromSrcAlias(charAlias))) goto cleanup; /* Best shot at this as the secinfo is destroyed after process launch @@ -4095,7 +4095,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, goto cleanup; if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { - if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + if (!(tlsAlias = qemuAliasTLSObjFromSrcAlias(charAlias))) goto cleanup; /* Best shot at this as the secinfo is destroyed after process launch diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 1d281af..7aa9e31 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6303,7 +6303,7 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, virJSONValueObjectAppendBoolean(data, "server", chr->data.tcp.listen) < 0) goto error; if (chr->data.tcp.tlscreds) { - if (!(tlsalias = qemuAliasTLSObjFromChardevAlias(chrID))) + if (!(tlsalias = qemuAliasTLSObjFromSrcAlias(chrID))) goto error; if (virJSONValueObjectAppendString(data, "tls-creds", tlsalias) < 0) -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:20 -0500, John Ferlan wrote:
It's not really 'Chardev' specific - we can reuse this for other objects.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 8 ++++---- src/qemu/qemu_alias.h | 2 +- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_hotplug.c | 6 +++--- src/qemu/qemu_monitor_json.c | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-)
ACK Jirka
Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be used with a migrate or nbd TLS object Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 73 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 88 +++++++++++++++++++++++++++++--------------------- 2 files changed, 124 insertions(+), 37 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be44843..dd3cfd5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1370,6 +1370,77 @@ qemuDomainSecretChardevPrepare(virConnectPtr conn, } +/* qemuDomainSecretMigrateDestroy: + * @migSecinfo: Pointer to the secinfo from the incoming def + * + * Clear and destroy memory associated with the secret + */ +void +qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo) +{ + if (!*migSecinfo) + return; + + qemuDomainSecretInfoFree(migSecinfo); +} + + +/* qemuDomainSecretMigratePrepare + * @conn: Pointer to connection + * @priv: pointer to domain private object + * @srcAlias: Alias to use (either migrate or nbd) + * @secretUUID: UUID for the secret from the cfg (migrate or nbd) + * + * Create and prepare the qemuDomainSecretInfoPtr to be used for either + * a migration or nbd. Unlike other domain secret prepare functions, this + * is only expected to be called for a single object/instance. Theoretically + * the object could be reused, although that results in keeping a secret + * stored in memory for perhaps longer than expected or necessary. + * + * Returns 0 on success, -1 on failure + */ +int +qemuDomainSecretMigratePrepare(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + const char *srcAlias, + const char *secretUUID) +{ + virSecretLookupTypeDef seclookupdef = {0}; + qemuDomainSecretInfoPtr secinfo = NULL; + + if (virUUIDParse(secretUUID, seclookupdef.u.uuid) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("malformed %s TLS secret uuid in qemu.conf"), + srcAlias); + return -1; + } + seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; + + if (VIR_ALLOC(secinfo) < 0) + return -1; + + if (qemuDomainSecretSetup(conn, priv, secinfo, srcAlias, + VIR_SECRET_USAGE_TYPE_TLS, NULL, + &seclookupdef, false) < 0) + goto error; + + if (secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TLS X.509 requires encrypted secrets " + "to be supported")); + goto error; + } + priv->migSecinfo = secinfo; + + return 0; + + error: + qemuDomainSecretInfoFree(&secinfo); + return -1; +} + + + /* qemuDomainSecretDestroy: * @vm: Domain object * @@ -1634,6 +1705,8 @@ qemuDomainObjPrivateFree(void *data) VIR_FREE(priv->libDir); VIR_FREE(priv->channelTargetDir); + + qemuDomainSecretMigrateDestroy(&priv->migSecinfo); qemuDomainMasterKeyFree(priv); VIR_FREE(priv); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 524a672..f796306 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -175,6 +175,43 @@ VIR_ENUM_DECL(qemuDomainNamespace) bool qemuDomainNamespaceEnabled(virDomainObjPtr vm, qemuDomainNamespace ns); +/* Type of domain secret */ +typedef enum { + VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0, + VIR_DOMAIN_SECRET_INFO_TYPE_AES, /* utilize GNUTLS_CIPHER_AES_256_CBC */ + + VIR_DOMAIN_SECRET_INFO_TYPE_LAST +} qemuDomainSecretInfoType; + +typedef struct _qemuDomainSecretPlain qemuDomainSecretPlain; +typedef struct _qemuDomainSecretPlain *qemuDomainSecretPlainPtr; +struct _qemuDomainSecretPlain { + char *username; + uint8_t *secret; + size_t secretlen; +}; + +# define QEMU_DOMAIN_AES_IV_LEN 16 /* 16 bytes for 128 bit random */ + /* initialization vector */ +typedef struct _qemuDomainSecretAES qemuDomainSecretAES; +typedef struct _qemuDomainSecretAES *qemuDomainSecretAESPtr; +struct _qemuDomainSecretAES { + char *username; + char *alias; /* generated alias for secret */ + char *iv; /* base64 encoded initialization vector */ + char *ciphertext; /* encoded/encrypted secret */ +}; + +typedef struct _qemuDomainSecretInfo qemuDomainSecretInfo; +typedef qemuDomainSecretInfo *qemuDomainSecretInfoPtr; +struct _qemuDomainSecretInfo { + qemuDomainSecretInfoType type; + union { + qemuDomainSecretPlain plain; + qemuDomainSecretAES aes; + } s; +}; + typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { @@ -246,48 +283,15 @@ struct _qemuDomainObjPrivate { /* note whether memory device alias does not correspond to slot number */ bool memAliasOrderMismatch; + + /* for migration's using TLS with a secret (not to be saved in our */ + /* private XML). */ + qemuDomainSecretInfoPtr migSecinfo; }; # define QEMU_DOMAIN_PRIVATE(vm) \ ((qemuDomainObjPrivatePtr) (vm)->privateData) -/* Type of domain secret */ -typedef enum { - VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0, - VIR_DOMAIN_SECRET_INFO_TYPE_AES, /* utilize GNUTLS_CIPHER_AES_256_CBC */ - - VIR_DOMAIN_SECRET_INFO_TYPE_LAST -} qemuDomainSecretInfoType; - -typedef struct _qemuDomainSecretPlain qemuDomainSecretPlain; -typedef struct _qemuDomainSecretPlain *qemuDomainSecretPlainPtr; -struct _qemuDomainSecretPlain { - char *username; - uint8_t *secret; - size_t secretlen; -}; - -# define QEMU_DOMAIN_AES_IV_LEN 16 /* 16 bytes for 128 bit random */ - /* initialization vector */ -typedef struct _qemuDomainSecretAES qemuDomainSecretAES; -typedef struct _qemuDomainSecretAES *qemuDomainSecretAESPtr; -struct _qemuDomainSecretAES { - char *username; - char *alias; /* generated alias for secret */ - char *iv; /* base64 encoded initialization vector */ - char *ciphertext; /* encoded/encrypted secret */ -}; - -typedef struct _qemuDomainSecretInfo qemuDomainSecretInfo; -typedef qemuDomainSecretInfo *qemuDomainSecretInfoPtr; -struct _qemuDomainSecretInfo { - qemuDomainSecretInfoType type; - union { - qemuDomainSecretPlain plain; - qemuDomainSecretAES aes; - } s; -}; - # define QEMU_DOMAIN_DISK_PRIVATE(disk) \ ((qemuDomainDiskPrivatePtr) (disk)->privateData) @@ -763,6 +767,16 @@ int qemuDomainSecretChardevPrepare(virConnectPtr conn, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); +void qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo) + ATTRIBUTE_NONNULL(1); + +int qemuDomainSecretMigratePrepare(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + const char *srcAlias, + const char *secretUUID) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4); + void qemuDomainSecretDestroy(virDomainObjPtr vm) ATTRIBUTE_NONNULL(1); -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:21 -0500, John Ferlan wrote:
Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be used with a migrate or nbd TLS object
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 73 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 88 +++++++++++++++++++++++++++++--------------------- 2 files changed, 124 insertions(+), 37 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be44843..dd3cfd5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1370,6 +1370,77 @@ qemuDomainSecretChardevPrepare(virConnectPtr conn, }
+/* qemuDomainSecretMigrateDestroy: + * @migSecinfo: Pointer to the secinfo from the incoming def + * + * Clear and destroy memory associated with the secret + */ +void +qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo) +{ + if (!*migSecinfo) + return; + + qemuDomainSecretInfoFree(migSecinfo); +}
This is a useless wrapper, please drop it.
+/* qemuDomainSecretMigratePrepare + * @conn: Pointer to connection + * @priv: pointer to domain private object + * @srcAlias: Alias to use (either migrate or nbd) + * @secretUUID: UUID for the secret from the cfg (migrate or nbd) + * + * Create and prepare the qemuDomainSecretInfoPtr to be used for either + * a migration or nbd. Unlike other domain secret prepare functions, this + * is only expected to be called for a single object/instance. Theoretically + * the object could be reused, although that results in keeping a secret + * stored in memory for perhaps longer than expected or necessary. + * + * Returns 0 on success, -1 on failure + */ +int +qemuDomainSecretMigratePrepare(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + const char *srcAlias, + const char *secretUUID) +{ + virSecretLookupTypeDef seclookupdef = {0}; + qemuDomainSecretInfoPtr secinfo = NULL; + + if (virUUIDParse(secretUUID, seclookupdef.u.uuid) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("malformed %s TLS secret uuid in qemu.conf"),
[1]
+ srcAlias); + return -1; + } + seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; + + if (VIR_ALLOC(secinfo) < 0) + return -1; + + if (qemuDomainSecretSetup(conn, priv, secinfo, srcAlias, + VIR_SECRET_USAGE_TYPE_TLS, NULL, + &seclookupdef, false) < 0) + goto error; + + if (secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TLS X.509 requires encrypted secrets " + "to be supported")); + goto error; + } + priv->migSecinfo = secinfo; + + return 0; + + error: + qemuDomainSecretInfoFree(&secinfo); + return -1; +}
Almost all lines in this functions were just copy-pasted from qemuDomainSecretChardevPrepare. Could you merge the two? Ideally you can just make it a function which lookups the secinfo and you can do the rest in the caller. Jirka
On Mon, Feb 20, 2017 at 16:43:54 +0100, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:21 -0500, John Ferlan wrote:
Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be used with a migrate or nbd TLS object ... +qemuDomainSecretMigratePrepare(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + const char *srcAlias, + const char *secretUUID) +{ + virSecretLookupTypeDef seclookupdef = {0}; + qemuDomainSecretInfoPtr secinfo = NULL; + + if (virUUIDParse(secretUUID, seclookupdef.u.uuid) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("malformed %s TLS secret uuid in qemu.conf"),
[1]
Oops, ignore this part of my reply :-) Jirka
On 02/20/2017 10:43 AM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:21 -0500, John Ferlan wrote:
Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be used with a migrate or nbd TLS object
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 73 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 88 +++++++++++++++++++++++++++++--------------------- 2 files changed, 124 insertions(+), 37 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be44843..dd3cfd5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1370,6 +1370,77 @@ qemuDomainSecretChardevPrepare(virConnectPtr conn, }
+/* qemuDomainSecretMigrateDestroy: + * @migSecinfo: Pointer to the secinfo from the incoming def + * + * Clear and destroy memory associated with the secret + */ +void +qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo) +{ + if (!*migSecinfo) + return; + + qemuDomainSecretInfoFree(migSecinfo); +}
This is a useless wrapper, please drop it.
Sure - it's like an automatic reaction - have allocation need free function... Will use qemuDomainSecretInfoFree instead... It does get used later in patch 12 and come to think of it, probably needs to be added to patch 13 in the cleanup of the Run.
+/* qemuDomainSecretMigratePrepare + * @conn: Pointer to connection + * @priv: pointer to domain private object + * @srcAlias: Alias to use (either migrate or nbd) + * @secretUUID: UUID for the secret from the cfg (migrate or nbd) + * + * Create and prepare the qemuDomainSecretInfoPtr to be used for either + * a migration or nbd. Unlike other domain secret prepare functions, this + * is only expected to be called for a single object/instance. Theoretically + * the object could be reused, although that results in keeping a secret + * stored in memory for perhaps longer than expected or necessary. + * + * Returns 0 on success, -1 on failure + */ +int +qemuDomainSecretMigratePrepare(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + const char *srcAlias, + const char *secretUUID) +{ + virSecretLookupTypeDef seclookupdef = {0}; + qemuDomainSecretInfoPtr secinfo = NULL; + + if (virUUIDParse(secretUUID, seclookupdef.u.uuid) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("malformed %s TLS secret uuid in qemu.conf"),
[1]
+ srcAlias); + return -1; + } + seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; + + if (VIR_ALLOC(secinfo) < 0) + return -1; + + if (qemuDomainSecretSetup(conn, priv, secinfo, srcAlias, + VIR_SECRET_USAGE_TYPE_TLS, NULL, + &seclookupdef, false) < 0) + goto error; + + if (secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TLS X.509 requires encrypted secrets " + "to be supported")); + goto error; + } + priv->migSecinfo = secinfo; + + return 0; + + error: + qemuDomainSecretInfoFree(&secinfo); + return -1; +}
Almost all lines in this functions were just copy-pasted from qemuDomainSecretChardevPrepare. Could you merge the two? Ideally you can just make it a function which lookups the secinfo and you can do the rest in the caller.
Jirka
Sure. no problem... I'll create: static qemuDomainSecretInfoPtr qemuDomainSecretTLSObjectPrepare(virConnectPtr conn, qemuDomainObjPrivatePtr priv, const char *srcAlias, const char *secretUUID) which grabs the guts and does the right thing... I'll also adjust the UUIDParse error message to just be "malformed secret uuid '%s' in qemu.conf" passing the secretUUID string for the error message Tks - John
On Mon, Feb 20, 2017 at 15:14:21 -0500, John Ferlan wrote:
On 02/20/2017 10:43 AM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:21 -0500, John Ferlan wrote:
Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be used with a migrate or nbd TLS object
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 73 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 88 +++++++++++++++++++++++++++++--------------------- 2 files changed, 124 insertions(+), 37 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be44843..dd3cfd5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1370,6 +1370,77 @@ qemuDomainSecretChardevPrepare(virConnectPtr conn, }
+/* qemuDomainSecretMigrateDestroy: + * @migSecinfo: Pointer to the secinfo from the incoming def + * + * Clear and destroy memory associated with the secret + */ +void +qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo) +{ + if (!*migSecinfo) + return; + + qemuDomainSecretInfoFree(migSecinfo); +}
This is a useless wrapper, please drop it.
Sure - it's like an automatic reaction - have allocation need free function... Will use qemuDomainSecretInfoFree instead...
It would make sense if migSecinfo was a new structure, but in this case we would just end up with two functions usable for freeing a single structure. Which is just asking for an inconsistent usage. And that's what happened even in your series; sometimes qemuDomainSecretInfoFree is used to free migSecinfo and sometime it's the new wrapper you call to do the job.
+int +qemuDomainSecretMigratePrepare(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + const char *srcAlias, + const char *secretUUID)
Almost all lines in this functions were just copy-pasted from qemuDomainSecretChardevPrepare. Could you merge the two? Ideally you can just make it a function which lookups the secinfo and you can do the rest in the caller.
Sure. no problem... I'll create:
static qemuDomainSecretInfoPtr qemuDomainSecretTLSObjectPrepare(virConnectPtr conn, qemuDomainObjPrivatePtr priv, const char *srcAlias, const char *secretUUID)
Great, but since the structure it creates is qemuDomainSecretInfo, can you just call it qemuDomainSecretInfoNew or something similar to this? Jirka
Refactor the TLS object adding code to make two separate API's that will handle the add/remove of the "secret" and "tls-creds-x509" objects including the Enter/Exit monitor commands. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 169 ++++++++++++++++++++++++++++-------------------- src/qemu/qemu_hotplug.h | 13 ++++ 2 files changed, 111 insertions(+), 71 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8d15eee..fb8a052 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1526,6 +1526,89 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, } +void +qemuDomainDelTLSObjects(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *secAlias, + const char *tlsAlias) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virErrorPtr orig_err; + + /* Nothing to do if neither defined */ + if (!tlsAlias && !secAlias) + return; + + orig_err = virSaveLastError(); + + qemuDomainObjEnterMonitor(driver, vm); + if (tlsAlias) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); + if (secAlias) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + ignore_value(qemuDomainObjExitMonitor(driver, vm)); +} + + +int +qemuDomainAddTLSObjects(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *secAlias, + virJSONValuePtr *secProps, + const char *tlsAlias, + virJSONValuePtr *tlsProps) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rc; + bool secobjAdded = false; + bool tlsobjAdded = false; + virErrorPtr orig_err; + + /* Nothing to do if neither defined */ + if (!tlsAlias && !secAlias) + return 0; + + qemuDomainObjEnterMonitor(driver, vm); + + if (secAlias) { + rc = qemuMonitorAddObject(priv->mon, "secret", + secAlias, *secProps); + *secProps = NULL; /* qemuMonitorAddObject consumes */ + if (rc < 0) + goto exit_monitor; + secobjAdded = true; + } + + if (tlsAlias) { + rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509", + tlsAlias, *tlsProps); + *tlsProps = NULL; /* qemuMonitorAddObject consumes */ + if (rc < 0) + goto exit_monitor; + tlsobjAdded = true; + } + + return qemuDomainObjExitMonitor(driver, vm); + + exit_monitor: + orig_err = virSaveLastError(); + if (tlsobjAdded) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); + if (secobjAdded) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + return -1; +} + + static int qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, qemuDomainObjPrivatePtr priv, @@ -1582,8 +1665,6 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, char *charAlias = NULL; char *devstr = NULL; bool chardevAdded = false; - bool tlsobjAdded = false; - bool secobjAdded = false; virJSONValuePtr tlsProps = NULL; virJSONValuePtr secProps = NULL; char *tlsAlias = NULL; @@ -1619,25 +1700,11 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, &secProps, &secAlias) < 0) goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - - if (secAlias) { - rc = qemuMonitorAddObject(priv->mon, "secret", - secAlias, secProps); - secProps = NULL; - if (rc < 0) - goto exit_monitor; - secobjAdded = true; - } + if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps, + tlsAlias, &tlsProps) < 0) + goto cleanup; - if (tlsAlias) { - rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509", - tlsAlias, tlsProps); - tlsProps = NULL; /* qemuMonitorAddObject consumes */ - if (rc < 0) - goto exit_monitor; - tlsobjAdded = true; - } + qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorAttachCharDev(priv->mon, charAlias, @@ -1672,15 +1739,12 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, /* detach associated chardev on error */ if (chardevAdded) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); - if (tlsobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); - if (secobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); if (orig_err) { virSetError(orig_err); virFreeError(orig_err); } ignore_value(qemuDomainObjExitMonitor(driver, vm)); + qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); goto audit; } @@ -1858,10 +1922,8 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, virDomainChrSourceDefPtr dev = chr->source; char *charAlias = NULL; bool chardevAttached = false; - bool tlsobjAdded = false; bool teardowncgroup = false; bool teardowndevice = false; - bool secobjAdded = false; virJSONValuePtr tlsProps = NULL; char *tlsAlias = NULL; virJSONValuePtr secProps = NULL; @@ -1908,24 +1970,11 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, &secProps, &secAlias) < 0) goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - if (secAlias) { - rc = qemuMonitorAddObject(priv->mon, "secret", - secAlias, secProps); - secProps = NULL; - if (rc < 0) - goto exit_monitor; - secobjAdded = true; - } + if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps, + tlsAlias, &tlsProps) < 0) + goto cleanup; - if (tlsAlias) { - rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509", - tlsAlias, tlsProps); - tlsProps = NULL; /* qemuMonitorAddObject consumes */ - if (rc < 0) - goto exit_monitor; - tlsobjAdded = true; - } + qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorAttachCharDev(priv->mon, charAlias, chr->source) < 0) goto exit_monitor; @@ -1966,16 +2015,13 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, /* detach associated chardev on error */ if (chardevAttached) qemuMonitorDetachCharDev(priv->mon, charAlias); - if (tlsobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); - if (secobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); if (orig_err) { virSetError(orig_err); virFreeError(orig_err); } ignore_value(qemuDomainObjExitMonitor(driver, vm)); + qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); goto audit; } @@ -2000,8 +2046,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, bool teardowndevice = false; bool chardevAdded = false; bool objAdded = false; - bool tlsobjAdded = false; - bool secobjAdded = false; virJSONValuePtr props = NULL; virJSONValuePtr tlsProps = NULL; virJSONValuePtr secProps = NULL; @@ -2076,27 +2120,13 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, charAlias, &tlsProps, &tlsAlias, &secProps, &secAlias) < 0) goto cleanup; - } - - qemuDomainObjEnterMonitor(driver, vm); - if (secAlias) { - rv = qemuMonitorAddObject(priv->mon, "secret", - secAlias, secProps); - secProps = NULL; - if (rv < 0) - goto exit_monitor; - secobjAdded = true; + if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps, + tlsAlias, &tlsProps) < 0) + goto cleanup; } - if (tlsAlias) { - rv = qemuMonitorAddObject(priv->mon, "tls-creds-x509", - tlsAlias, tlsProps); - tlsProps = NULL; /* qemuMonitorAddObject consumes */ - if (rv < 0) - goto exit_monitor; - tlsobjAdded = true; - } + qemuDomainObjEnterMonitor(driver, vm); if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && qemuMonitorAttachCharDev(priv->mon, charAlias, @@ -2152,10 +2182,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && chardevAdded) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); - if (tlsobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); - if (secobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); if (orig_err) { virSetError(orig_err); virFreeError(orig_err); @@ -2163,6 +2189,7 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, if (qemuDomainObjExitMonitor(driver, vm) < 0) releaseaddr = false; + qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); goto audit; } diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 13242ee..c4f33e0 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -33,6 +33,19 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, virStorageSourcePtr newsrc, bool force); + +void qemuDomainDelTLSObjects(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *secAlias, + const char *tlsAlias); + +int qemuDomainAddTLSObjects(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *secAlias, + virJSONValuePtr *secProps, + const char *tlsAlias, + virJSONValuePtr *tlsProps); + int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainControllerDefPtr controller); -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:22 -0500, John Ferlan wrote:
Refactor the TLS object adding code to make two separate API's that will handle the add/remove of the "secret" and "tls-creds-x509" objects including the Enter/Exit monitor commands.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 169 ++++++++++++++++++++++++++++-------------------- src/qemu/qemu_hotplug.h | 13 ++++ 2 files changed, 111 insertions(+), 71 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8d15eee..fb8a052 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1526,6 +1526,89 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, }
+void +qemuDomainDelTLSObjects(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *secAlias, + const char *tlsAlias) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virErrorPtr orig_err; + + /* Nothing to do if neither defined */
The comment is pretty redundant.
+ if (!tlsAlias && !secAlias) + return; + + orig_err = virSaveLastError(); + + qemuDomainObjEnterMonitor(driver, vm); + if (tlsAlias) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); + if (secAlias) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + ignore_value(qemuDomainObjExitMonitor(driver, vm));
It looks like this function is designed to preserve the original error, so shouldn't the call to ExitMonitor be done above the if (orig_err) statement?
+} + + +int +qemuDomainAddTLSObjects(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *secAlias, + virJSONValuePtr *secProps, + const char *tlsAlias, + virJSONValuePtr *tlsProps) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rc; + bool secobjAdded = false; + bool tlsobjAdded = false; + virErrorPtr orig_err; + + /* Nothing to do if neither defined */
Redundant comment again.
+ if (!tlsAlias && !secAlias) + return 0; + + qemuDomainObjEnterMonitor(driver, vm); + + if (secAlias) { + rc = qemuMonitorAddObject(priv->mon, "secret", + secAlias, *secProps); + *secProps = NULL; /* qemuMonitorAddObject consumes */ + if (rc < 0) + goto exit_monitor; + secobjAdded = true; + } + + if (tlsAlias) { + rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509", + tlsAlias, *tlsProps); + *tlsProps = NULL; /* qemuMonitorAddObject consumes */ + if (rc < 0) + goto exit_monitor; + tlsobjAdded = true; + } + + return qemuDomainObjExitMonitor(driver, vm); + + exit_monitor: + orig_err = virSaveLastError(); + if (tlsobjAdded) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); + if (secobjAdded) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + return -1;
This is suspicious, you're open coding almost all of the DelTLSObjects here. Could this function be rewritten to make use of the just introduced qemuDomainDelTLSObjects? Adding an extra ExitMonitor followed by EnterMonitor if something failed shouldn't be a big deal. We're entering and exiting the monitor all the time anyway.
static int qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, qemuDomainObjPrivatePtr priv, ... @@ -1672,15 +1739,12 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, /* detach associated chardev on error */ if (chardevAdded) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); - if (tlsobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); - if (secobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); if (orig_err) { virSetError(orig_err); virFreeError(orig_err); } ignore_value(qemuDomainObjExitMonitor(driver, vm)); + qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);
OK, we get here only when both tls and sec objects were added.
goto audit;
BTW, the audit label can be easily replaced with cleanup. And this likely applies to the two clones (qemuDomainAttachChrDevice, qemuDomainAttachRNGDevice) of this function too. Jirka
On 02/21/2017 03:00 PM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:22 -0500, John Ferlan wrote:
Refactor the TLS object adding code to make two separate API's that will handle the add/remove of the "secret" and "tls-creds-x509" objects including the Enter/Exit monitor commands.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 169 ++++++++++++++++++++++++++++-------------------- src/qemu/qemu_hotplug.h | 13 ++++ 2 files changed, 111 insertions(+), 71 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8d15eee..fb8a052 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1526,6 +1526,89 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, }
+void +qemuDomainDelTLSObjects(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *secAlias, + const char *tlsAlias) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virErrorPtr orig_err; + + /* Nothing to do if neither defined */
The comment is pretty redundant.
+ if (!tlsAlias && !secAlias) + return; + + orig_err = virSaveLastError(); + + qemuDomainObjEnterMonitor(driver, vm); + if (tlsAlias) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); + if (secAlias) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + ignore_value(qemuDomainObjExitMonitor(driver, vm));
It looks like this function is designed to preserve the original error, so shouldn't the call to ExitMonitor be done above the if (orig_err) statement?
One would think that would be fine, but then again once you check each of the places where I'm moving code the ExitMonitor is done after resetting the orig_err. IDC either way, but I was more or less following current design.
+} + + +int +qemuDomainAddTLSObjects(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *secAlias, + virJSONValuePtr *secProps, + const char *tlsAlias, + virJSONValuePtr *tlsProps) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rc; + bool secobjAdded = false; + bool tlsobjAdded = false; + virErrorPtr orig_err; + + /* Nothing to do if neither defined */
Redundant comment again.
+ if (!tlsAlias && !secAlias) + return 0; + + qemuDomainObjEnterMonitor(driver, vm); + + if (secAlias) { + rc = qemuMonitorAddObject(priv->mon, "secret", + secAlias, *secProps); + *secProps = NULL; /* qemuMonitorAddObject consumes */ + if (rc < 0) + goto exit_monitor; + secobjAdded = true; + } + + if (tlsAlias) { + rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509", + tlsAlias, *tlsProps); + *tlsProps = NULL; /* qemuMonitorAddObject consumes */ + if (rc < 0) + goto exit_monitor; + tlsobjAdded = true; + } + + return qemuDomainObjExitMonitor(driver, vm); + + exit_monitor: + orig_err = virSaveLastError(); + if (tlsobjAdded) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); + if (secobjAdded) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + return -1;
This is suspicious, you're open coding almost all of the DelTLSObjects here. Could this function be rewritten to make use of the just introduced qemuDomainDelTLSObjects? Adding an extra ExitMonitor followed by EnterMonitor if something failed shouldn't be a big deal. We're entering and exiting the monitor all the time anyway.
Yeah I can do this - probably just call the ExitMonitor first and then the DelTLSObjects afterwards which will re-enter and delete. Tks, - John
static int qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, qemuDomainObjPrivatePtr priv, ... @@ -1672,15 +1739,12 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, /* detach associated chardev on error */ if (chardevAdded) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); - if (tlsobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); - if (secobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); if (orig_err) { virSetError(orig_err); virFreeError(orig_err); } ignore_value(qemuDomainObjExitMonitor(driver, vm)); + qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);
OK, we get here only when both tls and sec objects were added.
goto audit;
BTW, the audit label can be easily replaced with cleanup. And this likely applies to the two clones (qemuDomainAttachChrDevice, qemuDomainAttachRNGDevice) of this function too.
Jirka
Create a qemuDomainAddChardevTLSObjects which will encapsulate the qemuDomainGetChardevTLSObjects and qemuDomainAddTLSObjects so that the callers don't need to worry about the props. Move the dev->type and haveTLS checks in to the Add function to avoid an unnecessary call to qemuDomainAddTLSObjects Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 80 ++++++++++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 37 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index fb8a052..6dd1d9e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1622,10 +1622,6 @@ qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, qemuDomainChrSourcePrivatePtr chrSourcePriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev); - if (dev->type != VIR_DOMAIN_CHR_TYPE_TCP || - dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_YES) - return 0; - /* Add a secret object in order to access the TLS environment. * The secinfo will only be created for serial TCP device. */ if (chrSourcePriv && chrSourcePriv->secinfo) { @@ -1652,6 +1648,43 @@ qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, } +static int +qemuDomainAddChardevTLSObjects(virQEMUDriverPtr driver, + virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, + virDomainChrSourceDefPtr dev, + char *charAlias, + char **tlsAlias, + char **secAlias) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virJSONValuePtr tlsProps = NULL; + virJSONValuePtr secProps = NULL; + + if (dev->type != VIR_DOMAIN_CHR_TYPE_TCP || + dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_YES) + return 0; + + if (qemuDomainGetChardevTLSObjects(cfg, priv, dev, charAlias, + &tlsProps, tlsAlias, + &secProps, secAlias) < 0) + goto cleanup; + + if (qemuDomainAddTLSObjects(driver, vm, *secAlias, &secProps, + *tlsAlias, &tlsProps) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virJSONValueFree(tlsProps); + virJSONValueFree(secProps); + + return ret; +} + + int qemuDomainAttachRedirdevDevice(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -1665,8 +1698,6 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, char *charAlias = NULL; char *devstr = NULL; bool chardevAdded = false; - virJSONValuePtr tlsProps = NULL; - virJSONValuePtr secProps = NULL; char *tlsAlias = NULL; char *secAlias = NULL; bool need_release = false; @@ -1695,13 +1726,8 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, redirdev->source) < 0) goto cleanup; - if (qemuDomainGetChardevTLSObjects(cfg, priv, redirdev->source, - charAlias, &tlsProps, &tlsAlias, - &secProps, &secAlias) < 0) - goto cleanup; - - if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps, - tlsAlias, &tlsProps) < 0) + if (qemuDomainAddChardevTLSObjects(driver, cfg, vm, redirdev->source, + charAlias, &tlsAlias, &secAlias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); @@ -1726,9 +1752,7 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, if (ret < 0 && need_release) qemuDomainReleaseDeviceAddress(vm, &redirdev->info, NULL); VIR_FREE(tlsAlias); - virJSONValueFree(tlsProps); VIR_FREE(secAlias); - virJSONValueFree(secProps); VIR_FREE(charAlias); VIR_FREE(devstr); virObjectUnref(cfg); @@ -1924,9 +1948,7 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, bool chardevAttached = false; bool teardowncgroup = false; bool teardowndevice = false; - virJSONValuePtr tlsProps = NULL; char *tlsAlias = NULL; - virJSONValuePtr secProps = NULL; char *secAlias = NULL; bool need_release = false; @@ -1965,13 +1987,8 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, dev) < 0) goto cleanup; - if (qemuDomainGetChardevTLSObjects(cfg, priv, dev, charAlias, - &tlsProps, &tlsAlias, - &secProps, &secAlias) < 0) - goto cleanup; - - if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps, - tlsAlias, &tlsProps) < 0) + if (qemuDomainAddChardevTLSObjects(driver, cfg, vm, dev, charAlias, + &tlsAlias, &secAlias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); @@ -2002,9 +2019,7 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, VIR_WARN("Unable to remove chr device from /dev"); } VIR_FREE(tlsAlias); - virJSONValueFree(tlsProps); VIR_FREE(secAlias); - virJSONValueFree(secProps); VIR_FREE(charAlias); VIR_FREE(devstr); virObjectUnref(cfg); @@ -2047,8 +2062,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, bool chardevAdded = false; bool objAdded = false; virJSONValuePtr props = NULL; - virJSONValuePtr tlsProps = NULL; - virJSONValuePtr secProps = NULL; virDomainCCWAddressSetPtr ccwaddrs = NULL; const char *type; int ret = -1; @@ -2116,13 +2129,8 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, rng->source.chardev) < 0) goto cleanup; - if (qemuDomainGetChardevTLSObjects(cfg, priv, rng->source.chardev, - charAlias, &tlsProps, &tlsAlias, - &secProps, &secAlias) < 0) - goto cleanup; - - if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps, - tlsAlias, &tlsProps) < 0) + if (qemuDomainAddChardevTLSObjects(driver, cfg, vm, rng->source.chardev, + charAlias, &tlsAlias, &secAlias) < 0) goto cleanup; } @@ -2155,8 +2163,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, audit: virDomainAuditRNG(vm, NULL, rng, "attach", ret == 0); cleanup: - virJSONValueFree(tlsProps); - virJSONValueFree(secProps); virJSONValueFree(props); if (ret < 0) { if (releaseaddr) -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:23 -0500, John Ferlan wrote:
Create a qemuDomainAddChardevTLSObjects which will encapsulate the qemuDomainGetChardevTLSObjects and qemuDomainAddTLSObjects so that the callers don't need to worry about the props.
Move the dev->type and haveTLS checks in to the Add function to avoid an unnecessary call to qemuDomainAddTLSObjects
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 80 ++++++++++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 37 deletions(-)
This looks OK. Jirka
Move the call to inside the qemuDomainAddChardevTLSObjects in order to further converge the code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6dd1d9e..63ff1c6 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1649,10 +1649,12 @@ qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, static int -qemuDomainAddChardevTLSObjects(virQEMUDriverPtr driver, +qemuDomainAddChardevTLSObjects(virConnectPtr conn, + virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, virDomainObjPtr vm, virDomainChrSourceDefPtr dev, + char *devAlias, char *charAlias, char **tlsAlias, char **secAlias) @@ -1666,6 +1668,9 @@ qemuDomainAddChardevTLSObjects(virQEMUDriverPtr driver, dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_YES) return 0; + if (qemuDomainSecretChardevPrepare(conn, cfg, priv, devAlias, dev) < 0) + goto cleanup; + if (qemuDomainGetChardevTLSObjects(cfg, priv, dev, charAlias, &tlsProps, tlsAlias, &secProps, secAlias) < 0) @@ -1722,12 +1727,9 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, if (VIR_REALLOC_N(def->redirdevs, def->nredirdevs+1) < 0) goto cleanup; - if (qemuDomainSecretChardevPrepare(conn, cfg, priv, redirdev->info.alias, - redirdev->source) < 0) - goto cleanup; - - if (qemuDomainAddChardevTLSObjects(driver, cfg, vm, redirdev->source, - charAlias, &tlsAlias, &secAlias) < 0) + if (qemuDomainAddChardevTLSObjects(conn, driver, cfg, vm, redirdev->source, + redirdev->info.alias, charAlias, + &tlsAlias, &secAlias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); @@ -1983,11 +1985,8 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, if (qemuDomainChrPreInsert(vmdef, chr) < 0) goto cleanup; - if (qemuDomainSecretChardevPrepare(conn, cfg, priv, chr->info.alias, - dev) < 0) - goto cleanup; - - if (qemuDomainAddChardevTLSObjects(driver, cfg, vm, dev, charAlias, + if (qemuDomainAddChardevTLSObjects(conn, driver, cfg, vm, dev, + chr->info.alias, charAlias, &tlsAlias, &secAlias) < 0) goto cleanup; @@ -2125,12 +2124,10 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, goto cleanup; if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { - if (qemuDomainSecretChardevPrepare(conn, cfg, priv, rng->info.alias, - rng->source.chardev) < 0) - goto cleanup; - - if (qemuDomainAddChardevTLSObjects(driver, cfg, vm, rng->source.chardev, - charAlias, &tlsAlias, &secAlias) < 0) + if (qemuDomainAddChardevTLSObjects(conn, driver, cfg, vm, + rng->source.chardev, + rng->info.alias, charAlias, + &tlsAlias, &secAlias) < 0) goto cleanup; } -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:24 -0500, John Ferlan wrote:
Move the call to inside the qemuDomainAddChardevTLSObjects in order to further converge the code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-)
This looks OK too. Jirka
Move the call to inside the qemuDomainAddChardevTLSObjects in order to further converge the code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 63ff1c6..c76a91e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1651,7 +1651,6 @@ qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, static int qemuDomainAddChardevTLSObjects(virConnectPtr conn, virQEMUDriverPtr driver, - virQEMUDriverConfigPtr cfg, virDomainObjPtr vm, virDomainChrSourceDefPtr dev, char *devAlias, @@ -1660,13 +1659,19 @@ qemuDomainAddChardevTLSObjects(virConnectPtr conn, char **secAlias) { int ret = -1; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virJSONValuePtr tlsProps = NULL; virJSONValuePtr secProps = NULL; + /* NB: This may alter haveTLS based on cfg */ + qemuDomainPrepareChardevSourceTLS(dev, cfg); + if (dev->type != VIR_DOMAIN_CHR_TYPE_TCP || - dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_YES) + dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_YES) { + virObjectUnref(cfg); return 0; + } if (qemuDomainSecretChardevPrepare(conn, cfg, priv, devAlias, dev) < 0) goto cleanup; @@ -1685,6 +1690,7 @@ qemuDomainAddChardevTLSObjects(virConnectPtr conn, cleanup: virJSONValueFree(tlsProps); virJSONValueFree(secProps); + virObjectUnref(cfg); return ret; } @@ -1697,7 +1703,6 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, { int ret = -1; int rc; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; char *charAlias = NULL; @@ -1708,8 +1713,6 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, bool need_release = false; virErrorPtr orig_err; - qemuDomainPrepareChardevSourceTLS(redirdev->source, cfg); - if (qemuAssignDeviceRedirdevAlias(def, redirdev, -1) < 0) goto cleanup; @@ -1727,7 +1730,7 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, if (VIR_REALLOC_N(def->redirdevs, def->nredirdevs+1) < 0) goto cleanup; - if (qemuDomainAddChardevTLSObjects(conn, driver, cfg, vm, redirdev->source, + if (qemuDomainAddChardevTLSObjects(conn, driver, vm, redirdev->source, redirdev->info.alias, charAlias, &tlsAlias, &secAlias) < 0) goto cleanup; @@ -1757,7 +1760,6 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, VIR_FREE(secAlias); VIR_FREE(charAlias); VIR_FREE(devstr); - virObjectUnref(cfg); return ret; exit_monitor: @@ -1940,7 +1942,6 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, virDomainChrDefPtr chr) { int ret = -1, rc; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virErrorPtr orig_err; virDomainDefPtr vmdef = vm->def; @@ -1958,8 +1959,6 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, qemuDomainPrepareChannel(chr, priv->channelTargetDir) < 0) goto cleanup; - qemuDomainPrepareChardevSourceTLS(dev, cfg); - if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) goto cleanup; @@ -1985,7 +1984,7 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, if (qemuDomainChrPreInsert(vmdef, chr) < 0) goto cleanup; - if (qemuDomainAddChardevTLSObjects(conn, driver, cfg, vm, dev, + if (qemuDomainAddChardevTLSObjects(conn, driver, vm, dev, chr->info.alias, charAlias, &tlsAlias, &secAlias) < 0) goto cleanup; @@ -2021,7 +2020,6 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, VIR_FREE(secAlias); VIR_FREE(charAlias); VIR_FREE(devstr); - virObjectUnref(cfg); return ret; exit_monitor: @@ -2046,7 +2044,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, virDomainObjPtr vm, virDomainRNGDefPtr rng) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_RNG, { .rng = rng } }; virErrorPtr orig_err; @@ -2107,9 +2104,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, goto cleanup; teardowncgroup = true; - if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) - qemuDomainPrepareChardevSourceTLS(rng->source.chardev, cfg); - /* build required metadata */ if (!(devstr = qemuBuildRNGDevStr(vm->def, rng, priv->qemuCaps))) goto cleanup; @@ -2124,7 +2118,7 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, goto cleanup; if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { - if (qemuDomainAddChardevTLSObjects(conn, driver, cfg, vm, + if (qemuDomainAddChardevTLSObjects(conn, driver, vm, rng->source.chardev, rng->info.alias, charAlias, &tlsAlias, &secAlias) < 0) @@ -2176,7 +2170,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, VIR_FREE(objAlias); VIR_FREE(devstr); virDomainCCWAddressSetFree(ccwaddrs); - virObjectUnref(cfg); return ret; exit_monitor: -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:25 -0500, John Ferlan wrote:
Move the call to inside the qemuDomainAddChardevTLSObjects in order to further converge the code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 63ff1c6..c76a91e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1651,7 +1651,6 @@ qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, static int qemuDomainAddChardevTLSObjects(virConnectPtr conn, virQEMUDriverPtr driver, - virQEMUDriverConfigPtr cfg, virDomainObjPtr vm, virDomainChrSourceDefPtr dev, char *devAlias, @@ -1660,13 +1659,19 @@ qemuDomainAddChardevTLSObjects(virConnectPtr conn, char **secAlias) { int ret = -1; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virJSONValuePtr tlsProps = NULL; virJSONValuePtr secProps = NULL;
+ /* NB: This may alter haveTLS based on cfg */ + qemuDomainPrepareChardevSourceTLS(dev, cfg); + if (dev->type != VIR_DOMAIN_CHR_TYPE_TCP || - dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_YES) + dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_YES) { + virObjectUnref(cfg);
I think ret = 0; goto cleanup; would be better here.
return 0; + }
Looks good otherwise. Jirka
Split apart and rename qemuDomainGetChardevTLSObjects in order to make a more generic API that can create the TLS JSON prop objects (secret and tls-creds-x509) to be used to create the objects Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 55 ++++++++++++++++++++++++++----------------------- src/qemu/qemu_hotplug.h | 11 ++++++++++ 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c76a91e..f5323f7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1609,40 +1609,34 @@ qemuDomainAddTLSObjects(virQEMUDriverPtr driver, } -static int -qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, - qemuDomainObjPrivatePtr priv, - virDomainChrSourceDefPtr dev, - char *charAlias, - virJSONValuePtr *tlsProps, - char **tlsAlias, - virJSONValuePtr *secProps, - char **secAlias) +int +qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps, + qemuDomainSecretInfoPtr secinfo, + const char *tlsCertdir, + bool tlsListen, + bool tlsVerify, + const char *srcAlias, + virJSONValuePtr *tlsProps, + char **tlsAlias, + virJSONValuePtr *secProps, + char **secAlias) { - qemuDomainChrSourcePrivatePtr chrSourcePriv = - QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev); - /* Add a secret object in order to access the TLS environment. * The secinfo will only be created for serial TCP device. */ - if (chrSourcePriv && chrSourcePriv->secinfo) { - if (qemuBuildSecretInfoProps(chrSourcePriv->secinfo, secProps) < 0) + if (secinfo) { + if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) return -1; - if (!(*secAlias = qemuDomainGetSecretAESAlias(charAlias, false))) + if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) return -1; } - if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, - dev->data.tcp.listen, - cfg->chardevTLSx509verify, - *secAlias, - priv->qemuCaps, - tlsProps) < 0) + if (qemuBuildTLSx509BackendProps(tlsCertdir, tlsListen, tlsVerify, + *secAlias, qemuCaps, tlsProps) < 0) return -1; - if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(charAlias))) + if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(srcAlias))) return -1; - dev->data.tcp.tlscreds = true; return 0; } @@ -1661,6 +1655,8 @@ qemuDomainAddChardevTLSObjects(virConnectPtr conn, int ret = -1; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; + qemuDomainChrSourcePrivatePtr chrSourcePriv; + qemuDomainSecretInfoPtr secinfo = NULL; virJSONValuePtr tlsProps = NULL; virJSONValuePtr secProps = NULL; @@ -1676,10 +1672,17 @@ qemuDomainAddChardevTLSObjects(virConnectPtr conn, if (qemuDomainSecretChardevPrepare(conn, cfg, priv, devAlias, dev) < 0) goto cleanup; - if (qemuDomainGetChardevTLSObjects(cfg, priv, dev, charAlias, - &tlsProps, tlsAlias, - &secProps, secAlias) < 0) + if ((chrSourcePriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev))) + secinfo = chrSourcePriv->secinfo; + + if (qemuDomainGetTLSObjects(priv->qemuCaps, secinfo, + cfg->chardevTLSx509certdir, + dev->data.tcp.listen, + cfg->chardevTLSx509verify, + charAlias, &tlsProps, tlsAlias, + &secProps, secAlias) < 0) goto cleanup; + dev->data.tcp.tlscreds = true; if (qemuDomainAddTLSObjects(driver, vm, *secAlias, &secProps, *tlsAlias, &tlsProps) < 0) diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index c4f33e0..458c818 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -46,6 +46,17 @@ int qemuDomainAddTLSObjects(virQEMUDriverPtr driver, const char *tlsAlias, virJSONValuePtr *tlsProps); +int qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps, + qemuDomainSecretInfoPtr secinfo, + const char *tlsCertdir, + bool tlsListen, + bool tlsVerify, + const char *srcAlias, + virJSONValuePtr *tlsProps, + char **tlsAlias, + virJSONValuePtr *secProps, + char **secAlias); + int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainControllerDefPtr controller); -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:26 -0500, John Ferlan wrote:
Split apart and rename qemuDomainGetChardevTLSObjects in order to make a more generic API that can create the TLS JSON prop objects (secret and tls-creds-x509) to be used to create the objects
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 55 ++++++++++++++++++++++++++----------------------- src/qemu/qemu_hotplug.h | 11 ++++++++++ 2 files changed, 40 insertions(+), 26 deletions(-)
Looks OK. Jirka
Add the fields to support setting tls-creds and tls-hostname during a migration (either source or target) Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 12 +++++++++--- src/qemu/qemu_monitor.h | 7 +++++++ src/qemu/qemu_monitor_json.c | 11 +++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index b15207a..f5720d3 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2504,12 +2504,16 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon, { VIR_DEBUG("compressLevel=%d:%d compressThreads=%d:%d " "decompressThreads=%d:%d cpuThrottleInitial=%d:%d " - "cpuThrottleIncrement=%d:%d", + "cpuThrottleIncrement=%d:%d tlsAlias=%d:%s " + "tlsHostname=%d:%s", params->compressLevel_set, params->compressLevel, params->compressThreads_set, params->compressThreads, params->decompressThreads_set, params->decompressThreads, params->cpuThrottleInitial_set, params->cpuThrottleInitial, - params->cpuThrottleIncrement_set, params->cpuThrottleIncrement); + params->cpuThrottleIncrement_set, params->cpuThrottleIncrement, + params->migrateTLSAlias_set, NULLSTR(params->migrateTLSAlias), + params->migrateTLSHostname_set, + NULLSTR(params->migrateTLSHostname)); QEMU_CHECK_MONITOR_JSON(mon); @@ -2517,7 +2521,9 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon, !params->compressThreads_set && !params->decompressThreads_set && !params->cpuThrottleInitial_set && - !params->cpuThrottleIncrement_set) + !params->cpuThrottleIncrement_set && + !params->migrateTLSAlias_set && + !params->migrateTLSHostname_set) return 0; return qemuMonitorJSONSetMigrationParams(mon, params); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8811d85..d719112 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -570,6 +570,13 @@ struct _qemuMonitorMigrationParams { bool cpuThrottleIncrement_set; int cpuThrottleIncrement; + + /* Input only for destination */ + bool migrateTLSAlias_set; + char *migrateTLSAlias; + + bool migrateTLSHostname_set; + char *migrateTLSHostname; }; int qemuMonitorGetMigrationParams(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7aa9e31..7a70366 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2637,6 +2637,17 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon, #undef APPEND + /* Set only parameters for TLS migration options */ + if (params->migrateTLSAlias_set && + virJSONValueObjectAppendString(args, "tls-creds", + params->migrateTLSAlias) < 0) + goto cleanup; + + if (params->migrateTLSHostname_set && + virJSONValueObjectAppendString(args, "tls-hostname", + params->migrateTLSHostname) < 0) + goto cleanup; + if (virJSONValueObjectAppend(cmd, "arguments", args) < 0) goto cleanup; args = NULL; -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:27 -0500, John Ferlan wrote:
Add the fields to support setting tls-creds and tls-hostname during a migration (either source or target)
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 12 +++++++++--- src/qemu/qemu_monitor.h | 7 +++++++ src/qemu/qemu_monitor_json.c | 11 +++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) ... diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8811d85..d719112 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -570,6 +570,13 @@ struct _qemuMonitorMigrationParams {
bool cpuThrottleIncrement_set; int cpuThrottleIncrement; + + /* Input only for destination */
What do you mean by this comment? I think you can just safely drop it :-)
+ bool migrateTLSAlias_set; + char *migrateTLSAlias; + + bool migrateTLSHostname_set; + char *migrateTLSHostname;
Both parameters are set-only, we never read them back from QEMU so there's no need for the *_set booleans. Especially when NULL tells that pretty clearly.
};
int qemuMonitorGetMigrationParams(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7aa9e31..7a70366 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2637,6 +2637,17 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
#undef APPEND
+ /* Set only parameters for TLS migration options */
Looks like another useless comment.
+ if (params->migrateTLSAlias_set && + virJSONValueObjectAppendString(args, "tls-creds", + params->migrateTLSAlias) < 0) + goto cleanup; + + if (params->migrateTLSHostname_set && + virJSONValueObjectAppendString(args, "tls-hostname", + params->migrateTLSHostname) < 0) + goto cleanup; + if (virJSONValueObjectAppend(cmd, "arguments", args) < 0) goto cleanup; args = NULL;
Jirka
On Tue, Feb 21, 2017 at 22:06:02 +0100, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:27 -0500, John Ferlan wrote:
Add the fields to support setting tls-creds and tls-hostname during a migration (either source or target)
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 12 +++++++++--- src/qemu/qemu_monitor.h | 7 +++++++ src/qemu/qemu_monitor_json.c | 11 +++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) ... diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8811d85..d719112 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -570,6 +570,13 @@ struct _qemuMonitorMigrationParams {
bool cpuThrottleIncrement_set; int cpuThrottleIncrement; + + /* Input only for destination */
What do you mean by this comment? I think you can just safely drop it :-)
+ bool migrateTLSAlias_set; + char *migrateTLSAlias; + + bool migrateTLSHostname_set; + char *migrateTLSHostname;
Both parameters are set-only, we never read them back from QEMU so there's no need for the *_set booleans. Especially when NULL tells that pretty clearly.
};
int qemuMonitorGetMigrationParams(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7aa9e31..7a70366 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2637,6 +2637,17 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
#undef APPEND
+ /* Set only parameters for TLS migration options */
Looks like another useless comment.
+ if (params->migrateTLSAlias_set && + virJSONValueObjectAppendString(args, "tls-creds", + params->migrateTLSAlias) < 0) + goto cleanup; + + if (params->migrateTLSHostname_set && + virJSONValueObjectAppendString(args, "tls-hostname", + params->migrateTLSHostname) < 0) + goto cleanup;
And we should always set these parameters if QEMU supports them to make sure some stale values are not used when VIR_MIGRATE_TLS is not set. So we might actually need the *_set booleans, but it depends on the way we implement the logic to reset the parameters. Which brings a question... how do we reset these parameters? Jirka
On 02/21/2017 04:53 PM, Jiri Denemark wrote:
On Tue, Feb 21, 2017 at 22:06:02 +0100, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:27 -0500, John Ferlan wrote:
Add the fields to support setting tls-creds and tls-hostname during a migration (either source or target)
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 12 +++++++++--- src/qemu/qemu_monitor.h | 7 +++++++ src/qemu/qemu_monitor_json.c | 11 +++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) ... diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8811d85..d719112 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -570,6 +570,13 @@ struct _qemuMonitorMigrationParams {
bool cpuThrottleIncrement_set; int cpuThrottleIncrement; + + /* Input only for destination */
What do you mean by this comment? I think you can just safely drop it :-)
Hmm it was only a couple days ago and I cannot recall...
+ bool migrateTLSAlias_set; + char *migrateTLSAlias; + + bool migrateTLSHostname_set; + char *migrateTLSHostname;
Both parameters are set-only, we never read them back from QEMU so there's no need for the *_set booleans. Especially when NULL tells that pretty clearly.
};
int qemuMonitorGetMigrationParams(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7aa9e31..7a70366 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2637,6 +2637,17 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
#undef APPEND
+ /* Set only parameters for TLS migration options */
Looks like another useless comment.
+ if (params->migrateTLSAlias_set && + virJSONValueObjectAppendString(args, "tls-creds", + params->migrateTLSAlias) < 0) + goto cleanup; + + if (params->migrateTLSHostname_set && + virJSONValueObjectAppendString(args, "tls-hostname", + params->migrateTLSHostname) < 0) + goto cleanup;
And we should always set these parameters if QEMU supports them to make sure some stale values are not used when VIR_MIGRATE_TLS is not set. So we might actually need the *_set booleans, but it depends on the way we implement the logic to reset the parameters. Which brings a question... how do we reset these parameters?
tls-hostname is only set on the client and only under certain circumstances. As for reset, I guess I assumed (hah!) deletion of the objects after the migration was done would cause the parameters to magically go away too. FWIW: Trying to follow the logic from: https://www.berrange.com/posts/2016/08/16/improving-qemu-security-part-7-tls... John
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 8 ++++++++ src/qemu/qemu_migration.h | 1 + tools/virsh-domain.c | 7 +++++++ 3 files changed, 16 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index e303140..931ff68 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -815,6 +815,14 @@ typedef enum { * post-copy mode. See virDomainMigrateStartPostCopy for more details. */ VIR_MIGRATE_POSTCOPY = (1 << 15), + + /* Setting the VIR_MIGRATE_TLS flag will cause the migration to attempt + * to use the TLS environment configured by the hypervisor in order to + * perform the migration. If incorrectly configured on either source or + * destination, the migration will fail. + */ + VIR_MIGRATE_TLS = (1 << 16), + } virDomainMigrateFlags; diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 14c6178..8d88632 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -33,6 +33,7 @@ typedef qemuMigrationCompression *qemuMigrationCompressionPtr; (VIR_MIGRATE_LIVE | \ VIR_MIGRATE_PEER2PEER | \ VIR_MIGRATE_TUNNELLED | \ + VIR_MIGRATE_TLS | \ VIR_MIGRATE_PERSIST_DEST | \ VIR_MIGRATE_UNDEFINE_SOURCE | \ VIR_MIGRATE_PAUSED | \ diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 023ec8a..63ca236 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10140,6 +10140,10 @@ static const vshCmdOptDef opts_migrate[] = { .type = VSH_OT_STRING, .help = N_("filename containing updated persistent XML for the target") }, + {.name = "tls", + .type = VSH_OT_BOOL, + .help = N_("use TLS for migration") + }, {.name = NULL} }; @@ -10381,6 +10385,9 @@ doMigrate(void *opaque) if (vshCommandOptBool(cmd, "postcopy")) flags |= VIR_MIGRATE_POSTCOPY; + if (vshCommandOptBool(cmd, "tls")) + flags |= VIR_MIGRATE_TLS; + if (flags & VIR_MIGRATE_PEER2PEER || vshCommandOptBool(cmd, "direct")) { if (virDomainMigrateToURI3(dom, desturi, params, nparams, flags) == 0) ret = '0'; -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:28 -0500, John Ferlan wrote:
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 8 ++++++++ src/qemu/qemu_migration.h | 1 + tools/virsh-domain.c | 7 +++++++ 3 files changed, 16 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index e303140..931ff68 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -815,6 +815,14 @@ typedef enum { * post-copy mode. See virDomainMigrateStartPostCopy for more details. */ VIR_MIGRATE_POSTCOPY = (1 << 15), + + /* Setting the VIR_MIGRATE_TLS flag will cause the migration to attempt + * to use the TLS environment configured by the hypervisor in order to + * perform the migration. If incorrectly configured on either source or + * destination, the migration will fail. + */ + VIR_MIGRATE_TLS = (1 << 16), + } virDomainMigrateFlags;
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 14c6178..8d88632 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -33,6 +33,7 @@ typedef qemuMigrationCompression *qemuMigrationCompressionPtr; (VIR_MIGRATE_LIVE | \ VIR_MIGRATE_PEER2PEER | \ VIR_MIGRATE_TUNNELLED | \ + VIR_MIGRATE_TLS | \
Why did you insert it in the middle of the list?
VIR_MIGRATE_PERSIST_DEST | \ VIR_MIGRATE_UNDEFINE_SOURCE | \ VIR_MIGRATE_PAUSED | \
Looks good otherwise. Jirka
On 02/21/2017 04:08 PM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:28 -0500, John Ferlan wrote:
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 8 ++++++++ src/qemu/qemu_migration.h | 1 + tools/virsh-domain.c | 7 +++++++ 3 files changed, 16 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index e303140..931ff68 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -815,6 +815,14 @@ typedef enum { * post-copy mode. See virDomainMigrateStartPostCopy for more details. */ VIR_MIGRATE_POSTCOPY = (1 << 15), + + /* Setting the VIR_MIGRATE_TLS flag will cause the migration to attempt + * to use the TLS environment configured by the hypervisor in order to + * perform the migration. If incorrectly configured on either source or + * destination, the migration will fail. + */ + VIR_MIGRATE_TLS = (1 << 16), + } virDomainMigrateFlags;
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 14c6178..8d88632 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -33,6 +33,7 @@ typedef qemuMigrationCompression *qemuMigrationCompressionPtr; (VIR_MIGRATE_LIVE | \ VIR_MIGRATE_PEER2PEER | \ VIR_MIGRATE_TUNNELLED | \ + VIR_MIGRATE_TLS | \
Why did you insert it in the middle of the list?
Dunno - I'll move it. John
VIR_MIGRATE_PERSIST_DEST | \ VIR_MIGRATE_UNDEFINE_SOURCE | \ VIR_MIGRATE_PAUSED | \
Looks good otherwise.
Jirka
Support TLS for a migration is a multistep process. The target guest must be started using the "-object tls-creds-x509,endpoint=server,...". If that TLS object requires a passphrase an addition "-object security..." would also be created. The alias/id used for the TLS object is "objmigrate_tls0", while the alias/id used for the security object is "migrate-secret0". Once the domain is started, the "tls-creds" migration parameter must be set to the alias/id of the "tls-creds-x509" object. Once the migration completes, removing the two objects is necessary since the newly started domain could then become the source of a migration and thus would not be an endpoint. Handle the possibility of libvirtd stop/reconnect by saving the fact that a migration is using TLS was started in a "migrateTLS" boolean. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 29 ++++++++++++++++- src/qemu/qemu_command.h | 4 ++- src/qemu/qemu_domain.c | 5 +++ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_migration.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.c | 4 +++ 6 files changed, 121 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d831d56..d8373aa 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -802,6 +802,27 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, } +static int +qemuBuildMigrateTLSCommandLine(virCommandPtr cmd, + virQEMUDriverConfigPtr cfg, + virQEMUCapsPtr qemuCaps, + qemuDomainSecretInfoPtr migSecinfo) +{ + if (migSecinfo && qemuBuildObjectSecretCommandLine(cmd, migSecinfo) < 0) + return -1; + + /* When starting from command line this is the target of a migration + * so we need to start a TLS endpoint server (3rd param) */ + if (qemuBuildTLSx509CommandLine(cmd, cfg->migrateTLSx509certdir, + true, cfg->migrateTLSx509verify, + !!cfg->migrateTLSx509secretUUID, + "migrate", qemuCaps) < 0) + return -1; + + return 0; +} + + #define QEMU_DEFAULT_NBD_PORT "10809" #define QEMU_DEFAULT_GLUSTER_PORT "24007" @@ -9639,6 +9660,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, bool monitor_json, virQEMUCapsPtr qemuCaps, const char *migrateURI, + bool migrateTLS, + qemuDomainSecretInfoPtr migSecinfo, virDomainSnapshotObjPtr snapshot, virNetDevVPortProfileOp vmop, bool standalone, @@ -9831,8 +9854,12 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildHostdevCommandLine(cmd, def, qemuCaps, &bootHostdevNet) < 0) goto error; - if (migrateURI) + if (migrateURI) { virCommandAddArgList(cmd, "-incoming", migrateURI, NULL); + if (migrateTLS && qemuBuildMigrateTLSCommandLine(cmd, cfg, qemuCaps, + migSecinfo) < 0) + goto error; + } if (qemuBuildMemballoonCommandLine(cmd, def, qemuCaps) < 0) goto error; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 69fe846..d6349be 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -50,6 +50,8 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, bool monitor_json, virQEMUCapsPtr qemuCaps, const char *migrateURI, + bool migrateTLS, + qemuDomainSecretInfoPtr migSecinfo, virDomainSnapshotObjPtr snapshot, virNetDevVPortProfileOp vmop, bool standalone, @@ -58,7 +60,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, size_t *nnicindexes, int **nicindexes, const char *domainLibDir) - ATTRIBUTE_NONNULL(15); + ATTRIBUTE_NONNULL(17); /* Generate the object properties for a secret */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index dd3cfd5..100ab9c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1864,6 +1864,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virBufferEscapeString(buf, "<channelTargetDir path='%s'/>\n", priv->channelTargetDir); + if (priv->migrateTLS) + virBufferAddLit(buf, "<migrateTLS/>\n"); + return 0; } @@ -2132,6 +2135,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (qemuDomainSetPrivatePathsOld(driver, vm) < 0) goto error; + priv->migrateTLS = virXPathBoolean("boolean(./migrateTLS)", ctxt) == 1; + return 0; error: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f796306..06af44a 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -287,6 +287,7 @@ struct _qemuDomainObjPrivate { /* for migration's using TLS with a secret (not to be saved in our */ /* private XML). */ qemuDomainSecretInfoPtr migSecinfo; + bool migrateTLS; }; # define QEMU_DOMAIN_PRIVATE(vm) \ diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0db1616..448d94e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1487,6 +1487,55 @@ qemuMigrationEatCookie(virQEMUDriverPtr driver, return NULL; } + +/* qemuMigrationCheckSetupTLS + * + * Check if flags desired to use TLS and whether it's configured for the + * host it's being run on (src or dst depending on caller). If configured + * to use a secret for the TLS config, generate and save the migSecinfo. + * + * Returns 0 on success (or no TLS) + */ +static int +qemuMigrationCheckSetupTLS(virQEMUDriverPtr driver, + virConnectPtr dconn, + virDomainObjPtr vm, + unsigned int flags) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverConfigPtr cfg = NULL; + + if (flags & VIR_MIGRATE_TLS) { + cfg = virQEMUDriverGetConfig(driver); + + if (!cfg->migrateTLS) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("migration TLS not enabled for the host")); + goto cleanup; + } + + priv->migrateTLS = true; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, + vm, driver->caps) < 0) + VIR_WARN("Failed to save migrateTLS for vm %s", vm->def->name); + + /* If there's a secret associated with the migrate TLS, then we + * need to grab it before attempting to create the command line. */ + if (cfg->migrateTLSx509secretUUID && + qemuDomainSecretMigratePrepare(dconn, priv, "migrate", + cfg->migrateTLSx509secretUUID) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + virObjectUnref(cfg); + return ret; +} + + static void qemuMigrationStoreDomainState(virDomainObjPtr vm) { @@ -3613,6 +3662,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, bool stopProcess = false; bool relabel = false; int rv; + char *tlsAlias = NULL; qemuMonitorMigrationParams migParams = { 0 }; virNWFilterReadLockFilterUpdates(); @@ -3779,6 +3829,13 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, VIR_QEMU_PROCESS_START_AUTODESTROY) < 0) goto stopjob; + if (qemuMigrationCheckSetupTLS(driver, dconn, vm, flags) < 0) + goto stopjob; + + if (priv->migrateTLS && + !(tlsAlias = qemuAliasTLSObjFromSrcAlias("migrate"))) + goto stopjob; + if (qemuProcessPrepareHost(driver, vm, !!incoming) < 0) goto stopjob; @@ -3806,6 +3863,12 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, compression, &migParams) < 0) goto stopjob; + /* A set only parameter to indicate the "tls-creds-x509" object id */ + if (priv->migrateTLS) { + migParams.migrateTLSAlias = tlsAlias; + migParams.migrateTLSAlias_set = true; + } + if (STREQ_NULLABLE(protocol, "rdma") && virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 0) { goto stopjob; @@ -3891,6 +3954,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, ret = 0; cleanup: + VIR_FREE(tlsAlias); qemuProcessIncomingDefFree(incoming); VIR_FREE(xmlout); VIR_FORCE_CLOSE(dataFD[0]); @@ -6185,6 +6249,22 @@ qemuMigrationFinish(virQEMUDriverPtr driver, qemuDomainCleanupRemove(vm, qemuMigrationPrepareCleanup); VIR_FREE(priv->job.completed); + /* If migration used TLS, then command line creation generated a + * secinfo object and a TLS server object. Remove both now as this + * domain would now be a potential client of the next migration. */ + if (priv->migrateTLS) { + char *tlsAlias = qemuAliasTLSObjFromSrcAlias("migrate"); + char *secAlias = qemuDomainGetSecretAESAlias("migrate", false); + + qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); + qemuDomainSecretMigrateDestroy(&priv->migSecinfo); + VIR_FREE(tlsAlias); + priv->migrateTLS = false; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, + vm, driver->caps) < 0) + VIR_WARN("Failed to save migrateTLS on vm %s", vm->def->name); + } + cookie_flags = QEMU_MIGRATION_COOKIE_NETWORK | QEMU_MIGRATION_COOKIE_STATS | QEMU_MIGRATION_COOKIE_NBD; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 76f132b..b9d6a9d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5469,6 +5469,8 @@ qemuProcessLaunch(virConnectPtr conn, vm->def, priv->monConfig, priv->monJSON, priv->qemuCaps, incoming ? incoming->launchURI : NULL, + priv->migrateTLS, + priv->migSecinfo, snapshot, vmop, false, qemuCheckFips(), @@ -5901,6 +5903,8 @@ qemuProcessCreatePretendCmd(virConnectPtr conn, priv->monJSON, priv->qemuCaps, migrateURI, + false, + NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, standalone, -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:29 -0500, John Ferlan wrote:
Support TLS for a migration is a multistep process. The target guest must be started using the "-object tls-creds-x509,endpoint=server,...". If that TLS object requires a passphrase an addition "-object security..." would also be created. The alias/id used for the TLS object is "objmigrate_tls0", while the alias/id used for the security object is "migrate-secret0".
Heh, we should just hotplug the objects similarly to what the source does. As a bonus we should get a bit better error message when the destination QEMU does not support TLS. We already use -incoming defer for quite some time exactly because we want to be able to configure migration parameters before asking QEMU to listen for the migration stream. And since -incoming defer was introduced long before TLS migration, we can unconditionally require it. If you want to be extra safe, you can report an error if anyone tries to use TLS migration with QEMU which does not support -incoming defer (QEMU_CAPS_INCOMING_DEFER). BTW, are the error messages good enough if the destination QEMU does not support TLS or do we need to probe for TLS support and report the error ourselves? Jirka
On 02/21/2017 04:25 PM, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:29 -0500, John Ferlan wrote:
Support TLS for a migration is a multistep process. The target guest must be started using the "-object tls-creds-x509,endpoint=server,...". If that TLS object requires a passphrase an addition "-object security..." would also be created. The alias/id used for the TLS object is "objmigrate_tls0", while the alias/id used for the security object is "migrate-secret0".
Heh, we should just hotplug the objects similarly to what the source does. As a bonus we should get a bit better error message when the destination QEMU does not support TLS. We already use -incoming defer for quite some time exactly because we want to be able to configure migration parameters before asking QEMU to listen for the migration stream. And since -incoming defer was introduced long before TLS migration, we can unconditionally require it. If you want to be extra safe, you can report an error if anyone tries to use TLS migration with QEMU which does not support -incoming defer (QEMU_CAPS_INCOMING_DEFER).
BTW, are the error messages good enough if the destination QEMU does not support TLS or do we need to probe for TLS support and report the error ourselves?
Jirka
I flipped a coin and it landed on heads which said, add it to the command line. I can add the objects to the server prior to usage and setting of the parameters just like is done for the client. My "experience" with libvirt migration is limited - so thanks for the extra details about -incoming... The original thoughts I had on this were very different, but thankfully I found Daniel's blog... John
https://bugzilla.redhat.com/show_bug.cgi?id=1300769 Modify the Begin phase to add the checks to determine whether a migration wishes to use TLS and whether it's configured including adding the secret into the priv->migSecinfo for the source domain. Modify the Perform phase in qemuMigrationRun in order to generate the TLS objects to be used for the migration and set the migration channel parameters 'tls-creds' and possibly 'tls-hostname' in order to enable TLS. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 448d94e..84eb6a3 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3362,6 +3362,9 @@ qemuMigrationBegin(virConnectPtr conn, goto endjob; } + if (qemuMigrationCheckSetupTLS(driver, conn, vm, flags) < 0) + goto endjob; + /* Check if there is any ejected media. * We don't want to require them on the destination. */ @@ -4709,8 +4712,14 @@ qemuMigrationRun(virQEMUDriverPtr driver, { int ret = -1; unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; + virQEMUDriverConfigPtr cfg = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; qemuMigrationCookiePtr mig = NULL; + virJSONValuePtr tlsProps = NULL; + virJSONValuePtr secProps = NULL; + char *tlsAlias = NULL; + char *tlsHostname = NULL; + char *secAlias = NULL; qemuMigrationIOThreadPtr iothread = NULL; int fd = -1; unsigned long migrate_speed = resource ? resource : priv->migMaxBandwidth; @@ -4774,6 +4783,44 @@ qemuMigrationRun(virQEMUDriverPtr driver, if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig, graphicsuri) < 0) VIR_WARN("unable to provide data for graphics client relocation"); + /* If we're using TLS attempt to add the objects */ + if (priv->migrateTLS) { + cfg = virQEMUDriverGetConfig(driver); + + if (qemuDomainGetTLSObjects(priv->qemuCaps, priv->migSecinfo, + cfg->migrateTLSx509certdir, false, + cfg->migrateTLSx509verify, + "migrate", &tlsProps, &tlsAlias, + &secProps, &secAlias) < 0) + goto cleanup; + + /* Ensure the domain doesn't already have the TLS objects defined... + * This should prevent any issues just in case some cleanup wasn't + * properly completed (both src and dst use the same aliases) or + * some other error path between now and perform . */ + qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); + + /* Add the migrate TLS objects to the domain */ + if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps, + tlsAlias, &tlsProps) < 0) + goto cleanup; + + migParams->migrateTLSAlias = tlsAlias; + migParams->migrateTLSAlias_set = true; + + /* We need to add the tls-hostname only for special circumstances. + * When using "fd:" or "exec:", qemu needs to know the hostname of + * the target qemu to correctly validate the x509 certificate + * it receives. */ + if (STREQ(spec->dest.host.protocol, "fd") || + STREQ(spec->dest.host.protocol, "exec")) { + if (VIR_STRDUP(tlsHostname, spec->dest.host.name) < 0) + goto cleanup; + migParams->migrateTLSHostname = tlsHostname; + migParams->migrateTLSHostname_set = true; + } + } + if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK | QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) { if (mig->nbd) { @@ -4954,6 +5001,14 @@ qemuMigrationRun(virQEMUDriverPtr driver, ret = -1; } + qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); + virJSONValueFree(tlsProps); + virJSONValueFree(secProps); + VIR_FREE(tlsAlias); + VIR_FREE(tlsHostname); + VIR_FREE(secAlias); + virObjectUnref(cfg); + if (spec->fwdType != MIGRATION_FWD_DIRECT) { if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0) ret = -1; -- 2.9.3
On Fri, Feb 17, 2017 at 14:39:30 -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1300769
Modify the Begin phase to add the checks to determine whether a migration wishes to use TLS and whether it's configured including adding the secret into the priv->migSecinfo for the source domain.
Modify the Perform phase in qemuMigrationRun in order to generate the TLS objects to be used for the migration and set the migration channel parameters 'tls-creds' and possibly 'tls-hostname' in order to enable TLS.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 448d94e..84eb6a3 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3362,6 +3362,9 @@ qemuMigrationBegin(virConnectPtr conn, goto endjob; }
+ if (qemuMigrationCheckSetupTLS(driver, conn, vm, flags) < 0) + goto endjob; + /* Check if there is any ejected media. * We don't want to require them on the destination. */ @@ -4709,8 +4712,14 @@ qemuMigrationRun(virQEMUDriverPtr driver, { int ret = -1; unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; + virQEMUDriverConfigPtr cfg = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; qemuMigrationCookiePtr mig = NULL; + virJSONValuePtr tlsProps = NULL; + virJSONValuePtr secProps = NULL; + char *tlsAlias = NULL; + char *tlsHostname = NULL; + char *secAlias = NULL; qemuMigrationIOThreadPtr iothread = NULL; int fd = -1; unsigned long migrate_speed = resource ? resource : priv->migMaxBandwidth; @@ -4774,6 +4783,44 @@ qemuMigrationRun(virQEMUDriverPtr driver, if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig, graphicsuri) < 0) VIR_WARN("unable to provide data for graphics client relocation");
+ /* If we're using TLS attempt to add the objects */
Redundant comment.
+ if (priv->migrateTLS) { + cfg = virQEMUDriverGetConfig(driver); + + if (qemuDomainGetTLSObjects(priv->qemuCaps, priv->migSecinfo, + cfg->migrateTLSx509certdir, false, + cfg->migrateTLSx509verify, + "migrate", &tlsProps, &tlsAlias, + &secProps, &secAlias) < 0) + goto cleanup; + + /* Ensure the domain doesn't already have the TLS objects defined... + * This should prevent any issues just in case some cleanup wasn't + * properly completed (both src and dst use the same aliases) or + * some other error path between now and perform . */ + qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);
This shouldn't be needed if you do what I suggest at the end of this email, but I agree we can just call it anyway for extra safety.
+ + /* Add the migrate TLS objects to the domain */ + if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps, + tlsAlias, &tlsProps) < 0) + goto cleanup; + + migParams->migrateTLSAlias = tlsAlias; + migParams->migrateTLSAlias_set = true; + + /* We need to add the tls-hostname only for special circumstances. + * When using "fd:" or "exec:", qemu needs to know the hostname of + * the target qemu to correctly validate the x509 certificate + * it receives. */ + if (STREQ(spec->dest.host.protocol, "fd") || + STREQ(spec->dest.host.protocol, "exec")) { + if (VIR_STRDUP(tlsHostname, spec->dest.host.name) < 0) + goto cleanup; + migParams->migrateTLSHostname = tlsHostname; + migParams->migrateTLSHostname_set = true; + } + } +
Can most of this code be moved into a separate function?
if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK | QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) { if (mig->nbd) { @@ -4954,6 +5001,14 @@ qemuMigrationRun(virQEMUDriverPtr driver, ret = -1; }
+ qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias); + virJSONValueFree(tlsProps); + virJSONValueFree(secProps); + VIR_FREE(tlsAlias); + VIR_FREE(tlsHostname); + VIR_FREE(secAlias); + virObjectUnref(cfg); + if (spec->fwdType != MIGRATION_FWD_DIRECT) { if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0) ret = -1;
You store the migrateTLS info in the status XML on the destination host where libvirtd restart almost always kills the QEMU process. But you didn't bother storing the flag on the source where the QEMU process almost always remains running when libvirtd is restarted. The freshly started libvirtd calls qemuProcessRecoverMigration* to finish or cancel the ongoing migration and both functions (or functions which are called from them) need to properly cleanup the TLS objects. Jirka
On Tue, Feb 21, 2017 at 22:47:38 +0100, Jiri Denemark wrote:
On Fri, Feb 17, 2017 at 14:39:30 -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1300769
Modify the Begin phase to add the checks to determine whether a migration wishes to use TLS and whether it's configured including adding the secret into the priv->migSecinfo for the source domain.
Modify the Perform phase in qemuMigrationRun in order to generate the TLS objects to be used for the migration and set the migration channel parameters 'tls-creds' and possibly 'tls-hostname' in order to enable TLS.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) ... You store the migrateTLS info in the status XML on the destination host where libvirtd restart almost always kills the QEMU process. But you didn't bother storing the flag on the source where the QEMU process almost always remains running when libvirtd is restarted.
The freshly started libvirtd calls qemuProcessRecoverMigration* to finish or cancel the ongoing migration and both functions (or functions which are called from them) need to properly cleanup the TLS objects.
Actually I think we don't need to store migrateTLS in the status XML at all since we can just unconditionally delete the objects when a restarted libvirtd founds a domain with running migration. Jirka
participants (4)
-
Daniel P. Berrange -
Jiri Denemark -
John Ferlan -
Peter Krempa