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
