From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58762) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWnAe-0002hV-Qp for qemu-devel@nongnu.org; Tue, 01 Sep 2015 11:08:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWnAZ-00058x-0G for qemu-devel@nongnu.org; Tue, 01 Sep 2015 11:08:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42598) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWnAY-00058r-Pl for qemu-devel@nongnu.org; Tue, 01 Sep 2015 11:08:18 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 8388A341AC2 for ; Tue, 1 Sep 2015 15:08:17 +0000 (UTC) References: <1440601524-30316-1-git-send-email-berrange@redhat.com> <1440601524-30316-10-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <55E5BF5B.3040801@redhat.com> Date: Tue, 1 Sep 2015 09:08:11 -0600 MIME-Version: 1.0 In-Reply-To: <1440601524-30316-10-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="7MMJ82VwqsdmAiCR6kr4JsfL25FShXS2j" Subject: Re: [Qemu-devel] [PATCH v5 9/9] ui: convert VNC server to use QCryptoTLSSession List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Paolo Bonzini , Gerd Hoffmann This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --7MMJ82VwqsdmAiCR6kr4JsfL25FShXS2j Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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? ... >=20 > 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. >=20 > 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. =2E..Ah, I should have just read further :) >=20 > Signed-off-by: Daniel P. Berrange > --- > +++ b/qemu-options.hx > @@ -1214,8 +1214,9 @@ By definition the Websocket port is 5700+@var{dis= play}. 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}=3D@var{port}. > -TLS encryption for the Websocket connection is supported if the requir= ed > -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 connect= ion 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. > =20 > +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; > } > =20 > -#ifdef CONFIG_VNC_TLS > /* Inform SASL that we've got an external SSF layer from TLS/x509 = */ > if (vs->auth =3D=3D VNC_AUTH_VENCRYPT && > vs->subauth =3D=3D VNC_AUTH_VENCRYPT_X509SASL) { > - gnutls_cipher_algorithm_t cipher; > + Error *local_err =3D NULL; > + int keysize; > sasl_ssf_t ssf; > =20 > - cipher =3D gnutls_cipher_get(vs->tls.session); > - if (!(ssf =3D (sasl_ssf_t)gnutls_cipher_get_key_size(cipher)))= { > - VNC_DEBUG("%s", "cannot TLS get cipher size\n"); > + keysize =3D 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 =3D NULL; > goto authabort; > } > - ssf *=3D 8; /* tls key size is bytes, sasl wants bits */ > + ssf =3D keysize * 8; /* tls key size is bytes, sasl wants bits= */ Worth using the standard CHAR_BITS here rather than a magic number? > =20 > err =3D sasl_setprop(vs->sasl.conn, SASL_SSF_EXTERNAL, &ssf); > if (err !=3D SASL_OK) { > @@ -549,20 +552,19 @@ void start_auth_sasl(VncState *vs) > vs->sasl.conn =3D NULL; > goto authabort; > } > - } else > -#endif /* CONFIG_VNC_TLS */ > + } else { > vs->sasl.wantSSF =3D 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); > =20 > static int vnc_start_vencrypt_handshake(VncState *vs) > { > - int ret; > - > - if ((ret =3D 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, NU= LL, 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; > + } > =20 > - start_auth_vencrypt_subauth(vs); > + return 0; > =20 > + error: > + VNC_DEBUG("Handshake failed %s\n", error_get_pretty(err)); > + error_free(err); > + vnc_client_error(vs); > return 0; =2E..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, v= s); > + break; > } > =20 > - VNC_DEBUG("Handshake done, switching to TLS data mode\n"); > - qemu_set_fd_handler(vs->csock, vncws_handshake_read, NULL, vs); > + return 0; > =20 > + 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 =3D { > }; > =20 > =20 > -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 =3D g_strdup_printf("tlsvnc%s", id); > + Object *parent =3D object_get_objects_root(); > + Object *creds; > + > + if (x509) { > + creds =3D object_new_with_props(TYPE_QCRYPTO_TLS_CREDS_X509, > + parent, > + credsid, > + errp, > + "endpoint", "server", > + "dir", dir, > + "verify-peer", x509verify ? "yes= " : "no", > + NULL); > + } else { > + creds =3D 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --7MMJ82VwqsdmAiCR6kr4JsfL25FShXS2j Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJV5b9bAAoJEKeha0olJ0NqQYYIAK8rSz0Uqs5jFcmqeqE8o0BD LbIE/xqtrtPgIyi6fm8e6/8b2Mf1f1wRyALNM+979QnKzqwGFRF7pNcTfXfvOIq+ by1cS5lSYdaYAKYRkFzX9ra4enHRURmBUqqBV/htKVMr1K5XkUIuS/5nGgqHXzdz iRs4hKIR+DFXLf8kBpfwsh2DPfUBbRISblsX6oXmCcJpjl2SJRm4gjWTPwVNAOLX d3/kUNtUzir/qmQh9NCG6nRU9sg51ap6c1rkn3NICRvji+WxkiRDsc+w9sRV0nxk PhAhQt0bAKDTzD/7StplFwJ83IE705lH1SKXqfuqfkzTy2TqIrBAx0Cb+Qq4Avw= =BMCz -----END PGP SIGNATURE----- --7MMJ82VwqsdmAiCR6kr4JsfL25FShXS2j--