Re: [libvirt] [PATCH 4/4] qemu: vnc: switch to tls-creds-x509

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(a)redhat.com&gt; > > > > 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(a)redhat.com&gt; > > --- > > 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?