From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LUn9f-0005fv-0V for qemu-devel@nongnu.org; Wed, 04 Feb 2009 14:11:23 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LUn9e-0005fS-4k for qemu-devel@nongnu.org; Wed, 04 Feb 2009 14:11:22 -0500 Received: from [199.232.76.173] (port=60040 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LUn9d-0005fD-Sd for qemu-devel@nongnu.org; Wed, 04 Feb 2009 14:11:21 -0500 Received: from mail-qy0-f20.google.com ([209.85.221.20]:56092) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LUn9d-0003x2-AJ for qemu-devel@nongnu.org; Wed, 04 Feb 2009 14:11:21 -0500 Received: by qyk13 with SMTP id 13so5110767qyk.10 for ; Wed, 04 Feb 2009 11:11:21 -0800 (PST) Message-ID: <4989E843.5000004@codemonkey.ws> Date: Wed, 04 Feb 2009 13:10:59 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] PATCH: RFC: Support SASL authentication in VNC References: <20090204171625.GI26946@redhat.com> In-Reply-To: <20090204171625.GI26946@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Hi Dan, Daniel P. Berrange wrote: > Previously I provided patches for QEMU's VNC server to support SSL/TLS > and x509 certificates. This provides good encryption capabilities for > the VNC session. It doesn't really address the authentication problem > though. > > I have been working to create a new authentication type in the RFB > protocol to address this need in a generic, extendable way, by mapping > the SASL API into the RFB protocol. Since SASL is a generic plugin > based API, this will allow use of a huge range of auth mechanims over > VNC, without us having to add any more auth code. For example, PAM, > Digest-MD5, GSSAPI/Kerberos, One-time key/password, LDAP password > lookup, SQL db password lookup, and more. > > I have got a VNC auth type assigned by the RFB spec maintainers: > > http://realvnc.com/pipermail/vnc-list/2008-December/059463.html > > With the full current spec for the SASL extension currently documented > here: > > http://realvnc.com/pipermail/vnc-list/2008-December/059462.html > > This patch provides an initial implementation of this extension for the > QEMU VNC server. I am not requesting it is merged just yet, but I'd like > get it all finished in time of the suggested QEMU release at the end > of the month, so I'm sending the patch out now for early comments > > In most circumstances, it is neccessary to combine use of SASL, with > the TLS + x509 support, since most SASL mechanisms only provide for > authentication. In this case you would launch QEMU with > > qemu ... -vnc localhost:1,tls,x509,sasl > > > The Kerberos GSSAPI mechanism for SASL is unusual in that it can also > provide encryption of the data session (so called SSF layer in SASL > terminology). If using this, QEMU can be launched with just > > qemu ... -vnc localhost:1,sasl > > The actual choice of SASL mechanism is controlled by the SASL service > configuration file. THis patch integrates with Cyrus-SASL library and > thus the config is in /etc/sasl2/qemu.conf. This patch does not yet > support it, but I intend too add support for overriding this config > with $HOME/.sasl2/qemu.conf, since most people use QEMU in their regular > user accounts and may not have access to the system SASL configs. The > updates to qemu-doc.texi show how to config SASL, or the Cyrus-SASL docs > can be referred to. > > As I mentioned, this uses Cyrus-SASL libraries. The configure script > probes for this, and disables SASL support if not found, so it should > not cause problems for places without Cyrus-SASL like Windows. > > The main missing points in this patch > > - Authorization. Once we've authenticated the user, how do we > decide whether they're allow to use VNC. eg, just because a > user has a valid Kerberos principle, does not imply we should > allow access. > > We really need an access control list, listing the allowed > SASL usernames and/or x509 client certificate CNAMEs which > are authorized. This should probably live in an external > file, perhaps allowing for ACLs against multiple different > QEMU network based devices. eg I could just add a arg > > -acl /path/to/aclfile > Not a huge fan of using an acl file but I'm not terribly opposed to it either. I like the monitor commands but obviously, we'll need a mechanism to list existing acls too. > Makefile.target | 5 > configure | 34 ++ > qemu-doc.texi | 94 ++++++ > qemu.sasl | 34 ++ > vnc.c | 800 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > vnc.h | 16 - > 6 files changed, 964 insertions(+), 19 deletions(-) > > Regards, > Daniel > > > Index: Makefile.target > =================================================================== > --- Makefile.target (revision 6511) > +++ Makefile.target (working copy) > @@ -554,6 +554,11 @@ > LIBS += $(CONFIG_VNC_TLS_LIBS) > endif > > +ifdef CONFIG_VNC_SASL > +CPPFLAGS += $(CONFIG_VNC_SASL_CFLAGS) > +LIBS += $(CONFIG_VNC_SASL_LIBS) > +endif > + > ifdef CONFIG_BLUEZ > LIBS += $(CONFIG_BLUEZ_LIBS) > endif > Index: vnc.c > =================================================================== > --- vnc.c (revision 6511) > +++ vnc.c (working copy) > @@ -43,8 +43,12 @@ > #include > #endif /* CONFIG_VNC_TLS */ > > -// #define _VNC_DEBUG 1 > +#ifdef CONFIG_VNC_SASL > +#include > +#endif > > +#define _VNC_DEBUG 1 > + > #ifdef _VNC_DEBUG > #define VNC_DEBUG(fmt, ...) do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) > > @@ -123,6 +127,32 @@ > char *x509cert; > char *x509key; > #endif > + > +#ifdef CONFIG_VNC_SASL > + sasl_conn_t *saslconn; > + /* If we want to negotiate an SSF layer with client */ > + int saslWantSSF :1; > + /* If we are now running the SSF layer */ > + int saslRunSSF :1; > + /* > + * If this is non-zero, then wait for that many bytes > + * to be written plain, before switching to SSF encoding > + * This allows the VNC auth result to finish being > + * written in plain. > + */ > + unsigned int saslWaitWriteSSF; > + > + /* > + * Buffering encoded data to allow more clear data > + * to be stuffed onto the output buffer > + */ > + const uint8_t *saslEncoded; > + unsigned int saslEncodedLength; > + unsigned int saslEncodedOffset; > + char *saslUsername; > + char *saslMechlist; > +#endif > I think we could use a little more love here that moved the sasl bits to a vnc-sasl.c provide that it's a sane thing to do. > + > char challenge[VNC_AUTH_CHALLENGE_SIZE]; > > #ifdef CONFIG_VNC_TLS > @@ -829,6 +859,18 @@ > } > vs->wiremode = VNC_WIREMODE_CLEAR; > #endif /* CONFIG_VNC_TLS */ > +#ifdef CONFIG_VNC_SASL > + if (vs->saslconn) { > + vs->saslRunSSF = vs->saslWaitWriteSSF = vs->saslWantSSF = 0; > + vs->saslEncodedLength = vs->saslEncodedOffset = 0; > + vs->saslEncoded = NULL; > + free(vs->saslUsername); > + free(vs->saslMechlist); > + vs->saslUsername = vs->saslMechlist = NULL; > + sasl_dispose(&vs->saslconn); > + vs->saslconn = NULL; > + } > +#endif /* CONFIG_VNC_SASL */ > audio_del(vs); > return 0; > } > @@ -840,14 +882,12 @@ > vnc_client_io_error(vs, -1, EINVAL); > } > > -static void vnc_client_write(void *opaque) > +static long vnc_client_write_wire(VncState *vs, const uint8_t *data, size_t datalen) > { > long ret; > - VncState *vs = opaque; > - > #ifdef CONFIG_VNC_TLS > if (vs->tls_session) { > - ret = gnutls_write(vs->tls_session, vs->output.buffer, vs->output.offset); > + ret = gnutls_write(vs->tls_session, data, datalen); > if (ret < 0) { > if (ret == GNUTLS_E_AGAIN) > errno = EAGAIN; > @@ -857,35 +897,117 @@ > } > } else > #endif /* CONFIG_VNC_TLS */ > - ret = send(vs->csock, vs->output.buffer, vs->output.offset, 0); > - ret = vnc_client_io_error(vs, ret, socket_error()); > + ret = send(vs->csock, data, datalen, 0); > + VNC_DEBUG("Wrote wire %p %d -> %ld\n", data, datalen, ret); > + return vnc_client_io_error(vs, ret, socket_error()); > +} > These changes seem extraneous. I'll just assume the patch needs cleanup for such things. > +#ifdef CONFIG_VNC_SASL > +static long vnc_client_write_sasl(VncState *vs) > +{ > + long ret; > + > + VNC_DEBUG("Write SASL: Pending output %p size %d offset %d Encoded: %p size %d offset %d\n", > + vs->output.buffer, vs->output.capacity, vs->output.offset, > + vs->saslEncoded, vs->saslEncodedLength, vs->saslEncodedOffset); > + > + if (!vs->saslEncoded) { > + int err; > + err = sasl_encode(vs->saslconn, > + (char *)vs->output.buffer, > + vs->output.offset, > + (const char **)&vs->saslEncoded, > + &vs->saslEncodedLength); > + if (err != SASL_OK) > + return vnc_client_io_error(vs, -1, EIO); > + > + vs->output.offset = 0; > + vs->saslEncodedOffset = 0; > + } > + > + ret = vnc_client_write_wire(vs, > + vs->saslEncoded + vs->saslEncodedOffset, > + vs->saslEncodedLength - vs->saslEncodedOffset); > if (!ret) > - return; > + return 0; > > + vs->saslEncodedOffset += ret; > + if (vs->saslEncodedOffset == vs->saslEncodedLength) { > + vs->saslEncoded = NULL; > + vs->saslEncodedOffset = vs->saslEncodedLength = 0; > + } > + > + /* Can't merge this block with one above, because > + * someone might have written more unencrypted > + * data in vs->output while we were processing > + * SASL encoded output > + */ > + if (vs->output.offset == 0) { > + qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs); > + } > + > + return ret; > +} > +#endif /* CONFIG_VNC_SASL */ > + > +static long vnc_client_write_plain(VncState *vs) > +{ > + long ret; > + > +#ifdef CONFIG_VNC_SASL > + VNC_DEBUG("Write Plain: Pending output %p size %d offset %d. Wait SSF %d\n", > + vs->output.buffer, vs->output.capacity, vs->output.offset, > + vs->saslWaitWriteSSF); > + > + if (vs->saslconn && > + vs->saslRunSSF && > + vs->saslWaitWriteSSF) { > + ret = vnc_client_write_wire(vs, vs->output.buffer, vs->saslWaitWriteSSF); > + if (ret) > + vs->saslWaitWriteSSF -= ret; > + } else > +#endif /* CONFIG_VNC_SASL */ > + ret = vnc_client_write_wire(vs, vs->output.buffer, vs->output.offset); > + if (!ret) > + return 0; > + > memmove(vs->output.buffer, vs->output.buffer + ret, (vs->output.offset - ret)); > vs->output.offset -= ret; > > if (vs->output.offset == 0) { > qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs); > } > + > + return ret; > } > So there are three possible layers of write interaction. SASL -> TLS -> Socket. I think adding a mechanism to register write hooks to implement SASL and TLS would be good. If you can, moving the TLS support into a separate file would be helpful too. > > +#ifdef CONFIG_VNC_SASL > +/* Max amount of data we send/recv for SASL steps to prevent DOS */ > +#define SASL_DATA_MAX_LEN (1024 * 1024) Be careful with this. Kerberos tickets can contain arbitrary data (for instance, the MS PAC). This can make the data transfer size be much larger than you'd expect. I like the general idea here. SASL is a nice mechanism and the implementation is pretty straight forward. I'll do a more thorough review when you get something closer to ready for inclusion. Regards, Anthony Liguori