[libvirt] [libvirt PATCH] qemu: Add support for specifying SPICE TLS ciphers
From: Christophe Fergeau <cfergeau@redhat.com> This commit adds a 'spice_tls_ciphers' parameter in qemu.conf which allows to configure which TLS ciphers SPICE will be using for its TLS connections. https://bugzilla.redhat.com/show_bug.cgi?id=1562032 Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com> --- This is mostly useful as a workaround for missing crypto policies, so I'm not sure if it's upstream material. Changes from the patch attached to the BZ: ciphers(2) -> ciphers(1) Added augeas changes and tests escape the string before passing it to QEMU src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 5 ++++ src/qemu/qemu_command.c | 8 +++++- src/qemu/qemu_conf.c | 3 +++ src/qemu/qemu_conf.h | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + .../graphics-spice-sasl-ciphers.args | 29 ++++++++++++++++++++++ .../graphics-spice-sasl-ciphers.xml | 1 + tests/qemuxml2argvtest.c | 5 ++++ 9 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/graphics-spice-sasl-ciphers.args create mode 120000 tests/qemuxml2argvdata/graphics-spice-sasl-ciphers.xml diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index c19bf3a43..15222d7e3 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -44,6 +44,7 @@ module Libvirtd_qemu = let spice_entry = str_entry "spice_listen" | bool_entry "spice_tls" | str_entry "spice_tls_x509_cert_dir" + | str_entry "spice_tls_ciphers" | bool_entry "spice_auto_unix_socket" | str_entry "spice_password" | bool_entry "spice_sasl" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 07eab7eff..1d7b6c555 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -181,6 +181,11 @@ #spice_tls_x509_cert_dir = "/etc/pki/libvirt-spice" +# The ciphers used by spice can be overridden here. This is an OpenSSL cipher +# list as documented in ciphers(1) +#spice_tls_ciphers = "DEFAULT" + + # Enable this option to have SPICE served over an automatically created # unix socket. This prevents unprivileged access from users on the # host machine. diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 682d71441..adf0b2cb9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8028,8 +8028,14 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, !cfg->spicePassword) virBufferAddLit(&opt, "disable-ticketing,"); - if (hasSecure) + if (hasSecure) { virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir); + if (cfg->spiceTLSCiphers) { + virBufferAddLit(&opt, "tls-ciphers="); + virQEMUBuildBufferEscapeComma(&opt, cfg->spiceTLSCiphers); + virBufferAddLit(&opt, ","); + } + } switch (graphics->data.spice.defaultMode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 36cf3a281..92afd252d 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -374,6 +374,7 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->vncSASLdir); VIR_FREE(cfg->spiceTLSx509certdir); + VIR_FREE(cfg->spiceTLSCiphers); VIR_FREE(cfg->spiceListen); VIR_FREE(cfg->spicePassword); VIR_FREE(cfg->spiceSASLdir); @@ -550,6 +551,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueString(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0) goto cleanup; + if (virConfGetValueString(conf, "spice_tls_ciphers", &cfg->spiceTLSCiphers) < 0) + goto cleanup; if (virConfGetValueBool(conf, "spice_sasl", &cfg->spiceSASL) < 0) goto cleanup; if (virConfGetValueString(conf, "spice_sasl_dir", &cfg->spiceSASLdir) < 0) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index e1ad5463f..9ab9f4e37 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -128,6 +128,7 @@ struct _virQEMUDriverConfig { bool spiceTLS; char *spiceTLSx509certdir; + char *spiceTLSCiphers; bool spiceSASL; char *spiceSASLdir; char *spiceListen; diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 688e5b9fd..2f62b383e 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -17,6 +17,7 @@ module Test_libvirtd_qemu = { "spice_listen" = "0.0.0.0" } { "spice_tls" = "1" } { "spice_tls_x509_cert_dir" = "/etc/pki/libvirt-spice" } +{ "spice_tls_ciphers" = "DEFAULT" } { "spice_auto_unix_socket" = "1" } { "spice_password" = "XYZ12345" } { "spice_sasl" = "1" } diff --git a/tests/qemuxml2argvdata/graphics-spice-sasl-ciphers.args b/tests/qemuxml2argvdata/graphics-spice-sasl-ciphers.args new file mode 100644 index 000000000..2f608ad7c --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-spice-sasl-ciphers.args @@ -0,0 +1,29 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +SASL_CONF_PATH=/root/.sasl2 \ +QEMU_AUDIO_DRV=spice \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-spice port=5903,tls-port=5904,addr=127.0.0.1,sasl,\ +x509-dir=/etc/pki/libvirt-spice,tls-ciphers=DEFAULT,tls-channel=default \ +-vga qxl \ +-global qxl-vga.ram_size=67108864 \ +-global qxl-vga.vram_size=33554432 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/graphics-spice-sasl-ciphers.xml b/tests/qemuxml2argvdata/graphics-spice-sasl-ciphers.xml new file mode 120000 index 000000000..1bfac9efa --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-spice-sasl-ciphers.xml @@ -0,0 +1 @@ +graphics-spice-sasl.xml \ No newline at end of file diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 308d71f72..29f702c5c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1126,6 +1126,11 @@ mymain(void) DO_TEST("graphics-spice-sasl", QEMU_CAPS_SPICE, QEMU_CAPS_DEVICE_QXL); + ignore_value(VIR_STRDUP(driver.config->spiceTLSCiphers, "DEFAULT")); + DO_TEST("graphics-spice-sasl-ciphers", + QEMU_CAPS_SPICE, + QEMU_CAPS_DEVICE_QXL); + VIR_FREE(driver.config->spiceTLSCiphers); VIR_FREE(driver.config->spiceSASLdir); driver.config->spiceSASL = 0; DO_TEST("graphics-spice-agentmouse", -- 2.13.6
On Tue, Apr 03, 2018 at 17:23:50 +0200, Ján Tomko wrote:
From: Christophe Fergeau <cfergeau@redhat.com>
This commit adds a 'spice_tls_ciphers' parameter in qemu.conf which allows to configure which TLS ciphers SPICE will be using for its TLS connections.
https://bugzilla.redhat.com/show_bug.cgi?id=1562032
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com> --- This is mostly useful as a workaround for missing crypto policies, so I'm not sure if it's upstream material.
Are OpenSSL crypto policies supported upstream? If so, I think we should just rely on them and leave this workaround for downstreams if they want it. Also, what would we do if spice changed its TLS code to use another library, wouldn't it force us to translate the parameters from OpenSSL to the other library if this happens and this code is merged upstream? Jirka
On Tue, Apr 03, 2018 at 08:11:05PM +0200, Jiri Denemark wrote:
On Tue, Apr 03, 2018 at 17:23:50 +0200, Ján Tomko wrote:
From: Christophe Fergeau <cfergeau@redhat.com>
This commit adds a 'spice_tls_ciphers' parameter in qemu.conf which allows to configure which TLS ciphers SPICE will be using for its TLS connections.
https://bugzilla.redhat.com/show_bug.cgi?id=1562032
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com> --- This is mostly useful as a workaround for missing crypto policies, so I'm not sure if it's upstream material.
Are OpenSSL crypto policies supported upstream? If so, I think we should just rely on them and leave this workaround for downstreams if they want it. Also, what would we do if spice changed its TLS code to use another library, wouldn't it force us to translate the parameters from OpenSSL to the other library if this happens and this code is merged upstream?
The latter is actually my biggest concern with this. I would really like to get SPICE to use the QEMU TLS creds framework, so we can do the normal -object tls-creds-x509 ... setup as we do for other parts ofo QEMU. This would mean SPICE using GNUTLS format for specifying ciphers. In fact the GNUTLS format specifies ciphers and TLS protocol versions - both of which the quoted bug is asking for - whereas this patch only specifies ciphers Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Apr 04, 2018 at 09:11:25AM +0100, Daniel P. Berrangé wrote:
On Tue, Apr 03, 2018 at 08:11:05PM +0200, Jiri Denemark wrote:
On Tue, Apr 03, 2018 at 17:23:50 +0200, Ján Tomko wrote:
From: Christophe Fergeau <cfergeau@redhat.com>
This commit adds a 'spice_tls_ciphers' parameter in qemu.conf which allows to configure which TLS ciphers SPICE will be using for its TLS connections.
https://bugzilla.redhat.com/show_bug.cgi?id=1562032
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com> --- This is mostly useful as a workaround for missing crypto policies, so I'm not sure if it's upstream material.
Are OpenSSL crypto policies supported upstream? If so, I think we should just rely on them and leave this workaround for downstreams if they want it.
--system-ciphers-file support seems to be a fedora patch (openssl-1.1.0-system-cipherlist.patch), I could not find it in the upstream git. Something I realized recently is that OpenSSL crypto policies as they are now in Fedora only allow to configure TLS ciphers, you cannot enable/disable TLS protocol versions. This is possible with the gnutls crypto policies.
Also, what would we do if spice changed its TLS code to use another library, wouldn't it force us to translate the parameters from OpenSSL to the other library if this happens and this code is merged upstream?
The latter is actually my biggest concern with this. I would really like to get SPICE to use the QEMU TLS creds framework, so we can do the normal -object tls-creds-x509 ... setup as we do for other parts ofo QEMU. This would mean SPICE using GNUTLS format for specifying ciphers. In fact the GNUTLS format specifies ciphers and TLS protocol versions - both of which the quoted bug is asking for - whereas this patch only specifies ciphers
Yes, I also want to explore making use of -object tls-creds-x509 for SPICE. However, if doing that, it seems better to go the whole way, and let QEMU do the socket handling including TLS, which is probably some longer term work I also have a set of patches to address the TLS version issue, which adds a -spice tls-min-version command line option, and another libvirt qemu.conf option. I can definitely spend more time on -object tls-creds-x509 support before we decide whether to move forward one way or the other. Another stop gap solution I was thinking of, but which I'm not sure it would work would be to extend <qemu:commandline> so that we can use it to append options to existing commandline parameters, rather than always adding a new parameter with the option. If there had been a way to generate -spice $libvirt_generated_args,tls-ciphers=xxxx rather than -spice $libvirt_generated_args -spice tls-ciphers=xxxx , I don't think https://bugzilla.redhat.com/show_bug.cgi?id=1562032 would have been opened. Christophe
participants (4)
-
Christophe Fergeau -
Daniel P. Berrangé -
Jiri Denemark -
Ján Tomko