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

[Daniel P. Berrangé]

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