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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list