Re: [Qemu-devel] [PATCH 08/10] ui: convert VNC websockets to use crypto APIs

[Gonglei <arei.gonglei@huawei.com>]

On 2015/5/21 18:56, Daniel P. Berrange wrote:
> Remove the direct use of gnutls for hash processing in the
> websockets code, in favour of using the crypto APIs. This
> allows the websockets code to be built unconditionally
> removing countless conditional checks from the VNC code.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  configure        | 19 +--------------
>  ui/Makefile.objs |  2 +-
>  ui/vnc-ws.c      | 22 ++++++++----------
>  ui/vnc-ws.h      |  2 --
>  ui/vnc.c         | 70 ++++++++++++--------------------------------------------
>  ui/vnc.h         |  8 -------
>  6 files changed, 26 insertions(+), 97 deletions(-)
> 
> diff --git a/configure b/configure
> index cc60f0b..661e7be 100755
> --- a/configure
> +++ b/configure
> @@ -245,7 +245,6 @@ vnc_tls=""
>  vnc_sasl=""
>  vnc_jpeg=""
>  vnc_png=""
> -vnc_ws=""
>  xen=""
>  xen_ctrl_version=""
>  xen_pci_passthrough=""
> @@ -887,10 +886,6 @@ for opt do
>    ;;
>    --enable-vnc-png) vnc_png="yes"
>    ;;
> -  --disable-vnc-ws) vnc_ws="no"
> -  ;;
> -  --enable-vnc-ws) vnc_ws="yes"
> -  ;;
>    --disable-slirp) slirp="no"
>    ;;
>    --disable-uuid) uuid="no"
> @@ -2378,7 +2373,7 @@ fi
>  
>  ##########################################
>  # VNC TLS/WS detection
> -if test "$vnc" = "yes" -a \( "$vnc_tls" != "no" -o "$vnc_ws" != "no" \) ; then
> +if test "$vnc" = "yes" -a "$vnc_tls" != "no" ; then
>    cat > $TMPC <<EOF
>  #include <gnutls/gnutls.h>
>  int main(void) { gnutls_session_t s; gnutls_init(&s, GNUTLS_SERVER); return 0; }
> @@ -2389,20 +2384,13 @@ EOF
>      if test "$vnc_tls" != "no" ; then
>        vnc_tls=yes
>      fi
> -    if test "$vnc_ws" != "no" ; then
> -      vnc_ws=yes
> -    fi
>      libs_softmmu="$vnc_tls_libs $libs_softmmu"
>      QEMU_CFLAGS="$QEMU_CFLAGS $vnc_tls_cflags"
>    else
>      if test "$vnc_tls" = "yes" ; then
>        feature_not_found "vnc-tls" "Install gnutls devel"
>      fi
> -    if test "$vnc_ws" = "yes" ; then
> -      feature_not_found "vnc-ws" "Install gnutls devel"
> -    fi
>      vnc_tls=no
> -    vnc_ws=no
>    fi
>  fi
>  
> @@ -4465,7 +4453,6 @@ if test "$vnc" = "yes" ; then
>      echo "VNC SASL support  $vnc_sasl"
>      echo "VNC JPEG support  $vnc_jpeg"
>      echo "VNC PNG support   $vnc_png"
> -    echo "VNC WS support    $vnc_ws"
>  fi
>  if test -n "$sparc_cpu"; then
>      echo "Target Sparc Arch $sparc_cpu"
> @@ -4671,10 +4658,6 @@ fi
>  if test "$vnc_png" = "yes" ; then
>    echo "CONFIG_VNC_PNG=y" >> $config_host_mak
>  fi
> -if test "$vnc_ws" = "yes" ; then
> -  echo "CONFIG_VNC_WS=y" >> $config_host_mak
> -  echo "VNC_WS_CFLAGS=$vnc_ws_cflags" >> $config_host_mak
> -fi
>  if test "$fnmatch" = "yes" ; then
>    echo "CONFIG_FNMATCH=y" >> $config_host_mak
>  fi
> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> index d9796b1..7e70d55 100644
> --- a/ui/Makefile.objs
> +++ b/ui/Makefile.objs
> @@ -4,7 +4,7 @@ vnc-obj-y += vnc-enc-tight.o vnc-palette.o
>  vnc-obj-y += vnc-enc-zrle.o
>  vnc-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
>  vnc-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
> -vnc-obj-$(CONFIG_VNC_WS) += vnc-ws.o
> +vnc-obj-y += vnc-ws.o
>  vnc-obj-y += vnc-jobs.o
>  
>  common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o
> diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
> index 38a1b8b..215b9d6 100644
> --- a/ui/vnc-ws.c
> +++ b/ui/vnc-ws.c
> @@ -20,6 +20,7 @@
>  
>  #include "vnc.h"
>  #include "qemu/main-loop.h"
> +#include "crypto/hash.h"
>  
>  #ifdef CONFIG_VNC_TLS
>  #include "qemu/sockets.h"
> @@ -203,24 +204,21 @@ static char *vncws_extract_handshake_entry(const char *handshake,
>  static void vncws_send_handshake_response(VncState *vs, const char* key)
>  {
>      char combined_key[WS_CLIENT_KEY_LEN + WS_GUID_LEN + 1];
> -    unsigned char hash[SHA1_DIGEST_LEN];
> -    size_t hash_size = sizeof(hash);
>      char *accept = NULL, *response = NULL;
> -    gnutls_datum_t in;
> -    int ret;
> +    Error *err = NULL;
>  
>      g_strlcpy(combined_key, key, WS_CLIENT_KEY_LEN + 1);
>      g_strlcat(combined_key, WS_GUID, WS_CLIENT_KEY_LEN + WS_GUID_LEN + 1);
>  
>      /* hash and encode it */
> -    in.data = (void *)combined_key;
> -    in.size = WS_CLIENT_KEY_LEN + WS_GUID_LEN;
> -    ret = gnutls_fingerprint(GNUTLS_DIG_SHA1, &in, hash, &hash_size);
> -    if (ret == GNUTLS_E_SUCCESS && hash_size <= SHA1_DIGEST_LEN) {
> -        accept = g_base64_encode(hash, hash_size);
> -    }
> -    if (accept == NULL) {
> -        VNC_DEBUG("Hashing Websocket combined key failed\n");
> +    if (qcrypto_hash_base64(QCRYPTO_HASH_ALG_SHA1,
> +                            combined_key,
> +                            WS_CLIENT_KEY_LEN + WS_GUID_LEN,
> +                            &accept,
> +                            &err) < 0) {
> +        VNC_DEBUG("Hashing Websocket combined key failed %s\n",
> +                  error_get_pretty(err));
> +        error_free(err);
>          vnc_client_error(vs);
>          return;
>      }
> diff --git a/ui/vnc-ws.h b/ui/vnc-ws.h
> index 14d4230..9494225 100644
> --- a/ui/vnc-ws.h
> +++ b/ui/vnc-ws.h
> @@ -21,8 +21,6 @@
>  #ifndef __QEMU_UI_VNC_WS_H
>  #define __QEMU_UI_VNC_WS_H
>  
> -#include <gnutls/gnutls.h>
> -
>  #define B64LEN(__x) (((__x + 2) / 3) * 12 / 3)
>  #define SHA1_DIGEST_LEN 20
>  
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 2b80160..73a6d5f 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -38,6 +38,7 @@
>  #include "qemu/osdep.h"
>  #include "ui/input.h"
>  #include "qapi-event.h"
> +#include "crypto/hash.h"
>  
>  #define VNC_REFRESH_INTERVAL_BASE GUI_REFRESH_INTERVAL_DEFAULT
>  #define VNC_REFRESH_INTERVAL_INC  50
> @@ -353,9 +354,7 @@ static VncClientInfo *qmp_query_vnc_client(const VncState *client)
>      info->base->host = g_strdup(host);
>      info->base->service = g_strdup(serv);
>      info->base->family = inet_netfamily(sa.ss_family);
> -#ifdef CONFIG_VNC_WS
>      info->base->websocket = client->websocket;
> -#endif
>  
>  #ifdef CONFIG_VNC_TLS
>      if (client->tls.session && client->tls.dname) {
> @@ -580,12 +579,10 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
>              info->server = qmp_query_server_entry(vd->lsock, false,
>                                                    info->server);
>          }
> -#ifdef CONFIG_VNC_WS
>          if (vd->lwebsock != -1) {
>              info->server = qmp_query_server_entry(vd->lwebsock, true,
>                                                    info->server);
>          }
> -#endif
>  
>          item = g_new0(VncInfo2List, 1);
>          item->value = info;
> @@ -1229,10 +1226,8 @@ void vnc_disconnect_finish(VncState *vs)
>  
>      buffer_free(&vs->input);
>      buffer_free(&vs->output);
> -#ifdef CONFIG_VNC_WS
>      buffer_free(&vs->ws_input);
>      buffer_free(&vs->ws_output);
> -#endif /* CONFIG_VNC_WS */
>  
>      qapi_free_VncClientInfo(vs->info);
>  
> @@ -1411,12 +1406,9 @@ static void vnc_client_write_locked(void *opaque)
>      } else
>  #endif /* CONFIG_VNC_SASL */
>      {
> -#ifdef CONFIG_VNC_WS
>          if (vs->encode_ws) {
>              vnc_client_write_ws(vs);
> -        } else
> -#endif /* CONFIG_VNC_WS */
> -        {
> +        } else {
>              vnc_client_write_plain(vs);
>          }
>      }
> @@ -1427,11 +1419,7 @@ void vnc_client_write(void *opaque)
>      VncState *vs = opaque;
>  
>      vnc_lock_output(vs);
> -    if (vs->output.offset
> -#ifdef CONFIG_VNC_WS
> -            || vs->ws_output.offset
> -#endif
> -            ) {
> +    if (vs->output.offset || vs->ws_output.offset) {
>          vnc_client_write_locked(opaque);
>      } else if (vs->csock != -1) {
>          qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
> @@ -1537,7 +1525,6 @@ void vnc_client_read(void *opaque)
>          ret = vnc_client_read_sasl(vs);
>      else
>  #endif /* CONFIG_VNC_SASL */
> -#ifdef CONFIG_VNC_WS
>          if (vs->encode_ws) {
>              ret = vnc_client_read_ws(vs);
>              if (ret == -1) {
> @@ -1547,10 +1534,8 @@ void vnc_client_read(void *opaque)
>                  vnc_client_error(vs);
>                  return;
>              }
> -        } else
> -#endif /* CONFIG_VNC_WS */
> -        {
> -        ret = vnc_client_read_plain(vs);
> +        } else {
> +            ret = vnc_client_read_plain(vs);
>          }
>      if (!ret) {
>          if (vs->csock == -1)
> @@ -1622,11 +1607,8 @@ void vnc_write_u8(VncState *vs, uint8_t value)
>  void vnc_flush(VncState *vs)
>  {
>      vnc_lock_output(vs);
> -    if (vs->csock != -1 && (vs->output.offset
> -#ifdef CONFIG_VNC_WS
> -                || vs->ws_output.offset
> -#endif
> -                )) {
> +    if (vs->csock != -1 && (vs->output.offset ||
> +                            vs->ws_output.offset)) {
>          vnc_client_write_locked(vs);
>      }
>      vnc_unlock_output(vs);
> @@ -3017,7 +2999,6 @@ static void vnc_connect(VncDisplay *vd, int csock,
>      VNC_DEBUG("New client on socket %d\n", csock);
>      update_displaychangelistener(&vd->dcl, VNC_REFRESH_INTERVAL_BASE);
>      qemu_set_nonblock(vs->csock);
> -#ifdef CONFIG_VNC_WS
>      if (websocket) {
>          vs->websocket = 1;
>  #ifdef CONFIG_VNC_TLS
> @@ -3030,9 +3011,7 @@ static void vnc_connect(VncDisplay *vd, int csock,
>              qemu_set_fd_handler2(vs->csock, NULL, vncws_handshake_read,
>                                   NULL, vs);
>          }
> -    } else
> -#endif /* CONFIG_VNC_WS */
> -    {
> +    } else {
>          qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
>      }
>  
> @@ -3040,10 +3019,7 @@ static void vnc_connect(VncDisplay *vd, int csock,
>      vnc_qmp_event(vs, QAPI_EVENT_VNC_CONNECTED);
>      vnc_set_share_mode(vs, VNC_SHARE_MODE_CONNECTING);
>  
> -#ifdef CONFIG_VNC_WS
> -    if (!vs->websocket)
> -#endif
> -    {
> +    if (!vs->websocket) {
>          vnc_init_state(vs);
>      }
>  
> @@ -3099,12 +3075,9 @@ static void vnc_listen_read(void *opaque, bool websocket)
>  
>      /* Catch-up */
>      graphic_hw_update(vs->dcl.con);
> -#ifdef CONFIG_VNC_WS
>      if (websocket) {
>          csock = qemu_accept(vs->lwebsock, (struct sockaddr *)&addr, &addrlen);
> -    } else
> -#endif /* CONFIG_VNC_WS */
> -    {
> +    } else {
>          csock = qemu_accept(vs->lsock, (struct sockaddr *)&addr, &addrlen);
>      }
>  
> @@ -3119,12 +3092,10 @@ static void vnc_listen_regular_read(void *opaque)
>      vnc_listen_read(opaque, false);
>  }
>  
> -#ifdef CONFIG_VNC_WS
>  static void vnc_listen_websocket_read(void *opaque)
>  {
>      vnc_listen_read(opaque, true);
>  }
> -#endif /* CONFIG_VNC_WS */
>  
>  static const DisplayChangeListenerOps dcl_ops = {
>      .dpy_name             = "vnc",
> @@ -3150,9 +3121,7 @@ void vnc_display_init(const char *id)
>      QTAILQ_INSERT_TAIL(&vnc_displays, vs, next);
>  
>      vs->lsock = -1;
> -#ifdef CONFIG_VNC_WS
>      vs->lwebsock = -1;
> -#endif
>  
>      QTAILQ_INIT(&vs->clients);
>      vs->expires = TIME_MAX;
> @@ -3186,14 +3155,12 @@ static void vnc_display_close(VncDisplay *vs)
>          close(vs->lsock);
>          vs->lsock = -1;
>      }
> -#ifdef CONFIG_VNC_WS
>      vs->ws_enabled = false;
>      if (vs->lwebsock != -1) {
>          qemu_set_fd_handler2(vs->lwebsock, NULL, NULL, NULL, NULL);
>          close(vs->lwebsock);
>          vs->lwebsock = -1;
>      }
> -#endif /* CONFIG_VNC_WS */
>      vs->auth = VNC_AUTH_INVALID;
>      vs->subauth = VNC_AUTH_INVALID;
>  #ifdef CONFIG_VNC_TLS
> @@ -3579,13 +3546,12 @@ void vnc_display_open(const char *id, Error **errp)
>  
>      websocket = qemu_opt_get(opts, "websocket");
>      if (websocket) {
> -#ifdef CONFIG_VNC_WS
>          vs->ws_enabled = true;
>          qemu_opt_set(wsopts, "port", websocket, &error_abort);
> -#else /* ! CONFIG_VNC_WS */
> -        error_setg(errp, "Websockets protocol requires gnutls support");
> -        goto fail;
> -#endif /* ! CONFIG_VNC_WS */
> +        if (!qcrypto_hash_supports(QCRYPTO_HASH_ALG_SHA1)) {
> +            error_setg(errp, "SHA1 hash support is required for websockets");
> +            goto fail;
> +        }
>      }
>  
>  #ifdef CONFIG_VNC_JPEG
> @@ -3668,9 +3634,7 @@ void vnc_display_open(const char *id, Error **errp)
>          /* connect to viewer */
>          int csock;
>          vs->lsock = -1;
> -#ifdef CONFIG_VNC_WS
>          vs->lwebsock = -1;
> -#endif
>          if (strncmp(vnc, "unix:", 5) == 0) {
>              csock = unix_connect(vnc+5, errp);
>          } else {
> @@ -3693,7 +3657,6 @@ void vnc_display_open(const char *id, Error **errp)
>              if (vs->lsock < 0) {
>                  goto fail;
>              }
> -#ifdef CONFIG_VNC_WS
>              if (vs->ws_enabled) {
>                  vs->lwebsock = inet_listen_opts(wsopts, 0, errp);
>                  if (vs->lwebsock < 0) {
> @@ -3704,17 +3667,14 @@ void vnc_display_open(const char *id, Error **errp)
>                      goto fail;
>                  }
>              }
> -#endif /* CONFIG_VNC_WS */
>          }
>          vs->enabled = true;
>          qemu_set_fd_handler2(vs->lsock, NULL,
>                  vnc_listen_regular_read, NULL, vs);
> -#ifdef CONFIG_VNC_WS
>          if (vs->ws_enabled) {
>              qemu_set_fd_handler2(vs->lwebsock, NULL,
>                      vnc_listen_websocket_read, NULL, vs);
>          }
> -#endif /* CONFIG_VNC_WS */
>      }
>      qemu_opts_del(sopts);
>      qemu_opts_del(wsopts);
> @@ -3724,9 +3684,7 @@ fail:
>      qemu_opts_del(sopts);
>      qemu_opts_del(wsopts);
>      vs->enabled = false;
> -#ifdef CONFIG_VNC_WS
>      vs->ws_enabled = false;
> -#endif /* CONFIG_VNC_WS */
>  }
>  
>  void vnc_display_add_client(const char *id, int csock, bool skipauth)
> diff --git a/ui/vnc.h b/ui/vnc.h
> index 3f7c6a9..814d720 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -108,9 +108,7 @@ typedef struct VncDisplay VncDisplay;
>  #ifdef CONFIG_VNC_SASL
>  #include "vnc-auth-sasl.h"
>  #endif
> -#ifdef CONFIG_VNC_WS
>  #include "vnc-ws.h"
> -#endif
>  
>  struct VncRectStat
>  {
> @@ -156,10 +154,8 @@ struct VncDisplay
>      int connections_limit;
>      VncSharePolicy share_policy;
>      int lsock;
> -#ifdef CONFIG_VNC_WS
>      int lwebsock;
>      bool ws_enabled;
> -#endif
>      DisplaySurface *ds;
>      DisplayChangeListener dcl;
>      kbd_layout_t *kbd_layout;
> @@ -294,21 +290,17 @@ struct VncState
>  #ifdef CONFIG_VNC_SASL
>      VncStateSASL sasl;
>  #endif
> -#ifdef CONFIG_VNC_WS
>      bool encode_ws;
>      bool websocket;
> -#endif /* CONFIG_VNC_WS */
>  
>      VncClientInfo *info;
>  
>      Buffer output;
>      Buffer input;
> -#ifdef CONFIG_VNC_WS
>      Buffer ws_input;
>      Buffer ws_output;
>      size_t ws_payload_remain;
>      WsMask ws_payload_mask;
> -#endif
>      /* current output mode information */
>      VncWritePixels *write_pixels;
>      PixelFormat client_pf;
> 

It's more clear now :)

Regards,
-Gonglei