1:39 p.m.
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
1:39 p.m.
New subject: [libvirt] [PATCH 01/13] qemu: Create #define for TLS configuration setup.
Create GET_CONFIG_TLS_CERT to set up the TLS for 'chardev' TLS setting.
Soon to be reused.
Signed-off-by: John Ferlan <jferlan(a)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
1:54 a.m.
New subject: [libvirt] [PATCH 01/13] qemu: Create #define for TLS configuration setup.
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(a)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?
3:17 a.m.
New subject: [libvirt] [PATCH 01/13] qemu: Create #define for TLS configuration setup.
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(a)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.
5:55 a.m.
New subject: [libvirt] [PATCH 01/13] qemu: Create #define for TLS configuration setup.
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(a)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
9:09 a.m.
New subject: [libvirt] [PATCH 01/13] qemu: Create #define for TLS configuration setup.
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(a)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
1:39 p.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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(a)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
9:13 a.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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(a)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
9:26 a.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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(a)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
9:30 a.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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(a)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/ :|
9:35 a.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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(a)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/ :|
10:03 a.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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(a)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
1:28 p.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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(a)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
5:43 a.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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
7:10 a.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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
8:19 a.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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
11:19 a.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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
3:36 a.m.
New subject: [libvirt] [PATCH 02/13] conf: Introduce migrate_tls_x509_cert_dir
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
1:39 p.m.
New subject: [libvirt] [PATCH 03/13] qemu: Rename qemuAliasTLSObjFromChardevAlias
It's not really 'Chardev' specific - we can reuse this for other objects.
Signed-off-by: John Ferlan <jferlan(a)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
9:15 a.m.
New subject: [libvirt] [PATCH 03/13] qemu: Rename qemuAliasTLSObjFromChardevAlias
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(a)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
1:39 p.m.
New subject: [libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}
Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be
used with a migrate or nbd TLS object
Signed-off-by: John Ferlan <jferlan(a)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
9:43 a.m.
New subject: [libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}
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(a)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
9:49 a.m.
New subject: [libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}
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
2:14 p.m.
New subject: [libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}
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(a)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
5:56 a.m.
New subject: [libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}
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(a)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
1:39 p.m.
New subject: [libvirt] [PATCH 05/13] qemu: Refactor hotplug to introduce qemuDomain{Add|Del}TLSObjects
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(a)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
2 p.m.
New subject: [libvirt] [PATCH 05/13] qemu: Refactor hotplug to introduce qemuDomain{Add|Del}TLSObjects
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(a)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
10:20 p.m.
New subject: [libvirt] [PATCH 05/13] qemu: Refactor hotplug to introduce qemuDomain{Add|Del}TLSObjects
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(a)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
1:39 p.m.
New subject: [libvirt] [PATCH 06/13] qemu: Refactor qemuDomainGetChardevTLSObjects to converge code
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(a)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
2:10 p.m.
New subject: [libvirt] [PATCH 06/13] qemu: Refactor qemuDomainGetChardevTLSObjects to converge code
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(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 80 ++++++++++++++++++++++++++-----------------------
1 file changed, 43 insertions(+), 37 deletions(-)
This looks OK.
Jirka
1:39 p.m.
New subject: [libvirt] [PATCH 07/13] qemu: Move qemuDomainSecretChardevPrepare call
Move the call to inside the qemuDomainAddChardevTLSObjects in order to
further converge the code.
Signed-off-by: John Ferlan <jferlan(a)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
2:21 p.m.
New subject: [libvirt] [PATCH 07/13] qemu: Move qemuDomainSecretChardevPrepare call
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(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 33 +++++++++++++++------------------
1 file changed, 15 insertions(+), 18 deletions(-)
This looks OK too.
Jirka
1:39 p.m.
New subject: [libvirt] [PATCH 08/13] qemu: Move qemuDomainPrepareChardevSourceTLS call
Move the call to inside the qemuDomainAddChardevTLSObjects in order to
further converge the code.
Signed-off-by: John Ferlan <jferlan(a)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
2:44 p.m.
New subject: [libvirt] [PATCH 08/13] qemu: Move qemuDomainPrepareChardevSourceTLS call
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(a)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
1:39 p.m.
New subject: [libvirt] [PATCH 09/13] qemu: Introduce qemuDomainGetTLSObjects
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(a)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
2:47 p.m.
New subject: [libvirt] [PATCH 09/13] qemu: Introduce qemuDomainGetTLSObjects
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(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 55 ++++++++++++++++++++++++++-----------------------
src/qemu/qemu_hotplug.h | 11 ++++++++++
2 files changed, 40 insertions(+), 26 deletions(-)
Looks OK.
Jirka
1:39 p.m.
New subject: [libvirt] [PATCH 10/13] qemu: Add TLS params to _qemuMonitorMigrationParams
Add the fields to support setting tls-creds and tls-hostname during
a migration (either source or target)
Signed-off-by: John Ferlan <jferlan(a)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
3:06 p.m.
New subject: [libvirt] [PATCH 10/13] qemu: Add TLS params to _qemuMonitorMigrationParams
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(a)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
3:53 p.m.
New subject: [libvirt] [PATCH 10/13] qemu: Add TLS params to _qemuMonitorMigrationParams
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(a)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
10:27 p.m.
New subject: [libvirt] [PATCH 10/13] qemu: Add TLS params to _qemuMonitorMigrationParams
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(a)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-...
John
1:39 p.m.
New subject: [libvirt] [PATCH 11/13] Add new migration flag VIR_MIGRATE_TLS
Signed-off-by: John Ferlan <jferlan(a)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
3:08 p.m.
New subject: [libvirt] [PATCH 11/13] Add new migration flag VIR_MIGRATE_TLS
On Fri, Feb 17, 2017 at 14:39:28 -0500, John Ferlan wrote:
Signed-off-by: John Ferlan <jferlan(a)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
10:27 p.m.
New subject: [libvirt] [PATCH 11/13] Add new migration flag VIR_MIGRATE_TLS
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(a)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
1:39 p.m.
New subject: [libvirt] [PATCH 12/13] qemu: Set up the migrate TLS objects for target
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(a)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
3:25 p.m.
New subject: [libvirt] [PATCH 12/13] qemu: Set up the migrate TLS objects for target
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
10:32 p.m.
New subject: [libvirt] [PATCH 12/13] qemu: Set up the migrate TLS objects for target
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
1:39 p.m.
New subject: [libvirt] [PATCH 13/13] qemu: Set up the migration TLS objects for source
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(a)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
3:47 p.m.
New subject: [libvirt] [PATCH 13/13] qemu: Set up the migration TLS objects for source
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(a)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
2:15 a.m.
New subject: [libvirt] [PATCH 13/13] qemu: Set up the migration TLS objects for source
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(a)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
2863
days inactive
2870
days old
48 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Jiri Denemark -
John Ferlan -
Peter Krempa