From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LUqWw-0003fJ-3e for qemu-devel@nongnu.org; Wed, 04 Feb 2009 17:47:38 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LUqWs-0003f1-NS for qemu-devel@nongnu.org; Wed, 04 Feb 2009 17:47:37 -0500 Received: from [199.232.76.173] (port=35787 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LUqWs-0003ey-ID for qemu-devel@nongnu.org; Wed, 04 Feb 2009 17:47:34 -0500 Received: from mx1.redhat.com ([66.187.233.31]:48581) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LUqWs-0006qV-2W for qemu-devel@nongnu.org; Wed, 04 Feb 2009 17:47:34 -0500 Date: Wed, 4 Feb 2009 22:47:31 +0000 From: "Daniel P. Berrange" Subject: Re: [Qemu-devel] PATCH: RFC: Support SASL authentication in VNC Message-ID: <20090204224728.GB8095@redhat.com> References: <20090204171625.GI26946@redhat.com> <4989E843.5000004@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4989E843.5000004@codemonkey.ws> Reply-To: "Daniel P. Berrange" , qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org On Wed, Feb 04, 2009 at 01:10:59PM -0600, Anthony Liguori wrote: > Daniel P. Berrange wrote: > > > >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. > > > >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. I don't particularly like it either, but need some kind of external persistent config for the authorization data - and it needs to be independant of any QEMU machine config file we create, because one authorization list could be shared across many VMs. The only other option I consider was to punt the problem on to the caller, and require them to issue a series of monitor commands to setup the ACL at startup. I don't particularly like that though since it'd just be pushing the problem elsewhere. Suggestions welcome from the audience... > > > 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. I don't think it is desirable to move all the SASL code to an external file - in particular the interaction at the I/O layer I think is important to have in one place to make it clear to read & see the whole picture. I wouldn't mind moving all this metadata into a separate struct though, and just include an instance of that in the main QEMU client struct. Likewise I think its reasonable to move the auth handshake impl methods into a separate file, since they're nicely self-contained and quite large pieces of code. > >-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. It is required actually - you have two layers of buffering here. The vnc_write() methods, still put stuff into vs->output.buffer as now. This buffer cannot expect to go straight onto the wire though. It may need to be passed through the SASL SSF layer, by calling sasl_encode() (or sasl_decode() for recv'd data). For clarity the psuedo-code logical algorithm, is now vnc_client_write() if using SASL call vnc_client_write_sasl else call vnc_client_write_plain vnc_write_plain() call vnc_client_write_wire(vs->output.bufffer) vnc_write_sasl() data = sasl_encode(vs->output.buffer) call vnc_client_write_wire(data) vnc_write_wire() if using SSL call gnutls_write else call send() There are a few complications in the SASL path relating to fact that we have to deal with an intermediate buffer and cannot guarentee that we will be able to write all of this intermediate buffer in one go (if the socket gives EAGAIN). There's also a scenario where we have decided to run SASL, but still have some clear text data to send before actually switching in the encoding routines. The read path basically follows exactly same model / structure. > 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. Well, 4 scenarios, and upto 3 layers - Direct to socket - TLS -> socket - SASL -> socket - SASL -> TLS -> socket NB, the last case of SASL -> TLS -> socket, is ordinarily identical to the TLS -> socket case, because we inform SASL that TLS is providing a encryption layer, and thus sasl_encode/decode will (likely) become a plain pass-through. This is all decided based on the min_ssf/max_ssf stuff we setup during the SASL auth process. We still do the sasl_decode step though, because you never know when someone will invent a new SASL plugin that will want to actually do something here, despite TLS already providing encryption. I don't think a write hooks would be particularly helpful at making this clearer, because SASL vs TLS use quite different approachs to how/where the encryption is done. In SASL case you just have encode and decode methods, and you have to take care of all buffering of data yourself. In TLS case, you have replacement read/write methods and TLS hides all the encoding & decoding from you. I think trying to hide these approaches in a generic wrappers will make it harder to understand exactly what is going on & who's doing it. For TLS, it'd certainly be viable to move the auth handshake methods to separate files since again that stuff is nicely self-contained. I'd just rather have all the I/O code in one place, since that's got subtle interactions between the layers. > > > >+#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. That is a place where I'm not hugely clear on where to draw the line. I definitely need some kind of limit to avoid DOS attack from the client, but how big is big enough ? I figured 1 MB would be sufficient for a Kerberos ticket, unless someone has counter examples from the real world ? > 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. FYI, the way I have structured the SASL / TLS / plain layering of I/O is basically following the exact same model / structure I used in libvirt daemon for doing the same, so I'm fairly confident in the way fits together. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|