[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] PATCH: RFC: Support SASL authentication in VNC
|
From: |
Anthony Liguori |
|
Subject: |
Re: [Qemu-devel] PATCH: RFC: Support SASL authentication in VNC |
|
Date: |
Wed, 04 Feb 2009 13:10:59 -0600 |
|
User-agent: |
Thunderbird 2.0.0.19 (X11/20090105) |
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 <gnutls/x509.h>
#endif /* CONFIG_VNC_TLS */
-// #define _VNC_DEBUG 1
+#ifdef CONFIG_VNC_SASL
+#include <sasl/sasl.h>
+#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