On Wed, Jul 18, 2018 at 09:55:47AM +0200, Ján Tomko wrote:
On Wed, Jul 18, 2018 at 08:32:44AM +0100, Daniel P. Berrangé wrote:
On Tue, Jul 17, 2018 at 05:15:56PM +0200, Ján Tomko wrote:
The tls, x509 and x509verify options were deprecated in QEMU v2.5.0:
commit 3e305e4a4752f70c0b5c3cf5b43ec957881714f7 Author: Daniel P. Berrange <berrange@redhat.com>
ui: convert VNC server to use QCryptoTLSSession
Use the tls-creds-x509 object when available.
https://bugzilla.redhat.com/show_bug.cgi?id=1598167
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 26 +++++++++++++++++----- .../graphics-vnc-tls.x86_64-latest.args | 4 +++- 2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 44ae8dcef7..9326abbe63 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7917,13 +7917,27 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, virBufferAddLit(&opt, ",password");
if (cfg->vncTLS) { - virBufferAddLit(&opt, ",tls"); - if (cfg->vncTLSx509verify) { - virBufferAddLit(&opt, ",x509verify="); - virQEMUBuildBufferEscapeComma(&opt, cfg->vncTLSx509certdir); + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509)) { + const char *alias = "vnc-tls-creds0"; + if (qemuBuildTLSx509CommandLine(cmd, + cfg->vncTLSx509certdir, + true, + cfg->vncTLSx509verify, + NULL, + alias, + qemuCaps) < 0) + goto error; + + virBufferAsprintf(&opt, ",tls-creds=%s", alias); } else { - virBufferAddLit(&opt, ",x509="); - virQEMUBuildBufferEscapeComma(&opt, cfg->vncTLSx509certdir); + virBufferAddLit(&opt, ",tls"); + if (cfg->vncTLSx509verify) { + virBufferAddLit(&opt, ",x509verify="); + virQEMUBuildBufferEscapeComma(&opt, cfg->vncTLSx509certdir); + } else { + virBufferAddLit(&opt, ",x509="); + virQEMUBuildBufferEscapeComma(&opt, cfg->vncTLSx509certdir); + } } }
So this is better than what we have today, but it is still not comparable with what John did for the other TLS features. Specifically it is missing support for encrypted keys, so we're still broken if the user has editted qemu.conf to set a "default_tls_x509_secret_uuid". We should also have a new "vnc_tls_x509_secret_uuid" to match what's done for chardev and migration.
I'm aware of that, as I said in the bugzilla comment: https://bugzilla.redhat.com/show_bug.cgi?id=1598167#c1
Do you consider the lack of this feature a blocker for this patch aiming to prevent a regression when QEMU stops accepting the old syntax?
Even today if you use an encrypted key for default_tls_x509_dir the VNC server config will be broken, so it isn't a regression, and thus not a blocker. I should have made it clearer in the BZ though, that the intention was to get parity for TLS config across all servers, so that we both avoid the deprecated feature & fix the existing bug wrt encrypted TLS keys. Perhaps better file a separate BZ for the latter now. 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 :|