Re: [Qemu-devel] [PATCH v5 9/9] ui: convert VNC server to use QCryptoTLSSession

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8870 bytes --]

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 <berrange@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]