This patch set implements SASL channel authentication as described in the protocol patch entitled "protocol: add common channel caps for AUTH mechanism selection". SASL is enabled via spice_server_set_sasl(), that a following QEMU patch implements. For testing, current spice-gtk git implements the client side. Changed since v1: - removed the RFC SSL, since it's not doing anything useful - splitted the patch moving to stream_{read,write}() functions - fixed the if {} else {} style - renamed s/RedsSASLContext/RedsSASL
Created attachment 43871 [details] [review] build: make it silent This patch make it easier to spot warnings in compilation. It should work with older versions of automake that don't support silent rules. If you want verbose build, make V=1. Signed-off-by: Uri Lublin <uril@redhat.com>
Created attachment 43872 [details] [review] server/reds: remove unused readv Let's not bother with it since nobody uses it, and it's not implemented for SSL anyway
Created attachment 43873 [details] [review] server: s/RedsStreamContext/RedsStream
Created attachment 43874 [details] [review] server: add reds_stream_{read,write,free,remove_watch}()
Created attachment 43875 [details] [review] server: use reds_stream_remove_watch() helper
Created attachment 43876 [details] [review] server: use reds_{link,stream}_free() Be carefull removing the watch before, like __release_link
Created attachment 43877 [details] [review] server: remove cb_free, not needed anymore
Created attachment 43878 [details] [review] server: use the new reds_stream_{read,write}
Created attachment 43879 [details] [review] server/reds: remove the void* ctx field
Created attachment 43880 [details] [review] server: rename s/peer/stream This is stylish change again. We are talking about a RedStream object, so let's just name the variable "stream" everywhere, to avoid confusion with a non existent RedPeer object.
Created attachment 43881 [details] [review] server/reds: make writev fallback more generic We are going to reuse it for SASL/SSF encode write().
Created attachment 43882 [details] [review] common: add SpiceBuffer - based on qemu-vnc Buffer
Created attachment 43883 [details] [review] build: add --with-sasl Using cyrus SASL library (same as gtk-vnc/qemu).
Created attachment 43884 [details] [review] server: pull out reds_handle_link(), for future reuse + a couple of indent, style change
Created attachment 43885 [details] [review] server: simplify and constify sync_write() + symplify, improving style of code using it.
Created attachment 43886 [details] [review] server: add reds_channel_dispose() Try to have a common base dispose() method for channels. For now, it just free the caps. Make use of it in snd_worker, and in sync_write() - sync_write() is going to have default caps later on.
Created attachment 43887 [details] [review] server: add auth mechanism selection
Created attachment 43888 [details] [review] server: add SASL support We introduce 2 public functions to integrate with the library user. spice_server_set_sasl() - turn on SASL spice_server_set_sasl_appname() - specify the name of the app (It is used for where to find the default configuration file) The patch for QEMU is on its way.
Comment on attachment 43885 [details] [review] server: simplify and constify sync_write() >From 1049e3f5983b8fb502fd2228b10f2bf3b6fb00fc Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@redhat.com> >Date: Fri, 11 Feb 2011 04:00:27 +0100 >Subject: [PATCH] server: simplify and constify sync_write() > >+ symplify, improving style of code using it. > >https://bugs.freedesktop.org/show_bug.cgi?id=34795 >--- > server/reds.c | 31 +++++++++++++++---------------- > 1 files changed, 15 insertions(+), 16 deletions(-) > >diff --git a/server/reds.c b/server/reds.c >index eb1ea44..1e7e318 100644 >--- a/server/reds.c >+++ b/server/reds.c >@@ -1321,9 +1321,9 @@ void reds_on_main_receive_migrate_data(MainMigrateData *data, uint8_t *end) > while (write_to_vdi_port() || read_from_vdi_port()); > } > >-static int sync_write(RedsStream *stream, void *in_buf, size_t n) >+static int sync_write(RedsStream *stream, const void *in_buf, size_t n) > { >- uint8_t *buf = (uint8_t *)in_buf; >+ const uint8_t *buf = (uint8_t *)in_buf; > while (n) { > int now = reds_stream_write(stream, buf, n); > if (now <= 0) { >@@ -1342,6 +1342,7 @@ static int reds_send_link_ack(RedLinkInfo *link) > { > SpiceLinkHeader header; > SpiceLinkReply ack; >+ Channel caps = { 0, }; > Channel *channel; > BUF_MEM *bmBuf; > BIO *bio; >@@ -1354,14 +1355,14 @@ static int reds_send_link_ack(RedLinkInfo *link) > > ack.error = SPICE_LINK_ERR_OK; > >- if ((channel = reds_find_channel(link->link_mess->channel_type, 0))) { >- ack.num_common_caps = channel->num_common_caps; >- ack.num_channel_caps = channel->num_caps; >- header.size += (ack.num_common_caps + ack.num_channel_caps) * sizeof(uint32_t); >- } else { >- ack.num_common_caps = 0; >- ack.num_channel_caps = 0; >+ channel = reds_find_channel(link->link_mess->channel_type, 0); >+ if (!channel) { >+ channel = ∩︀ > } >+ >+ ack.num_common_caps = channel->num_common_caps; >+ ack.num_channel_caps = channel->num_caps; >+ header.size += (ack.num_common_caps + ack.num_channel_caps) * sizeof(uint32_t); > ack.caps_offset = sizeof(SpiceLinkReply); > > if (!(link->tiTicketing.rsa = RSA_new())) { >@@ -1382,13 +1383,11 @@ static int reds_send_link_ack(RedLinkInfo *link) > BIO_get_mem_ptr(bio, &bmBuf); > memcpy(ack.pub_key, bmBuf->data, sizeof(ack.pub_key)); > >- ret = sync_write(link->stream, &header, sizeof(header)) && sync_write(link->stream, &ack, >- sizeof(ack)); >- if (channel) { >- ret = ret && sync_write(link->stream, channel->common_caps, >- channel->num_common_caps * sizeof(uint32_t)) && >- sync_write(link->stream, channel->caps, channel->num_caps * sizeof(uint32_t)); >- } >+ ret = sync_write(link->stream, &header, sizeof(header)); >+ ret &= sync_write(link->stream, &ack, sizeof(ack)); >+ ret &= sync_write(link->stream, channel->common_caps, channel->num_common_caps * sizeof(uint32_t)); >+ ret &= sync_write(link->stream, channel->caps, channel->num_caps * sizeof(uint32_t)); >+ So what about this - in the last round you said you weren't sure if it is good enough? you said it does sync_write's even if the channel is not found - it looks like it still does. > BIO_free(bio); > return ret; > } >-- >1.7.4
(In reply to comment #19) > >- if (channel) { > >- ret = ret && sync_write(link->stream, channel->common_caps, > >- channel->num_common_caps * sizeof(uint32_t)) && > >- sync_write(link->stream, channel->caps, channel->num_caps * sizeof(uint32_t)); > >- } > >+ ret = sync_write(link->stream, &header, sizeof(header)); > >+ ret &= sync_write(link->stream, &ack, sizeof(ack)); > >+ ret &= sync_write(link->stream, channel->common_caps, channel->num_common_caps * sizeof(uint32_t)); > >+ ret &= sync_write(link->stream, channel->caps, channel->num_caps * sizeof(uint32_t)); > >+ > > So what about this - in the last round you said you weren't sure if it is good > enough? you > said it does sync_write's even if the channel is not found - it looks like it > still does. > Yeah, I wasn't sure about it, but I realized that the previous version of the code had channel->num_{,common}_caps == 0. So that's why there was a if(channel) condition. Now, it goes through sync_write anyway, either channel->num_{,common}_caps == 0 (without the AUTH_SELECTION cap, or with it, then channel->num_common_caps == 1) So it looks fine to me. Agree?
ACK to all. (don't forget the SPICE_UNUSED and the fix to the sync_write - please post the updated patches here for future reference).
committed to master (ghhh, I forgot to reorder: build add --with-sasl) commit 8f9cbd19dbf3612abeafae60bdc4df2a97552b91 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Fri Feb 11 04:04:28 2011 +0100 server: add SASL support We introduce 2 public functions to integrate with the library user. spice_server_set_sasl() - turn on SASL spice_server_set_sasl_appname() - specify the name of the app (It is used for where to find the default configuration file) The patch for QEMU is on its way. https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit f4dddc50f06f90d7ff97d4ed60c6197c85814c5e Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Tue Feb 22 13:54:09 2011 +0100 server: add auth mechanism selection https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 4f983b2c9c642ac96abf214cefc1c51fad90791e Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Tue Feb 22 03:52:38 2011 +0100 server: add reds_channel_dispose() Try to have a common base dispose() method for channels. For now, it just free the caps. Make use of it in snd_worker, and in sync_write() - sync_write() is going to have default caps later on. https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 09c01c9516045cfad1a95108f0386e50fcd9dfa0 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Fri Feb 11 04:00:27 2011 +0100 server: simplify and constify sync_write() + symplify, improving style of code using it. https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 37dbb8aec917f02075094d991b471b56f5f0a4a6 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Fri Feb 11 03:57:59 2011 +0100 server: pull out reds_handle_link(), for future reuse + a couple of indent, style change https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 414e1de7207c1c807832836d56f5e301e5cb156f Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Fri Feb 11 03:50:51 2011 +0100 build: add --with-sasl Using cyrus SASL library (same as gtk-vnc/qemu). https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 2bbb4fca0c97043b183fb65126b7198bc0750a78 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Wed Feb 9 15:11:22 2011 +0100 common: add SpiceBuffer - based on qemu-vnc Buffer https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 2ced8996c86605bad91c7d2f73ea5460252b4517 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Thu Feb 10 04:26:00 2011 +0100 server/reds: make writev fallback more generic We are going to reuse it for SASL/SSF encode write(). https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 76dc27f08a15bd2cb0e0f536972e361e6d7acc25 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Wed Feb 9 21:44:45 2011 +0100 server: rename s/peer/stream This is stylish change again. We are talking about a RedStream object, so let's just name the variable "stream" everywhere, to avoid confusion with a non existent RedPeer object. https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 1a4923c2107c89314714718ca1431d232cc23edf Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Thu Feb 10 03:00:57 2011 +0100 server/reds: remove the void* ctx field https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit fc5d7f76257bf51c693dfb888c1a3dd61d8d2318 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Sun Feb 27 13:14:00 2011 +0100 server: use the new reds_stream_{read,write} https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit f32503258fa1f0a88de410005609770bd23cac97 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Sun Feb 27 13:40:52 2011 +0100 server: remove cb_free, not needed anymore https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 07d6dd61080efdfe97fbce7a9a12abb7f315bdc3 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Sun Feb 27 13:33:28 2011 +0100 server: use reds_{link,stream}_free() Be carefull removing the watch before, like __release_link https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit fca602124ded4b3333344a8737a08751635bacdb Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Sun Feb 27 13:31:31 2011 +0100 server: use reds_stream_remove_watch() helper https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit b79e7320de6b2cd07000b9713ec77f3439eaf80d Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Sun Feb 27 09:46:14 2011 +0100 server: add reds_stream_{read,write,free,remove_watch}() https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit d47912241f4bc263cf3c19c07ed3907681826277 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Sun Feb 27 09:38:04 2011 +0100 server: s/RedsStreamContext/RedsStream https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 29be54f6d3549c44b2b771ca3c21952d2d1b7026 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Wed Feb 9 18:01:18 2011 +0100 server/reds: remove unused readv Let's not bother with it since nobody uses it, and it's not implemented for SSL anyway https://bugs.freedesktop.org/show_bug.cgi?id=34795 commit 0e64e2d02c7353ccad8951dfb9b5ec8b03cd59ee Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Tue Nov 9 15:16:41 2010 +0100 build: make it silent This patch make it easier to spot warnings in compilation. It should work with older versions of automake that don't support silent rules. If you want verbose build, make V=1. Signed-off-by: Uri Lublin <uril@redhat.com> https://bugs.freedesktop.org/show_bug.cgi?id=34795
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.