On 08/26/2015 09:05 AM, Daniel P. Berrange wrote: > Switch VNC server over to using the QCryptoTLSSession object > for the TLS session. This removes the direct use of gnutls > from the VNC server code. It also removes most knowledge > about TLS certificate handling from the VNC server code. > This has the nice effect that all the CONFIG_VNC_TLS > conditionals go away and the user gets an actual error > message when requesting TLS instead of it being silently > ignored. > > With this change, the existing configuration options for > enabling TLS with -vnc are deprecated.
We don't want to disable the old way right away; does it issue a warning on the command line, or does it silently just act as sugar for the correct new way? ... > > This aligns VNC with the way TLS credentials are to be > configured in the future for chardev, nbd and migration > backends. It also has the benefit that the same TLS > credentials can be shared across multiple VNC server > instances, if desired. > > If someone uses the deprecated syntax, it will internally > result in the creation of a 'tls-creds' object with an ID > based on the VNC server ID. This allows backwards compat > with the CLI syntax, while still deleting all the original > TLS code from the VNC server. ...Ah, I should have just read further :) > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > --- > +++ b/qemu-options.hx > @@ -1214,8 +1214,9 @@ By definition the Websocket port is 5700+@var{display}. > If @var{host} is > specified connections will only be allowed from this host. > As an alternative the Websocket port could be specified by using > @code{websocket}=@var{port}. > -TLS encryption for the Websocket connection is supported if the required > -certificates are specified with the VNC option @option{x509}. > +If no TLS credentials are provided, the websocket connection runs in > +uncrypted mode. If TLS credentials are provided, the websocket connection s/uncrypted/unencrypted/ > @@ -1243,6 +1258,9 @@ uses anonymous TLS credentials so is susceptible to a > man-in-the-middle > attack. It is recommended that this option be combined with either the > @option{x509} or @option{x509verify} options. > > +This option is now deprecated in favour of using the @option{tls-creds} > +argument. I don't know if US or UK spellings have precedence in the user-facing documentation. > +++ b/ui/vnc-auth-sasl.c > @@ -525,21 +525,24 @@ void start_auth_sasl(VncState *vs) > goto authabort; > } > > -#ifdef CONFIG_VNC_TLS > /* Inform SASL that we've got an external SSF layer from TLS/x509 */ > if (vs->auth == VNC_AUTH_VENCRYPT && > vs->subauth == VNC_AUTH_VENCRYPT_X509SASL) { > - gnutls_cipher_algorithm_t cipher; > + Error *local_err = NULL; > + int keysize; > sasl_ssf_t ssf; > > - cipher = gnutls_cipher_get(vs->tls.session); > - if (!(ssf = (sasl_ssf_t)gnutls_cipher_get_key_size(cipher))) { > - VNC_DEBUG("%s", "cannot TLS get cipher size\n"); > + keysize = qcrypto_tls_session_get_key_size(vs->tls, > + &local_err); > + if (keysize < 0) { > + VNC_DEBUG("cannot TLS get cipher size: %s\n", > + error_get_pretty(local_err)); > + error_free(local_err); > sasl_dispose(&vs->sasl.conn); > vs->sasl.conn = NULL; > goto authabort; > } > - ssf *= 8; /* tls key size is bytes, sasl wants bits */ > + ssf = keysize * 8; /* tls key size is bytes, sasl wants bits */ Worth using the standard CHAR_BITS here rather than a magic number? > > err = sasl_setprop(vs->sasl.conn, SASL_SSF_EXTERNAL, &ssf); > if (err != SASL_OK) { > @@ -549,20 +552,19 @@ void start_auth_sasl(VncState *vs) > vs->sasl.conn = NULL; > goto authabort; > } > - } else > -#endif /* CONFIG_VNC_TLS */ > + } else { > vs->sasl.wantSSF = 1; > + } Not needed for this patch, but a followup patch that converts wantSSF to bool might be nice. > +++ b/ui/vnc-auth-vencrypt.c > @@ -67,37 +67,41 @@ static void vnc_tls_handshake_io(void *opaque); > > static int vnc_start_vencrypt_handshake(VncState *vs) > { > - int ret; > - > - if ((ret = gnutls_handshake(vs->tls.session)) < 0) { > - if (!gnutls_error_is_fatal(ret)) { > - VNC_DEBUG("Handshake interrupted (blocking)\n"); > - if (!gnutls_record_get_direction(vs->tls.session)) > - qemu_set_fd_handler(vs->csock, vnc_tls_handshake_io, NULL, > vs); > - else > - qemu_set_fd_handler(vs->csock, NULL, vnc_tls_handshake_io, > vs); > - return 0; > - } > - VNC_DEBUG("Handshake failed %s\n", gnutls_strerror(ret)); > - vnc_client_error(vs); > - return -1; Old code returns -1 on failure... > + case QCRYPTO_TLS_HANDSHAKE_SENDING: > + VNC_DEBUG("Handshake interrupted (blocking write)\n"); > + qemu_set_fd_handler(vs->csock, NULL, vnc_tls_handshake_io, vs); > + break; > + } > > - start_auth_vencrypt_subauth(vs); > + return 0; > > + error: > + VNC_DEBUG("Handshake failed %s\n", error_get_pretty(err)); > + error_free(err); > + vnc_client_error(vs); > return 0; ...new code does not. Oops. > +++ b/ui/vnc-ws.c > static int vncws_start_tls_handshake(VncState *vs) > { > - } > - VNC_DEBUG("Handshake failed %s\n", gnutls_strerror(ret)); > - vnc_client_error(vs); > - return -1; > + case QCRYPTO_TLS_HANDSHAKE_SENDING: > + VNC_DEBUG("Handshake interrupted (blocking write)\n"); > + qemu_set_fd_handler(vs->csock, NULL, vncws_tls_handshake_io, vs); > + break; > } > > - VNC_DEBUG("Handshake done, switching to TLS data mode\n"); > - qemu_set_fd_handler(vs->csock, vncws_handshake_read, NULL, vs); > + return 0; > > + error: > + VNC_DEBUG("Handshake failed %s\n", error_get_pretty(err)); > + error_free(err); > + vnc_client_error(vs); > return 0; And again. > +++ b/ui/vnc.c > @@ -3301,13 +3303,12 @@ static QemuOptsList qemu_vnc_opts = { > }; > > > -static void > +static int > vnc_display_setup_auth(VncDisplay *vs, > bool password, > bool sasl, > - bool tls, > - bool x509, > - bool websocket) > + bool websocket, > + Error **errp) > { Adding a return value and an errp pointer? Can't callers just check whether errp was set, and then you can leave this as returning void? > +/* > + * Handle back compat with old CLI syntax by creating some > + * suitable QCryptoTLSCreds objects > + */ > +static QCryptoTLSCreds * > +vnc_display_create_creds(bool x509, > + bool x509verify, > + const char *dir, > + const char *id, > + Error **errp) > +{ > + gchar *credsid = g_strdup_printf("tlsvnc%s", id); > + Object *parent = object_get_objects_root(); > + Object *creds; > + > + if (x509) { > + creds = object_new_with_props(TYPE_QCRYPTO_TLS_CREDS_X509, > + parent, > + credsid, > + errp, > + "endpoint", "server", > + "dir", dir, > + "verify-peer", x509verify ? "yes" : > "no", > + NULL); > + } else { > + creds = object_new_with_props(TYPE_QCRYPTO_TLS_CREDS_ANON, > + parent, > + credsid, > + errp, > + "endpoint", "server", > + NULL); > + } > + > + g_free(credsid); > + > + if (*errp) { Won't work. Caller might pass in NULL to ignore the error, in which case you will segfault. You need a local error object coupled with error_propagate() if you are going to make control flow decisions on whether an error occurs in object_new_with_props(). Overall, looks like it is mostly a sane upgrade to use the new framework, and good validation that the earlier patches in the series provide a sane framework. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature